Full test and fix for lazy-load issues with ReplayAction [JENKINS-50784]

jenkinsci · Apr 19, 2018 · b295001 · b295001
1 parent 86c0232
commit b295001
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 10 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/replay/ReplayAction.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/replay/ReplayAction.java
@@ -152,10 +152,26 @@ private ReplayAction(Run run) {
         }
 
         CpsFlowExecution exec = getExecutionLazy();
-        if (exec != null && exec.isSandbox() && !(Jenkins.getActiveInstance().hasPermission(Jenkins.ADMINISTER))) {
-            return false; // We have to check for ADMIN because un-sandboxed code can execute arbitrary on-master code
+        if (exec != null) {
+            return exec.isSandbox() || Jenkins.getActiveInstance().hasPermission(Jenkins.ADMINISTER); // We have to check for ADMIN because un-sandboxed code can execute arbitrary on-master code
+        } else {
+            // If the execution hasn't been lazy-loaded then we will wait to do deeper checks until someone tries to lazy load
+            // OR until isReplayableSandboxTest is invoked b/c they actually try to replay the build
+            return true;
+        }
+    }
+
+    /** Runs the extra tests for replayability beyond {@link #isEnabled()} that require a blocking load of the execution. */
+    /* accessible to Jelly */ public boolean isReplayableSandboxTest() {
+        CpsFlowExecution exec = getExecutionBlocking();
+        if (exec != null) {
+            if (!exec.isSandbox()) {
+                // We have to check for ADMIN because un-sandboxed code can execute arbitrary on-master code
+                return Jenkins.getActiveInstance().hasPermission(Jenkins.ADMINISTER);
+            }
+            return true;
         }
-        return true;  // If the execution hasn't been lazy-loaded then we will wait to do deeper checks until someone tries to lazy load.
+        return false;
     }
 
     /** @see CpsFlowExecution#getScript */
@@ -184,7 +200,7 @@ private ReplayAction(Run run) {
     @Restricted(DoNotUse.class)
     @RequirePOST
     public void doRun(StaplerRequest req, StaplerResponse rsp) throws ServletException, IOException {
-        if (!isEnabled()) {
+        if (!isEnabled() || !(isReplayableSandboxTest())) {
             throw new AccessDeniedException("not allowed to replay"); // AccessDeniedException2 requires us to look up the specific Permission
         }
         JSONObject form = req.getSubmittedForm();

diff --git a/src/main/resources/org/jenkinsci/plugins/workflow/cps/replay/ReplayAction/index.jelly b/src/main/resources/org/jenkinsci/plugins/workflow/cps/replay/ReplayAction/index.jelly
@@ -10,7 +10,7 @@
                     <j:out value="${%blurb}"/>
                 </p>
                 <j:choose>
-                    <j:when test="${it.enabled}">
+                    <j:when test="${it.enabled and it.replayableSandboxTest}">
                         <f:form action="run" method="POST" name="config">
                             <f:entry field="mainScript" title="Main Script">
                                 <wfe:workflow-editor script="${it.originalScript}" checkUrl="${rootURL}/${it.owner.url}${it.urlName}/checkScript" checkDependsOn=""/>

diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/replay/ReplayActionTest.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/replay/ReplayActionTest.java
@@ -53,15 +53,13 @@
 import org.hamcrest.Matchers;
 import static org.hamcrest.Matchers.*;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
-import org.jenkinsci.plugins.workflow.flow.FlowExecution;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
 import org.jenkinsci.plugins.workflow.job.WorkflowRun;
 import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep;
 import static org.junit.Assert.*;
 
 import org.junit.Assert;
 import org.junit.ClassRule;
-import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runners.model.Statement;
@@ -140,27 +138,41 @@ public class ReplayActionTest {
     @Test public void lazyLoadExecutionStillReplayable() throws Exception {
         story.then( r-> {
             WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
+            WorkflowJob p2 = r.jenkins.createProject(WorkflowJob.class, "p2");
             p.setDefinition(new CpsFlowDefinition("echo 'I did a thing'", false));
+            p2.setDefinition(new CpsFlowDefinition("echo 'I did a thing'", true));
             // Start off with a simple run of the first script.
             r.buildAndAssertSuccess(p);
+            r.buildAndAssertSuccess(p2);
 
             r.jenkins.setSecurityRealm(r.createDummySecurityRealm());
             GlobalMatrixAuthorizationStrategy gmas = new GlobalMatrixAuthorizationStrategy();
             gmas.add(Jenkins.ADMINISTER, "admin");
+            gmas.add(ReplayAction.REPLAY, "normal");
+            r.jenkins.setAuthorizationStrategy(gmas);
         });
         story.then( r-> {
             WorkflowJob job = r.jenkins.getItemByFullName("p", WorkflowJob.class);
+            WorkflowJob job2 = r.jenkins.getItemByFullName("p2", WorkflowJob.class);
             WorkflowRun run = job.getLastBuild();
+            WorkflowRun run2 = job2.getLastBuild();
+
             JenkinsRule.WebClient wc = r.createWebClient();
             Assert.assertNull(run.asFlowExecutionOwner().getOrNull());
             Assert.assertTrue(canReplay(run, "admin"));
+            Assert.assertTrue(canReplay(run, "normal"));
             Assert.assertTrue(canRebuild(run, "admin"));
             Assert.assertNull(run.asFlowExecutionOwner().getOrNull());
 
-            FlowExecution exec = run.getExecution();
+            // After lazy-load we can do deeper checks easily, and the deep test triggers a full load of the execution
+            Assert.assertTrue(canReplayDeepTest(run, "admin"));
+            Assert.assertTrue(canReplayDeepTest(run2, "normal"));
+
             Assert.assertNotNull(run.asFlowExecutionOwner().getOrNull());
-            canReplay(run, "admin");
-            canRebuild(run, "admin");
+            Assert.assertTrue(canReplay(run, "admin"));
+            Assert.assertFalse(canReplay(run, "normal")); // Now we know to check if the user can run outside sandbox, and they can't
+            Assert.assertTrue(canReplay(run2, "normal")); // We can still run stuff inside sandbox
+            Assert.assertTrue(canRebuild(run, "admin"));
         });
     }
 
@@ -227,6 +239,15 @@ private static boolean canReplay(WorkflowRun b, String user) {
         });
     }
 
+    private static boolean canReplayDeepTest(WorkflowRun b, String user) {
+        final ReplayAction a = b.getAction(ReplayAction.class);
+        return ACL.impersonate(User.get(user).impersonate(), new NotReallyRoleSensitiveCallable<Boolean,RuntimeException>() {
+            @Override public Boolean call() throws RuntimeException {
+                return a.isReplayableSandboxTest();
+            }
+        });
+    }
+
     private static boolean canRebuild(WorkflowRun b, String user) {
         final ReplayAction a = b.getAction(ReplayAction.class);
         return ACL.impersonate(User.get(user).impersonate(), new NotReallyRoleSensitiveCallable<Boolean,RuntimeException>() {