Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
JENKINS-49952 Groovy post-build script in matrix job throws exception
  • Loading branch information
Daniel Heid committed Apr 5, 2018
1 parent eb197bf commit 2048ada
Show file tree
Hide file tree
Showing 16 changed files with 59 additions and 61 deletions.
8 changes: 8 additions & 0 deletions README.md
Expand Up @@ -88,6 +88,14 @@ changes without needing to run to `package` phase.

## Release Notes

### Version 2.6.0

Removed access to workspace on master for Groovy script execution, because secure groovy scripts cannot be configured
on slaves when using a master-to-slave callable. Groovy scripts may not access files of the master's workspace as a
result. However Groovy scripts can be run on slaves again. Thanks to John David for reporting this issue.

* JENKINS-49952 - Groovy post-build script in matrix job throws exception

### Version 2.5.1

If the shebang contains a space in front of the interpreter, it will be stripped out.
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
Expand Up @@ -4,7 +4,7 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>3.5</version>
<version>3.7</version>
</parent>

<artifactId>postbuildscript</artifactId>
Expand Down
Expand Up @@ -5,7 +5,6 @@
import hudson.FilePath;
import hudson.Util;
import hudson.model.AbstractBuild;
import jenkins.security.MasterToSlaveCallable;
import org.jenkinsci.plugins.postbuildscript.Logger;
import org.jenkinsci.plugins.postbuildscript.model.Script;
import org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SecureGroovyScript;
Expand All @@ -14,25 +13,24 @@
import java.util.ArrayList;
import java.util.List;

