Merge lp:~benji/launchpad/bug-597324 into lp:launchpad

[Benji York]

Fix bug #597324 as well as two other bugs found while investigating (one in the development OpenID provider, the other in Launchpad).

[Benji York (benji)]

It strikes me that my description above could use some fleshing out. Here goes.

The bug was triggered when the OpenID provider chooses to present the user with a form with a single Continue button to click on instead of redirecting the user directly to Launchpad (because the redirect would have contained a Location header that is too long for some browsers).

Launchpad wasn't expecting this behavior and couldn't handle it. The solution was to gather the data needed to complete the OpenID handshaking from POST requests as well as GET requests (the data ends up in different data structures in the two scenarios).

There was also a bug in the development OpenID provider such that it did not set the content-type correctly when generating the aforementioned continue-button-containing-form, so the user only saw raw HTML.

[Robert Collins (lifeless)]

test_redirect_logic looks like it has 3 or 4 separate tests in it - please split that out for readability.

Your fakes could do with a docstring each to describe their intent, and lastly your static methods are a little weird; if they are not stateful, please consider functions; if they are tightly coupled, but are called from instances, please use normal methods.

With these changes it can land without another review.

Thanks,
Rob

review: Needs Fixing (code)

[Benji York (benji)]

On Tue, Jul 20, 2010 at 5:33 AM, Robert Collins
<email address hidden> wrote:
> Review: Needs Fixing code
> test_redirect_logic looks like it has 3 or 4 separate tests in it -
> please split that out for readability.

Done. They somehow seem less readable though. And the constants that
they use now have greater scope than before. Perhaps these should be
in their own TestCase instead. Yeah, that's what I'll do.

> Your fakes could do with a docstring each to describe their intent,

Added.

> and lastly your static methods are a little weird; if they are not
> stateful, please consider functions; if they are tightly coupled, but
> are called from instances, please use normal methods.

They are not stateful, but are very tightly related to the class in
question and called from instances thereof.

I prefer methods that don't use self to be static, but am glad to honor
local custom. I removed the staticmethod decorator and added self to
the parameter list.

> With these changes it can land without another review.

Land ho!
--
Benji York

[Benji York (benji)]

Robert, lp-land is refusing to land this branch with the message "bzr: ERROR: bzrlib.plugins.pqm.lpland.MissingReviewError: Need approved votes in order to land."

I believe you need to give this MP an "Approve" vote.

Thanks.

[Robert Collins (lifeless)]

review: Approve

=== modified file 'lib/canonical/launchpad/webapp/login.py'
--- lib/canonical/launchpad/webapp/login.py	2010-07-02 10:37:33 +0000
+++ lib/canonical/launchpad/webapp/login.py	2010-07-20 19:23:51 +0000
@@ -1,11 +1,12 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
+"""Stuff to do with logging in and logging out."""
+
 from __future__ import with_statement
 
-"""Stuff to do with logging in and logging out."""
-
 __metaclass__ = type
 
+import cgi
 import urllib
 
 from datetime import datetime, timedelta
@@ -245,9 +246,25 @@
     suspended_account_template = ViewPageTemplateFile(
         '../templates/login-suspended-account.pt')
 
+    def _gather_params(self, request):
+        params = dict(request.form)
+        query_string = request.get('QUERY_STRING')
+        if query_string is not None:
+            params.update(cgi.parse_qsl(query_string))
+        return params
+
+    def _get_requested_url(self, request):
+        requested_url = request.getURL()
+        query_string = request.get('QUERY_STRING')
+        if query_string is not None:
+            requested_url += '?' + query_string
+        return requested_url
+
     def initialize(self):
-        self.openid_response = self._getConsumer().complete(
-            self.request.form, self.request.getURL())
+        params = self._gather_params(self.request)
+        requested_url = self._get_requested_url(self.request)
+        consumer = self._getConsumer()
+        self.openid_response = consumer.complete(params, requested_url)
 
     def login(self, account):
         loginsource = getUtility(IPlacelessLoginSource)
