launchpad-reviewers team mailing list archive

[Deryck Hodge <deryck.hodge@xxxxxxxxxxxxx>]

Deryck Hodge has proposed merging lp:~deryck/launchpad/support-pape-max-auth-age into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~deryck/launchpad/support-pape-max-auth-age/+merge/117781

This branch is a first step in fixing bug 363916.  That bug notes that we need to re-authenticate users when they try to update their email, gpg, or ssh info.  In order to redirect them to Ubuntu SSO and refresh their login, we need to make use of PAPE extensions in our login process and set max_auth_age to 0.

This branch updates the login process to accept a reauth=1 query parameter.  If that param is present when you redirect a user to +login, then the user will be prompted to re-authenticate and get a fresh login.

This work is being landed in isolation.  There is a test to confirm this works, but otherwise, it's unused at this point.  The branch to make use of it for updating email addresses was growing large, so I split this off by itself.

We also need a newer version of python-openid that supports pape extensions, so I've updated versions.cfg, too.  This version of python-openid was committed to download-cache sometime ago, when I started in this work.  I confirmed with wgrant that we had our patch in the fork version applied upstream.  So we're fine to run a regular release now.

In terms of LOC change, this branch adds lines of code, but I have other branches that preceded this that changed doctests or dropped tests altogether which has given me some LOC credit for this work.  Those tests were related to updating emails and didn't play well with re-authenticating, so the doctests were dropped in favor of unit tests, which gave me about 350 LOC credit.

I had a couple pre-implementation style calls with Francis about this work when I started work on it.

The branch is lint free.
-- 
https://code.launchpad.net/~deryck/launchpad/support-pape-max-auth-age/+merge/117781
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~deryck/launchpad/support-pape-max-auth-age into lp:launchpad.

=== modified file 'lib/lp/services/webapp/login.py'
--- lib/lp/services/webapp/login.py	2012-01-20 06:29:10 +0000
+++ lib/lp/services/webapp/login.py	2012-08-06 13:07:21 +0000
@@ -16,7 +16,10 @@
     FAILURE,
     SUCCESS,
     )
-from openid.extensions import sreg
+from openid.extensions import (
+    pape,
+    sreg,
+    )
 from openid.fetchers import (
     setDefaultFetcher,
     Urllib2Fetcher,
@@ -194,7 +197,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
@@ -215,6 +221,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
@@ -242,7 +253,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-01-20 06:29:10 +0000
+++ lib/lp/services/webapp/tests/test_login.py	2012-08-06 13:07:21 +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

=== modified file 'versions.cfg'
--- versions.cfg	2012-08-06 09:29:34 +0000
+++ versions.cfg	2012-08-06 13:07:21 +0000
@@ -100,9 +100,7 @@
 # python setup.py egg_info -bDEV-r`bzr revno` sdist
 # mv dist/python-memcached-1.49DEV-r57.tar.gz [LOCATION]/download-cache/dist
 python-memcached = 1.49DEV-r57
-# 2.2.1 with the one-liner Expect: 100-continue fix from
-# lp:~wgrant/python-openid/python-openid-2.2.1-fix676372.
-python-openid = 2.2.1-fix676372
+python-openid = 2.2.5
 python-subunit = 0.0.8beta
 pytz = 2012c
 rdflib = 3.1.0