On Wed, Feb 24, 2016 at 01:17:27PM +0100, Sumit Bose wrote:
> On Tue, Feb 23, 2016 at 09:37:57PM +0100, Jakub Hrozek wrote:
> > On Mon, Feb 22, 2016 at 06:04:07PM +0100, Jakub Hrozek wrote:
> > > Hi,
> > >
> > > the attached patches implement https://fedorahosted.org/sssd/ticket/2522
> > >
> > > Here is what I tested:
> > > 1) topgr -> bottomgr -> extgr -> [email protected]
> > > - this is a simple case to test a member of an external group
> > >
> > > 2) ipa_uni -> ipa_uni_ext -> [email protected]
> > > |
> > > +-- [email protected]
> > > \- [email protected]
> > >
> > > - this is to test that a universal group with members from two
> > > domains resolves correctly
> > >
> > > 3) idm_admin -> idm_admin_ext -> [email protected]
> > > \- [email protected]
> > > -> [email protected]
> > > - tests that a combination of user and group members work
> > >
> > > 4) toc_admin -> toc_admin_ext -> [email protected]
> > > -> [email protected]
> > > - tests that two users with the same name from different domains
> > > work correctly
> > >
> > > I also tested adding and removing group members and found some bugs in
> > > our groups processing (#2940, #2952)
> > >
> > > Please feel free to suggest more tests. If the patches are accepted,
> > > I'll also add unit tests for the nested groups code, but currently we
> > > need to make the patches available to upstream.
> > >
> > > I also tested a combination of user lookups and group lookups,
> > > especially with someuser to test that the initgroups and group lookups
> > > play well together. And that's where I found one issue that I think we
> > > should fix in master -- the initgroups code adds the external members as
> > > direct members of the POSIX group, so I did the same in the group
> > > lookup code. This is safer, but doesn't allow us to remove the hack we
> > > have (6fac5e5f0c54a0f92872ce1450606cfcb577a920) to retain external
> > > members.
> > >
> > > I think in 1.14 it would be more systematic to store the group
> > > membership as:
> > > gr -> ext_gr -> ad_member
> > > rather than:
> > > gr -> ext_gr
> > > -> ad_member
> > >
> > > The reason we can't remove those hacks is that if the external group is
> > > up-to-date and only the top group is resolved, we would only write the
> > > group memberships as stored in LDAP, effectively removing the AD
> > > memberships. This of course depends on fixing the issues in memberof
> > > plugin, but I still think it's worth it. The current code works well,
> > > though, so I guess we can accept it for 1.13.
> > >
> > > Our initgroups code should be well equipped to handle that and we could
> > > remove the hacks we have at the moment.
> >
> > The attached patches fix issues in the reallocation logic that Sumit
> > found during his review. Only the third patch had changed.
>
> Thank you. I do not have additional comments on the code and the patches
> work well in my test. I tested with nested groups on the AD and IPA side
> as well to put even more groups between the group with them members in
> AD and the group looked up on the IPA side but all worked well. CI
> passed as well http://sssd-ci.duckdns.org/logs/job/37/87/summary.html so
>
> ACK.
>
> Btw, about the change you suggested to store the AD members in the
> external groups. All PAC processing does not know about the external
> group and can only lookup up and add the AD members to the real group.
> So I'm not sure if this change is really needed.
>
> bye,
> Sumit
Thank you for the review:
master:
3cf7fdfcaedb986f42a6640e26aa057007b64045
e2d96566aeb881bd89e5c9236d663f6a9a88019a
c32266e79f9d4bebd0c31eaa8d6fa26050e7fb3e
sssd-1-13:
7db3bdfd6b1b845866c1ff062d25de5804141e89
00ee45423f0712b83926c6f8b354a1a18ff741c8
19194cb18a1cc20f02423861dd831aa5bc3a1003
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/admin/lists/[email protected]
