Merge lp:~deryck/launchpad/support-pape-max-auth-age into lp:launchpad

Proposed by Deryck Hodge
Status: Merged
Approved by: Deryck Hodge
Approved revision: no longer in the source branch.
Merged at revision: 15759
Proposed branch: lp:~deryck/launchpad/support-pape-max-auth-age
Merge into: lp:launchpad
Diff against target: 102 lines (+35/-7)
3 files modified
lib/lp/services/webapp/login.py (+14/-3)
lib/lp/services/webapp/tests/test_login.py (+20/-1)
versions.cfg (+1/-3)
To merge this branch: bzr merge lp:~deryck/launchpad/support-pape-max-auth-age
Reviewer Review Type Date Requested Status
Abel Deuring (community) code Approve
Review via email: mp+117781@code.launchpad.net

Commit message

Add support for PAPE extension max_auth_age to Launchpad login, which will allow us to force users to refresh their logins.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Abel Deuring (adeuring) wrote :

looks good

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
16 FAILURE,16 FAILURE,
17 SUCCESS,17 SUCCESS,
18 )18 )
19from openid.extensions import sreg19from openid.extensions import (
20 pape,
21 sreg,
22 )
20from openid.fetchers import (23from openid.fetchers import (
21 setDefaultFetcher,24 setDefaultFetcher,
22 Urllib2Fetcher,25 Urllib2Fetcher,
@@ -194,7 +197,10 @@
194 return Consumer(session, openid_store)197 return Consumer(session, openid_store)
195198
196 def render(self):199 def render(self):
197 if self.account is not None:200 # Reauthentication is called for by a query string parameter.
201 reauth_qs = self.request.query_string_params.get('reauth', ['0'])
202 do_reauth = int(reauth_qs[0])
203 if self.account is not None and not do_reauth:
198 return AlreadyLoggedInView(self.context, self.request)()204 return AlreadyLoggedInView(self.context, self.request)()
199205
200 # Allow unauthenticated users to have sessions for the OpenID206 # Allow unauthenticated users to have sessions for the OpenID
@@ -215,6 +221,11 @@
215 self.openid_request.addExtension(221 self.openid_request.addExtension(
216 sreg.SRegRequest(required=['email', 'fullname']))222 sreg.SRegRequest(required=['email', 'fullname']))
217223
224 # Force the Open ID handshake to re-authenticate, using
225 # pape extension's max_auth_age, if the URL indicates it.
226 if do_reauth:
227 self.openid_request.addExtension(pape.Request(max_auth_age=0))
228
218 assert not self.openid_request.shouldSendRedirect(), (229 assert not self.openid_request.shouldSendRedirect(), (
219 "Our fixed OpenID server should not need us to redirect.")230 "Our fixed OpenID server should not need us to redirect.")
220 # Once the user authenticates with the OpenID provider they will be231 # Once the user authenticates with the OpenID provider they will be
@@ -242,7 +253,7 @@
242 def starting_url(self):253 def starting_url(self):
243 starting_url = self.request.getURL(1)254 starting_url = self.request.getURL(1)
244 query_string = "&".join([arg for arg in self.form_args])255 query_string = "&".join([arg for arg in self.form_args])
245 if query_string:256 if query_string and query_string != 'reauth=1':
246 starting_url += "?%s" % query_string257 starting_url += "?%s" % query_string
247 return starting_url258 return starting_url
248259
249260
=== 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 @@
28 FAILURE,28 FAILURE,
29 SUCCESS,29 SUCCESS,
30 )30 )
31from openid.extensions import sreg31from openid.extensions import (
32 pape,
33 sreg,
34 )
32from openid.yadis.discover import DiscoveryFailure35from openid.yadis.discover import DiscoveryFailure
33from testtools.matchers import Contains36from testtools.matchers import Contains
34from zope.component import getUtility37from zope.component import getUtility
@@ -731,6 +734,22 @@
731 self.assertEquals(sorted(sreg_extension.required),734 self.assertEquals(sorted(sreg_extension.required),
732 sorted(sreg_extension.allRequestedFields()))735 sorted(sreg_extension.allRequestedFields()))
733736
737 def test_pape_extension_added_with_reauth_query(self):
738 # We can signal that a request should be reauthenticated via
739 # a reauth URL parameter, which should add PAPE extension's
740 # max_auth_age paramter.
741 request = LaunchpadTestRequest(QUERY_STRING='reauth=1')
742 # This is a hack to make the request.getURL(1) call issued by the view
743 # not raise an IndexError.
744 request._app_names = ['foo']
745 view = StubbedOpenIDLogin(object(), request)
746 view()
747 extensions = view.openid_request.extensions
748 self.assertIsNot(None, extensions)
749 pape_extension = extensions[1]
750 self.assertIsInstance(pape_extension, pape.Request)
751 self.assertEqual(0, pape_extension.max_auth_age)
752
734 def test_logs_to_timeline(self):753 def test_logs_to_timeline(self):
735 # Beginning an OpenID association makes an HTTP request to the754 # Beginning an OpenID association makes an HTTP request to the
736 # OP, so it's a potentially long action. It is logged to the755 # OP, so it's a potentially long action. It is logged to the
737756
=== 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 @@
100# python setup.py egg_info -bDEV-r`bzr revno` sdist100# python setup.py egg_info -bDEV-r`bzr revno` sdist
101# mv dist/python-memcached-1.49DEV-r57.tar.gz [LOCATION]/download-cache/dist101# mv dist/python-memcached-1.49DEV-r57.tar.gz [LOCATION]/download-cache/dist
102python-memcached = 1.49DEV-r57102python-memcached = 1.49DEV-r57
103# 2.2.1 with the one-liner Expect: 100-continue fix from103python-openid = 2.2.5
104# lp:~wgrant/python-openid/python-openid-2.2.1-fix676372.
105python-openid = 2.2.1-fix676372
106python-subunit = 0.0.8beta104python-subunit = 0.0.8beta
107pytz = 2012c105pytz = 2012c
108rdflib = 3.1.0106rdflib = 3.1.0