Add custom parameters to authorize and logout endpoints #480

eva-mueller-coremedia · 2024-12-14T23:08:53Z

This PR

provides a different approach to solve Logout issue: Required String parameter 'redirect_uri' is not present #229 resp. New config option: Enable AWS Cognito Logout #241
solves Allow query params for ID provider URLs #443
Add new input fields for login resp. logout query parameters in the Jenkins management backend
maybeOpenIdLogoutEndpoint has been refactored when it comes to combining all parameters
Login query parameters are not allowed to overwrite keys as defined in org.pac4j.oidc.config.OidcConfiguration
Logout query parameters are not allowed to overwrite keys as set in maybeOpenIdLogoutEndpoint like id_token_hint, state, post_logout_redirect_uri

Testing done

This change has been tested by unit tests as well as local testing agains AWS Cognito.

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 demonstrates feature works or fixes the issue

codecov · 2024-12-14T23:17:59Z

Codecov Report

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

Project coverage is 74.38%. Comparing base (13300b6) to head (4ceb342).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
...insci/plugins/oic/AbstractKeyValueDescribable.java	86.36%	2 Missing and 1 partial ⚠️
...va/org/jenkinsci/plugins/oic/OicSecurityRealm.java	94.28%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #480      +/-   ##
============================================
+ Coverage     72.55%   74.38%   +1.83%     
- Complexity      248      282      +34     
============================================
  Files            20       24       +4     
  Lines          1115     1183      +68     
  Branches        154      169      +15     
============================================
+ Hits            809      880      +71     
+ Misses          210      207       -3     
  Partials         96       96

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

jtnord

Hi @eva-mueller-coremedia thanks for this.

I have had a quick look and have a few comments around the UX side.

The extra parameters would IIUC not be needed in the majority of conformant IDPs, as such do you thing they should be hidden behind an advanced section?

the use of a single string for multiple keyvalue pairs (and the encoding /splitting shenaningans) would seem to be error prone (and it lacks good warnings on invalid input). (a value may need to encode & and then you need to escape it)
Would making the KeyValue a Describable and using a repeatable option using repeatableProperty or the like (which can then perform individual valuation of the key and the value). NB not sur how this would look in CasC.

eva-mueller-coremedia · 2024-12-24T10:14:30Z

@jtnord Thanks for the feedback, will try to follow a key value pair pattern.

If you don't mind, I would like to remove the UI at all - only provide it to be set via CasC resp. Java/Groovy code. Any objections?

jtnord · 2024-12-24T10:16:53Z

@jtnord Thanks for the feedback, will try to follow a key value pair pattern.

If you don't mind, I would like to remove the UI at all - only provide it to be set via CasC resp. Java/Groovy code. Any objections?

it's generally considered bad to have something exposed in the UI and not CasC and vice versa. Not exposing in the UI can lead to it getting overridden if someone saves the Jenkins form in the UI (which I say you should never do as if you manage by code you should have a read only system UI, but it is useful on test systems where you want to make changes in the UI and then export the config for applying to a different server).

eva-mueller-coremedia · 2024-12-24T10:19:29Z

@jtnord Thanks for the feedback, will try to follow a key value pair pattern.
If you don't mind, I would like to remove the UI at all - only provide it to be set via CasC resp. Java/Groovy code. Any objections?

it's generally considered bad to have something exposed in the UI and not CasC and vice versa. Not exposing in the UI can lead to it getting overridden if someone saves the Jenkins form in the UI (which I say you should never do as if you manage by code you should have a read only system UI, but it is useful on test systems where you want to make changes in the UI and then export the config for applying to a different server).

Understood. 👍 I will have a look to implement it the way you described initially. Thanks for the explanation!

src/main/java/org/jenkinsci/plugins/oic/OicQueryParameterConfiguration.java

src/main/java/org/jenkinsci/plugins/oic/OicServerWellKnownConfiguration.java

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

src/main/java/org/jenkinsci/plugins/oic/OicQueryParameterConfiguration.java

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

eva-mueller-coremedia · 2025-01-11T07:11:44Z

@jtnord Thanks for the review. Will take care after my 1 week vacation.

eva-mueller-coremedia · 2025-01-20T07:53:45Z

@jtnord How to re-trigger the failed Jenkins run - there has been a java.net.SocketTimeoutException: Read timed out when downloading artifacts from azure...

jtnord · 2025-01-20T17:20:00Z

@jtnord How to re-trigger the failed Jenkins run - there has been a java.net.SocketTimeoutException: Read timed out when downloading artifacts from azure...

For non maintainers, adding a new commit.

eva-mueller-coremedia · 2025-01-22T22:09:59Z

@jtnord The PR is now ready for a re-review.

I also extracted the FormValidation fix to #507

