Repository navigation
Pluggable Features of Token Manager - #12551
Conversation
|
The PR provides keycloak users with capability of adding their own features to token generation in the pluggable way, namely client policies' executors/conditions. The PR provides the following arquillian integration tests:
|
|
@tnorimat do you have a date, to merge this branch to master ? |
|
@evertongodoi Hello, this PR is under review. I will rebase the PR on the latest master because the the PR has conflicts with the latest master branch. |
facb8ba to
ceda4da
Compare
| return createTokenResponse(user, userSession, clientSessionCtx, scopeParam, true, s -> {return new TokenResponseContext(formParams, parseResult, clientSessionCtx, s);}); | ||
| } | ||
|
|
||
| public Response createTokenResponse(UserModel user, UserSessionModel userSession, ClientSessionContext clientSessionCtx, |
There was a problem hiding this comment.
Looks like it is possible to remove method createTokenResponse with this signature and keep just the newly added with clientPolicyContextGenerator as an argument?
There was a problem hiding this comment.
Yes, I will remove the createTokenResponse in L443-446. Nobody calls the method.
| } | ||
|
|
||
| private void verifyDPoPProof(ClientPolicyContext context) throws ClientPolicyException { | ||
| // retrieve a DPoP Proof JWT from the "DPoP" HTTP header |
There was a problem hiding this comment.
Question: Should there be follow-up issue to uncomment (and test) this once DPoP is available in Keycloak? Or possibly it can be done during DPoP initial PR itself? In this case, note should be added likely to the DPoP issue about that?
There was a problem hiding this comment.
As you suggested, I think this test should be done in the follow-up issue of the DPoP initial PR. Therefore, I will remove this test from the PR.
| } | ||
|
|
||
| private void verifyBindingTokensWithDPoPPublicKey(TokenResponseContext context) { | ||
| // retrieve a DPoP public key from the DPoP Proof JWT |
There was a problem hiding this comment.
As you suggested, I think this test should be done in the follow-up issue of the DPoP initial PR. Therefore, I will remove this test from the PR.
| } | ||
|
|
||
| @Test | ||
| public void testExtendedCl 8000 ientPolicyIntefacesForDeviceTokenResponse() throws Exception { |
There was a problem hiding this comment.
Note: Looks that ClientPoliciesTest class is already very big and contains 49 test methods. Will it makes sense to add the new test for "extended" methods into separate test class?
There was a problem hiding this comment.
Yes, I also think that ClientPoliciesTest became too big. How about creating other PR for refactoring ClientPoliciesTest? I am willing to work on this refactoring.
There was a problem hiding this comment.
@tnorimat +1, if you can create follow-up GH issue and the follow-up PR, it will be very nice. Thank you!
|
@mposolda I have fixed the PR as you suggested. Could you check it? |
|
@mposolda I will create follow-up GH issue and the follow-up PR. |
|
@tnorimat Thanks! |
|
@tnorimat Thanks for creating follow-up issue! |
Closes #12065