[SSSD] [PATCH] LDAP: Don't add a user member twice when adding a primary group
Jakub Hrozek
jhrozek at redhat.com
Tue Aug 26 12:54:24 UTC 2014
On Tue, Aug 26, 2014 at 02:01:07PM +0200, Pavel Reichl wrote:
>
> On 08/26/2014 11:37 AM, Jakub Hrozek wrote:
> >On Mon, Aug 25, 2014 at 08:18:15PM +0200, Pavel Reichl wrote:
> >>On 08/25/2014 06:21 PM, Jakub Hrozek wrote:
> >>>On Mon, Aug 25, 2014 at 05:23:44PM +0200, Pavel Reichl wrote:
> >>>>Hello,
> >>>>
> >>>>I tested the patch and it fixes the problem described in the ticket.
> >>>>
> >>>>I personally dislike the break and mainly the continue check - I believe
> >>>>that people reading the code would benefit from encapsulating the inner loop
> >>>>into function (return value would indicate what is happening and function
> >>>>name is the best comment anyway) and same would go for the outer loop IMO.
> >>>That's a fair comment. Would you prefer to split the for loop into one
> >>>function or two?
> >>two, but I can live with one :-)
> >OK, see the attached patches. Please note I wasn't able to test them
> >properly because my new AD test servers don't carry POSIX attributes
> >yet. I'll test the patches in a short while, in the meantime, feel free
> >to test in your environment and/or look at the code.
> >
>
> After applying the patches I can't replicate the problem any longer.
>
> Thanks for the code style change I think it's better now.
>
> Just 2 little details, please see inline, but I'm willing to ACK patches as
> are now.
I fixed both. Thanks for the review.
-------------- next part --------------
>From 8164e510221bb63804168b112fab88bc309638b8 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Tue, 26 Aug 2014 11:11:15 +0200
Subject: [PATCH 1/2] LDAP: Split out linking primary group members into a
separate function
The function sdap_fill_memberships did several tasks. It's more readable
to split linking the primary members into a separate function.
---
src/providers/ldap/sdap_async_groups.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c
index a21b0332e0acc111d387cc39fe8009794a4baa43..c8e8932597da232a35c56321b213ef8528a85510 100644
--- a/src/providers/ldap/sdap_async_groups.c
+++ b/src/providers/ldap/sdap_async_groups.c
@@ -191,6 +191,21 @@ sdap_dn_by_primary_gid(TALLOC_CTX *mem_ctx, struct sysdb_attrs *ldap_attrs,
return EOK;
}
+static void link_pgroup_members(struct sysdb_attrs *group_attrs,
+ struct ldb_message_element *member_el,
+ char **userdns,
+ size_t nuserdns)
+{
+ int i;
+
+ for (i=0; i < nuserdns; i++) {
+ member_el->values[member_el->num_values + i].data = (uint8_t *) \
+ talloc_steal(group_attrs, userdns[i]);
+ member_el->values[member_el->num_values + i].length = strlen(userdns[i]);
+ }
+ member_el->num_values += nuserdns;
+}
+
static int sdap_fill_memberships(struct sdap_options *opts,
struct sysdb_attrs *group_attrs,
struct sysdb_ctx *ctx,
@@ -285,13 +300,7 @@ static int sdap_fill_memberships(struct sdap_options *opts,
}
el->num_values = j;
- for (i=0; i < nuserdns; i++) {
- el->values[el->num_values + i].data = (uint8_t *) \
- talloc_steal(group_attrs, userdns[i]);
- el->values[el->num_values + i].length = strlen(userdns[i]);
- }
- el->num_values += nuserdns;
-
+ link_pgroup_members(group_attrs, el, userdns, nuserdns);
ret = EOK;
done:
--
1.9.3
-------------- next part --------------
>From b50a9d87d82f4ddb26aacce5e288bd55a3e1913e Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Tue, 26 Aug 2014 11:23:20 +0200
Subject: [PATCH 2/2] LDAP: Don't add a user member twice when adding a primary
group
https://fedorahosted.org/sssd/ticket/2406
In the AD case, deployments sometimes add groups as parents of the
primary GID group. These groups are then returned during initgroups
in the tokenGroups attribute and member/memberof links are established
between the user and the group. However, any update of these groups
would remove the links, so a sequence of calls: id -G user; id user; id
-G user would return different group memberships.
Our code errored out in the rare case when the user was *also* an LDAP
member of his primary group.
---
src/providers/ldap/sdap_async_groups.c | 38 +++++++++++++++++++++++++++++-----
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c
index c8e8932597da232a35c56321b213ef8528a85510..a82d2aa3418cf5d59181e4be5a1ed6aaeb0b05e9 100644
--- a/src/providers/ldap/sdap_async_groups.c
+++ b/src/providers/ldap/sdap_async_groups.c
@@ -191,19 +191,47 @@ sdap_dn_by_primary_gid(TALLOC_CTX *mem_ctx, struct sysdb_attrs *ldap_attrs,
return EOK;
}
+static bool has_member(struct ldb_message_element *member_el,
+ char *member)
+{
+ struct ldb_val val;
+
+ val.data = (uint8_t *) member;
+ val.length = strlen(member);
+
+ /* This is bad complexity, but the this loop should only be invoked in
+ * the very rare scenario of AD POSIX group that is primary group of
+ * some users but has user member attributes at the same time
+ */
+ if (ldb_msg_find_val(member_el, &val) != NULL) {
+ return true;
+ }
+
+ return false;
+}
+
static void link_pgroup_members(struct sysdb_attrs *group_attrs,
struct ldb_message_element *member_el,
char **userdns,
size_t nuserdns)
{
- int i;
+ int i, j;
+ j = 0;
for (i=0; i < nuserdns; i++) {
- member_el->values[member_el->num_values + i].data = (uint8_t *) \
- talloc_steal(group_attrs, userdns[i]);
- member_el->values[member_el->num_values + i].length = strlen(userdns[i]);
+ if (has_member(member_el, userdns[i])) {
+ DEBUG(SSSDBG_TRACE_INTERNAL,
+ "Member %s already included, skipping\n", userdns[i]);
+ continue;
+ }
+
+ member_el->values[member_el->num_values + j].data = (uint8_t *) \
+ talloc_steal(group_attrs, userdns[i]);
+ member_el->values[member_el->num_values + j].length = \
+ strlen(userdns[i]);
+ j++;
}
- member_el->num_values += nuserdns;
+ member_el->num_values += j;
}
static int sdap_fill_memberships(struct sdap_options *opts,
--
1.9.3
More information about the sssd-devel
mailing list