Merge lp:~wallyworld/launchpad/openid-oops-810623 into lp:launchpad

Proposed by Ian Booth
Status: Merged
Approved by: Curtis Hovey
Approved revision: no longer in the source branch.
Merged at revision: 16173
Proposed branch: lp:~wallyworld/launchpad/openid-oops-810623
Merge into: lp:launchpad
Diff against target: 86 lines (+33/-6)
2 files modified
lib/lp/services/webapp/login.py (+20/-6)
lib/lp/services/webapp/tests/test_login.py (+13/-0)
To merge this branch: bzr merge lp:~wallyworld/launchpad/openid-oops-810623
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+130467@code.launchpad.net

Commit message

Display a login error rather than oopsing if open id provider sends a response with missing fields

Description of the change

== Implementation ==

When the response from the open id provider does not contain the expected full name and email fields, display the error page rather than oopsing.

== Tests ==

Add new test to TestOpenIDCallbackView

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/services/webapp/login.py
  lib/lp/services/webapp/tests/test_login.py

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

Thank. I really do dislike the "assert" keyword

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/services/webapp/login.py'
2--- lib/lp/services/webapp/login.py 2012-09-28 06:01:02 +0000
3+++ lib/lp/services/webapp/login.py 2012-10-19 02:20:31 +0000
4@@ -24,6 +24,10 @@
5 setDefaultFetcher,
6 Urllib2Fetcher,
7 )
8+from paste.httpexceptions import (
9+ HTTPBadRequest,
10+ HTTPException,
11+ )
12 import transaction
13 from z3c.ptcompat import ViewPageTemplateFile
14 from zope.app.security.interfaces import IUnauthenticatedPrincipal
15@@ -333,12 +337,14 @@
16 # asked to. Once we start using other OPs we won't be able to
17 # make this assumption here as they might not include what we
18 # want in the response.
19- assert self.sreg_response is not None, (
20- "OP didn't include an sreg extension in the response.")
21+ if self.sreg_response is None:
22+ raise HTTPBadRequest(
23+ "OP didn't include an sreg extension in the response.")
24 email_address = self.sreg_response.get('email')
25 full_name = self.sreg_response.get('fullname')
26- assert email_address is not None and full_name is not None, (
27- "No email address or full name found in sreg response")
28+ if email_address is None or full_name is None:
29+ raise HTTPBadRequest(
30+ "No email address or full name found in sreg response.")
31 return email_address, full_name
32
33 def processPositiveAssertion(self):
34@@ -389,7 +395,11 @@
35
36 def render(self):
37 if self.openid_response.status == SUCCESS:
38- return self.processPositiveAssertion()
39+ try:
40+ return self.processPositiveAssertion()
41+ except HTTPException as error:
42+ return OpenIDLoginErrorView(
43+ self.context, self.request, login_error=error.message)()
44
45 if self.account is not None:
46 # The authentication failed (or was canceled), but the user is
47@@ -432,10 +442,14 @@
48 page_title = 'Error logging in'
49 template = ViewPageTemplateFile("templates/login-error.pt")
50
51- def __init__(self, context, request, openid_response):
52+ def __init__(self, context, request, openid_response=None,
53+ login_error=None):
54 super(OpenIDLoginErrorView, self).__init__(context, request)
55 assert self.account is None, (
56 "Don't try to render this page when the user is logged in.")
57+ if login_error:
58+ self.login_error = login_error
59+ return
60 if openid_response.status == CANCEL:
61 self.login_error = "User cancelled"
62 elif openid_response.status == FAILURE:
63
64=== modified file 'lib/lp/services/webapp/tests/test_login.py'
65--- lib/lp/services/webapp/tests/test_login.py 2012-09-28 06:25:44 +0000
66+++ lib/lp/services/webapp/tests/test_login.py 2012-10-19 02:20:31 +0000
67@@ -439,6 +439,19 @@
68 main_content = extract_text(find_main_content(html))
69 self.assertIn('Team email address conflict', main_content)
70
71+ def test_missing_fields(self):
72+ # If the OpenID provider response does not include required fields
73+ # (full name or email missing), the login error page is shown.
74+ person = self.factory.makePerson()
75+ with SRegResponse_fromSuccessResponse_stubbed():
76+ view, html = self._createViewWithResponse(
77+ person.account, email=None)
78+ self.assertFalse(view.login_called)
79+ main_content = extract_text(find_main_content(html))
80+ self.assertIn(
81+ 'No email address or full name found in sreg response',
82+ main_content)
83+
84 def test_negative_openid_assertion(self):
85 # The OpenID provider responded with a negative assertion, so the
86 # login error page is shown.