launchpad-reviewers team mailing list archive

[Deryck Hodge <deryck.hodge@xxxxxxxxxxxxx>]

Deryck Hodge has proposed merging lp:~deryck/launchpad/reauth-ssh-363916 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~deryck/launchpad/reauth-ssh-363916/+merge/120000

This change builds on my earlier work to require users to re-authenticate when editing emails.  In this branch, we now add a requirement to re-auth when editing ssh keys.  For more on the earlier work see bug #363916 and linked branches.

The heart of the work here is to re-factor the code used to call for a redirect in the edit emails view into a helper function called require_fresh_login.  Then this method is added to the edit ssh keys view.  Related doctests needed updating since they fail if the browser isn't using a fresh login.  This makes use of the page test helper I created earlier called setupBrowserFreshLogin.

Finally, I decided to add some browser tests for both editing email and ssk keys, just to ensure we always do the login redirect.  All the pieces involved were tested individually in earlier branches, but it dawned on me during this branch that the actual browser redirect wasn't checked.

To try this locally or QA the work:

* Login to launchpad
* Go get coffee (to let your login sit around for 5 minutes or so)
* Go to your person page
* Click the edit icon for editing ssh keys

You should be redirected to SSO and forced to re-authenticate.
-- 
https://code.launchpad.net/~deryck/launchpad/reauth-ssh-363916/+merge/120000
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~deryck/launchpad/reauth-ssh-363916 into lp:launchpad.

=== modified file 'lib/lp/code/stories/branches/xx-upload-directions.txt'
--- lib/lp/code/stories/branches/xx-upload-directions.txt	2011-05-31 22:28:58 +0000
+++ lib/lp/code/stories/branches/xx-upload-directions.txt	2012-08-16 18:58:19 +0000
@@ -9,7 +9,13 @@
 We will need multiple browsers, logged in with different users. Set them up
 now.
 
-    >>> name12_browser = setupBrowser(auth='Basic test@xxxxxxxxxxxxx:test')
+    >>> from zope.component import getUtility
+    >>> from lp.registry.interfaces.person import IPersonSet
+    >>> from lp.testing.pages import setupBrowserFreshLogin
+    >>> login(ANONYMOUS)
+    >>> name12 = getUtility(IPersonSet).getByEmail('test@xxxxxxxxxxxxx')
+    >>> logout()
+    >>> name12_browser = setupBrowserFreshLogin(name12)
     >>> ddaa_browser = setupBrowser(
     ...     auth='Basic david.allouche@xxxxxxxxxxxxx:test')
 

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2012-08-15 17:18:09 +0000
+++ lib/lp/registry/browser/person.py	2012-08-16 18:58:19 +0000
@@ -259,8 +259,8 @@
     IOpenLaunchBag,
     )
 from lp.services.webapp.login import (
-    isFreshLogin,
     logoutPerson,
+    require_fresh_login,
     )
 from lp.services.webapp.menu import get_current_view
 from lp.services.webapp.publisher import LaunchpadView
@@ -2413,6 +2413,8 @@
     error_message = None
 
     def initialize(self):
+        require_fresh_login(self.request, self.context, '+editsshkeys')
+
         if self.request.method != "POST":
             # Nothing to do
             return
@@ -2780,11 +2782,7 @@
     label = 'Change your e-mail settings'
 
     def initialize(self):
-        if not isFreshLogin(self.request):
-            reauth_query = '+login?reauth=1'
-            base_url = canonical_url(self.context, view_name='+editemails')
-            login_url = '%s/%s' % (base_url, reauth_query)
-            self.request.response.redirect(login_url)
+        require_fresh_login(self.request, self.context, '+editemails')
         if self.context.is_team:
             # +editemails is not available on teams.
             name = self.request['PATH_INFO'].split('/')[-1]

=== modified file 'lib/lp/registry/browser/tests/test_person.py'
--- lib/lp/registry/browser/tests/test_person.py	2012-08-16 13:05:53 +0000
+++ lib/lp/registry/browser/tests/test_person.py	2012-08-16 18:58:19 +0000
@@ -621,6 +621,15 @@
             " doesn't seem to be a valid email address.")
         self._assertEmailAndError(xss_email, expected_msg)
 
+    def test_edit_email_login_redirect(self):
+        """+editemails should redirect to force you to re-authenticate."""
+        view = create_initialized_view(self.person, "+editemails")
+        response = view.request.response
+        self.assertEqual(302, response.getStatus())
+        expected_url = (
+            '%s/+editemails/+login?reauth=1' % canonical_url(self.person))
+        self.assertEqual(expected_url, response.getHeader('location'))
+
 
 class PersonAdministerViewTestCase(TestPersonRenameFormMixin,
                                    TestCaseWithFactory):

