8000 Fixes #9482: A user could be assigned to a parent group if he is already assigned to a subgroup. by leischt · Pull Request #10785 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fixes #9482: A user could be assigned to a parent group if he is already assigned to a subgroup. #10785

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 6, 2022
Merged

Fixes #9482: A user could be assigned to a parent group if he is already assigned to a subgroup. #10785

merged 1 commit into from
Sep 6, 2022

Conversation

leischt
Copy link
Contributor
@leischt leischt commented Mar 17, 2022 •
edited
Loading

Closes #9482
Changed precondition for joining a group from is not a member to has group not directly assigned.

@leischt leischt mentioned this pull request Mar 17, 2022
Closed
@leischt leischt force-pushed the 9482-add_user_to_parent_group_if_member_of_subgroup branch from 4839c94 to 22db324 Compare April 25, 2022 07:37
@ahus1 ahus1 self-assigned this Apr 25, 2022
@ahus1
Copy link
Contributor
ahus1 commented Apr 25, 2022

Thanks for contributing to this project with this pull request!

Please add Closes #9482 to the description of this pull request.

This will help collaboration on GitHub in two ways:

  1. It populates the right-hand column on both the issue and the PR with a reference to the issue you're about to fix.
  2. It will close the issue once the PR has been merged automatically.

Thanks!

@ahus1 ahus1 removed their assignment May 14, 2022
@stianst stianst requested review from mposolda and hmlnarik June 7, 2022 09:48
@sschu
Copy link
Contributor
sschu commented Jun 21, 2022

@mposolda @hmlnarik Could any of you please have a look? Would be great to have this in KC19.

@hmlnarik hmlnarik added the status/hold PR should not be merged. On hold for later. label Jun 22, 2022
hmlnarik
hmlnarik suggested changes Jun 22, 2022
View reviewed changes
Copy link
Contributor
@hmlnarik hmlnarik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the test.
Sadly, the fix is not valid. The isMemberOf accounts for recursive walk of group hierarchy. This is not the case with the code proposed in this PR which only checks the direct assignment of a user to groups.

@hmlnarik
Copy link
Contributor

@leischt @sschu Please refer to the original issue #9482 for further discussion

@sschu
Copy link
Contributor
sschu commented Aug 10, 2022

@hmlnarik In the light of #12726 (reply in thread), do you think this could be merged? Also tests need to be rerun I guess...

@leischt leischt force-pushed the 9482-add_user_to_parent_group_if_member_of_subgroup branch from 22db324 to 8c5ce2c Compare August 25, 2022 07:27
@stianst stianst added priority/important Must be worked on very soon priority/blocker Highest Priority. Has a deadline and it blocks other tasks and removed priority/important Must be worked on very soon priority/blocker Highest Priority. Has a deadline and it blocks other tasks labels Sep 2, 2022
@stianst stianst requested a review from hmlnarik September 2, 2022 06:32
@stianst stianst removed the status/hold PR should not be merged. On hold for later. label Sep 2, 2022
hmlnarik
hmlnarik suggested changes Sep 2, 2022
View reviewed changes
Copy link
Contributor
@hmlnarik hmlnarik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR. Could you please rebase, it has conflicts, and also please see inline

@sschu sschu force-pushed the 9482-add_user_to_parent_group_if_member_of_subgroup branch from 8c5ce2c to b89a6c6 Compare September 5, 2022 11:28
hmlnarik
hmlnarik reviewed Sep 6, 2022
View reviewed changes
hmlnarik
hmlnarik reviewed Sep 6, 2022
View reviewed changes
Copy link
Contributor
@hmlnarik hmlnarik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the changes, looks better now. Could you please see the comments inline?

Nit: For the sake of better readability, leveraging the newly introduced method RoleUtils.isDirectMember in the changed lines which currently use getGroupsStream().{none,any}Match would be preferred.

@sschu sschu force-pushed the 9482-add_user_to_parent_group_if_member_of_subgroup branch from b89a6c6 to 5ccc549 Compare September 6, 2022 10:24
hmlnarik
hmlnarik approved these changes Sep 6, 2022
View reviewed changes
Copy link
Contributor
@hmlnarik hmlnarik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR!

@hmlnarik hmlnarik merged commit cc2bb96 into keycloak:main Sep 6, 2022
7BBC
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sschu sschu sschu left review comments

@hmlnarik hmlnarik hmlnarik approved these changes

@mposolda mposolda Awaiting requested review from mposolda

Assignees

@hmlnarik hmlnarik

Labels
priority/important Must be worked on very soon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can't add user to "parent group" if member of a subgroup
5 participants
@leischt @ahus1 @sschu @hmlnarik @stianst
0