launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00816
[Merge] lp:~leonardr/launchpad/publish-tokens-2 into lp:launchpad
Leonard Richardson has proposed merging lp:~leonardr/launchpad/publish-tokens-2 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
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_for_user and tests in which there is no current request. Please check this code to make sure I haven't opened a security hole.
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.
--
https://code.launchpad.net/~leonardr/launchpad/publish-tokens-2/+merge/34123
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~leonardr/launchpad/publish-tokens-2 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/database/oauth.py'
--- lib/canonical/launchpad/database/oauth.py 2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/database/oauth.py 2010-08-30 19:49:45 +0000
@@ -46,6 +46,7 @@
)
from canonical.launchpad.webapp.interfaces import (
AccessLevel,
+ ICanonicalUrlData,
IStoreSelector,
MAIN_STORE,
MASTER_FLAVOR,
@@ -136,7 +137,17 @@
class OAuthAccessToken(OAuthBase):
"""See `IOAuthAccessToken`."""
- implements(IOAuthAccessToken)
+ implements(IOAuthAccessToken, ICanonicalUrlData)
+
+ @property
+ def inside(self):
+ return self.person
+
+ @property
+ def path(self):
+ return "+oauth-access-token/" + self.key
+
+ rootsite = 'api'
consumer = ForeignKey(
dbName='consumer', foreignKey='OAuthConsumer', notNull=True)
=== modified file 'lib/canonical/launchpad/doc/oauth.txt'
--- lib/canonical/launchpad/doc/oauth.txt 2010-04-16 15:06:55 +0000
+++ lib/canonical/launchpad/doc/oauth.txt 2010-08-30 19:49:45 +0000
@@ -186,6 +186,13 @@
True
>>> request_token.permission
<DBItem OAuthPermission.WRITE_PUBLIC...
+
+Because an access token can only be viewed or edited by its owner or
+an admin, we need to log in at this point.
+
+ >>> token_owner = request_token.person
+ >>> login_person(token_owner)
+
>>> access_token = request_token.createAccessToken()
>>> verifyObject(IOAuthAccessToken, access_token)
True
@@ -245,14 +252,30 @@
>>> consumer.getAccessToken(access_token.key) == access_token
True
-An access token can only be changed by the person associated with it.
-
- >>> access_token.permission = OAuthPermission.WRITE_PUBLIC
- Traceback (most recent call last):
- ...
- Unauthorized:...
- >>> login_person(access_token.person)
- >>> access_token.permission = AccessLevel.WRITE_PUBLIC
+In general, one user cannot edit another user's access token.
+
+ >>> login('no-priv@xxxxxxxxxxxxx')
+ >>> access_token.permission
+ Traceback (most recent call last):
+ ...
+ Unauthorized:...
+
+ >>> access_token.permission = AccessLevel.WRITE_PUBLIC
+ Traceback (most recent call last):
+ ...
+ Unauthorized:...
+
+An admin can view and edit someone else's access token.
+
+ >>> login("foo.bar@xxxxxxxxxxxxx")
+ >>> print access_token.permission.name
+ WRITE_PUBLIC
+ >>> access_token.permission = AccessLevel.WRITE_PUBLIC
+
+And a user can edit their own access tokens.
+
+ >>> login_person(token_owner)
+ >>> access_token.permission = AccessLevel.WRITE_PRIVATE
From any given person it's possible to retrieve his non-expired access
tokens.
@@ -305,6 +328,7 @@
- We can use a nonce.
+ >>> login_person(token_owner)
>>> import time
>>> now = time.time() - 1
>>> nonce1 = access_token.checkNonceAndTimestamp('boo', now)
=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2010-08-27 11:19:54 +0000
+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2010-08-30 19:49:45 +0000
@@ -32,6 +32,7 @@
IMessage,
IUserToUserEmail,
)
+from canonical.launchpad.interfaces.oauth import IOAuthToken
from lp.blueprints.interfaces.specification import ISpecification
from lp.blueprints.interfaces.specificationbranch import ISpecificationBranch
from lp.bugs.interfaces.bug import (
@@ -196,6 +197,8 @@
patch_choice_parameter_type(
IHasBugs, 'searchTasks', 'hardware_bus', HWBus)
+IOAuthToken['person'].schema = IPerson
+
IPreviewDiff['branch_merge_proposal'].schema = IBranchMergeProposal
patch_reference_property(IPersonPublic, 'archive', IArchive)
=== modified file 'lib/canonical/launchpad/interfaces/oauth.py'
--- lib/canonical/launchpad/interfaces/oauth.py 2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/interfaces/oauth.py 2010-08-30 19:49:45 +0000
@@ -34,12 +34,14 @@
TextLine,
)
+from lazr.restful.declarations import (
+ export_as_webservice_entry, exported)
+
from canonical.launchpad import _
from canonical.launchpad.webapp.interfaces import (
AccessLevel,
OAuthPermission,
)
-from lp.registry.interfaces.person import IPerson
# The challenge included in responses with a 401 status.
OAUTH_REALM = 'https://api.launchpad.net'
@@ -128,23 +130,24 @@
schema=IOAuthConsumer, title=_('The consumer.'),
description=_("The consumer which will access Launchpad on the "
"user's behalf."))
- person = Object(
- schema=IPerson, title=_('Person'), required=False, readonly=False,
- description=_('The user on whose behalf the consumer is accessing.'))
- date_created = Datetime(
- title=_('Date created'), required=True, readonly=True)
- date_expires = Datetime(
+ person = exported(Object(
+ schema=Interface, title=_('Person'), required=False, readonly=False,
+ description=_('The user on whose behalf the consumer is accessing.')),
+ readonly=True)
+ date_created = exported(Datetime(
+ title=_('Date created'), required=True, readonly=True))
+ date_expires = exported(Datetime(
title=_('Date expires'), required=False, readonly=False,
description=_('From this date onwards this token can not be used '
- 'by the consumer to access protected resources.'))
- key = TextLine(
+ 'by the consumer to access protected resources.')))
+ key = exported(TextLine(
title=_('Key'), required=True, readonly=True,
description=_('The key used to identify this token. It is included '
- 'by the consumer in each request.'))
- secret = TextLine(
+ 'by the consumer in each request.')))
+ secret = exported(TextLine(
title=_('Secret'), required=True, readonly=True,
description=_('The secret associated with this token. It is used '
- 'by the consumer to sign its requests.'))
+ 'by the consumer to sign its requests.')))
product = Choice(title=_('Project'), required=False, vocabulary='Product')
project = Choice(
title=_('Project'), required=False, vocabulary='ProjectGroup')
@@ -161,12 +164,14 @@
It's created automatically once a user logs in and grants access to a
consumer. The consumer then exchanges an `IOAuthRequestToken` for it.
"""
+ export_as_webservice_entry()
- permission = Choice(
+ permission = exported(Choice(
title=_('Access level'), required=True, readonly=False,
vocabulary=AccessLevel,
description=_('The level of access given to the application acting '
- 'on your behalf.'))
+ 'on your behalf.')),
+ readonly=True)
def checkNonceAndTimestamp(nonce, timestamp):
"""Verify the nonce and timestamp.
=== added file 'lib/canonical/launchpad/pagetests/webservice/oauth.txt'
--- lib/canonical/launchpad/pagetests/webservice/oauth.txt 1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/pagetests/webservice/oauth.txt 2010-08-30 19:49:45 +0000
@@ -0,0 +1,65 @@
+**********************
+OAuth token management
+**********************
+
+A properly authorized web service client can examine and manage the
+OAuth access tokens used to manage access to the web service itself.
+
+ >>> launchpad = launchpadlib_for(
+ ... 'Token management test', 'salgado', 'GRANT_PERMISSIONS')
+
+The person 'salgado' has three associated tokens: two created as part
+of sample data, and one ("Grant Permissions") created in the
+launchpadlib_for() call immediately above.
+
+ >>> my_tokens = launchpad.me.oauth_access_tokens
+ >>> sorted([token.permission for token in my_tokens])
+ [u'Change Anything', u'Grant Permissions', u'Read Non-Private Data']
+
+You can change certain details of an existing OAuth access
+token. Let's set a token's expiration date to a date in the far
+future.
+
+ >>> from datetime import date
+ >>> token = [token for token in my_tokens
+ ... if token.permission == "Read Non-Private Data"][0]
+ >>> old_date_expires = token.date_expires
+ >>> new_date_expires = str(date(9000, 1, 1))
+ >>> token.date_expires = new_date_expires
+ >>> token.lp_save()
+
+Now let's change it back.
+
+ >>> token.date_expires = old_date_expires
+ >>> token.lp_save()
+
+You can't see another person's OAuth tokens.
+
+ >>> someone_else = launchpadlib_for(
+ ... 'Token management test', 'name12', 'GRANT_PERMISSIONS')
+
+ >>> [x for x in someone_else.people['salgado'].oauth_access_tokens]
+ []
+
+If you haven't given your client the GRANT_PERMISSIONS access level,
+you can't see your own OAuth tokens.
+
+ >>> low_access = launchpadlib_for(
+ ... 'Token management test', 'salgado', 'WRITE_PRIVATE')
+ >>> [x for x in low_access.me.oauth_access_tokens]
+ []
+
+Hacking the URL from an underprivileged account (such as the
+'webservice' object, which has WRITE_PRIVATE) won't get you
+anything.
+
+ >>> url = '/~salgado/+oauth-access-token/salgado-read-nonprivate'
+ >>> print webservice.get(url)
+ HTTP/1.1 401 Unauthorized
+ ...
+
+ >>> import simplejson
+ >>> body = simplejson.dumps(dict(date_expires=new_date_expires))
+ >>> print webservice.patch(url, 'application/json', body)
+ HTTP/1.1 401 Unauthorized
+ ...
=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py 2010-08-27 23:01:42 +0000
+++ lib/canonical/launchpad/security.py 2010-08-30 19:49:45 +0000
@@ -33,12 +33,15 @@
from canonical.launchpad.interfaces.oauth import (
IOAuthAccessToken,
IOAuthRequestToken,
+ IOAuthSignedRequest,
)
from canonical.launchpad.webapp.authorization import check_permission
from canonical.launchpad.webapp.interfaces import (
+ AccessLevel,
IAuthorization,
ILaunchpadRoot,
)
+from canonical.lazr.utils import get_current_browser_request
from lp.answers.interfaces.faq import IFAQ
from lp.answers.interfaces.faqtarget import IFAQTarget
from lp.answers.interfaces.question import IQuestion
@@ -373,15 +376,42 @@
return user.in_admin or user.in_registry_experts
-class EditOAuthAccessToken(AuthorizationBase):
- permission = 'launchpad.Edit'
- usedfor = IOAuthAccessToken
+class OAuthToken(AuthorizationBase):
+ """Special rules for access to OAuth tokens.
+
+ Through the website, a person can always access their own OAuth
+ tokens, and an admin can access someone else's OAuth tokens.
+
+ To minimize the risk of privilege escalation attacks, much greater
+ restrictions apply through the web service. A person can only
+ access their own OAuth tokens, and they must be using a client
+ authorized at the GRANT_PERMISSIONS access level.
+ """
def checkAuthenticated(self, user):
- return self.obj.person == user.person or user.in_admin
-
-
-class EditOAuthRequestToken(EditOAuthAccessToken):
+ request = get_current_browser_request()
+ if IOAuthSignedRequest.providedBy(request):
+ # This is a web service request.
+ if self.obj.person != user.person:
+ return False
+ return (request.principal.access_level ==
+ AccessLevel.GRANT_PERMISSIONS)
+ else:
+ # This is a website request.
+ return self.obj.person == user.person or user.in_admin
+
+
+class ViewOAuthAccessToken(OAuthToken):
+ permission = 'launchpad.View'
+ usedfor = IOAuthAccessToken
+
+
+class EditOAuthAccessToken(OAuthToken):
+ permission = 'launchpad.Edit'
+ usedfor = IOAuthAccessToken
+
+
+class EditOAuthRequestToken(OAuthToken):
permission = 'launchpad.Edit'
usedfor = IOAuthRequestToken
=== modified file 'lib/canonical/launchpad/testing/pages.py'
--- lib/canonical/launchpad/testing/pages.py 2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/testing/pages.py 2010-08-30 19:49:45 +0000
@@ -153,10 +153,16 @@
self.consumer = OAuthConsumer(oauth_consumer_key, '')
self.access_token = OAuthToken(oauth_access_key, '')
else:
- # The client wants to make an authorized request
- # using a recognized consumer key.
- self.access_token = self.consumer.getAccessToken(
- oauth_access_key)
+ # The client wants to make authorized requests
+ # using a recognized consumer key. The security
+ # policy for IOAuthAccessToken assumes the request
+ # is being processed on the server side and has
+ # already been signed with some access token.
+ #
+ # Here, the requests haven't even gone out yet,
+ # so it's okay to strip the security proxy.
+ self.access_token = removeSecurityProxy(
+ self.consumer.getAccessToken(oauth_access_key))
logout()
else:
self.consumer = None
@@ -694,7 +700,11 @@
consumer = oacs.new(consumer_key)
request_token = consumer.newRequestToken()
request_token.review(person, permission, context)
- access_token = request_token.createAccessToken()
+ # The security proxy assumes that an OAuth token has been
+ # associated with the current HTTP request. Since we're using an
+ # OAuth token to construct the HTTP requests in the first place,
+ # we need to strip the security proxy.
+ access_token = removeSecurityProxy(request_token.createAccessToken())
logout()
return LaunchpadWebServiceCaller(consumer_key, access_token.key)
=== modified file 'lib/canonical/launchpad/webapp/authorization.py'
--- lib/canonical/launchpad/webapp/authorization.py 2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/webapp/authorization.py 2010-08-30 19:49:45 +0000
@@ -61,7 +61,8 @@
lp_permission = getUtility(ILaunchpadPermission, permission)
if lp_permission.access_level == "write":
required_access_level = [
- AccessLevel.WRITE_PUBLIC, AccessLevel.WRITE_PRIVATE]
+ AccessLevel.WRITE_PUBLIC, AccessLevel.WRITE_PRIVATE,
+ AccessLevel.GRANT_PERMISSIONS]
if access_level not in required_access_level:
return False
elif lp_permission.access_level == "read":
=== modified file 'lib/canonical/launchpad/webapp/servers.py'
--- lib/canonical/launchpad/webapp/servers.py 2010-08-28 03:23:19 +0000
+++ lib/canonical/launchpad/webapp/servers.py 2010-08-30 19:49:45 +0000
@@ -1276,6 +1276,12 @@
token = consumer.getAccessToken(token_key)
if token is None:
raise Unauthorized('Unknown access token (%s).' % token_key)
+
+ # The security policy for IOAuthAccessToken assumes that the
+ # access level of the request has already been
+ # identified. Since identifying the access level is what we're
+ # trying to do here, we need to strip the security policy.
+ token = removeSecurityProxy(token)
nonce = form.get('oauth_nonce')
timestamp = form.get('oauth_timestamp')
try:
=== modified file 'lib/canonical/launchpad/zcml/oauth.zcml'
--- lib/canonical/launchpad/zcml/oauth.zcml 2009-07-13 18:15:02 +0000
+++ lib/canonical/launchpad/zcml/oauth.zcml 2010-08-30 19:49:45 +0000
@@ -39,7 +39,10 @@
</securedutility>
<class class="canonical.launchpad.database.OAuthAccessToken">
- <allow interface="canonical.launchpad.interfaces.IOAuthAccessToken"/>
+ <allow interface="canonical.launchpad.webapp.interfaces.ICanonicalUrlData" />
+ <require
+ permission="launchpad.View"
+ interface="canonical.launchpad.interfaces.IOAuthAccessToken"/>
<require
permission="launchpad.Edit"
set_schema="canonical.launchpad.interfaces.IOAuthAccessToken"/>
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2010-08-30 06:38:53 +0000
+++ lib/lp/registry/browser/person.py 2010-08-30 19:49:45 +0000
@@ -488,6 +488,15 @@
return None
return email
+ @stepthrough('+oauth-access-token')
+ def traverse_access_token(self, key):
+ """Traverse to an OAuth access token on the webservice layer."""
+ matches = [token for token in self.context.oauth_access_tokens
+ if token.key == key]
+ if len(matches) > 0:
+ return matches[0]
+ return None
+
@stepthrough('+wikiname')
def traverse_wikiname(self, id):
"""Traverse to this person's WikiNames on the webservice layer."""
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2010-08-22 03:09:51 +0000
+++ lib/lp/registry/interfaces/person.py 2010-08-30 19:49:45 +0000
@@ -103,6 +103,7 @@
IHasMugshot,
IPrivacy,
)
+from canonical.launchpad.interfaces.oauth import IOAuthAccessToken
from canonical.launchpad.interfaces.validation import validate_new_team_email
from canonical.launchpad.validators import LaunchpadValidationError
from canonical.launchpad.validators.email import email_validator
@@ -596,7 +597,9 @@
# apart from here.
registrant = Attribute('The user who created this profile.')
- oauth_access_tokens = Attribute(_("Non-expired access tokens"))
+ oauth_access_tokens = exported(CollectionField(
+ title=u"Non-expired access tokens",
+ value_type=Reference(schema=IOAuthAccessToken)))
oauth_request_tokens = Attribute(_("Non-expired request tokens"))
=== modified file 'lib/lp/registry/stories/webservice/xx-person.txt'
--- lib/lp/registry/stories/webservice/xx-person.txt 2010-07-13 15:29:08 +0000
+++ lib/lp/registry/stories/webservice/xx-person.txt 2010-08-30 19:49:45 +0000
@@ -37,6 +37,7 @@
memberships_details_collection_link: u'http://.../~salgado/memberships_details'
mugshot_link: u'http://.../~salgado/mugshot'
name: u'salgado'
+ oauth_access_tokens_collection_link: u'http://.../~salgado/oauth-access-tokens'
open_membership_invitations_collection_link: u'http://.../~salgado/open_membership_invitations'
participants_collection_link: u'http://.../~salgado/participants'
ppas_collection_link: u'http://.../~salgado/ppas'
@@ -86,6 +87,7 @@
memberships_details_collection_link: u'http://.../~ubuntu-team/memberships_details'
mugshot_link: u'http://.../~ubuntu-team/mugshot'
name: u'ubuntu-team'
+ oauth_access_tokens_collection_link: u'http://.../~ubuntu-team/oauth-access-tokens'
open_membership_invitations_collection_link: u'http://.../~ubuntu-team/open_membership_invitations'
participants_collection_link: u'http://.../~ubuntu-team/participants'
ppas_collection_link: u'http://.../~ubuntu-team/ppas'
=== modified file 'lib/lp/testing/_webservice.py'
--- lib/lp/testing/_webservice.py 2010-08-20 20:31:18 +0000
+++ lib/lp/testing/_webservice.py 2010-08-30 19:49:45 +0000
@@ -24,6 +24,7 @@
from zope.app.publication.interfaces import IEndRequestEvent
from zope.app.testing import ztapi
from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
import zope.testing.cleanup
from canonical.launchpad.interfaces import (
@@ -72,19 +73,23 @@
# We didn't have to create the consumer. Maybe this user
# already has an access token for this
# consumer+person+permission?
- existing_token = [token for token in person.oauth_access_tokens
- if (token.consumer == consumer
- and token.permission == permission
- and token.context == context)]
- if len(existing_token) >= 1:
- return existing_token[0]
+ for token in person.oauth_access_tokens:
+ # The security proxy on OAuthAccessToken assumes an access
+ # token has already been associated with the current
+ # request. Since there is no current request, it doesn't
+ # make sense to run those security checks.
+ token = removeSecurityProxy(token)
+ if (token.consumer == consumer
+ and token.permission == permission
+ and token.context == context):
+ return token
# There is no existing access token for this
# consumer+person+permission+context. Create one and review it.
request_token = consumer.newRequestToken()
request_token.review(person, permission, context)
access_token = request_token.createAccessToken()
- return access_token
+ return removeSecurityProxy(access_token)
def launchpadlib_credentials_for(
@@ -109,8 +114,7 @@
access_token = oauth_access_token_for(
consumer_name, person, permission, context)
logout()
- launchpadlib_token = AccessToken(
- access_token.key, access_token.secret)
+ launchpadlib_token = AccessToken(access_token.key, access_token.secret)
return Credentials(consumer_name=consumer_name,
access_token=launchpadlib_token)
=== modified file 'versions.cfg'
--- versions.cfg 2010-08-27 02:21:01 +0000
+++ versions.cfg 2010-08-30 19:49:45 +0000
@@ -31,7 +31,7 @@
lazr.delegates = 1.2.0
lazr.enum = 1.1.2
lazr.lifecycle = 1.1
-lazr.restful = 0.11.3
+lazr.restful = 0.12.0
lazr.restfulclient = 0.10.0
lazr.smtptest = 1.1
lazr.testing = 0.1.1
