Support to enforce LoA in authentication flow (Step-up)#16897
Support to enforce LoA in authentication flow (Step-up)#16897sventorben wants to merge 2 commits intokeycloak:mainfrom
Conversation
pedroigor
left a comment
There was a problem hiding this comment.
@sventorben Nice one :)
Test failures are related and seem related to changes in the number of built-in providers.
|
Thanks @pedroigor! I fixed the test cases. Anything else this would need? |
|
@sventorben Thanks. We need to provide a test for it. @mposolda May I ask you to continue reviewing and help with tests? |
mposolda
left a comment
There was a problem hiding this comment.
@sventorben Thanks for the PR. I've added one minor note inline. Besides that:
- I vote against
ConditionalClientIdAuthenticator. It doesn't scale as realms can have thousands of clients. We already discussed (and rejected) this idea some time ago during the work on "client policies". The idea is to instead use some other ways to make sure that particular client matches the condition. For example it can be "marker" client role added to client or some kind of "marker" client scope.
This also allows that "client administrator" is not forced to contact "global realm administrator" to update authentication flow always when client wants to "enforce loa".
- Regarding
ForceLoAAuthenticator, I would also prefer this to be rather "client policy executor" instead of "Authenticator". This will allow to re-use all existing conditions provided by current client policies instead of add new dedicated condition implementation to the authentication flow.
I see the possible issue that authenticationSession is not available when AUTHORIZATION_REQUEST client policy is triggered (on AuthorizationEndpoint - line 202). However hopefully this can be mitigated by either:
- rewrite
claimsparameter and add enforced level to "essential" claims (if bigger than already provided inclaimsparameter) - add some
contextData<String, Object>map toAuthorizationEndpointRequest. Client policy executor can set some attribute to it like MIN_ENFORCED_LOA_LEVEL. Then AuthorizationEndpoint (part of the codebase where Loa is parsed) can consume this. - add dedicated client policy event type like POST_AUTHORIZATION_REQUEST, which can be triggered after authenticationSession is parsed and available.
I think (2) is my favourite right now, but any of those works (whatever you find easiest).
- Tests are possible in
LevelOfAssuranceFlowTestto test step-up. Tests for client policies are in packageorg.keycloak.testsuite.client.policies- for inspiration how to setup client policies (if we go this path).
| } | ||
| for (String configuredAcr : defaultAcrValues) { | ||
| int loa; | ||
| if (acrToLoaMap.containsKey(configuredAcr)) { |
There was a problem hiding this comment.
Minor: Is it possible to add util method to AcrUtils like:
Integer getMappedLoad(String loa, Map<String, Integer> acrLoaMap);
and make the code in lines 50-59 to use that util method? For that case, it will be also good to update method AcrUtils.mapLoaToAcr (lines around 132-141) to use your new method. Just trying to avoid using same/similar logic on multiple places around codebase.
There was a problem hiding this comment.
Yes, sure. Will do.
|
@mposolda Regarding the usage of a "marker" client role or "marker" client scope, there is a |
Yes, true. I think the SAML clients were not yet used with client policies. But if you need SAML, you can possibly add some "execution point" somewhere to the SamlEndpoint (right before authentication is triggered). I think that some conditions like for example
@sventorben Nope, it is not right now. That's also one of the reasons why I am more towards the "Client Policies" path, because the conditions are already available here. So only new thing needed here is the implementation of the executor though. |
|
@mposolda I have a use case in which this being an authenticator came in handy. I wanted to force an LoA after the "selected user" was determined (such as after a username/password form being completed) in an authentication flow. Not sure if I have access to user attributes/metadata in a client policy. |
|
@sventorben Thanks for the PR and sorry for the late feedback. In the end, we ended with the approach of #33205 . If it doesn't met your needs, feel free to create a follow-up issue if needed. |
Please see #16884 for details.
Closes #16884