rework configuration of the plugin #399

jtnord · 2024-09-16T16:06:29Z

The server configuration (token URLs, scopes etc) are now in separate describable, flags to contol setting of options is removed and is based on the actual option/type

This makes the UX marginally cleaner, but helps the backend code to not need flags to know if something is set.

The configuration format is backwards compatible with previous versions, but the casc format is not.

⚠️The behaviour of overriding scopes in "auto" merge has changed. If override scopes is set then those scopes are used unconditionally. Previously the scopes where only used if they where also advertised, but this did not make sense as the provider is not required to advertise all the scopes it supported. This is a change in behaviour, but should not be breaking as we will only end up request more scopes, which the server should ignore if they are unsupported.

⚠️Users of casc will need to adapt their configuration.

configuration of the provider side has been moved into a serverConfiguration section and split to 2 different types wellKnown for configuration via a auto discovery and manual for manual configuration.

e.g.

for manual configuration:

  securityRealm:
    oic:
      serverConfiguration:
        manual:
          authorizationServerUrl: https://url.example.com/authorize
          jwksServerUrl: https://jwks.example.com/jwks
          tokenAuthMethod: client_secret_post
          tokenServerUrl: https://token.example.com/token
          scopes: scopes

and for auto configuration:


  securityRealm:
    oic:
      serverConfiguration:
        wellKnown:
          wellKnownOpenIDConfigurationUrl: https://idp.example.com:/someRealm/.well-known/openid-configuration

Screen Shots

Before

After

Testing done

run some local tests against an IDP configured with a well-known endpoint.
saved a manual config and manually inspected the config.xml
ran mvn verify locally

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

jtnord · 2024-09-16T17:02:25Z

src/main/resources/org/jenkinsci/plugins/oic/OicServerManualConfiguration/config.jelly

+        <f:radioBlock title="${%Basic}" name="tokenAuthMethod"
+            checked="${instance.tokenAuthMethod == 'client_secret_basic'}" value="client_secret_basic" inline="true" help="${null}"/>
+        <f:radioBlock title="${%Post}" name="tokenAuthMethod"
+            checked="${instance.tokenAuthMethod == null || instance.tokenAuthMethod == 'client_secret_post'}" value="client_secret_post" inline="true" help="${null}"/>


the previous did not have help=${null} but it was added here as they did (prior to the change and with this change before help=${null]) actually render with a help button, but the help was unrelated to the option.

jtnord · 2024-09-16T17:03:25Z

src/main/resources/org/jenkinsci/plugins/oic/OicServerManualConfiguration/config.jelly

+        <f:radioBlock title="${%Basic}" name="tokenAuthMethod"
+            checked="${instance.tokenAuthMethod == 'client_secret_basic'}" value="client_secret_basic" inline="true" help="${null}"/>
+        <f:radioBlock title="${%Post}" name="tokenAuthMethod"
+            checked="${instance.tokenAuthMethod == null || instance.tokenAuthMethod == 'client_secret_post'}" value="client_secret_post" inline="true" help="${null}"/>


set the default here for new installs with the == null check (slightly fewer clicks for the defaults)

jtnord · 2024-09-16T17:10:09Z

...ain/resources/org/jenkinsci/plugins/oic/OicServerManualConfiguration/help-jwksServerUrl.html

@@ -0,0 +1,4 @@
+<div>
+    Recommended. json webtoken key signature url of the openid connect provider


original had Recommended. jswon webtoken which is fixed here and causing this not to show as a rename

src/main/resources/org/jenkinsci/plugins/oic/OicServerManualConfiguration/config.properties

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

jtnord · 2024-09-17T11:44:08Z

pom.xml

@@ -49,6 +49,7 @@
    <spotless.check.skip>false</spotless.check.skip>
    <spotbugs.effort>Max</spotbugs.effort>
    <configuration-as-code.version>1836.vccda_4a_122a_a_e</configuration-as-code.version>
+    <hpi.compatibleSinceVersion>4.431</hpi.compatibleSinceVersion>


config is migrated, but the Jenkins config-as-code format is no longer compatable, so noting so users get a warning before updating.

