8000 Using DPoP token type in the access-token and as token_type in introspection response by tnorimat · Pull Request #22244 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Using DPoP token type in the access-token and as token_type in introspection response #22244

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 1 commit into from
Aug 7, 2023
Merged

Using DPoP token type in the access-token and as token_type in introspection response #22244

merged 1 commit into from
Aug 7, 2023

Conversation

tnorimat
Copy link
Contributor
@tnorimat tnorimat commented Aug 4, 2023

closes #21919

@tnorimat tnorimat requested review from a team as code owners August 4, 2023 07:02
@tnorimat tnorimat requested a review from a team August 4, 2023 07:02
@tnorimat tnorimat requested a review from a team as a code owner August 4, 2023 07:02
@tnorimat
Copy link
Contributor Author
tnorimat commented Aug 4, 2023

@mposolda The PR resolve the one issue of the epic #21916 . Could you check it?

mposolda
mposolda reviewed Aug 4, 2023
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.

@tnorimat I've added few inline comments. Could you please check it?

services/src/main/java/org/keycloak/protocol/oidc/AccessTokenIntrospectionProvider.java Outdated
@@ -85,6 +85,13 @@ public Response introspect(String token) {
}
}
}

if (accessToken.getConfirmation() != null && accessToken.getConfirmation().getKeyThumbprint() != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the description of the issue, I suggested also to add DPoP instead of Bearer as the token type during authentication - in case of Bearer tokens. Does this look ok to you?

Maybe the easiest would be:

tokenMetadata.put(OAuth2Constants.TOKEN_TYPE, accessToken.getType());

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, only tokenMetadata.put(OAuth2Constants.TOKEN_TYPE, accessToken.getType()); is enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that when token type is updated to DPoP for the DPoP tokens, some small updates might be needed also in other places. I think that TokenVerifier.withDefaultChecks() may need to be updated to allow both Bearer and DPoP as token types.

Copy link
Contributor

Choose a reason for hiding this comment

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

But maybe not... We can see if there are any failing tests :-)

services/src/main/java/org/keycloak/protocol/oidc/AccessTokenIntrospectionProvider.java Outdated
@@ -85,6 +85,13 @@ public Response introspect(String token) {
}
}
}

if (accessToken.getConfirmation() != null && accessToken.getConfirmation().getKeyThumbprint() != null) {
tokenMetadata.put("token_type", "DPoP");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to use some constant for token_type like we're doing for other things? Maybe OAuth2Constants is good class to use it or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the same above.

@tnorimat tnorimat force-pushed the ISSUE-21919-token-type-introspect branch from c8a8e04 to a76f666 Compare August 4, 2023 07:54
mposolda
mposolda reviewed Aug 4, 2023
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.

@tnorimat Thanks for the updates.

I think that DPoPTest is failing because you don't yet set DPoP token type in the AccessToken. The token type is set to Bearer. We set DPoP as token_type claim only in the whole response, but not yet in the token. I've proposed to add DPoP also as token type, which would makes the test passing (See my description in Issue #21919).

...e/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/DPoPTest.java Outdated
@@ -193,6 +193,7 @@ public void testDPoPProofByConfidentialClient() throws Exception {
TokenMetadataRepresentation tokenMetadataRepresentation = JsonSerialization.readValue(tokenResponse, TokenMetadataRepresentation.class);
Assert.assertTrue(tokenMetadataRepresentation.isActive());
assertEquals(jkt, tokenMetadataRepresentation.getConfirmation().getKeyThumbprint());
assertEquals("DPoP", tokenMetadataRepresentation.getOtherClaims().get("token_type"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assertEquals("DPoP", tokenMetadataRepresentation.getOtherClaims().get("token_type"));
assertEquals("DPoP", tokenMetadataRepresentation.getOtherClaims().get(OAuth2Constants.TOKEN_TYPE));

NOTE: Maybe class OAuth2Constants would need to be imported in DPoPTest

8000

...arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/TokenIntrospectionTest.java Outdated
@@ -352,6 +352,7 @@ private void testIntrospectAccessToken(String jwaAlgorithm) throws Exception {
assertEquals("test-user@localhost", rep.getUserName());
assertEquals("test-app", rep.getClientId());
assertEquals(loginEvent.getUserId(), rep.getSubject());
assertEquals("Bearer", rep.getOtherClaims().get("token_type"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assertEquals("Bearer", rep.getOtherClaims().get("token_type"));
assertEquals("Bearer", rep.getOtherClaims().get(OAuth2Constants.TOKEN_TYPE));

NOTE: Maybe class OAuth2Constants would need to be imported in TokenIntrospectionTest

@mposolda mposolda mentioned this pull request Aug 4, 2023
Merged
@tnorimat tnorimat force-pushed the ISSUE-21919-token-type-introspect branch 3 times, most recently from 659dae4 to aaedc60 Compare August 5, 2023 00:22
@tnorimat tnorimat force-pushed the ISSUE-21919-token-type-introspect branch from aaedc60 to cfa6da7 Compare August 5, 2023 00:36
@tnorimat
Copy link
Contributor Author
tnorimat commented Aug 7, 2023

@mposolda I resolved the errors of the CI tests. Could you check the revised PR?

mposolda
mposolda approved these changes Aug 7, 2023
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.

@tnorimat Thanks!

@mposolda mposolda merged commit 9d0960d into keycloak:main Aug 7, 2023
@tnorimat tnorimat deleted the ISSUE-21919-token-type-introspect branch May 21, 2025 05:10
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

Assignees
No one assigned
Labels
team/cloud-native
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Using DPoP token type in the access-token and as token_type in introspection response
2 participants
@tnorimat @mposolda
0