Revert adding slave-side support for sudo.

Adding sudo to a command can easily be handled by on the master-side. Also,
different people will have different expectations, which a single argument
isn't enough to capture.

See fabric/fabric#503 for some discussions.

This reverts commit b5cbb73, reversing
changes made to d0cd62b.

master/buildbot/process/buildstep.py

@@ -30,7 +30,6 @@
      EXCEPTION, RETRY, worst_status
 from buildbot.process import metrics, properties
 from buildbot.util.eventual import eventually
-from buildbot.interfaces import BuildSlaveTooOldError
 
 class BuildStepFailed(Exception):
     pass
@@ -362,8 +361,7 @@ def __init__(self, workdir, command, env=None,
                  usePTY="slave-config", logEnviron=True,
                  collectStdout=False,collectStderr=False,
                  interruptSignal=None,
-                 initialStdin=None, decodeRC={0:SUCCESS},
-                 user=None):
+                 initialStdin=None, decodeRC={0:SUCCESS}):
 
         self.command = command # stash .command, set it later
         if env is not None:
@@ -384,8 +382,6 @@ def __init__(self, workdir, command, env=None,
                 }
         if interruptSignal is not None:
             args['interruptSignal'] = interruptSignal
-        if user is not None:
-            args['user'] = user
         RemoteCommand.__init__(self, "shell", args, collectStdout=collectStdout,
                                collectStderr=collectStderr,
                                decodeRC=decodeRC)
@@ -397,10 +393,6 @@ def _start(self):
             # fixup themselves
             if self.step.slaveVersion("shell", "old") == "old":
                 self.args['dir'] = self.args['workdir']
-        if ('user' in self.args and
-                self.step.slaveVersionIsOlderThan("shell", "2.16")):
-            m = "slave does not support the 'user' parameter"
-            raise BuildSlaveTooOldError(m)
         what = "command '%s' in dir '%s'" % (self.args['command'],
                                              self.args['workdir'])
         log.msg(what)

master/buildbot/test/fake/remotecommand.py

@@ -81,15 +81,12 @@ def __init__(self, workdir, command, env=None,
                  timeout=20*60, maxTime=None, logfiles={},
                  usePTY="slave-config", logEnviron=True, collectStdout=False,
                  collectStderr=False,
-                 interruptSignal=None, initialStdin=None, decodeRC={0:SUCCESS},
-                 user=None):
+                 interruptSignal=None, initialStdin=None, decodeRC={0:SUCCESS}):
         args = dict(workdir=workdir, command=command, env=env or {},
                 want_stdout=want_stdout, want_stderr=want_stderr,
                 initial_stdin=initialStdin,
                 timeout=timeout, maxTime=maxTime, logfiles=logfiles,
                 usePTY=usePTY, logEnviron=logEnviron)
-        if user is not None:
-            args['user'] = user
         FakeRemoteCommand.__init__(self, "shell", args,
                                    collectStdout=collectStdout,
                                    collectStderr=collectStderr,
@@ -286,8 +283,7 @@ class ExpectShell(Expect):
     def __init__(self, workdir, command, env={},
                  want_stdout=1, want_stderr=1, initialStdin=None,
                  timeout=20*60, maxTime=None, logfiles={},
-                 usePTY="slave-config", logEnviron=True,
-                 user=None):
+                 usePTY="slave-config", logEnviron=True):
         args = dict(workdir=workdir, command=command, env=env,
                 want_stdout=want_stdout, want_stderr=want_stderr,
                 initial_stdin=initialStdin,

master/buildbot/test/interfaces/test_remotecommand.py

@@ -44,7 +44,7 @@ def __init__(self, workdir, command, env=None, want_stdout=1,
                 want_stderr=1, timeout=20*60, maxTime=None, logfiles={},
                 usePTY="slave-config", logEnviron=True, collectStdout=False,
                 collectStderr=False, interruptSignal=None, initialStdin=None,
-                decodeRC={0:SUCCESS}, user=None):
+                decodeRC={0:SUCCESS}):
             pass
 
     def test_signature_run(self):

master/buildbot/test/unit/test_process_buildstep.py

@@ -270,36 +270,3 @@ def cb(_):
             self.assertEqual(len(self.flushLoggedErrors(ValueError)), 1)
         return d
 
-
-class RemoteShellCommandTests(object):
-
-    def test_user_argument(self):
-        """
-        Test that the 'user' parameter is correctly threaded through
-        RemoteShellCommand to the 'args' member of the RemoteCommand
-        parent command, if and only if it is passed in as a non-None
-        value.
-        """
-
-        rc = self.makeRemoteShellCommand("build", ["echo", "hello"])
-        self.assertNotIn('user', rc.args)
-
-        rc = self.makeRemoteShellCommand("build", ["echo", "hello"], user=None)
-        self.assertNotIn('user', rc.args)
-
-        user = 'test'
-        rc = self.makeRemoteShellCommand("build", ["echo", "hello"], user=user)
-        self.assertIn('user', rc.args)
-        self.assertEqual(rc.args['user'], user)
-
-
-class TestRealRemoteShellCommand(unittest.TestCase, RemoteShellCommandTests):
-
-    def makeRemoteShellCommand(self, *args, **kwargs):
-        return buildstep.RemoteShellCommand(*args, **kwargs)
-
-
-class TestFakeRemoteShellCommand(unittest.TestCase, RemoteShellCommandTests):
-
-    def makeRemoteShellCommand(self, *args, **kwargs):
-        return remotecommand.FakeRemoteShellCommand(*args, **kwargs)

master/docs/manual/cfg-buildsteps.rst

@@ -1733,12 +1733,6 @@ The :bb:step:`ShellCommand` arguments are:
     The default is to treat just 0 as successful. (``{0:SUCCESS}``)
     any exit code not present in the dictionary will be treated as ``FAILURE``
 
-``user``
-    When this is not None, runs the command as the given user by wrapping the
-    command with 'sudo', which typically requires root permissions to run
-    (and as discussed in the :ref:`System Architecture <System-Architecture>`
-    section, is generally not a good idea).
-
 .. bb:step:: Configure
 
 Configure

master/docs/manual/introduction.rst

@@ -112,15 +112,6 @@ admin*, who do not need to be quite as involved. Generally slaves are
 run by anyone who has an interest in seeing the project work well on
 their favorite platform.
 
-While not always the case, it is typically a good idea to run the
-buildslaves with reduced privileges (e.g. not as the 'root' user).
-This protects the system installed on the machine the buildslave
-is running on from the commands that are running on behalf of the
-buildmaster. There are some cases where running buildslaves with
-admin privileges is appropriate, one example of which is doing so
-on latent slaves that are reverted to a snapshot on each startup
-and contain no sensitive information.
-
 .. _BuildSlave-Connections:
 
 BuildSlave Connections

master/docs/relnotes/index.rst

@@ -53,8 +53,8 @@ Features
   only a single property and therefore allows commas to be included in the property
   name and value.
 
-* The ``Git`` step has a new ``config`` option, which accepts a dict of git configuration options to pass to the low-level git commands.
-  See :bb:step:`Git` for details.
+* The ``Git`` step has a new ``config`` option, which accepts a dict of git configuration options to
+  pass to the low-level git commands.  See :bb:step:`Git` for details.
 
 * The ``TryScheduler`` now accepts an additional ``properties`` argument to its
   ``getAvailableBuilderNames`` method, which 'buildbot try' uses to send the properties
@@ -64,9 +64,6 @@ Features
 
 * The list of force schedulers in the web UI is now sorted by name.
 
-* The :bb:step:`ShellCommand` step has a new parameter ``user``.
-  When this is set, the slave will use 'sudo' to run the command as the given user.
-
 * OpenStack-based Latent Buildslave support was added.
   See :bb:pull:`666`.
 

slave/buildslave/commands/base.py

@@ -32,7 +32,7 @@
 # this used to be a CVS $-style "Revision" auto-updated keyword, but since I
 # moved to Darcs as the primary repository, this is updated manually each
 # time this file is changed. The last cvs_ver that was here was 1.51 .
-command_version = "2.16"
+command_version = "2.15"
 
 # version history:
 #  >=1.17: commands are interruptable
@@ -64,7 +64,6 @@
 #  >= 2.13: SlaveFileUploadCommand supports option 'keepstamp'
 #  >= 2.14: RemoveDirectory can delete multiple directories
 #  >= 2.15: 'interruptSignal' option is added to SlaveShellCommand
-#  >= 2.16: 'user' option is added to SlaveShellCommand
 
 class Command:
     implements(ISlaveCommand)

slave/buildslave/commands/shell.py

@@ -39,7 +39,6 @@ def start(self):
                          logfiles=args.get('logfiles', {}),
                          usePTY=args.get('usePTY', "slave-config"),
                          logEnviron=args.get('logEnviron', True),
-                         user=args.get('user'),
                          )
         if args.get('interruptSignal'):
             c.interruptSignal = args['interruptSignal']

