8000 Fix scope validation for realm-level credential definitions in Authorization Code flow by Awambeng · Pull Request #39148 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix scope validation for realm-level credential definitions in Authorization Code flow #39148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
May 7, 2025
Merged

Fix scope validation for realm-level credential definitions in Authorization Code flow #39148

merged 4 commits into from
May 7, 2025

Conversation

Awambeng
Copy link
Contributor
@Awambeng Awambeng commented Apr 23, 2025 •
edited
Loading

This PR updates the checkScope method in the OID4VC credential issuance flow to fetch the required scope from realm-level attributes instead of client attributes. This change ensures compatibility with the new design where credential definitions are no longer client-scoped but managed at the realm level.

This resolves scope validation failures during credential issuance using the Authorization Code flow

Closes #39130

@Awambeng
Refactor scope check to support realm-level credential definitions
e69611c 
Updated scope retrieval to use realm attributes instead of client attributes
Closes keycloak#39130

Signed-off-by: Awambeng Rodrick <awambengrodrick@gmail.com>
@Awambeng Awambeng requested a review from a team as a code owner April 23, 2025 11:35
@tnorimat
Copy link
Contributor

@Awambeng Hello, I will review the PR. Thank you.

tnorimat
tnorimat suggested changes Apr 27, 2025
View reviewed changes
Copy link
Contributor
@tnorimat tnorimat left a comment

Choose a reason for hiding this comment

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

Firstly, it seems that the PR lacks its integration tests to check the modified codes.
Could you add the integration tests?

@Awambeng
test(oid4vc): add tests for realm scope validation (matched, unmatche…
3506b3b 
…d, missing) in credential issuance

Signed-off-by: Awambeng Rodrick <awambengrodrick@gmail.com>
@Awambeng Awambeng requested review from a team as code owners April 28, 2025 16:24
@Awambeng
Copy link
Contributor Author

Thank you @tnorimat for the feedback. I’ve added tests to cover the new scope validation logic at the realm level. The tests include the following cases:

  1. testCredentialIssuanceWithAuthZCodeWithScopeMatched: Verifies that credential issuance succeeds when the access token contains the correct scope as defined in the realm attribute.

  2. testCredentialIssuanceWithRealmScopeUnmatched: Verifies that credential issuance fails with the expected error when the access token contains a scope that does not match the realm attribute.

  3. testCredentialIssuanceWithRealmScopeMissing: Verifies that credential issuance fails with the expected error when the required scope attribute is missing from the realm.

@Awambeng Awambeng requested a review from tnorimat April 28, 2025 16:30
@Awambeng Awambeng mentioned this pull request Apr 28, 2025
Closed
1 task
wistefan
wistefan approved these changes Apr 30, 2025
View reviewed changes
Copy link
Contributor
@wistefan wistefan left a comment

Choose a reason for hiding this comment

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

LGTM

tnorimat
tnorimat approved these changes May 1, 2025
View reviewed changes
Copy link
Contributor
@tnorimat tnorimat left a comment

Choose a reason for hiding this comment

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

LGTM

@tnorimat
Copy link
Contributor
tnorimat commented May 1, 2025

@wistefan Thank you for reviewing the PR.
@mposolda The PR only affects experimental OID4VCI feature and does not affect other parts. Therefore merging the PR is safe. Could you check the PR?

Captain-P-Goldfish
Captain-P-Goldfish approved these changes May 2, 2025
View reviewed changes
Copy link
Contributor
@Captain-P-Goldfish Captain-P-Goldfish left a comment

Choose a reason for hiding this comment

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

The changes are likely to be removed again under the condition, that we should decide to implement my suggested changes in #39265.
But until then the PR looks good to me.

@mposolda mposolda self-assigned this May 7, 2025
@mposolda mposolda added the area/oid4vc Issue related to OpenID for Verifiable Credentials label May 7, 2025
mposolda
mposolda approved these changes May 7, 2025
View reviewed changes
Copy link
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.

@Awambeng @tnorimat @Captain-P-Goldfish @wistefan Thanks everyone for the PR and the review!

@mposolda mposolda merged commit ca3859b into keycloak:main May 7, 2025
76 checks passed
@Awambeng Awambeng deleted the issue-39130 branch May 9, 2025 10:28
mposolda pushed a commit to mposolda/keycloak that referenced this pull request May 16, 2025
@Awambeng @mposolda
Fix scope validation for realm-level credential definitions in Author…
a7f38de 
…ization Code flow (keycloak#39148)

Closes keycloak#39130

Signed-off-by: Awambeng Rodrick <awambengrodrick@gmail.com>
(cherry picked from commit ca3859b)
mposolda pushed a commit to mposolda/keycloak that referenced this pull request May 19, 2025
@Awambeng @mposolda
Fix scope validation for realm-level credential definitions in Author…
9560177 
…ization Code flow (keycloak#39148)

Closes keycloak#39130

Signed-off-by: Awambeng Rodrick <awambengrodrick@gmail.com>
(cherry picked from commit ca3859b)
mposolda pushed a commit that referenced this pull request May 21, 2025
@Awambeng @mposolda
Fix scope validation for realm-level credential definitions in Author…
60445d2 
…ization Code flow (#39148)

Closes #39130

Signed-off-by: Awambeng Rodrick <awambengrodrick@gmail.com>
(cherry picked from commit ca3859b)
@stianst stianst mentioned this pull request Jun 3, 2025
Merged
@ryanemerson ryanemerson mentioned this pull request Jun 20, 2025
Merged
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

@Captain-P-Goldfish Captain-P-Goldfish Captain-P-Goldfish approved these changes

@wistefan wistefan wistefan approved these changes

@tnorimat tnorimat tnorimat approved these changes

Assignees

@mposolda mposolda

Labels
area/oid4vc Issue related to OpenID for Verifiable Credentials 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.

Authorization Code Flow Fails Scope Validation After Credential Definition Migration to Realm Level
5 participants
@Awambeng @tnorimat @mposolda @Captain-P-Goldfish @wistefan
0