OAuthClient cannot handle invalid authorization requests #48308

tdiesler · 2026-04-21T08:18:09Z

tdiesler
Apr 21, 2026

This discussion is about invalid authorization requests and error error handling thereof.

Happy Flow

Lets consider the easy case whereby there need to be some additional parameters on the authorization request.
In this case code_challenge and code_challenge because the client requires it.

        AuthorizationEndpointResponse authResponse = oauth.loginForm()
                .scope("jwt-credential")
                .codeChallenge(pkce)
                .doLogin("alice", TEST_PASSWORD);

        assertNull(authResponse.getError(), "No error");
        assertNotNull(authResponse.getCode(), "Has auth code");

The doLogin method internally executes this sequence ...

        open();
        client.fillLoginForm(username, password);
        client.parseLoginResponse();

Error Flow - Option A

Let's assume we want to test the error response when the required code_challenge is not given.

The authorization request is invalid, open() does not display a login page, which doLogin() ignores, the root cause of the
server error is lost.

        try {
            AuthorizationEndpointResponse authResponse = oauth.loginForm()
                    .scope("jwt-credential")
                    .doLogin("alice", TEST_PASSWORD);
            fail("Selenium exception expected");
        } catch (org.openqa.selenium.NoSuchElementException ex) {
            // ignore selenium exception
        }

Option A (the current state of affairs) states that the caller "MUST know the outcome of the authorization request in advance and use the API accordingly".

Hence, doLogin must not be used ...

        oauth.loginForm()
                .scope("jwt-credential")
                .open();
        AuthorizationEndpointResponse authResponse = oauth.parseLoginResponse();

        assertNull(authResponse.getCode(), "Expected no auth code");
        assertEquals("invalid_request", authResponse.getError());
        assertEquals("Missing parameter: code_challenge_method", authResponse.getErrorDescription());

This effectively skips the second step client.fillLoginForm(username, password) because the caller expects that open() returns an error response.

Error Flow - Option B

Option B states that the caller should not be required to know the outcome of the authorization request in advance. Instead, the API is expected to handle the error case internally.

        AuthorizationEndpointResponse authResponse = oauth.loginForm()
                .scope("jwt-credential")
                .doLogin("alice", TEST_PASSWORD);

        assertNull(authResponse.getCode(), "Expected no auth code");
        assertEquals("invalid_request", authResponse.getError());
        assertEquals("Missing parameter: code_challenge_method", authResponse.getErrorDescription());

The doLogin method handles the error internally, conceptually like this ...

        if (open()) {
            client.fillLoginForm(username, password);
        }
        client.parseLoginResponse();

In summary this discussion is about nothing more than making the call to fillLoginForm(username, password) conditional on the successful display of the login page.

stianst · 2026-04-22T04:55:14Z

stianst
Apr 22, 2026
Maintainer

As mentioned before doLogin is expected to succeed, and not to fail. What you are proposing is actually breaking the API, and also making things harder.

I don't really understand what problem you are trying to solve here; if you are writing a test that is not expected to show a login form, well then don't make it try to fill the login form by using doLogin. You certainly shouldn't be catching the exception like you are showing in Option A.

0 replies

tdiesler · 2026-04-22T06:09:57Z

tdiesler
Apr 22, 2026
Author

We have code that configures the authorization request in non-trivial ways for example by providing authorization details that might be invalid. Depending on whether open() succeeds, we fill in the login credentials and carry on or process the error and go an alternative path.

From a pure Java perspective, it is such that fillLoginForm() has pre conditions, namely that open() has succeeded. In the language, it is rarely such that the client is asked to "only call a method when you can guarantee that is succeeds". Instead, the client can usually expect an exception or an error response.

The problem I'm trying to solve is to make doLogin() aware of whether open() actually worked and therefore allow it to take appropriate action, which in all cases results in an AuthorizationEndpointResponse.

IMHO, open(), because it is quite a complex operation on the server side, should in some way indicate whether it succeeded or not. This could be done by ...

throwing an exception with the root cause of why it failed
returning an appropriate success/error response

In the case of authorization code flow, open() has failed when there is a server side error and because selenium's navigate() is synchronous it also knows instantly that it has failed. This information should be passed on to the caller and hence allow it to take appropriate action. In this case doLogin() may not want to call fillLoginForm() (i.e. the pre-condition is not met) but pass the error through to its caller.

0 replies

tdiesler · 2026-04-22T06:33:11Z

tdiesler
Apr 22, 2026
Author

