Merge lp:~alecu/ubuntuone-client/proxy-tunnel-process into lp:ubuntuone-client

[Alejandro J. Cura]

- A proxy tunnel process to be started when proxies are enabled (LP: #929207).

- A proxy tunnel process to be started when proxies are enabled. (LP: #929207)

[Manuel de la Peña (mandel)]

A very stupid comments but why doing:

if len(proxies) and proxies[0].type() != QNetworkProxy.NoProxy:
    return True

When you an do:

return len(proxies) and proxies[0].type() != QNetworkProxy.NoProxy

same for:

if settings:
    return True

In IsProxyEnabledTestCase I think you can merge the tests cases, for example:

             def _assert_enabled(self, platform):
                 """Assert that the proxy is enabled."""
                 self.patch(tunnel_server.sys, "platform", platform)
                 ret = tunnel_server.is_proxy_enabled(SAMPLE_HOST, str(SAMPLE_PORT))
                 self.assertTrue(ret, "Proxy is enabled.")

             def test_platform_linux_enabled(self):
                 """Tests for the linux platform with proxies enabled."""
                 self.patch(tunnel_server.gsettings, "get_proxy_settings",
                            lambda: FAKE_SETTINGS)
                 self._assert_enabled('linux3')

             def test_platform_other_enabled(self):
                 """Tests for any other platform with proxies enabled."""
                 fake_netproxfact = FakeNetworkProxyFactoryClass(True)
                 self.patch(tunnel_server, "QNetworkProxyFactory", fake_netproxfact)
                 self._assert_enabled('windows 1.0')

Same for the disabled one.

I think it would be a good idea to use sys.exit in main specially in the case were there is no proxy.

review: Needs Fixing

[Alejandro J. Cura (alecu)]

> A very stupid comments but why doing:
>
> if len(proxies) and proxies[0].type() != QNetworkProxy.NoProxy:
> return True
>
> When you an do:
>
> return len(proxies) and proxies[0].type() != QNetworkProxy.NoProxy
>
> same for:
>
> if settings:
> return True
>
> In IsProxyEnabledTestCase I think you can merge the tests cases, for example:
>
> def _assert_enabled(self, platform):
> """Assert that the proxy is enabled."""
> self.patch(tunnel_server.sys, "platform", platform)
> ret = tunnel_server.is_proxy_enabled(SAMPLE_HOST,
> str(SAMPLE_PORT))
> self.assertTrue(ret, "Proxy is enabled.")
>
> def test_platform_linux_enabled(self):
> """Tests for the linux platform with proxies enabled."""
> self.patch(tunnel_server.gsettings, "get_proxy_settings",
> lambda: FAKE_SETTINGS)
> self._assert_enabled('linux3')
>
> def test_platform_other_enabled(self):
> """Tests for any other platform with proxies enabled."""
> fake_netproxfact = FakeNetworkProxyFactoryClass(True)
> self.patch(tunnel_server, "QNetworkProxyFactory",
> fake_netproxfact)
> self._assert_enabled('windows 1.0')
>
> Same for the disabled one.

Refactored all those tests as suggested.

> I think it would be a good idea to use sys.exit in main specially in the case
> were there is no proxy.

It's a normal condition for this process to exit if there's no proxy configured, so I think that it would be redundant to use sys.exit in this case.

Thanks for the review.

[Manuel de la Peña (mandel)]

Superb!

review: Approve

[Eric Casteleijn (thisfred)]

Looks great, two small observations that don't need any changes on this branch:

1. if we pass too many arguments to tunnel_server.py, we'll get strange errors, but since it doesn't look like anything users will ever call themselves, it's probably fine.

2. The new Python 3 style formatting is nice, but it means we need 2.7+ for this to work (which may already be a requirement,) but also, it's reportedly *way* less efficient than the old string formatting syntax, and there are no plans to ever remove that from the language, which makes me think we should probably just stick with the old one.

review: Approve

[Alejandro J. Cura (alecu)]

> 2. The new Python 3 style formatting is nice, but it means we need 2.7+ for
> this to work (which may already be a requirement,) but also, it's reportedly
> *way* less efficient than the old string formatting syntax, and there are no
> plans to ever remove that from the language, which makes me think we should
> probably just stick with the old one.

This is a very good point.
Since str.format() is not being used anywhere else in u1-client I'll change this in the following branch to use the % formatting operator.

Thanks for the review!

[Ubuntu One Auto Pilot (otto-pilot)]

Voting does not meet specified criteria. Required: Approve >= 1, Disapprove == 0, Needs Fixing == 0, Needs Information == 0, Resubmit == 0, Pending == 0. Got: 2 Approve, 1 Pending.

[Alejandro J. Cura (alecu)]

review: Abstain

=== modified file 'Makefile.am'
--- Makefile.am	2012-02-24 17:57:27 +0000
+++ Makefile.am	2012-03-09 16:20:31 +0000
@@ -19,6 +19,7 @@
 
 libexec_SCRIPTS =			\
 	bin/ubuntuone-syncdaemon	\
+	bin/ubuntuone-proxy-tunnel      \
 	bin/ubuntuone-login
 
 clientdefsdir = $(pythonpkgdir)/ubuntuone

=== added file 'bin/ubuntuone-proxy-tunnel'
--- bin/ubuntuone-proxy-tunnel	1970-01-01 00:00:00 +0000
+++ bin/ubuntuone-proxy-tunnel	2012-03-09 16:20:31 +0000
@@ -0,0 +1,24 @@
+#!/usr/bin/python
+#
+# Copyright 2012 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU General Public License version 3, as published
+# by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Ubuntu One tunnel for proxy support."""
+
+import sys
+
+from ubuntuone.proxy.tunnel_server import main
+
+if __name__ == "__main__":
+    main(sys.argv)

=== modified file 'tests/proxy/test_tunnel_server.py'
--- tests/proxy/test_tunnel_server.py	2012-03-09 14:36:34 +0000
+++ tests/proxy/test_tunnel_server.py	2012-03-09 16:20:31 +0000
@@ -15,10 +15,12 @@
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 """Tests for the proxy tunnel."""
 
+from StringIO import StringIO
 from urlparse import urlparse
 
 from twisted.internet import defer, protocol, reactor
-
+from twisted.trial.unittest import TestCase
+from PyQt4.QtCore import QCoreApplication
 from ubuntuone.devtools.testcases.squid import SquidTestCase
 
 from tests.proxy import (
@@ -39,6 +41,14 @@
     "GET {0.path} HTTP/1.0" + CRLF + CRLF
 )
 
+FAKE_SETTINGS = {
+    "host": "myhost",
+    "port": "8888",
+}
+
+SAMPLE_HOST = "samplehost.com"
+SAMPLE_PORT = 443
+
 
 class DisconnectingProtocol(protocol.Protocol):
     """A protocol that just disconnects."""
@@ -351,3 +361,124 @@
     """Tests for the client going thru an authenticated proxy."""
 
     get_proxy_settings = RemoteSocketTestCase.get_auth_proxy_settings
+
+
+class FakeNetworkProxyFactoryClass(object):
+    """A fake QNetworkProxyFactory."""
+    last_query = None
+
+    def __init__(self, enabled):
+        """Initialize this fake instance."""
+        if enabled:
+            self.proxy_type = tunnel_server.QNetworkProxy.HttpProxy
+        else:
+            self.proxy_type = tunnel_server.QNetworkProxy.NoProxy
+
+    def type(self):
+        """Return the proxy type configured."""
+        return self.proxy_type
+
+    def systemProxyForQuery(self, query):
+        """A list of proxies, but only type() will be called on the first."""
+        return [self]
+
+
+class IsProxyEnabledTestCase(TestCase):
+    """Tests for the is_proxy_enabled function."""
+
+    def _assert_proxy_state(self, platform, state, assertion):
+        """Assert the proxy is in a given state."""
+        self.patch(tunnel_server.sys, "platform", platform)
+        ret = tunnel_server.is_proxy_enabled(SAMPLE_HOST, str(SAMPLE_PORT))
+        self.assertTrue(ret == state, assertion)
+
+    def _assert_proxy_enabled(self, platform):
+        """Assert that the proxy is enabled."""
+        self._assert_proxy_state(platform, True, "Proxy is enabled.")
+
+    def _assert_proxy_disabled(self, platform):
+        """Assert that the proxy is disabled."""
+        self._assert_proxy_state(platform, False, "Proxy is disabled.")
+
+    def test_platform_linux_enabled(self):
+        """Tests for the linux platform with proxies enabled."""
+        self.patch(tunnel_server.gsettings, "get_proxy_settings",
+                   lambda: FAKE_SETTINGS)
+        self._assert_proxy_enabled("linux3")
+
+    def test_platform_linux_disabled(self):
+        """Tests for the linux platform with proxies disabled."""
+        self.patch(tunnel_server.gsettings, "get_proxy_settings", lambda: {})
+        self._assert_proxy_disabled("linux3")
+
+    def test_platform_other_enabled(self):
+        """Tests for any other platform with proxies enabled."""
+        fake_netproxfact = FakeNetworkProxyFactoryClass(True)
+        self.patch(tunnel_server, "QNetworkProxyFactory", fake_netproxfact)
+        self._assert_proxy_enabled("windows 1.0")
+
+    def test_platform_other_disabled(self):
+        """Tests for any other platform with proxies disabled."""
+        fake_netproxfact = FakeNetworkProxyFactoryClass(False)
+        self.patch(tunnel_server, "QNetworkProxyFactory", fake_netproxfact)
+        self._assert_proxy_disabled("windows 1.0")
+
+
+class FakeQCoreApp(object):
+    """A fake QCoreApplication."""
+
+    fake_instance = None
+
+    def __init__(self, argv):
+        """Initialize this fake."""
+        self.executed = False
+        self.argv = argv
+        FakeQCoreApp.fake_instance = self
+
+    def exec_(self):
+        """Fake the execution of this app."""
+        self.executed = True
+
+    @staticmethod
+    def instance():
+        """But return the real instance."""
+        return QCoreApplication.instance()
+
+
+class MainFunctionTestCase(TestCase):
+    """Tests for the main function of the tunnel server."""
+
+    @defer.inlineCallbacks
+    def setUp(self):
+        """Initialize these testcases."""
+        yield super(MainFunctionTestCase, self).setUp()
+        self.called = []
+        self.proxies_enabled = False
+
+        def fake_is_proxy_enabled(*args):
+            """Store the call, return false."""
+            self.called.append(args)
+            return self.proxies_enabled
+
+        self.patch(tunnel_server, "is_proxy_enabled", fake_is_proxy_enabled)
+        self.fake_stdout = StringIO()
+        self.patch(tunnel_server.sys, "stdout", self.fake_stdout)
+        self.patch(tunnel_server, "QCoreApplication", FakeQCoreApp)
+
+    def test_checks_proxies(self):
+        """Main checks that the proxies are enabled."""
+        tunnel_server.main([])
+        self.assertEqual(len(self.called), 1)
+
+    def test_on_proxies_enabled_prints_port(self):
+        """With proxies enabled print port to stdout and start the mainloop."""
+        self.proxies_enabled = True
+        tunnel_server.main(["example.com", "443"])
+        self.assertIn("Tunnel port:", self.fake_stdout.getvalue())
+
+    def test_on_proxies_disabled_exit(self):
+        """With proxies disabled, print a message and exit gracefully."""
+        self.proxies_enabled = False
+        tunnel_server.main(["example.com", "443"])
+        self.assertIn("Proxy not enabled.", self.fake_stdout.getvalue())
+        self.assertEqual(FakeQCoreApp.fake_instance, None)

=== modified file 'ubuntuone/proxy/tunnel_server.py'
--- ubuntuone/proxy/tunnel_server.py	2012-03-07 23:43:07 +0000
+++ ubuntuone/proxy/tunnel_server.py	2012-03-09 16:20:31 +0000
@@ -29,18 +29,22 @@
 
 """
 
+import sys
 
 from PyQt4.QtCore import QCoreApplication, QTimer
 from PyQt4.QtNetwork import (
     QAbstractSocket,
     QHostAddress,
     QNetworkProxy,
+    QNetworkProxyQuery,
+    QNetworkProxyFactory,
     QTcpServer,
     QTcpSocket,
 )
 from twisted.internet import defer, interfaces
 from zope.interface import implements
 
+from ubuntu_sso.utils.webclient import gsettings
 from ubuntuone.proxy.common import BaseTunnelProtocol, CRLF
 from ubuntuone.proxy.logger import logger
 
@@ -236,9 +240,11 @@
         self.server = QTcpServer(QCoreApplication.instance())
         self.server.newConnection.connect(self.new_connection)
         self.server.listen(QHostAddress.LocalHost, 0)
+        logger.info("Starting tunnel server at port %d", self.port)
 
     def new_connection(self):
         """On a new connection create a new tunnel instance."""
+        logger.info("New connection made")
         local_socket = self.server.nextPendingConnection()
         tunnel = Tunnel(local_socket)
         self.tunnels.append(tunnel)
@@ -251,3 +257,28 @@
     def port(self):
         """The port where this server listens."""
         return self.server.serverPort()
+
+
+def is_proxy_enabled(host, port):
+    """Check if the proxy is enabled."""
+    port = int(port)
+    if sys.platform.startswith("linux"):
+        settings = gsettings.get_proxy_settings()
+        return len(settings) > 0
+    else:
+        query = QNetworkProxyQuery(host, port)
+        proxies = QNetworkProxyFactory.systemProxyForQuery(query)
+        return len(proxies) and proxies[0].type() != QNetworkProxy.NoProxy
+
+
+def main(argv):
+    """The main function for the tunnel server."""
+    if not is_proxy_enabled(*argv[1:]):
+        sys.stdout.write("Proxy not enabled.")
+        sys.stdout.flush()
+    else:
+        app = QCoreApplication(argv)
+        tunnel_server = TunnelServer()
+        sys.stdout.write("Tunnel port: {}\n".format(tunnel_server.port))
+        sys.stdout.flush()
+        app.exec_()