feat: restrict who is shown a buildAddUrl link, and enforce it on the redirect - #44
Merged
Merged
Conversation
… 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: <null> but was: </plugin/.../deploy.png>
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<String> 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #41.
Optional — listing nothing means no narrowing, and existing behaviour is unchanged.
Why not upstream's version
It returned
nullfromgetIconFileName()and stopped. That hides an icon, which is not access control: the target URL is in the page source, and on this fork the link is 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 sidebar icon, the details-bar
Detail, anddoIndex, which now answers 404 instead of redirecting someone who was never meant to see it. Thelgtm[jenkins/no-permission-check]justification ondoIndexis gone, because it no longer applies.Upstream also hand-rolled membership against
User.getAuthorities()while ignoring 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.The tests fail without the enforcement, one per surface
That 302 is upstream's design failing exactly as described — hidden in the UI, served on request.
A bug the round-trip test caught
The step had no
config.jellyat all, so the freestyle form and the Snippet Generator both rendered an empty panel. Adding one exposed this:A textarea submits one string, and binding that onto a
List<String>stores the list's owntoString()as a single element. The form rendered perfectly while quietly corrupting what was typed into it. It binds through text properties now. I would not have found this without writing the round trip, and a form that discards input is worse than no form.Scope, stated in the README
visibleTois not the security boundary for deploying. The job the link points at enforces its own permissions — aparambuildURL still requiresItem.BUILDthere. This decides who is shown a shortcut, and the README says so where someone configuring it will read it.Local
mvn clean verify: 44 tests, green.