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

[JEP-223] Deprecate permissions RUN_SCRIPTS, UPLOAD_PLUGINS, and CONFIGURE_UPDATECENTER #4365

Merged
merged 35 commits into from
Feb 22, 2020
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
e9b2a73
Flag RUN_SCRIPTS permission as deprecated
mikecirioli Nov 20, 2019
825afaa
Change permission checks for RUN_SCRIPTS to check for ADMINISTER
mikecirioli Nov 20, 2019
ede3a6d
flag UPLOAD_PLUGINS as deprecated
mikecirioli Nov 20, 2019
adc4ef8
change checks for UPLOAD_PLUGINS to check for ADMINISTER
mikecirioli Nov 20, 2019
779ccd1
flag CONFIGURE_UPDATECENTER as deprecated
mikecirioli Nov 20, 2019
3b70d30
change permission checks for CONFIGURE_UPDATECENTER to check for ADMI…
mikecirioli Nov 20, 2019
2bcad23
Update core/src/main/java/jenkins/model/Jenkins.java
mikecirioli Dec 6, 2019
aa0f378
Update core/src/main/java/jenkins/management/ConsoleLink.java
mikecirioli Dec 6, 2019
953a5ee
Update core/src/main/java/jenkins/management/ConsoleLink.java
mikecirioli Dec 6, 2019
db42863
Update core/src/main/java/jenkins/management/ConsoleLink.java
mikecirioli Dec 6, 2019
caed539
Revert "Update core/src/main/java/jenkins/management/ConsoleLink.java"
mikecirioli Dec 6, 2019
e7bc69d
make the deprecated permissions == Jenkins.ADMINISTER, since that is …
mikecirioli Dec 6, 2019
63af530
Check for Jenkins.ADMINISTER instead of Jenkins.RUN_SCRIPTS
mikecirioli Dec 6, 2019
b8e6d4d
Update jelly checks for deprecated permissions to now check for
mikecirioli Dec 6, 2019
3eaaf12
Fix broken test that checked for RUN_SCRIPTS
mikecirioli Dec 6, 2019
8b72487
Revert "make the deprecated permissions == Jenkins.ADMINISTER, since …
mikecirioli Dec 17, 2019
17063fe
re-enable setting dangerous permissions == Jenkins.ADMINISTER
mikecirioli Dec 19, 2019
6dfa633
remove commented code
mikecirioli Feb 20, 2020
543ab06
fixup javadocs for deprecated permissions
mikecirioli Feb 20, 2020
61be95d
Merge branch 'master' into deprecate_risky_perms
mikecirioli Feb 20, 2020
b300f46
fix broken javadoc link
mikecirioli Feb 20, 2020
00dd15b
Merge branch 'deprecate_risky_perms' of github.com:mikecirioli/jenkin…
mikecirioli Feb 20, 2020
f87fc3a
Revert "remove commented code"
mikecirioli Feb 20, 2020
df2c0a8
Reverts explicity making dangerous permissions == Jenkins.ADMINISTER
mikecirioli Feb 20, 2020
3ff6a56
Update english descriptive text properties for dangerous permissions
mikecirioli Feb 20, 2020
0491ee6
fix: do not use deprecated getActiveInstance()
mikecirioli Feb 20, 2020
1e308ac
This broken test
mikecirioli Feb 20, 2020
db4ae53
add comment to update test once RUN_SCRIPTS has been retired
mikecirioli Feb 20, 2020
625fc45
nit: add space
mikecirioli Feb 20, 2020
51276ea
nit: add space
mikecirioli Feb 20, 2020
7ab354e
nit: add space
mikecirioli Feb 20, 2020
cd1adf4
remove un-needed permission checks in plugimanager jelly files
mikecirioli Feb 20, 2020
8050bf3
remove localizations of deprecated permissions
mikecirioli Feb 21, 2020
16ffec7
revert italian locazation that got bungled on local system
mikecirioli Feb 21, 2020
2019f8d
remove deprecated perms from italian localizations
mikecirioli Feb 21, 2020
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
16 changes: 10 additions & 6 deletions core/src/main/java/hudson/PluginManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -1397,7 +1397,7 @@ public HttpResponse doPlugins() {

@RequirePOST
public HttpResponse doUpdateSources(StaplerRequest req) throws IOException {
Jenkins.get().checkPermission(CONFIGURE_UPDATECENTER);
Jenkins.get().checkPermission(Jenkins.ADMINISTER);

if (req.hasParameter("remove")) {
UpdateCenter uc = Jenkins.get().getUpdateCenter();
Expand Down Expand Up @@ -1603,7 +1603,7 @@ private UpdateSite.Plugin getPlugin(String pluginName, String siteName) {
@RequirePOST
public HttpResponse doSiteConfigure(@QueryParameter String site) throws IOException {
Jenkins hudson = Jenkins.get();
hudson.checkPermission(CONFIGURE_UPDATECENTER);
hudson.checkPermission(Jenkins.ADMINISTER);
UpdateCenter uc = hudson.getUpdateCenter();
PersistedList<UpdateSite> sites = uc.getSites();
for (UpdateSite s : sites) {
Expand All @@ -1618,7 +1618,7 @@ public HttpResponse doSiteConfigure(@QueryParameter String site) throws IOExcept
@POST
public HttpResponse doProxyConfigure(StaplerRequest req) throws IOException, ServletException {
Jenkins jenkins = Jenkins.get();
jenkins.checkPermission(CONFIGURE_UPDATECENTER);
jenkins.checkPermission(Jenkins.ADMINISTER);

ProxyConfiguration pc = req.bindJSON(ProxyConfiguration.class, req.getSubmittedForm());
if (pc.name==null) {
Expand All @@ -1637,7 +1637,7 @@ public HttpResponse doProxyConfigure(StaplerRequest req) throws IOException, Ser
@RequirePOST
public HttpResponse doUploadPlugin(StaplerRequest req) throws IOException, ServletException {
try {
Jenkins.get().checkPermission(UPLOAD_PLUGINS);
Jenkins.get().checkPermission(Jenkins.ADMINISTER);
Copy link
Member

Choose a reason for hiding this comment

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

Strictly speaking, none of the permission checks in this file matter, but if System Read is about to land, it probably makes sense to keep them.


ServletFileUpload upload = new ServletFileUpload(new DiskFileItemFactory());

Expand Down Expand Up @@ -2105,8 +2105,12 @@ public String toString() {
}
public static boolean FAST_LOOKUP = !SystemProperties.getBoolean(PluginManager.class.getName()+".noFastLookup");

public static final Permission UPLOAD_PLUGINS = new Permission(Jenkins.PERMISSIONS, "UploadPlugins", Messages._PluginManager_UploadPluginsPermission_Description(),Jenkins.ADMINISTER,PermissionScope.JENKINS);
public static final Permission CONFIGURE_UPDATECENTER = new Permission(Jenkins.PERMISSIONS, "ConfigureUpdateCenter", Messages._PluginManager_ConfigureUpdateCenterPermission_Description(),Jenkins.ADMINISTER,PermissionScope.JENKINS);
/** @deprecated use {@link Jenkins#ADMINISTER} instead */
mikecirioli marked this conversation as resolved.
Show resolved Hide resolved
@Deprecated
mikecirioli marked this conversation as resolved.
Show resolved Hide resolved
public static final Permission UPLOAD_PLUGINS = Jenkins.ADMINISTER;
/** @deprecated use {@link Jenkins#ADMINISTER} instead */
@Deprecated
public static final Permission CONFIGURE_UPDATECENTER = Jenkins.ADMINISTER;

/**
* Remembers why a plugin failed to deploy.
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/cli/GroovyCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public String getShortDescription() {

protected int run() throws Exception {
// this allows the caller to manipulate the JVM state, so require the execute script privilege.
Jenkins.getActiveInstance().checkPermission(Jenkins.RUN_SCRIPTS);
Jenkins.getActiveInstance().checkPermission(Jenkins.ADMINISTER);
mikecirioli marked this conversation as resolved.
Show resolved Hide resolved

Binding binding = new Binding();
binding.setProperty("out",new PrintWriter(stdout,true));
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/cli/GroovyshCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public String getShortDescription() {
@Override
protected int run() {
// this allows the caller to manipulate the JVM state, so require the admin privilege.
Jenkins.getActiveInstance().checkPermission(Jenkins.RUN_SCRIPTS);
Jenkins.getActiveInstance().checkPermission(Jenkins.ADMINISTER);
mikecirioli marked this conversation as resolved.
Show resolved Hide resolved

// this being remote means no jline capability is available
System.setProperty("jline.terminal", UnsupportedTerminal.class.getName());
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/cli/InstallPluginCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public String getShortDescription() {
@Override
protected int run() throws Exception {
Jenkins h = Jenkins.get();
h.checkPermission(PluginManager.UPLOAD_PLUGINS);
h.checkPermission(Jenkins.ADMINISTER);
PluginManager pm = h.getPluginManager();

if (name != null) {
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/util/RemotingDiagnostics.java
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ public void doIndex(StaplerResponse rsp) throws IOException {

@WebMethod(name="heapdump.hprof")
public void doHeapDump(StaplerRequest req, StaplerResponse rsp) throws IOException, InterruptedException {
owner.checkPermission(Jenkins.RUN_SCRIPTS);
owner.checkPermission(Jenkins.ADMINISTER);
rsp.setContentType("application/octet-stream");

FilePath dump = obtain();
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/jenkins/management/ConsoleLink.java
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,6 @@ public String getUrlName() {

@Override
public Permission getRequiredPermission() {
mikecirioli marked this conversation as resolved.
Show resolved Hide resolved
return Jenkins.RUN_SCRIPTS;
return Jenkins.ADMINISTER;
mikecirioli marked this conversation as resolved.
Show resolved Hide resolved
}
mikecirioli marked this conversation as resolved.
Show resolved Hide resolved
}
8 changes: 5 additions & 3 deletions core/src/main/java/jenkins/model/Jenkins.java
Original file line number Diff line number Diff line change
Expand Up @@ -4494,7 +4494,7 @@ public void doScriptText(StaplerRequest req, StaplerResponse rsp) throws IOExcep
*/
public static void _doScript(StaplerRequest req, StaplerResponse rsp, RequestDispatcher view, VirtualChannel channel, ACL acl) throws IOException, ServletException {
// ability to run arbitrary script is dangerous
acl.checkPermission(RUN_SCRIPTS);
acl.checkPermission(ADMINISTER);

String text = req.getParameter("script");
if (text != null) {
Expand Down Expand Up @@ -4524,7 +4524,7 @@ public static void _doScript(StaplerRequest req, StaplerResponse rsp, RequestDis
*/
@RequirePOST
public void doEval(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException {
checkPermission(RUN_SCRIPTS);
checkPermission(ADMINISTER);
mikecirioli marked this conversation as resolved.
Show resolved Hide resolved
req.getWebApp().getDispatchValidator().allowDispatch(req, rsp);
try {
MetaClass mc = req.getWebApp().getMetaClass(getClass());
Expand Down Expand Up @@ -5269,7 +5269,9 @@ private static void computeVersion(ServletContext context) {
public static final PermissionGroup PERMISSIONS = Permission.HUDSON_PERMISSIONS;
public static final Permission ADMINISTER = Permission.HUDSON_ADMINISTER;
public static final Permission READ = new Permission(PERMISSIONS,"Read",Messages._Hudson_ReadPermission_Description(),Permission.READ,PermissionScope.JENKINS);
public static final Permission RUN_SCRIPTS = new Permission(PERMISSIONS, "RunScripts", Messages._Hudson_RunScriptsPermission_Description(),ADMINISTER,PermissionScope.JENKINS);
/** @deprecated use {@link #ADMINISTER} instead */
@Deprecated
mikecirioli marked this conversation as resolved.
Show resolved Hide resolved
public static final Permission RUN_SCRIPTS = ADMINISTER;

/**
* Urls that are always visible without READ permission.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ public boolean getMasterKillSwitch() {
public void setMasterKillSwitch(boolean state) {
final Jenkins jenkins = Jenkins.get();
try {
jenkins.checkPermission(Jenkins.RUN_SCRIPTS);
jenkins.checkPermission(Jenkins.ADMINISTER);
File f = getMasterKillSwitchFile(jenkins);
FileUtils.writeStringToFile(f, Boolean.toString(state), Charset.defaultCharset());
// treat the file as the canonical source of information in case write fails
Expand All @@ -227,7 +227,7 @@ public void setMasterKillSwitch(boolean state) {
*/
@Override
public Object getTarget() {
Jenkins.get().checkPermission(Jenkins.RUN_SCRIPTS);
Jenkins.get().checkPermission(Jenkins.ADMINISTER);
return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public boolean configure(StaplerRequest req, JSONObject json) throws FormExcepti
* Unless this option is relevant, we don't let users choose this.
*/
public boolean isRelevant() {
return jenkins.hasPermission(Jenkins.RUN_SCRIPTS) && jenkins.isUseSecurity();
return jenkins.hasPermission(Jenkins.ADMINISTER) && jenkins.isUseSecurity();
}
}

6 changes: 3 additions & 3 deletions core/src/main/resources/hudson/PluginManager/advanced.jelly
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ THE SOFTWARE.
<table id="pluginsAdv" class="pane" style="margin-top:0; border-top:none">
<tr style="border-top:none; white-space: normal">
<td>
<l:hasPermission permission="${app.pluginManager.CONFIGURE_UPDATECENTER}">
<l:hasPermission permission="${app.ADMINISTER}">
<h1>${%HTTP Proxy Configuration}</h1>
<f:form method="post" action="proxyConfigure" name="proxyConfigure">
<j:scope>
Expand All @@ -49,7 +49,7 @@ THE SOFTWARE.
</f:form>
</l:hasPermission>

<l:hasPermission permission="${app.pluginManager.UPLOAD_PLUGINS}">
<l:hasPermission permission="${app.ADMINISTER}">
<h1>${%Upload Plugin}</h1>
<f:form method="post" action="uploadPlugin" name="uploadPlugin" enctype="multipart/form-data">
<f:block>
Expand All @@ -66,7 +66,7 @@ THE SOFTWARE.
</f:block>
</f:form>
</l:hasPermission>
<l:hasPermission permission="${app.pluginManager.CONFIGURE_UPDATECENTER}">
<l:hasPermission permission="${app.ADMINISTER}">
mikecirioli marked this conversation as resolved.
Show resolved Hide resolved
<h1>${%Update Site}</h1>
<f:form method="post" action="siteConfigure" name="siteConfigure">
<f:entry title="${%URL}" >
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/resources/hudson/PluginManager/tabBar.jelly
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ THE SOFTWARE.
<l:tab name="${%Updates}" active="${page=='updates'}" href="." />
<l:tab name="${%Available}" active="${page=='available'}" href="./available" />
<l:tab name="${%Installed}" active="${page=='installed'}" href="./installed" />
<j:if test="${h.hasPermission(app.pluginManager.UPLOAD_PLUGINS) || h.hasPermission(app.pluginManager.CONFIGURE_UPDATECENTER)}">
<j:if test="${h.hasPermission(app.ADMINISTER) }">
mikecirioli marked this conversation as resolved.
Show resolved Hide resolved
<l:tab name="${%Advanced}" active="${page=='advanced'}" href="./advanced" />
</j:if>
</l:tabBar>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ THE SOFTWARE.
<l:task href="${rootURL}/${it.url}builds" icon="icon-notepad icon-md" title="${%Build History}"/>
<l:task href="${rootURL}/${it.url}load-statistics" icon="icon-monitor icon-md" title="${%Load Statistics}"/>
<j:if test="${it.channel!=null}">
<l:task href="${rootURL}/${it.url}script" icon="icon-terminal icon-md" permission="${app.RUN_SCRIPTS}" title="${%Script Console}"/>
<l:task href="${rootURL}/${it.url}script" icon="icon-terminal icon-md" permission="${app.ADMINISTER}" title="${%Script Console}"/>
</j:if>
<st:include page="sidepanel2.jelly" optional="true" /><!-- hook for derived class to add more items -->
<t:actions />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ THE SOFTWARE.
-->
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
<l:layout title="${%Whitelist}" permission="${app.RUN_SCRIPTS}">
<l:layout title="${%Whitelist}" permission="${app.ADMINISTER}">
<st:include page="sidepanel.jelly" it="${app}"/>
<l:main-panel>
<h1>${%Agent &#8594; Master Access Control}</h1>
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/resources/lib/hudson/scriptConsole.jelly
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ THE SOFTWARE.
-->
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:f="/lib/form">
<l:layout norefresh="true" permission="${h.RUN_SCRIPTS}">
<l:layout norefresh="true" permission="${h.ADMINISTER}">
<st:include page="sidepanel.jelly" />

<l:main-panel>
Expand Down
2 changes: 1 addition & 1 deletion test/src/test/java/hudson/cli/GroovyshCommandTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public class GroovyshCommandTest {
@Issue("JENKINS-17929")
@Test public void authentication() throws Exception {
CLICommandInvoker.Result result = new CLICommandInvoker(r, new GroovyshCommand())
.authorizedTo(Jenkins.READ, Jenkins.RUN_SCRIPTS)
.authorizedTo(Jenkins.READ, Jenkins.ADMINISTER)
.withStdin(new StringInputStream("println(jenkins.model.Jenkins.instance.getClass().name)\n:quit\n"))
.invoke();
assertThat(result, succeeded());
Expand Down
4 changes: 2 additions & 2 deletions test/src/test/java/jenkins/model/JenkinsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -355,8 +355,8 @@ public void testDoEval() throws Exception {

wc.withBasicApiToken(User.getById("charlie", true));
page = eval(wc);
assertEquals("charlie has ADMINISTER but not RUN_SCRIPTS",
HttpURLConnection.HTTP_FORBIDDEN,
assertEquals("charlie has ADMINISTER and READ",
HttpURLConnection.HTTP_OK,
page.getWebResponse().getStatusCode());
}
private Page eval(WebClient wc) throws Exception {
Expand Down