On Wed, Jul 27, 2016 at 06:56:15PM +0200, Jakub Hrozek wrote: > On Wed, Jul 27, 2016 at 11:36:09AM +0200, Jakub Hrozek wrote: > > On Wed, Jul 27, 2016 at 11:11:48AM +0200, Jakub Hrozek wrote: > > > On Tue, Jul 26, 2016 at 10:05:21PM +0200, Sumit Bose wrote: > > > > On Tue, Jul 26, 2016 at 06:06:48PM +0200, Jakub Hrozek wrote: > > > > > On Tue, Jul 26, 2016 at 05:25:11PM +0200, Jakub Hrozek wrote: > > > > > > On Tue, Jul 26, 2016 at 01:51:56PM +0200, Sumit Bose wrote: > > > > > > > > > The third patch adds a sysdb call to recursively resolve all > > > > > > > > > user-members of a group. Since the groups in SSSD's cache are > > > > > > > > > hierarchically organized the member attribute only contains > > > > > > > > > direct > > > > > > > > > user and group members. To get all users the group members > > > > > > > > > must be > > > > > > > > > resolved recursively. > > > > > > > > > > > > > > > > Would dereferencing memberof:top-level-group yield different > > > > > > > > results? > > > > > > > > > > > > > > It worked in my testing but I have to admit that I'm not sure if > > > > > > > it can > > > > > > > be used reliable all the time, i.e. is independent of all the > > > > > > > different > > > > > > > lookup sequences you can have with nested groups. If you are sure > > > > > > > it is > > > > > > > reliable, the call can be simplified. > > > > > > > > > > > > This is how memberof is supposed to work. I haven't tested all > > > > > > scenarios either (if there are some corner cases you'd like me to > > > > > > test, > > > > > > just let me know), but if there are differences, I would say these > > > > > > would > > > > > > be bugs in the memberof plugin and should be fixed. > > > > > > > > > > btw the patches seem to work fine, I tested getent passwd on an > > > > > overriden user, getent group on a group this user is a memberof (both > > > > > an > > > > > AD group and an IPA group with an external group in it) and id of this > > > > > user. > > > > > > > > > > All lookups show the expected data. Coverity is clean and CI passed. > > > > > > > > > > Therefore provisional ACK - the only remaining point remains the > > > > > recursive > > > > > member vs. memberof part. I don't mind accepting the patch as-is now, > > > > > if we agree to open a ticket and switch to memberof later in 1.14. > > > > > > > > Please have a look at attached patch, it replaces the recursive member > > > > based lookup by a (memberof=group_dn) search. It works well for me in > > > > some basic tests. > > > > > > Thank you, this patch works for me as well and Coverity is clean as > > > well. If you agree, I would squash the last patch into the one that adds > > > sysdb_get_user_members_recursively() and then push once CI finishes. > > > > > > (Also, ACK) > > > > Actually, tests fail after adding the memberof patch: > > [ FAILED ] test_nss_getgrnam_mix_subdom > > This turned out to be an off-by-one bug in the test. I'll fix it before > pushing. Otherwise ACK, waiting for CI to finish before I push the > patches.
CI: http://sssd-ci.duckdns.org/logs/job/50/39/summary.html _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
