Merge lp:~barry/ubuntuone-storage-protocol/lp1077092 into lp:ubuntuone-storage-protocol

Proposed by Barry Warsaw
Status: Merged
Approved by: dobey
Approved revision: 161
Merged at revision: 157
Proposed branch: lp:~barry/ubuntuone-storage-protocol/lp1077092
Merge into: lp:ubuntuone-storage-protocol
Diff against target: 151 lines (+56/-38)
2 files modified
tests/test_client.py (+37/-21)
ubuntuone/storageprotocol/client.py (+19/-17)
To merge this branch: bzr merge lp:~barry/ubuntuone-storage-protocol/lp1077092
Reviewer Review Type Date Requested Status
dobey (community) Abstain
Roberto Alsina (community) Approve
Alejandro J. Cura (community) Approve
Review via email: mp+146528@code.launchpad.net

Commit message

Switch from python-oauth to python-oauthlib for using OAuth.

Description of the change

Use the supported and Python 3 compatible python-oauthlib instead of the abandoned and no longer supported oauth library.

To post a comment you must log in.
Revision history for this message
Alejandro J. Cura (alecu) wrote :

Thanks for working on this!

One comment: the oauth_authenticate method changed changed here is only used (afaict) from lp:ubuntuone-client ActionQueue.authenticate(), and with a different signature. That means that with this change a new branch for ubuntuone-client would be needed.

Instead, I propose that the changes to the signature of oauth_authenticate in this branch are reverted, and that the compatibility properties defined in lp:~barry/ubuntuone-client/lp1077089 are used instead, like:

135 + client = Client(token.key, token.secret, consumer.key, consumer.secret ...

Does this seem reasonable?

review: Needs Fixing
Revision history for this message
Barry Warsaw (barry) wrote :

On Feb 06, 2013, at 02:04 PM, Alejandro J. Cura wrote:

>One comment: the oauth_authenticate method changed changed here is only used
>(afaict) from lp:ubuntuone-client ActionQueue.authenticate(), and with a
>different signature. That means that with this change a new branch for
>ubuntuone-client would be needed.

Ah right, my changes to ubuntuone-client didn't change the call signature over
there.

>Instead, I propose that the changes to the signature of oauth_authenticate in
>this branch are reverted, and that the compatibility properties defined in
>lp:~barry/ubuntuone-client/lp1077089 are used instead, like:
>
>135 + client = Client(token.key, token.secret, consumer.key, consumer.secret ...
>
>Does this seem reasonable?

It does, good catch.

I suppose if we wanted to get rid of this API backward compatibility hack, we
could do that in a future branch.

Branch update.

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

I'm trying to run both branches together like this:
(make sure to ./run-tests in the u1sp branch first, so protobuf files get generated).

alecu@bollo:~/canonical/ubuntuone-client/review_lp1077089$ u1sdtool -q; PYTHONPATH=.:~/canonical/ubuntuone-storage-protocol/review_lp1077092/ ./bin/ubuntuone-syncdaemon --debug

The syncdaemon debug log shows that the authentication fails when using the new branches, and I'm able to authenticate using the system's syncdaemon, so there must be something that's working differently.

review: Needs Fixing
Revision history for this message
Barry Warsaw (barry) wrote :

Phew! That was a fun one to track down and fix. ;)

Let's start by disabling the dbus auto-restart of the ubuntuone-syncdaemon on
a production system. You have to do this or the `u1sdtool -q` command is
ineffectual. It does quit the daemon, but dbus re-activates it so the one in
bin/ubuntuone-syncdaemon will never be able to start.

The easiest way I found for doing this was to comment out the Name and Exec
lines in /usr/share/dbus-1/services/com.ubuntuone.SyncDaemon.service, then
kill -HUP dbus to re-read its service files. Now when you `u1sdtool -q`, it
kills the running syncdaemon and dbus won't restart it.

On Feb 07, 2013, at 12:13 AM, Alejandro J. Cura wrote:

>I'm trying to run both branches together like this:
>(make sure to ./run-tests in the u1sp branch first, so protobuf files get generated).
>
>alecu@bollo:~/canonical/ubuntuone-client/review_lp1077089$ u1sdtool -q; PYTHONPATH=.:~/canonical/ubuntuone-storage-protocol/review_lp1077092/ ./bin/ubuntuone-syncdaemon --debug
>
>The syncdaemon debug log shows that the authentication fails when using the
>new branches, and I'm able to authenticate using the system's syncdaemon, so
>there must be something that's working differently.

Yep. It turns out that I was confused by the confusing differences in
nomenclature. python-oauth uses the older terms "consumer" and "access token"
while python-oauthlib uses the RFC 5849 terms "client" and "resource owner".
When doing the conversion between the two regimes, I got them backwards and
was passing the first four arguments to oauthlib's Client constructor in the
reverse order.

This should now be fixed. I've updated the LP: #1077092 merge proposal branch
for the correct order in client.py, and it looks to me like both the tests
pass, and the cross-branch test you describe above successfully connects.

Hopefully this resolves any outstanding issues with the two branches, but
please let me know if it's still not working!

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

> Let's start by disabling the dbus auto-restart of the ubuntuone-syncdaemon on
> a production system. You have to do this or the `u1sdtool -q` command is
> ineffectual. It does quit the daemon, but dbus re-activates it so the one in
> bin/ubuntuone-syncdaemon will never be able to start.
>
> The easiest way I found for doing this was to comment out the Name and Exec
> lines in /usr/share/dbus-1/services/com.ubuntuone.SyncDaemon.service, then
> kill -HUP dbus to re-read its service files. Now when you `u1sdtool -q`, it
> kills the running syncdaemon and dbus won't restart it.

Thanks a lot for researching the workaround for this!
I'm ashamed to admit the autorestarting of syncdaemon has been a long standing bug that we've never prioritized.
Sorry again about that.

> Hopefully this resolves any outstanding issues with the two branches, but
> please let me know if it's still not working!

It's connecting perfectly now, thanks again for working on all these branches.

review: Approve
Revision history for this message
Roberto Alsina (ralsina) wrote :

works ok IRL for me

review: Approve
Revision history for this message
dobey (dobey) wrote :

Am just putting a negative vote at the moment, as we can't immediately land this, as it will break ubuntuone-client trunk, and as we need to fix the issue with getting branches landed there, due to squid (ncsa_auth) crashing on raring.

review: Needs Information
Revision history for this message
Barry Warsaw (barry) wrote :

NP. Will this (and the other branches) get auto-merged once trunk is fixed, or do you need me to do anything else?

Revision history for this message
dobey (dobey) wrote :

I'll handle getting it merged, once the squid related kinks are worked around, as it's already been approved.

Revision history for this message
dobey (dobey) :
review: Abstain
Revision history for this message
Ubuntu One Auto Pilot (otto-pilot) wrote :

The attempt to merge lp:~barry/ubuntuone-storage-protocol/lp1077092 into lp:ubuntuone-storage-protocol failed. Below is the output from the failed tests.

running build

*** Cannot find protoc; is the protobuf-compiler package installed?

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'tests/test_client.py'
2--- tests/test_client.py 2012-12-03 19:45:43 +0000
3+++ tests/test_client.py 2013-02-07 21:46:21 +0000
4@@ -38,6 +38,8 @@
5 import unittest
6 import uuid
7
8+from urlparse import urlparse, parse_qsl
9+
10 from twisted.application import internet, service
11 from twisted.internet import defer
12 from twisted.internet.defer import Deferred, inlineCallbacks
13@@ -48,10 +50,12 @@
14 from ubuntuone.storageprotocol.client import (
15 StorageClient, CreateUDF, ListVolumes, DeleteVolume, GetDelta, Unlink,
16 Authenticate, MakeFile, MakeDir, PutContent, Move, BytesMessageProducer,
17- oauth, TwistedTimestampChecker, tx_timestamp_checker,
18+ TwistedTimestampChecker, tx_timestamp_checker,
19 )
20+
21 from ubuntuone.storageprotocol import volumes
22 from tests import test_delta_info
23+from oauthlib.oauth1 import Client
24
25 # let's not get picky about aatributes outside __init__ in tests
26 # pylint: disable=W0201
27@@ -795,31 +799,43 @@
28
29 def test_oauth_authenticate_uses_server_timestamp(self):
30 """The oauth authentication uses the server timestamp."""
31- fromcandt_call = []
32-
33- fake_token = oauth.OAuthToken('token', 'token_secret')
34- fake_consumer = oauth.OAuthConsumer('consumer_key', 'consumer_secret')
35-
36- fake_timestamp = object()
37+ timestamps = []
38+
39+ original_sign = Client.sign
40+
41+ def fake_sign(self, *args, **kwargs):
42+ """A fake Client.sign()."""
43+ url, headers, body = original_sign(self, *args, **kwargs)
44+ timestamps.extend(
45+ value for name, value in parse_qsl(urlparse(url).query)
46+ if name == 'oauth_timestamp')
47+ return url, headers, body
48+
49+ fake_timestamp = '801'
50 timestamp_d = Deferred()
51 self.patch(tx_timestamp_checker, "get_faithful_time",
52 lambda: timestamp_d)
53- original_fromcandt = oauth.OAuthRequest.from_consumer_and_token
54-
55- @staticmethod
56- def fake_from_consumer_and_token(**kwargs):
57- """A fake from_consumer_and_token."""
58- fromcandt_call.append(kwargs)
59- return original_fromcandt(**kwargs)
60-
61- self.patch(oauth.OAuthRequest, "from_consumer_and_token",
62- fake_from_consumer_and_token)
63+
64+ self.patch(Client, 'sign', fake_sign)
65 protocol = FakedProtocol()
66- protocol.oauth_authenticate(fake_consumer, fake_token)
67- self.assertEqual(len(fromcandt_call), 0)
68+ # For backward compatibility of the API, oauth_authenticate() takes a
69+ # consumer object and a token object. Both of those must have .key
70+ # and .secret attributes. In modern OAuth1 (i.e. RFC 5849)
71+ # terminology, the consumer is really the OAuth client, and the token
72+ # is really the OAuth resource owner.
73+
74+ class OAuthClient:
75+ key = 'consumer_key'
76+ secret = 'consumer_secret'
77+
78+ class OAuthResourceOwner:
79+ key = 'token'
80+ secret = 'token_secret'
81+ protocol.oauth_authenticate(OAuthResourceOwner, OAuthClient)
82+ self.assertEqual(len(timestamps), 0)
83 timestamp_d.callback(fake_timestamp)
84- parameters = fromcandt_call[0]["parameters"]
85- self.assertEqual(parameters["oauth_timestamp"], fake_timestamp)
86+ self.assertEqual(len(timestamps), 1)
87+ self.assertEqual(timestamps[0], fake_timestamp)
88
89
90 class RootResource(resource.Resource):
91
92=== modified file 'ubuntuone/storageprotocol/client.py'
93--- ubuntuone/storageprotocol/client.py 2012-12-03 19:45:43 +0000
94+++ ubuntuone/storageprotocol/client.py 2013-02-07 21:46:21 +0000
95@@ -39,7 +39,8 @@
96
97 from functools import partial
98 from itertools import chain
99-from oauth import oauth
100+from urlparse import urlparse, parse_qsl
101+from oauthlib.oauth1 import Client, SIGNATURE_PLAINTEXT, SIGNATURE_TYPE_QUERY
102
103 from twisted.internet.protocol import ClientFactory
104 from twisted.internet import reactor, defer
105@@ -143,29 +144,30 @@
106 def oauth_authenticate(self, consumer, token, metadata=None):
107 """Authenticate to a server using the OAuth provider.
108
109- @param consumer: the OAuth consumer to authenticate with.
110- @type consumer: `oauth.OAuthConsumer`
111- @param token: a previously acquired OAuth access token.
112- @type consumer: `oauth.OAuthToken`
113- @param kwargs: key/values to send as metadata
114+ @param consumer: the OAuth consumer (a.k.a. client in RFC 5849) to
115+ authenticate with.
116+ @type consumer: object having both a `.key` and `.secret` attribute.
117+ @param token: a previously acquired OAuth access token (a.k.a.
118+ resource owner in RFC 5849).
119+ @type token: object having both a `.key` and `.secret` attribute.
120+ @param metadata: key/values to send as metadata
121
122 Return a deferred that will get called with the request
123 object when completed.
124
125 """
126 timestamp = yield tx_timestamp_checker.get_faithful_time()
127- req = oauth.OAuthRequest.from_consumer_and_token(
128- oauth_consumer=consumer,
129- token=token,
130- parameters={"oauth_timestamp": timestamp},
131- http_method="GET",
132- http_url="storage://server")
133- req.sign_request(
134- oauth.OAuthSignatureMethod_PLAINTEXT(), consumer, token)
135-
136- # Make sure all parameter values are strings.
137+ client = Client(consumer.key, consumer.secret,
138+ token.key, token.secret,
139+ signature_method=SIGNATURE_PLAINTEXT,
140+ signature_type=SIGNATURE_TYPE_QUERY,
141+ timestamp=timestamp)
142+ url, headers, body = client.sign('storage://server')
143+ # Parse out the authentication parameters from the query string.
144 auth_parameters = dict(
145- (key, str(value)) for key, value in req.parameters.iteritems())
146+ (name, value) for name, value in
147+ parse_qsl(urlparse(url).query)
148+ if name.startswith('oauth_'))
149 p = Authenticate(self, auth_parameters, metadata=metadata)
150 p.start()
151 result = yield p.deferred

Subscribers

People subscribed via source and target branches