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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
