Skip to content

OidcProperty follow-ups; Fix application of clock skew to generated credentials#654

Merged
jtnord merged 8 commits into
jenkinsci:masterfrom
Vlatombe:oicproperty-follow-up
Sep 11, 2025
Merged

OidcProperty follow-ups; Fix application of clock skew to generated credentials#654
jtnord merged 8 commits into
jenkinsci:masterfrom
Vlatombe:oicproperty-follow-up

Conversation

@Vlatombe
Copy link
Copy Markdown
Member

@Vlatombe Vlatombe commented Sep 10, 2025

Follows up reviews from #644

The new clock skew value was not correctly applied to the generated OicCredentials

Testing done

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests that demonstrate the feature works or the issue is fixed

@Vlatombe Vlatombe requested a review from a team as a code owner September 10, 2025 07:49
Comment thread src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java
accessToken.getLifetime(),
CLOCK.millis(),
getAllowedTokenExpirationClockSkewSeconds());
(long) client.getConfiguration().getMaxClockSkew());
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ditto

@Vlatombe Vlatombe requested review from jglick and jtnord September 10, 2025 07:50
@jtnord jtnord added the bug label Sep 10, 2025
Comment thread src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java Outdated
Comment thread src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java Outdated
Comment thread src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java
@Vlatombe Vlatombe changed the title OidcProperty follow-ups OidcProperty follow-ups; Fix application of clock skew to generated credentials Sep 10, 2025
jtnord
jtnord previously approved these changes Sep 10, 2025
@jtnord jtnord enabled auto-merge September 10, 2025 10:56
@Vlatombe Vlatombe marked this pull request as draft September 10, 2025 10:59
auto-merge was automatically disabled September 10, 2025 10:59

Pull request was converted to draft

@Vlatombe
Copy link
Copy Markdown
Member Author

Still working on cleaning up removed methods...

@Vlatombe Vlatombe marked this pull request as ready for review September 10, 2025 11:10
@Vlatombe Vlatombe requested a review from jtnord September 10, 2025 11:10
assertTrue(crumb.process(request, response, chain));
}

@Test
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved from OicSecurityRealmTest

assertEquals(rootUrl, realm.getValidRedirectUrl("http://localhost/jenkins/../bar/"));
}

@Test
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved to EscapeHatchTest

if (FIPS140.useCompliantAlgorithms() && isDisableSslVerification()) {
throw new IllegalStateException(Messages.OicSecurityRealm_DisableSslVerificationFipsMode());
}
try {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

(ignore WS)

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 10, 2025

Codecov Report

❌ Patch coverage is 27.90698% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.60%. Comparing base (0d6d19c) to head (9a9a85d).
⚠️ Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
...va/org/jenkinsci/plugins/oic/OicSecurityRealm.java 21.42% 15 Missing and 7 partials ⚠️
.../jenkinsci/plugins/oic/properties/EscapeHatch.java 45.45% 6 Missing ⚠️
...ugins/oic/properties/DisableTokenVerification.java 25.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #654      +/-   ##
============================================
- Coverage     74.74%   74.60%   -0.15%     
+ Complexity      326      315      -11     
============================================
  Files            33       33              
  Lines          1299     1280      -19     
  Branches        179      176       -3     
============================================
- Hits            971      955      -16     
  Misses          241      241              
+ Partials         87       84       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines -795 to -797
@DataBoundSetter
@Deprecated
public void setDisableTokenVerification(boolean disableTokenVerification) throws FormException {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this break CasC even if you set the flag to tolerate deprecated properties? (see #655)

@jtnord jtnord merged commit 9de140f into jenkinsci:master Sep 11, 2025
18 of 20 checks passed
@Vlatombe Vlatombe deleted the oicproperty-follow-up branch September 11, 2025 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants