Allow adding a mapping between OIDC user role (from ID token) and application user role #37989
Comments
|
/cc @pedroigor (oidc), @sberyozkin (oidc) |
|
Thanks @jycr for opening this enhancement request, it makes sense. FYI, we already have a generic support for this kind of mapping, which is meant to work with all authentication mechanisms, with @michalvavrik completing #37275, will be available in 3.7.0.CR1, for example: Since you'd like to use should do (CC @michalvavrik just in case) Can you please try a snapshot and see if it works for you ? |
Is there any Public Maven Repository can I use to test a Quarkus SNAPSHOT? |
|
Just tested with: <dependencyManagement>
<dependencies>
<dependency>
<groupId>io.quarkus.platform</groupId>
<artifactId>quarkus-bom</artifactId>
<version>999-SNAPSHOT</version>
<type>pom</type>
<scope>import</scope>
</dependency>
</dependencies>
</dependencyManagement>io.quarkus.platform:quarkus-bom:999-SNAPSHOT from https://s01.oss.sonatype.org/content/repositories/snapshots/io/quarkus/platform/quarkus-bom/999-SNAPSHOT/quarkus-bom-999-20240103.021525-126.pom My project dependency tree-> It doesn't seem to work :'( but isn't it strange that the Quarkus BOM 999-SNAPSHOT references 3.6.0 as the version for Quarkus modules and not version 999-SNAPSHOT? |
|
@jycr I believe you should use quarkus-bom directly, for example: https://s01.oss.sonatype.org/content/repositories/snapshots/io/quarkus/quarkus-bom/999-SNAPSHOT/quarkus-bom-999-20240103.021116-987.pom |
@sberyozkin : Just tested with: <dependencyManagement>
<dependencies>
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-bom</artifactId>
<version>999-SNAPSHOT</version>
<type>pom</type>
<scope>import</scope>
</dependency>
</dependencies>
</dependencyManagement>NB: According article Platforms and Streams: a new way to discover Quarkus extensions:
My project dependency treeNB: There are so many differences between:
-> But it doesn't seem to work :'( |
|
Can you clarify please, can we have only and then ? It is not easy to spot from the PR. If it is not possible yet then it would be great if one can have @jycr FYI, you can implement this kind of mapping easily with a custom SecurityIdentityAugmentor right now, you can inject a custom mapping map property and augment the identity: https://quarkus.io/guides/security-customization#security-identity-customization Hopefully we can make it simpler, but the augmentor approach is always available |
|
I confirm that It works when I add following properties: |
we are talking the path matching policy, hence if there is no path, there is no match
The fact that policy is only applied on matching path wasn't changed by my PR, it's here from the beginning. My PR just says: if policy is applied, identity augmentation will happen
I think we can add add default permission for policies without matching HTTP permissions, but it smells like a lot of magic. I think we have already such complexity with shared policies and repeated wildcards, it's much better to understand for user what they are doing - if they want the policy to apply, they need to specify winning-the most specific path.
IMHO it is unnecessary, documentation is quite clear: here https://quarkus.io/version/main/guides/security-authorize-web-endpoints-reference#map-security-identity-roles I copied and pasted
+1 |
|
@sberyozkin maybe docs tweak is needed to make it clear for users that read docs and are not sure what is needed like you? I can't tell, lgtm sorry, let me know. |
|
@michalvavrik Thanks, the problem is, that duplicates the idea of IMHO the concept of the role mapping is independent to how these roles are enforced. But right now, after mapping I'm not sure now what you meant in the docs that it works for In any case, I can certainly fix it at the OIDC level only with the IMHO the mapping should be done at the higher level, for example at |
|
@michalvavrik I'm sorry I did not provide all the above feedback during the earlier review of #37275, it was very thoroughly done as always, but IMHO it needs some follow up work. I'm more concerned now about the mapping being done at the policy level, and as such it has to be repeated many times, for every policy, while it really should be done only once. The problem is, I'm off from this Fri afternoon till 15th Jan, so even if you will have time and agree to do something else I won't have time to review, may be on 15th/16th Jan only, though Stuart, David may help. If you won't have time to update it then may be we should revert it as it is still only on main and target for 3.9 |
@sberyozkin I'm out till 18.1., so I can't address it now anyway, I already spent more time responding to other issues (and tickets not related to Quarkus then I have). But I can tell you what you suggest now is in my eyes completely different and it is definitely not a bug, it's more like enhancement feature. I also disagree, but please do what is necessary from your POV. np!
The policy you mentioned enforces only authentication. Only authenticated requests can ever pass
As said above, there is no role enforcement I'm aware of in your example configuration properties.
I don't know what you suggest, if you map role
They are not, are you sure you are not confusing
I'm honestly sorry, but I don't see anything to fix.
Why is that problem? Are you sure you realized that only non-anonymous
I complete disagree, but I recognize it is your responsibility as guardian of Quarkus Security, so please go ahead and revert the PR, I can't provide any alterations anytime soon. Absolutely fine, np!
This suggestion will result in same behavior because if you want to map roles, you need authenticated policy. You can use shared policies to define this mapping for path |
I confirm that this ticket is not a bug, but a suggestion for improvement. The solutions described above (based on the SNAPSHOT version of Quarkus) seem to work for me. I think we can close this issue? |
|
thank you @jycr , let's wait for Sergey and close / not close the issue based on his answer, I'm not sure if this is not confusion between the 2 config properties I mentioned above, but if not and Sergey is convinced about his suggestion, we can enhance role mapping. |
Thanks, but your enhancement request gave an opportunity to myself and Michal to discuss the solution in place again. We are still in the SNAPSHOT in this regard so we have a chance to tune things. So lets keep this issue open for now. @michalvavrik You made my day with referring to me as a Guardian of Quarkus Security :-), that would be Stuart I guess, I'd have an IMHO the fact that one can do shows the mapping is done at a too low level. HTTP Security policy like The solution proposed by @jycr is correct because it is done at a higher level, as you can see -
It can be deprecated at some point and done similarly at the higher level too.
I did not pay the attention, the 2 policies above are correct. Now that you mentioned it, I believe https://quarkus.io/version/main/guides/security-authorize-web-endpoints-reference#map-security-identity-roles needs an update - if this is an authentication policy, why do the mapping, who cares what roles are there ? Do you mean you can complement that with
No it won't. If we have it done outside of HTTP policy, for example, works or
works without having to do IMHO we should revert it for now and tune a little bit |
|
@stuartwdouglas Stuart, would you like to comment ? |
go ahead (no need to involve me, I'll know about the revert as you mentioned it here), I respect your opinions. Just note, you will need to revert shared policies PR as well, as the tests will fail and documentation will become incorrect, or you need to rewrite it. I'll provide my view on your latest comments but I strongly disagree with your comments. IMHO you are incorrectly describing what is done by current implementation:
Default
It was your suggestion to use policies for roles to permissions mapping, so I'm not sure why is it any worse for roles to roles.
That's just your opinion and I don't think you can support it with any data or references. If I want different roles mapping for different paths, why will you take this from me? It is possible now with path matching policies.
Your example enforces and annotations
mapping for non-authenticated request for anonymous
Alright, acceptable, but I think might be interesting for you to know that what you suggest now is what I suggested for permissions and you preferred using policies. And I think you were right. Your new suggestion will give less flexibility and I can't see pros.
Roles allowed policy is
I don't asks users to complement HTTP security policies and standard security annotations. I say: you can use authorization using configuration to map roles. There is no complementation, there is only logic: if you want to map roles, you need authenticated identity.
So your suggestion will take away from users option to apply mapping based on paths and for them it is "better" because they don't need to write 2 lines of code. I think I understand you now, but please note that I don't agree, IMHO you should revert it in few days and think about it for a while. |
|
@michalvavrik Can you please give me at least one good reason why, with the same application, we'd want to let users |
|
@michalvavrik Please think about till 15th, it is not that bad as you are feeling right now, I'll ping Stuart directly and ask for his feedback, if you can convince yourself and agree that users won't lose anything with pushing the config a little bit up, then we can quickly revert both of these commits, IMHO, if you will have time to experiment, you will be able to prove things will become simpler. We don't have to rush, even though I appreciate users like @jycr already need it, the easy SecurityIdentityAugmentor workaround is always possible for now - I can type the custom augmentor here to help @jycr if the final solution will get postponed a bit. |
An application with 2 resources: But I think what you meant to ask is that on pah
Your suggestion will work, please go ahead. I can't be available from now till 18th, so just go ahead please whenever it suits you. Maybe I won't be available after that due to personal reasons.
Sure. I'd not do it for 2 lines, IMO if these 2 lines are so bad, we can just add default HTTP permission. But it's ok.
IMHO @jycr use case is solved by what is currently in place, no augmentor is needed. What you suggestion is simplification. |
No. You are concerned about losing the flexibility if the configuration is pushed higher and I'm trying to clarify that no flexibility will be lost at the practical level. The only flexibility that we can lose is mapping the same input role to different deployment specific roles which is possible now simply because each path specific policy can do its own mapping but I honestly can't see it being necessary - which is what I asked about - what can we lose if an input
Yeah, I agree with that - I suppose we all try to simplify the dev experience. Let me offer you an example and we can all think if it is an oversimplification or not, may be it is, and the whole revert process is not worth it. So, here is a resource A and all I want is to enforce RBAC with This is not an over complication, basic resource. Now, I need to map some token which has Current option: Proposed option: With HTTP permissions the proposed solution would only be marginally simpler though. Here you go. Maybe, now that I've typed it all, we can add, in 3.9. in addition to what we already have, this mappings will add to whatever policy specific mappings exist ? I agree, that this optimization won't save much when only HTTP security policies are used. I'm not comfortable that for the mapping to work with So, what about this compromise, add |
We can discuss it later with Michal, Stuart if there will be an interest. Thanks @jycr @michalvavrik |
|
@michalvavrik I'll open an enhancement request and we can continue there in a few weeks, cheers |
|
Your example and arguments are good @sberyozkin . |
|
@michalvavrik Hope I haven't affected your quiet New Year break Michal :-), thanks for another invigorating debate :-) |
Description
Thanks to the Quarkus OIDC extension, I was able to interconnect my application with an identity provider (based on Keycloak).
I was able to retrieve my user's profiles provided by the identity provider via the idToken (in a specific
user_profileclaim)Here is the subset of an example ID token received:
{ "name": "[email protected]", "issuer": "https://idp.acme.com/auth/realms/ACME", "subject": "f:c9507fd1-12f3-487f-8f23-77eb06728749:[email protected]", "tokenId": "d0e14a43-8545-4b67-bb9d-7704c01901dc", "claims": { "sub": "f:c9507fd1-12f3-487f-8f23-77eb06728749:[email protected]", "typ": "ID", "user_profile": [ "AP_DEV_UI_USER", "AP_DEV_UI_ADMIN", "AP_PROD_UI_USER" ] } }And here a subset of my configuration:
The list of profiles provided by the IdP are relatively "obscure".
I need to map profile to role according different environment
Is it possible to add functionality to “map” a profile to a role?
Thus, I will be able to protect my resources simply via the corresponding annotations such as for example:
Implementation ideas
We can execute the mapping in findClaimWithRoles method of OidcUtils.java#L203-L218 from configuration provided in
OidcTenantConfig.Roles rolesConfigparameter:Property to add in OidcTenantConfig.Roles config:
Here is an example of configuration to correctly respond to my use case described above:
The text was updated successfully, but these errors were encountered: