Merge lp:~leonardr/launchpad/publish-tokens-2 into lp:launchpad/db-devel

[Leonard Richardson]

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.

[Aaron Bentley (abentley)]

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/PythonStyleGuide#Whitespace%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.

review: Needs Fixing

[lp:~leonardr/launchpad/publish-tokens-2]

11452. By Leonard Richardson on 2010-08-31

Converted oauth.txt into a unit test.

11453. By Leonard Richardson on 2010-08-31

Got the new unit tests to pass.

11454. By Leonard Richardson on 2010-08-31

Fixed indentation.

11455. By Leonard Richardson on 2010-08-31

Moved all the code requiring access to OAuthAccessToken internals into OAuthAccessToken.

[Guilherme Salgado (salgado)]

Leonard asked me to do a follow up review of http://pastebin.ubuntu.com/486435/:

The signature of check_oauth_signature() was changed and I see only one callsite changed, so if we indeed have only one callsite I think it might make sense to make the new argument (token_secret) required as it's always provided in that callsite.

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, IFooEditRestricted)?

review: Approve (code)

[lp:~leonardr/launchpad/publish-tokens-2]

11456. By Leonard Richardson on 2010-09-01

Merge with trunk.

11457. By Leonard Richardson on 2010-09-01

Fixed test failures.

11458. By Leonard Richardson on 2010-09-01

Renamed the *Verification interfaces to *Public to comply with existing naming standards.

[Guilherme Salgado (salgado)]

And here's another follow up review: http://pastebin.ubuntu.com/486932/

+class OAuthTokenBase(OAuthBase):
+ """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(self, request):
+ return check_oauth_signature(request, self.consumer, self.secret)

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.createAccessToken()
+ >>> access_token = removeSecurityProxy(token.createAccessToken())

Why do you need to remove the security proxy now?

review: Needs Information (code)

[lp:~leonardr/launchpad/publish-tokens-2]

11459. By Leonard Richardson on 2010-09-01

Corrected permissions.

[Leonard Richardson (leonardr)]

> -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.createAccessToken()
> + >>> access_token = removeSecurityProxy(token.createAccessToken())
>
> 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)]

review: Approve (code)

[lp:~leonardr/launchpad/publish-tokens-2]

11460. By Leonard Richardson on 2010-09-01

Fixed typo and added docstring.

11461. By Leonard Richardson on 2010-09-02

I wasn't paying attention when I fixed this test failure, and I 'fixed' something that was right the first time.

11462. By Leonard Richardson on 2010-09-02

Changed the text to explain why I changed the code.

11463. By Leonard Richardson on 2010-09-02

Aand... make the change to the test that apparently I didn't make before.

11464. By Leonard Richardson on 2010-09-02

Strip security proxy in one more test.

11465. By Leonard Richardson on 2010-09-02

Added workaround for bug 619017 discovered by jcsackett.

Unmerged revisions

11465. By Leonard Richardson on 2010-09-02

Added workaround for bug 619017 discovered by jcsackett.

11464. By Leonard Richardson on 2010-09-02

Strip security proxy in one more test.

11463. By Leonard Richardson on 2010-09-02

Aand... make the change to the test that apparently I didn't make before.

11462. By Leonard Richardson on 2010-09-02

Changed the text to explain why I changed the code.

11461. By Leonard Richardson on 2010-09-02

I wasn't paying attention when I fixed this test failure, and I 'fixed' something that was right the first time.

11460. By Leonard Richardson on 2010-09-01

Fixed typo and added docstring.

11459. By Leonard Richardson on 2010-09-01

Corrected permissions.

11458. By Leonard Richardson on 2010-09-01

Renamed the *Verification interfaces to *Public to comply with existing naming standards.

11457. By Leonard Richardson on 2010-09-01

Fixed test failures.

11456. By Leonard Richardson on 2010-09-01

Merge with trunk.

=== modified file 'lib/canonical/launchpad/browser/oauth.py'
--- lib/canonical/launchpad/browser/oauth.py	2010-08-24 16:44:42 +0000
+++ lib/canonical/launchpad/browser/oauth.py	2010-09-02 21:17:47 +0000
@@ -83,7 +83,7 @@
         if consumer is None:
             consumer = consumer_set.new(key=consumer_key)
 
-        if not check_oauth_signature(self.request, consumer, None):
+        if not check_oauth_signature(self.request, consumer, ''):
             return u''
 
         token = consumer.newRequestToken()
@@ -284,13 +284,12 @@
             self.request.unauthorized(OAUTH_CHALLENGE)
             return u'No request token specified.'
 
-        if not check_oauth_signature(self.request, consumer, token):
+        if not token.checkSignature(self.request):
             return u'Invalid OAuth signature.'
 
         if not token.is_reviewed:
             self.request.unauthorized(OAUTH_CHALLENGE)
             return u'Request token has not yet been reviewed. Try again later.'
-
         if token.permission == OAuthPermission.UNAUTHORIZED:
             # The end-user explicitly refused to authorize this
             # token. We send 403 ("Forbidden") instead of 401
@@ -301,9 +300,4 @@
             return u'End-user refused to authorize request token.'
 
         access_token = token.createAccessToken()
-        context_name = None
-        if access_token.context is not None:
-            context_name = access_token.context.name
-        body = u'oauth_token=%s&oauth_token_secret=%s&lp.context=%s' % (
-            access_token.key, access_token.secret, context_name)
-        return body
+        return access_token.form_encoded

=== 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-09-02 21:17:47 +0000
@@ -24,6 +24,7 @@
 from storm.expr import And
 from zope.component import getUtility
 from zope.interface import implements
+from zope.security.interfaces import Unauthorized
 
 from canonical.database.constants import UTC_NOW
 from canonical.database.datetimecol import UtcDateTimeCol
@@ -36,21 +37,25 @@
 from canonical.launchpad.interfaces import (
     ClockSkew,
     IOAuthAccessToken,
+    IOAuthAccessTokenPublic,
     IOAuthConsumer,
     IOAuthConsumerSet,
     IOAuthNonce,
     IOAuthRequestToken,
     IOAuthRequestTokenSet,
+    IOAuthTokenPublic,
     NonceAlreadyUsed,
     TimestampOrderingError,
     )
 from canonical.launchpad.webapp.interfaces import (
     AccessLevel,
+    ICanonicalUrlData,
     IStoreSelector,
     MAIN_STORE,
     MASTER_FLAVOR,
     OAuthPermission,
     )
+from canonical.launchpad.webapp.authentication import check_oauth_signature
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.distributionsourcepackage import (
     IDistributionSourcePackage,
@@ -134,9 +139,28 @@
         return OAuthConsumer.selectOneBy(key=key)
 
 
-class OAuthAccessToken(OAuthBase):
+class OAuthTokenBase(OAuthBase):
+    """Base implementation of code for OAuth tokens."""
+
+    def checkSignature(self, request):
+        """See `IOAuthTokenPublic`."""
+        return check_oauth_signature(request, self.consumer, self.secret)
+
+
+class OAuthAccessToken(OAuthTokenBase):
     """See `IOAuthAccessToken`."""
-    implements(IOAuthAccessToken)
+    implements(
+        IOAuthAccessToken, IOAuthAccessTokenPublic, 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)
@@ -178,7 +202,7 @@
             return None
 
     def checkNonceAndTimestamp(self, nonce, timestamp):
-        """See `IOAuthAccessToken`."""
+        """See `IOAuthAccessTokenPublic`."""
         timestamp = float(timestamp)
         date = datetime.fromtimestamp(timestamp, pytz.UTC)
         # Determine if the timestamp is too far off from now.
@@ -209,10 +233,38 @@
         return OAuthNonce(
             access_token=self, nonce=nonce, request_timestamp=date)
 
-
-class OAuthRequestToken(OAuthBase):
+    def authorize(self, request, nonce, timestamp):
+        """See `IOAuthAccessTokenPublic`."""
+        try:
+            self.checkNonceAndTimestamp(nonce, timestamp)
+        except (NonceAlreadyUsed, TimestampOrderingError, ClockSkew), e:
+            raise Unauthorized('Invalid nonce/timestamp: %s' % e)
+        now = datetime.now(pytz.timezone('UTC'))
+        if self.permission == OAuthPermission.UNAUTHORIZED:
+            raise Unauthorized('Unauthorized token (%s).' % self.key)
+        elif self.date_expires is not None and self.date_expires <= now:
+            raise Unauthorized('Expired token (%s).' % self.key)
+        elif not self.checkSignature(request):
+            raise Unauthorized('Invalid signature.')
+        else:
+            # Everything is fine, let's return the principal.
+            pass
+        return (self.person.account.id, self.permission, self.context)
+
+    @property
+    def form_encoded(self):
+        """See `IOAuthAccessTokenPublic`."""
+        context_name = None
+        if self.context is not None:
+            context_name = self.context.name
+        body = u'oauth_token=%s&oauth_token_secret=%s&lp.context=%s' % (
+            self.key, self.secret, context_name)
+        return body
+
+
+class OAuthRequestToken(OAuthTokenBase):
     """See `IOAuthRequestToken`."""
-    implements(IOAuthRequestToken)
+    implements(IOAuthRequestToken, IOAuthTokenPublic)
 
     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-09-02 21:17:47 +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('[e-mail]')
+    >>> 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("[e-mail]")
+    >>> 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/doc/webapp-authorization.txt'
--- lib/canonical/launchpad/doc/webapp-authorization.txt	2010-08-24 16:44:42 +0000
+++ lib/canonical/launchpad/doc/webapp-authorization.txt	2010-09-02 21:17:47 +0000
@@ -79,9 +79,8 @@
     >>> check_permission('launchpad.View', bug_1)
     False
 
-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 level
+of permission equivalent to WRITE_PUBLIC.
 
     >>> principal.access_level = AccessLevel.GRANT_PERMISSIONS
     >>> setupInteraction(principal)
@@ -89,14 +88,7 @@
     False
 
     >>> check_permission('launchpad.Edit', sample_person)
-    False
-
-This may seem useless from a security standpoint, since once a
-malicious client is authorized to create OAuth tokens, it can escalate
-its privileges at any time by creating a new token for itself. The
-security benefit is more subtle: by discouraging feature creep in
-clients that have this super-access level, we reduce the risk that a
-bug in a _trusted_ client will enable privilege escalation attacks.
+    True
 
 Users logged in through the web application have full access, which
 means they can read/change any object they have access to.

=== modified file 'lib/canonical/launchpad/doc/webapp-publication.txt'
--- lib/canonical/launchpad/doc/webapp-publication.txt	2010-08-05 13:23:52 +0000
+++ lib/canonical/launchpad/doc/webapp-publication.txt	2010-09-02 21:17:47 +0000
@@ -1156,14 +1156,20 @@
 principal's access_level and scope will match what was specified in the
 token.
 
+First let's create a token.
+
     >>> from lp.registry.interfaces.product import IProductSet
+    >>> from zope.security.proxy import removeSecurityProxy
     >>> getUtility(IStoreSelector).push(MasterDatabasePolicy())
     >>> salgado = getUtility(IPersonSet).getByName('salgado')
     >>> consumer = getUtility(IOAuthConsumerSet).getByKey('foobar123451432')
     >>> token = consumer.newRequestToken()
     >>> firefox = getUtility(IProductSet)['firefox']
     >>> token.review(salgado, OAuthPermission.WRITE_PUBLIC, context=firefox)
-    >>> access_token = token.createAccessToken()
+    >>> access_token = removeSecurityProxy(token.createAccessToken())
+
+Now let's use it to make a request.
+
     >>> form = dict(
     ...     oauth_consumer_key='foobar123451432',
     ...     oauth_token=access_token.key,

=== 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-09-02 21:17:47 +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-09-02 21:17:47 +0000
@@ -11,12 +11,15 @@
     'OAUTH_REALM',
     'OAUTH_CHALLENGE',
     'IOAuthAccessToken',
+    'IOAuthAccessTokenPublic',
     'IOAuthConsumer',
     'IOAuthConsumerSet',
+    'IOAuthToken',
     'IOAuthNonce',
     'IOAuthRequestToken',
     'IOAuthRequestTokenSet',
     'IOAuthSignedRequest',
+    'IOAuthTokenPublic',
     'NonceAlreadyUsed',
     'TimestampOrderingError',
     'ClockSkew',
@@ -34,12 +37,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 +133,28 @@
         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(
-        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(
-        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(
-        title=_('Secret'), required=True, readonly=True,
-        description=_('The secret associated with this token.  It is used '
-                      'by the consumer to sign its requests.'))
+    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 = 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 = 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.')))
     product = Choice(title=_('Project'), required=False, vocabulary='Product')
     project = Choice(
         title=_('Project'), required=False, vocabulary='ProjectGroup')
@@ -155,18 +165,22 @@
     context = Attribute("FIXME")
 
 
-class IOAuthAccessToken(IOAuthToken):
-    """A token used by a consumer to access protected resources in LP.
-
-    It's created automatically once a user logs in and grants access to a
-    consumer.  The consumer then exchanges an `IOAuthRequestToken` for it.
-    """
-
-    permission = Choice(
-        title=_('Access level'), required=True, readonly=False,
-        vocabulary=AccessLevel,
-        description=_('The level of access given to the application acting '
-                      'on your behalf.'))
+class IOAuthTokenPublic(Interface):
+    """Public access to methods necessary to authenticate OAuth tokens."""
+
+    def checkSignature(request):
+        """Check the signature of an incoming request.
+
+        :param request: A request that's supposedly signed with this
+            token's secret.
+        :return: True if the request is correctly signed, False otherwise.
+        """
+
+
+class IOAuthAccessTokenPublic(IOAuthTokenPublic):
+    """Public access to methods necessary to authenticate access tokens."""
+
+    form_encoded = Attribute("Form-encoded description of the token.")
 
     def checkNonceAndTimestamp(nonce, timestamp):
         """Verify the nonce and timestamp.
@@ -189,6 +203,42 @@
         and associated with this token.
         """
 
+    def authorize(request, nonce, timestamp):
+        """Authorize an incoming request against an OAuth token.
+
+        :param request: A request that's supposedly signed with this
+            token's secret.
+        :param nonce: An OAuth nonce provided with the request.
+        :param timestamp: An OAuth timestamp provided with the request.
+
+        :raise Unauthorized: If the request is not correctly signed
+            with this token's secret, if the nonce or timestamp are
+            bad, if the token has expired, if the token does not
+            actually grant access, or if for any other reason the
+            request cannot be authorized.
+
+        :return: A 3-tuple (account_id, access_level, context)
+          containing all the information necessary to identify the
+          authenticated principal.
+        """
+
+
+class IOAuthAccessToken(IOAuthToken):
+    """A token used by a consumer to access protected resources in LP.
+
+    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 = exported(
+        Choice(
+            title=_('Access level'), required=True, readonly=False,
+            vocabulary=AccessLevel,
+            description=_('The level of access given to the application acting '
+                          'on your behalf.')),
+        readonly=True)
+
 
 class IOAuthRequestToken(IOAuthToken):
     """A token used by a consumer to ask the user to authenticate on LP.

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2010-08-31 20:12:22 +0000
+++ lib/canonical/launchpad/security.py	2010-09-02 21:17:47 +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-09-02 21:17:47 +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)
 

=== added file 'lib/canonical/launchpad/tests/test_webservice_oauth.py'
--- lib/canonical/launchpad/tests/test_webservice_oauth.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/tests/test_webservice_oauth.py	2010-09-02 21:17:47 +0000
@@ -0,0 +1,82 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from datetime import date
+import simplejson
+
+from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
+from canonical.testing import LaunchpadFunctionalLayer
+from lp.testing import (
+    launchpadlib_for,
+    TestCase,
+)
+
+class TestWebServiceAccessToOAuth(TestCase):
+    """Tests of OAuth tokens as published in the web service."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        """Setup."""
+        super(TestWebServiceAccessToOAuth, self).setUp()
+        self.launchpad = launchpadlib_for(
+            'Token management test', 'salgado', 'GRANT_PERMISSIONS')
+
+    def test_authorized_read(self):
+        """You can read tokens if your client has the right access level."""
+        # The person 'salgado' has three associated tokens: two
+        # created as part of sample data, and one ("Grant
+        # Permissions") created in the launchpadlib_for() call
+        # in setUp().
+        my_tokens = [token.permission for token in
+                     self.launchpad.me.oauth_access_tokens]
+        self.assertEquals(
+            sorted(my_tokens), [u'Change Anything', u'Grant Permissions',
+                                u'Read Non-Private Data'])
+
+    def test_authorized_write(self):
+        """You can write tokens if your client has the right access level."""
+        # Change a token's expiration date.
+        token = [token for token in self.launchpad.me.oauth_access_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()
+
+        # Change it back.
+        token.date_expires = old_date_expires
+        token.lp_save()
+
+    def test_others_tokens_are_invisible(self):
+        """No one else can see your tokens."""
+        someone_else = launchpadlib_for(
+            'Token management test', 'name12', 'GRANT_PERMISSIONS')
+        tokens = [token for token in
+                  someone_else.people['salgado'].oauth_access_tokens]
+        self.assertEquals(tokens, [])
+
+    def test_insufficient_access_level(self):
+        """A client can't read your tokens without GRANT_PERMISSIONS."""
+        write_private = launchpadlib_for(
+            'Token management test', 'salgado', 'WRITE_PRIVATE')
+        my_tokens = [token for token in
+                     write_private.me.oauth_access_tokens]
+        self.assertEquals(my_tokens, [])
+
+    def test_url_hacking_doesnt_work(self):
+        """You can't access tokens by URL hacking."""
+        webservice = LaunchpadWebServiceCaller(
+            'launchpad-library', 'salgado-change-anything')
+
+        # A read request gets a 401.
+        url = '/~salgado/+oauth-access-token/salgado-read-nonprivate'
+        response = webservice.get(url)
+        self.assertEqual(response.status, 401)
+
+        # A write request gets a 401.
+        body = simplejson.dumps(dict(date_expires=str(date(9000, 1, 1))))
+        response = webservice.patch(url, 'application/json', body)
+        self.assertEqual(response.status, 401)

