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

fix: external group memberships should have correct origin #3033

Merged
merged 3 commits into from
Sep 13, 2024

Conversation

mikeroda
Copy link
Contributor

@mikeroda mikeroda commented Sep 9, 2024

External group memberships determined from processing an OIDC or SAML login should be stored with the origin of the identity provider and not the default uaa origin.

mikeroda and others added 3 commits September 9, 2024 13:53
External group memberships determined from processing an OIDC or
SAML login should be stored with the origin of the identity
provider and not the default uaa origin.

Change-Id: I7a3fb410d3ef781398f356b0df3a6973a5836c4c
…ext-group-origin

# Conflicts:
#	server/src/test/java/org/cloudfoundry/identity/uaa/scim/bootstrap/ScimUserBootstrapTests.java
@strehle strehle requested a review from a team September 11, 2024 11:58
@strehle
Copy link
Member

strehle commented Sep 11, 2024

please check if my rebase is Ok for you.

Sonar is green: https://sonarcloud.io/summary/new_code?id=cloudfoundry-identity-parent&pullRequest=3033

The issue is known but I was not sure in the past, wheather this is a bug or feature , so from our side (SAP) we can agree to this change but from @hsinn0 @peterhaochen47 broadcom side you should check as well

@mikeroda
Copy link
Contributor Author

Yes the rebase looks fine to me

@hsinn0
Copy link
Contributor

hsinn0 commented Sep 11, 2024

The issue is known but I was not sure in the past, wheather this is a bug or feature , so from our side (SAP) we can agree to this change but from @hsinn0 @peterhaochen47 broadcom side you should check as well

Thanks, we will take a look at this.

@hsinn0
Copy link
Contributor

hsinn0 commented Sep 12, 2024

We will try to find out whether the existing implementation was intentional or an oversight, and also if there is anything out there that depends on the current behavior.
Meanwhile, a question to @mikeroda. Are you trying to resolve an actual field issue with this change, or did you just find the problem by examining the code?

@mikeroda
Copy link
Contributor Author

@hsinn0, It is causing an actual issue for me.

@peterhaochen47
Copy link
Member

peterhaochen47 commented Sep 12, 2024

Here is my guess/understanding after a quick read of the existing code:

It looks likeScimUserBootstrap is for processing any users creation/update/delete operations as defined in server config (uaa.yml), which I think only defines UAA internal users, whose origin can only be uaa.

The external IDP users (origin != uaa) are not defined in uaa.yml. They are created as a result of external IDP logins, and their group membership are defined by various relevant settings (e.g. external group mapping), rather than the scim users/groups sections of uaa.yml. At least I think that's the intention, and the reason why when ScimUserBootstrap is processing users, it's always assuming UAA internal users (origin=uaa).

What is the actual issue you are seeing? What is your use case?

@mikeroda
Copy link
Contributor Author

In addition to processing users in the server config, it also receives events like ExternalGroupAuthorizationEvent, used to process external groups during external logins. Anyway, my commit still works for users processed from the server config.
To answer your question, I have some custom code that does additional processing for external group memberships and I rely on the origin being set correctly.

@peterhaochen47
Copy link
Member

peterhaochen47 commented Sep 13, 2024

In addition to processing users in the server config, it also receives events like ExternalGroupAuthorizationEvent, used to process external groups during external logins. Anyway, my commit still works for users processed from the server config. To answer your question, I have some custom code that does additional processing for external group memberships and I rely on the origin being set correctly.

I see. Yeah I don't see any issues with this PR then.

@hsinn0 hsinn0 self-requested a review September 13, 2024 20:58
@hsinn0 hsinn0 merged commit dbc191d into cloudfoundry:develop Sep 13, 2024
22 checks passed
Copy link
Contributor

@hsinn0 hsinn0 left a comment

Choose a reason for hiding this comment

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

Approving and merged to develop.

strehle added a commit that referenced this pull request Oct 2, 2024
fix for issue 3069

The change #3033 forbids the user and group creation with UAA origin but assigns always the origin from the user itself

This breaks the scim user bootstrapping from uaa.yml.

Separate the create user into
- from uaa.yml (propertiesSetup)
- from shadow user creation

In case of propertiesSetup and LDAP origin we allow
again the creation of group assignments to UAA origin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants