diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java index b3d6b4a92..8e70701ea 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java +++ b/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); diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java index ff5e2f453..0a9308250 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -604,6 +604,7 @@ synchronized boolean isEmpty() { pendingClasspathEntries.isEmpty(); } + /** * Used when someone is configuring a script. * Typically you would call this from a {@link DataBoundConstructor}. @@ -617,36 +618,49 @@ synchronized boolean isEmpty() { * @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 @@ public synchronized String using(@NonNull String script, @NonNull Language langu } 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; diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java index 1405c0141..2dec4be42 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java +++ b/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 { 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(); }