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.model.infinispan.EmbeddedInfinispanSplitBrainTest#testLocalCacheClearedOnMergeEvent

Keycloak CI - Store Model Tests

java.lang.AssertionError
	at org.junit.Assert.fail(Assert.java:87)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertTrue(Assert.java:53)
	at org.keycloak.testsuite.model.infinispan.EmbeddedInfinispanSplitBrainTest.awaitLatch(EmbeddedInfinispanSplitBrainTest.java:112)
...

java.lang.AssertionError
	at org.junit.Assert.fail(Assert.java:87)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertTrue(Assert.java:53)
	at org.keycloak.testsuite.model.infinispan.EmbeddedInfinispanSplitBrainTest.awaitLatch(EmbeddedInfinispanSplitBrainTest.java:112)
...

Report flaky test

Sending an Authorization request with low level LoginForm API looks like this

        oauth.loginForm().scope(credScopeName).authorizationDetails(authDetail).open();
        oauth.fillLoginForm(appUsername, "password");
        AuthorizationEndpointResponse authResponse = oauth.parseLoginResponse();

It is awkward, imho

We can preserve the use of LoginForm (i.e. not send an HttpGetRequest from another AuthorizationRequestRequest) and at the time align with the general OAuthClient API like this ...

        AuthorizationEndpointResponse authResponse = oauth
                .authorizationRequest()
                .scope(credScopeName)
                .authorizationDetails(authDetail)
                .send(appUsername, "password");
        String authCode = authResponse.getCode();

Note, this would ...

• not be oid4vci specific
• authorization_details is added to LoginForm
• the response object is the same as with the LoginForm API
• using the LoginForm API works as before (i.e. no change to code that uses it)

Would that be acceptable?

@tdiesler Thanks for the updates. It is probably subjective what is awkward. I am not very sure for a new way with AuthorizationEndpointRequest to be much better and moreover:

• We would have multiple ways to achieve the same thing with the API
• The way of AuthorizationEndpointRequest supports only very specific case when login happens with username/password and redirects to the client straight after. Does not support more complex scenarios (EG. if additional authentication like OTP is required, or login with social, flows with forget password etc)
• AFAIK it is by design that some parts (like clientId) are configured in the OAuth client rather than for the specific request. As it helps with the subsequent requests that stuff like clientId is configured on OAuthClient and does not need to be configured again for the particular test
• The clientSecret is certainly not needed on AuthorizationEndpointRequest ? Client authentication usually happens in the later stage (during token-request rather than during OIDC authentication request).

I will check with the rest of the team as would like more feedback on OAuth client API used in many tests.

AFAIK the minimal needed would be to add authorizationDetails (and eventually also issuerState if you prefer) to the LoginUrlBuilder .

• The way of AuthorizationEndpointRequest supports only very specific case when login happens with username/password and redirects to the client straight after. Does not support more complex scenarios (EG. if additional authentication like OTP is required, or login with social, flows with forget password etc)

It allows us to write code like this

        AuthorizationEndpointResponse authResponse = oauth
                .authorizationRequest()
                .scope(credScopeName)
                .authorizationDetails(authDetail)
                .send(holder, "password");
        String authCode = authResponse.getCode();
        assertNotNull(authCode, "No authCode");

If you remember, I originally did not wan to have it on oauth but on oauth.oid4cv() so it does not get exposed to erveryone before we are sure that it is ready.

In a follow up, I want to add support for improved error handling

    public AuthorizationEndpointResponse send(String username, String password) {
        initRequest();
        loginForm.open();
        AuthPageState authPageState = waitForLoginOrError();
        switch (authPageState) {
            case ERROR_REDIRECT -> {
                return new AuthorizationEndpointResponse(client);
            }
            case ERROR_PAGE -> {
                Map<String, String> params = new HashMap<>();
                params.put("error", "invalid_request");
                client.driver.findElements(By.id("kc-error-message")).stream()
                        .map(d -> d.getText().trim().split("\\n")[0])
                        .forEach(s -> params.put("error_description", s));
                return new AuthorizationEndpointResponse(params);
            }
            // AuthPageState.LOGIN
            default -> {
                client.fillLoginForm(username, password);
                return new AuthorizationEndpointResponse(client);
            }
        }
    }

also preferable for oid4vc only at first.

Still later, I want to add IDToken Authorization #44657

We need this abstraction for a number of reasons and ideally we would have it for oid4vc only and later promote it to oauth when it needed.

Does not support more complex scenarios (EG. if additional authentication like OTP is required, or login with social, flows with forget password etc)

Yes, these are not supported nor needed in our current testsuite migration effort

• AFAIK it is by design that some parts (like clientId) are configured in the OAuth client rather than for the specific request. As it helps with the subsequent requests that stuff like clientId is configured on OAuthClient and does not need to be configured again for the particular test

That remains the default.

• The clientSecret is certainly not needed on AuthorizationEndpointRequest ? Client authentication usually happens in the later stage (during token-request rather than during OIDC authentication request).

Yes, I'm sorry that needs to be removed.

AFAIK the minimal needed would be to add authorizationDetails (and eventually also issuerState if you prefer) to the LoginUrlBuilder .

Yes, that is already done so

How about, I move it back to oid4vc? It would not do any harm there and could unblock the testsuite migration PR.

This is the right way of doing this:

        oauth.loginForm().scope(credScopeName).authorizationDetails(authDetail).open();
        oauth.fillLoginForm(appUsername, "password");
        AuthorizationEndpointResponse authResponse = oauth.parseLoginResponse();

Just do it that way and you're all good?

That basically goes hand in hand with OAuth (app sends authz request, the user fills the login form aka authenticates, and you move on with oauth stuff).

User may already be authenticated, may not use password only, may need to have some required actions, etc. That is basically the reason it is done the way it is currently.