launchpad-reviewers team mailing list archive

[Leonard Richardson <leonard.richardson@xxxxxxxxxxxxx>]

Leonard Richardson has proposed merging lp:~leonardr/launchpad/distinguish-between-unauthorized-and-forbidden into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


This branch returns the response code 403 ("Forbidden") when the client attempts to exchange a request token for an access token, but the end-user has explicitly declined to authorize the request token. Previously the response code was 401 ("Unauthorized"), which made it impossible to distinguish between this case and the case where the end-user has not gotten around to make a decision about the request token.

I also return human-readable strings for various error conditions instead of the empty string.
-- 
https://code.launchpad.net/~leonardr/launchpad/distinguish-between-unauthorized-and-forbidden/+merge/29803
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~leonardr/launchpad/distinguish-between-unauthorized-and-forbidden into lp:launchpad.

=== modified file 'lib/canonical/launchpad/browser/oauth.py'
--- lib/canonical/launchpad/browser/oauth.py	2010-03-30 15:36:27 +0000
+++ lib/canonical/launchpad/browser/oauth.py	2010-07-13 15:48:51 +0000
@@ -245,15 +245,23 @@
         token = consumer.getRequestToken(form.get('oauth_token'))
         if token is None:
             self.request.unauthorized(OAUTH_CHALLENGE)
-            return u''
+            return u'No request token specified.'
 
         if not check_oauth_signature(self.request, consumer, token):
-            return u''
+            return u'Invalid OAuth signature.'
 
-        if (not token.is_reviewed
-            or token.permission == OAuthPermission.UNAUTHORIZED):
+        if not token.is_reviewed:
             self.request.unauthorized(OAUTH_CHALLENGE)
-            return u''
+            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."
+            self.request.response.setStatus(403)
+            return u'End-user refused to authorize request token.'
 
         access_token = token.createAccessToken()
         context_name = None

=== modified file 'lib/canonical/launchpad/pagetests/oauth/access-token.txt'
--- lib/canonical/launchpad/pagetests/oauth/access-token.txt	2009-10-22 13:02:12 +0000
+++ lib/canonical/launchpad/pagetests/oauth/access-token.txt	2010-07-13 15:48:51 +0000
@@ -74,9 +74,10 @@
     ... """ % urlencode(data2))
     HTTP/1.1 401 Unauthorized
     ...
+    Request token has not yet been reviewed. Try again later.
 
-The same will happen if the signature is wrong or if the token's
-permission is UNAUTHORIZED.
+If the token is missing or the signature is wrong the response will
+also be 401.
 
     >>> data2['oauth_signature'] = '&'.join(['foobar', token.secret])
     >>> print http(r"""
@@ -85,6 +86,22 @@
     ... """ % urlencode(data2))
     HTTP/1.1 401 Unauthorized
     ...
+    Invalid OAuth signature.
+
+    >>> data3 = data.copy()
+    >>> del(data3['oauth_token'])
+    >>> print http(r"""
+    ... GET /+access-token?%s HTTP/1.1
+    ... Host: launchpad.dev
+    ... """ % urlencode(data3))
+    HTTP/1.1 401 Unauthorized
+    ...
+    No request token specified.
+
+It the token's permission is set to UNAUTHORIZED, the response code is
+403 ("Forbidden"). This conveys that (to quote the HTTP RFC)
+"authorization will not help" and that the token will _never_ be
+exchanged for an access token.
 
     >>> token.review(salgado, OAuthPermission.UNAUTHORIZED)
     >>> data2['oauth_signature'] = '&'.join([consumer.secret, token.secret])
@@ -92,6 +109,7 @@
     ... GET /+access-token?%s HTTP/1.1
     ... Host: launchpad.dev
     ... """ % urlencode(data2))
-    HTTP/1.1 401 Unauthorized
+    HTTP/1.1 403 Forbidden
     ...
+    End-user refused to authorize request token.