Will need updating if anything gets merged before this

codecov · 2024-09-17T11:53:51Z

Codecov Report

Attention: Patch coverage is 77.02703% with 51 lines in your changes missing coverage. Please review.

Project coverage is 76.03%. Comparing base (c0be63e) to head (3e8cbca).
Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
...i/plugins/oic/OicServerWellKnownConfiguration.java	73.33%	24 Missing and 4 partials ⚠️
...va/org/jenkinsci/plugins/oic/OicSecurityRealm.java	72.34%	7 Missing and 6 partials ⚠️
...nsci/plugins/oic/OicServerManualConfiguration.java	85.50%	9 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #399      +/-   ##
============================================
+ Coverage     72.47%   76.03%   +3.55%     
- Complexity      244      259      +15     
============================================
  Files            12       15       +3     
  Lines          1021     1043      +22     
  Branches        148      149       +1     
============================================
+ Hits            740      793      +53     
+ Misses          201      174      -27     
+ Partials         80       76       -4

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

jtnord · 2024-09-17T11:46:52Z

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

-            Boolean useRefreshTokens)
+            Secret clientSecret,
+            OicServerConfiguration serverConfiguration,
+            Boolean disableSslVerification)


for a future refactoring, given this is a Boolean it is obvious that this should not be a required field and moved to a @DataboundSetter

jtnord · 2024-09-17T11:47:43Z

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

-            try {
-                Field field = getClass().getDeclaredField("endSessionEndpoint");
-                field.setAccessible(true);
-                field.set(this, endSessionUrl + "/");
-            } catch (IllegalArgumentException | IllegalAccessException | NoSuchFieldException | SecurityException e) {
-                LOGGER.log(Level.SEVERE, "Can't set endSessionEndpoint from old value", e);
-            }


I could see no need for reflection here - the field is available so we can just use it directly.

jtnord · 2024-09-17T11:48:30Z

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

