Repository navigation
Customization for JWTAuthorizationGrantType to support ID-JAG - #46048
Conversation
|
@bucchi Hello, could you run |
5f016d7 to
bfd1d78
Compare
There was a problem hiding this comment.
It is better to add javadoc comment to interface class for a reviewer who is not familiar with ID-JAG to understand them.
There was a problem hiding this comment.
@tnorimat
Thank you for your comment.
I have just added some comments.
There was a problem hiding this comment.
Pull request overview
This PR introduces support for Identity Assertion JWT Authorization Grant (ID-JAG), a new type of JWT that can be used as an authorization grant per RFC 7523. To accommodate different JWT types for authorization grants, the PR implements a new Service Provider Interface (SPI) called JWTAuthorizationGrantValidatorSpi, which allows for pluggable JWT validators. The existing JWTAuthorizationGrantValidator class has been refactored into a base class (JWTAuthorizationGrantValidatorBase), with a new ID-JAG-specific validator (IDJWTAuthorizationGrantValidator) and a default validator implementation (DefaultJWTAuthorizationGrantValidator).
Changes:
- Introduced JWTAuthorizationGrantValidatorSpi for pluggable JWT validation based on token type
- Refactored JWT authorization grant code into a new jwtauthorization package
- Added ID-JAG validator with client_id claim validation
- Added feature flag IDENTITY_ASSERTION_JWT_VALIDATOR and corresponding tests
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| common/src/main/java/org/keycloak/common/Profile.java | Adds IDENTITY_ASSERTION_JWT_VALIDATOR feature flag |
| server-spi-private/src/main/java/org/keycloak/protocol/oidc/JWTAuthorizationGrantValidatorSpi.java | Defines new SPI for JWT authorization grant validators |
| server-spi-private/src/main/java/org/keycloak/protocol/oidc/JWTAuthorizationGrantValidatorFactory.java | Factory interface for JWT authorization grant validators |
| server-spi-private/src/main/java/org/keycloak/protocol/oidc/JWTAuthorizationGrantValidator.java | Provider interface for JWT authorization grant validators |
| server-spi-private/src/main/resources/META-INF/services/org.keycloak.provider.Spi | Registers the new SPI |
| services/src/main/java/org/keycloak/authentication/authenticators/client/AbstractBaseJWTValidator.java | Makes clientAssertionState non-final to allow setting after construction |
| services/src/main/java/org/keycloak/protocol/oidc/grants/jwtauthorization/JWTAuthorizationGrantType.java | Refactored grant type to support pluggable validators based on JWT type header |
| services/src/main/java/org/keycloak/protocol/oidc/grants/jwtauthorization/JWTAuthorizationGrantTypeFactory.java | Moved to new package location |
| services/src/main/java/org/keycloak/protocol/oidc/grants/jwtauthorization/validator/JWTAuthorizationGrantValidatorBase.java | Base validator class refactored from original validator |
| services/src/main/java/org/keycloak/protocol/oidc/grants/jwtauthorization/validator/DefaultJWTAuthorizationGrantValidator.java | Default validator for standard JWT authorization grants |
| services/src/main/java/org/keycloak/protocol/oidc/grants/jwtauthorization/validator/IDJWTAuthorizationGrantValidator.java | ID-JAG specific validator that validates client_id claim |
| services/src/main/java/org/keycloak/protocol/oidc/grants/jwtauthorization/validator/IDJWTAuthorizationGrantValidatorFactory.java | Factory for ID-JAG validator |
| services/src/main/resources/META-INF/services/org.keycloak.protocol.oidc.grants.OAuth2GrantTypeFactory | Updates path for JWTAuthorizationGrantTypeFactory |
| services/src/main/resources/META-INF/services/org.keycloak.protocol.oidc.JWTAuthorizationGrantValidatorFactory | Registers ID-JAG validator factory |
| test-framework/oauth/src/main/java/org/keycloak/testframework/oauth/OAuthIdentityProvider.java | Adds encodeIDJAG method for creating ID-JAG tokens |
| tests/base/src/test/java/org/keycloak/tests/oauth/IDJWTAuthorizationGrantTest.java | Tests for ID-JAG functionality |
Comments suppressed due to low confidence (4)
services/src/main/java/org/keycloak/protocol/oidc/grants/jwtauthorization/JWTAuthorizationGrantType.java:89
- The catch block has inconsistent indentation. The throw statement should be aligned with the opening brace of the catch block (should not have extra indentation).
services/src/main/java/org/keycloak/protocol/oidc/grants/jwtauthorization/JWTAuthorizationGrantType.java:81 - Missing space after the 'if' keyword. The code should be formatted as "if (" instead of "if(".
services/src/main/java/org/keycloak/protocol/oidc/grants/jwtauthorization/JWTAuthorizationGrantType.java:90 - The JWT parsing is being done twice: once in line 73-76 and again in lines 86-90. This is inefficient. Consider parsing the JWT once and reusing the result, or refactor the logic to avoid duplicate parsing.
services/src/main/java/org/keycloak/protocol/oidc/grants/jwtauthorization/validator/JWTAuthorizationGrantValidatorBase.java:42 - The JWTAuthorizationGrantValidatorBase class implements JWTAuthorizationGrantValidator (which extends JWTAuthorizationGrantValidationContext), but it's missing the implementation of the getJws() method declared in JWTAuthorizationGrantValidationContext. This method should be implemented to return clientAssertionState.getJws().
| protected ManagedUser user; | ||
|
|
||
| @Test | ||
| public void testClientInTokenAndInRequestPrameter() { |
There was a problem hiding this comment.
The method name contains a typo: "Prameter" should be "Parameter"
| public void testClientInTokenAndInRequestPrameter() { | |
| public void testClientInTokenAndInRequestParameter() { |
There was a problem hiding this comment.
I have just fixed this as suggested.
| String clientIdInToken = (String) accessToken.getOtherClaims().get("client_id"); | ||
| if (!session.getContext().getClient().getClientId().equals(clientIdInToken)){ |
There was a problem hiding this comment.
There's a potential NullPointerException if clientIdInToken is null. The code should check if clientIdInToken is null before using equals comparison. Consider using Objects.equals or check for null explicitly before comparison.
There was a problem hiding this comment.
I have just added explicit null check.
And also I have just added a test case for Identity Assertion JWT with no client_id included.
| public void validateClient() { | ||
| super.validateClient(); | ||
|
|
||
| try{ |
There was a problem hiding this comment.
Missing space after 'try' keyword. The code should be formatted as "try {" instead of "try{".
| try{ | |
| try { |
There was a problem hiding this comment.
I have just fixed this as suggested.
| try{ | ||
| AccessToken accessToken = new JWSInput(getAssertion()).readJsonContent(AccessToken.class); | ||
| String clientIdInToken = (String) accessToken.getOtherClaims().get("client_id"); | ||
| if (!session.getContext().getClient().getClientId().equals(clientIdInToken)){ |
There was a problem hiding this comment.
Missing space after 'if' keyword. The code should be formatted as "if (" instead of "if (".
There was a problem hiding this comment.
The code should be formatted as "if (" instead of "if (".
From this, I can not find out what is the problem here.
But at the end of "if condition statement", there is a space missing at "oken)){".
So, I have just changed this to "oken)) {" for here.
| super.validateClient(); | ||
|
|
||
| try{ | ||
| AccessToken accessToken = new JWSInput(getAssertion()).readJsonContent(AccessToken.class); |
There was a problem hiding this comment.
The assertion is being parsed again with new JWSInput(getAssertion()), even though it was already parsed when creating the ClientAssertionState. This is inefficient. Consider reusing the already parsed JWSInput from clientAssertionState.getJws() instead.
| AccessToken accessToken = new JWSInput(getAssertion()).readJsonContent(AccessToken.class); | |
| JWSInput jws = clientAssertionState.getJws(); | |
| AccessToken accessToken = jws.readJsonContent(AccessToken.class); |
There was a problem hiding this comment.
I have just fixed this as suggested.
|
Hi @bucchi and @tnorimat, Thank you very much for ths PR, I really appreciate your work on this. I wanted to kindly check whether the PR is currently stalled, and if there is a tentative timeline your team has in mind for it. Are you planning for this PR to be included in the Keycloak 26.6 release? If you haven’t had a chance to watch it yet, I would kindly recommend taking a look at the following video, which demonstrates how Okta is leveraging ID-JAG for MCP use cases - https://www.brighttalk.com/webcast/14899/661521 |
|
Would it be possible to expose the initial assertion (read-only) to subsequently executed protocol mappers? If the initial assertion contained additional claims that should be carried over the the new token, we would need access to the original claims from the initial assertion. |
There was a problem hiding this comment.
Looks good so far. I added a few minor remarks.
|
|
||
| public static DefaultJWTAuthorizationGrantValidator createValidator(KeycloakSession session, ClientModel client, String assertion, String scope) { | ||
| if (assertion == null) { | ||
| throw new RuntimeException("Missing parameter:" + OAuth2Constants.ASSERTION); |
There was a problem hiding this comment.
| throw new RuntimeException("Missing parameter:" + OAuth2Constants.ASSERTION); | |
| throw new IllegalArgumentException("Missing parameter:" + OAuth2Constants.ASSERTION); |
There was a problem hiding this comment.
Thank you for your commnet.
I have just fixed this as you suggested.
| JWSInput jws = clientAssertionState.getJws(); | ||
| AccessToken accessToken = jws.readJsonContent(AccessToken.class); | ||
| String clientIdInToken = (String) accessToken.getOtherClaims().get("client_id"); | ||
| if (clientIdInToken == null || !session.getContext().getClient().getClientId().equals(clientIdInToken)) { |
There was a problem hiding this comment.
You could extract the client into a local var, this eases debugging.
There was a problem hiding this comment.
I have just fixed this as you suggested.
| @@ -57,6 +57,10 @@ public String encodeToken(JsonWebToken token, OAuthIdentityProviderKeys keys) { | |||
| return new JWSBuilder().type("JWT").jsonContent(token).sign(new ECDSASignatureSignerContext(keys.getKeyWrapper())); | |||
| } | |||
|
|
|||
| public String encodeIDJAG(JsonWebToken token) { | |||
| return new JWSBuilder().type("oauth-id-jag+jwt").jsonContent(token).sign(new ECDSASignatureSignerContext(keys.getKeyWrapper())); | |||
There was a problem hiding this comment.
Some custom jwt types are defined in org.keycloak.util.TokenUtil and org.keycloak.OAuth2Constants. I suggest to create a constant for "oauth-id-jag+jwt" in OAuth2Constants.
There was a problem hiding this comment.
I have just fixed this as you suggested.
| @@ -56,8 +64,38 @@ public Response process(Context context) { | |||
|
|
|||
| try { | |||
|
|
|||
| JWTAuthorizationGrantValidator authorizationGrantContext = JWTAuthorizationGrantValidator.createValidator( | |||
| if (assertion == null) { | |||
| throw new RuntimeException("Missing parameter:" + OAuth2Constants.ASSERTION); | |||
There was a problem hiding this comment.
| throw new RuntimeException("Missing parameter:" + OAuth2Constants.ASSERTION); | |
| throw new IllegalArgumentException("Missing parameter:" + OAuth2Constants.ASSERTION); |
There was a problem hiding this comment.
I have just fixed this as you suggested,
and also added some test cases for this.
| JWTAuthorizationGrantValidatorBase authorizationGrantContext = | ||
| (JWTAuthorizationGrantValidatorBase) session.getProvider(JWTAuthorizationGrantValidator.class, jwtTokenType); | ||
| if( authorizationGrantContext == null){ | ||
| authorizationGrantContext = DefaultJWTAuthorizationGrantValidator.createValidator( |
There was a problem hiding this comment.
I sugget to create a protected method like createValidator(...) that uses DefaultJWTAuthorizationGrantValidator.createValidator(..) by default.
This would enable to customize the validation behavior if necessary.
There was a problem hiding this comment.
I have just fixed this as you suggested.
055b308 to
37a541e
Compare
|
@bucchi The PR branch currently has some conflicts that need to be resolved. When you have a moment, could you please help address them? |
37a541e to
4e4e205
Compare
There was a problem hiding this comment.
@bucchi Thanks for this PR, it’s nice to see the work to support this new specification. A few considerations:
The Identity Assertion JWT Authorization Grant (ID-JAG) is defined in the draft specification https://www.ietf.org/archive/id/draft-ietf-oauth-identity-assertion-authz-grant-01.html as a profile of the JWT Authorization Grant (RFC7523).
This specification is also based on another draft specification, Identity Chaining Across Trust Domains https://datatracker.ietf.org/doc/draft-ietf-oauth-identity-chaining/, which combines Token Exchange (RFC8693) and the JWT Authorization Grant (RFC7523).
So this ID-JAG is obtained with a token exchange request and then used as an assertion in an access token request using the JWT Authorization Grant.
With this PR we are adding support for validating the Identity Assertion JWT Authorization Grant (ID-JAG) for JWT Authorization Grant requests.
Because of this, I was wondering if it would make sense to use a more general feature to support the whole specification, instead of having the feature IDENTITY_ASSERTION_JWT_VALIDATOR only for the ID-JAG validation.
I added some inline comments, but in general the introduction of a new SPI for the validator in this way IMO makes the classes a bit more complex, and it also introduces some duplication in the assertion parsing.
Additionally, we should consider the options configured on the identity provider. For example, jti is not mandatory for the JWT Authorization Grant, but it is required for ID-JAG. On the identity provider we have an option for replay checking of the assertion using the jti claim. This option should probably be hidden or disabled and enforced when ID-JAG is used.
| @@ -162,6 +162,8 @@ public enum Feature { | |||
|
|
|||
| CIMD("OAuth Client ID Metadata Document", Type.EXPERIMENTAL), | |||
|
|
|||
| IDENTITY_ASSERTION_JWT_VALIDATOR("Identity Assertion JWT validator", Type.EXPERIMENTAL), | |||
There was a problem hiding this comment.
I was thinking if it makes sense to have a dedicated feature for the entire specification instead of only for validating the ID JAG.
There was a problem hiding this comment.
Thank you for your comment.
I made that IDENTITY_ASSERTION_JWT so that it could be used not only for validator part but also for issuer part.
|
|
||
| try { | ||
| JWSInput jws = clientAssertionState.getJws(); | ||
| AccessToken accessToken = jws.readJsonContent(AccessToken.class); |
There was a problem hiding this comment.
It should already be parsed in the client assertion state: clientAssertionState.getToken()
There was a problem hiding this comment.
I fixed as you suggested.
Thank you.
| * | ||
| * @author <a href="mailto:yutaka.obuchi.sd@hitachi.com">Yutaka Obuchi</a> | ||
| */ | ||
| public class IDJWTAuthorizationGrantValidator extends JWTAuthorizationGrantValidatorBase { |
There was a problem hiding this comment.
Some checks are missing. For example, jti and iat are REQUIRED according to the specification: https://www.ietf.org/archive/id/draft-ietf-oauth-identity-assertion-authz-grant-01.html#section-3.”
There was a problem hiding this comment.
I added some checks according to the specifications.
Here I also added some test cases for required fields.
| if (assertion == null) { | ||
| throw new IllegalArgumentException("Missing parameter:" + OAuth2Constants.ASSERTION); | ||
| } | ||
|
|
||
| JWSInput jws; | ||
| try { | ||
| jws = new JWSInput(assertion); | ||
| } catch (JWSInputException e) { | ||
| throw new RuntimeException("The provided assertion is not a valid JWT"); | ||
| } | ||
|
|
||
| String jwtTokenType = jws.getHeader().getType(); | ||
| JWTAuthorizationGrantValidatorBase authorizationGrantContext = | ||
| (JWTAuthorizationGrantValidatorBase) session.getProvider(JWTAuthorizationGrantValidator.class, jwtTokenType); | ||
| if( authorizationGrantContext == null){ | ||
| authorizationGrantContext = createDefaultValidator( | ||
| context.getSession(), client, assertion, formParams.getFirst(OAuth2Constants.SCOPE)); | ||
| } else { | ||
| JsonWebToken jwt; | ||
| try { | ||
| jwt = jws.readJsonContent(JsonWebToken.class); | ||
| } catch (JWSInputException e) { | ||
| throw new RuntimeException("The provided assertion is not a valid JWT"); | ||
| } | ||
|
|
||
| ClientAssertionState clientAssertionState = new ClientAssertionState(OAuth2Constants.JWT_AUTHORIZATION_GRANT, assertion, jws, jwt); | ||
| clientAssertionState.setClient(client); | ||
|
|
||
| authorizationGrantContext.setClientAssertionState(clientAssertionState); | ||
| authorizationGrantContext.setScopeParam(formParams.getFirst(OAuth2Constants.SCOPE)); | ||
| } |
There was a problem hiding this comment.
Only IDJWTAuthorizationGrantValidator is obtained via getProvider, for normal jwt it is created manually. There are also some duplications in the parsing of the assertion.
There was a problem hiding this comment.
I removed some duplications.
|
@bucchi @graziang I think it would be best to use the links below to access the latest versions of the drafts. This should help ensure everyone is reviewing the up-to-date documents and avoid any confusion caused by earlier versions. https://datatracker.ietf.org/doc/draft-ietf-oauth-identity-assertion-authz-grant/ |
|
@bucchi @tnorimat Just a kind reminder regarding the 26.6 release schedule, which is planned for the next few weeks. If it may be difficult for you to prioritise this PR within that time frame, would it be possible to consider granting the Keycloak core maintainer write access to the PR to adopt the changes they have suggested? This might help ensure the PR can be merged in time for the 26.6 release. Thank you very much for considering this. I really appreciate your help. |
f6a7d2b to
5afbf42
Compare
@graziang, From my understanding, it could be done by extending the identity provider of JWT Auhtorization Grant. Btw, will you be in KeycloakCon EU next weak by any chance? |
|
really looking forward to this PR landing in a (hopefully soon 🙂) release (in the mean-time for testing over in https://github.com/archestra-ai/archestra I've built an image off of your latest PR commit) |
5afbf42 to
37fd6a9
Compare
|
|
||
| public interface JWTAuthorizationGrantValidator extends Provider, JWTAuthorizationGrantValidationContext{ | ||
|
|
||
| public void validateClient(); |
There was a problem hiding this comment.
You could remove the public modifier from those interface methods as there are public by default :)
There was a problem hiding this comment.
Thank you for your comment.
I fixed this.
37fd6a9 to
768893a
Compare
|
Hi everyone and @mposolda, |
768893a to
19f78f4
Compare
19f78f4 to
a079b97
Compare
a079b97 to
3e5427e
Compare
| authorizationGrantContext = createDefaultValidator( | ||
| context.getSession(), formParams.getFirst(OAuth2Constants.SCOPE), clientAssertionState); | ||
| } else { | ||
| authorizationGrantContext.setClientAssertionState(clientAssertionState); |
There was a problem hiding this comment.
A provider created via SPI starts with clientAssertionState = null. Any method called before setClientAssertionState() can causes NPE, non-ideal use of the provider, It might be better to pass clientAssertionState as a parameter to validation methods instead, or some other refactor.
There was a problem hiding this comment.
@graziang
Thank you for your comment.
As you can see, that is actually one of tricky part. And it might make my implementation a little bit ugly.
So it is OK to modify the source code as your suggestion.
From my understanding, your suggestion is that all methods in JWTAuthorizationGrantValidator interface below better be modified to have ClientAssertionState as parameter.
Do I understand your suggestion correctly?
void validateClient();
void validateIssuer();
void validateSubject();
boolean validateTokenActive(int allowedClockSkew, int maxExp, boolean reusePermitted);
boolean validateSignatureAlgorithm(String expectedSignatureAlg);
boolean validateTokenAudience(List<String> expectedAudiences, boolean multipleAudienceAllowed);
There was a problem hiding this comment.
Reason here I am asking this is that for this modification, AbstractBaseJWTValidator, AbstractJWTClientValidator and the related test codes have to be modified too and the affected area will be larger.
I wanted to confirm whether it was intended for just in case.
There was a problem hiding this comment.
Hi @bucchi, yeah, or create a ClientAssertionState for each method without touching the AbstractBaseJWTValidator, but the point here is that an implementation of an SPI should be usable via the methods defined by the SPI itself. If every SPI implementation requires different ways to configure or use it, and you need to cast to the concrete implementation in order to use it properly, there’s a design problem.
I know the issue is related to the relationship with AbstractBaseJWTValidator, but as things are getting complicated, maybe you could check if it would be simpler to just extend the original JWTAuthorizationGrantValidator as IDJWTAuthorizationGrantValidator and override only the methods needed to validate the ID-JAG and use it if needed in the JWTAuthorizationGrantType. WDYT?
There was a problem hiding this comment.
@graziang
I have a couple of random ideas.
As you mentioned, I too think that the relationship with AbstractBaseJWTValidator makes this implementation complex and ugly.
So how about not using AbstractBaseJWTValidator like below:
-
Keeping the original logic as it is in JWTAuhtorizationGrantType and just adding some code to get provider from session for making it possible to plugin extra validator for each token type.
The SPI implementation here does not extend AbstractBaseJWTValidator. We better keep it as simple as possible.
This way, no matter what JWT token type comes in, original logic in JWTAuhtorizationGrantType is executed and also depending on the incoming token type, the corresponding extra validation is executed. -
Using ClientPolicy with TokenType condition and IDJAGEnforcer executor for instance:
Like built-in GranTypeCondition.java, TokenTypeCondition.java is implemented for checking token type in JWT header.
Like built-in JWTClaimEnforcerExecutor.java, IDJAGEnforcerExecutor is implemented for checking if the other claims comply with the IDJAG specification.
There are three alternatives including your idea so far.
I have no particular preference among the these.
Please let me know your preference.
Thank you
There was a problem hiding this comment.
@bucchi If I understood correctly the first proposal is to keep the current behavior and add extra validation based on the provider. The problem with this approach is that the validation is not necessarily more restrictive than the default one, so it might be necessary to override the validation of some claims. I’m not against keeping the provider, but we need to find a way to use it that works well.
For the second approach, I’d prefer not to use client policies because I’d like support for this specification to be integrated with the idp and clients. As a follow-up, I can imagine enabling ID-JAG validation on the identity provider as well, and if we use client policies, that might not be possible. I see client policies for extra custom claims validation which is also already supported using the JWTClaimEnforcerExecutor.
I’ll leave it to you to find a solution that works better, maybe something in between the first approach and my proposal.
Thanks for all the work and the discussion on this
There was a problem hiding this comment.
A provider created via SPI starts with clientAssertionState = null. Any method called before
setClientAssertionState()can causes NPE, non-ideal use of the provider, It might be better to pass clientAssertionState as a parameter to validation methods instead, or some other refactor.
About your concern above,
I have just modified the instance creation process of JWTAuthorizationGrantValidator implementation class like below:
public DefaultJWTAuthorizationGrantValidator(KeycloakSession session) {
this(session,
session.getContext().getHttpRequest().getDecodedFormParameters().getFirst(OAuth2Constants.SCOPE),
buildClientAssertionState(session));
}
So that a provider created via SPI starts with non null clientAssertionState.
How about this workaround?
Does it work for you?
Let me know what you think.
There was a problem hiding this comment.
@bucchi The workaround initializes the provider by reading from the HTTP context in the constructor. This works for normal HTTP flows and single assertion because the provider is recreated for each session, but it creates tight coupling between the provider and the HTTP context, which can cause issues.
For example, if the provider is invoked multiple times in the same session with different assertions the cached provider will have stale state from the first invocation, how you can select the assertion to validate?
If we want to use the SPI provider pattern, I see two options:
- Pass and set state in each method:
This way we don't need to change AbstractBaseJWTValidator:
public void validateClient(ClientAssertionState state) {
this.clientAssertionState = state; // Update field before validation
super.validateClient(); // Call existing base logic
}
public void validateIssuer(ClientAssertionState state) {
this.clientAssertionState = state;
super.validateIssuer();
}
// ... same pattern for other methods
- : Initialize method: Set the state at the beginning using an initialize method (similar to the previous setAssertionState approach but clearer), with proper NPE handling:
public interface JWTAuthorizationGrantValidator {
void initialize(String scope, ClientAssertionState state);
void validateClient(); // Call after initialize()
void validateIssuer();
// ...
}
I don't see much additional value in using the provider via SPI for this use case, I see more complications than benefits. But if we want to go in this direction, I don't see any other good options compared to the two listed above, I slightly prefer the first one. WDYT? Do you have better ideas?
There was a problem hiding this comment.
I don't see much additional value in using the provider via SPI for this use case, I see more complications than benefits.
Maybe you are right about this.
I can not find the way to implement this more simpler than above.
I think your original idea below is the simplest because there is no other specific JWT type than ID-JAG so far. So, I have just modified the code based on your suggestion:
I know the issue is related to the relationship with AbstractBaseJWTValidator, but as things are getting complicated, maybe you could check if it would be simpler to just extend the original JWTAuthorizationGrantValidator as IDJWTAuthorizationGrantValidator and override only the methods needed to validate the ID-JAG and use it if needed in the JWTAuthorizationGrantType. WDYT?
| jwtTokenType = OAuth2Constants.JWT; | ||
| } | ||
|
|
||
| JWTAuthorizationGrantValidatorBase authorizationGrantContext = |
There was a problem hiding this comment.
Consider removing JWTAuthorizationGrantValidatorBase it seems to exist only as an adapter between AbstractBaseJWTValidator and the SPI interfaces, you could extend AbstractBaseJWTValidator directly in DefaultJWTAuthorizationGrantValidator and implement the interface methods themselves, simplifying the
hierarchy which is already a bit complex. Then IDJWTAuthorizationGrantValidator could directly extend DefaultJWTAuthorizationGrantValidator. And in this line you could get the provider directly as interface without casting to an implementation.
There was a problem hiding this comment.
+1 that hierarchy is a bit complex. If there is possibility to simplify a bit as suggested above, it can be nice.
There was a problem hiding this comment.
Thank you for your comment and suggestion.
I totally agree with you for making the hierarchy more simple.
I have just tried removing JWTAuthorizationGrantValidatorBase in my local repository.
It looks ok , but
for the part
And in this line you could get the provider directly as interface without casting to an implementation.
we can make the line your mentioned like below
JWTAuthorizationGrantValidator authorizationGrantContext =
session.getProvider(JWTAuthorizationGrantValidator.class, jwtTokenType);
without casting.
But some other lines needs some casting like below:
UserModel user = lookupUserByFederatedIdentity(federatedIdentityModel, ((DefaultJWTAuthorizationGrantValidator) authorizationGrantContext).getState());
((DefaultJWTAuthorizationGrantValidator) authorizationGrantContext).setScopeParam(formParams.getFirst(OAuth2Constants.SCOPE));
As you can imagine, that is because those methods are originally from AbstractBaseJWTValidator, not in JWTAuthorizationGrantValidator interface.
Do I understand correctly?
There was a problem hiding this comment.
Thank you for your feedback.
I have just removed JWTAuthorizationGrantValidatorBase as you suggested.
And the following is also included in the PR source code as you wanted
JWTAuthorizationGrantValidator authorizationGrantContext =
session.getProvider(JWTAuthorizationGrantValidator.class, jwtTokenType);
without any extra casting.
With some modification to how to create an instance of JWTAuthorizationGrantValidator implementation class like below:
public DefaultJWTAuthorizationGrantValidator(KeycloakSession session) {
this(session,
session.getContext().getHttpRequest().getDecodedFormParameters().getFirst(OAuth2Constants.SCOPE),
buildClientAssertionState(session));
}
We do not need this any more
authorizationGrantContext).setScopeParam(formParams.getFirst(OAuth2Constants.SCOPE));
And for the following:
UserModel user = lookupUserByFederatedIdentity(federatedIdentityModel, ((DefaultJWTAuthorizationGrantValidator) authorizationGrantContext).getState());
((DefaultJWTAuthorizationGrantValidator)
I have just removed authorizationGrantContext).getState() parameter, because it is not used in lookupUserByFederatedIdentity method.
b3f3b81 to
bbf607b
Compare
|
Hi, @graziang Just a quick heads-up: Could you please take another look? Thanks! |
| @@ -164,7 +190,7 @@ public Response process(Context context) { | |||
| } | |||
| } | |||
|
|
|||
| protected UserModel lookupUserByFederatedIdentity(FederatedIdentityModel federatedIdentityModel, ClientAssertionState clientAssertionState) { | |||
| protected UserModel lookupUserByFederatedIdentity(FederatedIdentityModel federatedIdentityModel) { | |||
There was a problem hiding this comment.
I was using the assertionState in a subclass to reference some claims from the assertion to narrow the client/user lookup.
Any chance to get that in again?
There was a problem hiding this comment.
I hope to merge the PR once the last remaining comment by @thomasdarimont regarding the removed method parameter is solved. @bucchi Is it ok to add the parameter back?
|
Given that it is already in use, I would be more than happy to. Give me some time, please. |
…ssertion and send back access token working as as a part of token endpoint Signed-off-by: Yutaka Obuchi <yutaka.obuchi.sd@hitachi.com>
bbf607b to
7887708
Compare
|
@mposolda , @thomasdarimont, @graziang I have just modified to make the lookupUserByFederatedIdentity's parameter back. |
|
Thanks @bucchi that works for me again :) |
There was a problem hiding this comment.
@bucchi @graziang @tnorimat @thomasdarimont Thanks everyone for the participation and review of this PR!
…ssertion and send back access token working as as a part of token endpoint (keycloak#46048) closes keycloak#49005 Signed-off-by: Yutaka Obuchi <yutaka.obuchi.sd@hitachi.com> Co-authored-by: Yutaka Obuchi <yutaka.obuchi.sd@hitachi.com>
closes #49005
This PR is for ID-JAG receiver, which receives ID-JAG and send back access token working as a part of token endpoint.
ID-JAG is one type of JWT, which can be used as Authorization Grant(https://www.rfc-editor.org/rfc/rfc7523.html#section-2.1).
AFAIK, the type for JWTAuthorizationGrant clearly mentioned in the standard specs or the draft of those is only one, for now.
But in the future there could be more types coming.
So here in this PR, we make validators for every types of JWT pluggable by introducing brand new SPI:JWTAuthorizationGrantValidatorSpi.