Skip to content

[OID4VCI] Revisit invalid authorization requests handling#47850

Merged
mposolda merged 1 commit intokeycloak:mainfrom
tdiesler:ghi47649
Apr 23, 2026
Merged

[OID4VCI] Revisit invalid authorization requests handling#47850
mposolda merged 1 commit intokeycloak:mainfrom
tdiesler:ghi47649

Conversation

@tdiesler
Copy link
Copy Markdown
Contributor

@tdiesler tdiesler commented Apr 8, 2026 •
edited
Loading

closes #47649

This PR is about these three lines of code in LoginUrlBuilder ...

    public AuthorizationEndpointResponse doLogin(String username, String password) {
        open();
        client.fillLoginForm(username, password);
        return client.parseLoginResponse();
    }

It fixes the incorrect assumption that a login form can always be filled regardless of the http response resulting from sending an authorization request.

The second step is now conditioned on success of the first step.

    /**
     * Composite login method for the Authorization Code Flow
     *
     * <ol>
     *  <li>It builds and opens the authorization request url</li>
     *  <li>Fills the login form with user credentials (i.e. username, password)</li>
     *  <li>Parses the authorization response</li>
     * </ol>
     *
     * Step two is conditioned on successful processing of step one (i.e. the login page is displayed).
     * In case of error, step two is skipped and the authorization response is parsed directly. It then
     * contains the root cause of the authorization request error.
     *
     * @return An AuthorizationEndpointResponse
     */
    public AuthorizationEndpointResponse doLogin(String username, String password) {
        open();
        String currUrl = client.driver.getCurrentUrl();
        if (currUrl != null && !currUrl.contains("error=") && !currUrl.contains("error_description=")) {
            client.fillLoginForm(username, password);
        }
        return client.parseLoginResponse();
    }

A related discussion is here: #48308

Depends on

  1. [OID4VCI] Remove unwanted selenium exception handling #48312

@tdiesler tdiesler requested a review from a team as a code owner April 8, 2026 14:02
@tdiesler tdiesler force-pushed the ghi47649 branch 2 times, most recently from 1141e97 to 489d062 Compare April 8, 2026 14:13
@tdiesler tdiesler mentioned this pull request Apr 8, 2026
Merged
@tdiesler tdiesler requested review from a team as code owners April 9, 2026 10:23
@mposolda mposolda self-assigned this Apr 9, 2026
@mposolda
Copy link
Copy Markdown
Contributor

mposolda commented Apr 9, 2026

@tdiesler Replied on the issue #47649 (comment)

@tdiesler
Copy link
Copy Markdown
Contributor Author

tdiesler commented Apr 9, 2026 •
edited
Loading

This would also work for me ...

    public AuthorizationEndpointResponse doLogin(String username, String password) {
        open();

        String currUrl = client.driver.getCurrentUrl();
        if (currUrl.contains("error=") || currUrl.contains("error_description=")) {
            throw new AuthorizationException(...); // with root cause filled in from url params
        }

        return client.parseLoginResponse();
    }

However, information from all other url params would be lost.

@tdiesler tdiesler force-pushed the ghi47649 branch 2 times, most recently from 3b4fa83 to 9a75e83 Compare April 10, 2026 08:18
@mposolda
Copy link
Copy Markdown
Contributor

mposolda commented Apr 10, 2026

This would also work for me ...

    public AuthorizationEndpointResponse doLogin(String username, String password) {
        open();

        String currUrl = client.driver.getCurrentUrl();
        if (currUrl.contains("error=") || currUrl.contains("error_description=")) {
            throw new AuthorizationException(...); // with root cause filled in from url params
        }

        return client.parseLoginResponse();
    }

However, information from all other url params would be lost.

For me, this does not work. Please see #47649 (comment)

@tdiesler tdiesler force-pushed the ghi47649 branch 3 times, most recently from 39d4c67 to cd1dac7 Compare April 15, 2026 04:11
@mposolda
Copy link
Copy Markdown
Contributor

mposolda commented Apr 16, 2026

@tdiesler As you suggested yesterday, I've updated tdiesler#3 with the javadoc for LoginUrlBuilder.doLogin , which clarifies contract where/how is this method is supposed to be used. Does this work for you?

