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

Jakub Hrozek jhrozek at redhat.com
Wed Nov 21 09:49:43 UTC 2012


On Tue, Nov 20, 2012 at 01:22:52PM +0100, Pavel Březina wrote:
> On 11/20/2012 02:09 AM, Simo Sorce wrote:
> >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.
> >
> 
> I tried it and it seems to work fine.
> Ack.

Pushed to master and sssd-1-9



More information about the sssd-devel mailing list