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