slave/buildslave/runprocess.py

@@ -237,7 +237,7 @@ def __init__(self, builder, command,
                  timeout=None, maxTime=None, initialStdin=None,
                  keepStdout=False, keepStderr=False,
                  logEnviron=True, logfiles={}, usePTY="slave-config",
-                 useProcGroup=True, user=None):
+                 useProcGroup=True):
         """
 
         @param keepStdout: if True, we keep a copy of all the stdout text
@@ -368,11 +368,6 @@ def subst(match):
                                follow=follow)
             self.logFileWatchers.append(w)
 
-        if user is not None and runtime.platformType != 'posix':
-            raise RuntimeError(
-                "Cannot use 'user' parameter on this platform")
-        self.user = user
-
     def __repr__(self):
         return "<%s '%s'>" % (self.__class__.__name__, self.fake_command)
 
@@ -442,11 +437,6 @@ def _startCommand(self):
             # Attempt to format this for use by a shell, although the process isn't perfect
             display = shell_quote(self.fake_command)
 
-        # If requested, wrap the call in 'sudo' to run the command as a
-        # different user.
-        if self.user is not None:
-            argv = ['sudo', '-u', self.user, '-H'] + argv
-
         # $PWD usually indicates the current directory; spawnProcess may not
         # update this value, though, so we set it explicitly here.  This causes
         # weird problems (bug #456) on msys, though..

slave/buildslave/test/fake/runprocess.py

@@ -103,8 +103,7 @@ def __init__(self, builder, command, workdir, **kwargs):
                  sendStdout=True, sendStderr=True, sendRC=True,
                  timeout=None, maxTime=None, initialStdin=None,
                  keepStdout=False, keepStderr=False,
-                 logEnviron=True, logfiles={}, usePTY="slave-config",
-                 user=None)
+                 logEnviron=True, logfiles={}, usePTY="slave-config")
 
         if not self._expectations:
             raise AssertionError("unexpected instantiation: %s" % (kwargs,))
@@ -133,7 +132,6 @@ def __init__(self, builder, command, workdir, **kwargs):
         self._builder = builder
         self.stdout = ''
         self.stderr = ''
-        self.user = kwargs.get('user')
 
     def start(self):
         # figure out the stdio-related parameters

slave/buildslave/test/unit/test_commands_shell.py

@@ -49,33 +49,4 @@ def check(_):
         d.addCallback(check)
         return d
 
-    def test_user_parameter(self):
-        """
-        Test that the 'user' parameter is threaded through into the
-        underlying RunProcess object.
-        """
-
-        user = 'test'
-        self.make_command(shell.SlaveShellCommand, dict(
-            command=[ 'true' ],
-            workdir='workdir',
-            user=user,
-        ))
-
-        self.patch_runprocess(
-            Expect([ 'true', ], self.basedir_workdir, user=user)
-            + { 'hdr' : 'headers' } + { 'rc' : 0 }
-            + 0,
-        )
-
-        d = self.run_command()
-
-        # Verify that the 'user' parameter is correctly passed into the
-        # RunProcess object.
-        def check(_):
-            self.assertTrue(hasattr(self.cmd.command, 'user'))
-            self.assertEqual(self.cmd.command.user, user)
-        d.addCallback(check)
-        return d
-
     # TODO: test all functionality that SlaveShellCommand adds atop RunProcess

slave/buildslave/test/unit/test_runprocess.py

@@ -404,29 +404,6 @@ def testEnvironInt(self):
             runprocess.RunProcess(b, stdoutCommand('hello'), self.basedir,
                             environ={"BUILD_NUMBER":13}))
 
-    @compat.skipUnlessPlatformIs("posix")
-    def testUserParameter(self):
-        """
-        Test that setting the 'user' parameter causes RunProcess to
-        wrap the command using 'sudo'.
-        """
-
-        user = 'test'
-        cmd = ['whatever']
-        b = FakeSlaveBuilder(False, self.basedir)
-        s = runprocess.RunProcess(b, cmd, self.basedir, user=user)
-
-        # Override the '_spawnProcess' method so that we can verify
-        # that the command is run using 'sudo', as we expect.
-        def _spawnProcess(*args, **kwargs):
-            executable = args[1]
-            args = args[2]
-            self.assertEqual(executable, 'sudo')
-            self.assertEqual(args, ['sudo', '-u', user, '-H'] + cmd)
-        s._spawnProcess = _spawnProcess
-        s.start()
-        return s.finished(None, 0)
-
 
 class TestPOSIXKilling(BasedirMixin, unittest.TestCase):