launchpad-reviewers team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

Benji York has proposed merging lp:~benji/launchpad/bug-597324 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #597324 Login fails when logging in while accessing URL with query parameters 
  https://bugs.launchpad.net/bugs/597324


Fix bug #597324 as well as two other bugs found while investigating (one in the development OpenID provider, the other in Launchpad).
-- 
https://code.launchpad.net/~benji/launchpad/bug-597324/+merge/30330
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-597324 into lp:launchpad/devel.

=== 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-19 19:42:57 +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,27 @@
     suspended_account_template = ViewPageTemplateFile(
         '../templates/login-suspended-account.pt')
 
+    @staticmethod
+    def _gather_params(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
+
+    @staticmethod
+    def _get_requested_url(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 +369,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-19 19:42:57 +0000
@@ -62,6 +62,18 @@
                 "Not using the master store: %s" % current_policy)
 
 
+class FakeConsumer:
+    def complete(self, params, requested_url):
+        self.params = params
+        self.requested_url = requested_url
+
+
+class FakeConsumerOpenIDCallbackView(OpenIDCallbackView):
+    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 +166,92 @@
         # slave DBs.
         self.assertNotIn('last_write', ISession(view.request)['lp.dbpolicy'])
 
+    def test_redirect_logic(self):
+        # The starting_url can either be in the request.form values (in the
+        # case of a GET) or in the query string (in the case of a POST).  The
+        # OpenIDCallbackView._redirect method pulls it out and redirects to
+        # it.
+        APPLICATION_URL = 'http://example.com'
+        STARTING_URL = APPLICATION_URL + '/start'
+        view = OpenIDCallbackView(context=None, request=None)
+
+        # If the request was a POST or GET with no form or query string values
+        # at all, then the application URL is used.
+        view.request = LaunchpadTestRequest(SERVER_URL=APPLICATION_URL)
+        view._redirect()
+        self.assertEquals(
+            httplib.TEMPORARY_REDIRECT, view.request.response.getStatus())
+        self.assertEquals(
+            view.request.response.getHeader('Location'), APPLICATION_URL)
+
+        # If the request was a GET, the starting_url is extracted correctly.
+        view.request = LaunchpadTestRequest(
+            SERVER_URL=APPLICATION_URL, form={'starting_url': STARTING_URL})
+        view._redirect()
+        self.assertEquals(
+            httplib.TEMPORARY_REDIRECT, view.request.response.getStatus())
+        self.assertEquals(
+            view.request.response.getHeader('Location'), STARTING_URL)
+
+        # If the request was a POST, the starting_url is extracted correctly.
+        view.request = LaunchpadTestRequest(
+            SERVER_URL=APPLICATION_URL, form={'fake': 'value'},
+            QUERY_STRING='starting_url='+STARTING_URL)
+        view._redirect()
+        self.assertEquals(
+            httplib.TEMPORARY_REDIRECT, view.request.response.getStatus())
+        self.assertEquals(
+            view.request.response.getHeader('Location'), STARTING_URL)
+
+    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': '/'})
+        params = OpenIDCallbackView._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'})
+        url = OpenIDCallbackView._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.

=== 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-19 19:42:57 +0000
@@ -170,6 +170,9 @@
         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:
+            response.setHeader('content-type', 'text/html')
         for header, value in webresponse.headers.items():
             response.setHeader(header, value)
         return webresponse.body