=== modified file 'lib/lp/registry/browser/tests/test_sshkey.py'
--- lib/lp/registry/browser/tests/test_sshkey.py	2012-06-14 05:18:22 +0000
+++ lib/lp/registry/browser/tests/test_sshkey.py	2012-08-16 18:58:19 +0000
@@ -10,6 +10,7 @@
 from lp.registry.interfaces.ssh import ISSHKeySet
 from lp.services.webapp import canonical_url
 from lp.testing import (
+    login_person,
     person_logged_in,
     TestCaseWithFactory,
     )
@@ -18,6 +19,7 @@
     extract_text,
     find_tags_by_class,
     )
+from lp.testing.views import create_initialized_view
 
 
 class TestCanonicalUrl(TestCaseWithFactory):
@@ -55,3 +57,14 @@
             self.assertEqual(
                 extract_text(find_tags_by_class(browser.contents, 'message')[0]),
                 msg)
+
+    def test_edit_ssh_keys_login_redirect(self):
+        """+editsshkeys should redirect to force you to re-authenticate."""
+        person = self.factory.makePerson()
+        login_person(person)
+        view = create_initialized_view(person, "+editsshkeys")
+        response = view.request.response
+        self.assertEqual(302, response.getStatus())
+        expected_url = (
+            '%s/+editsshkeys/+login?reauth=1' % canonical_url(person))
+        self.assertEqual(expected_url, response.getHeader('location'))

=== modified file 'lib/lp/registry/stories/person/xx-add-sshkey.txt'
--- lib/lp/registry/stories/person/xx-add-sshkey.txt	2012-04-16 12:31:43 +0000
+++ lib/lp/registry/stories/person/xx-add-sshkey.txt	2012-08-16 18:58:19 +0000
@@ -28,8 +28,14 @@
 
 Salgado sees a message explaining that he can register his ssh key.
 
-    >>> browser = setupBrowser(
-    ...     auth='Basic guilherme.salgado@xxxxxxxxxxxxx:test')
+    >>> from zope.component import getUtility
+    >>> from lp.registry.interfaces.person import IPersonSet
+    >>> from lp.testing.pages import setupBrowserFreshLogin
+    >>> login(ANONYMOUS)
+    >>> salgado = getUtility(IPersonSet).getByEmail(
+    ...     'guilherme.salgado@xxxxxxxxxxxxx')
+    >>> logout()
+    >>> browser = setupBrowserFreshLogin(salgado)
     >>> browser.open('http://launchpad.dev/~salgado')
     >>> print browser.title
     Guilherme Salgado in Launchpad
@@ -97,6 +103,10 @@
 
 Launchpad administrators are not allowed to poke at other user's ssh keys.
 
+    >>> login(ANONYMOUS)
+    >>> foo_bar = getUtility(IPersonSet).getByEmail('foo.bar@xxxxxxxxxxxxx')
+    >>> logout()
+    >>> admin_browser = setupBrowserFreshLogin(foo_bar)
     >>> admin_browser.open('http://launchpad.dev/~salgado/+editsshkeys')
     Traceback (most recent call last):
     ...

=== modified file 'lib/lp/services/webapp/login.py'
--- lib/lp/services/webapp/login.py	2012-08-13 16:09:26 +0000
+++ lib/lp/services/webapp/login.py	2012-08-16 18:58:19 +0000
@@ -52,6 +52,7 @@
 from lp.services.openid.interfaces.openidconsumer import IOpenIDConsumerStore
 from lp.services.propertycache import cachedproperty
 from lp.services.timeline.requesttimeline import get_request_timeline
+from lp.services.webapp import canonical_url
 from lp.services.webapp.dbpolicy import MasterDatabasePolicy
 from lp.services.webapp.error import SystemErrorView
 from lp.services.webapp.interfaces import (
@@ -460,6 +461,15 @@
     return False
 
 
+def require_fresh_login(request, context, view_name):
+    """Redirect request to login if the request is not recently logged in."""
+    if not isFreshLogin(request):
+        reauth_query = '+login?reauth=1'
+        base_url = canonical_url(context, view_name=view_name)
+        login_url = '%s/%s' % (base_url, reauth_query)
+        request.response.redirect(login_url)
+
+
 def logInPrincipal(request, principal, email, when=None):
     """Log the principal in. Password validation must be done in callsites."""
     # Force a fresh session, per Bug #828638. Any changes to any