Merge lp:~leonardr/launchpad/bug-271010 into lp:launchpad

[Leonard Richardson]

[r=gmb][bug=271010] [ui=none] When a request token is authorized, display the success message immediately instead of redirecting to a page that displays the success message. By the time the browser requests that page, the request token may have been exchanged for an access token, making it impossible to display the success message.

When you authorize a request token on +authorize-token, your browser is redirected to another page. If there's an OAuth callback URL set, you get redirected to that URL. If there's no callback URL, you get redirected to +token-authorized, which just prints a message "yay, you authorized the request token".

Here's the problem: newer versions of launchpadlib are polling Launchpad once a second, trying to exchange that request token for an access token. Once the exchange happens, the request token is destroyed and +token-authorized stops working. If the exchange happens before your browser makes that redirect request, by the time you arrive on +token-authorized your request token will no longer exist. You'll get an OOPS (see bug 271010). Worse, it looks like your integration failed, even though it actually succeeded.

This branch gets rid of +token-authorized altogether. The "yay, you authorized the request token" message is now part of the +authorize-token view, and is sent in response to the request that authorizes the request token. There's no redirect (except to an OAuth callback URL, which is a completely different case) and thus no possibility that the request token is destroyed before the message can be printed.

[Graham Binns (gmb)]

review: Approve (code)

=== modified file 'lib/canonical/launchpad/browser/oauth.py'
--- lib/canonical/launchpad/browser/oauth.py	2010-11-23 23:22:27 +0000
+++ lib/canonical/launchpad/browser/oauth.py	2011-03-01 15:22:04 +0000
@@ -6,7 +6,6 @@
     'OAuthAccessTokenView',
     'OAuthAuthorizeTokenView',
     'OAuthRequestTokenView',
