Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JENKINS-73941 - ForceSandbox - Unify logic in Script-Security for reducing techDebt #586

Merged
merged 8 commits into from
Nov 13, 2024
Merged

JENKINS-73941 - ForceSandbox - Unify logic in Script-Security for reducing techDebt #586

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a02bf24
JENKINS-73941 - HideSandbox - Unify all the logic in Script-Security …
jgarciacloudbees Oct 31, 2024
bfb4853
JENKINS-73941 - HideSandbox - Unify all the logic in Script-Security …
jgarciacloudbees Oct 31, 2024
7f1e594
JENKINS-73941 - HideSandbox - Unify all the logic in Script-Security …
jgarciacloudbees Oct 31, 2024
c2f1467
JENKINS-73941 - HideSandbox - Unify all the logic in Script-Security …
jgarciacloudbees Oct 31, 2024
bb14a9b
JENKINS-73941 - HideSandbox - Javadocs
jgarciacloudbees Oct 31, 2024
bec8b7a
JENKINS-73941 - Apply suggestions from code review
jgarciacloudbees Oct 31, 2024
aa0f5bf
JENKINS-73941 - Apply suggestions from code review
jgarciacloudbees Nov 4, 2024
d22f9ce
JENKINS-73941 - Apply suggestions from code review
jgarciacloudbees Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,8 @@ public final class SecureGroovyScript extends AbstractDescribableImpl<SecureGroo
@DataBoundConstructor public SecureGroovyScript(@NonNull String script, boolean sandbox,
@CheckForNull List<ClasspathEntry> 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;
Expand Down Expand Up @@ -473,8 +472,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);
}

}
Expand Down
36 changes: 34 additions & 2 deletions src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

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;
Expand Down Expand Up @@ -68,6 +69,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;
Expand Down Expand Up @@ -1004,12 +1006,17 @@ public void setForceSandbox(boolean forceSandbox) {
save();
}


/**
* Flag indicating whether the current system is blocking non sandbox operations for non Admin users.
*/
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. <br />
* It does not apply for admin users.
*/
public boolean isForceSandboxForCurrentUser() {
return forceSandbox && !Jenkins.get().hasPermission(Jenkins.ADMINISTER);
}
Expand Down Expand Up @@ -1335,4 +1342,29 @@ public synchronized JSON clearApprovedClasspathEntries() throws IOException {
@Restricted(NoExternalUse.class)
@Extension
public static class FormValidationPageDecorator extends PageDecorator {}

/**
* All sandbox checkboxes in the system should confirm their visibility based on this flag.<br />
* It depends on the current sandbox value in the affected instance and
* {@link org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval#isForceSandboxForCurrentUser}
jgarciacloudbees marked this conversation as resolved.
Show resolved Hide resolved
* @param isSandbox method handle in the instance class confirming the sandbox current value for the instance.
*/
public static <T> boolean shouldHideSandbox(@CheckForNull T instance, Predicate<T> isSandbox){
jglick marked this conversation as resolved.
Show resolved Hide resolved
return get().isForceSandboxForCurrentUser()
&& (instance == null || isSandbox.test(instance));
}

/**
* All describable containing the Sandbox flag should invoke this method before saving.<br />
* 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
jgarciacloudbees marked this conversation as resolved.
Show resolved Hide resolved
jgarciacloudbees marked this conversation as resolved.
Show resolved Hide resolved
*/
public static void validateSandbox(boolean sandbox) throws Descriptor.FormException{
if (!sandbox && get().isForceSandboxForCurrentUser()) {
throw new Descriptor.FormException(Messages.ScriptApproval_SandboxCantBeDisabled(), "sandbox");
}
}
}
117 changes: 96 additions & 21 deletions src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
Expand Down