[SSSD] [PATCH] LDAP: Don't add a user member twice when adding a primary group

Jakub Hrozek jhrozek at redhat.com
Tue Aug 26 09:37:36 UTC 2014


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.
-------------- 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 1ff95702295ed5e16c3c810b1ebcaaf6fe2496a6 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..b6b62f39374c9f9b9ac0037c167cba58c0e06a2d 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)) {
+        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]) == true) {
+            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