Merge lp:~nataliabidart/ubuntuone-client/stable-3-0-update-2.99.91.1 into lp:ubuntuone-client/stable-3-0

[Natalia Bidart]

[ Alejandro J. Cura <email address hidden> ]
  - reactor.spawnProcess throws an error on windows if it can't find
    the binary to run (LP: #965897).
  - Stop the proxy tunnel when stopping the reactor (LP: #963485).
  - Don't write the proxy credentials to the log (LP: #963568).

[ Brian Curtin <email address hidden> ]
  - Loosen the is_root check on Windows to allow Windows XP users,
    who commonly run with Administrator rights, to start U1 properly
    (LP: #930398).

[Roberto Alsina (ralsina)]

+1

review: Approve

[Manuel de la Peña (mandel)]

review: Approve

=== modified file 'tests/platform/windows/test_os_helper.py'
--- tests/platform/windows/test_os_helper.py	2012-01-10 18:09:30 +0000
+++ tests/platform/windows/test_os_helper.py	2012-03-27 15:49:21 +0000
@@ -20,6 +20,7 @@
 import os
 import shutil
 import sys
+import platform
 
 from twisted.internet import defer
 from twisted.trial.unittest import TestCase
@@ -478,3 +479,10 @@
         self.patch(os_helper.shell, 'IsUserAnAdmin', lambda: expected)
         actual = os_helper.is_root()
         self.assertEqual(expected, actual)
+
+    def test_is_root_on_xp(self):
+        """Test that os_helper.is_root always returns False on XP"""
+        expected = False
+        self.patch(platform, "version", "5.1.2600")
+        actual = os_helper.is_root()
+        self.assertEqual(expected, actual)
 
=== modified file 'tests/proxy/test_tunnel_server.py'
--- tests/proxy/test_tunnel_server.py	2012-03-16 13:40:59 +0000
+++ tests/proxy/test_tunnel_server.py	2012-03-27 15:49:21 +0000
@@ -51,6 +51,15 @@
     }
 }
 
+FAKE_AUTH_SETTINGS = {
+    "http": {
+        "host": "myhost",
+        "port": 8888,
+        "username": "fake_user",
+        "password": "fake_password",
+    }
+}
+
 SAMPLE_HOST = "samplehost.com"
 SAMPLE_PORT = 443
 
@@ -296,6 +305,27 @@
                    lambda _, domain: defer.succeed(FAKE_CREDS))
         yield self.proto.headers_done()
 
+    def test_creds_are_not_logged(self):
+        """The proxy credentials are not logged."""
+        log = []
+        self.patch(tunnel_server.logger, "info",
+                   lambda text, *args: log.append(text % args))
+        proxy = tunnel_server.build_proxy(FAKE_AUTH_SETTINGS)
+        authenticator = QAuthenticator()
+        username = FAKE_AUTH_SETTINGS["http"]["username"]
+        password = FAKE_AUTH_SETTINGS["http"]["password"]
+        self.proto.proxy_credentials = {
+            "username": username,
+            "password": password,
+        }
+        self.proto.proxy_domain = proxy.hostName()
+
+        self.proto.proxy_auth_required(proxy, authenticator)
+
+        for line in log:
+            self.assertNotIn(username, line)
+            self.assertNotIn(password, line)
+
 
 class FakeServerTunnelProtocol(object):
     """A fake ServerTunnelProtocol."""
 
=== modified file 'tests/syncdaemon/test_tunnel_runner.py'
--- tests/syncdaemon/test_tunnel_runner.py	2012-03-19 03:31:10 +0000
+++ tests/syncdaemon/test_tunnel_runner.py	2012-03-27 15:49:21 +0000
@@ -47,6 +47,28 @@
         client = yield tr.get_client()
         self.assertEqual(client, reactor)
 
+    @defer.inlineCallbacks
+    def test_executable_not_found(self):
+        """The executable is not found anywhere."""
+        self.patch(tunnel_runner, "TUNNEL_EXECUTABLE", "this_does_not_exist")
+        tr = tunnel_runner.TunnelRunner(FAKE_HOST, FAKE_PORT)
+        client = yield tr.get_client()
+        self.assertEqual(client, reactor)
+
+
+class FakeProcessTransport(object):
+    """A fake ProcessTransport."""
+
+    pid = 0
+
+    def __init__(self):
+        """Initialize this fake."""
+        self._signals_sent = []
+
+    def signalProcess(self, signalID):
+        """Send a signal to the process."""
+        self._signals_sent.append(signalID)
+
 
 class TunnelRunnerTestCase(TestCase):
     """Tests for the TunnelRunner."""
@@ -58,8 +80,22 @@
         """Initialize this testcase."""
         yield super(TunnelRunnerTestCase, self).setUp()
         self.spawned = []
-        self.patch(tunnel_client.reactor, "spawnProcess",
-                   lambda *args, **kwargs: self.spawned.append((args, kwargs)))
+        self.triggers = []
+        self.fake_process_transport = FakeProcessTransport()
+
+        def fake_spawn_process(*args, **kwargs):
+            """A fake spawnProcess."""
+            self.spawned.append((args, kwargs))
+            return self.fake_process_transport
+
+        self.patch(tunnel_client.reactor, "spawnProcess", fake_spawn_process)
+
+        def fake_add_system_event_trigger(*args, **kwargs):
+            """A fake addSystemEventTrigger."""
+            self.triggers.append((args, kwargs))
+
+        self.patch(tunnel_client.reactor, "addSystemEventTrigger",
+                   fake_add_system_event_trigger)
         self.process_protocol = None
         self.process_protocol_class = tunnel_client.TunnelProcessProtocol
         self.patch(tunnel_client, "TunnelProcessProtocol",
@@ -76,6 +112,23 @@
         self.assertEqual(len(self.spawned), 1,
                         "The tunnel process is started.")
 
+    def test_system_event_finished(self):
+        """An event is added to stop the process with the reactor."""
+        expected = [(("before", "shutdown", self.tr.stop), {})]
+        self.assertEqual(self.triggers, expected)
+
+    def test_stop_process(self):
+        """The process is stopped if still running."""
+        self.tr.process_transport.pid = 1234
+        self.tr.stop()
+        self.assertEqual(self.fake_process_transport._signals_sent, ["KILL"])
+
+    def test_not_stopped_if_already_finished(self):
+        """Do not stop the tunnel process if it's already finished."""
+        self.tr.process_transport.pid = None
+        self.tr.stop()
+        self.assertEqual(self.fake_process_transport._signals_sent, [])
+
     @defer.inlineCallbacks
     def test_tunnel_process_get_client_yielded_twice(self):
         """The get_client method can be yielded twice."""
 
=== modified file 'ubuntuone/platform/windows/os_helper.py'
--- ubuntuone/platform/windows/os_helper.py	2012-01-10 18:09:30 +0000
+++ ubuntuone/platform/windows/os_helper.py	2012-03-27 15:49:21 +0000
@@ -23,6 +23,7 @@
 import stat
 import sys
 
+from platform import version
 from contextlib import contextmanager
 from functools import wraps
 
@@ -872,10 +873,13 @@
 
 def is_root():
     """Return if the user is running as root."""
-    # This solution works on Windows XP, Vista and 7
+    # On Windows XP (v5.1), always return False. Nearly all users run with
+    # Administrator access, so this would restrict too many users.
+    # On Vista (v6.0) and 7 (v6.1), return the real Administrator value.
     # The MSDN docs say this may go away in a later version, but look at what
     # the alternative is: http://bit.ly/rXbIwc
-    return shell.IsUserAnAdmin()
+    is_xp = version()[0] == "5"
+    return False if is_xp else shell.IsUserAnAdmin()
 
 
 @windowspath()
 
=== modified file 'ubuntuone/proxy/tunnel_server.py'
--- ubuntuone/proxy/tunnel_server.py	2012-03-16 13:40:59 +0000
+++ ubuntuone/proxy/tunnel_server.py	2012-03-27 15:49:21 +0000
@@ -201,11 +201,10 @@
 
     def proxy_auth_required(self, proxy, authenticator):
         """Proxy authentication is required."""
-        logger.info("auth_required %r, %r, %r", self.proxy_credentials,
+        logger.info("auth_required %r, %r",
                     proxy.hostName(), self.proxy_domain)
         if self.proxy_credentials and proxy.hostName() == self.proxy_domain:
-            logger.info("Credentials added to authenticator: %r.",
-                        self.proxy_credentials)
+            logger.info("Credentials added to authenticator.")
             authenticator.setUser(self.proxy_credentials["username"])
             authenticator.setPassword(self.proxy_credentials["password"])
         else:
 
=== modified file 'ubuntuone/syncdaemon/tunnel_runner.py'
--- ubuntuone/syncdaemon/tunnel_runner.py	2012-03-19 13:20:36 +0000
+++ ubuntuone/syncdaemon/tunnel_runner.py	2012-03-27 15:49:21 +0000
@@ -33,11 +33,15 @@
     def __init__(self, host, port):
         """Start this runner instance."""
         self.client_d = defer.Deferred()
+        self.process_transport = None
         try:
             self.start_process(host, port)
         except ImportError:
             logger.info("Proxy support not installed.")
             self.client_d.callback(reactor)
+        except Exception:
+            logger.exception("Error while starting tunnel process:")
+            self.client_d.callback(reactor)
 
     def start_process(self, host, port):
         """Start the tunnel process."""
@@ -45,7 +49,15 @@
         protocol = TunnelProcessProtocol(self.client_d)
         process_path = self.get_process_path()
         args = [TUNNEL_EXECUTABLE, host, str(port)]
-        reactor.spawnProcess(protocol, process_path, env=None, args=args)
+        self.process_transport = reactor.spawnProcess(protocol, process_path,
+                                                      env=None, args=args)
+        reactor.addSystemEventTrigger("before", "shutdown", self.stop)
+
+    def stop(self):
+        """Stop the tunnel process if still running."""
+        logger.info("Stopping process %r", self.process_transport.pid)
+        if self.process_transport.pid is not None:
+            self.process_transport.signalProcess("KILL")
 
     def get_process_path(self):
         """Get the path to the tunnel process."""