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.

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

Add allowedGroups to UserConfig in IdZ configuration #2606

Merged
merged 40 commits into from Dec 20, 2023
Merged

Add allowedGroups to UserConfig in IdZ configuration #2606

merged 40 commits into from Dec 20, 2023

Conversation

klaus-sap
Copy link
Contributor

@klaus-sap klaus-sap commented Nov 17, 2023 •
edited

#2505 -> Proposal 3.
We want to add an optional attribute "allowedGroups" to the UserConfig in IdZ configuration. If this attribute is not set (i.e., allowedGroups == null), then all groups are allowed as before. If this attribute contains an array of groups, then only the groups from defaultGroups plus allowedGroups are allowed. All other groups can neither be created nor assigned to users.

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/186503483

The labels on this github issue will be updated when the story is started.

@strehle
Copy link
Member

strehle commented Nov 17, 2023

@klaus-sap here you can see the smells from sonar
https://sonarcloud.io/summary/new_code?id=cloudfoundry-identity-parent&pullRequest=2606

strehle

This comment was marked as outdated.

Sign in to view

strehle
strehle previously requested changes Nov 21, 2023
View reviewed changes
Copy link
Member

@strehle strehle left a comment

Choose a reason for hiding this comment

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

Open
a) combine default and allowed groups. So from understanding, if you have default groups defined, then these entries are automatically part of allowedGroups. So this behaviour should be implemented.
b) Check if the limitation should be really done always or only in Identity Zone NOT EQUAL to 'uaa'

@klaus-sap
Copy link
Contributor Author

Open a) combine default and allowed groups. So from understanding, if you have default groups defined, then these entries are automatically part of allowedGroups. So this behaviour should be implemented. b) Check if the limitation should be really done always or only in Identity Zone NOT EQUAL to 'uaa'

a) commit 6e6ab18
b) I would prefer to handle the IdZ configuration of all zones in the same way. If you don't want allowedGroups in the "uaa" zone, simply don't configure them

strehle
strehle reviewed Nov 22, 2023
View reviewed changes
Copy link
Member

@strehle strehle left a comment

Choose a reason for hiding this comment

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

see here new sonar

https://sonarcloud.io/summary/new_code?id=cloudfoundry-identity-parent&pullRequest=2606

coverage
https://sonarcloud.io/component_measures?id=cloudfoundry-identity-parent&pullRequest=2606&metric=new_coverage&view=list

smells
not all need to be fixed, but please check them
https://sonarcloud.io/project/issues?resolved=false&types=CODE_SMELL&sinceLeakPeriod=true&pullRequest=2606&id=cloudfoundry-identity-parent

server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaAuthorizationRequestManager.java Show resolved Hide resolved
server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimGroupProvisioning.java Outdated Show resolved Hide resolved
server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimGroupProvisioning.java Outdated Show resolved Hide resolved
server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimGroupProvisioning.java Outdated Show resolved Hide resolved
model/src/main/java/org/cloudfoundry/identity/uaa/zone/UserConfig.java Outdated Show resolved Hide resolved
@strehle
Copy link
Member

strehle commented Nov 22, 2023

@klaus-sap Please add some scenario tests into
https://github.com/cloudfoundry/uaa/blob/develop/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/ScimGroupEndpointsIntegrationTests.java

you can create an extra zone or stay in default , btw. zone creation https://github.com/cloudfoundry/uaa/blob/develop/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/util/IntegrationTestUtils.java#L671

Scenario I see
a) Add allowedGroups to zone and perform positive and negative test , means create the group
b) Update or deleted one entry in defaultGroups (because this has influence to allowedGroups) and then do the same create the groups and verify result

@strehle strehle self-requested a review November 24, 2023 10:31
strehle
strehle approved these changes Nov 24, 2023
View reviewed changes
Copy link
Member

@strehle strehle left a comment •
edited

Choose a reason for hiding this comment

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

sonar ok,

  • almost 100 coverage.
  • Integration Tests available

https://sonarcloud.io/component_measures?id=cloudfoundry-identity-parent&pullRequest=2606&metric=new_coverage&view=list

@hsinn0 @Tallicia FYI please check who can do a review from VMware side in next week , thanks

@strehle strehle requested a review from a team November 24, 2023 10:32
@strehle strehle added this to the 76.27.0 milestone Nov 24, 2023
@strehle strehle mentioned this pull request Dec 1, 2023
Merged
@strehle strehle modified the milestones: 76.27.0, 76.28.0 Dec 11, 2023
bruce-ricard
bruce-ricard previously requested changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@bruce-ricard bruce-ricard left a comment

Choose a reason for hiding this comment

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

I have one major concern about the validation only happening at group write time, see inline comments.

And a couple sub-major implementation design comments.

server/src/main/java/org/cloudfoundry/identity/uaa/zone/UserConfigValidator.java Outdated Show resolved Hide resolved
server/src/main/java/org/cloudfoundry/identity/uaa/zone/UserConfigValidator.java Outdated Show resolved Hide resolved
server/src/main/java/org/cloudfoundry/identity/uaa/zone/UserConfigValidator.java Show resolved Hide resolved
server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimGroupProvisioning.java Show resolved Hide resolved
server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimGroupProvisioning.java Show resolved Hide resolved
model/src/main/java/org/cloudfoundry/identity/uaa/zone/UserConfig.java Show resolved Hide resolved
server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimGroupProvisioning.java Show resolved Hide resolved
server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimGroupProvisioning.java Show resolved Hide resolved
@hsinn0 hsinn0 requested review from bruce-ricard and removed request for bruce-ricard December 20, 2023 00:54
@hsinn0 hsinn0 dismissed bruce-ricard’s stale review December 20, 2023 00:56

The change request has been actually resolved. I am dismissing it as Bruce won't be available for a couple weeks.

@hsinn0 hsinn0 removed the request for review from bruce-ricard December 20, 2023 00:57
@hsinn0
Copy link
Contributor

hsinn0 commented Dec 20, 2023

@bruce-ricard & I agreed that I would take over this PR review from our side as he will be unavailable for a couple of weeks. I am approving this PR.

@strehle strehle removed unscheduled in_review The PR is currently in review labels Dec 20, 2023
This was linked to issues Dec 20, 2023
Missing user management from custom IdZ to CC #2505
Open
Limit user creation within a zone #2581
Closed
@strehle strehle removed a link to an issue Dec 20, 2023
Limit user creation within a zone #2581
Closed
@strehle strehle merged commit 63ad5f3 into cloudfoundry:develop Dec 20, 2023
20 checks passed
@adrianhoelzl-sap adrianhoelzl-sap mentioned this pull request May 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bruce-ricard bruce-ricard bruce-ricard left review comments

@strehle strehle strehle approved these changes

@tack-sap tack-sap tack-sap approved these changes

@hsinn0 hsinn0 hsinn0 approved these changes

Assignees

@Tallicia Tallicia

Labels
None yet
Projects
Foundational Infrastructure Working Group
Archived in project
Milestone
76.28.0
Development

Successfully merging this pull request may close these issues.

Missing user management from custom IdZ to CC
7 participants
@klaus-sap @cf-gitbot @strehle @hsinn0 @bruce-ricard @tack-sap @Tallicia