eva-mueller-coremedia · 2025-02-04T21:32:49Z

Sorry for nudging again - @jtnord @michael-doubez would be great if you could have a look.

jtnord

The code looks reasonable but some things look a little weird and I am wondering if thebUX can be improved at all.
I wanted to try and get some time to see if this can be improved and submit a PR to the PR as doing so after merging would possibly require even more data migration that we already have.realistically this is not going to happen this week.

eva-mueller-coremedia · 2025-02-04T22:24:58Z

realistically this is not going to happen this week.

Thanks for this estimate. Would be great to see this feature in the (near) future since AWS Cognito requires some additional query parameters.

It would be great if you could keep me/subscribers posted about the state of this PR from time to time. Thanks in advance.

jtnord · 2025-02-17T19:38:31Z

@eva-mueller-coremedia I filed
eva-mueller-coremedia#1 against your PR.

Moves the parameters to their own types and added validation for them so it's validated in the UI. Also ensures the config can never be invalid.
Tidies up the jelly slightly.

src/main/java/org/jenkinsci/plugins/oic/AbstractKeyValueDescribable.java

jtnord · 2025-02-17T20:52:00Z

src/main/java/org/jenkinsci/plugins/oic/LoginQueryParameter.java

+    public static class DescriptorImpl extends AbstractKeyValueDescribable.DescriptorImpl<LoginQueryParameter> {
+
+        @Override
+        public FormValidation doCheckKey(@QueryParameter String key) {


There is nothing sensitive here.
Whilst we could add a permission check we are only doing string comparisons

@jtnord Unfortunately, I do not know how to resolve all the problems found by the github-advanced-security. Can you help me here or step in? That would be great.

Also there is already one dismissed problem. Dunno who or how this got dismissed.

I can take a look.
I dismissed one as a false positive. (key was not sensitive in that context)

src/main/java/org/jenkinsci/plugins/oic/LoginQueryParameter.java

+    public static class DescriptorImpl extends AbstractKeyValueDescribable.DescriptorImpl<LoginQueryParameter> {
+
+        @Override
+        public FormValidation doCheckKey(@QueryParameter String key) {


src/main/java/org/jenkinsci/plugins/oic/LogoutQueryParameter.java

+    public static class DescriptorImpl extends AbstractKeyValueDescribable.DescriptorImpl<LogoutQueryParameter> {
+
+        @Override
+        public FormValidation doCheckKey(@QueryParameter String key) {


src/main/java/org/jenkinsci/plugins/oic/LogoutQueryParameter.java

+    public static class DescriptorImpl extends AbstractKeyValueDescribable.DescriptorImpl<LogoutQueryParameter> {
+
+        @Override
+        public FormValidation doCheckKey(@QueryParameter String key) {


eva-mueller-coremedia · 2025-02-17T21:59:40Z

@eva-mueller-coremedia I filed eva-mueller-coremedia#1 against your PR.

Moves the parameters to their own types and added validation for them so it's validated in the UI. Also ensures the config can never be invalid. Tidies up the jelly slightly.

Thank you for the PR. I additionally removed the commented out test.

This allows us to add parameter validation in the UI as the validation is now performed in the descriptor and prevents a bad describable from being constructed.

eva-mueller-coremedia · 2025-02-27T22:51:18Z

src/main/java/org/jenkinsci/plugins/oic/AbstractKeyValueDescribable.java

+            throw new Descriptor.FormException(valueValidation.getMessage(), "value");
+        }
+        this.key = key.trim();
+        this.value = value == null ? "" : value.trim();


Hi @jtnord - I added some more test cases and encountered for LogoutQueryParameters a NPE when the value is null.

Is this solution OK for you?

In case the this.value should then be null, then all getValue() needs to be covered as well.

eva-mueller-coremedia requested a review from a team as a code owner December 14, 2024 23:08

eva-mueller-coremedia force-pushed the query-params branch from 58e2662 to 36fe900 Compare December 14, 2024 23:24

eva-mueller-coremedia changed the title ~~Add custom parameters to authorize and logout endpoint~~ Add custom parameters to authorize and logout endpoints Dec 15, 2024

jtnord reviewed Dec 24, 2024

View reviewed changes

eva-mueller-coremedia force-pushed the query-params branch 2 times, most recently from 418804d to dce7676 Compare December 24, 2024 21:59

github-advanced-security bot found potential problems Dec 24, 2024

View reviewed changes

src/main/java/org/jenkinsci/plugins/oic/OicQueryParameterConfiguration.java Fixed Show fixed Hide fixed

eva-mueller-coremedia force-pushed the query-params branch 2 times, most recently from 01a48ec to bf28327 Compare December 25, 2024 11:55

eva-mueller-coremedia commented Dec 25, 2024

View reviewed changes

src/main/java/org/jenkinsci/plugins/oic/OicServerWellKnownConfiguration.java Outdated Show resolved Hide resolved

eva-mueller-coremedia requested a review from jtnord December 25, 2024 12:13

jtnord reviewed Jan 10, 2025

View reviewed changes

jtnord added the enhancement label Jan 10, 2025

eva-mueller-coremedia force-pushed the query-params branch from bf28327 to 08746a7 Compare January 19, 2025 16:41

eva-mueller-coremedia requested a review from jtnord January 19, 2025 17:27

eva-mueller-coremedia mentioned this pull request Jan 19, 2025

Some smaller housekeeping #509

Open

6 tasks

eva-mueller-coremedia force-pushed the query-params branch 3 times, most recently from 2c20523 to e933a1c Compare January 22, 2025 21:52

eva-mueller-coremedia force-pushed the query-params branch 3 times, most recently from 3e7c8cd to 219bd26 Compare February 4, 2025 20:44

jtnord reviewed Feb 4, 2025

View reviewed changes

eva-mueller-coremedia force-pushed the query-params branch from cb33a89 to 4093c81 Compare February 17, 2025 20:39

github-advanced-security bot found potential problems Feb 17, 2025

View reviewed changes

jtnord self-assigned this Feb 26, 2025

eva-mueller-coremedia and others added 2 commits February 27, 2025 22:26

Add custom parameters to authorize and logout endpoint

8bfa4cc

Introduce specific describables for query parameters

e33fadb

This allows us to add parameter validation in the UI as the validation is now performed in the descriptor and prevents a bad describable from being constructed.

eva-mueller-coremedia force-pushed the query-params branch from 85a93f0 to e33fadb Compare February 27, 2025 22:48

eva-mueller-coremedia commented Feb 27, 2025

View reviewed changes

Fix JavaDoc

4ceb342

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom parameters to authorize and logout endpoints #480

Add custom parameters to authorize and logout endpoints #480

eva-mueller-coremedia commented Dec 14, 2024 •

edited

Loading

codecov bot commented Dec 14, 2024 •

edited

Loading

jtnord left a comment

eva-mueller-coremedia commented Dec 24, 2024

jtnord commented Dec 24, 2024

eva-mueller-coremedia commented Dec 24, 2024

eva-mueller-coremedia commented Jan 11, 2025

eva-mueller-coremedia commented Jan 20, 2025

jtnord commented Jan 20, 2025

eva-mueller-coremedia commented Jan 22, 2025 •

edited

Loading

eva-mueller-coremedia commented Feb 4, 2025

jtnord left a comment

eva-mueller-coremedia commented Feb 4, 2025

jtnord commented Feb 17, 2025

jtnord Feb 17, 2025

eva-mueller-coremedia Feb 17, 2025

jtnord Feb 17, 2025

eva-mueller-coremedia commented Feb 17, 2025 •

edited

Loading

eva-mueller-coremedia Feb 27, 2025 •

edited

Loading

Add custom parameters to authorize and logout endpoints #480

Are you sure you want to change the base?

Add custom parameters to authorize and logout endpoints #480

Conversation

eva-mueller-coremedia commented Dec 14, 2024 • edited Loading

Testing done

Submitter checklist

codecov bot commented Dec 14, 2024 • edited Loading

Codecov Report

jtnord left a comment

Choose a reason for hiding this comment

eva-mueller-coremedia commented Dec 24, 2024

jtnord commented Dec 24, 2024

eva-mueller-coremedia commented Dec 24, 2024

eva-mueller-coremedia commented Jan 11, 2025

eva-mueller-coremedia commented Jan 20, 2025

jtnord commented Jan 20, 2025

eva-mueller-coremedia commented Jan 22, 2025 • edited Loading

eva-mueller-coremedia commented Feb 4, 2025

jtnord left a comment

Choose a reason for hiding this comment

eva-mueller-coremedia commented Feb 4, 2025

jtnord commented Feb 17, 2025

jtnord Feb 17, 2025

Choose a reason for hiding this comment

eva-mueller-coremedia Feb 17, 2025

Choose a reason for hiding this comment

jtnord Feb 17, 2025

Choose a reason for hiding this comment

eva-mueller-coremedia commented Feb 17, 2025 • edited Loading

eva-mueller-coremedia Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

eva-mueller-coremedia commented Dec 14, 2024 •

edited

Loading

codecov bot commented Dec 14, 2024 •

edited

Loading

eva-mueller-coremedia commented Jan 22, 2025 •

edited

Loading

eva-mueller-coremedia commented Feb 17, 2025 •

edited

Loading

eva-mueller-coremedia Feb 27, 2025 •

edited

Loading