8000 [OID4VCI] Revisit invalid authorization requests handling by tdiesler · Pull Request #47850 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

[OID4VCI] Revisit invalid authorization requests handling - #47850

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

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

Conversation

@tdiesler
@tdiesler tdiesler commented Apr 8, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor

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
mposolda commented Apr 9, 2026
Copy link
Copy Markdown
Contributor

@tdiesler Replied on the issue #47649 (comment)

@tdiesler
tdiesler commented Apr 9, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

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
mposolda commented Apr 10, 2026
Copy link
Copy Markdown
Contributor

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
mposolda commented Apr 16, 2026
Copy link
Copy Markdown
Contributor

@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
Closed
@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
@keycloak-github-bot keycloak-github-bot Bot left a comment
Copy link
Copy Markdown

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
keycloak-github-bot Bot commented Apr 22, 2026
Copy link
Copy Markdown

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
[OID4VCI] Revisit invalid authorization requests handling
c499f84 
Signed-off-by: Thomas Diesler <tdiesler@proton.me>
@tdiesler tdiesler changed the title OAuthClient cannot handle invalid authorization requests [OID4VCI] Revisit invalid authorization requests handling Apr 23, 2026
@tdiesler
tdiesler commented Apr 23, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

@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
@mposolda mposolda left a comment
Copy link
Copy Markdown
Contributor

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
@stianst stianst mentioned this pull request Jun 1, 2026
Closed
sashyo pushed a commit to tide-foundation/keycloak-IGA that referenced this pull request Jul 20, 2026
@tdiesler @sashyo
[OID4VCI] Revisit invalid authorization requests handling (keycloak#4…
a5f8b92 
…7850)

closes keycloak#47649

Signed-off-by: Thomas Diesler <tdiesler@proton.me>
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
0