Skip to content
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.

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

Reimplement PKCE to serialize verifier in session #291

Merged

Conversation

michael-doubez
Copy link
Contributor

@michael-doubez michael-doubez commented Apr 7, 2024

Persist code verifier in session information in order to allow HA Jenkins.

Fixes #290 regression: PKCE information was stored in google's AuthorizationCode flow and not serialized into session.
Current google implementation doesn't allow the get/set of verifier code code so, in our case, PKCE can no longer be used from within google flow. Therefore, this change implements PKCE in the plugin.

Testing done

Unit test now verifies PKCE validity instead of presence only.

Change manually tested with Google, with PKCE activated.

Submitter checklist

@michael-doubez
Copy link
Contributor Author

@Vlatombe can you please have a look ? Do I need to increment the serialVersionUID ?

Copy link

codecov bot commented Apr 7, 2024

Codecov Report

Attention: Patch coverage is 88.37209% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 73.60%. Comparing base (c3d9244) to head (e8fcd0f).
Report is 1 commits behind head on master.

Files Patch % Lines
...ain/java/org/jenkinsci/plugins/oic/OicSession.java 86.84% 5 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #291      +/-   ##
============================================
+ Coverage     73.16%   73.60%   +0.44%     
- Complexity      189      194       +5     
============================================
  Files             9        9              
  Lines           775      807      +32     
  Branches        113      115       +2     
============================================
+ Hits            567      594      +27     
- Misses          151      156       +5     
  Partials         57       57              

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

Copy link
Contributor Author

@michael-doubez michael-doubez left a comment

Choose a reason for hiding this comment

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

Self-review: LGTM

@Vlatombe
Copy link
Member

Vlatombe commented Apr 8, 2024

Caused by #288.

Do I need to increment the serialVersionUID ?

Not needed.

@michael-doubez michael-doubez merged commit 4124503 into jenkinsci:master Apr 8, 2024
18 checks passed
@michael-doubez michael-doubez deleted the 290-pkce-verification-failed branch April 8, 2024 08:28
@michael-doubez michael-doubez removed the request for review from Vlatombe April 8, 2024 08:29
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.

PKCE verification failed
2 participants