Launchpad itself

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

  1. bug-597324
  2. Merge into devel
Proposed by Benji York
Status: Merged
Approved by: Robert Collins
Approved revision: no longer in the source branch.
Merge reported by: Benji York
Merged at revision: not available
Proposed branch: lp:~benji/launchpad/bug-597324
Merge into: lp:launchpad
Diff against target: 229 lines (+144/-7)
3 files modified
lib/canonical/launchpad/webapp/login.py (+28/-5)
lib/canonical/launchpad/webapp/tests/test_login.py (+111/-1)
lib/lp/testopenid/browser/server.py (+5/-1)
To merge this branch: bzr merge lp:~benji/launchpad/bug-597324
Reviewer Review Type Date Requested Status
Robert Collins (community) Approve
Review via email: mp+30330@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
Benji York (benji) wrote :

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.

Revision history for this message
Robert Collins (lifeless) wrote :

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)
Revision history for this message
Benji York (benji) wrote :

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

Revision history for this message
Benji York (benji) wrote :

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.

Revision history for this message
Robert Collins (lifeless) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
1# Copyright 2009 Canonical Ltd. This software is licensed under the1# Copyright 2009 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
3"""Stuff to do with logging in and logging out."""
4
3from __future__ import with_statement5from __future__ import with_statement
46
5"""Stuff to do with logging in and logging out."""
6
7__metaclass__ = type7__metaclass__ = type
88
9import cgi
9import urllib10import urllib
1011
11from datetime import datetime, timedelta12from datetime import datetime, timedelta
@@ -245,9 +246,25 @@
245 suspended_account_template = ViewPageTemplateFile(246 suspended_account_template = ViewPageTemplateFile(
246 '../templates/login-suspended-account.pt')247 '../templates/login-suspended-account.pt')
247248
249 def _gather_params(self, request):
250 params = dict(request.form)
251 query_string = request.get('QUERY_STRING')
252 if query_string is not None:
253 params.update(cgi.parse_qsl(query_string))
254 return params
255
256 def _get_requested_url(self, request):
257 requested_url = request.getURL()
258 query_string = request.get('QUERY_STRING')
259 if query_string is not None:
260 requested_url += '?' + query_string
261 return requested_url
262
248 def initialize(self):263 def initialize(self):
249 self.openid_response = self._getConsumer().complete(264 params = self._gather_params(self.request)
250 self.request.form, self.request.getURL())265 requested_url = self._get_requested_url(self.request)
266 consumer = self._getConsumer()
267 self.openid_response = consumer.complete(params, requested_url)
251268
252 def login(self, account):269 def login(self, account):
253 loginsource = getUtility(IPlacelessLoginSource)270 loginsource = getUtility(IPlacelessLoginSource)
@@ -350,7 +367,13 @@
350 def _redirect(self):367 def _redirect(self):
351 target = self.request.form.get('starting_url')368 target = self.request.form.get('starting_url')
352 if target is None:369 if target is None:
353 target = self.getApplicationURL()370 # If this was a POST, then the starting_url won't be in the form
371 # values, but in the query parameters instead.
372 target = self.request.query_string_params.get('starting_url')
373 if target is None:
374 target = self.request.getApplicationURL()
375 else:
376 target = target[0]
354 self.request.response.redirect(target, temporary_if_possible=True)377 self.request.response.redirect(target, temporary_if_possible=True)
355378
356379
357380
=== 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 @@
32from canonical.launchpad.webapp.login import OpenIDCallbackView, OpenIDLogin32from canonical.launchpad.webapp.login import OpenIDCallbackView, OpenIDLogin
33from canonical.launchpad.webapp.interfaces import IStoreSelector33from canonical.launchpad.webapp.interfaces import IStoreSelector
34from canonical.launchpad.webapp.servers import LaunchpadTestRequest34from canonical.launchpad.webapp.servers import LaunchpadTestRequest
35from canonical.testing.layers import AppServerLayer, DatabaseFunctionalLayer35from canonical.testing.layers import (
36 AppServerLayer, DatabaseFunctionalLayer, FunctionalLayer)
3637
37from lp.registry.interfaces.person import IPerson38from lp.registry.interfaces.person import IPerson
38from lp.testopenid.interfaces.server import ITestOpenIDPersistentIdentity39from lp.testopenid.interfaces.server import ITestOpenIDPersistentIdentity
@@ -62,6 +63,20 @@
62 "Not using the master store: %s" % current_policy)63 "Not using the master store: %s" % current_policy)
6364
6465
66class FakeConsumer:
67 """An OpenID consumer that stashes away arguments for test instection."""
68 def complete(self, params, requested_url):
69 self.params = params
70 self.requested_url = requested_url
71
72
73class FakeConsumerOpenIDCallbackView(OpenIDCallbackView):
74 """An OpenID handler with fake consumer so arguments can be inspected."""
75 def _getConsumer(self):
76 self.fake_consumer = FakeConsumer()
77 return self.fake_consumer
78
79
65@contextmanager80@contextmanager
66def SRegResponse_fromSuccessResponse_stubbed():81def SRegResponse_fromSuccessResponse_stubbed():
67 def sregFromFakeSuccessResponse(cls, success_response, signed_only=True):82 def sregFromFakeSuccessResponse(cls, success_response, signed_only=True):
@@ -154,6 +169,57 @@
154 # slave DBs.169 # slave DBs.
155 self.assertNotIn('last_write', ISession(view.request)['lp.dbpolicy'])170 self.assertNotIn('last_write', ISession(view.request)['lp.dbpolicy'])
156171
172 def test_gather_params(self):
173 # If the currently requested URL includes a query string, the
174 # parameters in the query string must be included when constructing
175 # the params mapping (which is then used to complete the open ID
176 # response). OpenIDCallbackView._gather_params does that gathering.
177 request = LaunchpadTestRequest(
178 SERVER_URL='http://example.com',
179 QUERY_STRING='foo=bar',
180 form={'starting_url': 'http://launchpad.dev/after-login'},
181 environ={'PATH_INFO': '/'})
182 view = OpenIDCallbackView(context=None, request=None)
183 params = view._gather_params(request)
184 expected_params = {
185 'starting_url': 'http://launchpad.dev/after-login',
186 'foo': 'bar',
187 }
188 self.assertEquals(params, expected_params)
189
190 def test_get_requested_url(self):
191 # The OpenIDCallbackView needs to pass the currently-being-requested
192 # URL to the OpenID library. OpenIDCallbackView._get_requested_url
193 # returns the URL.
194 request = LaunchpadTestRequest(
195 SERVER_URL='http://example.com',
196 QUERY_STRING='foo=bar',
197 form={'starting_url': 'http://launchpad.dev/after-login'})
198 view = OpenIDCallbackView(context=None, request=None)
199 url = view._get_requested_url(request)
200 self.assertEquals(url, 'http://example.com?foo=bar')
201
202 def test_open_id_callback_handles_query_string(self):
203 # If the currently requested URL includes a query string, the
204 # parameters in the query string must be included when constructing
205 # the params mapping (which is then used to complete the open ID
206 # response).
207 request = LaunchpadTestRequest(
208 SERVER_URL='http://example.com',
209 QUERY_STRING='foo=bar',
210 form={'starting_url': 'http://launchpad.dev/after-login'},
211 environ={'PATH_INFO': '/'})
212 view = FakeConsumerOpenIDCallbackView(object(), request)
213 view.initialize()
214 self.assertEquals(
215 view.fake_consumer.params,
216 {
217 'starting_url': 'http://launchpad.dev/after-login',
218 'foo': 'bar',
219 })
220 self.assertEquals(
221 view.fake_consumer.requested_url,'http://example.com?foo=bar')
222
157 def test_personless_account(self):223 def test_personless_account(self):
158 # When there is no Person record associated with the account, we224 # When there is no Person record associated with the account, we
159 # create one.225 # create one.
@@ -339,6 +405,50 @@
339 self.assertTrue(datetime.utcnow() - last_write < timedelta(minutes=1))405 self.assertTrue(datetime.utcnow() - last_write < timedelta(minutes=1))
340406
341407
408class TestOpenIDCallbackRedirects(TestCaseWithFactory):
409 layer = FunctionalLayer
410
411 APPLICATION_URL = 'http://example.com'
412 STARTING_URL = APPLICATION_URL + '/start'
413
414 def test_open_id_callback_redirect_from_get(self):
415 # If OpenID callback request was a GET, the starting_url is extracted
416 # correctly.
417 view = OpenIDCallbackView(context=None, request=None)
418 view.request = LaunchpadTestRequest(
419 SERVER_URL=self.APPLICATION_URL,
420 form={'starting_url': self.STARTING_URL})
421 view._redirect()
422 self.assertEquals(
423 httplib.TEMPORARY_REDIRECT, view.request.response.getStatus())
424 self.assertEquals(
425 view.request.response.getHeader('Location'), self.STARTING_URL)
426
427 def test_open_id_callback_redirect_from_post(self):
428 # If OpenID callback request was a POST, the starting_url is extracted
429 # correctly.
430 view = OpenIDCallbackView(context=None, request=None)
431 view.request = LaunchpadTestRequest(
432 SERVER_URL=self.APPLICATION_URL, form={'fake': 'value'},
433 QUERY_STRING='starting_url='+self.STARTING_URL)
434 view._redirect()
435 self.assertEquals(
436 httplib.TEMPORARY_REDIRECT, view.request.response.getStatus())
437 self.assertEquals(
438 view.request.response.getHeader('Location'), self.STARTING_URL)
439
440 def test_openid_callback_redirect_fallback(self):
441 # If OpenID callback request was a POST or GET with no form or query
442 # string values at all, then the application URL is used.
443 view = OpenIDCallbackView(context=None, request=None)
444 view.request = LaunchpadTestRequest(SERVER_URL=self.APPLICATION_URL)
445 view._redirect()
446 self.assertEquals(
447 httplib.TEMPORARY_REDIRECT, view.request.response.getStatus())
448 self.assertEquals(
449 view.request.response.getHeader('Location'), self.APPLICATION_URL)
450
451
342urls_redirected_to = []452urls_redirected_to = []
343453
344454
345455
=== 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 @@
22from zope.session.interfaces import ISession22from zope.session.interfaces import ISession
2323
24from openid import oidutil24from openid import oidutil
25from openid.server.server import CheckIDRequest, Server25from openid.server.server import CheckIDRequest, Server, ENCODE_HTML_FORM
26from openid.store.memstore import MemoryStore26from openid.store.memstore import MemoryStore
27from openid.extensions.sreg import SRegRequest, SRegResponse27from openid.extensions.sreg import SRegRequest, SRegResponse
2828
@@ -170,6 +170,10 @@
170 webresponse = self.openid_server.encodeResponse(openid_response)170 webresponse = self.openid_server.encodeResponse(openid_response)
171 response = self.request.response171 response = self.request.response
172 response.setStatus(webresponse.code)172 response.setStatus(webresponse.code)
173 # encodeResponse doesn't generate a content-type, help it out
174 if (webresponse.code == 200 and webresponse.body
175 and openid_response.whichEncoding() == ENCODE_HTML_FORM):
176 response.setHeader('content-type', 'text/html')
173 for header, value in webresponse.headers.items():177 for header, value in webresponse.headers.items():
174 response.setHeader(header, value)178 response.setHeader(header, value)
175 return webresponse.body179 return webresponse.body