Merge pull request #43 from jenkinsci/RejectedAccessException-paralle…

…l-JENKINS-26541 [JENKINS-26541] Handle wrapped RejectedAccessException
jenkinsci · Jan 23, 2015 · b83b89d · b83b89d
2 parents 8f9dc9a + d31a8d7
commit b83b89d
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 7 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -10,6 +10,7 @@ Only noting significant user-visible or major API changes, not internal code cle
 * JENKINS-25924: added `error` step.
 * JENKINS-26030: file locks could prevent build deletion.
 * JENKINS-26074: completed parallel branches become invisible until the whole parallel step is done
+* JENKINS-26541: rejected sandbox methods were not offered for approval when inside `parallel`.
 * Snippet generator incorrectly suggested `pwd` when Groovy requires `pwd()`.
 
 ## 1.1 (Dec 09 2014)

diff --git a/aggregator/src/test/java/org/jenkinsci/plugins/workflow/WorkflowJobNonRestartingTest.java b/aggregator/src/test/java/org/jenkinsci/plugins/workflow/WorkflowJobNonRestartingTest.java
@@ -27,7 +27,10 @@
 import hudson.model.Result;
 import hudson.model.Slave;
 import hudson.model.queue.QueueTaskFuture;
+import java.util.Collections;
+import java.util.Set;
 import org.apache.commons.io.FileUtils;
+import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval;
 import org.jenkinsci.plugins.workflow.actions.LogAction;
 import org.jenkinsci.plugins.workflow.cps.AbstractCpsFlowTest;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
@@ -40,6 +43,7 @@
 import static org.junit.Assert.*;
 import org.junit.Before;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 
 /**
@@ -182,14 +186,27 @@ public void sandbox() throws Exception {
     /**
      * If a prohibited method is called, execution should fail.
      */
+    @Issue("JENKINS-26541")
     @Test
     public void sandboxRejection() throws Exception {
-        p.setDefinition(new CpsFlowDefinition("Jenkins.getInstance();", true));
-
+        assertRejected("Jenkins.getInstance()");
+        assertRejected("parallel(main: {Jenkins.getInstance()})");
+        assertRejected("parallel(main: {parallel(main2: {Jenkins.getInstance()})})");
+        assertRejected("node {parallel(main: {ws {parallel(main2: {ws {Jenkins.getInstance()}})}})}");
+    }
+    private void assertRejected(String script) throws Exception {
+        String signature = "staticMethod jenkins.model.Jenkins getInstance";
+        ScriptApproval scriptApproval = ScriptApproval.get();
+        scriptApproval.denySignature(signature);
+        assertEquals(Collections.emptySet(), scriptApproval.getPendingSignatures());
+        p.setDefinition(new CpsFlowDefinition(script, true));
         WorkflowRun b = p.scheduleBuild2(0).get();
-
-        jenkins.assertLogContains("org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException: Scripts not permitted to use staticMethod jenkins.model.Jenkins getInstance", b);
+        jenkins.assertLogContains("org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException: Scripts not permitted to use " + signature, b);
         jenkins.assertBuildStatus(Result.FAILURE, b);
+        Set<ScriptApproval.PendingSignature> pendingSignatures = scriptApproval.getPendingSignatures();
+        assertEquals(script, 1, pendingSignatures.size());
+        assertEquals(signature, pendingSignatures.iterator().next().signature);
+
     }
 
     /**

diff --git a/cps/src/main/java/org/jenkinsci/plugins/workflow/cps/SandboxContinuable.java b/cps/src/main/java/org/jenkinsci/plugins/workflow/cps/SandboxContinuable.java
@@ -9,6 +9,7 @@
 import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval;
 
 import java.util.concurrent.Callable;
+import javax.annotation.CheckForNull;
 
 /**
  * {@link Continuable} that executes code inside sandbox execution.
@@ -30,9 +31,9 @@ public Outcome run0(final Outcome cn) {
                 @Override
                 public Outcome call() {
                     Outcome outcome = SandboxContinuable.super.run0(cn);
-                    Throwable t = outcome.getAbnormal();
-                    if (t instanceof RejectedAccessException) {
-                        ScriptApproval.get().accessRejected((RejectedAccessException) t, ApprovalContext.create());
+                    RejectedAccessException x = findRejectedAccessException(outcome.getAbnormal());
+                    if (x != null) {
+                        ScriptApproval.get().accessRejected(x, ApprovalContext.create());
                     }
                     return outcome;
                 }
@@ -43,4 +44,15 @@ public Outcome call() {
             throw new AssertionError(e);    // Callable doesn't throw anything
         }
     }
+
+    private static @CheckForNull RejectedAccessException findRejectedAccessException(@CheckForNull Throwable t) {
+        if (t == null) {
+            return null;
+        } else if (t instanceof RejectedAccessException) {
+            return (RejectedAccessException) t;
+        } else {
+            return findRejectedAccessException(t.getCause());
+        }
+    }
+
 }