From a02bf249a2326812542aebc3696ceac943d91f16 Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Thu, 31 Oct 2024 09:33:11 +0100 Subject: [PATCH 1/8] JENKINS-73941 - HideSandbox - Unify all the logic in Script-Security plugin --- .../sandbox/groovy/SecureGroovyScript.java | 10 +++++----- .../scriptsecurity/scripts/ScriptApproval.java | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 5 deletions(-) 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 073d14e4..f5ac162d 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 @@ -28,6 +28,8 @@ import groovy.lang.Binding; import groovy.lang.GroovyClassLoader; import groovy.lang.GroovyShell; +import groovy.lang.Script; + import hudson.Extension; import hudson.PluginManager; import hudson.model.AbstractDescribableImpl; @@ -96,9 +98,8 @@ public final class SecureGroovyScript extends AbstractDescribableImpl classpath) throws Descriptor.FormException { - if (!sandbox && ScriptApproval.get().isForceSandboxForCurrentUser()) { - throw new Descriptor.FormException(Messages.ScriptApproval_SandboxCantBeDisabled(), "sandbox"); - } + ScriptApproval.validateSandbox(sandbox); + this.script = script; this.sandbox = sandbox; this.classpath = classpath; @@ -473,8 +474,7 @@ public FormValidation doCheckScript(@QueryParameter String value, @QueryParamete public boolean shouldHideSandbox(@CheckForNull SecureGroovyScript instance) { // sandbox checkbox is shown to admins even if the global configuration says otherwise // it's also shown when sandbox == false, so regular users can enable it - return ScriptApproval.get().isForceSandboxForCurrentUser() - && (instance == null || instance.sandbox); + return ScriptApproval.shouldHideSandbox(instance,SecureGroovyScript::isSandbox); } } 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 e8ae4940..8cd9ee97 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -26,11 +26,14 @@ import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import hudson.model.BallColor; +import hudson.model.Descriptor; import hudson.model.PageDecorator; import hudson.security.ACLContext; import jenkins.model.GlobalConfiguration; import jenkins.model.GlobalConfigurationCategory; import jenkins.util.SystemProperties; + +import gnu.crypto.hash.IMessageDigest; import net.sf.json.JSONArray; import net.sf.json.JSONObject; import org.apache.commons.lang.StringUtils; @@ -68,6 +71,7 @@ import java.util.Stack; import java.util.TreeSet; import java.util.function.Consumer; +import java.util.function.Predicate; import java.util.logging.Level; import java.util.logging.Logger; import java.util.regex.Pattern; @@ -1335,4 +1339,15 @@ public synchronized JSON clearApprovedClasspathEntries() throws IOException { @Restricted(NoExternalUse.class) @Extension public static class FormValidationPageDecorator extends PageDecorator {} + + public static boolean shouldHideSandbox(@CheckForNull T instance, Predicate isSandbox){ + return get().isForceSandboxForCurrentUser() + && (instance == null || isSandbox.test(instance)); + } + + public static void validateSandbox(boolean sandbox) throws Descriptor.FormException{ + if (!sandbox && get().isForceSandboxForCurrentUser()) { + throw new Descriptor.FormException(Messages.ScriptApproval_SandboxCantBeDisabled(), "sandbox"); + } + } } From bfb485327ae439d613c8cedc3622eae674878bf8 Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Thu, 31 Oct 2024 10:01:00 +0100 Subject: [PATCH 2/8] JENKINS-73941 - HideSandbox - Unify all the logic in Script-Security plugin - Tests --- .../scripts/ScriptApprovalTest.java | 117 ++++++++++++++---- 1 file changed, 96 insertions(+), 21 deletions(-) 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 f83b59e2..e25bd110 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java @@ -26,6 +26,8 @@ import org.htmlunit.html.HtmlPage; import org.htmlunit.html.HtmlTextArea; + +import hudson.model.Descriptor; import hudson.model.FreeStyleBuild; import hudson.model.FreeStyleProject; import hudson.model.Item; @@ -209,23 +211,7 @@ public void reload() throws Exception { @Test public void forceSandboxTests() throws Exception { - r.jenkins.setSecurityRealm(r.createDummySecurityRealm()); - - ScriptApproval.get().setForceSandbox(true); - - MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy(); - mockStrategy.grant(Jenkins.READ).everywhere().to("devel"); - for (Permission p : Item.PERMISSIONS.getPermissions()) { - mockStrategy.grant(p).everywhere().to("devel"); - } - - mockStrategy.grant(Jenkins.READ).everywhere().to("admin"); - mockStrategy.grant(Jenkins.ADMINISTER).everywhere().to("admin"); - for (Permission p : Item.PERMISSIONS.getPermissions()) { - mockStrategy.grant(p).everywhere().to("admin"); - } - - r.jenkins.setAuthorizationStrategy(mockStrategy); + setBasicSecurity(); try (ACLContext ctx = ACL.as(User.getById("devel", true))) { assertTrue(ScriptApproval.get().isForceSandbox()); @@ -299,10 +285,7 @@ public void forceSandboxScriptSignatureException() throws Exception { @Test public void forceSandboxFormValidation() throws Exception { - r.jenkins.setSecurityRealm(r.createDummySecurityRealm()); - r.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy(). - grant(Jenkins.READ, Item.READ).everywhere().to("dev"). - grant(Jenkins.ADMINISTER).everywhere().to("admin")); + setBasicSecurity(); try (ACLContext ctx = ACL.as(User.getById("devel", true))) { ScriptApproval.get().setForceSandbox(true); @@ -346,6 +329,98 @@ public void forceSandboxFormValidation() throws Exception { } } + @Test + public void shouldHideSandboxTest() throws Exception { + setBasicSecurity(); + + ScriptApproval.get().setForceSandbox(true); + + SecureGroovyScript testSandboxTrue = new SecureGroovyScript("jenkins.model.Jenkins.instance", true, null); + SecureGroovyScript testSandboxFalse = new SecureGroovyScript("jenkins.model.Jenkins.instance", false, null); + + try (ACLContext ctx = ACL.as(User.getById("devel", true))) { + assertTrue(ScriptApproval.shouldHideSandbox(testSandboxTrue, SecureGroovyScript::isSandbox)); + assertFalse(ScriptApproval.shouldHideSandbox(testSandboxFalse, SecureGroovyScript::isSandbox)); + assertTrue(ScriptApproval.shouldHideSandbox(null, SecureGroovyScript::isSandbox)); + } + + try (ACLContext ctx = ACL.as(User.getById("admin", true))) { + assertFalse(ScriptApproval.shouldHideSandbox(testSandboxTrue, SecureGroovyScript::isSandbox)); + assertFalse(ScriptApproval.shouldHideSandbox(testSandboxFalse, SecureGroovyScript::isSandbox)); + assertFalse(ScriptApproval.shouldHideSandbox(null, SecureGroovyScript::isSandbox)); + } + + ScriptApproval.get().setForceSandbox(false); + + try (ACLContext ctx = ACL.as(User.getById("devel", true))) { + assertFalse(ScriptApproval.shouldHideSandbox(testSandboxTrue, SecureGroovyScript::isSandbox)); + assertFalse(ScriptApproval.shouldHideSandbox(testSandboxFalse, SecureGroovyScript::isSandbox)); + assertFalse(ScriptApproval.shouldHideSandbox(null, SecureGroovyScript::isSandbox)); + } + + try (ACLContext ctx = ACL.as(User.getById("admin", true))) { + assertFalse(ScriptApproval.shouldHideSandbox(testSandboxTrue, SecureGroovyScript::isSandbox)); + assertFalse(ScriptApproval.shouldHideSandbox(testSandboxFalse, SecureGroovyScript::isSandbox)); + assertFalse(ScriptApproval.shouldHideSandbox(null, SecureGroovyScript::isSandbox)); + } + } + + @Test + public void validateSandboxTest() throws Exception { + setBasicSecurity(); + + ScriptApproval.get().setForceSandbox(true); + + try (ACLContext ctx = ACL.as(User.getById("devel", true))) { + ScriptApproval.validateSandbox(true); + assertThrows(Descriptor.FormException.class, + () -> ScriptApproval.validateSandbox(false)); + } + + try (ACLContext ctx = ACL.as(User.getById("admin", true))) { + ScriptApproval.validateSandbox(true); + ScriptApproval.validateSandbox(false); + } + + ScriptApproval.get().setForceSandbox(false); + + try (ACLContext ctx = ACL.as(User.getById("devel", true))) { + ScriptApproval.validateSandbox(true); + ScriptApproval.validateSandbox(false); + } + + try (ACLContext ctx = ACL.as(User.getById("admin", true))) { + ScriptApproval.validateSandbox(true); + ScriptApproval.validateSandbox(false); + } + } + + /** + * Will configure a mock security settings with users: + * Devel: overall Read and write without admin permission + * admin: System administrator + */ + private void setBasicSecurity() + { + r.jenkins.setSecurityRealm(r.createDummySecurityRealm()); + + ScriptApproval.get().setForceSandbox(true); + + MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy(); + mockStrategy.grant(Jenkins.READ).everywhere().to("devel"); + for (Permission p : Item.PERMISSIONS.getPermissions()) { + mockStrategy.grant(p).everywhere().to("devel"); + } + + mockStrategy.grant(Jenkins.READ).everywhere().to("admin"); + mockStrategy.grant(Jenkins.ADMINISTER).everywhere().to("admin"); + for (Permission p : Item.PERMISSIONS.getPermissions()) { + mockStrategy.grant(p).everywhere().to("admin"); + } + + r.jenkins.setAuthorizationStrategy(mockStrategy); + } + private Script script(String groovy) { return new Script(groovy); } From 7f1e5945a11d7392453de42e0a0c62bdff011858 Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Thu, 31 Oct 2024 10:56:26 +0100 Subject: [PATCH 3/8] JENKINS-73941 - HideSandbox - Unify all the logic in Script-Security plugin - remove unused dependencies --- .../plugins/scriptsecurity/scripts/ScriptApproval.java | 2 -- 1 file changed, 2 deletions(-) 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 8cd9ee97..611deee3 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -32,8 +32,6 @@ import jenkins.model.GlobalConfiguration; import jenkins.model.GlobalConfigurationCategory; import jenkins.util.SystemProperties; - -import gnu.crypto.hash.IMessageDigest; import net.sf.json.JSONArray; import net.sf.json.JSONObject; import org.apache.commons.lang.StringUtils; From c2f1467cbd7e91853b269fdc67821b3ceba555df Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Thu, 31 Oct 2024 10:58:28 +0100 Subject: [PATCH 4/8] JENKINS-73941 - HideSandbox - Unify all the logic in Script-Security plugin - remove unused dependencies --- .../scriptsecurity/sandbox/groovy/SecureGroovyScript.java | 2 -- 1 file changed, 2 deletions(-) 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 f5ac162d..37148175 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 @@ -28,8 +28,6 @@ import groovy.lang.Binding; import groovy.lang.GroovyClassLoader; import groovy.lang.GroovyShell; -import groovy.lang.Script; - import hudson.Extension; import hudson.PluginManager; import hudson.model.AbstractDescribableImpl; From bb14a9b5c3f07299d64b4838710d05b80e3e6367 Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Thu, 31 Oct 2024 17:01:04 +0100 Subject: [PATCH 5/8] JENKINS-73941 - HideSandbox - Javadocs --- .../scripts/ScriptApproval.java | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) 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 611deee3..6d30ee70 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -1006,12 +1006,19 @@ public void setForceSandbox(boolean forceSandbox) { save(); } - + /** + * Flag indicating whether the current system is blocking non sandbox operations for non Admin users. + * @return + */ public boolean isForceSandbox() { return forceSandbox; } - //ForceSandbox restrictions does not apply to ADMINISTER users. + /** + * Logic to indicate if the flag {@link #isForceSandbox} applies for the current user.
+ * It does not apply for admin users. + * @return + */ public boolean isForceSandboxForCurrentUser() { return forceSandbox && !Jenkins.get().hasPermission(Jenkins.ADMINISTER); } @@ -1338,11 +1345,29 @@ public synchronized JSON clearApprovedClasspathEntries() throws IOException { @Extension public static class FormValidationPageDecorator extends PageDecorator {} + /** + * All sandbox checkboxes in the system should confirm their visibility based on this flag.
+ * It depends on the current sandbox value in the affected instance and + * {@link org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval#isForceSandboxForCurrentUser} + * @param instance + * @param isSandbox + * method in the instance class confirming the sandbox current value for the instance. + * @return + * @param + */ public static boolean shouldHideSandbox(@CheckForNull T instance, Predicate isSandbox){ return get().isForceSandboxForCurrentUser() && (instance == null || isSandbox.test(instance)); } + /** + * All describable containing the Sandbox flag should invoke this method before saving.
+ * It will confirm if the current user can persist the information in case the sandbox flag is disabled. + * It depends on {@link org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval#isForceSandboxForCurrentUser} + * In case the current user can't save it will raise a new {@link Descriptor.FormException} + * @param sandbox + * @throws Descriptor.FormException + */ public static void validateSandbox(boolean sandbox) throws Descriptor.FormException{ if (!sandbox && get().isForceSandboxForCurrentUser()) { throw new Descriptor.FormException(Messages.ScriptApproval_SandboxCantBeDisabled(), "sandbox"); From bec8b7a674fa679c22e7fe3bfe39540e7e0805cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Garc=C3=ADa?= <60674455+jgarciacloudbees@users.noreply.github.com> Date: Thu, 31 Oct 2024 17:52:53 +0100 Subject: [PATCH 6/8] JENKINS-73941 - Apply suggestions from code review Co-authored-by: Jesse Glick --- .../plugins/scriptsecurity/scripts/ScriptApproval.java | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) 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 6d30ee70..667cd670 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -1008,7 +1008,6 @@ public void setForceSandbox(boolean forceSandbox) { /** * Flag indicating whether the current system is blocking non sandbox operations for non Admin users. - * @return */ public boolean isForceSandbox() { return forceSandbox; @@ -1017,7 +1016,6 @@ public boolean isForceSandbox() { /** * Logic to indicate if the flag {@link #isForceSandbox} applies for the current user.
* It does not apply for admin users. - * @return */ public boolean isForceSandboxForCurrentUser() { return forceSandbox && !Jenkins.get().hasPermission(Jenkins.ADMINISTER); @@ -1349,11 +1347,7 @@ public static class FormValidationPageDecorator extends PageDecorator {} * All sandbox checkboxes in the system should confirm their visibility based on this flag.
* It depends on the current sandbox value in the affected instance and * {@link org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval#isForceSandboxForCurrentUser} - * @param instance - * @param isSandbox - * method in the instance class confirming the sandbox current value for the instance. - * @return - * @param + * @param isSandbox method handle in the instance class confirming the sandbox current value for the instance. */ public static boolean shouldHideSandbox(@CheckForNull T instance, Predicate isSandbox){ return get().isForceSandboxForCurrentUser() @@ -1363,7 +1357,7 @@ public static boolean shouldHideSandbox(@CheckForNull T instance, Predicate< /** * All describable containing the Sandbox flag should invoke this method before saving.
* It will confirm if the current user can persist the information in case the sandbox flag is disabled. - * It depends on {@link org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval#isForceSandboxForCurrentUser} + * It depends on {@link #isForceSandboxForCurrentUser} * In case the current user can't save it will raise a new {@link Descriptor.FormException} * @param sandbox * @throws Descriptor.FormException From aa0f5bf0e2a8c42c79c37147e3c36e1c44f15f51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Garc=C3=ADa?= <60674455+jgarciacloudbees@users.noreply.github.com> Date: Mon, 4 Nov 2024 08:57:23 +0100 Subject: [PATCH 7/8] JENKINS-73941 - Apply suggestions from code review Co-authored-by: Jesse Glick --- .../plugins/scriptsecurity/scripts/ScriptApproval.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 667cd670..fef4df7f 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -1346,7 +1346,7 @@ public static class FormValidationPageDecorator extends PageDecorator {} /** * All sandbox checkboxes in the system should confirm their visibility based on this flag.
* It depends on the current sandbox value in the affected instance and - * {@link org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval#isForceSandboxForCurrentUser} + * {@link #isForceSandboxForCurrentUser} * @param isSandbox method handle in the instance class confirming the sandbox current value for the instance. */ public static boolean shouldHideSandbox(@CheckForNull T instance, Predicate isSandbox){ From d22f9ce290ef4a5efd6e428c2b2707401f779ae4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Garc=C3=ADa?= <60674455+jgarciacloudbees@users.noreply.github.com> Date: Mon, 4 Nov 2024 17:22:09 +0100 Subject: [PATCH 8/8] JENKINS-73941 - Apply suggestions from code review Co-authored-by: Jesse Glick --- .../plugins/scriptsecurity/scripts/ScriptApproval.java | 2 -- 1 file changed, 2 deletions(-) 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 fef4df7f..907b4339 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -1359,8 +1359,6 @@ public static boolean shouldHideSandbox(@CheckForNull T instance, Predicate< * It will confirm if the current user can persist the information in case the sandbox flag is disabled. * It depends on {@link #isForceSandboxForCurrentUser} * In case the current user can't save it will raise a new {@link Descriptor.FormException} - * @param sandbox - * @throws Descriptor.FormException */ public static void validateSandbox(boolean sandbox) throws Descriptor.FormException{ if (!sandbox && get().isForceSandboxForCurrentUser()) {