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

[Jakub Hrozek]

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