=== modified file 'lib/canonical/launchpad/webapp/authentication.py'
--- lib/canonical/launchpad/webapp/authentication.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/webapp/authentication.py	2010-09-02 21:17:47 +0000
@@ -363,24 +363,19 @@
         return request.form
 
 
-def check_oauth_signature(request, consumer, token):
+def check_oauth_signature(request, consumer, token_secret):
     """Check that the given OAuth request is correctly signed.
 
     If the signature is incorrect or its method is not supported, set the
     appropriate status in the request's response and return False.
     """
     authorization = get_oauth_authorization(request)
-
     if authorization.get('oauth_signature_method') != 'PLAINTEXT':
         # XXX: 2008-03-04, salgado: Only the PLAINTEXT method is supported
         # now. Others will be implemented later.
         request.response.setStatus(400)
         return False
 
-    if token is not None:
-        token_secret = token.secret
-    else:
-        token_secret = ''
     expected_signature = "&".join([consumer.secret, token_secret])
     if expected_signature != authorization.get('oauth_signature'):
         request.unauthorized(OAUTH_CHALLENGE)

=== 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-09-02 21:17:47 +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-09-02 21:17:47 +0000
@@ -8,7 +8,6 @@
 __metaclass__ = type
 
 import cgi
-from datetime import datetime
 import threading
 import xmlrpclib
 
