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

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

Merged
merged 7 commits into from
Aug 17, 2023
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
* The MIT License
*
* Copyright 2023 CloudBees, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/

package org.jenkinsci.plugins.displayurlapi;

import edu.umd.cs.findbugs.annotations.CheckForNull;
import hudson.Extension;
import hudson.ExtensionList;
import hudson.Util;
import hudson.util.ListBoxModel;
import java.util.Objects;
import jenkins.model.GlobalConfiguration;
import org.jenkinsci.plugins.displayurlapi.user.PreferredProviderUserProperty;
import org.kohsuke.stapler.DataBoundSetter;

@Extension
public class DefaultProviderGlobalConfiguration extends GlobalConfiguration {

public DefaultProviderGlobalConfiguration() {
load();
}

private @CheckForNull String providerId;

public @CheckForNull String getProviderId() {
return providerId;
}

@DataBoundSetter
public void setProviderId(@CheckForNull String providerId) {
providerId = Util.fixEmptyAndTrim(providerId);
if (PreferredProviderUserProperty.ProviderOption.DEFAULT_OPTION.getId().equals(providerId)) {
providerId = null;
}
this.providerId = providerId;
save();
}

public @CheckForNull DisplayURLProvider getConfiguredProvider() {
if (providerId == null) {
return null;
}
return DisplayURLProvider.all().stream()
.filter(provider -> provider.getClass().getName().equals(providerId))
.findFirst()
.orElse(null);
}

public static DefaultProviderGlobalConfiguration get() {
return ExtensionList.lookupSingleton(DefaultProviderGlobalConfiguration.class);
}

public ListBoxModel doFillProviderIdItems() {
ListBoxModel items = new ListBoxModel();
for (PreferredProviderUserProperty.ProviderOption providerOption : PreferredProviderUserProperty.getAll()) {
ListBoxModel.Option option = new ListBoxModel.Option(
providerOption.getName(),
providerOption.getId(),
Objects.equals(providerOption.getId(), providerId)
);
items.add(option);
}
return items;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ public abstract class DisplayURLProvider implements ExtensionPoint {
* @return DisplayURLProvider
*/
public static DisplayURLProvider get() {
// 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.
Copy link
Member Author

@dwnusbaum dwnusbaum Aug 14, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

DisplayURLProvider preferredProvider = getPreferredProvider();
return preferredProvider != null ? preferredProvider : DisplayURLProviderImpl.INSTANCE;
}
Expand Down Expand Up @@ -189,6 +193,8 @@ public static DisplayURLProvider getPreferredProvider() {
if (prefProperty != null && prefProperty.getConfiguredProvider() != null) {
return prefProperty.getConfiguredProvider();
}
// 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.
Copy link
Member Author

@dwnusbaum dwnusbaum Aug 14, 2023

Choose a reason for hiding this comment

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

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

String clazz = findClass();
if (isNotEmpty(clazz)) {
return ExtensionList.lookup(DisplayURLProvider.class).getDynamic(clazz);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import java.util.function.Predicate;
import org.apache.commons.lang.StringUtils;
import org.jenkinsci.plugins.displayurlapi.ClassicDisplayURLProvider;
import org.jenkinsci.plugins.displayurlapi.DefaultProviderGlobalConfiguration;
import org.jenkinsci.plugins.displayurlapi.DisplayURLProvider;
import org.jenkinsci.plugins.displayurlapi.user.PreferredProviderUserProperty;
import org.kohsuke.stapler.StaplerRequest;
Expand Down Expand Up @@ -61,11 +62,16 @@ DisplayURLProvider lookupProvider(StaplerRequest req) {
}

DisplayURLProvider lookupProvider() {
dwnusbaum marked this conversation as resolved.
Show resolved Hide resolved
// Check user preferences, then the global default (if any), then fall back to the extension with the highest ordinal value.
PreferredProviderUserProperty prefProperty = getUserPreferredProviderProperty();

if (prefProperty != null && prefProperty.getConfiguredProvider() != null) {
return prefProperty.getConfiguredProvider();
}
DisplayURLProvider globalProvider = DefaultProviderGlobalConfiguration.get().getConfiguredProvider();
if (globalProvider != null) {
return globalProvider;
}
DisplayURLProvider displayURLProvider = DisplayURLProvider.getPreferredProvider();
if (displayURLProvider == null) {
ExtensionList<DisplayURLProvider> all = DisplayURLProvider.all();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public DisplayURLProvider getConfiguredProvider() {
.orElse(null);
}

public List<ProviderOption> getAll() {
public static List<ProviderOption> getAll() {
List<ProviderOption> options = DisplayURLProvider.all().stream()
.map(input -> new ProviderOption(input.getClass().getName(), input.getDisplayName()))
.collect(Collectors.toList());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public UserProperty newInstance(User user) {
public ListBoxModel doFillProviderIdItems() {
ListBoxModel items = new ListBoxModel();
PreferredProviderUserProperty property = PreferredProviderUserProperty.forCurrentUser();
for (ProviderOption providerOption : property.getAll()) {
for (ProviderOption providerOption : PreferredProviderUserProperty.getAll()) {
ListBoxModel.Option option = new ListBoxModel.Option(
providerOption.getName(),
providerOption.getId(),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout"
xmlns:t="/lib/hudson" xmlns:f="/lib/form">
<f:section title="${%Default notification URL handler}">
dwnusbaum marked this conversation as resolved.
Show resolved Hide resolved
<f:entry field="providerId">
<f:select/>
</f:entry>
</f:section>
</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<div>
Select the default handler when clicking links to Jenkins from notifications (e.g. Email, Slack, or GitHub).
Individual users can override this value as desired in their preferences.
</div>
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@
import hudson.model.Run;
import hudson.model.User;
import hudson.security.ACL;
import hudson.security.ACLContext;
import jenkins.model.Jenkins;
import org.jenkinsci.plugins.displayurlapi.actions.RunDisplayAction;
import org.jenkinsci.plugins.displayurlapi.user.PreferredProviderUserProperty;
import org.junit.Rule;
import org.junit.Test;
Expand Down Expand Up @@ -270,6 +272,26 @@ public void decoration() throws Exception {
assertEquals(DisplayURLProvider.get().getJobURL(project), environment.get("JOB_DISPLAY_URL"));
}

@Test
public void providerConfigurationPrecedence() throws Exception {
rule.jenkins.setSecurityRealm(rule.createDummySecurityRealm());
// user1 does not have a preference, but user2 does.
User user1 = User.getById("user1", true);
User user2 = User.getById("user2", true);
user2.addProperty(new PreferredProviderUserProperty(ClassicDisplayURLProvider.class.getName()));
// admin configures TestUserDisplayURLProvider as the default provider.
DefaultProviderGlobalConfiguration.get().setProviderId(TestUserDisplayURLProvider.class.getName());
FreeStyleProject p = rule.createFreeStyleProject();
Run<?, ?> b = rule.buildAndAssertSuccess(p);
RunDisplayAction action = b.getAction(RunDisplayAction.class);
try (ACLContext unused = ACL.as2(user1.impersonate2())) {
assertEquals(rule.getURL() + b.getUrl() + TestUserDisplayURLProvider.EXTRA_CONTENT_IN_URL, action.getDisplayUrl());
}
try (ACLContext unused = ACL.as2(user2.impersonate2())) {
assertEquals(rule.getURL() + b.getUrl(), action.getDisplayUrl());
}
}

@TestExtension("decoration")
public static class DisplayURLDecoratorImpl extends DisplayURLDecorator {
@NonNull
Expand Down