+                }
+            }
+        } catch (FormException e) {
+            // FormException does not override toString() so looses info on the fields set and the message may not have


https://issues.jenkins.io/browse/JENKINS-73771

jtnord · 2024-09-17T11:50:23Z

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

@@ -421,28 +458,9 @@ public Secret getClientSecret() {
        return clientSecret == null ? Secret.fromString(NO_SECRET) : clientSecret;
    }

-    public String getWellKnownOpenIDConfigurationUrl() {


there are no reported OSS plugins or known private plugins that depend on this plugin, so the only backwards compatibility we need to worry about is the config.xml migration

jtnord · 2024-09-17T12:26:39Z

src/main/resources/org/jenkinsci/plugins/oic/Messages.properties

-OicSecurityRealm.UsingDefaultScopes = Using ''openid email''.
-OicSecurityRealm.RUSureOpenIdNotInScope = Are you sure you don''t want to include ''openid'' as an scope?
+OicSecurityRealm.RUSureOpenIdNotInScope = Are you sure you don''t want to include ''openid'' as a scope?
+OicSecurityRealm.ScopesRequired = Scopes is required.


sounds like bad English, but "scopes" is the field, not a plural of "scope" so the field is singular.
Probably should be reworded so that it has something other than just "the field is required"

jtnord · 2024-09-17T12:56:53Z

src/main/resources/org/jenkinsci/plugins/oic/Messages.properties

@@ -3,19 +3,21 @@ OicLogoutAction.OicLogout = Oic Logout
 OicSecurityRealm.DisplayName = Login with Openid Connect
 OicSecurityRealm.ClientIdRequired = Client id is required.
 OicSecurityRealm.ClientSecretRequired = Client secret is required.
-OicSecurityRealm.URLNotAOpenIdEnpoint = URL does seem to describe OpenID Connect endpoints
+OicSecurityRealm.URLNotAOpenIdEnpoint = URL does not seem to describe OpenID Connect endpoints


unrelated, but noticed this was wrong when moving the tests.

jtnord · 2024-09-17T12:57:05Z

src/main/resources/org/jenkinsci/plugins/oic/Messages.properties

 OicSecurityRealm.NotAValidURL = Not a valid url.
 OicSecurityRealm.CouldNotRetreiveWellKnownConfig = Could not retrieve well-known config {0,number,integer} {1}
 OicSecurityRealm.CouldNotParseResponse = Could not parse response
 OicSecurityRealm.ErrorRetreivingWellKnownConfig = Error when retrieving well-known config
 OicSecurityRealm.TokenServerURLKeyRequired = Token Server Url Key is required.
 OicSecurityRealm.TokenAuthMethodRequired = Token auth method is required.
 OicSecurityRealm.UsingDefaultUsername = Using ''sub''.
-OicSecurityRealm.UsingDefaultScopes = Using ''openid email''.
-OicSecurityRealm.RUSureOpenIdNotInScope = Are you sure you don''t want to include ''openid'' as an scope?
+OicSecurityRealm.RUSureOpenIdNotInScope = Are you sure you don''t want to include ''openid'' as a scope?


unrelated, but noticed this when moving the tests.

mikecirioli · 2024-09-18T20:45:25Z

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

+                            new OicServerWellKnownConfiguration(wellKnownOpenIDConfigurationUrl);
+                    conf.setScopesOverride(this.overrideScopes);
+                    serverConfiguration = conf;
+                } else {


consider making this else into an explicit if to protect against future changes? if the expected values of automanualconfigure ever change and this is not updated then you would get the default case even when you don't want it

The field is dead from this version onward so there would be no issues with it changing in future.

Just here for the readRosolve and is transient so it's either "auto"``"manual" or null

The server configuration (token URLs, scopes etc) are now in separate describable. This makes the UX cleaner, and the code cleaner in the realm. The config.xml is backwards compatable with previous versions, but the casc format is not. when in discovery mode via well known endpoint the override scopes are now used explicitly rather than using the overlapping scopes from the overrides and those provided by the server

timja · 2024-09-19T11:04:30Z

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

+    private transient LocalDateTime wellKnownExpires = null;
+
+    @DataBoundConstructor
+    public OicServerWellKnownConfiguration(String wellKnownOpenIDConfigurationUrl) {


JCasC wise you would normally drop the class prefix so this field would be called openIDConfigurationUrl as its already in a well known configuration class the well known bit is implied

That way you don't get the double wellKnown in the yaml:

securityRealm: oic: serverConfiguration: wellKnown: openIDConfigurationUrl: [https://idp.example.com:/someRealm/.well-known/openid-configuration](https://idp.example.com/someRealm/.well-known/openid-configuration)

is there a way to do this with an annotation so that the config.xml format stays the same or is the only choice now to fix this to use a custom piece of code(at which point I think it may be better to live with it)?

@Symbol only works on types (enum, class, interfaces) not on fields or methods, so I will bear this in mind for any future work, but am not planning on adding another data migration at this time.

Sure readResolve would be the normal approach, there's no alias via annotation.

See also jenkinsci#399

jimsnab · 2024-10-04T20:31:24Z

@jtnord could you update the docs?

jtnord added the breaking label Sep 16, 2024

jtnord force-pushed the refactor-config branch 2 times, most recently from ae8f74a to 276c6b8 Compare September 16, 2024 16:57

jtnord commented Sep 16, 2024

View reviewed changes

jtnord force-pushed the refactor-config branch 2 times, most recently from 0a585d0 to c2ef3e6 Compare September 16, 2024 17:49

jtnord commented Sep 16, 2024

View reviewed changes

jtnord marked this pull request as ready for review September 16, 2024 17:55

jtnord requested a review from a team as a code owner September 16, 2024 17:55

jtnord marked this pull request as draft September 16, 2024 17:56

jtnord force-pushed the refactor-config branch from c2ef3e6 to e1676c1 Compare September 17, 2024 11:28

jtnord requested review from mikecirioli and michael-doubez September 17, 2024 11:30

github-advanced-security bot found potential problems Sep 17, 2024

View reviewed changes

src/main/java/org/jenkinsci/plugins/oic/OicServerManualConfiguration.java Dismissed Show dismissed Hide dismissed

jtnord marked this pull request as ready for review September 17, 2024 11:33

jtnord force-pushed the refactor-config branch 2 times, most recently from 4cdb879 to 9799c0b Compare September 17, 2024 11:42

jtnord commented Sep 17, 2024

View reviewed changes

jtnord force-pushed the refactor-config branch from 9799c0b to 7430677 Compare September 17, 2024 12:31

jtnord commented Sep 17, 2024

View reviewed changes

jtnord force-pushed the refactor-config branch from 7430677 to b1e87c0 Compare September 17, 2024 12:43

jtnord commented Sep 17, 2024

View reviewed changes

jtnord force-pushed the refactor-config branch from b1e87c0 to 6cbd0c6 Compare September 18, 2024 17:29

jtnord mentioned this pull request Sep 18, 2024

Remove deprecated constructor #401

Merged

6 tasks

mikecirioli previously approved these changes Sep 18, 2024

View reviewed changes

jtnord dismissed mikecirioli’s stale review via 3e8cbca September 19, 2024 09:37

jtnord force-pushed the refactor-config branch from 6cbd0c6 to 3e8cbca Compare September 19, 2024 09:37

jtnord mentioned this pull request Sep 19, 2024

adapt OicAuthPluginTest to https://github.com/jenkinsci/oic-auth-plugin/pull/399 jenkinsci/acceptance-test-harness#1723

Closed

jtnord merged commit 289bd3f into jenkinsci:master Sep 19, 2024
20 checks passed

jtnord deleted the refactor-config branch September 19, 2024 10:31

timja reviewed Sep 19, 2024

View reviewed changes

eva-mueller-coremedia added a commit to eva-mueller-coremedia/oic-auth-plugin that referenced this pull request Sep 20, 2024

Use endSessionUrl instead of endSessionEndpoint for manual configuration

c6bf1a8

See also jenkinsci#399

eva-mueller-coremedia mentioned this pull request Sep 20, 2024

Use endSessionUrl instead of endSessionEndpoint for manual configuration in UI #402

Merged

6 tasks

fcojfernandez mentioned this pull request Sep 20, 2024

Adapt the OicAuthPluginTest to the changes in the describables jenkinsci/acceptance-test-harness#1724

Merged

6 tasks

jtnord mentioned this pull request Oct 7, 2024

CasC docs are not up to date. #416

Closed

fcojfernandez mentioned this pull request Oct 7, 2024

Update CasC documentation #417

Merged

6 tasks

TomHellier mentioned this pull request Oct 9, 2024

Jenkins controller breaks with the new version of the plugin 4.355.v3a_fb_fca_b_96d4 #421

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rework configuration of the plugin #399

rework configuration of the plugin #399

jtnord commented Sep 16, 2024 •

edited

Loading

jtnord Sep 16, 2024

jtnord Sep 16, 2024

jtnord Sep 16, 2024

jtnord Sep 17, 2024 •

edited

Loading

codecov bot commented Sep 17, 2024 •

edited

Loading

jtnord Sep 17, 2024

jtnord Sep 17, 2024

jtnord Sep 17, 2024

jtnord Sep 17, 2024

jtnord Sep 17, 2024

jtnord Sep 17, 2024

jtnord Sep 17, 2024

mikecirioli Sep 18, 2024

jtnord Sep 18, 2024

timja Sep 19, 2024

jtnord Sep 19, 2024 •

edited

Loading

jtnord Sep 19, 2024

timja Sep 20, 2024

jimsnab commented Oct 4, 2024

		@@ -0,0 +1,4 @@
		<div>
		Recommended. json webtoken key signature url of the openid connect provider

rework configuration of the plugin #399

rework configuration of the plugin #399

Conversation

jtnord commented Sep 16, 2024 • edited Loading

Before

After

Testing done

Submitter checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Sep 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimsnab commented Oct 4, 2024

jtnord commented Sep 16, 2024 •

edited

Loading

jtnord Sep 17, 2024 •

edited

Loading

codecov bot commented Sep 17, 2024 •

edited

Loading

jtnord Sep 19, 2024 •

edited

Loading