Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

memberof: keep memberOf attribute for nested member #541

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@pbrezina
Copy link
Member

commented Mar 19, 2018

If we have a member that is both direct and nested member,
memberOf attribute was removed if the direct membership
was deleted.

user ----------> groupB -> groupC
-> groupA /

user -> groupA -> groupB -> groupC

If we remove user->groupB from 1), we get 2) but groupB was still
removed from user memberOf attribute.

Resolves:
https://pagure.io/SSSD/sssd/issue/3636

@pbrezina

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2018

Do we have any memberof plugin unit or integration tests that I can amend? I did not find any...

@pbrezina

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2018

Please, review carefully. This fixed the issue and I did not run into any troubles with this patch, however I'm not sure if this check was only "just in case this happens but we should never get here" or it is actually used to do something meaningful.

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2018

yes, in the sysdb tests (the old check-based one, not under the cmocka/ folder)

@pbrezina

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2018

I gave a scratch build to users and the confirmed that the patch fix their issue.

@sumit-bose

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2018

@pbrezina, have you checked if you have only 1.

user -> groupB -> groupC -> groupA

That the memberships to groupC and groupA are removed properly as well if the membership to groupB is removed form the user?

And second that the memberships to groupC and groupA are remove if you remove groupC from groupB but keep the user a member of groupB?

The comment in the removed code sounds a bit like one of those cases.

Another test case might be

user -> groupB -> groupC -> groupA
|-> groupA
(groupA is direct and indirect member of groupB and should be kept for user if groupC is removed from groupB)

@pbrezina pbrezina closed this Aug 16, 2018

@pbrezina pbrezina deleted the pbrezina:memberof branch Aug 16, 2018

@pbrezina pbrezina restored the pbrezina:memberof branch Aug 16, 2018

@pbrezina pbrezina reopened this Aug 16, 2018

@pbrezina pbrezina force-pushed the pbrezina:memberof branch from 4126837 to a884873 Aug 16, 2018

memberof: keep memberOf attribute for nested member
If we have a member that is both direct and nested member,
memberOf attribute was removed if the direct membership
was deleted.

1)
user ----------> groupB -> groupC
     -> groupA /

2)
user -> groupA -> groupB -> groupC

If we remove user->groupB from 1), we get 2) but groupB was still
removed from user memberOf attribute.

Resolves:
https://pagure.io/SSSD/sssd/issue/3636

@pbrezina pbrezina force-pushed the pbrezina:memberof branch from a884873 to 361e4b5 Apr 2, 2019

@pbrezina

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

Sorry, this patch got stalled. I rebased it on top of current master.

All cases mentioned by @sumit-bose works correctly with this patch. I have not found any problem and the users confirmed that it works a year ago :-)

@sumit-bose

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

ACK.

Btw, after some digging I found that the code block you just removed was added to fix https://pagure.io/SSSD/sssd/issue/400. I guess this issue is specific for the local provider since of other provider SSSD does not have to manage the nested group hierarchy but just gets it from the external source. And since the local provider is not supported anymore this issue is gone as well.

bye,
Sumit

@sumit-bose sumit-bose added the Accepted label Apr 4, 2019

@pbrezina

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

Great! Thank you.

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

@jhrozek jhrozek closed this Apr 8, 2019

@jhrozek jhrozek added the Pushed label Apr 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.