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

Proposed by Alejandro J. Cura on 2012-03-06
Status: Merged
Approved by: dobey on 2012-03-09
Approved revision: 1219
Merged at revision: 1204
Proposed branch: lp:~alecu/ubuntuone-client/proxy-tunnel-process
Merge into: lp:ubuntuone-client
Prerequisite: lp:~alecu/ubuntuone-client/proxy-tunnel-client
Diff against target: 266 lines (+188/-1)
4 files modified
Makefile.am (+1/-0)
bin/ubuntuone-proxy-tunnel (+24/-0)
tests/proxy/test_tunnel_server.py (+132/-1)
ubuntuone/proxy/tunnel_server.py (+31/-0)
To merge this branch: bzr merge lp:~alecu/ubuntuone-client/proxy-tunnel-process
Reviewer Review Type Date Requested Status
Alejandro J. Cura (community) Abstain on 2012-03-09
Eric Casteleijn (community) Approve on 2012-03-09
Manuel de la Peña (community) 2012-03-06 Approve on 2012-03-09
Review via email: mp+96212@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Manuel de la Peña (mandel) wrote :

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) wrote :

> 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) wrote :

Superb!

review: Approve
Eric Casteleijn (thisfred) wrote :

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) wrote :

> 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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Makefile.am'
2--- Makefile.am 2012-02-24 17:57:27 +0000
3+++ Makefile.am 2012-03-09 16:20:31 +0000
4@@ -19,6 +19,7 @@
5
6 libexec_SCRIPTS = \
7 bin/ubuntuone-syncdaemon \
8+ bin/ubuntuone-proxy-tunnel \
9 bin/ubuntuone-login
10
11 clientdefsdir = $(pythonpkgdir)/ubuntuone
12
13=== added file 'bin/ubuntuone-proxy-tunnel'
14--- bin/ubuntuone-proxy-tunnel 1970-01-01 00:00:00 +0000
15+++ bin/ubuntuone-proxy-tunnel 2012-03-09 16:20:31 +0000
16@@ -0,0 +1,24 @@
17+#!/usr/bin/python
18+#
19+# Copyright 2012 Canonical Ltd.
20+#
21+# This program is free software: you can redistribute it and/or modify it
22+# under the terms of the GNU General Public License version 3, as published
23+# by the Free Software Foundation.
24+#
25+# This program is distributed in the hope that it will be useful, but
26+# WITHOUT ANY WARRANTY; without even the implied warranties of
27+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
28+# PURPOSE. See the GNU General Public License for more details.
29+#
30+# You should have received a copy of the GNU General Public License along
31+# with this program. If not, see <http://www.gnu.org/licenses/>.
32+
33+"""Ubuntu One tunnel for proxy support."""
34+
35+import sys
36+
37+from ubuntuone.proxy.tunnel_server import main
38+
39+if __name__ == "__main__":
40+ main(sys.argv)
41
42=== modified file 'tests/proxy/test_tunnel_server.py'
43--- tests/proxy/test_tunnel_server.py 2012-03-09 14:36:34 +0000
44+++ tests/proxy/test_tunnel_server.py 2012-03-09 16:20:31 +0000
45@@ -15,10 +15,12 @@
46 # with this program. If not, see <http://www.gnu.org/licenses/>.
47 """Tests for the proxy tunnel."""
48
49+from StringIO import StringIO
50 from urlparse import urlparse
51
52 from twisted.internet import defer, protocol, reactor
53-
54+from twisted.trial.unittest import TestCase
55+from PyQt4.QtCore import QCoreApplication
56 from ubuntuone.devtools.testcases.squid import SquidTestCase
57
58 from tests.proxy import (
59@@ -39,6 +41,14 @@
60 "GET {0.path} HTTP/1.0" + CRLF + CRLF
61 )
62
63+FAKE_SETTINGS = {
64+ "host": "myhost",
65+ "port": "8888",
66+}
67+
68+SAMPLE_HOST = "samplehost.com"
69+SAMPLE_PORT = 443
70+
71
72 class DisconnectingProtocol(protocol.Protocol):
73 """A protocol that just disconnects."""
74@@ -351,3 +361,124 @@
75 """Tests for the client going thru an authenticated proxy."""
76
77 get_proxy_settings = RemoteSocketTestCase.get_auth_proxy_settings
78+
79+
80+class FakeNetworkProxyFactoryClass(object):
81+ """A fake QNetworkProxyFactory."""
82+ last_query = None
83+
84+ def __init__(self, enabled):
85+ """Initialize this fake instance."""
86+ if enabled:
87+ self.proxy_type = tunnel_server.QNetworkProxy.HttpProxy
88+ else:
89+ self.proxy_type = tunnel_server.QNetworkProxy.NoProxy
90+
91+ def type(self):
92+ """Return the proxy type configured."""
93+ return self.proxy_type
94+
95+ def systemProxyForQuery(self, query):
96+ """A list of proxies, but only type() will be called on the first."""
97+ return [self]
98+
99+
100+class IsProxyEnabledTestCase(TestCase):
101+ """Tests for the is_proxy_enabled function."""
102+
103+ def _assert_proxy_state(self, platform, state, assertion):
104+ """Assert the proxy is in a given state."""
105+ self.patch(tunnel_server.sys, "platform", platform)
106+ ret = tunnel_server.is_proxy_enabled(SAMPLE_HOST, str(SAMPLE_PORT))
107+ self.assertTrue(ret == state, assertion)
108+
109+ def _assert_proxy_enabled(self, platform):
110+ """Assert that the proxy is enabled."""
111+ self._assert_proxy_state(platform, True, "Proxy is enabled.")
112+
113+ def _assert_proxy_disabled(self, platform):
114+ """Assert that the proxy is disabled."""
115+ self._assert_proxy_state(platform, False, "Proxy is disabled.")
116+
117+ def test_platform_linux_enabled(self):
118+ """Tests for the linux platform with proxies enabled."""
119+ self.patch(tunnel_server.gsettings, "get_proxy_settings",
120+ lambda: FAKE_SETTINGS)
121+ self._assert_proxy_enabled("linux3")
122+
123+ def test_platform_linux_disabled(self):
124+ """Tests for the linux platform with proxies disabled."""
125+ self.patch(tunnel_server.gsettings, "get_proxy_settings", lambda: {})
126+ self._assert_proxy_disabled("linux3")
127+
128+ def test_platform_other_enabled(self):
129+ """Tests for any other platform with proxies enabled."""
130+ fake_netproxfact = FakeNetworkProxyFactoryClass(True)
131+ self.patch(tunnel_server, "QNetworkProxyFactory", fake_netproxfact)
132+ self._assert_proxy_enabled("windows 1.0")
133+
134+ def test_platform_other_disabled(self):
135+ """Tests for any other platform with proxies disabled."""
136+ fake_netproxfact = FakeNetworkProxyFactoryClass(False)
137+ self.patch(tunnel_server, "QNetworkProxyFactory", fake_netproxfact)
138+ self._assert_proxy_disabled("windows 1.0")
139+
140+
141+class FakeQCoreApp(object):
142+ """A fake QCoreApplication."""
143+
144+ fake_instance = None
145+
146+ def __init__(self, argv):
147+ """Initialize this fake."""
148+ self.executed = False
149+ self.argv = argv
150+ FakeQCoreApp.fake_instance = self
151+
152+ def exec_(self):
153+ """Fake the execution of this app."""
154+ self.executed = True
155+
156+ @staticmethod
157+ def instance():
158+ """But return the real instance."""
159+ return QCoreApplication.instance()
160+
161+
162+class MainFunctionTestCase(TestCase):
163+ """Tests for the main function of the tunnel server."""
164+
165+ @defer.inlineCallbacks
166+ def setUp(self):
167+ """Initialize these testcases."""
168+ yield super(MainFunctionTestCase, self).setUp()
169+ self.called = []
170+ self.proxies_enabled = False
171+
172+ def fake_is_proxy_enabled(*args):
173+ """Store the call, return false."""
174+ self.called.append(args)
175+ return self.proxies_enabled
176+
177+ self.patch(tunnel_server, "is_proxy_enabled", fake_is_proxy_enabled)
178+ self.fake_stdout = StringIO()
179+ self.patch(tunnel_server.sys, "stdout", self.fake_stdout)
180+ self.patch(tunnel_server, "QCoreApplication", FakeQCoreApp)
181+
182+ def test_checks_proxies(self):
183+ """Main checks that the proxies are enabled."""
184+ tunnel_server.main([])
185+ self.assertEqual(len(self.called), 1)
186+
187+ def test_on_proxies_enabled_prints_port(self):
188+ """With proxies enabled print port to stdout and start the mainloop."""
189+ self.proxies_enabled = True
190+ tunnel_server.main(["example.com", "443"])
191+ self.assertIn("Tunnel port:", self.fake_stdout.getvalue())
192+
193+ def test_on_proxies_disabled_exit(self):
194+ """With proxies disabled, print a message and exit gracefully."""
195+ self.proxies_enabled = False
196+ tunnel_server.main(["example.com", "443"])
197+ self.assertIn("Proxy not enabled.", self.fake_stdout.getvalue())
198+ self.assertEqual(FakeQCoreApp.fake_instance, None)
199
200=== modified file 'ubuntuone/proxy/tunnel_server.py'
201--- ubuntuone/proxy/tunnel_server.py 2012-03-07 23:43:07 +0000
202+++ ubuntuone/proxy/tunnel_server.py 2012-03-09 16:20:31 +0000
203@@ -29,18 +29,22 @@
204
205 """
206
207+import sys
208
209 from PyQt4.QtCore import QCoreApplication, QTimer
210 from PyQt4.QtNetwork import (
211 QAbstractSocket,
212 QHostAddress,
213 QNetworkProxy,
214+ QNetworkProxyQuery,
215+ QNetworkProxyFactory,
216 QTcpServer,
217 QTcpSocket,
218 )
219 from twisted.internet import defer, interfaces
220 from zope.interface import implements
221
222+from ubuntu_sso.utils.webclient import gsettings
223 from ubuntuone.proxy.common import BaseTunnelProtocol, CRLF
224 from ubuntuone.proxy.logger import logger
225
226@@ -236,9 +240,11 @@
227 self.server = QTcpServer(QCoreApplication.instance())
228 self.server.newConnection.connect(self.new_connection)
229 self.server.listen(QHostAddress.LocalHost, 0)
230+ logger.info("Starting tunnel server at port %d", self.port)
231
232 def new_connection(self):
233 """On a new connection create a new tunnel instance."""
234+ logger.info("New connection made")
235 local_socket = self.server.nextPendingConnection()
236 tunnel = Tunnel(local_socket)
237 self.tunnels.append(tunnel)
238@@ -251,3 +257,28 @@
239 def port(self):
240 """The port where this server listens."""
241 return self.server.serverPort()
242+
243+
244+def is_proxy_enabled(host, port):
245+ """Check if the proxy is enabled."""
246+ port = int(port)
247+ if sys.platform.startswith("linux"):
248+ settings = gsettings.get_proxy_settings()
249+ return len(settings) > 0
250+ else:
251+ query = QNetworkProxyQuery(host, port)
252+ proxies = QNetworkProxyFactory.systemProxyForQuery(query)
253+ return len(proxies) and proxies[0].type() != QNetworkProxy.NoProxy
254+
255+
256+def main(argv):
257+ """The main function for the tunnel server."""
258+ if not is_proxy_enabled(*argv[1:]):
259+ sys.stdout.write("Proxy not enabled.")
260+ sys.stdout.flush()
261+ else:
262+ app = QCoreApplication(argv)
263+ tunnel_server = TunnelServer()
264+ sys.stdout.write("Tunnel port: {}\n".format(tunnel_server.port))
265+ sys.stdout.flush()
266+ app.exec_()

Subscribers

People subscribed via source and target branches