Ubuntu Single Sign On Client

Merge lp:~damiano-albani/ubuntu-sso-client/proxy-support into lp:ubuntu-sso-client

  1. proxy-support
  2. Merge into trunk
Proposed by Damiano Albani
Status: Rejected
Rejected by: John O'Brien
Proposed branch: lp:~damiano-albani/ubuntu-sso-client/proxy-support
Merge into: lp:ubuntu-sso-client
Diff against target: 154 lines (+35/-10)
3 files modified
ubuntu_sso/account.py (+31/-7)
ubuntu_sso/credentials.py (+1/-0)
ubuntu_sso/tests/test_account.py (+3/-3)
To merge this branch: bzr merge lp:~damiano-albani/ubuntu-sso-client/proxy-support
Reviewer Review Type Date Requested Status
John O'Brien (community) Disapprove
Alejandro J. Cura (community) Needs Fixing
Natalia Bidart (community) Needs Fixing
Review via email: mp+71170@code.launchpad.net

Description of the change

Fix bug #633280 (proxy support)

To post a comment you must log in.
Revision history for this message
Natalia Bidart (nataliabidart) wrote :

Hello Damiano!

First of all, thanks for contributing to Ubuntu One. I've reviewed your merge proposal and, despite I'm not an expert on proxy support handling (so I will seek more reviewers with expertise in this area), there are already a couple of things that needs fixing:

* if you have not done it already, you need to sign the Canonical Contributor's Agreement so we can merge your contributions into trunk. Please read http://www.canonical.com/contributors for details.

* we have a strict policy about proposed branches: every single change needs to also provide test suites proving its correctness. You can look the existing test suite for reference.

* we also require that the proposed code complains the python styling specs (you can read the PEP-8 and PEP-257). To validate this, we run the pep8 and pylint tools over the branches. Inside trunk, you will find a run-tests bash script that will do all this for you.

Thanks!

review: Needs Fixing
Revision history for this message
Damiano Albani (damiano-albani) wrote :

Hello Natalia,

On Fri, Aug 12, 2011 at 15:58, Natalia Bidart
<email address hidden>wrote:

>
> First of all, thanks for contributing to Ubuntu One. I've reviewed your
> merge proposal and, despite I'm not an expert on proxy support handling (so
> I will seek more reviewers with expertise in this area), there are already a
> couple of things that needs fixing:

Thanks for all those tips, it's the first time that I try to contribute to
Ubuntu so I don't know very much what is the overall process.

Inside trunk, you will find a run-tests bash script that will do all this
> for you.

When I launch the "run-tests" script, it looks like something's going wrong:
my screen gets filled with dozens and dozens of Ubuntu One sign-in/sign-up
windows.
Basically one for each test, as if it "forgot" to close it once the
individual test has been run.
Is it the normal behavior? Am I encountering a bug?

Regards,

--
Damiano ALBANI

Revision history for this message
Shane Fagan (shanepatrickfagan) wrote :

Hey Damiano,

To fix the tests spawning loads of windows install xvfb.

lp:~damiano-albani/ubuntu-sso-client/proxy-support updated
757. By Damiano <damiano@cent-quatre>

Now passes unit tests and complies with syntax requirements.

Revision history for this message
Damiano Albani (damiano-albani) wrote :

On Fri, Aug 12, 2011 at 17:11, Shane Fagan <email address hidden>wrote:

>
> To fix the tests spawning loads of windows install xvfb.

OK, thanks for your help, I've managed to use the test suite to check and
push some code which at least syntactically correct.

--
Damiano ALBANI

Revision history for this message
Alejandro J. Cura (alecu) wrote :

Hi Damiano, thanks for working on this branch.

A few comments:

----

In Account.__init__, this branch assumes that the protocol type of the https proxy is "http", but this code will fail if the user has set it to "https" (for instance, with something like: "https_proxy=https://host:port"),
I think that this branch should consider this, and either use an alternative to the PROXY_TYPE_HTTP constant, or log a warning and ignore it.

----

The "except: pass" does not look good. All except clauses should normally specify the types of exceptions that should be handled, and if they are just passing they should log some error accordingly. This way, if our code can't support some combination of settings, the user will be able to find out why his proxy settings are not being used.

