From cd71f6b1e728e6d19bc96a33de9c54e5fcd30b0b Mon Sep 17 00:00:00 2001 From: kurok <22548029+kurok@users.noreply.github.com> Date: Fri, 18 Sep 2026 18:57:37 +0100 Subject: [PATCH] feat: restrict who is shown a buildAddUrl link, and enforce it on the redirect buildAddUrl(title: 'Deploy to PROD', url: '/job/deploy-prod/parambuild/?v=1.2.3', visibleTo: [users: ['alice'], groups: ['release-managers']]) Optional. Listing nothing means no narrowing, and behaviour is unchanged. The upstream proposal this comes from returned null out of getIconFileName and stopped there. That hides an icon, which is not access control: the target URL is in the page source, and this fork serves the link as a Stapler-routed redirect whose path is derivable from the title and url. So the check runs in all three places a link surfaces -- the classic sidebar icon, the details-bar Detail, and doIndex, which now answers 404 rather than redirecting someone who was not meant to see it. The lgtm[jenkins/no-permission-check] justification on doIndex is gone because it no longer applies. Upstream also hand-rolled membership against User.getAuthorities() and ignored the authorization strategy, which hid the link from an administrator who was not on the list. This only narrows what Jenkins has already granted: it cannot widen anything, and an unset visibleTo is everyone who can read the build. Two tests fail without the enforcement, one per surface, the second being the one that matters: aRestrictedLinkIsHiddenAndRefusedForEveryoneElse no sidebar icon ==> expected: but was: aGroupGrantsVisibility expected: <404> but was: <302> That 302 is upstream's design failing exactly as described: hidden in the UI, served on request. The step also gains a config.jelly, since it had none and both the freestyle form and the Snippet Generator rendered an empty panel. The form binds through text properties rather than the lists: a textarea submits one string, and binding that onto a List stores the list's own toString as a single element, so a round trip of ["alice"] came back as ["[alice]"]. It rendered perfectly while quietly corrupting what was typed, which only the round-trip test showed. README documents visibleTo, and says plainly that it is not the security boundary for deploying: the target job still enforces Item.BUILD, and this decides who is shown a shortcut. Signed-off-by: kurok <22548029+kurok@users.noreply.github.com> --- README.md | 23 +++ .../environmentdashboard/BuildAddUrl.java | 170 +++++++++++++++++- .../BuildUrlDetailFactory.java | 5 + .../BuildAddUrl/VisibleTo/config.jelly | 15 ++ .../BuildAddUrl/config.jelly | 14 ++ .../environmentdashboard/BuildAddUrlTest.java | 106 +++++++++++ 6 files changed, 329 insertions(+), 4 deletions(-) create mode 100644 src/main/resources/org/jenkinsci/plugins/environmentdashboard/BuildAddUrl/VisibleTo/config.jelly create mode 100644 src/main/resources/org/jenkinsci/plugins/environmentdashboard/BuildAddUrl/config.jelly diff --git a/README.md b/README.md index 146dbcb..4dd5cd7 100644 --- a/README.md +++ b/README.md @@ -82,6 +82,29 @@ node { ![Sidebar](docs/images/deploy-action.png) +#### Restricting who is shown a link + +A link can be narrowed to particular people with the optional `visibleTo`: + +```groovy +buildAddUrl(title: 'Deploy to PROD', url: '/job/deploy-prod/parambuild/?version=1.2.3', + visibleTo: [users: ['alice'], groups: ['release-managers']]) +``` + +Listing neither users nor groups means no narrowing, and the link behaves as it +always has. When something is listed, the link is hidden from everyone else in +the sidebar and the details bar, **and its URL answers 404** — hiding an icon +is not access control, since the target is in the page source and the link's +path is derivable. + +This only narrows; it cannot widen. Jenkins has already decided who may read +the build before any of this runs. + +**It is not the security boundary for deploying.** The job the link points at +enforces its own permissions -- a `parambuild` URL still requires `Item.BUILD` +on that job. `visibleTo` decides who is shown a shortcut, not who may use what +it points at. + #### Notes on modern Jenkins (plugin 0.2.0+) Since 0.2.0 the button no longer exposes the raw target URL as the action URL. diff --git a/src/main/java/org/jenkinsci/plugins/environmentdashboard/BuildAddUrl.java b/src/main/java/org/jenkinsci/plugins/environmentdashboard/BuildAddUrl.java index b4cfdf3..5b0be43 100644 --- a/src/main/java/org/jenkinsci/plugins/environmentdashboard/BuildAddUrl.java +++ b/src/main/java/org/jenkinsci/plugins/environmentdashboard/BuildAddUrl.java @@ -6,8 +6,10 @@ import hudson.FilePath; import hudson.Launcher; import hudson.Util; +import hudson.model.AbstractDescribableImpl; import hudson.model.AbstractProject; import hudson.model.Action; +import hudson.model.Descriptor; import hudson.model.Run; import hudson.model.TaskListener; import hudson.tasks.BuildStepDescriptor; @@ -16,18 +18,26 @@ import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; import java.util.Locale; +import jenkins.model.Jenkins; import jenkins.tasks.SimpleBuildStep; import org.jenkins.ui.icon.IconSpec; import org.jenkinsci.Symbol; import org.kohsuke.stapler.DataBoundConstructor; +import org.kohsuke.stapler.DataBoundSetter; import org.kohsuke.stapler.StaplerRequest2; import org.kohsuke.stapler.StaplerResponse2; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; public class BuildAddUrl extends Builder implements SimpleBuildStep { private final String title; private final String url; + private VisibleTo visibleTo; @DataBoundConstructor public BuildAddUrl(String title, String url) { @@ -43,6 +53,15 @@ public String getUrl() { return url; } + public VisibleTo getVisibleTo() { + return visibleTo; + } + + @DataBoundSetter + public void setVisibleTo(VisibleTo visibleTo) { + this.visibleTo = visibleTo; + } + @Override public BuildStepMonitor getRequiredMonitorService() { return BuildStepMonitor.NONE; @@ -56,7 +75,97 @@ public void perform( @NonNull Launcher launcher, @NonNull TaskListener listener) throws InterruptedException, IOException { - run.addAction(new BuildUrlAction(title, url)); + run.addAction(new BuildUrlAction(title, url, visibleTo)); + } + + /** + * Optional narrowing of who is shown a link, by user id or group name. + * + *

Deliberately only principal matching on top of what the authorization + * strategy has already granted. The upstream proposal hand-rolled a + * membership model that ignored the strategy entirely, which both hid the + * link from administrators who were not listed and showed it to listed + * users with no permission on the target. + */ + public static class VisibleTo extends AbstractDescribableImpl { + private List users; + private List groups; + + @DataBoundConstructor + public VisibleTo(List users, List groups) { + this.users = clean(users); + this.groups = clean(groups); + } + + private static List clean(List raw) { + if (raw == null) { + return Collections.emptyList(); + } + List out = new ArrayList<>(raw.size()); + for (String entry : raw) { + String trimmed = Util.fixEmptyAndTrim(entry); + if (trimmed != null) { + out.add(trimmed); + } + } + return Collections.unmodifiableList(out); + } + + private static List split(String text) { + if (Util.fixEmptyAndTrim(text) == null) { + return Collections.emptyList(); + } + return clean(java.util.Arrays.asList(text.split("[,\r\n]"))); + } + + public List getUsers() { + return users; + } + + public List getGroups() { + return groups; + } + + /* + * The form binds through these rather than through the lists directly. + * A textarea submits one string, and binding that straight onto a + * List stores the list's own toString as a single element: a + * round trip of ["alice"] came back as ["[alice]"]. The form rendered + * perfectly well while quietly corrupting what was typed into it, which + * a round-trip test is the only thing that shows. + */ + public String getUsersText() { + return String.join("\n", users); + } + + @DataBoundSetter + public void setUsersText(String usersText) { + this.users = split(usersText); + } + + public String getGroupsText() { + return String.join("\n", groups); + } + + @DataBoundSetter + public void setGroupsText(String groupsText) { + this.groups = split(groupsText); + } + + /** No principals listed means no narrowing, not "nobody". */ + public boolean isEmpty() { + return users.isEmpty() && groups.isEmpty(); + } + + @Extension + @Symbol("visibleTo") + public static class DescriptorImpl extends Descriptor { + @Override + @NonNull + public String getDisplayName() { + return "Visible to"; + } + } } @Extension @@ -88,14 +197,62 @@ public boolean isApplicable(Class t) { public static class BuildUrlAction implements Action, IconSpec { private final String title; private final String url; + private final VisibleTo visibleTo; BuildUrlAction(String title, String url) { + this(title, url, null); + } + + BuildUrlAction(String title, String url, VisibleTo visibleTo) { this.title = title; this.url = url; + this.visibleTo = visibleTo; + } + + public VisibleTo getVisibleTo() { + return visibleTo; + } + + /** + * Whether this link exists at all for the given caller. + * + *

An unset {@code visibleTo} means everyone who can read the build, + * which is what Jenkins has already decided by the time anything here + * runs. When it is set, this narrows that further; it never widens it, + * so it cannot grant anyone access the authorization strategy withheld. + * + *

It is also not the security boundary for deploying. The target of + * the link enforces its own permissions -- a {@code parambuild} URL + * still requires Item.BUILD on the job it points at. This decides who + * is shown a shortcut. + */ + public boolean isVisibleTo(Authentication authentication) { + if (visibleTo == null || visibleTo.isEmpty()) { + return true; + } + if (authentication == null) { + return false; + } + if (visibleTo.getUsers().contains(authentication.getName())) { + return true; + } + for (GrantedAuthority authority : authentication.getAuthorities()) { + if (authority.getAuthority() != null && visibleTo.getGroups().contains(authority.getAuthority())) { + return true; + } + } + return false; + } + + public boolean isVisible() { + return isVisibleTo(Jenkins.getAuthentication2()); } @Override public String getIconFileName() { + if (!isVisible()) { + return null; + } // Hardcoded artifact id: getClass().getPackage().getImplementationTitle() // is unreliable under modern plugin classloaders (may return null). return "/plugin/deploy-dashboard/deploy.png"; @@ -103,7 +260,7 @@ public String getIconFileName() { @Override public String getIconClassName() { - return "symbol-rocket-outline plugin-ionicons-api"; + return isVisible() ? "symbol-rocket-outline plugin-ionicons-api" : null; } @Override @@ -149,10 +306,15 @@ boolean isSafeUrl() { * sidebar and the details bar), it has no side effects, and it is only * reachable through the run's URL, which Jenkins already gates on * Item.READ while resolving the job and the build. + * + *

It does check {@code visibleTo}. Hiding the icon is not access + * control -- the URL is in the page source, and this endpoint has a + * derivable path -- so a link someone is not meant to see answers 404 + * here rather than merely being absent from their sidebar. */ - // lgtm[jenkins/csrf] lgtm[jenkins/no-permission-check] + // lgtm[jenkins/csrf] public void doIndex(StaplerRequest2 req, StaplerResponse2 rsp) throws IOException { - if (!isSafeUrl()) { + if (!isSafeUrl() || !isVisible()) { rsp.sendError(StaplerResponse2.SC_NOT_FOUND); return; } diff --git a/src/main/java/org/jenkinsci/plugins/environmentdashboard/BuildUrlDetailFactory.java b/src/main/java/org/jenkinsci/plugins/environmentdashboard/BuildUrlDetailFactory.java index 27dfc64..912d752 100644 --- a/src/main/java/org/jenkinsci/plugins/environmentdashboard/BuildUrlDetailFactory.java +++ b/src/main/java/org/jenkinsci/plugins/environmentdashboard/BuildUrlDetailFactory.java @@ -29,6 +29,11 @@ public Class type() { @NonNull public List createFor(@NonNull Run target) { return target.getActions(BuildUrlAction.class).stream() + // The details bar is the third place a link surfaces, next to + // the classic sidebar and the redirect endpoint itself. All + // three have to agree, or "hidden" means only "hidden in one + // of the places you might look". + .filter(BuildUrlAction::isVisible) .map(action -> new BuildUrlDetail(target, action)) .collect(Collectors.toList()); } diff --git a/src/main/resources/org/jenkinsci/plugins/environmentdashboard/BuildAddUrl/VisibleTo/config.jelly b/src/main/resources/org/jenkinsci/plugins/environmentdashboard/BuildAddUrl/VisibleTo/config.jelly new file mode 100644 index 0000000..9d53c3c --- /dev/null +++ b/src/main/resources/org/jenkinsci/plugins/environmentdashboard/BuildAddUrl/VisibleTo/config.jelly @@ -0,0 +1,15 @@ + + + + + + + + + + diff --git a/src/main/resources/org/jenkinsci/plugins/environmentdashboard/BuildAddUrl/config.jelly b/src/main/resources/org/jenkinsci/plugins/environmentdashboard/BuildAddUrl/config.jelly new file mode 100644 index 0000000..19e524f --- /dev/null +++ b/src/main/resources/org/jenkinsci/plugins/environmentdashboard/BuildAddUrl/config.jelly @@ -0,0 +1,14 @@ + + + + + + + + + + + diff --git a/src/test/java/org/jenkinsci/plugins/environmentdashboard/BuildAddUrlTest.java b/src/test/java/org/jenkinsci/plugins/environmentdashboard/BuildAddUrlTest.java index cc0d4e5..98bd2c8 100644 --- a/src/test/java/org/jenkinsci/plugins/environmentdashboard/BuildAddUrlTest.java +++ b/src/test/java/org/jenkinsci/plugins/environmentdashboard/BuildAddUrlTest.java @@ -3,10 +3,16 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; +import hudson.model.FreeStyleProject; +import hudson.model.User; +import hudson.security.ACL; +import hudson.security.ACLContext; import java.net.URL; import java.util.List; +import jenkins.model.Jenkins; import jenkins.model.details.Detail; import org.htmlunit.WebRequest; import org.htmlunit.WebResponse; @@ -23,6 +29,27 @@ class BuildAddUrlTest { private static final String TARGET = "/job/app-deploy/parambuild/?imageRegistry=reg.example.com/app&version=1.2.3&revision=abc123&build_url=https://ci.example.com/job/app/1/"; + /** A run whose link is only meant for the listed principals. */ + private static WorkflowRun runWithRestrictedLink(JenkinsRule j, String name, String visibleTo) throws Exception { + WorkflowJob job = j.createProject(WorkflowJob.class, name); + job.setDefinition(new CpsFlowDefinition( + "node { buildAddUrl(title: 'Deploy to PROD', url: '/job/deploy/parambuild/?v=1', visibleTo: [" + + visibleTo + "]) }", + true)); + return j.buildAndAssertSuccess(job); + } + + private static int statusFor(JenkinsRule j, WorkflowRun run, String as) throws Exception { + BuildAddUrl.BuildUrlAction action = run.getAction(BuildAddUrl.BuildUrlAction.class); + try (JenkinsRule.WebClient wc = j.createWebClient()) { + wc.login(as); + wc.getOptions().setRedirectEnabled(false); + wc.getOptions().setThrowExceptionOnFailingStatusCode(false); + return wc.loadWebResponse(new WebRequest(new URL(j.getURL(), run.getUrl() + action.getUrlName() + "/"))) + .getStatusCode(); + } + } + private static WorkflowRun runWithLink(JenkinsRule j, String url) throws Exception { WorkflowJob job = j.createProject(WorkflowJob.class, "app-" + Math.abs(url.hashCode())); job.setDefinition( @@ -82,6 +109,85 @@ void protocolRelativeUrlIsNotRedirected(JenkinsRule j) throws Exception { assertFalse(action.isSafeUrl()); } + @Test + void aRestrictedLinkIsHiddenAndRefusedForEveryoneElse(JenkinsRule j) throws Exception { + // Upstream's version returned null from getIconFileName and stopped + // there. The URL is still in the page source and the endpoint's path is + // derivable, so hiding an icon protects nothing. All three surfaces get + // asserted here, and the redirect is the one that matters. + j.jenkins.setSecurityRealm(j.createDummySecurityRealm()); + WorkflowRun run = runWithRestrictedLink(j, "restricted", "users: ['alice']"); + BuildAddUrl.BuildUrlAction action = run.getAction(BuildAddUrl.BuildUrlAction.class); + assertNotNull(action); + + try (ACLContext ignored = ACL.as2(User.getById("alice", true).impersonate2())) { + assertTrue(action.isVisible(), "the listed user must see the link"); + assertNotNull(action.getIconFileName()); + assertNotNull(action.getIconClassName()); + assertEquals(1, new BuildUrlDetailFactory().createFor(run).size()); + } + try (ACLContext ignored = ACL.as2(User.getById("bob", true).impersonate2())) { + assertFalse(action.isVisible(), "an unlisted user must not see the link"); + assertNull(action.getIconFileName(), "no sidebar icon"); + assertNull(action.getIconClassName(), "no details-bar icon"); + assertEquals(0, new BuildUrlDetailFactory().createFor(run).size(), "no details-bar entry"); + } + + assertEquals(302, statusFor(j, run, "alice"), "the listed user must still be redirected"); + assertEquals(404, statusFor(j, run, "bob"), "the redirect itself must refuse an unlisted user"); + } + + @Test + void aGroupGrantsVisibility(JenkinsRule j) throws Exception { + org.jvnet.hudson.test.JenkinsRule.DummySecurityRealm realm = j.createDummySecurityRealm(); + realm.addGroups("dave", "release-managers"); + j.jenkins.setSecurityRealm(realm); + + WorkflowRun run = runWithRestrictedLink(j, "grouped", "groups: ['release-managers']"); + BuildAddUrl.BuildUrlAction action = run.getAction(BuildAddUrl.BuildUrlAction.class); + + try (ACLContext ignored = ACL.as2(User.getById("dave", true).impersonate2())) { + assertTrue(action.isVisible(), "a member of the listed group must see the link"); + } + try (ACLContext ignored = ACL.as2(User.getById("carol", true).impersonate2())) { + assertFalse(action.isVisible(), "membership grants it, not merely being a user"); + } + assertEquals(302, statusFor(j, run, "dave")); + assertEquals(404, statusFor(j, run, "carol")); + } + + @Test + void withoutVisibleToTheLinkIsShownToAnyoneWhoCanReadTheBuild(JenkinsRule j) throws Exception { + // The narrowing is opt-in; absent it, nothing changes. + WorkflowRun run = runWithLink(j, TARGET); + BuildAddUrl.BuildUrlAction action = run.getAction(BuildAddUrl.BuildUrlAction.class); + + try (ACLContext ignored = ACL.as2(Jenkins.ANONYMOUS2)) { + assertTrue(action.isVisible(), "an unrestricted link stays visible"); + assertNotNull(action.getIconFileName()); + assertEquals(1, new BuildUrlDetailFactory().createFor(run).size()); + } + } + + @Test + void theFreestyleFormRoundTripsVisibleTo(JenkinsRule j) throws Exception { + // The step's form is new, and a form that renders but does not bind is + // worse than none: it silently discards what was typed into it. + FreeStyleProject job = j.createFreeStyleProject("form-roundtrip"); + BuildAddUrl step = new BuildAddUrl("Deploy to PROD", "/job/deploy/parambuild/?v=1"); + step.setVisibleTo(new BuildAddUrl.VisibleTo(List.of("alice"), List.of("release-managers"))); + job.getBuildersList().add(step); + + j.configRoundtrip(job); + + BuildAddUrl saved = job.getBuildersList().get(BuildAddUrl.class); + assertNotNull(saved, "the build step must survive a configuration round trip"); + assertEquals("Deploy to PROD", saved.getTitle()); + assertNotNull(saved.getVisibleTo(), "visibleTo must survive the round trip"); + assertEquals(List.of("alice"), saved.getVisibleTo().getUsers()); + assertEquals(List.of("release-managers"), saved.getVisibleTo().getGroups()); + } + @Test void detailFactoryExposesLinkOnDetailsBar(JenkinsRule j) throws Exception { WorkflowRun run = runWithLink(j, TARGET);