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

Only the first token role is validated #93

Open
flo-mic opened this issue Feb 24, 2025 · 3 comments
Open

Only the first token role is validated #93

flo-mic opened this issue Feb 24, 2025 · 3 comments
Labels
bug Something isn't working

Comments

@flo-mic
Copy link

flo-mic commented Feb 24, 2025

Vaultwarden Support String

Vaultwarden Build Version

1.32.2-2-alpine

Deployment method

Official Container Image

Custom deployment method

No response

Reverse Proxy

Ingress

Host/Server Operating System

Linux

Operating System Version

Linux

Clients

Web Vault

Client Version

Firefox Latest

Steps To Reproduce

Hi Timshel, first of all thank you very much for driving the sso support. Great job!

While testing the role mapping feature I found a small bug. If a user has more than 1 role on a client, only the first role is validated.

In my case i add a role „restricted-access“ to every client where keycloak is managing authorization. If the group is not present in the token, keycloak rejects the authentication and does not allow accessing the client.

The roles in the token claim are sorted alphabetically and so the sso login only checks the role „restricted—access“ and returns with a 400 bad request as I have disabled the fallback to user role and so no valid role was found.

Expected Result

All roles in the claim should be verified before returning 400 bad request

Actual Result

The roles in the token claim are sorted alphabetically and so the sso login only checks the role „restricted—access“ and returns with a 400 bad request as I have disabled the fallback to user role and so no valid role was found.

Logs


Screenshots or Videos

No response

Additional Context

No response

@flo-mic flo-mic added the bug Something isn't working label Feb 24, 2025
@Timshel
Copy link
Owner

Timshel commented Feb 25, 2025

Hey,

Yes the implementation was made with the expectation to only receive the supported roles: admin or user.

While ignoring unsupported roles is not really complicated it would then mean do I add another setting to control this behavior ? Which I believe there is already enough of.

So it goes back to the relation between the client app and the provider and who is defining what ?
In this case the role while relatively generic are defined by the client, so it seems logical to me that the client should receive only those roles (I'm aware that different providers might handle mapping differently).

To be specific to Keycloak, when defining the Mapper you have the ability to include only the role specific to an application (Token claim name: resource_access.${client_id}.roles). Any reason why you are forced to include other roles ?

@flo-mic
Copy link
Author

flo-mic commented Feb 26, 2025

Hi Timshel,

I'm only sending the roles of the specific client to vaultwarden. However the list of available roles is containing 3 roles and a user will always be assigned to 2 of them.

To give you some context, whenever i can configure user roles on a client, I'm also adding a role 'restricted-access' to this particular client. During the browser and direct grant login flows, a plugin is checking for the role 'restricted-access' on the client. If it can find the role on the client but not inside the user roles, keycloak denies the login and does not redirect back to vaultwarden.

You can find details about the plugin here https://github.com/sventorben/keycloak-restrict-client-auth

From my perspective we should not ignore specific roles or allowing only a single role in the token claim. I would just check if the roles contain a known role and select the highest one.

@Timshel
Copy link
Owner

Timshel commented Feb 26, 2025

allowing only a single role in the token claim

Multiple known role are supported, if a user has admin and user he will be logged as admin.

we should not ignore specific roles

But that's what you are asking, you want unknown roles to be ignored as opposed to the current failure.
As I mentioned, I don't want to add another setting, so I'll have to think on if ignoring unsupported role is a valid default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants