Skip to content

Commit 9b98a4e

Browse files
jgarciacloudbeesjglick
jgarciacloudbees
and
jglick
authored
JENKINS-73941 - ForceSandbox - Unify logic in Script-Security for reducing techDebt (#586)
* JENKINS-73941 - HideSandbox - Unify all the logic in Script-Security plugin * JENKINS-73941 - HideSandbox - Unify all the logic in Script-Security plugin - Tests * JENKINS-73941 - HideSandbox - Unify all the logic in Script-Security plugin - remove unused dependencies * JENKINS-73941 - HideSandbox - Unify all the logic in Script-Security plugin - remove unused dependencies * JENKINS-73941 - HideSandbox - Javadocs * JENKINS-73941 - Apply suggestions from code review Co-authored-by: Jesse Glick <jglick@cloudbees.com> * JENKINS-73941 - Apply suggestions from code review Co-authored-by: Jesse Glick <jglick@cloudbees.com> * JENKINS-73941 - Apply suggestions from code review Co-authored-by: Jesse Glick <jglick@cloudbees.com> --------- Co-authored-by: Jesse Glick <jglick@cloudbees.com>
1 parent bb402e3 commit 9b98a4e

File tree

3 files changed

+131
-28
lines changed

3 files changed

+131
-28
lines changed

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

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,8 @@ public final class SecureGroovyScript extends AbstractDescribableImpl<SecureGroo
9696
@DataBoundConstructor public SecureGroovyScript(@NonNull String script, boolean sandbox,
9797
@CheckForNull List<ClasspathEntry> classpath)
9898
throws Descriptor.FormException {
99-
if (!sandbox && ScriptApproval.get().isForceSandboxForCurrentUser()) {
100-
throw new Descriptor.FormException(Messages.ScriptApproval_SandboxCantBeDisabled(), "sandbox");
101-
}
99+
ScriptApproval.validateSandbox(sandbox);
100+
102101
this.script = script;
103102
this.sandbox = sandbox;
104103
this.classpath = classpath;
@@ -473,8 +472,7 @@ public FormValidation doCheckScript(@QueryParameter String value, @QueryParamete
473472
public boolean shouldHideSandbox(@CheckForNull SecureGroovyScript instance) {
474473
// sandbox checkbox is shown to admins even if the global configuration says otherwise
475474
// it's also shown when sandbox == false, so regular users can enable it
476-
return ScriptApproval.get().isForceSandboxForCurrentUser()
477-
&& (instance == null || instance.sandbox);
475+
return ScriptApproval.shouldHideSandbox(instance,SecureGroovyScript::isSandbox);
478476
}
479477

480478
}

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

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626

2727
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
2828
import hudson.model.BallColor;
29+
import hudson.model.Descriptor;
2930
import hudson.model.PageDecorator;
3031
import hudson.security.ACLContext;
3132
import jenkins.model.GlobalConfiguration;
@@ -68,6 +69,7 @@
6869
import java.util.Stack;
6970
import java.util.TreeSet;
7071
import java.util.function.Consumer;
72+
import java.util.function.Predicate;
7173
import java.util.logging.Level;
7274
import java.util.logging.Logger;
7375
import java.util.regex.Pattern;
@@ -1004,12 +1006,17 @@ public void setForceSandbox(boolean forceSandbox) {
10041006
save();
10051007
}
10061008

1007-
1009+
/**
1010+
* Flag indicating whether the current system is blocking non sandbox operations for non Admin users.
1011+
*/
10081012
public boolean isForceSandbox() {
10091013
return forceSandbox;
10101014
}
10111015

1012-
//ForceSandbox restrictions does not apply to ADMINISTER users.
1016+
/**
1017+
* Logic to indicate if the flag {@link #isForceSandbox} applies for the current user. <br />
1018+
* It does not apply for admin users.
1019+
*/
10131020
public boolean isForceSandboxForCurrentUser() {
10141021
return forceSandbox && !Jenkins.get().hasPermission(Jenkins.ADMINISTER);
10151022
}
@@ -1335,4 +1342,27 @@ public synchronized JSON clearApprovedClasspathEntries() throws IOException {
13351342
@Restricted(NoExternalUse.class)
13361343
@Extension
13371344
public static class FormValidationPageDecorator extends PageDecorator {}
1345+
1346+
/**
1347+
* All sandbox checkboxes in the system should confirm their visibility based on this flag.<br />
1348+
* It depends on the current sandbox value in the affected instance and
1349+
* {@link #isForceSandboxForCurrentUser}
1350+
* @param isSandbox method handle in the instance class confirming the sandbox current value for the instance.
1351+
*/
1352+
public static <T> boolean shouldHideSandbox(@CheckForNull T instance, Predicate<T> isSandbox){
1353+
return get().isForceSandboxForCurrentUser()
1354+
&& (instance == null || isSandbox.test(instance));
1355+
}
1356+
1357+
/**
1358+
* All describable containing the Sandbox flag should invoke this method before saving.<br />
1359+
* It will confirm if the current user can persist the information in case the sandbox flag is disabled.
1360+
* It depends on {@link #isForceSandboxForCurrentUser}
1361+
* In case the current user can't save it will raise a new {@link Descriptor.FormException}
1362+
*/
1363+
public static void validateSandbox(boolean sandbox) throws Descriptor.FormException{
1364+
if (!sandbox && get().isForceSandboxForCurrentUser()) {
1365+
throw new Descriptor.FormException(Messages.ScriptApproval_SandboxCantBeDisabled(), "sandbox");
1366+
}
1367+
}
13381368
}

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

Lines changed: 96 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@
2626

2727
import org.htmlunit.html.HtmlPage;
2828
import org.htmlunit.html.HtmlTextArea;
29+
30+
import hudson.model.Descriptor;
2931
import hudson.model.FreeStyleBuild;
3032
import hudson.model.FreeStyleProject;
3133
import hudson.model.Item;
@@ -209,23 +211,7 @@ public void reload() throws Exception {
209211

210212
@Test
211213
public void forceSandboxTests() throws Exception {
212-
r.jenkins.setSecurityRealm(r.createDummySecurityRealm());
213-
214-
ScriptApproval.get().setForceSandbox(true);
215-
216-
MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy();
217-
mockStrategy.grant(Jenkins.READ).everywhere().to("devel");
218-
for (Permission p : Item.PERMISSIONS.getPermissions()) {
219-
mockStrategy.grant(p).everywhere().to("devel");
220-
}
221-
222-
mockStrategy.grant(Jenkins.READ).everywhere().to("admin");
223-
mockStrategy.grant(Jenkins.ADMINISTER).everywhere().to("admin");
224-
for (Permission p : Item.PERMISSIONS.getPermissions()) {
225-
mockStrategy.grant(p).everywhere().to("admin");
226-
}
227-
228-
r.jenkins.setAuthorizationStrategy(mockStrategy);
214+
setBasicSecurity();
229215

230216
try (ACLContext ctx = ACL.as(User.getById("devel", true))) {
231217
assertTrue(ScriptApproval.get().isForceSandbox());
@@ -299,10 +285,7 @@ public void forceSandboxScriptSignatureException() throws Exception {
299285

300286
@Test
301287
public void forceSandboxFormValidation() throws Exception {
302-
r.jenkins.setSecurityRealm(r.createDummySecurityRealm());
303-
r.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy().
304-
grant(Jenkins.READ, Item.READ).everywhere().to("dev").
305-
grant(Jenkins.ADMINISTER).everywhere().to("admin"));
288+
setBasicSecurity();
306289

307290
try (ACLContext ctx = ACL.as(User.getById("devel", true))) {
308291
ScriptApproval.get().setForceSandbox(true);
@@ -346,6 +329,98 @@ public void forceSandboxFormValidation() throws Exception {
346329
}
347330
}
348331

332+
@Test
333+
public void shouldHideSandboxTest() throws Exception {
334+
setBasicSecurity();
335+
336+
ScriptApproval.get().setForceSandbox(true);
337+
338+
SecureGroovyScript testSandboxTrue = new SecureGroovyScript("jenkins.model.Jenkins.instance", true, null);
339+
SecureGroovyScript testSandboxFalse = new SecureGroovyScript("jenkins.model.Jenkins.instance", false, null);
340+
341+
try (ACLContext ctx = ACL.as(User.getById("devel", true))) {
342+
assertTrue(ScriptApproval.shouldHideSandbox(testSandboxTrue, SecureGroovyScript::isSandbox));
343+
assertFalse(ScriptApproval.shouldHideSandbox(testSandboxFalse, SecureGroovyScript::isSandbox));
344+
assertTrue(ScriptApproval.shouldHideSandbox(null, SecureGroovyScript::isSandbox));
345+
}
346+
347+
try (ACLContext ctx = ACL.as(User.getById("admin", true))) {
348+
assertFalse(ScriptApproval.shouldHideSandbox(testSandboxTrue, SecureGroovyScript::isSandbox));
349+
assertFalse(ScriptApproval.shouldHideSandbox(testSandboxFalse, SecureGroovyScript::isSandbox));
350+
assertFalse(ScriptApproval.shouldHideSandbox(null, SecureGroovyScript::isSandbox));
351+
}
352+
353+
ScriptApproval.get().setForceSandbox(false);
354+
355+
try (ACLContext ctx = ACL.as(User.getById("devel", true))) {
356+
assertFalse(ScriptApproval.shouldHideSandbox(testSandboxTrue, SecureGroovyScript::isSandbox));
357+
assertFalse(ScriptApproval.shouldHideSandbox(testSandboxFalse, SecureGroovyScript::isSandbox));
358+
assertFalse(ScriptApproval.shouldHideSandbox(null, SecureGroovyScript::isSandbox));
359+
}
360+
361+
try (ACLContext ctx = ACL.as(User.getById("admin", true))) {
362+
assertFalse(ScriptApproval.shouldHideSandbox(testSandboxTrue, SecureGroovyScript::isSandbox));
363+
assertFalse(ScriptApproval.shouldHideSandbox(testSandboxFalse, SecureGroovyScript::isSandbox));
364+
assertFalse(ScriptApproval.shouldHideSandbox(null, SecureGroovyScript::isSandbox));
365+
}
366+
}
367+
368+
@Test
369+
public void validateSandboxTest() throws Exception {
370+
setBasicSecurity();
371+
372+
ScriptApproval.get().setForceSandbox(true);
373+
374+
try (ACLContext ctx = ACL.as(User.getById("devel", true))) {
375+
ScriptApproval.validateSandbox(true);
376+
assertThrows(Descriptor.FormException.class,
377+
() -> ScriptApproval.validateSandbox(false));
378+
}
379+
380+
try (ACLContext ctx = ACL.as(User.getById("admin", true))) {
381+
ScriptApproval.validateSandbox(true);
382+
ScriptApproval.validateSandbox(false);
383+
}
384+
385+
ScriptApproval.get().setForceSandbox(false);
386+
387+
try (ACLContext ctx = ACL.as(User.getById("devel", true))) {
388+
ScriptApproval.validateSandbox(true);
389+
ScriptApproval.validateSandbox(false);
390+
}
391+
392+
try (ACLContext ctx = ACL.as(User.getById("admin", true))) {
393+
ScriptApproval.validateSandbox(true);
394+
ScriptApproval.validateSandbox(false);
395+
}
396+
}
397+
398+
/**
399+
* Will configure a mock security settings with users:
400+
* Devel: overall Read and write without admin permission
401+
* admin: System administrator
402+
*/
403+
private void setBasicSecurity()
404+
{
405+
r.jenkins.setSecurityRealm(r.createDummySecurityRealm());
406+
407+
ScriptApproval.get().setForceSandbox(true);
408+
409+
MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy();
410+
mockStrategy.grant(Jenkins.READ).everywhere().to("devel");
411+
for (Permission p : Item.PERMISSIONS.getPermissions()) {
412+
mockStrategy.grant(p).everywhere().to("devel");
413+
}
414+
415+
mockStrategy.grant(Jenkins.READ).everywhere().to("admin");
416+
mockStrategy.grant(Jenkins.ADMINISTER).everywhere().to("admin");
417+
for (Permission p : Item.PERMISSIONS.getPermissions()) {
418+
mockStrategy.grant(p).everywhere().to("admin");
419+
}
420+
421+
r.jenkins.setAuthorizationStrategy(mockStrategy);
422+
}
423+
349424
private Script script(String groovy) {
350425
return new Script(groovy);
351426
}

0 commit comments

Comments
 (0)