[JENKINS-63668] fix: auto re-request approval once dismissed

src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java

@@ -141,7 +141,8 @@ public boolean isScriptAutoApprovalEnabled() {
     public SecureGroovyScript configuring(ApprovalContext context) {
         calledConfiguring = true;
         if (!sandbox) {
-            ScriptApproval.get().configuring(script, GroovyLanguage.get(), context, !StringUtils.equals(this.oldScript, this.script));
+            ScriptApproval.get().configuring(script, GroovyLanguage.get(), context,
+                    !StringUtils.equals(this.oldScript, this.script), false);
         }
         for (ClasspathEntry entry : getClasspath()) {
             ScriptApproval.get().configuring(entry, context);

src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java

@@ -604,6 +604,7 @@
                pendingClasspathEntries.isEmpty();
     }
 
+
     /**
      * Used when someone is configuring a script.
      * Typically you would call this from a {@link DataBoundConstructor}.
@@ -617,36 +618,49 @@
      * @param language the language in which it is written
      * @param context any additional information about how where or by whom this is being configured
      * @param approveIfAdmin indicates whether script should be approved if current user has admin permissions
+     * @param ignoreAdmin indicates whether an admin's ability to approve a script without visiting the script approval site should be ignored, regardless of any configurations.
      * @return {@code script}, for convenience
      */
-    public synchronized String configuring(@NonNull String script, @NonNull Language language, @NonNull ApprovalContext context, boolean approveIfAdmin) {
+    public synchronized String configuring(@NonNull String script, @NonNull Language language, @NonNull ApprovalContext context, boolean approveIfAdmin, boolean ignoreAdmin) {
         final ConversionCheckResult result = checkAndConvertApprovedScript(script, language);
-        if (!result.approved) {
-            if (!Jenkins.get().isUseSecurity() || 
-                    (ALLOW_ADMIN_APPROVAL_ENABLED &&
-                    ((Jenkins.getAuthentication2() != ACL.SYSTEM2 && Jenkins.get().hasPermission(Jenkins.ADMINISTER))
-                            && (ADMIN_AUTO_APPROVAL_ENABLED || approveIfAdmin)))) {
-                approvedScriptHashes.add(result.newHash);
-                //Pending scripts are not stored with a precalculated hash, so no need to remove any old hashes
-                removePendingScript(result.newHash);
-            } else {
-                String key = context.getKey();
-                if (key != null) {
-                    pendingScripts.removeIf(pendingScript -> key.equals(pendingScript.getContext().getKey()));
-                }
-                pendingScripts.add(new PendingScript(script, language, context));
+        if (result.approved) {
+            return script;
+        }
+        // Security is disabled globally.
+        boolean securityIsDisabled = !Jenkins.get().isUseSecurity();
+        // Has to be an actual user and the user must be admin. System-triggered jobs should not auto-approve. (I guess that is the reasonf or this boolean?)
+        boolean isAdminUser = Jenkins.getAuthentication2() != ACL.SYSTEM2 && Jenkins.get().hasPermission(Jenkins.ADMINISTER);
+        boolean implicitAdminApproval = ADMIN_AUTO_APPROVAL_ENABLED || approveIfAdmin;
+        if (securityIsDisabled ||
+                (ALLOW_ADMIN_APPROVAL_ENABLED && isAdminUser && implicitAdminApproval && !ignoreAdmin)) {
+            approvedScriptHashes.add(result.newHash);
+            // Pending scripts are not stored with a precalculated hash, so no need to remove any old hashes
+            removePendingScript(result.newHash);
+        } else {
+            String key = context.getKey();
+            if (key != null) {
+                pendingScripts.removeIf(pendingScript -> key.equals(pendingScript.getContext().getKey()));
             }
-            save();
+            pendingScripts.add(new PendingScript(script, language, context));
         }
+        save();
         return script;
     }
 
     /**
-     * @deprecated Use {@link #configuring(String, Language, ApprovalContext, boolean)} instead
+     * @deprecated Use {@link #configuring(String, Language, ApprovalContext, boolean, boolean)} instead
+     */
+    @Deprecated
+    public synchronized String configuring(@NonNull String script, @NonNull Language language, @NonNull ApprovalContext context, boolean approveIfAdmin) {
+        return configuring(script, language, context, approveIfAdmin, false);
+    }
+
+    /**
+     * @deprecated Use {@link #configuring(String, Language, ApprovalContext, boolean, boolean)} instead
      */
     @Deprecated
     public String configuring(@NonNull String script, @NonNull Language language, @NonNull ApprovalContext context) {
-        return this.configuring(script, language, context, false);
+        return this.configuring(script, language, context, false, false);
     }
 
     /**
@@ -664,7 +678,12 @@
         }
         ConversionCheckResult result = checkAndConvertApprovedScript(script, language);
         if (!result.approved) {
-            // Probably need not add to pendingScripts, since generally that would have happened already in configuring.
+            // Usually. this method is called once the job configuration with the script is saved.
+            // If a script was previously pending and is now deleted, however, it would require to re-configure the job.
+            // That's why we call it again if it is unapproved in a running job.
+            // 'ignoreAdmin' is set to true, so that administrators
+            // do not accidentally approve scripts when running a job.
+            this.configuring(script, language, ApprovalContext.create(), false, true);
             throw new UnapprovedUsageException(result.newHash);
         }
         return script;

src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java

@@ -53,6 +53,7 @@
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.nullValue;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.fail;
 
@@ -67,6 +68,41 @@ public class ScriptApprovalTest extends AbstractApprovalTest<ScriptApprovalTest.
     private static final String WHITELISTED_SIGNATURE = "method java.lang.String trim";
     private static final String DANGEROUS_SIGNATURE = "staticMethod hudson.model.User current";
 
+    private String approveIfExists(String script, ScriptApproval sa) throws IOException {
+        for (ScriptApproval.PendingScript pending : sa.getPendingScripts()) {
+            if(pending.script.equals(script)) {
+                String hash = pending.getHash();
+                sa.approveScript(hash);
+                return hash;
+            }
+        }
+        return null;
+    }
+
+    @Test
+    public void afterDenyScriptShouldBePending() throws Exception {
+        configureSecurity();
+        final ScriptApproval sa = ScriptApproval.get();
+        FreeStyleProject p = r.createFreeStyleProject();
+        String testScript = "jenkins.model.Jenkins.instance";
+        p.getPublishersList().add(new TestGroovyRecorder(
+                new SecureGroovyScript(testScript, false, null)));
+
+        String approvedHash = approveIfExists(testScript, sa);
+        assertNotNull(approvedHash);
+        r.assertBuildStatus(Result.SUCCESS, p.scheduleBuild2(0).get());
+
+        sa.denyScript(approvedHash);
+
+        /*
+            * The script is not approved but should be pending.
+         */
+        r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0).get());
+        approvedHash = approveIfExists(testScript, sa);
+        assertNotNull(approvedHash);
+        r.assertBuildStatus(Result.SUCCESS, p.scheduleBuild2(0).get());
+    }
+
     @Test public void emptyScript() throws Exception {
         configureSecurity();
         script("").use();
@@ -220,7 +256,8 @@ static final class Script extends Approvable<Script> {
 
         Script(String groovy) {
             final ApprovalContext ac = ApprovalContext.create();
-            this.groovy = ScriptApproval.get().configuring(groovy, GroovyLanguage.get(), ac, true);
+            this.groovy = ScriptApproval.get().configuring(groovy, GroovyLanguage.get(), ac, true,
+                    false);
             this.hash = new ScriptApproval.PendingScript(groovy, GroovyLanguage.get(), ac).getHash();
         }