[JENKINS-41684] Ensure that PluginManager.dynamicLoad runs as SYSTEM #2732
Conversation
…YSTEM.
Test plugin source:
package test;
import hudson.Plugin;
import jenkins.model.Jenkins;
public class ThePlugin extends Plugin {
@OverRide
public void postInitialize() throws Exception {
Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER);
}
}
|
Test failures look unrelated. |
| @@ -819,6 +819,7 @@ public void dynamicLoad(File arc) throws IOException, InterruptedException, Rest | |||
| */ | |||
| @Restricted(NoExternalUse.class) | |||
| public void dynamicLoad(File arc, boolean removeExisting) throws IOException, InterruptedException, RestartRequiredException { | |||
| try (ACLContext context = ACL.as(ACL.SYSTEM)) { | |||
There was a problem hiding this comment.
Do we really want to not indent this entire block?
There was a problem hiding this comment.
I wanted to create a minimal diff, since the intervening block is quite long.
There was a problem hiding this comment.
Maybe it is reasonable to keep the entire method within the clause anyway. There are extension points like PluginStrategy#updateDependency(), which may also blow up in the case of the non-System user
There was a problem hiding this comment.
I wanted to create a minimal diff, since the intervening block is quite long.
and max efforts to read logic later
There was a problem hiding this comment.
Extract to private method should also provide short diff.
There was a problem hiding this comment.
Would really prefer if something would be done here for readability.
| @@ -819,6 +819,7 @@ public void dynamicLoad(File arc) throws IOException, InterruptedException, Rest | |||
| */ | |||
| @Restricted(NoExternalUse.class) | |||
| public void dynamicLoad(File arc, boolean removeExisting) throws IOException, InterruptedException, RestartRequiredException { | |||
| try (ACLContext context = ACL.as(ACL.SYSTEM)) { | |||
There was a problem hiding this comment.
Maybe it is reasonable to keep the entire method within the clause anyway. There are extension points like PluginStrategy#updateDependency(), which may also blow up in the case of the non-System user
jglick
added
the
ready-for-merge
|
@reviewbybees done |
|
Reindented. I am going to be the one resolving the nasty merge conflicts here in the future so I think this was a bad idea, but I am sick of trying to explain to people how important minimal diffs are. |
| @@ -819,100 +819,102 @@ public void dynamicLoad(File arc) throws IOException, InterruptedException, Rest | |||
| */ | |||
| @Restricted(NoExternalUse.class) | |||
| public void dynamicLoad(File arc, boolean removeExisting) throws IOException, InterruptedException, RestartRequiredException { | |||
| LOGGER.info("Attempting to dynamic load "+arc); | |||
There was a problem hiding this comment.
Now you are going to have use this to understand what is going on.
You can make private method and you will have small diff. Making code messed will definitely make you alone to such parts of code. |
|
👍 |
Yes, but at a cost of added complexity worse than either the diff size or nonstandard indentation. |
JENKINS-41684
@reviewbybees