launchpad-reviewers team mailing list archive

[Deryck Hodge <deryck.hodge@xxxxxxxxxxxxx>]

Deryck Hodge has proposed merging lp:~deryck/launchpad/email-update-login-stuck-1036267 into lp:launchpad.

Requested reviews:
  Deryck Hodge (deryck)
Related bugs:
  Bug #1036267 in Launchpad itself: "Cannot edit email addresses"
  https://bugs.launchpad.net/launchpad/+bug/1036267

For more details, see:
https://code.launchpad.net/~deryck/launchpad/email-update-login-stuck-1036267/+merge/119381

This change has already landed once, been reverted, and partially landed again.  This branch is to complete the merge of the work I did to force re-authentication when editing email addresses.  I'm self reviewing since it's all been reviewed twice now, but the merge was messed up somehow.
-- 
https://code.launchpad.net/~deryck/launchpad/email-update-login-stuck-1036267/+merge/119381
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/services/webapp/login.py'
--- lib/lp/services/webapp/login.py	2012-08-10 18:16:36 +0000
+++ lib/lp/services/webapp/login.py	2012-08-13 16:30:41 +0000
@@ -16,7 +16,10 @@
     FAILURE,
     SUCCESS,
     )
-from openid.extensions import sreg
+from openid.extensions import (
+    pape,
+    sreg,
+    )
 from openid.fetchers import (
     setDefaultFetcher,
     Urllib2Fetcher,
@@ -168,7 +171,10 @@
         return Consumer(session, openid_store)
 
     def render(self):
-        if self.account is not None:
+        # Reauthentication is called for by a query string parameter.
+        reauth_qs = self.request.query_string_params.get('reauth', ['0'])
+        do_reauth = int(reauth_qs[0])
+        if self.account is not None and not do_reauth:
             return AlreadyLoggedInView(self.context, self.request)()
 
         # Allow unauthenticated users to have sessions for the OpenID
@@ -189,6 +195,11 @@
         self.openid_request.addExtension(
             sreg.SRegRequest(required=['email', 'fullname']))
 
+        # Force the Open ID handshake to re-authenticate, using
+        # pape extension's max_auth_age, if the URL indicates it.
+        if do_reauth:
+            self.openid_request.addExtension(pape.Request(max_auth_age=0))
+
         assert not self.openid_request.shouldSendRedirect(), (
             "Our fixed OpenID server should not need us to redirect.")
         # Once the user authenticates with the OpenID provider they will be
@@ -216,7 +227,7 @@
     def starting_url(self):
         starting_url = self.request.getURL(1)
         query_string = "&".join([arg for arg in self.form_args])
-        if query_string:
+        if query_string and query_string != 'reauth=1':
             starting_url += "?%s" % query_string
         return starting_url
 

=== modified file 'lib/lp/services/webapp/tests/test_login.py'
--- lib/lp/services/webapp/tests/test_login.py	2012-08-08 13:41:05 +0000
+++ lib/lp/services/webapp/tests/test_login.py	2012-08-13 16:30:41 +0000
@@ -28,7 +28,10 @@
     FAILURE,
     SUCCESS,
     )
-from openid.extensions import sreg
+from openid.extensions import (
+    pape,
+    sreg,
+    )
 from openid.yadis.discover import DiscoveryFailure
 from testtools.matchers import Contains
 from zope.component import getUtility
@@ -731,6 +734,22 @@
         self.assertEquals(sorted(sreg_extension.required),
                           sorted(sreg_extension.allRequestedFields()))
 
+    def test_pape_extension_added_with_reauth_query(self):
+        # We can signal that a request should be reauthenticated via
+        # a reauth URL parameter, which should add PAPE extension's
+        # max_auth_age paramter.
+        request = LaunchpadTestRequest(QUERY_STRING='reauth=1')
+        # This is a hack to make the request.getURL(1) call issued by the view
+        # not raise an IndexError.
+        request._app_names = ['foo']
+        view = StubbedOpenIDLogin(object(), request)
+        view()
+        extensions = view.openid_request.extensions
+        self.assertIsNot(None, extensions)
+        pape_extension = extensions[1]
+        self.assertIsInstance(pape_extension, pape.Request)
+        self.assertEqual(0, pape_extension.max_auth_age)
+
     def test_logs_to_timeline(self):
         # Beginning an OpenID association makes an HTTP request to the
         # OP, so it's a potentially long action. It is logged to the