Merge lp:~stevenk/launchpad/assertion-review-token into lp:launchpad

[Steve Kowalik]

Catch the OAuthValidationError exceptions that are raised by IOAuthRequestToken.createAccessToken() and IOAuthRequestToken.review() in the browser code and show nice error messages.

Catch the AssertionErrors that are raised by IOAuthRequestToken.createAccessToken() and IOAuthRequestToken.review() in the browser code. Two out of the three cases for createAccessToken were handled, so I left one and generalized the other two. None of the cases for review() were handled, so now they both are.

I claim the LoC credit from https://code.launchpad.net/~stevenk/launchpad/destroy-simplified-branch-ff/+merge/127630 for this MP.

[Robert Collins (lifeless)]

Wait it raises a real genuine AssertionError? Thats -very- bad form to catch....

[William Grant (wgrant)]

Thanks for the fixes. This looks good, except for the date_expires indentation on line 21.

review: Approve (code)

=== modified file 'lib/lp/services/oauth/browser/__init__.py'
--- lib/lp/services/oauth/browser/__init__.py	2012-01-01 02:58:52 +0000
+++ lib/lp/services/oauth/browser/__init__.py	2012-10-03 07:18:35 +0000
@@ -34,6 +34,7 @@
     IOAuthRequestTokenSet,
     OAUTH_CHALLENGE,
     )
+from lp.services.oauth.model import OAuthValidationError
 from lp.services.webapp import LaunchpadView
 from lp.services.webapp.authentication import (
     check_oauth_signature,
@@ -369,8 +370,13 @@
                 datetime.now(pytz.timezone('UTC')) + duration_delta)
         else:
             expiration_date = None
-        self.token.review(self.user, permission, self.token_context,
-                          date_expires=expiration_date)
+        try:
+            self.token.review(
+                self.user, permission, self.token_context,
+                date_expires=expiration_date)
+        except OAuthValidationError as e:
+            self.request.response.addErrorNotification(str(e))
+            return
         callback = self.request.form.get('oauth_callback')
         if callback:
             self.next_url = callback
@@ -427,16 +433,12 @@
             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
-            # ("Unauthorized") to distinguish this case and to
-            # indicate that, as RFC2616 says, "authorization will not
-            # help."
+        try:
+            access_token = token.createAccessToken()
+        except OAuthValidationError as e:
             self.request.response.setStatus(403)
-            return u'End-user refused to authorize request token.'
+            return unicode(e)
 
-        access_token = token.createAccessToken()
         context_name = None
         if access_token.context is not None:
             context_name = access_token.context.name

=== modified file 'lib/lp/services/oauth/model.py'
--- lib/lp/services/oauth/model.py	2012-09-28 06:25:44 +0000
+++ lib/lp/services/oauth/model.py	2012-10-03 07:18:35 +0000
@@ -8,7 +8,9 @@
     'OAuthConsumerSet',
     'OAuthNonce',
     'OAuthRequestToken',
-    'OAuthRequestTokenSet']
+    'OAuthRequestTokenSet',
+    'OAuthValidationError',
+    ]
 
 from datetime import (
     datetime,
@@ -86,6 +88,10 @@
 TIMESTAMP_SKEW_WINDOW = 60 * 60  # seconds, +/-
 
 
+class OAuthValidationError(Exception):
+    """Raised when the OAuth token cannot be validated."""
+
+
 class OAuthBase:
     """Base class for all OAuth database classes."""
 
@@ -335,10 +341,10 @@
     def review(self, user, permission, context=None, date_expires=None):
         """See `IOAuthRequestToken`."""
         if self.is_reviewed:
-            raise AssertionError(
+            raise OAuthValidationError(
                 "Request tokens can be reviewed only once.")
         if self.is_expired:
-            raise AssertionError(
+            raise OAuthValidationError(
                 'This request token has expired and can no longer be '
                 'reviewed.')
         self.date_reviewed = datetime.now(pytz.timezone('UTC'))
@@ -360,14 +366,14 @@
     def createAccessToken(self):
         """See `IOAuthRequestToken`."""
         if not self.is_reviewed:
-            raise AssertionError(
+            raise OAuthValidationError(
                 'Cannot create an access token from an unreviewed request '
                 'token.')
         if self.permission == OAuthPermission.UNAUTHORIZED:
-            raise AssertionError(
+            raise OAuthValidationError(
                 'The user did not grant access to this consumer.')
         if self.is_expired:
-            raise AssertionError(
+            raise OAuthValidationError(
                 'This request token has expired and can no longer be '
                 'exchanged for an access token.')
 

=== modified file 'lib/lp/services/oauth/stories/authorize-token.txt'
--- lib/lp/services/oauth/stories/authorize-token.txt	2012-02-25 03:01:34 +0000
+++ lib/lp/services/oauth/stories/authorize-token.txt	2012-10-03 07:18:35 +0000
@@ -275,6 +275,25 @@
     ...
     See all applications authorized to access Launchpad on your behalf.
 
+If the token has expired, we notify the user, and inhibit the callback.
+
+    >>> token = request_token_for(consumer)
+    >>> from datetime import datetime, timedelta
+    >>> from pytz import UTC
+    >>> from zope.security.proxy import removeSecurityProxy
+    >>> date_created = datetime.now(UTC) - timedelta(hours=3)
+    >>> removeSecurityProxy(token).date_created = date_created
+    >>> params = dict(
+    ...     oauth_token=token.key, oauth_callback='http://example.com/oauth')
+    >>> browser.open(
+    ...     "http://launchpad.dev/+authorize-token?%s" % urlencode(params))
+    >>> browser.getControl('Read Anything').click()
+    >>> browser.url
+    'http://launchpad.dev/+authorize-token'
+    >>> [tag] = find_tags_by_class(browser.contents, 'error message')
+    >>> print extract_text(tag)
+    This request token has expired and can no longer be reviewed.
+
 Desktop integration
 ===================