[JENKINS-66382] check item permission instead of the Jenkins root lev… #31
Conversation
…el permission for ShelveProjectAction
| @@ -54,7 +54,7 @@ public boolean isShelvingProject() { | |||
| @POST | |||
| public HttpResponse doShelveProject() | |||
| throws IOException, ServletException { | |||
| Jenkins.getInstance().checkPermission(Item.DELETE); | |||
| getItem().checkPermission(Item.DELETE); | |||
There was a problem hiding this comment.
If you protect against item being null line 34 (impossible in the production code, but some tests are using it), then you should also protect here
There was a problem hiding this comment.
Done, I checked and now it throws a ServletException in case a null is found. Additionaly, adding this check, we save the NullPointerException in the following LOGGER.info method.
| private static String getShelveIconPath() { | ||
| return Jenkins.getInstance().hasPermission(SHELVE_PERMISSION) ? ACTION_ICON_PATH : null; | ||
| private static String getShelveIconPath(Item item) { | ||
| return (item != null) ? item.hasPermission(SHELVE_PERMISSION) ? ACTION_ICON_PATH : null : null; |
There was a problem hiding this comment.
The method should not be static anymore. You can also simplify the code, instead of two ternary operators, hard to read, you can use one: item != null && item.hasPermission(SHELVE_PERMISSION) ? ...
| @@ -21,37 +24,39 @@ | |||
| public class ShelveProjectActionTest { | |||
| @Rule | |||
| public JenkinsRule jenkinsRule = new JenkinsRule(); | |||
| private FreeStyleProject project = null; | |||
There was a problem hiding this comment.
nit: we don't care about it being a FreeStyleProject:
| private FreeStyleProject project = null; | |
| private Item project; |
(you'll also have to modify the imports)
src/test/java/org/jvnet/hudson/plugins/shelveproject/ShelveProjectActionTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/jvnet/hudson/plugins/shelveproject/ShelveProjectActionTest.java
Outdated
Show resolved
Hide resolved
…jectActionTest.java Co-authored-by: Pierre Beitz <[email protected]>
…jectActionTest.java Co-authored-by: Pierre Beitz <[email protected]>
|
Thank @PierreBtz so much for the peer review. We will wait for the resolution of the issue inheritance question. |
…el permission for ShelveProjectAction
Checking the item permission we will review the
job-deletepermission for the current item, instead of thejob-deletepermission at the root level. Having this option, we will be able to grantshelvepermission to a specific job (instead to all jobs) using Role Based Strategy and Project Based Matrix Authorization Strategy.