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