-
Notifications
You must be signed in to change notification settings - Fork 182
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
proxy: Assign user roles from OIDC claim #5784
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
I configured Einstein to be an "ocisAdmin" in Keycloak before his first login. Therefore, his access token looks like this:
But oCIS says:
Another usecase was that I added a fresh user with the "ocisAdmin" role. The user could log in and do admin stuff. But when you change the role to "ocisSpaceAdmin" or "ocisUser", this user won't even see his personal space (in the graph api) anymore!? |
Did you by chance restart ocis multiple times? It yes, this might be caused by #3432 (the default role assignment are created with every restart of ocis). Which reminds we that we need to disable the default role assignment and the creation of the admin user in the idm in this kind of setup. (I added both to checklist in the description)
That seem to be a bug. Though changing roles worked fine for me when briefly testing it. Need to take a closer look. |
Could be the case that I reused an already existing docker volume 🤔
That would be kind of a "OIDC Provider has the authority about user roles"-flag? So no more "SETTINGS_ADMIN_USER_ID" and editable roles in the WebUI!?
|
Yeah. I think so. If we get the roles from the IDP we should not allow overriding them locally. This will just cause confusion (and also pretty difficult synchronization and conflict issues). Do you disagree? @tbsbdr I guess this is probably also interesting for you. @kulmann This also means we need some capability to tell the WebUI that roles cannot be re-assigned. |
3a59f75
to
cdbf607
Compare
💥 Acceptance test Core-API-Tests-ocis-storage-3 failed. Further test are cancelled... |
e7c3ea1
to
40bb15f
Compare
This should be ready for review now. Provided the CI is green. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general. Just nitpicking as always...
return nil, err | ||
case 1: | ||
// exactly one mapping. This is right | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious: The break
is needed as the linter won't allow having an empty branch on the switch
? Or is there another reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. I think that this was just my C habits breaking though. I am not even sure the linter would complain about this.
To align with what we're using everywhere else.
This removes the "withRoles" flag from the GetUserByClaims lookup and move the functionality into a separate method. This should make the code a bit more readable in preparation for maintaining the RoleAssignments from OIDC claims.
This will make it easier to add/remove options to the backend in the future.
This moves the lookup and the creation of the users' role assignemt out of the user backend into its own interface. This makes the user backend a bit simpler and allows to provide different implemenation for the user role assignment more easily.
Add a UserRoleAssigner implementation that extract role names from the users' claims and creates role assignments in the settings service based on a configured mapping of claim values to ocis role names. Closes: owncloud#5669
Avoid torturing the settings service with "ListRoles" request for every incoming request to the proxy. The role Mapping is refreshed if cached data is older than 5 minutes.
Response bodies need to be closed
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome now 👍
Description
This adjusts the account resolver middleware to update a user's role assignment from a claim provided via the IDPs userinfo endpoint.
It's still work in progress and needs some adjustments before we can merge it:
roles
ocis_keycloak
deployment example.cache expired.
For testing purposes this can be used with the
ocis_keycloak
deployment, which already has some roles assigned to the demo users.As ocis only allows one role per user. The code errors out if more than one role mapping is found for a user. We might need to change that behaviour somehow. See also: #5750 (review)
Currently this uses a very hacky way for obtaining a reva token with admin privileges (need for actually assigning the roles). We really need service accounts (#5550) for stuff like this.
Closes: #5669