Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.webauthn.registration.passwordless.PwdLessPubKeySignRegTest#publicKeySignaturesNonExisting

Keycloak CI - WebAuthn IT (chrome)

java.lang.AssertionError: Expected RegisterPage but was Sign in to test (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?response_type=code&client_id=test-app&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Fmaster%2Fapp%2Fauth&state=efbc9cae-1b0b-42cb-a4c9-8690e208d7fd&scope=openid)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.pages.AbstractPage.assertCurrent(AbstractPage.java:47)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
...

Report flaky test

@sonOfRa @thomasdarimont

Q1) My vote is to go with "Minimal ACR" instead of "Forced ACR" . IMO it is fine if client (or user) requests bigger value with the acr parameter, but he won't be allowed to request smaller one. Since Keycloak internally uses numeric values, it is easily possible to enforce this IMO. For example: I have "ACR to Loa" mapping defined at realm level like:

bronze -> 0
silver -> 1
gold -> 2
diamond -> 3

And then client uses Minimal ACR with the value gold . This means that client application (or user) will be able to request acr with the value diamond, which is stronger. He will be also able to request gold. But he won't be able to request silver or bronze.

I can see that if we go with "Minimal ACR", we can use single value rather than list?

I can also see that "Minimal ACR" will be the default level used in case that acr is not requested by client application and there are not Default ACR values configured for the client.

@sonOfRa Does this work for you?

Q2) My vote is to avoid realm-level for now. We can maybe add it as a follow-up if there is a need (maybe with the client policy as suggested by @thomasdarimont)? I can imagine that realm-level adds some additional complexity. Also if we go with the "Minimal ACR" approach (which is my preference), then I don't see much value in realm-level at all as administrator can always set the authentication flows in a way that this minimal level is the lowest one allowed by the authentication flow?

Q3) I agree with the @sonOfRa that we need to keep the "default ACR" as it is supported by OIDC client registration specification. @thomasdarimont I am sorry as I forgot that this is specification thing when we discussed on the KeyConf and when I mentioned the possibility that this can be removed. Considering that this is specification thing, we will probably need to keep it.

+1 to the following points mentioned by @sonOnfRa :

If the "minimal" strategy is chosen, we probably have to take into account the mapped LoA again, and check if any of the mapped LoA for any of the default_acr_values are below the minimum set, and reject the configuration as invalid based on that.

In general, I think rejecting config here as opposed to trying to make sense of seemingly confusing configuration is a benefit - it reduces the risk of accidentally misconfiguring the ACR/LoA settings and allowing in users with insecure login methods when we want to enforce a secure login

@sonOfRa I suggest to remove all the changes in OIDCClientRepresentation as it looks that forced_acr_values is not supported by any OIDC/OAuth specification? The class OIDCClientRepresentation is used just for the client registration used by the specification and not for adding Keycloak specific parameters.

That plan sounds good to me, our needs are covered by implementing it this way. Before I get started rewriting this PR, I have some questions/points on how the implementation should behave:

1. We should use ONE minimum value, rather than allowing multiple minimum values

2. When a client explicitly requests an ACR that is lower than the minimum, I think the best path is to silently "upgrade" the request, simply ignoring that the client requested a lower ACR (as opposed to aborting the request with an error). That means in getAcrValues, when a client sets a minimum ACR value, we can either just return List.of("minimumAcr"), or alternatively, return a list that contains all the values the client requested, minus the ones that are lower than the minimum ACR, plus the minimum ACR, if it is not already present. Both ways should yield the desired result, do you have a preference here?

3. Rejecting "invalid" configuration. This seemed simple at first, but there are several scenarios to keep in mind here:
   3.1. Updating a client is relatively straightforward, check either the realm ACR/LoA mapping or the one on the client if set, and reject the config if we can't find a LoA for the given minimum ACR value. When the LoA mapping is updated, we may also have to validate again and check if the minimum value (if present) is valid given the updated mapping
   3.2 When a realm's ACR/LoA mapping is updated, we would have to enumerate all clients and evaluate the minimum values of all clients for consistency. This seems expensive, and I feel like we might not want to do this.
   But if we don't actually validate the config when the realm is updated, we might also not want to validate when the client is updated, because it's more consistent that way?
   If we don't do any validation, we also have to consider behaviour if the configured minimum ACR is not mapped. Probably the most reasonable behaviour is simply retaining the current behaviour: Run into NumberFormatException in mapLoaToAcr and use -1 as the resulting LoA value and fall back to Keycloak's default behaviour