Perhaps you could let us know why adding this simple pre-condition check

is actually breaking the API, and also making things harder

It is well possible that I'm missing a crucial part here.

AFAICT, existing code will not be effected and future code will not have to do things differently. There are about 100 places where doLogin() is called - all of them send valid authorization requests.

Code that deals with invalid authorization requests already calls open() and then somehow parses the result it gets from the driver. What I'm proposing will likely have to happen in all places that deal with invalid authorization requests.

0 replies

mposolda · 2026-04-22T07:44:46Z

mposolda
Apr 22, 2026
Maintainer

@tdiesler +1 to what @stianst mentioned.

The method oauth.doLogin(username, password) is intended to be used for the cases when test wants to do all 3 actions of:

Open login form
Fill username/password
Being redirected to the client right after that (with AuthorizationEndpointResponse).

For any other use-case (EG. Opening login form, which is followed directly by redirect to the client without username/password screen being shown), the different methods should be used (EG. for that case, direct use of oauth.open() followed by oauth.parseLoginResponse()).

Using doLogin(username, password) for the case when login form is not even shown is unclear from the API perspective. Also it is very confusing as when looking to the code of the test, which calls doLogin(username, password) it would not be clear to me if login form was even shown or not.

There is no reason why OID4VCI tests should do things differently than all other Keycloak tests. I've sent you the PR to your branch to fix the OID4VCI tests , which updates your PR to Keycloak .

4 replies

tdiesler Apr 22, 2026
Author

I hear what you're saying - we now have PR #48312 that leaves everything as is and only cleans up the unwanted selenium handling (that should not have been added in the first place)

There is one section of code in OID4VCAuthorizationDetailsFlowTestBase that is not ideal ...

            AuthorizationEndpointRequest authRequest = authRequestSupplier.get();
            authRequest.openLoginForm();
            String currUrl = oauth.getDriver().getCurrentUrl();
            if (currUrl != null && !currUrl.contains("error=") && !currUrl.contains("error_description=")) {
                oauth.fillLoginForm(ctx.getHolder(), TEST_PASSWORD);
            }
            AuthorizationEndpointResponse authResponse = oauth.parseLoginResponse();

The required condition check has moved out as far as the test case itself. That works for me as long as we don't see that repeated over and over.

As you can see the AuthorizationEndpointRequest comes from a supplier, the code cannot know the outcome of open() before it makes the call.

The question for me is: how do we handle the case when the authorization request is been given to a piece of code that cannot know in advance whether it is correct or not - we will have to deal with that question somewhere.

tdiesler Apr 22, 2026
Author

When looking at the above, its important to realize that openLoginForm() already has the information about whether it succeeded or not, only that the caller does not get it.

mposolda Apr 22, 2026
Maintainer

I've merged the #48312 as it is better than what we have in main IMO. Thanks for that PR!

+1 that the code you mentioned in OID4VCAuthorizationDetailsFlowTestBase is still not ideal.

As you can see the AuthorizationEndpointRequest comes from a supplier, the code cannot know the outcome of open() before it makes the call.

The question for me is: how do we handle the case when the authorization request is been given to a piece of code that cannot know in advance whether it is correct or not - we will have to deal with that question somewhere.

I think the test should always know what it expects and whether it expects login-form to be shown or not.

I've addressed the problem you mentioned in the OID4VCAuthorizationDetailsFlowTestBase in my PR to your branch tdiesler#3 . It is solved by adding another function authzResponseSupplier to the method runAuthorizationDetailsTest, so test can "hint" whether it expects login-form or not . So many "suppliers" and "functions" are maybe not ideal for the runAuthorizationDetailsTest , but that is the problem already on that method (question is, whether it is not easier to just avoid that method runAuthorizationDetailsTest and rather extract the things into the individual tests themselves, but not 100% sure it is easier or not)

tdiesler Apr 22, 2026
Author

I think the test should always know what it expects and whether it expects login-form to be shown or not.

Ok, I'll go with that notion for now - in which case we indeed need to pass the auth request around together with a flag that marks it as invalid. The server side AuthorizationEndpointRequest has a similar helper property (i.e. invalidRequestMessage)

tdiesler · 2026-04-22T18:32:28Z

tdiesler
Apr 22, 2026
Author