@tdiesler tdiesler force-pushed the ghi47649 branch 7 times, most recently from ec8b49a to a96188c Compare April 21, 2026 12:35
@tdiesler tdiesler changed the title OAuthClient cannot handle invalid authorization requests OAuthClient cannot handle invalid authorization requests (blocked) Apr 21, 2026
@tdiesler tdiesler force-pushed the ghi47649 branch 5 times, most recently from 15bbe31 to f7273fc Compare April 21, 2026 16:50
@keycloak-github-bot keycloak-github-bot Bot mentioned this pull request Apr 21, 2026
Open
@tdiesler tdiesler force-pushed the ghi47649 branch 2 times, most recently from 2a01e26 to 2454642 Compare April 22, 2026 06:40
keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed Apr 22, 2026
View reviewed changes
Copy link
Copy Markdown

@keycloak-github-bot keycloak-github-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

@keycloak-github-bot
Copy link
Copy Markdown

keycloak-github-bot Bot commented Apr 22, 2026

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.adapter.servlet.SAMLServletAdapterTest#employeeSigPostNoIdpKeyTestKeyIdAsKeyNameInKeyInfo

Keycloak CI - Adapter IT Strict Cookies

java.lang.NullPointerException: Cannot invoke "String.contains(java.lang.CharSequence)" because the return value of "org.openqa.selenium.WebDriver.getPageSource()" is null
	at org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest.assertForbiddenLogin(SAMLServletAdapterTest.java:558)
	at org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest.testSuccessfulAndUnauthorizedLogin(SAMLServletAdapterTest.java:591)
	at org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest.testSuccessfulAndUnauthorizedLogin(SAMLServletAdapterTest.java:584)
	at org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest.testSuccessfulAndUnauthorizedLogin(SAMLServletAdapterTest.java:580)
...

Report flaky test

@tdiesler tdiesler closed this Apr 22, 2026
@tdiesler tdiesler deleted the ghi47649 branch April 22, 2026 18:45
@tdiesler tdiesler restored the ghi47649 branch April 23, 2026 05:05
@tdiesler tdiesler reopened this Apr 23, 2026
@tdiesler tdiesler force-pushed the ghi47649 branch 2 times, most recently from 158a885 to 4066756 Compare April 23, 2026 05:40
@tdiesler tdiesler changed the title OAuthClient cannot handle invalid authorization requests (blocked) OAuthClient cannot handle invalid authorization requests Apr 23, 2026
@tdiesler tdiesler changed the title OAuthClient cannot handle invalid authorization requests [OID4VCI] Revisit invalid authorization requests handling Apr 23, 2026
@tdiesler
Copy link
Copy Markdown
Contributor Author

tdiesler commented Apr 23, 2026 •
edited
Loading

@mposolda This now incorporates your idea about low level APIs on the AuthorizationEndpointRequest

The supplier use case now looks like this ...

            AuthorizationEndpointRequest authRequest = authRequestSupplier.get();
            if (authRequest.openLoginForm()) {
                authRequest.fillLoginForm(ctx.getHolder(), TEST_PASSWORD);
            }
            AuthorizationEndpointResponse authResponse = authRequest.parseLoginResponse();

The check for early request parsing/integrity errors stays with AuthorizationEndpointRequest for now. Depending on whether that proves useful, we can promote it later or not.

The OID4VCPublicClientTest does this ...

        AuthorizationEndpointRequest authRequest = wallet
                .authorizationRequest()
                .scope(ctx.getScope());

        assertFalse(authRequest.openLoginForm(), "Error expected");
        AuthorizationEndpointResponse authResponse = authRequest.parseLoginResponse();

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

mposolda
mposolda approved these changes Apr 23, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tdiesler LGTM, Thanks!

@mposolda mposolda merged commit f91e6ef into keycloak:main Apr 23, 2026
85 checks passed
@tdiesler tdiesler deleted the ghi47649 branch April 23, 2026 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mposolda mposolda mposolda approved these changes

+1 more reviewer

@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@mposolda mposolda

Labels

flaky-test team/cloud-native team/core-clients team/core-iam

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

OAuthClient cannot handle invalid authorization requests

2 participants

@tdiesler @mposolda