launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/openid-oops-810623 into lp:launchpad.

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #810623 in Launchpad itself: "launchpad oopses when no sreg attributes are returned by openid OP"
  https://bugs.launchpad.net/launchpad/+bug/810623

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/openid-oops-810623/+merge/130467

== 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

-- 
https://code.launchpad.net/~wallyworld/launchpad/openid-oops-810623/+merge/130467
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/openid-oops-810623 into lp:launchpad.

=== modified file 'lib/lp/services/webapp/login.py'
--- lib/lp/services/webapp/login.py	2012-09-28 06:01:02 +0000
+++ lib/lp/services/webapp/login.py	2012-10-19 02:20:31 +0000
@@ -24,6 +24,10 @@
     setDefaultFetcher,
     Urllib2Fetcher,
     )
+from paste.httpexceptions import (
+    HTTPBadRequest,
+    HTTPException,
+    )
 import transaction
 from z3c.ptcompat import ViewPageTemplateFile
 from zope.app.security.interfaces import IUnauthenticatedPrincipal
@@ -333,12 +337,14 @@
         # asked to.  Once we start using other OPs we won't be able to
         # make this assumption here as they might not include what we
         # want in the response.
-        assert self.sreg_response is not None, (
-            "OP didn't include an sreg extension in the response.")
+        if self.sreg_response is None:
+            raise HTTPBadRequest(
+                "OP didn't include an sreg extension in the response.")
         email_address = self.sreg_response.get('email')
         full_name = self.sreg_response.get('fullname')
-        assert email_address is not None and full_name is not None, (
-            "No email address or full name found in sreg response")
+        if email_address is None or full_name is None:
+            raise HTTPBadRequest(
+                "No email address or full name found in sreg response.")
         return email_address, full_name
 
     def processPositiveAssertion(self):
@@ -389,7 +395,11 @@
 
     def render(self):
         if self.openid_response.status == SUCCESS:
-            return self.processPositiveAssertion()
+            try:
+                return self.processPositiveAssertion()
+            except HTTPException as error:
+                return OpenIDLoginErrorView(
+                    self.context, self.request, login_error=error.message)()
 
         if self.account is not None:
             # The authentication failed (or was canceled), but the user is
@@ -432,10 +442,14 @@
     page_title = 'Error logging in'
     template = ViewPageTemplateFile("templates/login-error.pt")
 
-    def __init__(self, context, request, openid_response):
+    def __init__(self, context, request, openid_response=None,
+                 login_error=None):
         super(OpenIDLoginErrorView, self).__init__(context, request)
         assert self.account is None, (
             "Don't try to render this page when the user is logged in.")
+        if login_error:
+            self.login_error = login_error
+            return
         if openid_response.status == CANCEL:
             self.login_error = "User cancelled"
         elif openid_response.status == FAILURE:

=== modified file 'lib/lp/services/webapp/tests/test_login.py'
--- lib/lp/services/webapp/tests/test_login.py	2012-09-28 06:25:44 +0000
+++ lib/lp/services/webapp/tests/test_login.py	2012-10-19 02:20:31 +0000
@@ -439,6 +439,19 @@
         main_content = extract_text(find_main_content(html))
         self.assertIn('Team email address conflict', main_content)
 
+    def test_missing_fields(self):
+        # If the OpenID provider response does not include required fields
+        # (full name or email missing), the login error page is shown.
+        person = self.factory.makePerson()
+        with SRegResponse_fromSuccessResponse_stubbed():
+            view, html = self._createViewWithResponse(
+                person.account, email=None)
+        self.assertFalse(view.login_called)
+        main_content = extract_text(find_main_content(html))
+        self.assertIn(
+            'No email address or full name found in sreg response',
+            main_content)
+
     def test_negative_openid_assertion(self):
         # The OpenID provider responded with a negative assertion, so the
         # login error page is shown.