Skip to content

Commit

Permalink
[JENKINS-41684] Ensure that PluginManager.dynamicLoad runs as SYSTEM (#…
Browse files Browse the repository at this point in the history 
…2732)

* [FIXED JENKINS-41684] Ensure that PluginManager.dynamicLoad runs as SYSTEM.
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);
    }
}

* @daniel-beck wants this all reindented.
  • Loading branch information
@jglick @oleg-nenashev
jglick authored and oleg-nenashev committed Feb 12, 2017
1 parent f8b26a3 commit 6fb9e91
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 78 deletions.
158 changes: 80 additions & 78 deletions core/src/main/java/hudson/PluginManager.java
View file
Expand Up @@ -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);
PluginWrapper p = null;
String sn;
try {
sn = strategy.getShortName(arc);
} catch (AbstractMethodError x) {
LOGGER.log(WARNING, "JENKINS-12753 fix not active: {0}", x.getMessage());
p = strategy.createPluginWrapper(arc);
sn = p.getShortName();
}
PluginWrapper pw = getPlugin(sn);
if (pw!=null) {
if (removeExisting) { // try to load disabled plugins
for (Iterator<PluginWrapper> i = plugins.iterator(); i.hasNext();) {
pw = i.next();
if(sn.equals(pw.getShortName())) {
i.remove();
pw = null;
break;
try (ACLContext context = ACL.as(ACL.SYSTEM)) {
LOGGER.info("Attempting to dynamic load "+arc);
PluginWrapper p = null;
String sn;
try {
sn = strategy.getShortName(arc);
} catch (AbstractMethodError x) {
LOGGER.log(WARNING, "JENKINS-12753 fix not active: {0}", x.getMessage());
p = strategy.createPluginWrapper(arc);
sn = p.getShortName();
}
PluginWrapper pw = getPlugin(sn);
if (pw!=null) {
if (removeExisting) { // try to load disabled plugins
for (Iterator<PluginWrapper> i = plugins.iterator(); i.hasNext();) {
pw = i.next();
if(sn.equals(pw.getShortName())) {
i.remove();
pw = null;
break;
}
}
} else {
throw new RestartRequiredException(Messages._PluginManager_PluginIsAlreadyInstalled_RestartRequired(sn));
}
} else {
throw new RestartRequiredException(Messages._PluginManager_PluginIsAlreadyInstalled_RestartRequired(sn));
}
}
if (p == null) {
p = strategy.createPluginWrapper(arc);
}
if (p.supportsDynamicLoad()== YesNoMaybe.NO)
throw new RestartRequiredException(Messages._PluginManager_PluginDoesntSupportDynamicLoad_RestartRequired(sn));
if (p == null) {
p = strategy.createPluginWrapper(arc);
}
if (p.supportsDynamicLoad()== YesNoMaybe.NO)
throw new RestartRequiredException(Messages._PluginManager_PluginDoesntSupportDynamicLoad_RestartRequired(sn));

// there's no need to do cyclic dependency check, because we are deploying one at a time,
// so existing plugins can't be depending on this newly deployed one.
// there's no need to do cyclic dependency check, because we are deploying one at a time,
// so existing plugins can't be depending on this newly deployed one.

This comment has been minimized.

Sign in to view

Copy link
@stephenc

stephenc May 7, 2019

Member

This is an invalid assumption. Existing plugins may have an optional dependency on the newly deployed one. We can argue the merits of such a situation, but we are where we are, optional dependencies exist. I suspect the pragmatic solution in this case would be to detect up front and disable the dynamic install for such a plugin.

/cc @oleg-nenashev I give up chasing the git blame any further and I don't have time to fight with JIRA right now ;-)

This comment has been minimized.

Sign in to view

Copy link
@daniel-beck

daniel-beck May 7, 2019 •
edited

Member

This behavior existed for far longer than credentials/folders broke the job config screen on a new install, assuming that's what you're referring to. What changed?

This comment has been minimized.

Sign in to view

Copy link
@stephenc

stephenc May 7, 2019

Member

No change, I just spotted the invalid comment while chasing something else.

This comment has been minimized.

Sign in to view

Copy link
@oleg-nenashev

oleg-nenashev May 7, 2019

Member

Note that it was @jglick who submitted the change.
Would be nice if you guys chat with each other and create a follow-up task (or just send a summary to the dev list). I also do not have much time for this part in the foreseeable future

This comment has been minimized.

Sign in to view

Copy link
@jglick

jglick May 24, 2019

Author Member

credentials/folders broke the job config screen on a new install

No idea what this is referring to. If there is a reproducible bug, file it in JIRA please.

This comment has been minimized.

Sign in to view

Copy link
@daniel-beck

daniel-beck May 25, 2019

Member

@jglick https://issues.jenkins-ci.org/browse/JENKINS-57681

plugins.add(p);
if (p.isActive())
activePlugins.add(p);
synchronized (((UberClassLoader) uberClassLoader).loaded) {
((UberClassLoader) uberClassLoader).loaded.clear();
}

try {
p.resolvePluginDependencies();
strategy.load(p);
plugins.add(p);
if (p.isActive())
activePlugins.add(p);
synchronized (((UberClassLoader) uberClassLoader).loaded) {
((UberClassLoader) uberClassLoader).loaded.clear();
}

Jenkins.getInstance().refreshExtensions();
try {
p.resolvePluginDependencies();
strategy.load(p);

p.getPlugin().postInitialize();
} catch (Exception e) {
failedPlugins.add(new FailedPlugin(sn, e));
activePlugins.remove(p);
plugins.remove(p);
throw new IOException("Failed to install "+ sn +" plugin",e);
}
Jenkins.getInstance().refreshExtensions();

// run initializers in the added plugin
Reactor r = new Reactor(InitMilestone.ordering());
final ClassLoader loader = p.classLoader;
r.addAll(new InitializerFinder(loader) {
@Override
protected boolean filter(Method e) {
return e.getDeclaringClass().getClassLoader() != loader || super.filter(e);
p.getPlugin().postInitialize();
} catch (Exception e) {
failedPlugins.add(new FailedPlugin(sn, e));
activePlugins.remove(p);
plugins.remove(p);
throw new IOException("Failed to install "+ sn +" plugin",e);
}
}.discoverTasks(r));
try {
new InitReactorRunner().run(r);
} catch (ReactorException e) {
throw new IOException("Failed to initialize "+ sn +" plugin",e);
}

// recalculate dependencies of plugins optionally depending the newly deployed one.
for (PluginWrapper depender: plugins) {
if (depender.equals(p)) {
// skip itself.
continue;
// run initializers in the added plugin
Reactor r = new Reactor(InitMilestone.ordering());
final ClassLoader loader = p.classLoader;
r.addAll(new InitializerFinder(loader) {
@Override
protected boolean filter(Method e) {
return e.getDeclaringClass().getClassLoader() != loader || super.filter(e);
}
}.discoverTasks(r));
try {
new InitReactorRunner().run(r);
} catch (ReactorException e) {
throw new IOException("Failed to initialize "+ sn +" plugin",e);
}
for (Dependency d: depender.getOptionalDependencies()) {
if (d.shortName.equals(p.getShortName())) {
// this plugin depends on the newly loaded one!
// recalculate dependencies!
try {
getPluginStrategy().updateDependency(depender, p);
} catch (AbstractMethodError x) {
LOGGER.log(WARNING, "{0} does not yet implement updateDependency", getPluginStrategy().getClass());

// recalculate dependencies of plugins optionally depending the newly deployed one.
for (PluginWrapper depender: plugins) {
if (depender.equals(p)) {
// skip itself.
continue;
}
for (Dependency d: depender.getOptionalDependencies()) {
if (d.shortName.equals(p.getShortName())) {
// this plugin depends on the newly loaded one!
// recalculate dependencies!
try {
getPluginStrategy().updateDependency(depender, p);
} catch (AbstractMethodError x) {
LOGGER.log(WARNING, "{0} does not yet implement updateDependency", getPluginStrategy().getClass());
}
break;
}
break;
}
}
}

// Redo who depends on who.
resolveDependantPlugins();
// Redo who depends on who.
resolveDependantPlugins();

LOGGER.info("Plugin " + p.getShortName()+":"+p.getVersion() + " dynamically installed");
LOGGER.info("Plugin " + p.getShortName()+":"+p.getVersion() + " dynamically installed");
}
}

@Restricted(NoExternalUse.class)
Expand Down
14 changes: 14 additions & 0 deletions test/src/test/java/hudson/PluginManagerTest.java
View file
Expand Up @@ -30,7 +30,10 @@
import hudson.model.UpdateCenter;
import hudson.model.UpdateCenter.UpdateCenterJob;
import hudson.model.UpdateSite;
import hudson.model.User;
import hudson.scm.SubversionSCM;
import hudson.security.ACL;
import hudson.security.ACLContext;
import hudson.util.FormValidation;
import hudson.util.PersistedList;
import java.io.File;
Expand All @@ -53,6 +56,7 @@
import org.junit.rules.TemporaryFolder;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.MockAuthorizationStrategy;
import org.jvnet.hudson.test.Url;
import org.jvnet.hudson.test.recipes.WithPlugin;
import org.jvnet.hudson.test.recipes.WithPluginManager;
Expand Down Expand Up @@ -444,6 +448,16 @@ private String callDependerValue() throws Exception {
assertTrue(pluginInfo.getString("dependencies") != null);
}

@Issue("JENKINS-41684")
@Test
public void requireSystemDuringLoad() throws Exception {
r.jenkins.setSecurityRealm(r.createDummySecurityRealm());
r.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy());
try (ACLContext context = ACL.as(User.get("underprivileged").impersonate())) {
dynamicLoad("require-system-during-load.hpi");
}
}

private void dynamicLoad(String plugin) throws IOException, InterruptedException, RestartRequiredException {
PluginManagerUtil.dynamicLoad(plugin, r.jenkins);
}
Expand Down
Binary file added test/src/test/resources/plugins/require-system-during-load.hpi
View file
Binary file not shown.

0 comments on commit 6fb9e91

Please sign in to comment.