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.