----

This branch should include new unit tests to check that the new code in Account.__init__ works properly.

----

Thanks!

review: Needs Fixing
Revision history for this message
Damiano Albani (damiano-albani) wrote :

Hello Alejandro,

On Wed, Aug 31, 2011 at 19:49, Alejandro J. Cura <email address hidden>wrote:

>
> In Account.__init__, this branch assumes that the protocol type of the
> https proxy is "http", but this code will fail if the user has set it to
> "https" (for instance, with something like: "https_proxy=https://host:port
> "),
> I think that this branch should consider this, and either use an
> alternative to the PROXY_TYPE_HTTP constant, or log a warning and ignore it.
>

Well, what does the syntax "https_proxy=https://host:port" supposed to mean
exactly?
Because my understanding is that only the host and port information are
meaningful.
You could specify the environment variable as
"https_proxy=azerty://host:port" and it would work just as fine.
Check yourself this behavior using cURL or wget. And that's basically the
same thing with the GNOME Proxy configuration panel asking only for host and
port.

Now, there is indeed an issue with SOCKS proxies, which this code currently
ignores. If I'm not mistaken, the environment variable follows this syntax:
"all_proxy=socks5://myproxy:1080". I'm gonna update my patch.

Regards,

--
Damiano ALBANI

Revision history for this message
John O'Brien (jdobrien) wrote :

proxy support is being handled differently now so I'm rejecting this.

Thank you for your efforts to improve Ubuntu One.

review: Disapprove

Unmerged revisions

757. By Damiano <damiano@cent-quatre>

Now passes unit tests and complies with syntax requirements.

756. By Damiano <damiano@cent-quatre>

Adding proxy support, using urllib2 and httplib2 proxy features.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ubuntu_sso/account.py'
2--- ubuntu_sso/account.py 2011-03-16 01:36:07 +0000
3+++ ubuntu_sso/account.py 2011-08-12 15:59:49 +0000
4@@ -20,6 +20,9 @@
5 import os
6 import re
7 import urllib2
8+import urlparse
9+import socks
10+import httplib2
11
12 # Unable to import 'lazr.restfulclient.*'
13 # pylint: disable=F0401
14@@ -38,6 +41,8 @@
15 SSO_STATUS_OK = 'ok'
16 SSO_STATUS_ERROR = 'error'
17
18+urllib2.install_opener(urllib2.build_opener(urllib2.ProxyHandler()))
19+
20
21 class InvalidEmailError(Exception):
22 """The email is not valid."""
23@@ -81,6 +86,18 @@
24 self.sso_service_class = sso_service_class
25
26 self.service_url = os.environ.get('USSOC_SERVICE_URL', SERVICE_URL)
27+ self.proxy_info = None
28+ try:
29+ https_proxy = urllib2.getproxies()['https']
30+ # pylint: disable=E1101
31+ proxy_host, proxy_port = urlparse.urlparse(https_proxy) \
32+ .netloc.split(':')
33+ # pylint: enable=E1101
34+ proxy_port = int(proxy_port)
35+ self.proxy_info = httplib2.ProxyInfo(socks.PROXY_TYPE_HTTP,
36+ proxy_host, proxy_port)
37+ except:
38+ pass
39
40 logger.info('Created a new SSO access layer for service url %r',
41 self.service_url)
42@@ -111,7 +128,8 @@
43 """Generate a captcha using the SSO service."""
44 logger.debug('generate_captcha: requesting captcha, filename: %r',
45 filename)
46- sso_service = self.sso_service_class(None, self.service_url)
47+ sso_service = self.sso_service_class(None, self.service_url,
48+ proxy_info=self.proxy_info)
49 captcha = sso_service.captchas.new()
50
51 # download captcha and save to 'filename'
52@@ -133,7 +151,8 @@
53 logger.debug('register_user: email: %r password: <hidden>, '
54 'displayname: %r, captcha_id: %r, captcha_solution: %r',
55 email, displayname, captcha_id, captcha_solution)
56- sso_service = self.sso_service_class(None, self.service_url)
57+ sso_service = self.sso_service_class(None, self.service_url,
58+ proxy_info=self.proxy_info)
59 if not self._valid_email(email):
60 logger.error('register_user: InvalidEmailError for email: %r',
61 email)
62@@ -162,7 +181,8 @@
63 logger.debug('login: email: %r password: <hidden>, token_name: %r',
64 email, token_name)
65 basic = BasicHttpAuthorizer(email, password)
66- sso_service = self.sso_service_class(basic, self.service_url)
67+ sso_service = self.sso_service_class(basic, self.service_url,
68+ proxy_info=self.proxy_info)
69 service = sso_service.authentications.authenticate
70
71 try:
72@@ -184,7 +204,8 @@
73 authorizer = OAuthAuthorizer(token['consumer_key'],
74 token['consumer_secret'],
75 oauth_token)
76- sso_service = self.sso_service_class(authorizer, self.service_url)
77+ sso_service = self.sso_service_class(authorizer, self.service_url,
78+ proxy_info=self.proxy_info)
79
80 me_info = sso_service.accounts.me()
81 key = 'preferred_email'
82@@ -206,7 +227,8 @@
83 authorizer = OAuthAuthorizer(token['consumer_key'],
84 token['consumer_secret'],
85 oauth_token)
86- sso_service = self.sso_service_class(authorizer, self.service_url)
87+ sso_service = self.sso_service_class(authorizer, self.service_url,
88+ proxy_info=self.proxy_info)
89 result = sso_service.accounts.validate_email(email_token=email_token)
90 logger.info('validate_email: email: %r result: %r', email, result)
91 if 'errors' in result:
92@@ -219,7 +241,8 @@
93
94 def request_password_reset_token(self, email):
95 """Request a token to reset the password for the account 'email'."""
96- sso_service = self.sso_service_class(None, self.service_url)
97+ sso_service = self.sso_service_class(None, self.service_url,
98+ proxy_info=self.proxy_info)
99 service = sso_service.registrations.request_password_reset_token
100 try:
101 result = service(email=email)
102@@ -240,7 +263,8 @@
103 'request_password_reset_token'.
104
105 """
106- sso_service = self.sso_service_class(None, self.service_url)
107+ sso_service = self.sso_service_class(None, self.service_url,
108+ proxy_info=self.proxy_info)
109 service = sso_service.registrations.set_new_password
110 try:
111 result = service(email=email, token=token,
112
113=== modified file 'ubuntu_sso/credentials.py'
114--- ubuntu_sso/credentials.py 2011-07-29 14:03:01 +0000
115+++ ubuntu_sso/credentials.py 2011-08-12 15:59:49 +0000
116@@ -52,6 +52,7 @@
117
118 logger = setup_logging('ubuntu_sso.credentials')
119
120+urllib2.install_opener(urllib2.build_opener(urllib2.ProxyHandler()))
121
122 APP_NAME_KEY = 'app_name'
123 TC_URL_KEY = 'tc_url'
124
125=== modified file 'ubuntu_sso/tests/test_account.py'
126--- ubuntu_sso/tests/test_account.py 2011-06-30 00:28:39 +0000
127+++ ubuntu_sso/tests/test_account.py 2011-08-12 15:59:49 +0000
128@@ -135,7 +135,7 @@
129 class FakedSSOServer(object):
130 """Fake an SSO server."""
131
132- def __init__(self, authorizer, service_root):
133+ def __init__(self, authorizer, service_root, proxy_info):
134 self.captchas = FakedCaptchas()
135 self.registrations = FakedRegistrations()
136 self.authentications = FakedAuthentications()
137@@ -256,7 +256,7 @@
138
139 def test_is_not_validated(self):
140 """If preferred email is None, user is not validated."""
141- service = FakedSSOServer(None, None)
142+ service = FakedSSOServer(None, None, None)
143 service.accounts.preferred_email = None
144 result = self.processor.is_validated(sso_service=service,
145 token=TOKEN)
146@@ -264,7 +264,7 @@
147
148 def test_is_not_validated_empty_result(self):
149 """If preferred email is None, user is not validated."""
150- service = FakedSSOServer(None, None)
151+ service = FakedSSOServer(None, None, None)
152 service.accounts.me = lambda: {}
153 result = self.processor.is_validated(sso_service=service,
154 token=TOKEN)

Subscribers

People subscribed via source and target branches