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
* master:
* f2e8a7c3230fac11175c0bd17c14c66a8e9b25ad
* 160ba891ec483c5b7d2a3fcca5bd992fc790efe0
* 17bfd9f69251781140e4b2b55ffeb649d7a79e86
* 1594701fbdc341069e11cff9a85e7a795e52db3d