-    'OAuthTokenAuthorizedView',
     'lookup_oauth_context']
 
 from datetime import (
@@ -372,9 +371,6 @@
         callback = self.request.form.get('oauth_callback')
         if callback:
             self.next_url = callback
-        else:
-            self.next_url = (
-                '+token-authorized?oauth_token=%s' % self.token.key)
 
 
 def lookup_oauth_context(context):
@@ -397,22 +393,6 @@
     return context
 
 
-class OAuthTokenAuthorizedView(LaunchpadView):
-    """Where users who reviewed tokens may get redirected to.
-
-    If the consumer didn't include an oauth_callback when sending the user to
-    Launchpad, this is the page the user is redirected to after he logs in and
-    reviews the token.
-    """
-
-    def initialize(self):
-        key = self.request.form.get('oauth_token')
-        self.token = getUtility(IOAuthRequestTokenSet).getByKey(key)
-        assert self.token.is_reviewed, (
-            'Users should be directed to this page only if they already '
-            'authorized the token.')
-
-
 class OAuthAccessTokenView(LaunchpadView):
     """Where consumers may exchange a request token for an access token."""
 
 
=== modified file 'lib/canonical/launchpad/pagetests/oauth/authorize-token.txt'
--- lib/canonical/launchpad/pagetests/oauth/authorize-token.txt	2010-11-02 17:06:44 +0000
+++ lib/canonical/launchpad/pagetests/oauth/authorize-token.txt	2011-03-01 15:22:04 +0000
@@ -202,8 +202,7 @@
     >>> token.is_reviewed
     True
 
-If no oauth_callback is specified, we redirect the user to
-+authorized-token.
+If no oauth_callback is specified, we don't redirect the user.
 
     # Create a new (unreviewed) token.
     >>> token = request_token_for(consumer)
@@ -215,15 +214,17 @@
     >>> browser.getControl('Read Anything').click()
 
     >>> browser.url
-    'http://launchpad.dev/+token-authorized?...'
+    'http://launchpad.dev/+authorize-token'
     >>> print extract_text(find_tag_by_id(browser.contents, 'maincontent'))
     Almost finished ...
     To finish authorizing the application identified as foobar123451432 to
     access Launchpad on your behalf you should go back to the application
     window in which you started the process and inform it that you have done
     your part of the process.
+    See all applications authorized to access Launchpad on your behalf.
 
-If we can't find the token, we will explain that to the user.
+If we can't find the request token (possibly because it was already
+exchanged for an access token), we will explain that to the user.
 
     >>> params = dict(oauth_callback='http://example.com/oauth')
     >>> browser.open(
@@ -233,6 +234,7 @@
     The information provided by the remote application was incorrect or
     incomplete. Because of that we were unable to identify the application
     which would access Launchpad on your behalf.
+    You may have already authorized this application.
     See all applications authorized to access Launchpad on your behalf.
 
     >>> params = dict(
@@ -244,11 +246,12 @@
     The information provided by the remote application was incorrect or
     incomplete. Because of that we were unable to identify the application
     which would access Launchpad on your behalf.
+    You may have already authorized this application.
     See all applications authorized to access Launchpad on your behalf.
 
-The same happens if the token is already reviewed.  Although that's
-probably an indication that the user has already reviewed that token in
-another window/tab so there's no need to worry.
+If the token is already reviewed (perhaps by the same user in another
+window or tab), but has not yet been exchanged for an access token,
+the success message is printed.
 
     # Need to get the token again as it's been changed in another
     # transaction.
@@ -260,9 +263,9 @@
     >>> browser.open(
     ...     "http://launchpad.dev/+authorize-token?%s" % urlencode(params))
     >>> print extract_text(find_tag_by_id(browser.contents, 'maincontent'))
-    Request already reviewed
-    This request for accessing Launchpad on your behalf has been
-    reviewed ... ago.
+    Almost finished ...
+    To finish authorizing the application identified as foobar123451432
+    ...
     See all applications authorized to access Launchpad on your behalf.
 
 Desktop integration
@@ -397,6 +400,7 @@
     The Ubuntu computer called mycomputer now has access to your
     Launchpad account. Within a few seconds, you should be able to
     start using its Launchpad integration features.
+    See all applications authorized to access Launchpad on your behalf.
 
     >>> print token.is_reviewed
     True
@@ -422,6 +426,7 @@
     The integration you just authorized will expire in 59 minutes. At
     that time, you'll have to re-authorize mycomputer, if you want to
     keep using its Launchpad integration features.
+    See all applications authorized to access Launchpad on your behalf.
 
     >>> print token.is_reviewed
     True
@@ -481,6 +486,7 @@
     You decided against desktop integration
     You decided not to give mycomputer access to your Launchpad
     account. You can always change your mind later.
+    See all applications authorized to access Launchpad on your behalf.
 
     >>> print token.is_reviewed
     True
 
=== modified file 'lib/canonical/launchpad/templates/oauth-authorize.pt'
--- lib/canonical/launchpad/templates/oauth-authorize.pt	2010-11-02 17:06:44 +0000
+++ lib/canonical/launchpad/templates/oauth-authorize.pt	2011-03-01 15:22:04 +0000
@@ -15,6 +15,11 @@
         incomplete. Because of that we were unable to identify the
         application which would access Launchpad on your behalf.
       </p>
+
+      <p>
+        You may have already authorized this application.
+      </p>
+
     </tal:no-token>
 
     <tal:has-token condition="token">
@@ -123,12 +128,80 @@
       </tal:token-not-reviewed>
 
       <tal:token-reviewed condition="token/is_reviewed">
-        <h1>Request already reviewed</h1>
-        <p>
-          This request for accessing Launchpad on your behalf has been
-          reviewed <tal:date-reviewed 
-                      content="token/date_reviewed/fmt:approximatedate" />.
-        </p>
+
+        <tal:desktop-integration
+           condition="view/token/consumer/is_integrated_desktop">
+
+          <tal:unauthorized
+             condition="view/token/permission/enumvalue:UNAUTHORIZED">
+            <h1>You decided against desktop integration</h1>
+
+            <p>
+              You decided not to give
+              <strong
+                 tal:content="view/token/consumer/integrated_desktop_name">hostname</strong>
+              access to your Launchpad account. You can always change your
+              mind later.
+            </p>
+          </tal:unauthorized>
+
+          <tal:authorized
+             condition="not:view/token/permission/enumvalue:UNAUTHORIZED">
+            <h1>Almost finished ...</h1>
+
+            <p>
+              The
+              <tal:desktop
+                 replace="structure view/token/consumer/integrated_desktop_type" />
+              computer called
+              <strong
+                 tal:content="view/token/consumer/integrated_desktop_name">hostname</strong>
+              now has access to your Launchpad account. Within a few
+              seconds, you should be able to start using its Launchpad
+              integration features.
+            </p>
+
+            <p tal:condition="view/token/date_expires">
+              The integration you just authorized will expire
+              <tal:date
+                 replace="structure view/token/date_expires/fmt:approximatedate" />.
+              At that time, you'll have to re-authorize
+              <strong
+                 tal:content="view/token/consumer/integrated_desktop_name">hostname</strong>,
+              if you want to keep using its Launchpad integration features.
+            </p>
+
+          </tal:authorized>
+        </tal:desktop-integration>
+
+        <tal:application-integration
+           condition="not:view/token/consumer/is_integrated_desktop">
+
+          <tal:unauthorized
+             condition="view/token/permission/enumvalue:UNAUTHORIZED">
+            <h1>Access not granted to application</h1>
+            <p>
+              The application identified as
+              <strong tal:content="view/token/consumer/key">key</strong> has not
+              been given access to your protected resources on Launchpad.
+            </p>
+          </tal:unauthorized>
+
+          <tal:authorized
+             condition="not:view/token/permission/enumvalue:UNAUTHORIZED">
+            <h1>Almost finished ...</h1>
+            <p>
+              To finish authorizing the application identified as
+              <strong tal:content="view/token/consumer/key">key</strong>
+              to access Launchpad on your behalf you should go back to
+              the application window in which you started the process
+              and inform it that you have done your part of the
+              process.
+            </p>
+          </tal:authorized>
+
+        </tal:application-integration>
+
       </tal:token-reviewed>
     </tal:has-token>
 
 
=== removed file 'lib/canonical/launchpad/templates/token-authorized.pt'
--- lib/canonical/launchpad/templates/token-authorized.pt	2010-11-02 17:06:44 +0000
+++ lib/canonical/launchpad/templates/token-authorized.pt	1970-01-01 00:00:00 +0000
@@ -1,76 +0,0 @@
-<html
-  xmlns="http://www.w3.org/1999/xhtml"
-  xmlns:tal="http://xml.zope.org/namespaces/tal"
-  xmlns:metal="http://xml.zope.org/namespaces/metal"
-  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  metal:use-macro="view/macro:page/locationless"
-  i18n:domain="launchpad"
->
-<body>
-  <div class="top-portlet" metal:fill-slot="main">
-
-    <tal:desktop condition="view/token/consumer/is_integrated_desktop">
-
-      <tal:unauthorized condition="view/token/permission/enumvalue:UNAUTHORIZED">
-        <h1>You decided against desktop integration</h1>
-
-        <p>
-          You decided not to give
-          <strong tal:content="view/token/consumer/integrated_desktop_name">hostname</strong>
-          access to your Launchpad account. You can always change your
-          mind later.
-        </p>
-      </tal:unauthorized>
-
-      <tal:authorized condition="not:view/token/permission/enumvalue:UNAUTHORIZED">
-        <h1>Almost finished ...</h1>
-
-        <p>The
-          <tal:desktop replace="structure view/token/consumer/integrated_desktop_type" />
-          computer called
-          <strong tal:content="view/token/consumer/integrated_desktop_name">hostname</strong>
-          now has access to your Launchpad account. Within a few
-          seconds, you should be able to start using its Launchpad
-          integration features.</p>
-
-        <p tal:condition="view/token/date_expires">
-          The integration you just authorized will expire
-          <tal:date
-           replace="structure view/token/date_expires/fmt:approximatedate" />.
-          At that time, you'll have to re-authorize
-          <strong tal:content="view/token/consumer/integrated_desktop_name">hostname</strong>,
-          if you want to keep using its Launchpad integration features.
-
-        </p>
-
-      </tal:authorized>
-    </tal:desktop>
-
-    <tal:application condition="not:view/token/consumer/is_integrated_desktop">
-
-     <tal:unauthorized condition="view/token/permission/enumvalue:UNAUTHORIZED">
-       <h1>Access not granted to application</h1>
-       <p>
-         The application identified as
-         <strong tal:content="view/token/consumer/key">key</strong> has not
-         been given access to your protected resources on Launchpad.
-       </p>
-     </tal:unauthorized>
-
-     <tal:authorized condition="not:view/token/permission/enumvalue:UNAUTHORIZED">
-       <h1>Almost finished ...</h1>
-       <p>
-        To finish authorizing the application identified as
-        <strong tal:content="view/token/consumer/key">key</strong> to access
-        Launchpad on your behalf you should go back to the application window
-        in which you started the process and inform it that you have done your
-        part of the process.
-       </p>
-     </tal:authorized>
-
-    </tal:application>
-  </div>
-
-</body>
-</html>
-
 
=== modified file 'lib/canonical/launchpad/zcml/launchpad.zcml'
--- lib/canonical/launchpad/zcml/launchpad.zcml	2010-11-08 14:16:17 +0000
+++ lib/canonical/launchpad/zcml/launchpad.zcml	2011-03-01 15:22:04 +0000
@@ -261,13 +261,6 @@
 
   <browser:page
       for="canonical.launchpad.webapp.interfaces.ILaunchpadApplication"
-      name="+token-authorized"
-      class="canonical.launchpad.browser.OAuthTokenAuthorizedView"
-      template="../templates/token-authorized.pt"
-      permission="launchpad.AnyPerson" />
-
-  <browser:page
-      for="canonical.launchpad.webapp.interfaces.ILaunchpadApplication"
       name="+access-token"
       class="canonical.launchpad.browser.OAuthAccessTokenView"
       permission="zope.Public" />