[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