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

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
1=== modified file 'lib/canonical/launchpad/webapp/login.py'
2--- lib/canonical/launchpad/webapp/login.py 2010-07-02 10:37:33 +0000
3+++ lib/canonical/launchpad/webapp/login.py 2010-07-20 19:23:51 +0000
4@@ -1,11 +1,12 @@
5 # Copyright 2009 Canonical Ltd. This software is licensed under the
6 # GNU Affero General Public License version 3 (see the file LICENSE).
7+"""Stuff to do with logging in and logging out."""
8+
9 from __future__ import with_statement
10
11-"""Stuff to do with logging in and logging out."""
12-
13 __metaclass__ = type
14
15+import cgi
16 import urllib
17
18 from datetime import datetime, timedelta
19@@ -245,9 +246,25 @@
20 suspended_account_template = ViewPageTemplateFile(
21 '../templates/login-suspended-account.pt')
22
23+ def _gather_params(self, request):
24+ params = dict(request.form)
25+ query_string = request.get('QUERY_STRING')
26+ if query_string is not None:
27+ params.update(cgi.parse_qsl(query_string))
28+ return params
29+
30+ def _get_requested_url(self, request):
31+ requested_url = request.getURL()
32+ query_string = request.get('QUERY_STRING')
33+ if query_string is not None:
34+ requested_url += '?' + query_string
35+ return requested_url
36+
37 def initialize(self):
38- self.openid_response = self._getConsumer().complete(
39- self.request.form, self.request.getURL())
40+ params = self._gather_params(self.request)
41+ requested_url = self._get_requested_url(self.request)
42+ consumer = self._getConsumer()
43+ self.openid_response = consumer.complete(params, requested_url)
44
45 def login(self, account):
46 loginsource = getUtility(IPlacelessLoginSource)
47@@ -350,7 +367,13 @@
48 def _redirect(self):
49 target = self.request.form.get('starting_url')
50 if target is None:
51- target = self.getApplicationURL()
52+ # If this was a POST, then the starting_url won't be in the form
53+ # values, but in the query parameters instead.
54+ target = self.request.query_string_params.get('starting_url')
55+ if target is None:
56+ target = self.request.getApplicationURL()
57+ else:
58+ target = target[0]
59 self.request.response.redirect(target, temporary_if_possible=True)
60
61
62
63=== modified file 'lib/canonical/launchpad/webapp/tests/test_login.py'
64--- lib/canonical/launchpad/webapp/tests/test_login.py 2010-07-01 10:04:54 +0000
65+++ lib/canonical/launchpad/webapp/tests/test_login.py 2010-07-20 19:23:51 +0000
66@@ -32,7 +32,8 @@
67 from canonical.launchpad.webapp.login import OpenIDCallbackView, OpenIDLogin
68 from canonical.launchpad.webapp.interfaces import IStoreSelector
69 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
70-from canonical.testing.layers import AppServerLayer, DatabaseFunctionalLayer
71+from canonical.testing.layers import (
72+ AppServerLayer, DatabaseFunctionalLayer, FunctionalLayer)
73
74 from lp.registry.interfaces.person import IPerson
75 from lp.testopenid.interfaces.server import ITestOpenIDPersistentIdentity
76@@ -62,6 +63,20 @@
77 "Not using the master store: %s" % current_policy)
78
79
80+class FakeConsumer:
81+ """An OpenID consumer that stashes away arguments for test instection."""
82+ def complete(self, params, requested_url):
83+ self.params = params
84+ self.requested_url = requested_url
85+
86+
87+class FakeConsumerOpenIDCallbackView(OpenIDCallbackView):
88+ """An OpenID handler with fake consumer so arguments can be inspected."""
89+ def _getConsumer(self):
90+ self.fake_consumer = FakeConsumer()
91+ return self.fake_consumer
92+
93+
94 @contextmanager
95 def SRegResponse_fromSuccessResponse_stubbed():
96 def sregFromFakeSuccessResponse(cls, success_response, signed_only=True):
97@@ -154,6 +169,57 @@
98 # slave DBs.
99 self.assertNotIn('last_write', ISession(view.request)['lp.dbpolicy'])
100
101+ def test_gather_params(self):
102+ # If the currently requested URL includes a query string, the
103+ # parameters in the query string must be included when constructing
104+ # the params mapping (which is then used to complete the open ID
105+ # response). OpenIDCallbackView._gather_params does that gathering.
106+ request = LaunchpadTestRequest(
107+ SERVER_URL='http://example.com',
108+ QUERY_STRING='foo=bar',
109+ form={'starting_url': 'http://launchpad.dev/after-login'},
110+ environ={'PATH_INFO': '/'})
111+ view = OpenIDCallbackView(context=None, request=None)
112+ params = view._gather_params(request)
113+ expected_params = {
114+ 'starting_url': 'http://launchpad.dev/after-login',
115+ 'foo': 'bar',
116+ }
117+ self.assertEquals(params, expected_params)
118+
119+ def test_get_requested_url(self):
120+ # The OpenIDCallbackView needs to pass the currently-being-requested
121+ # URL to the OpenID library. OpenIDCallbackView._get_requested_url
122+ # returns the URL.
123+ request = LaunchpadTestRequest(
124+ SERVER_URL='http://example.com',
125+ QUERY_STRING='foo=bar',
126+ form={'starting_url': 'http://launchpad.dev/after-login'})
127+ view = OpenIDCallbackView(context=None, request=None)
128+ url = view._get_requested_url(request)
129+ self.assertEquals(url, 'http://example.com?foo=bar')
130+
131+ def test_open_id_callback_handles_query_string(self):
132+ # If the currently requested URL includes a query string, the
133+ # parameters in the query string must be included when constructing
134+ # the params mapping (which is then used to complete the open ID
135+ # response).
136+ request = LaunchpadTestRequest(
137+ SERVER_URL='http://example.com',
138+ QUERY_STRING='foo=bar',
139+ form={'starting_url': 'http://launchpad.dev/after-login'},
140+ environ={'PATH_INFO': '/'})
141+ view = FakeConsumerOpenIDCallbackView(object(), request)
142+ view.initialize()
143+ self.assertEquals(
144+ view.fake_consumer.params,
145+ {
146+ 'starting_url': 'http://launchpad.dev/after-login',
147+ 'foo': 'bar',
148+ })
149+ self.assertEquals(
150+ view.fake_consumer.requested_url,'http://example.com?foo=bar')
151+
152 def test_personless_account(self):
153 # When there is no Person record associated with the account, we
154 # create one.
155@@ -339,6 +405,50 @@
156 self.assertTrue(datetime.utcnow() - last_write < timedelta(minutes=1))
157
158
159+class TestOpenIDCallbackRedirects(TestCaseWithFactory):
160+ layer = FunctionalLayer
161+
162+ APPLICATION_URL = 'http://example.com'
163+ STARTING_URL = APPLICATION_URL + '/start'
164+
165+ def test_open_id_callback_redirect_from_get(self):
166+ # If OpenID callback request was a GET, the starting_url is extracted
167+ # correctly.
168+ view = OpenIDCallbackView(context=None, request=None)
169+ view.request = LaunchpadTestRequest(
170+ SERVER_URL=self.APPLICATION_URL,
171+ form={'starting_url': self.STARTING_URL})
172+ view._redirect()
173+ self.assertEquals(
174+ httplib.TEMPORARY_REDIRECT, view.request.response.getStatus())
175+ self.assertEquals(
176+ view.request.response.getHeader('Location'), self.STARTING_URL)
177+
178+ def test_open_id_callback_redirect_from_post(self):
179+ # If OpenID callback request was a POST, the starting_url is extracted
180+ # correctly.
181+ view = OpenIDCallbackView(context=None, request=None)
182+ view.request = LaunchpadTestRequest(
183+ SERVER_URL=self.APPLICATION_URL, form={'fake': 'value'},
184+ QUERY_STRING='starting_url='+self.STARTING_URL)
185+ view._redirect()
186+ self.assertEquals(
187+ httplib.TEMPORARY_REDIRECT, view.request.response.getStatus())
188+ self.assertEquals(
189+ view.request.response.getHeader('Location'), self.STARTING_URL)
190+
191+ def test_openid_callback_redirect_fallback(self):
192+ # If OpenID callback request was a POST or GET with no form or query
193+ # string values at all, then the application URL is used.
194+ view = OpenIDCallbackView(context=None, request=None)
195+ view.request = LaunchpadTestRequest(SERVER_URL=self.APPLICATION_URL)
196+ view._redirect()
197+ self.assertEquals(
198+ httplib.TEMPORARY_REDIRECT, view.request.response.getStatus())
199+ self.assertEquals(
200+ view.request.response.getHeader('Location'), self.APPLICATION_URL)
201+
202+
203 urls_redirected_to = []
204
205
206
207=== modified file 'lib/lp/testopenid/browser/server.py'
208--- lib/lp/testopenid/browser/server.py 2010-07-01 13:00:27 +0000
209+++ lib/lp/testopenid/browser/server.py 2010-07-20 19:23:51 +0000
210@@ -22,7 +22,7 @@
211 from zope.session.interfaces import ISession
212
213 from openid import oidutil
214-from openid.server.server import CheckIDRequest, Server
215+from openid.server.server import CheckIDRequest, Server, ENCODE_HTML_FORM
216 from openid.store.memstore import MemoryStore
217 from openid.extensions.sreg import SRegRequest, SRegResponse
218
219@@ -170,6 +170,10 @@
220 webresponse = self.openid_server.encodeResponse(openid_response)
221 response = self.request.response
222 response.setStatus(webresponse.code)
223+ # encodeResponse doesn't generate a content-type, help it out
224+ if (webresponse.code == 200 and webresponse.body
225+ and openid_response.whichEncoding() == ENCODE_HTML_FORM):
226+ response.setHeader('content-type', 'text/html')
227 for header, value in webresponse.headers.items():
228 response.setHeader(header, value)
229 return webresponse.body