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: Gary Poster
Approved revision: no longer in the source branch.
Merged at revision: 11221
Proposed branch: lp:~benji/launchpad/bug-597324
Merge into: lp:launchpad
Diff against target: 81 lines (+57/-3)
2 files modified
lib/canonical/launchpad/webapp/login.py (+15/-3)
lib/canonical/launchpad/webapp/tests/test_login.py (+42/-0)
To merge this branch: bzr merge lp:~benji/launchpad/bug-597324
Reviewer Review Type Date Requested Status
Gary Poster (community) Approve
Review via email: mp+30829@code.launchpad.net

Description of the change

Fix a unicode sin: use the decoded query parameters instead of creating
new, undecoded copies.

This /should/ fix the OOPS introduced in on edge by this branch (see bug
609029).

I also added a test to ensure this particular failure doesn't reoccur.

To post a comment you must log in.
Revision history for this message
Gary Poster (gary) wrote :

merge-conditional

Wow, great find!

Also, your cover letter neglected to mention that this fixes the problems discovered in QA for the original bug, but your commit messages clarify it. :-)

- Please file a bug against canonical-openid-provider about removing the additional "csrfmiddlewaretoken" field and add a reference to the bug at the top of the pertinent comment below (e.g., something like ``# XXX benji 2010-07-23 bug=12345``)

- I don't love the assert error for multi-valued fields. It feels like it ought to be a real exception that has a chance of being handled by one of our error views. However, the right error doesn't jump out at me, so I'll let it go unless you see where I'm coming from and have a good idea.

Thank you!

Gary

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

On Fri, Jul 23, 2010 at 5:52 PM, Gary Poster <email address hidden> wrote:
> - Please file a bug against canonical-openid-provider about removing
> the additional "csrfmiddlewaretoken" field and add a reference to the
> bug at the top of the pertinent comment below (e.g., something like
> ``# XXX benji 2010-07-23 bug=12345``)

https://bugs.edge.launchpad.net/canonical-identity-provider/+bug/608920

> - I don't love the assert error for multi-valued fields.  It feels
> like it ought to be a real exception that has a chance of being
> handled by one of our error views.  However, the right error doesn't
> jump out at me, so I'll let it go unless you see where I'm coming from
> and have a good idea.

Switched to a ValueError in r11111.
--
Benji York

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-21 13:32:53 +0000
3+++ lib/canonical/launchpad/webapp/login.py 2010-07-24 13:17:56 +0000
4@@ -248,9 +248,21 @@
5
6 def _gather_params(self, request):
7 params = dict(request.form)
8- query_string = request.get('QUERY_STRING')
9- if query_string is not None:
10- params.update(cgi.parse_qsl(query_string))
11+ for key, value in request.query_string_params.iteritems():
12+ if len(value) > 1:
13+ raise ValueError(
14+ 'Did not expect multi-valued fields.')
15+ params[key] = value[0]
16+
17+ # XXX benji 2010-07-23 bug=608920
18+ # The production OpenID provider has some Django middleware that
19+ # generates a token used to prevent XSRF attacks and stuffs it into
20+ # every form. Unfortunately that includes forms that have off-site
21+ # targets and since our OpenID client verifies that no form values have
22+ # been injected as a security precaution, this breaks logging-in in
23+ # certain circumstances (see bug 597324). The best we can do at the
24+ # moment is to remove the token before invoking the OpenID library.
25+ params.pop('csrfmiddlewaretoken', None)
26 return params
27
28 def _get_requested_url(self, request):
29
30=== modified file 'lib/canonical/launchpad/webapp/tests/test_login.py'
31--- lib/canonical/launchpad/webapp/tests/test_login.py 2010-07-21 13:32:53 +0000
32+++ lib/canonical/launchpad/webapp/tests/test_login.py 2010-07-24 13:17:56 +0000
33@@ -187,6 +187,48 @@
34 }
35 self.assertEquals(params, expected_params)
36
37+ def test_gather_params_with_unicode_data(self):
38+ # If the currently requested URL includes a query string, the
39+ # parameters in the query string will be included when constructing
40+ # the params mapping (which is then used to complete the open ID
41+ # response) and if there are non-ASCII characters in the query string,
42+ # they are properly decoded.
43+ request = LaunchpadTestRequest(
44+ SERVER_URL='http://example.com',
45+ QUERY_STRING='foo=%E1%9B%9D',
46+ environ={'PATH_INFO': '/'})
47+ view = OpenIDCallbackView(context=None, request=None)
48+ params = view._gather_params(request)
49+ self.assertEquals(params['foo'], u'\u16dd')
50+
51+ def test_unexpected_multivalue_fields(self):
52+ # The parameter gatering doesn't expect to find multi-valued form
53+ # field and it reports an error if it finds any.
54+ request = LaunchpadTestRequest(
55+ SERVER_URL='http://example.com',
56+ QUERY_STRING='foo=1&foo=2',
57+ environ={'PATH_INFO': '/'})
58+ view = OpenIDCallbackView(context=None, request=None)
59+ self.assertRaises(ValueError, view._gather_params, request)
60+
61+ def test_csrfmiddlewaretoken_is_ignored(self):
62+ # Show that the _gather_params filters out the errant
63+ # csrfmiddlewaretoken form field. See comment in _gather_params for
64+ # more info.
65+ request = LaunchpadTestRequest(
66+ SERVER_URL='http://example.com',
67+ QUERY_STRING='foo=bar',
68+ form={'starting_url': 'http://launchpad.dev/after-login',
69+ 'csrfmiddlewaretoken': '12345'},
70+ environ={'PATH_INFO': '/'})
71+ view = OpenIDCallbackView(context=None, request=None)
72+ params = view._gather_params(request)
73+ expected_params = {
74+ 'starting_url': 'http://launchpad.dev/after-login',
75+ 'foo': 'bar',
76+ }
77+ self.assertEquals(params, expected_params)
78+
79 def test_get_requested_url(self):
80 # The OpenIDCallbackView needs to pass the currently-being-requested
81 # URL to the OpenID library. OpenIDCallbackView._get_requested_url