[JENKINS-75056] Upgrade pac4j to version 6.1.0 #491
Conversation
| } | ||
|
|
||
| /** | ||
| * This method is needed as there seems to be a bug in pac4j and hasChanged is not able to return true |
There was a problem hiding this comment.
I am curious: Is there an official bug report link? Maybe helpful to add this here...
There was a problem hiding this comment.
There was a problem hiding this comment.
This method is needed
Is this statement true? It was not needed in #455.
There was a problem hiding this comment.
@pankajy-dev, @basil is expecting a more detailed explanation.
If we configure the plugin to issue the ID Token with a nonce, the refresh token shouldn't have nonce, and if present it should be the same as in the ID token. However pac4j expects that the refresh token contains the nonce and considers it should be new. That's the bug reported to pac4j.
When the plugin is calling pac4j to refresh the token and the nonce is in use from the doCommenceLogin, this method was always returning false (sincerely, I don't recall why as I checked close to 3 months ago), making the process halt. Once the bug is fixed the parent class method should work again, so this override can be removed.
It was not needed in #455.
it might be for 3 potential reasons:
- Either the
pac4jbug is fixed in pac4j-6.1.0, what I don't think it's the case - Or the differences between pac4j-6.0.10 and 6.1.0 makes this method not needed anymore (this override comes from one of my initial commits, that I tested using keycloak and AWS Cognito)
- Or it's because a workaround we had to introduce (and that's my suspicious)
The workaround is
oic-auth-plugin/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java
Lines 274 to 282 in 714a976
There was a problem hiding this comment.
it might be for 3 potential reasons:
- Upgrade to pac4j 6.x and migrate to EE 9 #455 is also using 6.1.0, so this reasoning is invalid.
- Upgrade to pac4j 6.x and migrate to EE 9 #455 is also using 6.1.0, so this reasoning is invalid.
- After removing
checkNonceInRefreshFlowworkaround you mentioned and retesting Upgrade to pac4j 6.x and migrate to EE 9 #455 in ATH, everything still works fine, so this reasoning is invalid.
sincerely, I don't recall why as I checked close to 3 months ago
I suggest running #455 in a debugger and stepping through the call to opMetadataResolver.init() before you continue this conversation.
Is there a valid reason why this hasChanged workaround is needed?
There was a problem hiding this comment.
IIRC, the ATH doesn't configure the use of nonce (because of the bug mainly), so the test you did won't catch the issue
I suggest running #455 in a debugger and stepping through the call to opMetadataResolver.init() before you continue this conversation.
I have a counteroffer: I suggest running #455 configuring the nonce in a debugger and stepping through the call before you continue this conversation
There was a problem hiding this comment.
IIRC, the ATH doesn't configure the use of nonce (because of the bug mainly), so the test you did won't catch the issue
I forced the use of a nonce with basil@7a79db4 against #455 and ATH is still passing, so as far as I can tell #455 is passing this scenario even without the hasChanged workaround (and therefore the hasChanged workaround is unnecessary). As far as I can tell, #455 is passing all test scenarios (including manual nonce testing) without the hasChanged workaround. If there is some legitimate test scenario where #455 is failing, then I would like to know how to reproduce the problem so that I can better understand the hasChanged workaround in this PR. As of now I am still not fully following the reasoning you have provided above.
There was a problem hiding this comment.
I ran an exhaustive set of 12 tests with the Keycloak Docker image, testing token refresh in a variety of nonce scenarios:
Test 1
Trunk: checkNonceInRefreshFlow = false
Result: Refresh succeeded
Test 2
Trunk: checkNonceInRefreshFlow = true
Actual: Refresh failed
Test 3
PR 491: checkNonceInRefreshFlow = false, hasChanged workaround present, init fix absent
Result: Refresh succeeded
Test 4
PR 491: checkNonceInRefreshFlow = true, hasChanged workaround present, init fix absent
Result: Refresh failed
Test 5
PR 491: checkNonceInRefreshFlow = false, hasChanged workaround absent, init fix absent
Result: Initial login (not refresh!) fails with NullPointerException: Cannot invoke com.nimbusds.openid.connect.sdk.op.OIDCProviderMetadata.getAuthorizationEndpointURI() because the return value of org.pac4j.oidc.metadata.OidcOpMetadataResolver.load() is null
Test 6
PR 491: checkNonceInRefreshFlow = true, hasChanged workaround absent, init fix absent
Result: Initial login (not refresh!) fails with NullPointerException: Cannot invoke com.nimbusds.openid.connect.sdk.op.OIDCProviderMetadata.getAuthorizationEndpointURI() because the return value of org.pac4j.oidc.metadata.OidcOpMetadataResolver.load() is null
Test 7
PR 491: checkNonceInRefreshFlow = false, hasChanged workaround present, init fix present
Result: Refresh succeeded
Test 8
PR 491: checkNonceInRefreshFlow = true, hasChanged workaround present, init fix present
Result: Refresh failed
Test 9
PR 491: checkNonceInRefreshFlow = false, hasChanged workaround absent, init fix present
Result: Refresh succeeded
Test 10
PR 491: checkNonceInRefreshFlow = true, hasChanged workaround absent, init fix present
Result: Refresh failed
Test 11
PR 455, checkNonceInRefreshFlow = false
Result: Refresh succeeded
Test 12
PR 455, checkNonceInRefreshFlow = true
Result: Refresh failed
Analysis
Tests 1 and 2 establish a baseline of current behavior on trunk. Refresh succeeds, but only when checkNonceInRefreshFlow = false. On trunk and in this test environment, refresh fails as expected when checkNonceInRefreshFlow = true.
It has been claimed in #491 (comment) that the hasChanged workaround is needed to allow refresh to succeed when checkNonceInRefreshFlow = true. Were this claim true, refresh would have succeeded in tests 4, 6, and 8. Refresh failed in tests 4 and 8, so the claim is false. Initial login failed in tests 5 and 6, consistent with the claim being false.
As I have been saying, the problem has been misdiagnosed. The hasChanged code is a workaround for something, but the reasoning given in #491 (comment) is invalid, as demonstrated by tests 3 and 4. Rather, the hasChanged code is working around a more basic issue, revealed in tests 5 and 6. A cleaner solution for that issue is my init fix, which restores the status quo (without regression!) both when the hasChanged workaround is present (tests 7 and 8) as well as when the hasChanged workaround is absent (tests 9 and 10). Tests 11 and 12 are a duplicate of tests 9 and 10 against PR 455, demonstrating that my original solution from over two months ago in PR 455 remains bug-free relative to the status quo on trunk.
My recommendations remain the same:
- Remove the unnecessary
hasChangedworkaround in favor of my simpler and more maintainableinitfix. Since this is the primary difference between PR 491 and PR 455, this suggestion is tantamount to closing PR 491 in favor of PR 455. - In lieu of coming up with a second counteroffer, run PR 455 in a debugger and step through the call to
opMetadataResolver.init()to understand the root cause of the issue. - Review PR 455, either demonstrating a reproducible problem with my
initapproach or otherwise approving PR 455.
src/main/java/org/jenkinsci/plugins/oic/OicdPluginOpMetadataResolver.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| /** | ||
| * This method is needed as there seems to be a bug in pac4j and hasChanged is not able to return true |
There was a problem hiding this comment.
fcojfernandez
changed the title
… like jackson lib coming from jackson2 plugin
pankajy-dev
changed the title
…solver.java Co-authored-by: Francisco Javier Fernandez <31063239+fcojfernandez@users.noreply.github.com>
|
I wonder if this PR super-seeds #455? |
|
CI is failing as pom uses 5.3 version of plugin-pom which needs min maven 3.9.6 I think CI needs an update to use the updated Maven version. |
|
Depends on #485 |
|
There is another PR for same changes with passing CI, therefore this PR will be closed in favor of #455 |
|
@pankajy-dev this PR should unblock you #496 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #491 +/- ##
============================================
+ Coverage 71.73% 72.29% +0.56%
- Complexity 222 228 +6
============================================
Files 17 18 +1
Lines 1033 1054 +21
Branches 148 148
============================================
+ Hits 741 762 +21
Misses 201 201
Partials 91 91 ☔ View full report in Codecov by Sentry. |
Since the security assessment is already done here we will be using this PR. |
pankajy-dev
requested review from
eva-mueller-coremedia,
fcojfernandez and
jtnord
| @@ -533,6 +541,8 @@ private OidcConfiguration buildOidcConfiguration() { | |||
| conf.setResourceRetriever(getResourceRetriever()); | |||
| if (this.isPkceEnabled()) { | |||
| conf.setPkceMethod(CodeChallengeMethod.S256); | |||
| } else { | |||
There was a problem hiding this comment.
Question: Set PKCE state in any case?
if (this.isPkceEnabled()) {
conf.setPkceMethod(CodeChallengeMethod.S256);
conf.setDisablePkce(false);
} else {
conf.setPkceMethod(ull);
conf.setDisablePkce(true);
}
| @@ -0,0 +1,37 @@ | |||
| package org.jenkinsci.plugins.oic; | |||
|
|
|||
There was a problem hiding this comment.
This file seem to need to be formatted: With mvn spotless:apply?
| @@ -1375,8 +1399,9 @@ public boolean handleTokenExpiration(HttpServletRequest httpRequest, HttpServlet | |||
| return true; | |||
| } | |||
|
|
|||
| private void redirectToLoginUrl(HttpServletRequest req, HttpServletResponse res) throws IOException { | |||
| if (req != null && (req.getSession(false) != null || Strings.isNullOrEmpty(req.getHeader("Authorization")))) { | |||
| private void redirectToLoginUrl(@NonNull HttpServletRequest req, @NonNull HttpServletResponse res) | |||
There was a problem hiding this comment.
why the annotations? they contradict the method body. For example, if res is NonNull then
if (res != null) {
res.sendRedirect(Jenkins.get().getSecurityRealm().getLoginUrl());
}
will happen always
There was a problem hiding this comment.
This might be based on a not so glorious comment by myself: #491 (comment)
I wanted to say: either tighten the contract by adding the @NonNull annotation (then there is no need for the null check) or leave the null check...
Sorry for not be more clear in my previous comment.
|
I've repeated the tests in #491 (comment) and now we can agree. I'm closing this PR in favor of #455 , that I'll approve after compare with this same PR. Thanks @pankajy-dev for this PR! |
Testing done
Executed all the Junit tests
Executed OicAuthPluginTest
The plugin is compiling and passing all the tests.
Jenkins issue 75056
Submitter checklist