[FIXED JENKINS-18485] Use correct node permissions in remote API

From now on user needs:
  Computer.CREATE to use 'create-node' CLI command
  Computer.DELETE to use 'delete-node' CLI command
  Computer.CONFIGURE to use 'update-node' CLI command and its REST alternative
  Computer.READ to use 'get-node' CLI command and its REST alternative

core/src/main/java/hudson/cli/CreateNodeCommand.java

@@ -25,6 +25,7 @@
 package hudson.cli;
 
 import hudson.Extension;
+import hudson.model.Computer;
 import hudson.model.Node;
 import hudson.model.Slave;
 import hudson.model.User;
@@ -53,7 +54,7 @@ public class CreateNodeCommand extends CLICommand {
     protected int run() throws Exception {
 
         final Jenkins jenkins = Jenkins.getInstance();
-        jenkins.checkPermission(Jenkins.ADMINISTER);
+        jenkins.checkPermission(Computer.CREATE);
 
         final Node newNode = (Node) Jenkins.XSTREAM2.fromXML(stdin);
 

core/src/main/java/hudson/cli/GetNodeCommand.java

@@ -24,6 +24,7 @@
 package hudson.cli;
 
 import hudson.Extension;
+import hudson.model.Computer;
 import hudson.model.Node;
 
 import java.io.IOException;
@@ -51,7 +52,7 @@ public class GetNodeCommand extends CLICommand {
     @Override
     protected int run() throws IOException {
 
-        node.checkPermission(Jenkins.ADMINISTER);
+        node.checkPermission(Computer.READ);
 
         Jenkins.XSTREAM2.toXMLUTF8(node, stdout);
 

core/src/main/java/hudson/model/Computer.java

@@ -1186,9 +1186,10 @@ public /*transient*/ abstract class Computer extends Actionable implements Acces
     @WebMethod(name = "config.xml")
     public void doConfigDotXml(StaplerRequest req, StaplerResponse rsp)
             throws IOException, ServletException {
-        checkPermission(Jenkins.ADMINISTER);
+
         if (req.getMethod().equals("GET")) {
             // read
+            checkPermission(READ);
             rsp.setContentType("application/xml");
             Jenkins.XSTREAM2.toXMLUTF8(getNode(), rsp.getOutputStream());
             return;
@@ -1228,7 +1229,7 @@ public /*transient*/ abstract class Computer extends Actionable implements Acces
      * @since XXX
      */
     public void updateByXml(final InputStream source) throws IOException, ServletException {
-        checkPermission(Jenkins.ADMINISTER);
+        checkPermission(CONFIGURE);
         Node result = (Node)Jenkins.XSTREAM2.fromXML(source);
         replaceBy(result);
     }
@@ -1352,6 +1353,7 @@ public /*transient*/ abstract class Computer extends Actionable implements Acces
     /**
      * Permission to configure slaves.
      */
+    public static final Permission READ = new Permission(PERMISSIONS,"Read", Messages._Computer_ReadPermission_Description(), Permission.READ, PermissionScope.COMPUTER);
     public static final Permission CONFIGURE = new Permission(PERMISSIONS,"Configure", Messages._Computer_ConfigurePermission_Description(), Permission.CONFIGURE, PermissionScope.COMPUTER);
     public static final Permission DELETE = new Permission(PERMISSIONS,"Delete", Messages._Computer_DeletePermission_Description(), Permission.DELETE, PermissionScope.COMPUTER);
     public static final Permission CREATE = new Permission(PERMISSIONS,"Create", Messages._Computer_CreatePermission_Description(), Permission.CREATE, PermissionScope.COMPUTER);

core/src/main/resources/hudson/model/Messages.properties

@@ -102,6 +102,7 @@ CLI.wait-node-offline.shortDescription=Wait for a node to become offline
 Computer.Caption=Slave {0}
 Computer.NoSuchSlaveExists=No such slave "{0}" exists. Did you mean "{1}"?
 Computer.Permissions.Title=Slave
+Computer.ReadPermission.Description=This permission allows users to read slave configuration.
 Computer.ConfigurePermission.Description=This permission allows users to configure slaves.
 Computer.DeletePermission.Description=This permission allows users to delete existing slaves.
 Computer.CreatePermission.Description=This permission allows users to create slaves.

test/src/test/java/hudson/cli/CreateNodeCommandTest.java

@@ -29,9 +29,9 @@ import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.text.IsEmptyString.isEmptyString;
+import hudson.model.Computer;
 import hudson.model.Node;
 import hudson.model.Slave;
-import hudson.security.Permission;
 import jenkins.model.Jenkins;
 
 import org.junit.Before;
@@ -50,15 +50,15 @@ public class CreateNodeCommandTest {
         command = new CLICommandInvoker(j, new CreateNodeCommand());
     }
 
-    @Test public void createNodeShouldFailWithoutAdministerPermision() throws Exception {
+    @Test public void createNodeShouldFailWithoutComputerCreatePermission() throws Exception {
 
         final CLICommandInvoker.Result result = command
-                .authorizedTo(Permission.READ)
-                .withStdin(getClass().getResourceAsStream("node.xml"))
+                .authorizedTo(Jenkins.READ)
+                .withStdin(Computer.class.getResourceAsStream("node.xml"))
                 .invoke()
         ;
 
-        assertThat(result.stderr(), containsString("user is missing the Overall/Administer permission"));
+        assertThat(result.stderr(), containsString("user is missing the Slave/Create permission"));
         assertThat("No output expected", result.stdout(), isEmptyString());
         assertThat("Command is expected to fail", result.returnCode(), equalTo(-1));
     }
@@ -66,8 +66,8 @@ public class CreateNodeCommandTest {
     @Test public void createNode() throws Exception {
 
         final CLICommandInvoker.Result result = command
-                .authorizedTo(Jenkins.ADMINISTER)
-                .withStdin(getClass().getResourceAsStream("node.xml"))
+                .authorizedTo(Computer.CREATE, Jenkins.READ)
+                .withStdin(Computer.class.getResourceAsStream("node.xml"))
                 .invoke()
         ;
 
@@ -83,8 +83,8 @@ public class CreateNodeCommandTest {
     @Test public void createNodeSpecifyingNameExplicitly() throws Exception {
 
         final CLICommandInvoker.Result result = command
-                .authorizedTo(Jenkins.ADMINISTER)
-                .withStdin(getClass().getResourceAsStream("node.xml"))
+                .authorizedTo(Computer.CREATE, Jenkins.READ)
+                .withStdin(Computer.class.getResourceAsStream("node.xml"))
                 .invokeWithArgs("CustomSlaveName")
         ;
 
@@ -104,8 +104,8 @@ public class CreateNodeCommandTest {
         final Node originalSlave = j.createSlave("SlaveFromXml", null, null);
 
         final CLICommandInvoker.Result result = command
-                .authorizedTo(Jenkins.ADMINISTER)
-                .withStdin(getClass().getResourceAsStream("node.xml"))
+                .authorizedTo(Computer.CREATE, Jenkins.READ)
+                .withStdin(Computer.class.getResourceAsStream("node.xml"))
                 .invokeWithArgs("CustomSlaveName")
         ;
 
@@ -125,8 +125,8 @@ public class CreateNodeCommandTest {
         j.createSlave("SlaveFromXML", null, null);
 
         final CLICommandInvoker.Result result = command
-                .authorizedTo(Jenkins.ADMINISTER)
-                .withStdin(getClass().getResourceAsStream("node.xml"))
+                .authorizedTo(Computer.CREATE, Jenkins.READ)
+                .withStdin(Computer.class.getResourceAsStream("node.xml"))
                 .invoke()
         ;
 
@@ -140,8 +140,8 @@ public class CreateNodeCommandTest {
         j.createSlave("ExistingSlave", null, null);
 
         final CLICommandInvoker.Result result = command
-                .authorizedTo(Jenkins.ADMINISTER)
-                .withStdin(getClass().getResourceAsStream("node.xml"))
+                .authorizedTo(Computer.CREATE, Jenkins.READ)
+                .withStdin(Computer.class.getResourceAsStream("node.xml"))
                 .invokeWithArgs("ExistingSlave")
         ;
 

test/src/test/java/hudson/cli/GetNodeCommandTest.java

@@ -29,7 +29,7 @@ import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.startsWith;
 import static org.hamcrest.text.IsEmptyString.isEmptyString;
-import hudson.security.Permission;
+import hudson.model.Computer;
 import jenkins.model.Jenkins;
 
 import org.junit.Before;
@@ -48,16 +48,16 @@ public class GetNodeCommandTest {
         command = new CLICommandInvoker(j, new GetNodeCommand());
     }
 
-    @Test public void getNodeShouldFailWithoutAdministerPermision() throws Exception {
+    @Test public void getNodeShouldFailWithoutComputerReadPermission() throws Exception {
 
         j.createSlave("MySlave", null, null);
 
         final CLICommandInvoker.Result result = command
-                .authorizedTo(Permission.READ)
+                .authorizedTo(Jenkins.READ)
                 .invokeWithArgs("MySlave")
         ;
 
-        assertThat(result.stderr(), containsString("user is missing the Overall/Administer permission"));
+        assertThat(result.stderr(), containsString("user is missing the Slave/Read permission"));
         assertThat("No output expected", result.stdout(), isEmptyString());
         assertThat("Command is expected to fail", result.returnCode(), equalTo(-1));
     }
@@ -67,7 +67,7 @@ public class GetNodeCommandTest {
         j.createSlave("MySlave", null, null);
 
         final CLICommandInvoker.Result result = command
-                .authorizedTo(Jenkins.ADMINISTER)
+                .authorizedTo(Computer.READ, Jenkins.READ)
                 .invokeWithArgs("MySlave")
         ;
 
@@ -80,7 +80,7 @@ public class GetNodeCommandTest {
     @Test public void getNodeShouldFailIfNodeDoesNotExist() throws Exception {
 
         final CLICommandInvoker.Result result = command
-                .authorizedTo(Jenkins.ADMINISTER)
+                .authorizedTo(Computer.READ, Jenkins.READ)
                 .invokeWithArgs("MySlave")
         ;
 

test/src/test/java/hudson/cli/UpdateNodeCommandTest.java

@@ -29,8 +29,8 @@ import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.text.IsEmptyString.isEmptyString;
+import hudson.model.Computer;
 import hudson.model.Node;
-import hudson.security.Permission;
 import jenkins.model.Jenkins;
 
 import org.junit.Before;
@@ -49,16 +49,16 @@ public class UpdateNodeCommandTest {
         command = new CLICommandInvoker(j, new UpdateNodeCommand());
     }
 
-    @Test public void updateNodeShouldFailWithoutAdministerPermision() throws Exception {
+    @Test public void updateNodeShouldFailWithoutComputerConfigurePermission() throws Exception {
 
         j.createSlave("MySlave", null, null);
 
         final CLICommandInvoker.Result result = command
-                .authorizedTo(Permission.READ)
+                .authorizedTo(Jenkins.READ)
                 .invokeWithArgs("MySlave")
         ;
 
-        assertThat(result.stderr(), containsString("user is missing the Overall/Administer permission"));
+        assertThat(result.stderr(), containsString("user is missing the Slave/Configure permission"));
         assertThat("No output expected", result.stdout(), isEmptyString());
         assertThat("Command is expected to fail", result.returnCode(), equalTo(-1));
     }
@@ -68,8 +68,8 @@ public class UpdateNodeCommandTest {
         j.createSlave("MySlave", null, null);
 
         final CLICommandInvoker.Result result = command
-                .authorizedTo(Jenkins.ADMINISTER)
-                .withStdin(getClass().getResourceAsStream("node.xml"))
+                .authorizedTo(Computer.CONFIGURE, Jenkins.READ)
+                .withStdin(Computer.class.getResourceAsStream("node.xml"))
                 .invokeWithArgs("MySlave")
         ;
 
@@ -86,8 +86,8 @@ public class UpdateNodeCommandTest {
     @Test public void updateNodeShouldFailIfNodeDoesNotExist() throws Exception {
 
         final CLICommandInvoker.Result result = command
-                .authorizedTo(Jenkins.ADMINISTER)
-                .withStdin(getClass().getResourceAsStream("node.xml"))
+                .authorizedTo(Computer.CONFIGURE, Jenkins.READ)
+                .withStdin(Computer.class.getResourceAsStream("node.xml"))
                 .invokeWithArgs("MySlave")
         ;
 

test/src/test/java/hudson/model/ComputerConfigDotXmlTest.java

+/*
+ * The MIT License
+ *
+ * Copyright (c) 2013 Red Hat, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+package hudson.model;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.startsWith;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
+import hudson.security.ACL;
+import hudson.security.AccessDeniedException2;
+import hudson.security.GlobalMatrixAuthorizationStrategy;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+
+import javax.servlet.ServletInputStream;
+import javax.servlet.ServletOutputStream;
+
+import org.acegisecurity.context.SecurityContext;
+import org.acegisecurity.context.SecurityContextHolder;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.kohsuke.stapler.StaplerRequest;
+import org.kohsuke.stapler.StaplerResponse;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+/**
+ * @author ogondza
+ */
+public class ComputerConfigDotXmlTest {
+
+    @Rule public final JenkinsRule rule = new JenkinsRule();
+
+    @Mock private StaplerRequest req;
+    @Mock private StaplerResponse rsp;
+
+    private Computer computer;
+    private SecurityContext oldSecurityContext;
+
+    @Before
+    public void setUp() throws Exception {
+
+        MockitoAnnotations.initMocks(this);
+        computer = spy(rule.createSlave().toComputer());
+        oldSecurityContext = ACL.impersonate(User.get("user").impersonate());
+    }
+
+    @After
+    public void tearDown() {
+
+        SecurityContextHolder.setContext(oldSecurityContext);
+    }
+
+    @Test(expected = AccessDeniedException2.class)
+    public void configXmlGetShouldFailForUnauthorized() throws Exception {
+
+        when(req.getMethod()).thenReturn("GET");
+
+        rule.jenkins.setAuthorizationStrategy(new GlobalMatrixAuthorizationStrategy());
+
+        computer.doConfigDotXml(req, rsp);
+    }
+
+    @Test(expected = AccessDeniedException2.class)
+    public void configXmlPostShouldFailForUnauthorized() throws Exception {
+
+        when(req.getMethod()).thenReturn("POST");
+
+        rule.jenkins.setAuthorizationStrategy(new GlobalMatrixAuthorizationStrategy());
+
+        computer.doConfigDotXml(req, rsp);
+    }
+
+    @Test
+    public void configXmlGetShouldYieldNodeConfiguration() throws Exception {
+
+        when(req.getMethod()).thenReturn("GET");
+
+        GlobalMatrixAuthorizationStrategy auth = new GlobalMatrixAuthorizationStrategy();
+        rule.jenkins.setAuthorizationStrategy(auth);
+        auth.add(Computer.READ, "user");
+
+        final OutputStream outputStream = captureOutput();
+
+        computer.doConfigDotXml(req, rsp);
+
+        final String out = outputStream.toString();
+        assertThat(out, startsWith("<?xml version=\"1.0\" encoding=\"UTF-8\"?>"));
+        assertThat(out, containsString("<name>slave0</name>"));
+        assertThat(out, containsString("<description>dummy</description>"));
+    }
+
+    @Test
+    public void configXmlPostShouldUpdateNodeConfiguration() throws Exception {
+
+        when(req.getMethod()).thenReturn("POST");
+
+        GlobalMatrixAuthorizationStrategy auth = new GlobalMatrixAuthorizationStrategy();
+        rule.jenkins.setAuthorizationStrategy(auth);
+        auth.add(Computer.CONFIGURE, "user");
+
+        when(req.getInputStream()).thenReturn(xmlNode("node.xml"));
+
+        computer.doConfigDotXml(req, rsp);
+
+        final Node updatedSlave = rule.jenkins.getNode("SlaveFromXML");
+        assertThat(updatedSlave.getNodeName(), equalTo("SlaveFromXML"));
+        assertThat(updatedSlave.getNumExecutors(), equalTo(42));
+    }
+
+    private OutputStream captureOutput() throws IOException {
+
+        final ByteArrayOutputStream baos = new ByteArrayOutputStream();
+
+        when(rsp.getOutputStream()).thenReturn(new ServletOutputStream() {
+
+            @Override
+            public void write(int b) throws IOException {
+
+                baos.write(b);
+            }
+        });
+
+        return baos;
+    }
+
+    private ServletInputStream xmlNode(final String name) {
+
+        class Stream extends ServletInputStream {
+
+            private final InputStream inner;
+
+            public Stream(final InputStream inner) {
+
+                this.inner = inner;
+            }
+
+            @Override
+            public int read() throws IOException {
+
+                return inner.read();
+            }
+        }
+
+        return new Stream(Computer.class.getResourceAsStream(name));
+    }
+}