-
Notifications
You must be signed in to change notification settings - Fork 3
[Keycloak-OID4VCI]: Add support for parsing and understanding authorization_details at the Token Endpoint #54
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.
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
base: main
Are you sure you want to change the base?
Conversation
ed438a9 to
57e34ee
Compare
services/src/main/java/org/keycloak/protocol/oid4vc/model/AuthorizationDetail.java
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oid4vc/model/AuthorizationDetailResponse.java
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oid4vc/model/AuthorizationDetailResponse.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oid4vc/model/AuthorizationDetail.java
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantType.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantType.java
Show resolved
Hide resolved
stephane-segning
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. My remarks are mostly comments and worries.
-
Response Structure and Compatibility
- Downstream clients expecting the previous token response format may break if they can’t handle the new authorization_details field.
- Future changes to the AuthorizationDetailResponse structure could cause incompatibility
- => Include comprehensive API changelogs and documentation updates & ensure strong versioning and feature toggling if needed.
-
Logical/Behavioral Regression
- => Consider additional concurrency testing under load
-
Code Maintainability & Extensibility
-
Error Handling & Feedback
- => Log internal context server-side, but give minimal info in the API response
-
Input Validation & Parsing
- => Use strict Jackson settings (fail on unknown, max depth/size)
- => Consider adding a maximum array/object depth or length to avoid DoS attacks 🤔
services/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oid4vc/model/AuthorizationDetail.java
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oid4vc/model/AuthorizationDetailResponse.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oid4vc/model/AuthorizationDetailResponse.java
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java
Outdated
Show resolved
Hide resolved
|
Permit me to continue with the review on monday, as my health doesn't allow me to continue with the review today. |
Awambeng
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the changes. I have two minor comments for you to take a look at
services/src/main/java/org/keycloak/protocol/oid4vc/model/AuthorizationDetail.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oid4vc/model/AuthorizationDetailResponse.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oid4vc/model/AuthorizationDetailResponse.java
Outdated
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oid4vc/model/AuthorizationDetailResponse.java
Outdated
Show resolved
Hide resolved
...test/java/org/keycloak/testsuite/oid4vc/issuance/signing/OID4VCSdJwtIssuingEndpointTest.java
Outdated
Show resolved
Hide resolved
...test/java/org/keycloak/testsuite/oid4vc/issuance/signing/OID4VCSdJwtIssuingEndpointTest.java
Show resolved
Hide resolved
services/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/keycloak/testsuite/oid4vc/issuance/signing/OID4VCJWTIssuerEndpointTest.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/keycloak/testsuite/oid4vc/issuance/signing/OID4VCJWTIssuerEndpointTest.java
Outdated
Show resolved
Hide resolved
|
I confirm that my changes has been resolve correctly, and i'll proceed by approving. |
d0c3644 to
336bd90
Compare
c5ebe36 to
8419995
Compare
3c85a54 to
d815854
Compare
…e Token Endpoint Closes: keycloak#39278 Closses: keycloak#39279 Signed-off-by: forkimenjeckayang <[email protected]>
d815854 to
40159e7
Compare
…ss in server-spi-private Signed-off-by: forkimenjeckayang <[email protected]>
Signed-off-by: forkimenjeckayang <[email protected]>
Signed-off-by: forkimenjeckayang <[email protected]>
…s tests Signed-off-by: mposolda <[email protected]>
Signed-off-by: forkimenjeckayang <[email protected]>
Fix failing tests - UserSessionLimitsTest, FineGrainedAdminPermission…
bda0e2a to
f99c912
Compare
Summary
This PR implements support for parsing and understanding
authorization_detailsparameter at the Token Endpoint for OpenID4VCI (OpenID for Verifiable Credential Issuance).Implementation Includes:
1. Authorization Details Processing
authorization_detailsparameter in token requestscredential_configuration_idandformat-based authorization details2. Credential Identifier Generation
{UUID}3. Session-based Identifier Persistence
credential_configuration_idwithin the same sessionPreAuthorizedCodeandAuthorizationCodegrant types4. Response Enhancement
authorization_detailsin token responses with generated credential identifiers