4. OIDCClientRepresentation handling. I added them to that class because the spec says "Additional Client Metadata parameters MAY also be used. Some are defined by other specifications, such as OpenID Connect RP-Initiated Logout 1.0.". I can remove them again if we don't want to expose Keycloak-specific settings here.

5. On avoiding realm level: I agree, not adding a realm wide flag makes things easier to implement, and a realm-wide implementation can easily be enforced by configuring the authentication flow to simply not providing a way to log in with a lower ACR than desired.

@sonOfRa Thanks for the discussion and for raising the follow-up clarifications! I am adding some answers inline, but overally, your ideas look nice to me.

That plan sounds good to me, our needs are covered by implementing it this way. Before I get started rewriting this PR, I have some questions/points on how the implementation should behave:

1. We should use ONE minimum value, rather than allowing multiple minimum values

+1

2. When a client explicitly requests an ACR that is _lower_ than the minimum, I think the best path is to silently "upgrade" the request, simply ignoring that the client requested a lower ACR (as opposed to aborting the request with an error). That means in `getAcrValues`, when a client sets a minimum ACR value, we can either just return `List.of("minimumAcr")`, or alternatively, return a list that contains all the values the client requested, _minus_ the ones that are lower than the minimum ACR, _plus_ the minimum ACR, if it is not already present. Both ways should yield the desired result, do you have a preference here?

This is interesting concern. My vote is that:

• In case that ACR is requested by claims parameter as "essential", then we reject the request if the requested claims cannot meet the minimum for the client
• In case that ACR is requested by claims or acr_values parameter as not "essential", then +1 to silently "upgrade" the request.

For the second case, I don't have much preference between List.of("minimumAcr") or removing lower ACRs from request than the minimum. Maybe I am slightly more for List.of("minimumAcr") as it is slightly less complex and error-prone :-) But for both cases, I suggest to add some logger.warn with mention that some acr values provided as parameters were ignored.

3. Rejecting "invalid" configuration. This seemed simple at first, but there are several scenarios to keep in mind here:
   3.1. Updating a client is relatively straightforward, check either the realm ACR/LoA mapping or the one on the client if set, and reject the config if we can't find a LoA for the given minimum ACR value. When the LoA mapping is updated, we may also have to validate again and check if the minimum value (if present) is valid given the updated mapping

+1

   3.2 When a realm's ACR/LoA mapping is updated, we would have to enumerate all clients and evaluate the minimum values of all clients for consistency. This seems expensive, and I feel like we might not want to do this.

+1 to not doing this expensive operation. I think we may already have this issue because if realm ACR-to-Loa mapping is updated, the client default ACR values may still have the original values, which may not be valid anymore.

   But if we don't actually validate the config when the realm is updated, we might also not want to validate when the client is updated, because it's more consistent that way?
   If we don't do any validation, we also have to consider behaviour if the configured minimum ACR is not mapped. Probably the most reasonable behaviour is simply retaining the current behaviour: Run into `NumberFormatException` in `mapLoaToAcr` and use `-1` as the resulting LoA value and fall back to Keycloak's default behaviour

+1 for the Keycloak default behaviour. ATM it is already done in a way that if non-existing claim is requested as "essential", we reject the request. If non-essential, we log the warning and continue https://github.com/keycloak/keycloak/blob/26.0.0/services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java#L329-L338

4. `OIDCClientRepresentation` handling. I added them to that class because the spec says "Additional Client Metadata parameters MAY also be used. Some are defined by other specifications, such as [OpenID Connect RP-Initiated Logout 1.0](https://openid.net/specs/openid-connect-registration-1_0.html#OpenID.RPInitiated).". I can remove them again if we don't want to expose Keycloak-specific settings here.

+1 for remove it from OIDCClientRepresentation. Right now, we use just parameters, which are present in either one of OIDC or OAuth2 specifications.

5. On avoiding realm level: I agree, not adding a realm wide flag makes things easier to implement, and a realm-wide implementation can easily be enforced by configuring the authentication flow to simply not providing a way to log in with a lower ACR than desired.

+1

Just pushed a starting point for the discussed changes. Currently writing some tests, but manual testing failed because the head of main branch currently won't start for me due to an exception:

❯ java -jar quarkus/server/target/lib/quarkus-run.jar start-dev --verbose
ERROR: Unexpected error when starting the server in (development) mode
Error details:
java.lang.RuntimeException: Failed to start quarkus
        at io.quarkus.runner.ApplicationImpl.<clinit>(Unknown Source)
        at java.base/jdk.internal.misc.Unsafe.ensureClassInitialized0(Native Method)
        at java.base/jdk.internal.misc.Unsafe.ensureClassInitialized(Unsafe.java:1160)
        at java.base/jdk.internal.reflect.MethodHandleAccessorFactory.ensureClassInitialized(MethodHandleAccessorFactory.java:300)
        at java.base/jdk.internal.reflect.MethodHandleAccessorFactory.newConstructorAccessor(MethodHandleAccessorFactory.java:103)
        at java.base/jdk.internal.reflect.ReflectionFactory.newConstructorAccessor(ReflectionFactory.java:200)
        at java.base/java.lang.reflect.Constructor.acquireConstructorAccessor(Constructor.java:549)
        at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
        at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:486)
        at io.quarkus.runtime.Quarkus.run(Quarkus.java:70)
        at org.keycloak.quarkus.runtime.KeycloakMain.start(KeycloakMain.java:146)
        at org.keycloak.quarkus.runtime.cli.command.AbstractStartCommand.run(AbstractStartCommand.java:57)
        at picocli.CommandLine.executeUserObject(CommandLine.java:2030)
        at picocli.CommandLine.access$1500(CommandLine.java:148)
        at picocli.CommandLine$RunLast.executeUserObjectOfLastSubcommandWithSameParent(CommandLine.java:2465)
        at picocli.CommandLine$RunLast.handle(CommandLine.java:2457)
        at picocli.CommandLine$RunLast.handle(CommandLine.java:2419)
        at picocli.CommandLine$AbstractParseResultHandler.execute(CommandLine.java:2277)
        at picocli.CommandLine$RunLast.execute(CommandLine.java:2421)
        at picocli.CommandLine.execute(CommandLine.java:2174)
        at org.keycloak.quarkus.runtime.cli.Picocli.run(Picocli.java:147)
        at org.keycloak.quarkus.runtime.cli.Picocli.parseAndRun(Picocli.java:135)
        at org.keycloak.quarkus.runtime.KeycloakMain.main(KeycloakMain.java:106)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
        at java.base/java.lang.reflect.Method.invoke(Method.java:580)
        at io.quarkus.bootstrap.runner.QuarkusEntryPoint.doRun(QuarkusEntryPoint.java:62)
        at io.quarkus.bootstrap.runner.QuarkusEntryPoint.main(QuarkusEntryPoint.java:33)
Caused by: java.lang.NoClassDefFoundError: oracle/sql/CharacterSet
        at io.quarkus.jdbc.oracle.runtime.OracleInitRecorder.setupCharSets(OracleInitRecorder.java:21)
        at io.quarkus.deployment.steps.ExtendedCharactersSupport$preinitializeCharacterSets1608602642.deploy_0(Unknown Source)
        at io.quarkus.deployment.steps.ExtendedCharactersSupport$preinitializeCharacterSets1608602642.deploy(Unknown Source)
        ... 27 more
Caused by: java.lang.ClassNotFoundException: oracle.sql.CharacterSet
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
        at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:526)
        at io.quarkus.bootstrap.runner.RunnerClassLoader.loadClass(RunnerClassLoader.java:114)
        at io.quarkus.bootstrap.runner.RunnerClassLoader.loadClass(RunnerClassLoader.java:72)
        ... 30 more

@thomasdarimont Do you want to review as well?

• Scenario that authentication request fails when requested essential claim with lower value than the "minimum" level?

• Scenario when attempt to update client fails with the attempt to use the "minimum" value, which is not mapped to any level?

I'll write some additional tests for those scenarios

Added the requested additional tests (and some more that seemed to make sense to me). Found a minor bug in the process where I only took into account numerical ACRs that directly match LoAs from authentication flows, which is fixed with the introduction of the getLoaForAcr(...) method.

Also changed the input to be a TextControl rather than MultiLineInput to reflect that it's a singular value, as well as added an updated screenshot of the advanced client configuration section from the admin UI to reflect the newly added field

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled

Keycloak CI - Java Distribution IT (windows-latest - temurin - 17)

java.lang.AssertionError: Did not find the event of expected type USER_DISABLED_BY_TEMPORARY_LOCKOUT. Events present: [IDENTITY_PROVIDER_POST_LOGIN_ERROR, IDENTITY_PROVIDER_POST_LOGIN_ERROR, IDENTITY_PROVIDER_POST_LOGIN_ERROR, IDENTITY_PROVIDER_POST_LOGIN_ERROR]
	at org.junit.Assert.fail(Assert.java:89)
	at org.keycloak.testsuite.AssertEvents$ExpectedEvent.assertEvent(AssertEvents.java:410)
	at org.keycloak.testsuite.broker.AbstractAdvancedBrokerTest.testPostBrokerLoginFlowWithOTP_bruteForceEnabled(AbstractAdvancedBrokerTest.java:586)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...

Report flaky test