[OID4VCI] TokenResponse requires credential_identifiers in authorization_details by tdiesler · Pull Request #47404 · keycloak/keycloak

tdiesler · 2026-03-24T14:39:39Z

keycloak-github-bot

Unreported flaky test detected, please review

forkimenjeckayang

A few concerns @tdiesler
Please take a look when you can

forkimenjeckayang · 2026-04-01T14:51:44Z

checkAuthorizationDetails() and validateAuthorizationDetail() apply different rules to request-time credential_identifiers (authorization rejects any presence, while the token path rejects only non-empty arrays), which can lead to divergent behavior (e.g., []).

Per Section 5.1.1, additional fields for openid_credential should not invalidate the authorization details type. Could we unify both paths by accepting this field on input and ignoring/clearing it, while ensuring token responses still emit server-authoritative credential_identifiers as defined in Section 6.2?

Yes, additional fields should not invalidate the authorization details. However, this is a field not totally unknown to the spec - it's a known field used in the wrong context.

On that basis, I suggest we align like this ...

if (credentialIdentifiers != null) { // we also reject an empty array of credential identifiers logger.warnf("Property credential_identifiers not allowed in authorization_details"); throw getInvalidRequestException("credential_identifiers not allowed"); }

It is unlikely that the conformance tests want to see the empty array pass.

WDYT?

mposolda

@tdiesler Thanks! Added comment inline (related also to other comment by @forkimenjeckayang ).

Also one question: As mentioned here in this comment #47386 (comment) , we do not need to support credential_configuration_id parameter in the credential-request. However in this PR it is still supported. Any issues with removing support for credential_configuration_id in this PR? Or do you rather want to do as a follow-up?

mposolda · 2026-04-07T14:30:56Z

+                throw new AuthorizationCheckException(Response.Status.BAD_REQUEST, Errors.INVALID_REQUEST,
+                        "Cannot parse authorization_details: " + authDetailsParam);
+            }
+            for (AuthorizationDetailsJSONRepresentation authDetailJson : authDetails) {


It will be nice if AuthorizationEndpointChecker does not have any direct dependencies on OID4VCI specific logic.

Is it possible to add new callback method to AuthorizationDetailsProcessorManager and AuthorizationDetailsProcessor, which will move this logic (lines 251-266) to the OID4VC specific processor? That can possibly help that we would be easily able to re-use same/similar validation like already done for token-endpoint? (As discussed in the other comment).

yes, that's how it should be (done)

tdiesler · 2026-04-08T10:24:06Z

Any issues with removing support for credential_configuration_id in this PR? Or do you rather want to do as a follow-up

It is infact not supported, because of ...

        // Verify that the requested credential_configuration_id in the request matches one in the authorization_details
        //
        if (!Strings.isEmpty(requestedCredentialConfigurationId)) {

            if (!authorizedCredentialConfigurationId.equals(requestedCredentialConfigurationId)) {
                var errorMessage = "Credential configuration '" + requestedCredentialConfigurationId + "' not found in authorization_details";
                LOGGER.debug(errorMessage);
                eventBuilder.detail(Details.REASON, errorMessage).error(ErrorType.UNKNOWN_CREDENTIAL_CONFIGURATION.getValue());
                throw new BadRequestException(getErrorResponse(ErrorType.UNKNOWN_CREDENTIAL_CONFIGURATION, errorMessage));
            }

            if (!authorizedCredentialIdentifiers.isEmpty()) {
                var errorMessage = "Credential must be requested by credential identifier from authorization_details: " + authorizedCredentialIdentifiers;
                LOGGER.debug(errorMessage);
                eventBuilder.detail(Details.REASON, errorMessage).error(ErrorType.INVALID_CREDENTIAL_REQUEST.getValue());
                throw new BadRequestException(getErrorResponse(ErrorType.INVALID_CREDENTIAL_REQUEST, errorMessage));
            }
        }

The conformance suite suite wants to see an UNKNOWN_CREDENTIAL_CONFIGURATION error, so we can't simply fail on any credential_configuration_id.

In our case, it always fails because we always return authorization_details from the token endpoint which MUST contain a credential_identifier. Remember, we decided to do it that way even for scope only requests that have no credential_identifiers configured on the scope - (in which case we generate the credential_identifier <= credential_configuration_id_0000).

This code would stay valid even if we decide to no longer return redundant authorization_details from the token endpoint

…ion_details Signed-off-by: Thomas Diesler <[email protected]>

@forkimenjeckayang

-- require well formed authorization details -- thanks @forkimenjeckayang Signed-off-by: Thomas Diesler <[email protected]>

Signed-off-by: Thomas Diesler <[email protected]>

mposolda

@tdiesler Thanks for the clarifications.

Leaving PR open for one more day in case @forkimenjeckayang has any additional feedback.

tdiesler · 2026-04-08T15:16:29Z

@mposolda FYI, this PR introduces a considerable amount of ugliness related to #47649. Invalid authorization requests are now caught early and will timeout with that Selenium exception - the root cause is lost. In case you prefer that this does not enter the code base, have a look at PR #47850. If that gets merged first, our tests could also be restored to handle their invalid auth requests properly.

git grep 'TODO #47649' 

tests/base/src/test/java/org/keycloak/tests/oid4vc/issuance/OID4VCAuthorizationCodeFlowTestBase.java:        // [TODO #47649] OAuthClient cannot handle invalid authorization requests
tests/base/src/test/java/org/keycloak/tests/oid4vc/issuance/OID4VCAuthorizationCodeFlowTestBase.java:        // [TODO #47649] OAuthClient cannot handle invalid authorization requests
tests/base/src/test/java/org/keycloak/tests/oid4vc/issuance/signing/OID4VCAuthorizationDetailsFlowTestBase.java:        // [TODO #47649] OAuthClient cannot handle invalid authorization requests
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oid4vc/issuance/signing/OID4VCAuthorizationCodeFlowWithPARTest.java:        // [TODO #47649] OAuthClient cannot handle i

In LoginUrlBuilder the relevant change is this ...

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

        // Login page may not get displayed (e.g. in case of an invalid AuthorizationRequest)
        String currUrl = client.driver.getCurrentUrl();
        if (currUrl != null && !currUrl.contains("error=") && !currUrl.contains("error_description=")) {
            client.fillLoginForm(username, password);
        }
        return client.parseLoginResponse();
    }

forkimenjeckayang

No further concerns, approach LGTM

mposolda · 2026-04-09T09:50:24Z

@tdiesler @forkimenjeckayang Thanks!

tdiesler requested review from a team as code owners March 24, 2026 14:39

keycloak-github-bot Bot added team/cloud-native team/core-clients team/core-iam labels Mar 24, 2026

tdiesler force-pushed the ghi47386 branch 2 times, most recently from 85752c2 to 4dc5080 Compare March 24, 2026 15:25

tdiesler requested a review from a team as a code owner March 24, 2026 15:25

keycloak-github-bot Bot added the team/ui label Mar 24, 2026

tdiesler force-pushed the ghi47386 branch 12 times, most recently from e682512 to cdceb4c Compare March 25, 2026 12:59

tdiesler mentioned this pull request Mar 25, 2026

[OID4VCI-HAIP] Pass oid4vci-1_0-issuer-happy-flow #47405

Closed

tdiesler force-pushed the ghi47386 branch 2 times, most recently from 7251c14 to f4ea1b4 Compare March 26, 2026 09:07

keycloak-github-bot Bot added the flaky-test label Mar 26, 2026

keycloak-github-bot Bot reviewed Mar 26, 2026

View reviewed changes

tdiesler force-pushed the ghi47386 branch 6 times, most recently from 421e8cb to 7139c38 Compare April 1, 2026 14:24

forkimenjeckayang suggested changes Apr 1, 2026

View reviewed changes

tdiesler force-pushed the ghi47386 branch 7 times, most recently from 4c14d33 to d503f21 Compare April 7, 2026 09:27

mposolda reviewed Apr 7, 2026

View reviewed changes

tdiesler force-pushed the ghi47386 branch from d503f21 to 6edb93a Compare April 8, 2026 10:13

tdiesler force-pushed the ghi47386 branch from 6edb93a to fd878ef Compare April 8, 2026 10:52

tdiesler added 2 commits April 8, 2026 12:52

[OID4VCI] TokenResponse requires credential_identifiers in authorizat…

504da48

…ion_details Signed-off-by: Thomas Diesler <[email protected]>

-- align credential identifiers check

6e3fc0f

-- require well formed authorization details -- thanks @forkimenjeckayang Signed-off-by: Thomas Diesler <[email protected]>

tdiesler force-pushed the ghi47386 branch 2 times, most recently from 25f5102 to ccab662 Compare April 8, 2026 11:47

-- unify authorization_details validation

9c3e25a

Signed-off-by: Thomas Diesler <[email protected]>

tdiesler force-pushed the ghi47386 branch from ccab662 to 9c3e25a Compare April 8, 2026 13:10

mposolda self-assigned this Apr 8, 2026

mposolda approved these changes Apr 8, 2026

View reviewed changes

forkimenjeckayang approved these changes Apr 9, 2026

View reviewed changes

mposolda merged commit 6fe5876 into keycloak:main Apr 9, 2026
84 checks passed

Conversation

tdiesler commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keycloak-github-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

forkimenjeckayang left a comment

Choose a reason for hiding this comment

Uh oh!

forkimenjeckayang Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

tdiesler Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

mposolda Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mposolda left a comment

Choose a reason for hiding this comment

Uh oh!

mposolda Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdiesler Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdiesler commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mposolda left a comment

Choose a reason for hiding this comment

Uh oh!

tdiesler commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

forkimenjeckayang left a comment

Choose a reason for hiding this comment

Uh oh!

mposolda commented Apr 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tdiesler commented Mar 24, 2026 •

edited

Loading

mposolda Apr 7, 2026 •

edited

Loading

tdiesler Apr 8, 2026 •

edited

Loading

tdiesler commented Apr 8, 2026 •

edited

Loading

tdiesler commented Apr 8, 2026 •

edited

Loading