[JENKINS-43205][JENKINS-69006] Allow admins to configure a global default URL provider and always make external links go through /display/redirect #202

dwnusbaum · 2023-08-14T20:07:55Z

See JENKINS-43205. This PR provides a new global configuration option for admins to select the default URL provider for /display/redirect links.

It also tries to address various issues caused by #42 noted in JENKINS-69006.

The main changes are as follows:

When producing redirect links, we always use DisplayURLProviderImpl.INSTANCE, which gives us /display/redirect links, never direct links to a specific provider URL
DisplayURLProvider.getDefault is no longer context-sensitive, it just returns the extension instance for ClassicDisplayURLProvider (fixes possible infinite recursion in blueocean-display-url-provider)
DisplayURLProvider.getPreferredProvider has been updated to resolve redirects in a consistent way, taking all possible configuration options into account
AbstractDisplayAction.lookupProvider delegates to DisplayURLProvider.getPreferredProvider
There is a new DefaultProviderGlobalConfiguration class that allows admins to select a default provider in the GUI

Testing done

I added a new test that exercises DefaultProviderGlobalConfiguration. Other tests were adjusted as needed. Honestly though there seems to be a huge amount of redundancy in the tests here; I am tempted to throw them all out and start over.

Submitter checklist

Give feedback

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
Options

dwnusbaum · 2023-08-14T20:09:04Z

src/main/java/org/jenkinsci/plugins/displayurlapi/DisplayURLProvider.java

+        // TODO: Would it not make more sense to return DisplayURLProviderImpl.INSTANCE unconditionally, always
+        // serving /display/redirect URLs that only get resolved when an actual user clicks on them? There is no
+        // guarantee that the current user (if any) is going to be the user who clicks the generated link so IDK why we
+        // take the current user's preferences into account here.


This method only gets used when generating links, not when following them, so it doesn't make sense to me to consider user preferences here. In practice I don't think it matters because almost no one uses authorize-project and so this more or less always gets called in a context where there is no active user, but it is confusing when looking at the code.

Turns out it used to work the way I had in mind prior to #42, and JENKINS-69006 is asking for it to be switched back, so I will look into fixing that issue while I am here.

dwnusbaum · 2023-08-14T20:09:25Z

src/main/java/org/jenkinsci/plugins/displayurlapi/DisplayURLProvider.java

+        // Note that this logic means that `/display/redirect` links will never be produced when using the environment
+        // variable or system property, meaning that it effetively disables user preferences.


In other words, the system properties are really a way to completely disable the effects of this plugin without having to uninstall it.

...esources/org/jenkinsci/plugins/displayurlapi/DefaultProviderGlobalConfiguration/config.jelly

dwnusbaum · 2023-08-16T19:25:42Z

src/main/java/org/jenkinsci/plugins/displayurlapi/DisplayURLProvider.java

