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 [email protected]: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('[email protected]') + >>> logout() + >>> name12_browser = setupBrowserFreshLogin(name12) >>> ddaa_browser = setupBrowser( ... auth='Basic [email protected]: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 [email protected]: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( + ... '[email protected]') + >>> 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('[email protected]') + >>> 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
_______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