@@ -70,11 +69,8 @@
     IWebServiceApplication,
     )
 from canonical.launchpad.interfaces.oauth import (
-    ClockSkew,
     IOAuthConsumerSet,
     IOAuthSignedRequest,
-    NonceAlreadyUsed,
-    TimestampOrderingError,
     )
 import canonical.launchpad.layers
 from canonical.launchpad.webapp.adapter import (
@@ -82,7 +78,6 @@
     RequestExpired,
     )
 from canonical.launchpad.webapp.authentication import (
-    check_oauth_signature,
     get_oauth_authorization,
     )
 from canonical.launchpad.webapp.authorization import (
@@ -1276,27 +1271,16 @@
         token = consumer.getAccessToken(token_key)
         if token is None:
             raise Unauthorized('Unknown access token (%s).' % token_key)
+
         nonce = form.get('oauth_nonce')
         timestamp = form.get('oauth_timestamp')
-        try:
-            token.checkNonceAndTimestamp(nonce, timestamp)
-        except (NonceAlreadyUsed, TimestampOrderingError, ClockSkew), e:
-            raise Unauthorized('Invalid nonce/timestamp: %s' % e)
-        now = datetime.now(pytz.timezone('UTC'))
-        if token.permission == OAuthPermission.UNAUTHORIZED:
-            raise Unauthorized('Unauthorized token (%s).' % token.key)
-        elif token.date_expires is not None and token.date_expires <= now:
-            raise Unauthorized('Expired token (%s).' % token.key)
-        elif not check_oauth_signature(request, consumer, token):
-            raise Unauthorized('Invalid signature.')
-        else:
-            # Everything is fine, let's return the principal.
-            pass
+        # If there's a problem with the token, authorize() will raise
+        # Unauthorized.
+        (account_id, access_level, scope) = token.authorize(
+            request, nonce, timestamp)
+        principal = getUtility(IPlacelessLoginSource).getPrincipal(
+            account_id, access_level=access_level, scope=scope)
         alsoProvides(request, IOAuthSignedRequest)
-        principal = getUtility(IPlacelessLoginSource).getPrincipal(
-            token.person.account.id, access_level=token.permission,
-            scope=token.context)
-
         return principal
 
 

=== modified file 'lib/canonical/launchpad/webapp/tests/test_publication.py'
--- lib/canonical/launchpad/webapp/tests/test_publication.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/webapp/tests/test_publication.py	2010-09-02 21:17:47 +0000
@@ -27,6 +27,7 @@
     )
 from zope.publisher.interfaces import Retry
 from zope.publisher.interfaces.browser import IBrowserRequest
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import dbconfig
 from canonical.launchpad.database.emailaddress import EmailAddress
