Merge lp:~leonardr/launchpad/publish-tokens-2 into lp:launchpad/db-devel
- publish-tokens-2
- Merge into db-devel
Status: | Rejected |
---|---|
Rejected by: | Aaron Bentley |
Proposed branch: | lp:~leonardr/launchpad/publish-tokens-2 |
Merge into: | lp:launchpad/db-devel |
Diff against target: |
988 lines (+380/-120) 21 files modified
lib/canonical/launchpad/browser/oauth.py (+3/-9) lib/canonical/launchpad/database/oauth.py (+58/-6) lib/canonical/launchpad/doc/oauth.txt (+32/-8) lib/canonical/launchpad/doc/webapp-authorization.txt (+3/-11) lib/canonical/launchpad/doc/webapp-publication.txt (+7/-1) lib/canonical/launchpad/interfaces/_schema_circular_imports.py (+3/-0) lib/canonical/launchpad/interfaces/oauth.py (+80/-30) lib/canonical/launchpad/security.py (+37/-7) lib/canonical/launchpad/testing/pages.py (+15/-5) lib/canonical/launchpad/tests/test_webservice_oauth.py (+82/-0) lib/canonical/launchpad/webapp/authentication.py (+1/-6) lib/canonical/launchpad/webapp/authorization.py (+2/-1) lib/canonical/launchpad/webapp/servers.py (+7/-23) lib/canonical/launchpad/webapp/tests/test_publication.py (+6/-1) lib/canonical/launchpad/zcml/oauth.zcml (+6/-1) lib/lp/bugs/tests/test_bugs_webservice.py (+8/-0) lib/lp/registry/browser/person.py (+9/-0) lib/lp/registry/interfaces/person.py (+5/-1) lib/lp/registry/stories/webservice/xx-person.txt (+2/-0) lib/lp/testing/_webservice.py (+13/-9) versions.cfg (+1/-1) |
To merge this branch: | bzr merge lp:~leonardr/launchpad/publish-tokens-2 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Guilherme Salgado (community) | code | Approve | |
Aaron Bentley (community) | Needs Fixing | ||
Review via email: mp+34123@code.launchpad.net |
Commit message
Description of the change
This branch publishes OAuth access tokens through the web service. This meant adding traversal rules an canonical URL data for OAuthAccessToken objects.
A user can only view their own access tokens, and only by using a client that's been given the GRANT_PERMISSIONS access level. The old rules apply when accessing access tokens through the website: a user can always access their own tokens, and admins can access other peoples'.
Because I changed the rules for permission checks on OAuth access tokens to check the current request, I've added removeSecurityProxy calls in test helper classes like oauth_token_
This branch takes advantage of a new version of lazr.restful so that the read-write 'permission' field of IOAuthAccessToken can be published as read-only in the web service. This code is tested in lazr.restful, but I can add a Launchpad test as well.
Guilherme Salgado (salgado) wrote : | # |
Leonard asked me to do a follow up review of http://
The signature of check_oauth_
Also, it'd be nice to follow the naming scheme we use for other interfaces that are divided into multiple ones based on the permissions of the attributes/methods (e.g. IFooPublic, IFooEditRestric
Guilherme Salgado (salgado) wrote : | # |
And here's another follow up review: http://
+class OAuthTokenBase(
+ """Base implementation of code to check an OAuth-signed request."""
I'd change that docstring to say just that this is a base class for OAuthToken classes, as we may want to add code not related to signature checking here in the future.
+
+ def checkSignature(
+ return check_oauth_
And here you should add a docstring pointing to the interface where the method is defined.
-Now consider a principal authorized to create OAuth tokens. Whenever
-it's not creating OAuth tokens, it has a level of permission
-equivalent to READ_PUBLIC.
+A principal with the GRANT_PERMISSIONS authorization level has a of
+permission equivalent to WRITE_PRIVATE.
Why is the permission changing in this incremental diff? Also, s/of//?
- >>> access_token = token.createAcc
+ >>> access_token = removeSecurityP
Why do you need to remove the security proxy now?
Leonard Richardson (leonardr) wrote : | # |
> -Now consider a principal authorized to create OAuth tokens. Whenever
> -it's not creating OAuth tokens, it has a level of permission
> -equivalent to READ_PUBLIC.
> +A principal with the GRANT_PERMISSIONS authorization level has a of
> +permission equivalent to WRITE_PRIVATE.
>
> Why is the permission changing in this incremental diff? Also, s/of//?
The permission changed last time, after we decided there was no easy way to make GRANT_PERMISSIONS have a 'write' level of access for OAuth tokens and a 'read' level for everything else. I'm just fixing the test.
> - >>> access_token = token.createAcc
> + >>> access_token = removeSecurityP
>
> Why do you need to remove the security proxy now?
Again, I'm just fixing a test failure. The security proxy started breaking this test as soon as I introduced the security checker that assumes there's a current request.
Guilherme Salgado (salgado) : | # |
Preview Diff
1 | === modified file 'lib/canonical/launchpad/browser/oauth.py' |
2 | --- lib/canonical/launchpad/browser/oauth.py 2010-08-24 16:44:42 +0000 |
3 | +++ lib/canonical/launchpad/browser/oauth.py 2010-09-02 21:17:47 +0000 |
4 | @@ -83,7 +83,7 @@ |
5 | if consumer is None: |
6 | consumer = consumer_set.new(key=consumer_key) |
7 | |
8 | - if not check_oauth_signature(self.request, consumer, None): |
9 | + if not check_oauth_signature(self.request, consumer, ''): |
10 | return u'' |
11 | |
12 | token = consumer.newRequestToken() |
13 | @@ -284,13 +284,12 @@ |
14 | self.request.unauthorized(OAUTH_CHALLENGE) |
15 | return u'No request token specified.' |
16 | |
17 | - if not check_oauth_signature(self.request, consumer, token): |
18 | + if not token.checkSignature(self.request): |
19 | return u'Invalid OAuth signature.' |
20 | |
21 | if not token.is_reviewed: |
22 | self.request.unauthorized(OAUTH_CHALLENGE) |
23 | return u'Request token has not yet been reviewed. Try again later.' |
24 | - |
25 | if token.permission == OAuthPermission.UNAUTHORIZED: |
26 | # The end-user explicitly refused to authorize this |
27 | # token. We send 403 ("Forbidden") instead of 401 |
28 | @@ -301,9 +300,4 @@ |
29 | return u'End-user refused to authorize request token.' |
30 | |
31 | access_token = token.createAccessToken() |
32 | - context_name = None |
33 | - if access_token.context is not None: |
34 | - context_name = access_token.context.name |
35 | - body = u'oauth_token=%s&oauth_token_secret=%s&lp.context=%s' % ( |
36 | - access_token.key, access_token.secret, context_name) |
37 | - return body |
38 | + return access_token.form_encoded |
39 | |
40 | === modified file 'lib/canonical/launchpad/database/oauth.py' |
41 | --- lib/canonical/launchpad/database/oauth.py 2010-08-20 20:31:18 +0000 |
42 | +++ lib/canonical/launchpad/database/oauth.py 2010-09-02 21:17:47 +0000 |
43 | @@ -24,6 +24,7 @@ |
44 | from storm.expr import And |
45 | from zope.component import getUtility |
46 | from zope.interface import implements |
47 | +from zope.security.interfaces import Unauthorized |
48 | |
49 | from canonical.database.constants import UTC_NOW |
50 | from canonical.database.datetimecol import UtcDateTimeCol |
51 | @@ -36,21 +37,25 @@ |
52 | from canonical.launchpad.interfaces import ( |
53 | ClockSkew, |
54 | IOAuthAccessToken, |
55 | + IOAuthAccessTokenPublic, |
56 | IOAuthConsumer, |
57 | IOAuthConsumerSet, |
58 | IOAuthNonce, |
59 | IOAuthRequestToken, |
60 | IOAuthRequestTokenSet, |
61 | + IOAuthTokenPublic, |
62 | NonceAlreadyUsed, |
63 | TimestampOrderingError, |
64 | ) |
65 | from canonical.launchpad.webapp.interfaces import ( |
66 | AccessLevel, |
67 | + ICanonicalUrlData, |
68 | IStoreSelector, |
69 | MAIN_STORE, |
70 | MASTER_FLAVOR, |
71 | OAuthPermission, |
72 | ) |
73 | +from canonical.launchpad.webapp.authentication import check_oauth_signature |
74 | from lp.registry.interfaces.distribution import IDistribution |
75 | from lp.registry.interfaces.distributionsourcepackage import ( |
76 | IDistributionSourcePackage, |
77 | @@ -134,9 +139,28 @@ |
78 | return OAuthConsumer.selectOneBy(key=key) |
79 | |
80 | |
81 | -class OAuthAccessToken(OAuthBase): |
82 | +class OAuthTokenBase(OAuthBase): |
83 | + """Base implementation of code for OAuth tokens.""" |
84 | + |
85 | + def checkSignature(self, request): |
86 | + """See `IOAuthTokenPublic`.""" |
87 | + return check_oauth_signature(request, self.consumer, self.secret) |
88 | + |
89 | + |
90 | +class OAuthAccessToken(OAuthTokenBase): |
91 | """See `IOAuthAccessToken`.""" |
92 | - implements(IOAuthAccessToken) |
93 | + implements( |
94 | + IOAuthAccessToken, IOAuthAccessTokenPublic, ICanonicalUrlData) |
95 | + |
96 | + @property |
97 | + def inside(self): |
98 | + return self.person |
99 | + |
100 | + @property |
101 | + def path(self): |
102 | + return "+oauth-access-token/" + self.key |
103 | + |
104 | + rootsite = 'api' |
105 | |
106 | consumer = ForeignKey( |
107 | dbName='consumer', foreignKey='OAuthConsumer', notNull=True) |
108 | @@ -178,7 +202,7 @@ |
109 | return None |
110 | |
111 | def checkNonceAndTimestamp(self, nonce, timestamp): |
112 | - """See `IOAuthAccessToken`.""" |
113 | + """See `IOAuthAccessTokenPublic`.""" |
114 | timestamp = float(timestamp) |
115 | date = datetime.fromtimestamp(timestamp, pytz.UTC) |
116 | # Determine if the timestamp is too far off from now. |
117 | @@ -209,10 +233,38 @@ |
118 | return OAuthNonce( |
119 | access_token=self, nonce=nonce, request_timestamp=date) |
120 | |
121 | - |
122 | -class OAuthRequestToken(OAuthBase): |
123 | + def authorize(self, request, nonce, timestamp): |
124 | + """See `IOAuthAccessTokenPublic`.""" |
125 | + try: |
126 | + self.checkNonceAndTimestamp(nonce, timestamp) |
127 | + except (NonceAlreadyUsed, TimestampOrderingError, ClockSkew), e: |
128 | + raise Unauthorized('Invalid nonce/timestamp: %s' % e) |
129 | + now = datetime.now(pytz.timezone('UTC')) |
130 | + if self.permission == OAuthPermission.UNAUTHORIZED: |
131 | + raise Unauthorized('Unauthorized token (%s).' % self.key) |
132 | + elif self.date_expires is not None and self.date_expires <= now: |
133 | + raise Unauthorized('Expired token (%s).' % self.key) |
134 | + elif not self.checkSignature(request): |
135 | + raise Unauthorized('Invalid signature.') |
136 | + else: |
137 | + # Everything is fine, let's return the principal. |
138 | + pass |
139 | + return (self.person.account.id, self.permission, self.context) |
140 | + |
141 | + @property |
142 | + def form_encoded(self): |
143 | + """See `IOAuthAccessTokenPublic`.""" |
144 | + context_name = None |
145 | + if self.context is not None: |
146 | + context_name = self.context.name |
147 | + body = u'oauth_token=%s&oauth_token_secret=%s&lp.context=%s' % ( |
148 | + self.key, self.secret, context_name) |
149 | + return body |
150 | + |
151 | + |
152 | +class OAuthRequestToken(OAuthTokenBase): |
153 | """See `IOAuthRequestToken`.""" |
154 | - implements(IOAuthRequestToken) |
155 | + implements(IOAuthRequestToken, IOAuthTokenPublic) |
156 | |
157 | consumer = ForeignKey( |
158 | dbName='consumer', foreignKey='OAuthConsumer', notNull=True) |
159 | |
160 | === modified file 'lib/canonical/launchpad/doc/oauth.txt' |
161 | --- lib/canonical/launchpad/doc/oauth.txt 2010-04-16 15:06:55 +0000 |
162 | +++ lib/canonical/launchpad/doc/oauth.txt 2010-09-02 21:17:47 +0000 |
163 | @@ -186,6 +186,13 @@ |
164 | True |
165 | >>> request_token.permission |
166 | <DBItem OAuthPermission.WRITE_PUBLIC... |
167 | + |
168 | +Because an access token can only be viewed or edited by its owner or |
169 | +an admin, we need to log in at this point. |
170 | + |
171 | + >>> token_owner = request_token.person |
172 | + >>> login_person(token_owner) |
173 | + |
174 | >>> access_token = request_token.createAccessToken() |
175 | >>> verifyObject(IOAuthAccessToken, access_token) |
176 | True |
177 | @@ -245,14 +252,30 @@ |
178 | >>> consumer.getAccessToken(access_token.key) == access_token |
179 | True |
180 | |
181 | -An access token can only be changed by the person associated with it. |
182 | - |
183 | - >>> access_token.permission = OAuthPermission.WRITE_PUBLIC |
184 | - Traceback (most recent call last): |
185 | - ... |
186 | - Unauthorized:... |
187 | - >>> login_person(access_token.person) |
188 | - >>> access_token.permission = AccessLevel.WRITE_PUBLIC |
189 | +In general, one user cannot edit another user's access token. |
190 | + |
191 | + >>> login('no-priv@canonical.com') |
192 | + >>> access_token.permission |
193 | + Traceback (most recent call last): |
194 | + ... |
195 | + Unauthorized:... |
196 | + |
197 | + >>> access_token.permission = AccessLevel.WRITE_PUBLIC |
198 | + Traceback (most recent call last): |
199 | + ... |
200 | + Unauthorized:... |
201 | + |
202 | +An admin can view and edit someone else's access token. |
203 | + |
204 | + >>> login("foo.bar@canonical.com") |
205 | + >>> print access_token.permission.name |
206 | + WRITE_PUBLIC |
207 | + >>> access_token.permission = AccessLevel.WRITE_PUBLIC |
208 | + |
209 | +And a user can edit their own access tokens. |
210 | + |
211 | + >>> login_person(token_owner) |
212 | + >>> access_token.permission = AccessLevel.WRITE_PRIVATE |
213 | |
214 | From any given person it's possible to retrieve his non-expired access |
215 | tokens. |
216 | @@ -305,6 +328,7 @@ |
217 | |
218 | - We can use a nonce. |
219 | |
220 | + >>> login_person(token_owner) |
221 | >>> import time |
222 | >>> now = time.time() - 1 |
223 | >>> nonce1 = access_token.checkNonceAndTimestamp('boo', now) |
224 | |
225 | === modified file 'lib/canonical/launchpad/doc/webapp-authorization.txt' |
226 | --- lib/canonical/launchpad/doc/webapp-authorization.txt 2010-08-24 16:44:42 +0000 |
227 | +++ lib/canonical/launchpad/doc/webapp-authorization.txt 2010-09-02 21:17:47 +0000 |
228 | @@ -79,9 +79,8 @@ |
229 | >>> check_permission('launchpad.View', bug_1) |
230 | False |
231 | |
232 | -Now consider a principal authorized to create OAuth tokens. Whenever |
233 | -it's not creating OAuth tokens, it has a level of permission |
234 | -equivalent to READ_PUBLIC. |
235 | +A principal with the GRANT_PERMISSIONS authorization level has a level |
236 | +of permission equivalent to WRITE_PUBLIC. |
237 | |
238 | >>> principal.access_level = AccessLevel.GRANT_PERMISSIONS |
239 | >>> setupInteraction(principal) |
240 | @@ -89,14 +88,7 @@ |
241 | False |
242 | |
243 | >>> check_permission('launchpad.Edit', sample_person) |
244 | - False |
245 | - |
246 | -This may seem useless from a security standpoint, since once a |
247 | -malicious client is authorized to create OAuth tokens, it can escalate |
248 | -its privileges at any time by creating a new token for itself. The |
249 | -security benefit is more subtle: by discouraging feature creep in |
250 | -clients that have this super-access level, we reduce the risk that a |
251 | -bug in a _trusted_ client will enable privilege escalation attacks. |
252 | + True |
253 | |
254 | Users logged in through the web application have full access, which |
255 | means they can read/change any object they have access to. |
256 | |
257 | === modified file 'lib/canonical/launchpad/doc/webapp-publication.txt' |
258 | --- lib/canonical/launchpad/doc/webapp-publication.txt 2010-08-05 13:23:52 +0000 |
259 | +++ lib/canonical/launchpad/doc/webapp-publication.txt 2010-09-02 21:17:47 +0000 |
260 | @@ -1156,14 +1156,20 @@ |
261 | principal's access_level and scope will match what was specified in the |
262 | token. |
263 | |
264 | +First let's create a token. |
265 | + |
266 | >>> from lp.registry.interfaces.product import IProductSet |
267 | + >>> from zope.security.proxy import removeSecurityProxy |
268 | >>> getUtility(IStoreSelector).push(MasterDatabasePolicy()) |
269 | >>> salgado = getUtility(IPersonSet).getByName('salgado') |
270 | >>> consumer = getUtility(IOAuthConsumerSet).getByKey('foobar123451432') |
271 | >>> token = consumer.newRequestToken() |
272 | >>> firefox = getUtility(IProductSet)['firefox'] |
273 | >>> token.review(salgado, OAuthPermission.WRITE_PUBLIC, context=firefox) |
274 | - >>> access_token = token.createAccessToken() |
275 | + >>> access_token = removeSecurityProxy(token.createAccessToken()) |
276 | + |
277 | +Now let's use it to make a request. |
278 | + |
279 | >>> form = dict( |
280 | ... oauth_consumer_key='foobar123451432', |
281 | ... oauth_token=access_token.key, |
282 | |
283 | === modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py' |
284 | --- lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2010-08-27 11:19:54 +0000 |
285 | +++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2010-09-02 21:17:47 +0000 |
286 | @@ -32,6 +32,7 @@ |
287 | IMessage, |
288 | IUserToUserEmail, |
289 | ) |
290 | +from canonical.launchpad.interfaces.oauth import IOAuthToken |
291 | from lp.blueprints.interfaces.specification import ISpecification |
292 | from lp.blueprints.interfaces.specificationbranch import ISpecificationBranch |
293 | from lp.bugs.interfaces.bug import ( |
294 | @@ -196,6 +197,8 @@ |
295 | patch_choice_parameter_type( |
296 | IHasBugs, 'searchTasks', 'hardware_bus', HWBus) |
297 | |
298 | +IOAuthToken['person'].schema = IPerson |
299 | + |
300 | IPreviewDiff['branch_merge_proposal'].schema = IBranchMergeProposal |
301 | |
302 | patch_reference_property(IPersonPublic, 'archive', IArchive) |
303 | |
304 | === modified file 'lib/canonical/launchpad/interfaces/oauth.py' |
305 | --- lib/canonical/launchpad/interfaces/oauth.py 2010-08-20 20:31:18 +0000 |
306 | +++ lib/canonical/launchpad/interfaces/oauth.py 2010-09-02 21:17:47 +0000 |
307 | @@ -11,12 +11,15 @@ |
308 | 'OAUTH_REALM', |
309 | 'OAUTH_CHALLENGE', |
310 | 'IOAuthAccessToken', |
311 | + 'IOAuthAccessTokenPublic', |
312 | 'IOAuthConsumer', |
313 | 'IOAuthConsumerSet', |
314 | + 'IOAuthToken', |
315 | 'IOAuthNonce', |
316 | 'IOAuthRequestToken', |
317 | 'IOAuthRequestTokenSet', |
318 | 'IOAuthSignedRequest', |
319 | + 'IOAuthTokenPublic', |
320 | 'NonceAlreadyUsed', |
321 | 'TimestampOrderingError', |
322 | 'ClockSkew', |
323 | @@ -34,12 +37,14 @@ |
324 | TextLine, |
325 | ) |
326 | |
327 | +from lazr.restful.declarations import ( |
328 | + export_as_webservice_entry, exported) |
329 | + |
330 | from canonical.launchpad import _ |
331 | from canonical.launchpad.webapp.interfaces import ( |
332 | AccessLevel, |
333 | OAuthPermission, |
334 | ) |
335 | -from lp.registry.interfaces.person import IPerson |
336 | |
337 | # The challenge included in responses with a 401 status. |
338 | OAUTH_REALM = 'https://api.launchpad.net' |
339 | @@ -128,23 +133,28 @@ |
340 | schema=IOAuthConsumer, title=_('The consumer.'), |
341 | description=_("The consumer which will access Launchpad on the " |
342 | "user's behalf.")) |
343 | - person = Object( |
344 | - schema=IPerson, title=_('Person'), required=False, readonly=False, |
345 | - description=_('The user on whose behalf the consumer is accessing.')) |
346 | - date_created = Datetime( |
347 | - title=_('Date created'), required=True, readonly=True) |
348 | - date_expires = Datetime( |
349 | - title=_('Date expires'), required=False, readonly=False, |
350 | - description=_('From this date onwards this token can not be used ' |
351 | - 'by the consumer to access protected resources.')) |
352 | - key = TextLine( |
353 | - title=_('Key'), required=True, readonly=True, |
354 | - description=_('The key used to identify this token. It is included ' |
355 | - 'by the consumer in each request.')) |
356 | - secret = TextLine( |
357 | - title=_('Secret'), required=True, readonly=True, |
358 | - description=_('The secret associated with this token. It is used ' |
359 | - 'by the consumer to sign its requests.')) |
360 | + person = exported( |
361 | + Object(schema=Interface, title=_('Person'), required=False, |
362 | + readonly=False, description=_( |
363 | + 'The user on whose behalf the consumer is accessing.')), |
364 | + readonly=True) |
365 | + date_created = exported( |
366 | + Datetime(title=_('Date created'), required=True, readonly=True)) |
367 | + date_expires = exported( |
368 | + Datetime( |
369 | + title=_('Date expires'), required=False, readonly=False, |
370 | + description=_('From this date onwards this token can not be used ' |
371 | + 'by the consumer to access protected resources.'))) |
372 | + key = exported( |
373 | + TextLine( |
374 | + title=_('Key'), required=True, readonly=True, |
375 | + description=_('The key used to identify this token. It is ' |
376 | + 'included by the consumer in each request.'))) |
377 | + secret = exported( |
378 | + TextLine( |
379 | + title=_('Secret'), required=True, readonly=True, |
380 | + description=_('The secret associated with this token. It is used ' |
381 | + 'by the consumer to sign its requests.'))) |
382 | product = Choice(title=_('Project'), required=False, vocabulary='Product') |
383 | project = Choice( |
384 | title=_('Project'), required=False, vocabulary='ProjectGroup') |
385 | @@ -155,18 +165,22 @@ |
386 | context = Attribute("FIXME") |
387 | |
388 | |
389 | -class IOAuthAccessToken(IOAuthToken): |
390 | - """A token used by a consumer to access protected resources in LP. |
391 | - |
392 | - It's created automatically once a user logs in and grants access to a |
393 | - consumer. The consumer then exchanges an `IOAuthRequestToken` for it. |
394 | - """ |
395 | - |
396 | - permission = Choice( |
397 | - title=_('Access level'), required=True, readonly=False, |
398 | - vocabulary=AccessLevel, |
399 | - description=_('The level of access given to the application acting ' |
400 | - 'on your behalf.')) |
401 | +class IOAuthTokenPublic(Interface): |
402 | + """Public access to methods necessary to authenticate OAuth tokens.""" |
403 | + |
404 | + def checkSignature(request): |
405 | + """Check the signature of an incoming request. |
406 | + |
407 | + :param request: A request that's supposedly signed with this |
408 | + token's secret. |
409 | + :return: True if the request is correctly signed, False otherwise. |
410 | + """ |
411 | + |
412 | + |
413 | +class IOAuthAccessTokenPublic(IOAuthTokenPublic): |
414 | + """Public access to methods necessary to authenticate access tokens.""" |
415 | + |
416 | + form_encoded = Attribute("Form-encoded description of the token.") |
417 | |
418 | def checkNonceAndTimestamp(nonce, timestamp): |
419 | """Verify the nonce and timestamp. |
420 | @@ -189,6 +203,42 @@ |
421 | and associated with this token. |
422 | """ |
423 | |
424 | + def authorize(request, nonce, timestamp): |
425 | + """Authorize an incoming request against an OAuth token. |
426 | + |
427 | + :param request: A request that's supposedly signed with this |
428 | + token's secret. |
429 | + :param nonce: An OAuth nonce provided with the request. |
430 | + :param timestamp: An OAuth timestamp provided with the request. |
431 | + |
432 | + :raise Unauthorized: If the request is not correctly signed |
433 | + with this token's secret, if the nonce or timestamp are |
434 | + bad, if the token has expired, if the token does not |
435 | + actually grant access, or if for any other reason the |
436 | + request cannot be authorized. |
437 | + |
438 | + :return: A 3-tuple (account_id, access_level, context) |
439 | + containing all the information necessary to identify the |
440 | + authenticated principal. |
441 | + """ |
442 | + |
443 | + |
444 | +class IOAuthAccessToken(IOAuthToken): |
445 | + """A token used by a consumer to access protected resources in LP. |
446 | + |
447 | + It's created automatically once a user logs in and grants access to a |
448 | + consumer. The consumer then exchanges an `IOAuthRequestToken` for it. |
449 | + """ |
450 | + export_as_webservice_entry() |
451 | + |
452 | + permission = exported( |
453 | + Choice( |
454 | + title=_('Access level'), required=True, readonly=False, |
455 | + vocabulary=AccessLevel, |
456 | + description=_('The level of access given to the application acting ' |
457 | + 'on your behalf.')), |
458 | + readonly=True) |
459 | + |
460 | |
461 | class IOAuthRequestToken(IOAuthToken): |
462 | """A token used by a consumer to ask the user to authenticate on LP. |
463 | |
464 | === modified file 'lib/canonical/launchpad/security.py' |
465 | --- lib/canonical/launchpad/security.py 2010-08-31 20:12:22 +0000 |
466 | +++ lib/canonical/launchpad/security.py 2010-09-02 21:17:47 +0000 |
467 | @@ -33,12 +33,15 @@ |
468 | from canonical.launchpad.interfaces.oauth import ( |
469 | IOAuthAccessToken, |
470 | IOAuthRequestToken, |
471 | + IOAuthSignedRequest, |
472 | ) |
473 | from canonical.launchpad.webapp.authorization import check_permission |
474 | from canonical.launchpad.webapp.interfaces import ( |
475 | + AccessLevel, |
476 | IAuthorization, |
477 | ILaunchpadRoot, |
478 | ) |
479 | +from canonical.lazr.utils import get_current_browser_request |
480 | from lp.answers.interfaces.faq import IFAQ |
481 | from lp.answers.interfaces.faqtarget import IFAQTarget |
482 | from lp.answers.interfaces.question import IQuestion |
483 | @@ -373,15 +376,42 @@ |
484 | return user.in_admin or user.in_registry_experts |
485 | |
486 | |
487 | -class EditOAuthAccessToken(AuthorizationBase): |
488 | - permission = 'launchpad.Edit' |
489 | - usedfor = IOAuthAccessToken |
490 | +class OAuthToken(AuthorizationBase): |
491 | + """Special rules for access to OAuth tokens. |
492 | + |
493 | + Through the website, a person can always access their own OAuth |
494 | + tokens, and an admin can access someone else's OAuth tokens. |
495 | + |
496 | + To minimize the risk of privilege escalation attacks, much greater |
497 | + restrictions apply through the web service. A person can only |
498 | + access their own OAuth tokens, and they must be using a client |
499 | + authorized at the GRANT_PERMISSIONS access level. |
500 | + """ |
501 | |
502 | def checkAuthenticated(self, user): |
503 | - return self.obj.person == user.person or user.in_admin |
504 | - |
505 | - |
506 | -class EditOAuthRequestToken(EditOAuthAccessToken): |
507 | + request = get_current_browser_request() |
508 | + if IOAuthSignedRequest.providedBy(request): |
509 | + # This is a web service request. |
510 | + if self.obj.person != user.person: |
511 | + return False |
512 | + return (request.principal.access_level == |
513 | + AccessLevel.GRANT_PERMISSIONS) |
514 | + else: |
515 | + # This is a website request. |
516 | + return self.obj.person == user.person or user.in_admin |
517 | + |
518 | + |
519 | +class ViewOAuthAccessToken(OAuthToken): |
520 | + permission = 'launchpad.View' |
521 | + usedfor = IOAuthAccessToken |
522 | + |
523 | + |
524 | +class EditOAuthAccessToken(OAuthToken): |
525 | + permission = 'launchpad.Edit' |
526 | + usedfor = IOAuthAccessToken |
527 | + |
528 | + |
529 | +class EditOAuthRequestToken(OAuthToken): |
530 | permission = 'launchpad.Edit' |
531 | usedfor = IOAuthRequestToken |
532 | |
533 | |
534 | === modified file 'lib/canonical/launchpad/testing/pages.py' |
535 | --- lib/canonical/launchpad/testing/pages.py 2010-08-20 20:31:18 +0000 |
536 | +++ lib/canonical/launchpad/testing/pages.py 2010-09-02 21:17:47 +0000 |
537 | @@ -153,10 +153,16 @@ |
538 | self.consumer = OAuthConsumer(oauth_consumer_key, '') |
539 | self.access_token = OAuthToken(oauth_access_key, '') |
540 | else: |
541 | - # The client wants to make an authorized request |
542 | - # using a recognized consumer key. |
543 | - self.access_token = self.consumer.getAccessToken( |
544 | - oauth_access_key) |
545 | + # The client wants to make authorized requests |
546 | + # using a recognized consumer key. The security |
547 | + # policy for IOAuthAccessToken assumes the request |
548 | + # is being processed on the server side and has |
549 | + # already been signed with some access token. |
550 | + # |
551 | + # Here, the requests haven't even gone out yet, |
552 | + # so it's okay to strip the security proxy. |
553 | + self.access_token = removeSecurityProxy( |
554 | + self.consumer.getAccessToken(oauth_access_key)) |
555 | logout() |
556 | else: |
557 | self.consumer = None |
558 | @@ -694,7 +700,11 @@ |
559 | consumer = oacs.new(consumer_key) |
560 | request_token = consumer.newRequestToken() |
561 | request_token.review(person, permission, context) |
562 | - access_token = request_token.createAccessToken() |
563 | + # The security proxy assumes that an OAuth token has been |
564 | + # associated with the current HTTP request. Since we're using an |
565 | + # OAuth token to construct the HTTP requests in the first place, |
566 | + # we need to strip the security proxy. |
567 | + access_token = removeSecurityProxy(request_token.createAccessToken()) |
568 | logout() |
569 | return LaunchpadWebServiceCaller(consumer_key, access_token.key) |
570 | |
571 | |
572 | === added file 'lib/canonical/launchpad/tests/test_webservice_oauth.py' |
573 | --- lib/canonical/launchpad/tests/test_webservice_oauth.py 1970-01-01 00:00:00 +0000 |
574 | +++ lib/canonical/launchpad/tests/test_webservice_oauth.py 2010-09-02 21:17:47 +0000 |
575 | @@ -0,0 +1,82 @@ |
576 | +# Copyright 2010 Canonical Ltd. This software is licensed under the |
577 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
578 | + |
579 | +__metaclass__ = type |
580 | + |
581 | +from datetime import date |
582 | +import simplejson |
583 | + |
584 | +from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller |
585 | +from canonical.testing import LaunchpadFunctionalLayer |
586 | +from lp.testing import ( |
587 | + launchpadlib_for, |
588 | + TestCase, |
589 | +) |
590 | + |
591 | +class TestWebServiceAccessToOAuth(TestCase): |
592 | + """Tests of OAuth tokens as published in the web service.""" |
593 | + |
594 | + layer = LaunchpadFunctionalLayer |
595 | + |
596 | + def setUp(self): |
597 | + """Setup.""" |
598 | + super(TestWebServiceAccessToOAuth, self).setUp() |
599 | + self.launchpad = launchpadlib_for( |
600 | + 'Token management test', 'salgado', 'GRANT_PERMISSIONS') |
601 | + |
602 | + def test_authorized_read(self): |
603 | + """You can read tokens if your client has the right access level.""" |
604 | + # The person 'salgado' has three associated tokens: two |
605 | + # created as part of sample data, and one ("Grant |
606 | + # Permissions") created in the launchpadlib_for() call |
607 | + # in setUp(). |
608 | + my_tokens = [token.permission for token in |
609 | + self.launchpad.me.oauth_access_tokens] |
610 | + self.assertEquals( |
611 | + sorted(my_tokens), [u'Change Anything', u'Grant Permissions', |
612 | + u'Read Non-Private Data']) |
613 | + |
614 | + def test_authorized_write(self): |
615 | + """You can write tokens if your client has the right access level.""" |
616 | + # Change a token's expiration date. |
617 | + token = [token for token in self.launchpad.me.oauth_access_tokens |
618 | + if token.permission == "Read Non-Private Data"][0] |
619 | + old_date_expires = token.date_expires |
620 | + new_date_expires = str(date(9000, 1, 1)) |
621 | + token.date_expires = new_date_expires |
622 | + token.lp_save() |
623 | + |
624 | + # Change it back. |
625 | + token.date_expires = old_date_expires |
626 | + token.lp_save() |
627 | + |
628 | + def test_others_tokens_are_invisible(self): |
629 | + """No one else can see your tokens.""" |
630 | + someone_else = launchpadlib_for( |
631 | + 'Token management test', 'name12', 'GRANT_PERMISSIONS') |
632 | + tokens = [token for token in |
633 | + someone_else.people['salgado'].oauth_access_tokens] |
634 | + self.assertEquals(tokens, []) |
635 | + |
636 | + def test_insufficient_access_level(self): |
637 | + """A client can't read your tokens without GRANT_PERMISSIONS.""" |
638 | + write_private = launchpadlib_for( |
639 | + 'Token management test', 'salgado', 'WRITE_PRIVATE') |
640 | + my_tokens = [token for token in |
641 | + write_private.me.oauth_access_tokens] |
642 | + self.assertEquals(my_tokens, []) |
643 | + |
644 | + def test_url_hacking_doesnt_work(self): |
645 | + """You can't access tokens by URL hacking.""" |
646 | + webservice = LaunchpadWebServiceCaller( |
647 | + 'launchpad-library', 'salgado-change-anything') |
648 | + |
649 | + # A read request gets a 401. |
650 | + url = '/~salgado/+oauth-access-token/salgado-read-nonprivate' |
651 | + response = webservice.get(url) |
652 | + self.assertEqual(response.status, 401) |
653 | + |
654 | + # A write request gets a 401. |
655 | + body = simplejson.dumps(dict(date_expires=str(date(9000, 1, 1)))) |
656 | + response = webservice.patch(url, 'application/json', body) |
657 | + self.assertEqual(response.status, 401) |
658 | |
659 | === modified file 'lib/canonical/launchpad/webapp/authentication.py' |
660 | --- lib/canonical/launchpad/webapp/authentication.py 2010-08-20 20:31:18 +0000 |
661 | +++ lib/canonical/launchpad/webapp/authentication.py 2010-09-02 21:17:47 +0000 |
662 | @@ -363,24 +363,19 @@ |
663 | return request.form |
664 | |
665 | |
666 | -def check_oauth_signature(request, consumer, token): |
667 | +def check_oauth_signature(request, consumer, token_secret): |
668 | """Check that the given OAuth request is correctly signed. |
669 | |
670 | If the signature is incorrect or its method is not supported, set the |
671 | appropriate status in the request's response and return False. |
672 | """ |
673 | authorization = get_oauth_authorization(request) |
674 | - |
675 | if authorization.get('oauth_signature_method') != 'PLAINTEXT': |
676 | # XXX: 2008-03-04, salgado: Only the PLAINTEXT method is supported |
677 | # now. Others will be implemented later. |
678 | request.response.setStatus(400) |
679 | return False |
680 | |
681 | - if token is not None: |
682 | - token_secret = token.secret |
683 | - else: |
684 | - token_secret = '' |
685 | expected_signature = "&".join([consumer.secret, token_secret]) |
686 | if expected_signature != authorization.get('oauth_signature'): |
687 | request.unauthorized(OAUTH_CHALLENGE) |
688 | |
689 | === modified file 'lib/canonical/launchpad/webapp/authorization.py' |
690 | --- lib/canonical/launchpad/webapp/authorization.py 2010-08-20 20:31:18 +0000 |
691 | +++ lib/canonical/launchpad/webapp/authorization.py 2010-09-02 21:17:47 +0000 |
692 | @@ -61,7 +61,8 @@ |
693 | lp_permission = getUtility(ILaunchpadPermission, permission) |
694 | if lp_permission.access_level == "write": |
695 | required_access_level = [ |
696 | - AccessLevel.WRITE_PUBLIC, AccessLevel.WRITE_PRIVATE] |
697 | + AccessLevel.WRITE_PUBLIC, AccessLevel.WRITE_PRIVATE, |
698 | + AccessLevel.GRANT_PERMISSIONS] |
699 | if access_level not in required_access_level: |
700 | return False |
701 | elif lp_permission.access_level == "read": |
702 | |
703 | === modified file 'lib/canonical/launchpad/webapp/servers.py' |
704 | --- lib/canonical/launchpad/webapp/servers.py 2010-08-28 03:23:19 +0000 |
705 | +++ lib/canonical/launchpad/webapp/servers.py 2010-09-02 21:17:47 +0000 |
706 | @@ -8,7 +8,6 @@ |
707 | __metaclass__ = type |
708 | |
709 | import cgi |
710 | -from datetime import datetime |
711 | import threading |
712 | import xmlrpclib |
713 | |
714 | @@ -70,11 +69,8 @@ |
715 | IWebServiceApplication, |
716 | ) |
717 | from canonical.launchpad.interfaces.oauth import ( |
718 | - ClockSkew, |
719 | IOAuthConsumerSet, |
720 | IOAuthSignedRequest, |
721 | - NonceAlreadyUsed, |
722 | - TimestampOrderingError, |
723 | ) |
724 | import canonical.launchpad.layers |
725 | from canonical.launchpad.webapp.adapter import ( |
726 | @@ -82,7 +78,6 @@ |
727 | RequestExpired, |
728 | ) |
729 | from canonical.launchpad.webapp.authentication import ( |
730 | - check_oauth_signature, |
731 | get_oauth_authorization, |
732 | ) |
733 | from canonical.launchpad.webapp.authorization import ( |
734 | @@ -1276,27 +1271,16 @@ |
735 | token = consumer.getAccessToken(token_key) |
736 | if token is None: |
737 | raise Unauthorized('Unknown access token (%s).' % token_key) |
738 | + |
739 | nonce = form.get('oauth_nonce') |
740 | timestamp = form.get('oauth_timestamp') |
741 | - try: |
742 | - token.checkNonceAndTimestamp(nonce, timestamp) |
743 | - except (NonceAlreadyUsed, TimestampOrderingError, ClockSkew), e: |
744 | - raise Unauthorized('Invalid nonce/timestamp: %s' % e) |
745 | - now = datetime.now(pytz.timezone('UTC')) |
746 | - if token.permission == OAuthPermission.UNAUTHORIZED: |
747 | - raise Unauthorized('Unauthorized token (%s).' % token.key) |
748 | - elif token.date_expires is not None and token.date_expires <= now: |
749 | - raise Unauthorized('Expired token (%s).' % token.key) |
750 | - elif not check_oauth_signature(request, consumer, token): |
751 | - raise Unauthorized('Invalid signature.') |
752 | - else: |
753 | - # Everything is fine, let's return the principal. |
754 | - pass |
755 | + # If there's a problem with the token, authorize() will raise |
756 | + # Unauthorized. |
757 | + (account_id, access_level, scope) = token.authorize( |
758 | + request, nonce, timestamp) |
759 | + principal = getUtility(IPlacelessLoginSource).getPrincipal( |
760 | + account_id, access_level=access_level, scope=scope) |
761 | alsoProvides(request, IOAuthSignedRequest) |
762 | - principal = getUtility(IPlacelessLoginSource).getPrincipal( |
763 | - token.person.account.id, access_level=token.permission, |
764 | - scope=token.context) |
765 | - |
766 | return principal |
767 | |
768 | |
769 | |
770 | === modified file 'lib/canonical/launchpad/webapp/tests/test_publication.py' |
771 | --- lib/canonical/launchpad/webapp/tests/test_publication.py 2010-08-20 20:31:18 +0000 |
772 | +++ lib/canonical/launchpad/webapp/tests/test_publication.py 2010-09-02 21:17:47 +0000 |
773 | @@ -27,6 +27,7 @@ |
774 | ) |
775 | from zope.publisher.interfaces import Retry |
776 | from zope.publisher.interfaces.browser import IBrowserRequest |
777 | +from zope.security.proxy import removeSecurityProxy |
778 | |
779 | from canonical.config import dbconfig |
780 | from canonical.launchpad.database.emailaddress import EmailAddress |
781 | @@ -254,7 +255,11 @@ |
782 | request_token = consumer.newRequestToken() |
783 | request_token.review( |
784 | person, permission=OAuthPermission.READ_PUBLIC, context=None) |
785 | - access_token = request_token.createAccessToken() |
786 | + # The security proxy protects against unauthorized requests to |
787 | + # access the OAuth token data. We're using the OAuth token |
788 | + # data to _create_ a request, so we can strip the security |
789 | + # proxy. |
790 | + access_token = removeSecurityProxy(request_token.createAccessToken()) |
791 | |
792 | # Use oauth.OAuthRequest just to generate a dictionary containing all |
793 | # the parameters we need to use in a valid OAuth request, using the |
794 | |
795 | === modified file 'lib/canonical/launchpad/zcml/oauth.zcml' |
796 | --- lib/canonical/launchpad/zcml/oauth.zcml 2009-07-13 18:15:02 +0000 |
797 | +++ lib/canonical/launchpad/zcml/oauth.zcml 2010-09-02 21:17:47 +0000 |
798 | @@ -26,6 +26,7 @@ |
799 | |
800 | <class class="canonical.launchpad.database.OAuthRequestToken"> |
801 | <allow interface="canonical.launchpad.interfaces.IOAuthRequestToken"/> |
802 | + <allow interface="canonical.launchpad.interfaces.IOAuthTokenPublic" /> |
803 | <require |
804 | permission="launchpad.Edit" |
805 | set_schema="canonical.launchpad.interfaces.IOAuthRequestToken"/> |
806 | @@ -39,7 +40,11 @@ |
807 | </securedutility> |
808 | |
809 | <class class="canonical.launchpad.database.OAuthAccessToken"> |
810 | - <allow interface="canonical.launchpad.interfaces.IOAuthAccessToken"/> |
811 | + <allow interface="canonical.launchpad.webapp.interfaces.ICanonicalUrlData" /> |
812 | + <allow interface="canonical.launchpad.interfaces.IOAuthAccessTokenPublic"/> |
813 | + <require |
814 | + permission="launchpad.View" |
815 | + interface="canonical.launchpad.interfaces.IOAuthAccessToken"/> |
816 | <require |
817 | permission="launchpad.Edit" |
818 | set_schema="canonical.launchpad.interfaces.IOAuthAccessToken"/> |
819 | |
820 | === modified file 'lib/lp/bugs/tests/test_bugs_webservice.py' |
821 | --- lib/lp/bugs/tests/test_bugs_webservice.py 2010-08-30 05:19:46 +0000 |
822 | +++ lib/lp/bugs/tests/test_bugs_webservice.py 2010-09-02 21:17:47 +0000 |
823 | @@ -10,6 +10,7 @@ |
824 | from BeautifulSoup import BeautifulSoup |
825 | from lazr.lifecycle.interfaces import IDoNotSnapshot |
826 | from simplejson import dumps |
827 | +from storm.store import Store |
828 | from testtools.matchers import ( |
829 | Equals, |
830 | LessThan, |
831 | @@ -149,6 +150,7 @@ |
832 | def test_attachments_query_counts_constant(self): |
833 | login(USER_EMAIL) |
834 | self.bug = self.factory.makeBug() |
835 | + store = Store.of(self.bug) |
836 | self.factory.makeBugAttachment(self.bug) |
837 | self.factory.makeBugAttachment(self.bug) |
838 | webservice = LaunchpadWebServiceCaller( |
839 | @@ -157,6 +159,9 @@ |
840 | collector.register() |
841 | self.addCleanup(collector.unregister) |
842 | url = '/bugs/%d/attachments' % self.bug.id |
843 | + # XXX [bug=619017] First request. |
844 | + store.flush() |
845 | + store.reset() |
846 | response = webservice.get(url) |
847 | self.assertThat(collector, HasQueryCount(LessThan(24))) |
848 | with_2_count = collector.count |
849 | @@ -164,6 +169,9 @@ |
850 | login(USER_EMAIL) |
851 | self.factory.makeBugAttachment(self.bug) |
852 | logout() |
853 | + # XXX [bug=619017] Second request. |
854 | + store.flush() |
855 | + store.reset() |
856 | response = webservice.get(url) |
857 | # XXX: Permit the second call to be == or less, because storm |
858 | # caching bugs (such as) https://bugs.launchpad.net/storm/+bug/619017 |
859 | |
860 | === modified file 'lib/lp/registry/browser/person.py' |
861 | --- lib/lp/registry/browser/person.py 2010-08-31 14:00:54 +0000 |
862 | +++ lib/lp/registry/browser/person.py 2010-09-02 21:17:47 +0000 |
863 | @@ -492,6 +492,15 @@ |
864 | return None |
865 | return email |
866 | |
867 | + @stepthrough('+oauth-access-token') |
868 | + def traverse_access_token(self, key): |
869 | + """Traverse to an OAuth access token on the webservice layer.""" |
870 | + matches = [token for token in self.context.oauth_access_tokens |
871 | + if token.key == key] |
872 | + if len(matches) > 0: |
873 | + return matches[0] |
874 | + return None |
875 | + |
876 | @stepthrough('+wikiname') |
877 | def traverse_wikiname(self, id): |
878 | """Traverse to this person's WikiNames on the webservice layer.""" |
879 | |
880 | === modified file 'lib/lp/registry/interfaces/person.py' |
881 | --- lib/lp/registry/interfaces/person.py 2010-08-22 03:09:51 +0000 |
882 | +++ lib/lp/registry/interfaces/person.py 2010-09-02 21:17:47 +0000 |
883 | @@ -103,6 +103,7 @@ |
884 | IHasMugshot, |
885 | IPrivacy, |
886 | ) |
887 | +from canonical.launchpad.interfaces.oauth import IOAuthAccessToken |
888 | from canonical.launchpad.interfaces.validation import validate_new_team_email |
889 | from canonical.launchpad.validators import LaunchpadValidationError |
890 | from canonical.launchpad.validators.email import email_validator |
891 | @@ -596,7 +597,10 @@ |
892 | # apart from here. |
893 | registrant = Attribute('The user who created this profile.') |
894 | |
895 | - oauth_access_tokens = Attribute(_("Non-expired access tokens")) |
896 | + oauth_access_tokens = exported( |
897 | + CollectionField( |
898 | + title=u"Non-expired access tokens", |
899 | + value_type=Reference(schema=IOAuthAccessToken))) |
900 | |
901 | oauth_request_tokens = Attribute(_("Non-expired request tokens")) |
902 | |
903 | |
904 | === modified file 'lib/lp/registry/stories/webservice/xx-person.txt' |
905 | --- lib/lp/registry/stories/webservice/xx-person.txt 2010-07-13 15:29:08 +0000 |
906 | +++ lib/lp/registry/stories/webservice/xx-person.txt 2010-09-02 21:17:47 +0000 |
907 | @@ -37,6 +37,7 @@ |
908 | memberships_details_collection_link: u'http://.../~salgado/memberships_details' |
909 | mugshot_link: u'http://.../~salgado/mugshot' |
910 | name: u'salgado' |
911 | + oauth_access_tokens_collection_link: u'http://.../~salgado/oauth_access_tokens' |
912 | open_membership_invitations_collection_link: u'http://.../~salgado/open_membership_invitations' |
913 | participants_collection_link: u'http://.../~salgado/participants' |
914 | ppas_collection_link: u'http://.../~salgado/ppas' |
915 | @@ -86,6 +87,7 @@ |
916 | memberships_details_collection_link: u'http://.../~ubuntu-team/memberships_details' |
917 | mugshot_link: u'http://.../~ubuntu-team/mugshot' |
918 | name: u'ubuntu-team' |
919 | + oauth_access_tokens_collection_link: u'http://.../~ubuntu-team/oauth_access_tokens' |
920 | open_membership_invitations_collection_link: u'http://.../~ubuntu-team/open_membership_invitations' |
921 | participants_collection_link: u'http://.../~ubuntu-team/participants' |
922 | ppas_collection_link: u'http://.../~ubuntu-team/ppas' |
923 | |
924 | === modified file 'lib/lp/testing/_webservice.py' |
925 | --- lib/lp/testing/_webservice.py 2010-08-20 20:31:18 +0000 |
926 | +++ lib/lp/testing/_webservice.py 2010-09-02 21:17:47 +0000 |
927 | @@ -24,6 +24,7 @@ |
928 | from zope.app.publication.interfaces import IEndRequestEvent |
929 | from zope.app.testing import ztapi |
930 | from zope.component import getUtility |
931 | +from zope.security.proxy import removeSecurityProxy |
932 | import zope.testing.cleanup |
933 | |
934 | from canonical.launchpad.interfaces import ( |
935 | @@ -72,19 +73,23 @@ |
936 | # We didn't have to create the consumer. Maybe this user |
937 | # already has an access token for this |
938 | # consumer+person+permission? |
939 | - existing_token = [token for token in person.oauth_access_tokens |
940 | - if (token.consumer == consumer |
941 | - and token.permission == permission |
942 | - and token.context == context)] |
943 | - if len(existing_token) >= 1: |
944 | - return existing_token[0] |
945 | + for token in person.oauth_access_tokens: |
946 | + # The security proxy on OAuthAccessToken assumes an access |
947 | + # token has already been associated with the current |
948 | + # request. Since there is no current request, it doesn't |
949 | + # make sense to run those security checks. |
950 | + token = removeSecurityProxy(token) |
951 | + if (token.consumer == consumer |
952 | + and token.permission == permission |
953 | + and token.context == context): |
954 | + return token |
955 | |
956 | # There is no existing access token for this |
957 | # consumer+person+permission+context. Create one and review it. |
958 | request_token = consumer.newRequestToken() |
959 | request_token.review(person, permission, context) |
960 | access_token = request_token.createAccessToken() |
961 | - return access_token |
962 | + return removeSecurityProxy(access_token) |
963 | |
964 | |
965 | def launchpadlib_credentials_for( |
966 | @@ -109,8 +114,7 @@ |
967 | access_token = oauth_access_token_for( |
968 | consumer_name, person, permission, context) |
969 | logout() |
970 | - launchpadlib_token = AccessToken( |
971 | - access_token.key, access_token.secret) |
972 | + launchpadlib_token = AccessToken(access_token.key, access_token.secret) |
973 | return Credentials(consumer_name=consumer_name, |
974 | access_token=launchpadlib_token) |
975 | |
976 | |
977 | === modified file 'versions.cfg' |
978 | --- versions.cfg 2010-09-01 08:41:40 +0000 |
979 | +++ versions.cfg 2010-09-02 21:17:47 +0000 |
980 | @@ -31,7 +31,7 @@ |
981 | lazr.delegates = 1.2.0 |
982 | lazr.enum = 1.1.2 |
983 | lazr.lifecycle = 1.1 |
984 | -lazr.restful = 0.11.3 |
985 | +lazr.restful = 0.12.0 |
986 | lazr.restfulclient = 0.10.0 |
987 | lazr.smtptest = 1.1 |
988 | lazr.testing = 0.1.1 |
lib/canonical/ launchpad/ pagetests/ webservice/ oauth.txt and most of the changes to lib/canonical/ launchpad/ doc/oauth. txt look like their primary purpose is to provide test coverage, not to provide documentation, so please write them as UnitTests.
A lot of the formatting of line-wraps doesn't match our official style: https:/ /dev.launchpad. net/PythonStyle Guide#Whitespac e%20and% 20Wrapping and often matches the "Do not indent your code like this" style. Please fix.
It appears that the removeSecurityProxy in lib/canonical/ launchpad/ webapp/ servers. py could be eliminated by moving some of the code into a method on OAuthAccessToken. Please try this, and if it's not possible, provide a follow-up comment explaining this.