@@ -350,7 +367,13 @@
     def _redirect(self):
         target = self.request.form.get('starting_url')
         if target is None:
-            target = self.getApplicationURL()
+            # If this was a POST, then the starting_url won't be in the form
+            # values, but in the query parameters instead.
+            target = self.request.query_string_params.get('starting_url')
+            if target is None:
+                target = self.request.getApplicationURL()
+            else:
+                target = target[0]
         self.request.response.redirect(target, temporary_if_possible=True)
 
 

=== modified file 'lib/canonical/launchpad/webapp/tests/test_login.py'
--- lib/canonical/launchpad/webapp/tests/test_login.py	2010-07-01 10:04:54 +0000
+++ lib/canonical/launchpad/webapp/tests/test_login.py	2010-07-20 19:23:51 +0000
@@ -32,7 +32,8 @@
 from canonical.launchpad.webapp.login import OpenIDCallbackView, OpenIDLogin
 from canonical.launchpad.webapp.interfaces import IStoreSelector
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
-from canonical.testing.layers import AppServerLayer, DatabaseFunctionalLayer
+from canonical.testing.layers import (
+    AppServerLayer, DatabaseFunctionalLayer, FunctionalLayer)
 
 from lp.registry.interfaces.person import IPerson
 from lp.testopenid.interfaces.server import ITestOpenIDPersistentIdentity
@@ -62,6 +63,20 @@
                 "Not using the master store: %s" % current_policy)
 
 
+class FakeConsumer:
+    """An OpenID consumer that stashes away arguments for test instection."""
+    def complete(self, params, requested_url):
+        self.params = params
+        self.requested_url = requested_url
+
+
+class FakeConsumerOpenIDCallbackView(OpenIDCallbackView):
+    """An OpenID handler with fake consumer so arguments can be inspected."""
+    def _getConsumer(self):
+        self.fake_consumer = FakeConsumer()
+        return self.fake_consumer
+
+
 @contextmanager
 def SRegResponse_fromSuccessResponse_stubbed():
     def sregFromFakeSuccessResponse(cls, success_response, signed_only=True):
@@ -154,6 +169,57 @@
         # slave DBs.
         self.assertNotIn('last_write', ISession(view.request)['lp.dbpolicy'])
 
+    def test_gather_params(self):
+        # If the currently requested URL includes a query string, the
+        # parameters in the query string must be included when constructing
+        # the params mapping (which is then used to complete the open ID
+        # response).  OpenIDCallbackView._gather_params does that gathering.
+        request = LaunchpadTestRequest(
+            SERVER_URL='http://example.com',
+            QUERY_STRING='foo=bar',
+            form={'starting_url': 'http://launchpad.dev/after-login'},
+            environ={'PATH_INFO': '/'})
+        view = OpenIDCallbackView(context=None, request=None)
+        params = view._gather_params(request)
+        expected_params = {
+            'starting_url': 'http://launchpad.dev/after-login',
+            'foo': 'bar',
+        }
+        self.assertEquals(params, expected_params)
+
+    def test_get_requested_url(self):
+        # The OpenIDCallbackView needs to pass the currently-being-requested
+        # URL to the OpenID library.  OpenIDCallbackView._get_requested_url
+        # returns the URL.
+        request = LaunchpadTestRequest(
+            SERVER_URL='http://example.com',
+            QUERY_STRING='foo=bar',
+            form={'starting_url': 'http://launchpad.dev/after-login'})
+        view = OpenIDCallbackView(context=None, request=None)
+        url = view._get_requested_url(request)
+        self.assertEquals(url, 'http://example.com?foo=bar')
+
+    def test_open_id_callback_handles_query_string(self):
+        # If the currently requested URL includes a query string, the
+        # parameters in the query string must be included when constructing
+        # the params mapping (which is then used to complete the open ID
+        # response).
+        request = LaunchpadTestRequest(
+            SERVER_URL='http://example.com',
+            QUERY_STRING='foo=bar',
+            form={'starting_url': 'http://launchpad.dev/after-login'},
+            environ={'PATH_INFO': '/'})
+        view = FakeConsumerOpenIDCallbackView(object(), request)
+        view.initialize()
+        self.assertEquals(
+            view.fake_consumer.params,
+            {
+                'starting_url': 'http://launchpad.dev/after-login',
+                'foo': 'bar',
+            })
+        self.assertEquals(
+            view.fake_consumer.requested_url,'http://example.com?foo=bar')
+
     def test_personless_account(self):
         # When there is no Person record associated with the account, we
         # create one.