@@ -254,7 +255,11 @@
         request_token = consumer.newRequestToken()
         request_token.review(
             person, permission=OAuthPermission.READ_PUBLIC, context=None)
-        access_token = request_token.createAccessToken()
+        # The security proxy protects against unauthorized requests to
+        # access the OAuth token data. We're using the OAuth token
+        # data to _create_ a request, so we can strip the security
+        # proxy.
+        access_token = removeSecurityProxy(request_token.createAccessToken())
 
         # Use oauth.OAuthRequest just to generate a dictionary containing all
         # the parameters we need to use in a valid OAuth request, using the

=== 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-09-02 21:17:47 +0000
@@ -26,6 +26,7 @@
 
   <class class="canonical.launchpad.database.OAuthRequestToken">
       <allow interface="canonical.launchpad.interfaces.IOAuthRequestToken"/>
+      <allow interface="canonical.launchpad.interfaces.IOAuthTokenPublic" />
       <require
           permission="launchpad.Edit"
           set_schema="canonical.launchpad.interfaces.IOAuthRequestToken"/>
@@ -39,7 +40,11 @@
   </securedutility>
 
   <class class="canonical.launchpad.database.OAuthAccessToken">
-      <allow interface="canonical.launchpad.interfaces.IOAuthAccessToken"/>
+      <allow interface="canonical.launchpad.webapp.interfaces.ICanonicalUrlData" />
+      <allow interface="canonical.launchpad.interfaces.IOAuthAccessTokenPublic"/>
+      <require
+          permission="launchpad.View"
+          interface="canonical.launchpad.interfaces.IOAuthAccessToken"/>
       <require
           permission="launchpad.Edit"
           set_schema="canonical.launchpad.interfaces.IOAuthAccessToken"/>

=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
--- lib/lp/bugs/tests/test_bugs_webservice.py	2010-08-30 05:19:46 +0000
+++ lib/lp/bugs/tests/test_bugs_webservice.py	2010-09-02 21:17:47 +0000
@@ -10,6 +10,7 @@
 from BeautifulSoup import BeautifulSoup
 from lazr.lifecycle.interfaces import IDoNotSnapshot
 from simplejson import dumps
+from storm.store import Store
 from testtools.matchers import (
     Equals,
     LessThan,
@@ -149,6 +150,7 @@
     def test_attachments_query_counts_constant(self):
         login(USER_EMAIL)
         self.bug = self.factory.makeBug()
+        store = Store.of(self.bug)
         self.factory.makeBugAttachment(self.bug)
         self.factory.makeBugAttachment(self.bug)
         webservice = LaunchpadWebServiceCaller(
@@ -157,6 +159,9 @@
         collector.register()
         self.addCleanup(collector.unregister)
         url = '/bugs/%d/attachments' % self.bug.id
+        # XXX [bug=619017] First request.
+        store.flush()
+        store.reset()
         response = webservice.get(url)
         self.assertThat(collector, HasQueryCount(LessThan(24)))
         with_2_count = collector.count
@@ -164,6 +169,9 @@
         login(USER_EMAIL)
         self.factory.makeBugAttachment(self.bug)
         logout()
+        # XXX [bug=619017] Second request.
+        store.flush()
+        store.reset()
         response = webservice.get(url)
         # XXX: Permit the second call to be == or less, because storm
         # caching bugs (such as) https://bugs.launchpad.net/storm/+bug/619017

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-08-31 14:00:54 +0000
+++ lib/lp/registry/browser/person.py	2010-09-02 21:17:47 +0000
@@ -492,6 +492,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-09-02 21:17:47 +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,10 @@
     # 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-09-02 21:17:47 +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-09-02 21:17:47 +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-09-01 08:41:40 +0000
+++ versions.cfg	2010-09-02 21:17:47 +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