public class GroovyScriptExecutor extends MasterToSlaveCallable<Boolean, Exception> {
public class GroovyScriptExecutor {

private static final long serialVersionUID = 3874477459736242748L;
private final String script;
private final boolean sandbox;
private final List<String> arguments;
private final transient AbstractBuild<?, ?> build;
private final Logger log;
private final SecureGroovyScript secureGroovyScript;

public GroovyScriptExecutor(Script script, List<String> arguments, AbstractBuild<?, ?> build, Logger log) {
this.script = script.getContent();
this.sandbox = script.isSandboxed();
this.arguments = new ArrayList<>(arguments);
this.build = build;
this.log = log;
String enrichedScript = Util.replaceMacro(script.getContent(), EnvVars.masterEnvVars);
secureGroovyScript = new SecureGroovyScript(enrichedScript, script.isSandboxed(), null);
secureGroovyScript.configuringWithNonKeyItem();
}

@Override
public Boolean call() throws Exception {
public void execute() throws Exception {

Binding binding = new Binding();
if (build != null) {
Expand All @@ -48,13 +46,6 @@ public Boolean call() throws Exception {
binding.setVariable("args", arguments); //NON-NLS

ClassLoader classLoader = getClass().getClassLoader();

String enrichedScript = Util.replaceMacro(script, EnvVars.masterEnvVars);
SecureGroovyScript groovyScript = new SecureGroovyScript(enrichedScript, sandbox, null);
groovyScript.configuringWithNonKeyItem();

groovyScript.evaluate(classLoader, binding);

return true;
secureGroovyScript.evaluate(classLoader, binding);
}
}
Expand Up @@ -46,12 +46,13 @@ public boolean evaluateScript(Script script, List<String> arguments) {
throw new IllegalArgumentException("The script content object must be set.");
}

if (ensureWorkspaceNotNull(workspace)) {
if (workspaceIsNull()) {
return false;
}

try {
return workspace.act(groovyScriptExecutorFactory.create(script, arguments));
GroovyScriptExecutor groovyScriptExecutor = groovyScriptExecutorFactory.create(script, arguments);
groovyScriptExecutor.execute();
} catch (Exception exception) {
StringWriter stringWriter = new StringWriter();
PrintWriter printWriter = new PrintWriter(stringWriter);
Expand All @@ -60,9 +61,10 @@ public boolean evaluateScript(Script script, List<String> arguments) {
return false;
}

return true;
}

private boolean ensureWorkspaceNotNull(FilePath workspace) {
private boolean workspaceIsNull() {
if (workspace == null) {
logger.info(Messages.PostBuildScript_WorkspaceEmpty());
return true;
Expand All @@ -72,7 +74,7 @@ private boolean ensureWorkspaceNotNull(FilePath workspace) {

public boolean evaluateCommand(ScriptFile scriptFile, Command command) throws PostBuildScriptException {

if (ensureWorkspaceNotNull(workspace)) {
if (workspaceIsNull()) {
return false;
}

Expand Down
Expand Up @@ -11,7 +11,7 @@ public class LoadScriptContentCallable extends MasterToSlaveFileCallable<String>
private static final long serialVersionUID = -8106062333861360484L;

@Override
public String invoke(File f, VirtualChannel channel) throws IOException, InterruptedException {
public String invoke(File f, VirtualChannel channel) throws IOException {
return Util.loadFile(f);
}
}
@@ -1,8 +1,5 @@
package org.jenkinsci.plugins.postbuildscript;

import java.io.IOException;
import java.io.PrintStream;

import hudson.Launcher;
import hudson.matrix.MatrixBuild;
import hudson.matrix.MatrixRun;
Expand All @@ -14,6 +11,8 @@
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;

import java.io.PrintStream;

import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.assertThat;
import static org.mockito.BDDMockito.given;
Expand Down Expand Up @@ -62,7 +61,7 @@ public void runsProcessorWithEndOfMatrixBuildEnabled() {
}

@Test
public void addsNewLineToLoggerAfterRun() throws IOException, InterruptedException {
public void addsNewLineToLoggerAfterRun() throws Exception {

given(listener.getLogger()).willReturn(logger);

Expand Down
Expand Up @@ -33,7 +33,7 @@ public void letTaskListenerReturnStream() {
}

@Test
public void prefixesInfoMessages() throws Exception {
public void prefixesInfoMessages() {

logger.info(MESSAGE);

Expand All @@ -42,7 +42,7 @@ public void prefixesInfoMessages() throws Exception {
}

@Test
public void prefixesErrorMessages() throws Exception {
public void prefixesErrorMessages() {

logger.error(MESSAGE);

Expand Down
Expand Up @@ -27,7 +27,7 @@ public class ConfigurationTest {
private final Configuration configuration = new Configuration();

@Test
public void addsBuildStep() throws Exception {
public void addsBuildStep() {

configuration.addBuildStep(postBuildStep);

Expand All @@ -38,7 +38,7 @@ public void addsBuildStep() throws Exception {


@Test
public void addsBuildSteps() throws Exception {
public void addsBuildSteps() {

configuration.addBuildSteps(Collections.singleton(postBuildStep));

Expand All @@ -48,7 +48,7 @@ public void addsBuildSteps() throws Exception {
}

@Test
public void storesMarkBuildUnstable() throws Exception {
public void storesMarkBuildUnstable() {

configuration.setMarkBuildUnstable(true);

Expand All @@ -57,7 +57,7 @@ public void storesMarkBuildUnstable() throws Exception {
}

@Test
public void addsGenericScriptFiles() throws Exception {
public void addsGenericScriptFiles() {

given(scriptFile.getScriptType()).willReturn(ScriptType.GENERIC);

Expand All @@ -70,7 +70,7 @@ public void addsGenericScriptFiles() throws Exception {


@Test
public void addsGroovyScripts() throws Exception {
public void addsGroovyScripts() {

configuration.addGroovyScripts(Collections.singleton(script));

Expand All @@ -80,7 +80,7 @@ public void addsGroovyScripts() throws Exception {
}

@Test
public void addsGroovyScriptFiles() throws Exception {
public void addsGroovyScriptFiles() {

given(scriptFile.getScriptType()).willReturn(ScriptType.GROOVY);

Expand Down
Expand Up @@ -17,20 +17,20 @@ public class PostBuildItemTest {
private PostBuildItem postBuildItem;

@Before
public void setUp() throws Exception {
public void setUp() {
postBuildItem = new PostBuildItem(Collections.singleton(SUCCESS));
}

@Test
public void doesNotHaveResultOnNullResult() throws Exception {
public void doesNotHaveResultOnNullResult() {

PostBuildItem postBuildItem = new PostBuildItem(null);
assertThat(postBuildItem.hasResult(), is(false));

}

@Test
public void doesNotHaveResultOnEmptyResults() throws Exception {
public void doesNotHaveResultOnEmptyResults() {

PostBuildItem postBuildItem = new PostBuildItem(Collections.emptySet());
assertThat(postBuildItem.hasResult(), is(false));
Expand All @@ -39,28 +39,28 @@ public void doesNotHaveResultOnEmptyResults() throws Exception {


@Test
public void allowsExecutionWhenContainsResults() throws Exception {
public void allowsExecutionWhenContainsResults() {

assertThat(postBuildItem.shouldBeExecuted(SUCCESS), is(true));

}

@Test
public void deniesExecutionWhenDoesNotContainResults() throws Exception {
public void deniesExecutionWhenDoesNotContainResults() {

assertThat(postBuildItem.shouldBeExecuted(FAILURE), is(false));

}

@Test
public void deniesExecutionOnNull() throws Exception {
public void deniesExecutionOnNull() {

assertThat(postBuildItem.shouldBeExecuted(null), is(false));

}

@Test
public void addsResultsWhenInitialized() throws Exception {
public void addsResultsWhenInitialized() {

postBuildItem.addResults(Collections.singleton(FAILURE));

Expand Down
Expand Up @@ -20,7 +20,7 @@ public class PostBuildStepTest {
private BuildStep buildStep;

@Test
public void containsBuildSteps() throws Exception {
public void containsBuildSteps() {

PostBuildStep postBuildStep = new PostBuildStep(
Collections.singleton(RESULT), Collections.singleton(buildStep));
Expand Down
Expand Up @@ -13,7 +13,7 @@ public class ScriptFileTest {
private static final String FILE_PATH = "println 'Hello World'";

@Test
public void containsFilePath() throws Exception {
public void containsFilePath() {

ScriptFile scriptFile = new ScriptFile(
Collections.singleton(RESULT), FILE_PATH);
Expand Down
Expand Up @@ -14,7 +14,7 @@ public class ScriptTest {
private static final String CONTENT = "println 'Hello World'";

@Test
public void containsScript() throws Exception {
public void containsScript() {

Script script = new Script(
Collections.singleton(RESULT), CONTENT);
Expand All @@ -24,7 +24,7 @@ public void containsScript() throws Exception {
}

@Test
public void containsNullOnEmptyScript() throws Exception {
public void containsNullOnEmptyScript() {

Script script = new Script(
Collections.singleton(RESULT), "");
Expand Down
Expand Up @@ -12,7 +12,6 @@
import org.mockito.junit.MockitoJUnitRunner;

import java.io.File;
import java.io.IOException;

import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.assertThat;
Expand Down Expand Up @@ -52,7 +51,7 @@ public void supportsShebangWithSpacesInFrontOfInterpreter() throws Exception {

}

private void givenExecutor() throws IOException {
private void givenExecutor() {
LocalLauncher launcher = jenkinsRule.createLocalLauncher();
FilePath workspace = new FilePath(scriptFile.getParentFile());
TaskListener listener = jenkinsRule.createTaskListener();
Expand Down
Expand Up @@ -14,6 +14,7 @@
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.notNullValue;
import static org.junit.Assert.assertThat;
import static org.mockito.BDDMockito.given;

@RunWith(MockitoJUnitRunner.class)
public class GroovyScriptExecutorFactoryTest {
Expand All @@ -31,7 +32,10 @@ public class GroovyScriptExecutorFactoryTest {
private GroovyScriptExecutorFactory executorFactory;

@Test
public void createsExecutor() throws Exception {
public void createsExecutor() {

given(script.getContent()).willReturn("scriptContent");
given(script.isSandboxed()).willReturn(true);

GroovyScriptExecutor executor = executorFactory.create(script, Collections.emptyList());

Expand Down
Expand Up @@ -16,8 +16,6 @@
import java.io.PrintStream;
import java.util.Collections;

import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.assertThat;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.verify;

Expand Down Expand Up @@ -52,9 +50,8 @@ public void runsGroovyScriptWithVariablesAndBindings() throws Exception {

GroovyScriptExecutor callable = new GroovyScriptExecutor(
script, Collections.emptyList(), executable, log);
Boolean result = callable.call();
callable.execute();

assertThat(result, is(true));
verify(log).info("hello world");
verify(printStream).println(executable.getId());

Expand Down

0 comments on commit 2048ada

Please sign in to comment.