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

Pavel Březina pbrezina at redhat.com
Tue Nov 20 12:22:52 UTC 2012


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.




More information about the sssd-devel mailing list