Skip to content

Switch to Manage permission and future Jenkins compatibility - #126

Merged
NotMyFault merged 2 commits into
jenkinsci:masterfrom
jtnord:future-jenkins-compatability
Jun 19, 2025
Merged

Switch to Manage permission and future Jenkins compatibility#126
NotMyFault merged 2 commits into
jenkinsci:masterfrom
jtnord:future-jenkins-compatability

Conversation

@jtnord

@jtnord jtnord commented Jun 19, 2025

Copy link
Copy Markdown
Member

Compatability with Future Jenkins version

The code was incorrectly checking the permission ${it.ADMINISTER}
however it when viewing the root of Jenkins (and other views) is an
instance of AllView. As such the permission check failed silently and
was then not shown.

jenkinsci/jenkins#10678 sets ${it} to Jenkins
now, and then this causes the incorrect permission check to blow up as
it is a single permission that is set for permissions (which expects an
array).

This fixes the issue and actually removes the permission check from the
task, as the permission is already checked in the layout.

We now take advantage of the Manage permission also so that users
without Jenkins.ADMINISTER but with Jenkins.MANAGE can clear the build
queue.

Obsoletes #124

Testing done

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 demonstrate the feature works or the issue is fixed

jtnord added 2 commits June 19, 2025 11:36
The code was incorrectly checking the permission ${it.ADMINISTER}
however it when vewing the root of Jenkins (and other views) is an
instance of AllView.  As such the permission check failed silently and
was then not shown.

jenkinsci/jenkins#10678 sets ${it} to Jenkins
now, and then this causes the incorrect permission check to blow up as
it is a single permission that is set for permissions (which expects an
array).

This fixes the issue and actually removes the permission check from the
task, as the permission is already checked in the layout.

We now take advantage of the Manage permission also so that users
without Jenkins.ADMINISTER but with JENKINS.MANAGE can clear the build
queue.
@jtnord
jtnord requested a review from a team as a code owner June 19, 2025 10:48
Comment thread pom.xml
<groupId>io.jenkins.tools.bom</groupId>
<artifactId>bom-${jenkins.baseline}.x</artifactId>
<version>3696.vb_b_4e2d1a_0542</version>
<version>4710.v016f0a_07e34d</version>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

lowest bound of the bom

Comment thread pom.xml
<changelist>999999-SNAPSHOT</changelist>
<jenkins.baseline>2.452</jenkins.baseline>
<jenkins.version>${jenkins.baseline}.4</jenkins.version>
<jenkins.baseline>2.504</jenkins.baseline>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

lowest LTS where the MANAGE permission is not Beta

@RequirePOST
public void doPurge(final StaplerRequest request, final StaplerResponse response) throws ServletException, IOException {
Jenkins.get().checkPermission(Jenkins.ADMINISTER);
Jenkins.get().checkPermission(Jenkins.MANAGE);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Switch to MANAGE as this does not allow arbirary command execution or esclation of privalledges

<l:hasAdministerOrManage>
<l:task icon="symbol-trash-outline plugin-ionicons-api" confirmationMessage="${%Purge the build queue?}"
href="purge-build-queue/purge/" post="true" requiresConfirmation="true"
title="${%Purge Build Queue}" permissions="${it.ADMINISTER}">

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

permissions="${it.ADMINISTER}" was the bug that caused this not to be compatable.

it is not Jenkins in all current versions as such it.ADMINISTER would return null and then the permission would not be checked.
In Future version it will be an instance of Jenkins and as such this becomes valid, but it is a single permission when permissions is expecting an array.

Given this is already protected by the permission check in layout we can simply rmeove the extra check.

@jtnord jtnord mentioned this pull request Jun 19, 2025
6 tasks
@jtnord

jtnord commented Jun 19, 2025

Copy link
Copy Markdown
Member Author

build fails due to the lack of "trust" to change the Jenkinsfile which is needed to remove Java11 support.

@jtnord jtnord changed the title Future jenkins compatability and switch to Manage permission Switch to Manage permission and future Jenkins compatibility Jun 19, 2025
@NotMyFault NotMyFault added the bug label Jun 19, 2025

@NotMyFault NotMyFault left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Replayed, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants