Merge lp:~salgado/launchpad/bug-547054 into lp:launchpad/db-devel

[Guilherme Salgado]

Do not render the login-error page when the user is already logged in.
Instead, redirect to the starting_url defined in the OpenID response and
add a notification message to the response.

--
Guilherme Salgado <email address hidden>

[Brad Crittenden (bac)]

review: Approve (code)

[Francis J. Lacoste (flacoste)]

review: Approve (release-critical)

=== modified file 'lib/canonical/launchpad/webapp/login.py'
--- lib/canonical/launchpad/webapp/login.py	2010-03-18 15:21:03 +0000
+++ lib/canonical/launchpad/webapp/login.py	2010-03-30 15:23:29 +0000
@@ -284,7 +284,6 @@
     def render(self):
         if self.openid_response.status == SUCCESS:
             identifier = self.openid_response.identity_url.split('/')[-1]
-            account_set = getUtility(IAccountSet)
             should_update_last_write = False
             # Force the use of the master database to make sure a lagged slave
             # doesn't fool us into creating a Person/Account when one already
@@ -311,15 +310,23 @@
                 # the master DB and thus see the changes we've just made.
                 session_data = ISession(self.request)['lp.dbpolicy']
                 session_data['last_write'] = datetime.utcnow()
-            target = self.request.form.get('starting_url')
-            if target is None:
-                target = self.getApplicationURL()
-            self.request.response.redirect(target, temporary_if_possible=True)
+            self._redirect()
             # No need to return anything as we redirect above.
             retval = None
         else:
-            retval = OpenIDLoginErrorView(
-                self.context, self.request, self.openid_response)()
+            if self.account is not None:
+                # The authentication failed (or was canceled), but the user is
+                # already logged in, so we just add a notification message and
+                # redirect.
+                self.request.response.addNoticeNotification(
+                    _(u'Your authentication failed but you were already '
+                       'logged into Launchpad.'))
+                self._redirect()
+                # No need to return anything as we redirect above.
+                retval = None
+            else:
+                retval = OpenIDLoginErrorView(
+                    self.context, self.request, self.openid_response)()
 
         # The consumer.complete() call above will create entries in
         # OpenIDConsumerNonce to prevent replay attacks, but since this will
@@ -329,6 +336,12 @@
 
         return retval
 
+    def _redirect(self):
+        target = self.request.form.get('starting_url')
+        if target is None:
+            target = self.getApplicationURL()
+        self.request.response.redirect(target, temporary_if_possible=True)
+
 
 class OpenIDLoginErrorView(LaunchpadView):
 

=== modified file 'lib/canonical/launchpad/webapp/tests/test_login.py'
--- lib/canonical/launchpad/webapp/tests/test_login.py	2010-03-04 14:03:34 +0000
+++ lib/canonical/launchpad/webapp/tests/test_login.py	2010-03-30 15:23:29 +0000
@@ -107,13 +107,16 @@
     layer = DatabaseFunctionalLayer
 
     def _createViewWithResponse(
-            self, account, response_status=SUCCESS, response_msg=''):
+            self, account, response_status=SUCCESS, response_msg='',
+            view_class=StubbedOpenIDCallbackView):
         openid_response = FakeOpenIDResponse(
             ITestOpenIDPersistentIdentity(account).openid_identity_url,
             status=response_status, message=response_msg)
-        return self._createAndRenderView(openid_response)
+        return self._createAndRenderView(
+            openid_response, view_class=view_class)
 
-    def _createAndRenderView(self, response):
+    def _createAndRenderView(self, response,
+                             view_class=StubbedOpenIDCallbackView):
         request = LaunchpadTestRequest(
             form={'starting_url': 'http://launchpad.dev/after-login'},
             environ={'PATH_INFO': '/'})
@@ -122,7 +125,7 @@
         # setup a newInteraction() using our request.
         logout()
         newInteraction(request)
-        view = StubbedOpenIDCallbackView(object(), request)
+        view = view_class(object(), request)
         view.initialize()
         view.openid_response = response
         # Monkey-patch getByOpenIDIdentifier() to make sure the view uses the
@@ -235,6 +238,28 @@
         main_content = extract_text(find_main_content(html))
         self.assertIn('Your login was unsuccessful', main_content)
 
+    def test_negative_openid_assertion_when_user_already_logged_in(self):
+        # The OpenID provider responded with a negative assertion, but the
+        # user already has a valid cookie, so we add a notification message to
+        # the response and redirect to the starting_url specified in the
+        # OpenID response.
+        test_account = self.factory.makeAccount('Test account')
+        class StubbedOpenIDCallbackViewLoggedIn(StubbedOpenIDCallbackView):
+            account = test_account
+        view, html = self._createViewWithResponse(
+            test_account, response_status=FAILURE,
+            response_msg='Server denied check_authentication',
+            view_class=StubbedOpenIDCallbackViewLoggedIn)
+        self.assertFalse(view.login_called)
+        response = view.request.response
+        self.assertEquals(httplib.TEMPORARY_REDIRECT, response.getStatus())
+        self.assertEquals(view.request.form['starting_url'],
+                          response.getHeader('Location'))
+        notification_msg = view.request.response.notifications[0].message
+        expected_msg = ('Your authentication failed but you were already '
+                        'logged into Launchpad')
+        self.assertIn(expected_msg, notification_msg)
+
     def test_IAccountSet_getByOpenIDIdentifier_monkey_patched(self):
         with IAccountSet_getByOpenIDIdentifier_monkey_patched():
             self.assertRaises(