[SSSD] [PATCH] LDAP: Only convert direct parents' ghost attribute to member

Simo Sorce simo at redhat.com
Tue Nov 20 01:09:12 UTC 2012


On Mon, 2012-11-19 at 23:31 +0100, Jakub Hrozek wrote:
> On Mon, Nov 19, 2012 at 05:51:53PM +0100, Pavel Březina wrote:
> > On 11/18/2012 11:58 PM, Jakub Hrozek wrote:
> > >[PATCH 1/2] SYSDB: Use the add_string convenience functions for managing
> > >ghost user attribute
> > >
> > >Using the convenience function instead of low-level ldb calls makes the
> > >code more compact and more readable.
> > >
> > >[PATCH 2/2] LDAP: Only convert direct parents' ghost attribute to member
> > >
> > >https://fedorahosted.org/sssd/ticket/1612
> > >
> > >I'm not too happy with the originalDN attribute exposed in the
> > >sysdb_add_user/store_user functions but it seemed like a better way
> > >than grabbing the attribute from the extended attributes in the "attrs"
> > >array. At the same time, we need to adjust the attributes during all user
> > >adds, not just during sdap_store_user from the LDAP provider. Ideas on
> > >how to make the process cleaner are welcome.
> > >
> > >This patch changes the handling of ghost attributes when saving the
> > >actual user entry. Instead of always linking all groups that contained
> > >the ghost attribute with the new user entry, the original member
> > >attributes are now saved in the group object and the user entry is only
> > >linked with its direct parents.
> > >
> > >As the member attribute is compared against the originalDN of the user,
> > >if either the originalDN or the originalMember attributes are missing,
> > >the user object is linked with all the groups as a fallback.
> > >
> > >The original member attributes are only saved if the LDAP schema
> > >supports nesting.
> > 
> > >@@ -235,6 +236,19 @@ sdap_process_ghost_members(struct sysdb_attrs *attrs,
> > >         return ret;
> > >     }
> > >
> > 
> > Hi,
> > just one nitpick.
> > 
> > >+    if (store_original_member) {
> > >+        DEBUG(SSSDBG_TRACE_FUNC, ("The group has %d members\n", memberel->num_values));
> > >+        for (i = 0; i < memberel->num_values; i++) {
> > >+            ret = sysdb_attrs_add_string(sysdb_attrs, SYSDB_ORIG_MEMBER,
> > >+                                        (const char *) memberel->values[i].data);
> > >+            if (ret) {
> > >+                DEBUG(SSSDBG_OP_FAILURE, ("Could not add member [%s]\n",
> > >+                    (const char *) memberel->values[i].data));
> > 
> >       wrong indentation^^
> > 
> > >+                return ret;
> > >+            }
> > >+        }
> > >+    }
> > >+
> > 
> > Otherwise it looks good to me.
> 
> Thank you for the review. Simo requested some additional changes, in
> particular:
>     * use "it" instead of "him" when talking about user
>     * don't misuse twopass, but use a separate variable
>     * fix code to adhere to 80-columns rule on several places
> 
> New patches are attached.

I haven't been able to test the patch yet, but at least looking at the
code I see no issues, so I would ack.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list