I believe here is another example that runs into a selenium exception

    public void testWrongEncryptionAlgorithm() throws Exception {
        try {
            ClientResource clientResource = AdminApiUtil.findClientByClientId(adminClient.realm(oauth.getRealm()), oauth.getClientId());
            ClientRepresentation clientRep = clientResource.toRepresentation();
            OIDCAdvancedConfigWrapper.fromClientRepresentation(clientRep).setRequestObjectEncryptionAlg(RSA_OAEP_256);
            clientResource.update(clientRep);
            oauth.loginForm().request(createEncryptedRequestObject(RSA_OAEP)).doLogin("test-user@localhost", "password");
            fail("Should fail due to invalid encryption algorithm");
        } catch (Exception ignore) {
            assertTrue(errorPage.isCurrent());
            oauth.loginForm().request(createEncryptedRequestObject(RSA_OAEP_256)).doLogin("test-user@localhost", "password");
            assertTrue(appPage.isCurrent());
        }

Its easy to spot those because they always run into a timeout (i.e. 10sec) before they throw that ugly selenium exception.

Nevermind for now, in the the not so distant future we will have non-interactive authorization requests that might even replace code-flow authorization for oid4vci.

With code flow, there are two classes of error that can happen ...

invalid request before the login page is even shown (e.g. parsing errors, request & request_uri validation, etc)
errors following the actual login (i.e. insufficient privileges, invalid for client/scope configuration, etc)

In headless environments it would be important to fail early - the response cannot be an html error page.

In the fapi2 conformance test suite, there are a bunch of test modules that verify that there is a proper error redirect to the client (i.e. error + error_description on the redirect_uri)

I still think the first class of errors can/should be handled by open() - it can see those early validation errors and it would be better to pass them through rather than ignore them. Anyway, I'll see how it goes with fapi2 - there is likely some work to be done with auth request parsing and error handling since the conformance test suite does not accept html error pages for the automated run.

1 reply

mposolda Apr 23, 2026
Maintainer

In general: the test should know whether it is supposed to show the error-page, redirect to the client or have the login form. IMHO that is one of the points of the "Fluent API" and composition used here. Caller can use something like open() followed by other things when you expect login-page to be shown. And caller use doLogin(username, password just in case that login form is really expected to be shown and redirect to the client right after submit username/password.

The test you mentioned can be probably rewritten to something like this (did not tried to run it, just for illustration). This doesn't use any timeout:

    public void testWrongEncryptionAlgorithm() throws Exception {
       
            ClientResource clientResource = AdminApiUtil.findClientByClientId(adminClient.realm(oauth.getRealm()), oauth.getClientId());
            ClientRepresentation clientRep = clientResource.toRepresentation();
            OIDCAdvancedConfigWrapper.fromClientRepresentation(clientRep).setRequestObjectEncryptionAlg(RSA_OAEP_256);
            clientResource.update(clientRep);
            
            // Test incorrect algorithm. Error page shown
            oauth.loginForm().request(createEncryptedRequestObject(RSA_OAEP)).open();
           errorPage.assertCurrent();

            // Test correct algorithm
            oauth.loginForm().request(createEncryptedRequestObject(RSA_OAEP_256)).doLogin("test-user@localhost", "password");
            appPage.assertCurrent();
}

OAuthClient cannot handle invalid authorization requests #48308

Uh oh!

tdiesler Apr 21, 2026

Happy Flow

Error Flow - Option A

Error Flow - Option B

Replies: 5 comments · 5 replies

Uh oh!

Uh oh!

stianst Apr 22, 2026 Maintainer

Uh oh!

Uh oh!

tdiesler Apr 22, 2026 Author

Uh oh!

Uh oh!

tdiesler Apr 22, 2026 Author

Uh oh!

Uh oh!

mposolda Apr 22, 2026 Maintainer

Uh oh!

Uh oh!

tdiesler Apr 22, 2026 Author

Uh oh!

tdiesler Apr 22, 2026 Author

Uh oh!

mposolda Apr 22, 2026 Maintainer

Uh oh!

tdiesler Apr 22, 2026 Author

Uh oh!

tdiesler Apr 22, 2026 Author

Uh oh!

mposolda Apr 23, 2026 Maintainer

tdiesler
Apr 21, 2026

Replies: 5 comments 5 replies

stianst
Apr 22, 2026
Maintainer

tdiesler
Apr 22, 2026
Author

tdiesler
Apr 22, 2026
Author

mposolda
Apr 22, 2026
Maintainer

tdiesler Apr 22, 2026
Author

tdiesler Apr 22, 2026
Author

mposolda Apr 22, 2026
Maintainer

tdiesler Apr 22, 2026
Author

tdiesler
Apr 22, 2026
Author

mposolda Apr 23, 2026
Maintainer