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.

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

JENKINS-73941 - Option to hide "Use Groovy Sandbox" for users without Administer permission globally in the system #584

Merged
merged 27 commits into from
Oct 25, 2024

Conversation

jgarciacloudbees
Copy link
Contributor

@jgarciacloudbees jgarciacloudbees commented Oct 17, 2024

JENKINS-73941

This is partially implemented in JENKINS-73470, where we created the new option "Hide Sandbox checkbox in pipeline jobs", but it was created in the scope of the workflow-cps-plugin.

The scope of this ticket is just to create the global option at the "script-security-plugin" scope and block the creation of new objects pending of approval.

This is usefull for highly secured environments where everything should be executed in SandBox, so we are removing the option to regular users to create pending to review scripts.

To do that, we have created the new option "forceSandbox" and created the method

org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval.isForceSandbox() 

To provide the new property to the dependand plugins using sandbox.

The next steps for this ticket is to modify the specific plugins to disable the sandbox checkbox based on this new property, forcing the nonadmin user to use the sandbox when creating new objects.

Workflow-cps-plugin PR:
jenkinsci/workflow-cps-plugin#948

Testing done

Created new testing cases when the new option is enabled, to make sure this is blocking the creating of new pending to approve objects.

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

Copy link
Contributor

@mikecirioli mikecirioli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, just small changes to some text

@jgarciacloudbees
Copy link
Contributor Author

jgarciacloudbees commented Oct 24, 2024

We have some flaky tests failure, Closing and Reopening to force running again the checks.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to conditionally hide

<f:entry field="sandbox">
<f:checkbox title="${%Use Groovy Sandbox}" default="${!h.hasPermission(app.ADMINISTER)}" />
</f:entry>
<f:entry title="${%Additional classpath}" field="classpath">
<f:repeatableProperty add="${%Add entry}" header="${%Classpath entry}" field="classpath"/>
</f:entry>

From searching around, for example try this plugin: https://github.com/jenkinsci/secure-post-script-plugin/blob/a8c196d18a8e034a66ee7cea13b8132a18e77197/src/main/resources/io/jenkins/plugins/securepostscript/SecurePostScriptConfiguration/config.jelly#L3-L6

@jglick
Copy link
Member

jglick commented Oct 25, 2024

@jgarciacloudbees please avoid force-pushing to a PR branch as it sometimes breaks the GH feature of allowing reviewers to only display changes since the last review.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#584 (comment) desirable while we are here

@jglick jglick merged commit d44b49a into jenkinsci:master Oct 25, 2024
17 checks passed
@jgarciacloudbees jgarciacloudbees deleted the JENKINS-73941 branch October 25, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants