Repository navigation
Implement DPoP for all grantTypes - #29967
Conversation
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabledKeycloak CI - Java Distribution IT (windows-latest - temurin - 19) |
|
@tnorimat Could you please help me review this one if you have some free cycles? |
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabledKeycloak CI - Java Distribution IT (windows-latest - temurin - 19) |
|
@mposolda Hello, Yes I will review the PR. |
There was a problem hiding this comment.
@Captain-P-Goldfish Hello, I add one minor review comment. Could you check it?
There was a problem hiding this comment.
"DPoP" might be TokenUtil.TOKEN_TYPE_DPOP.
There was a problem hiding this comment.
I am not sure where you are getting at?
Do you want the code like this?
get.setHeader("Authorization", TokenUtil.TOKEN_TYPE_DPOP + " " + accessTokenResponse.getAccessToken());
I don't think that this is a good idea. There is at least one case in which token_type Bearer is returned:
- DPoP not required
- DPoP not sent in AuthorizationRequest
You also added such a test and this test would fail then.
There was a problem hiding this comment.
L1183 get.addHeader("DPoP", dpopProof);
to
get.addHeader(TokenUtil.TOKEN_TYPE_DPOP, dpopProof);
But, you need not to modify it.
|
It would also be good to restructure the implementation here. EDIT: |
@Captain-P-Goldfish Could you please create separate GH issue for this topic you mentioned and add the comment here with the link to that issue? TBH I am not 100% sure from the top of my head if DPoP is supposed to be used in other grant types besides authorizationCode. But it might be good to investigate this anyway... |
|
@Captain-P-Goldfish Also it will be good to create dedicated GH issue for this issue itself with some more description. As every GH pull request (and commit) should have reference to the GH issue. |
|
unit tests are fixed except for the flaky tests from the main-branch :-) |
|
@Captain-P-Goldfish Thanks! Failing |
|
that should fix it. I am having serious trouble getting the DockerClientTest running locally due to ClassNotFound exceptions and an outdated testcontainers version that does not get along with my docker-desktop version... Is there somewhere a documentation that explains how to get this test running reliably? The DockerClientLogin implementation was trying to get the ProtocolMappers which I extended by the DPoP ProtocolMapper and expected that the mappers were having only a single entry. I extended the stream by a filter that makes sure that only the expected mapper is returned. |
|
@mposolda are you going to review this or is someone else doing it? |
|
@tnorimat Does this PR look ok to you? |
There was a problem hiding this comment.
@Captain-P-Goldfish I added some minor review comments. Could you check them?
- tokenType must be DPoP as defined in RFC9449 chapter 7.1 relatesTo keycloak#22311 Signed-off-by: Pascal Knüppel <captain.p.goldfish@gmx.de>
fixes keycloak#30179 fixes keycloak#30181 Signed-off-by: Pascal Knüppel <pascal.knueppel@governikus.de>
Signed-off-by: Pascal Knüppel <pascal.knueppel@governikus.de>
|
@tnorimat why do you want the documentation to be removed? I added it for those who read the code after us. People not knowing what DPoP is can find the related RFC and reasons why the code was added in this way faster if the comments are present. What is your reason that you want them removed? |
|
@Captain-P-Goldfish Hello, IMO, removing comments are Keycloak's convention. When I sent the PRs implementing several RFCs with comments, I was requested to remove the comments. |
There was a problem hiding this comment.
@Captain-P-Goldfish Thanks! I've added small comment related to the refresh token constructor.
@Captain-P-Goldfish @tnorimat Regarding comments: I think comments in the code and also javadoc can be useful, but always depends on the context. It is not good if comments are mis-used and there is too much of them, but IMO comments are good if it makes the code more readable and adds the useful info why the particular code is used etc. So I think it is case by case.
AFAIK Keycloak does not have any strong convention on how much comments to use. Added the inline notes with some suggestions where comments can be useful IMO. For the other places mentioned by @tnorimat , I agree we can possibly remove comments.
What do you think?
| * always be included in the response | ||
| */ | ||
| public RefreshToken(AccessToken token) { | ||
| public RefreshToken(AccessToken token, Confirmation confirmation) { |
There was a problem hiding this comment.
Is it possible if you keep also the constructor with single refresh token argument just for the backwards compatibility? So possibly maybe just something like this:
public RefreshToken(AccessToken token) {
this(token, null);
}
Signed-off-by: Pascal Knüppel <pascal.knueppel@governikus.de>
|
I have removed the comments that were not commented by @mposolda and re-added the constructor. |
|
@mposolda I approved the PR. Could you check it? |
There was a problem hiding this comment.
@Captain-P-Goldfish @tnorimat Thanks a lot for the reviews and applying all the comments from the review!
Fix claiming userInfoToken with DPoP
closes #30181
closes #30179
relatesTo #22311