@@ -339,6 +405,50 @@
         self.assertTrue(datetime.utcnow() - last_write < timedelta(minutes=1))
 
 
+class TestOpenIDCallbackRedirects(TestCaseWithFactory):
+    layer = FunctionalLayer
+
+    APPLICATION_URL = 'http://example.com'
+    STARTING_URL = APPLICATION_URL + '/start'
+
+    def test_open_id_callback_redirect_from_get(self):
+        # If OpenID callback request was a GET, the starting_url is extracted
+        # correctly.
+        view = OpenIDCallbackView(context=None, request=None)
+        view.request = LaunchpadTestRequest(
+            SERVER_URL=self.APPLICATION_URL,
+            form={'starting_url': self.STARTING_URL})
+        view._redirect()
+        self.assertEquals(
+            httplib.TEMPORARY_REDIRECT, view.request.response.getStatus())
+        self.assertEquals(
+            view.request.response.getHeader('Location'), self.STARTING_URL)
+
+    def test_open_id_callback_redirect_from_post(self):
+        # If OpenID callback request was a POST, the starting_url is extracted
+        # correctly.
+        view = OpenIDCallbackView(context=None, request=None)
+        view.request = LaunchpadTestRequest(
+            SERVER_URL=self.APPLICATION_URL, form={'fake': 'value'},
+            QUERY_STRING='starting_url='+self.STARTING_URL)
+        view._redirect()
+        self.assertEquals(
+            httplib.TEMPORARY_REDIRECT, view.request.response.getStatus())
+        self.assertEquals(
+            view.request.response.getHeader('Location'), self.STARTING_URL)
+
+    def test_openid_callback_redirect_fallback(self):
+        # If OpenID callback request was a POST or GET with no form or query
+        # string values at all, then the application URL is used.
+        view = OpenIDCallbackView(context=None, request=None)
+        view.request = LaunchpadTestRequest(SERVER_URL=self.APPLICATION_URL)
+        view._redirect()
+        self.assertEquals(
+            httplib.TEMPORARY_REDIRECT, view.request.response.getStatus())
+        self.assertEquals(
+            view.request.response.getHeader('Location'), self.APPLICATION_URL)
+
+
 urls_redirected_to = []
 
 

=== modified file 'lib/lp/testopenid/browser/server.py'
--- lib/lp/testopenid/browser/server.py	2010-07-01 13:00:27 +0000
+++ lib/lp/testopenid/browser/server.py	2010-07-20 19:23:51 +0000
@@ -22,7 +22,7 @@
 from zope.session.interfaces import ISession
 
 from openid import oidutil
-from openid.server.server import CheckIDRequest, Server
+from openid.server.server import CheckIDRequest, Server, ENCODE_HTML_FORM
 from openid.store.memstore import MemoryStore
 from openid.extensions.sreg import SRegRequest, SRegResponse
 
@@ -170,6 +170,10 @@
         webresponse = self.openid_server.encodeResponse(openid_response)
         response = self.request.response
         response.setStatus(webresponse.code)
+        # encodeResponse doesn't generate a content-type, help it out
+        if (webresponse.code == 200 and webresponse.body
+                and openid_response.whichEncoding() == ENCODE_HTML_FORM):
+            response.setHeader('content-type', 'text/html')
         for header, value in webresponse.headers.items():
             response.setHeader(header, value)
         return webresponse.body