@@ -28,8 +38,7 @@ public abstract class DisplayURLProvider implements ExtensionPoint {
     * @return DisplayURLProvider
     */
    public static DisplayURLProvider get() {
-        DisplayURLProvider preferredProvider = getPreferredProvider();
-        return preferredProvider != null ? preferredProvider : DisplayURLProviderImpl.INSTANCE;
+        return DisplayURLProviderImpl.INSTANCE;


As before #42. See discussion in JENKINS-69006.

dwnusbaum · 2023-08-16T19:32:54Z

src/test/java/org/jenkinsci/plugins/displayurlapi/DisplayURLProviderTest.java

-        assertEquals(root + "job/my%20folder/job/my%20job/1/" + TestSysPropDisplayURLProvider.EXTRA_CONTENT_IN_URL,
-                     DisplayURLProvider.get().getRunURL(run));


This test shows the exact issue described in JENKINS-69006 - /display/redirect was bypassed entirely and the external URL was static.

dwnusbaum · 2023-08-16T19:33:25Z

src/test/java/org/jenkinsci/plugins/displayurlapi/DisplayURLProviderTest.java

-
-        String root = DisplayURLProvider.get().getRoot();
-        assertEquals("http://localhost:" + rule.getLocalPort() + "/jenkins/", root);
-        assertEquals(root + "job/my%20folder/job/my%20job/1/" + TestUserDisplayURLProvider.EXTRA_CONTENT_IN_URL,


Same as above, although in practice I think this case can only be hit if you use something like authorize-project.

src/main/java/org/jenkinsci/plugins/displayurlapi/actions/AbstractDisplayAction.java

…ider and delete redundant test class

dwnusbaum · 2023-08-16T21:35:03Z

src/test/java/org/jenkinsci/plugins/displayurlapi/actions/ActionRedirectEligibilityTest.java

This test class did not meaningfully increase coverage and required a @VisibleForTest method that was awkward to preserve with the changes to AbstractDisplayAction.

julieheard

Looks sensible to me

…facing text, and rename GlobalConfiguration

...n/java/org/jenkinsci/plugins/displayurlapi/DefaultDisplayURLProviderGlobalConfiguration.java

...esources/org/jenkinsci/plugins/displayurlapi/DefaultProviderGlobalConfiguration/config.jelly

car-roll

LGTM, though I am not familiar enough with this plugin to look beyond just the pure functionality of the code here.

dwnusbaum · 2023-08-17T20:38:30Z

I'll merge this once the CI build passes and then file a PR to set up CD.

olamy · 2023-08-17T21:07:24Z

all good you can merge and I can cut a release if you need it.

dwnusbaum · 2023-08-17T21:08:11Z

@olamy thanks! I have permission to release too, but I'm setting up CD in #203 and jenkins-infra/repository-permissions-updater#3484

olamy · 2023-08-18T01:04:52Z

release 2.3.9 done

timja · 2023-08-19T09:27:12Z

FYI in case of interest in improving documentation: jenkins-infra/jenkins.io#6137

Allow admins to configure a global default URL provider

442ee08

dwnusbaum commented Aug 14, 2023

View reviewed changes

[JENKINS-69006] Always use /display/redirect links when generating URLs

10d50bb

dwnusbaum changed the title ~~Allow admins to configure a global default URL provider~~ [JENKINS-43205][JENKINS-69006] Allow admins to configure a global default URL provider and always make external links go through /display/redirect Aug 16, 2023

Update lookupProvider priority comment

1fabe18

dwnusbaum added the enhancement label Aug 16, 2023

dwnusbaum commented Aug 16, 2023

View reviewed changes

dwnusbaum marked this pull request as ready for review August 16, 2023 19:33

dwnusbaum requested a review from a team as a code owner August 16, 2023 19:33

dwnusbaum commented Aug 16, 2023

View reviewed changes

src/main/java/org/jenkinsci/plugins/displayurlapi/actions/AbstractDisplayAction.java Show resolved Hide resolved

dwnusbaum added 2 commits August 16, 2023 17:26

Move provider resolution logic to DisplayURLProvider.getPreferredProv…

a123356

…ider and delete redundant test class

Fix Javadoc link

820dc1a

dwnusbaum commented Aug 16, 2023

View reviewed changes

julieheard approved these changes Aug 17, 2023

View reviewed changes

car-roll approved these changes Aug 17, 2023

View reviewed changes

[JENKINS-43205] Add CasC symbol for GlobalConfiguration, update user-…

37a40bd

…facing text, and rename GlobalConfiguration

dwnusbaum commented Aug 17, 2023

View reviewed changes

...n/java/org/jenkinsci/plugins/displayurlapi/DefaultDisplayURLProviderGlobalConfiguration.java Outdated Show resolved Hide resolved

...esources/org/jenkinsci/plugins/displayurlapi/DefaultProviderGlobalConfiguration/config.jelly Outdated Show resolved Hide resolved

dwnusbaum requested a review from car-roll August 17, 2023 18:27

car-roll approved these changes Aug 17, 2023

View reviewed changes

[JENKINS-43205] Switch symbol to defaultDisplayUrlProvider

3452ee2

olamy approved these changes Aug 17, 2023

View reviewed changes

dwnusbaum merged commit ab54d9d into jenkinsci:master Aug 17, 2023

dwnusbaum deleted the default-provider branch August 17, 2023 21:09

dwnusbaum mentioned this pull request Aug 18, 2023

Allow PipelineGraphDisplayURLProvider to activate by default, preserve direct tests/artifacts/changes links, and do not try to handle non-Pipeline builds jenkinsci/pipeline-graph-view-plugin#293

Merged

dwnusbaum mentioned this pull request Aug 21, 2023

Add a page explaining how to change the default "notification" URL interface jenkins-infra/jenkins.io#6137

Open

dwnusbaum mentioned this pull request Aug 21, 2023

Document (implement?) a way to set the Jenkins-instance "Default" Notification URL to view links from GitHub build results etc. jenkinsci/pipeline-graph-view-plugin#245

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-43205][JENKINS-69006] Allow admins to configure a global default URL provider and always make external links go through /display/redirect #202

[JENKINS-43205][JENKINS-69006] Allow admins to configure a global default URL provider and always make external links go through /display/redirect #202

dwnusbaum commented Aug 14, 2023 •

edited

Loading

Submitter checklist

dwnusbaum Aug 14, 2023 •

edited

Loading

dwnusbaum Aug 16, 2023

dwnusbaum Aug 14, 2023 •

edited

Loading

dwnusbaum Aug 16, 2023

dwnusbaum Aug 16, 2023

dwnusbaum Aug 16, 2023

dwnusbaum Aug 16, 2023

julieheard left a comment

car-roll left a comment

dwnusbaum commented Aug 17, 2023

olamy commented Aug 17, 2023

dwnusbaum commented Aug 17, 2023

olamy commented Aug 18, 2023

timja commented Aug 19, 2023

		// Note that this logic means that `/display/redirect` links will never be produced when using the environment
		// variable or system property, meaning that it effetively disables user preferences.

		assertEquals(root + "job/my%20folder/job/my%20job/1/" + TestSysPropDisplayURLProvider.EXTRA_CONTENT_IN_URL,
		DisplayURLProvider.get().getRunURL(run));

[JENKINS-43205][JENKINS-69006] Allow admins to configure a global default URL provider and always make external links go through /display/redirect #202

[JENKINS-43205][JENKINS-69006] Allow admins to configure a global default URL provider and always make external links go through /display/redirect #202

Conversation

dwnusbaum commented Aug 14, 2023 • edited Loading

Testing done

Submitter checklist

dwnusbaum Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

dwnusbaum Aug 16, 2023

Choose a reason for hiding this comment

dwnusbaum Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

dwnusbaum Aug 16, 2023

Choose a reason for hiding this comment

dwnusbaum Aug 16, 2023

Choose a reason for hiding this comment

dwnusbaum Aug 16, 2023

Choose a reason for hiding this comment

dwnusbaum Aug 16, 2023

Choose a reason for hiding this comment

julieheard left a comment

Choose a reason for hiding this comment

car-roll left a comment

Choose a reason for hiding this comment

dwnusbaum commented Aug 17, 2023

olamy commented Aug 17, 2023

dwnusbaum commented Aug 17, 2023

olamy commented Aug 18, 2023

timja commented Aug 19, 2023

dwnusbaum commented Aug 14, 2023 •

edited

Loading

dwnusbaum Aug 14, 2023 •

edited

Loading

dwnusbaum Aug 14, 2023 •

edited

Loading