launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #22568
[Merge] lp:~cjwatson/launchpad/login-interstitial into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/login-interstitial into lp:launchpad.
Commit message:
Add interstitial pages when creating or reactivating an account.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/login-interstitial/+merge/346908
These provide an opportunity to present the user with the terms of service and privacy policy and require that they explicitly accept them, as well as making it harder to reactivate an account by accident.
To support testing this locally, I extended make-lp-user to be able to create placeholder accounts, and adjusted testopenid so that it can authenticate as an inactive account by explicitly supplying the username.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/login-interstitial into lp:launchpad.
=== modified file 'lib/lp/app/browser/configure.zcml'
--- lib/lp/app/browser/configure.zcml 2017-09-01 12:57:34 +0000
+++ lib/lp/app/browser/configure.zcml 2018-05-26 07:32:51 +0000
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2015 Canonical Ltd. This software is licensed under the
+<!-- Copyright 2009-2018 Canonical Ltd. This software is licensed under the
GNU Affero General Public License version 3 (see the file LICENSE).
-->
@@ -250,6 +250,18 @@
permission="zope.Public"
name="+openid-callback"
/>
+ <browser:page
+ for="lp.services.webapp.interfaces.ILaunchpadApplication"
+ class="lp.services.webapp.login.NewAccountView"
+ permission="zope.Public"
+ name="+new-account"
+ />
+ <browser:page
+ for="lp.services.webapp.interfaces.ILaunchpadApplication"
+ class="lp.services.webapp.login.ReactivateAccountView"
+ permission="zope.Public"
+ name="+reactivate-account"
+ />
<browser:page
for="*"
=== modified file 'lib/lp/app/browser/launchpad.py'
--- lib/lp/app/browser/launchpad.py 2016-06-22 21:04:30 +0000
+++ lib/lp/app/browser/launchpad.py 2018-05-26 07:32:51 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Browser code for the launchpad application."""
@@ -620,7 +620,9 @@
@property
def login_shown(self):
return (self.user is None and
- '+login' not in self.request['PATH_INFO'])
+ '+login' not in self.request['PATH_INFO'] and
+ '+new-account' not in self.request['PATH_INFO'] and
+ '+reactivate-account' not in self.request['PATH_INFO'])
@property
def logged_in(self):
=== modified file 'lib/lp/services/webapp/login.py'
--- lib/lp/services/webapp/login.py 2017-01-14 15:16:36 +0000
+++ lib/lp/services/webapp/login.py 2018-05-26 07:32:51 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Stuff to do with logging in and logging out."""
@@ -44,16 +44,27 @@
)
from lp import _
+from lp.app.browser.launchpadform import (
+ action,
+ LaunchpadFormView,
+ )
from lp.registry.interfaces.person import (
+ IPerson,
IPersonSet,
PersonCreationRationale,
TeamEmailAddressError,
)
from lp.services.config import config
+from lp.services.database.interfaces import IStore
from lp.services.database.policy import MasterDatabasePolicy
-from lp.services.identity.interfaces.account import AccountSuspendedError
+from lp.services.identity.interfaces.account import (
+ AccountStatus,
+ AccountSuspendedError,
+ )
+from lp.services.identity.interfaces.emailaddress import IEmailAddressSet
from lp.services.openid.extensions import macaroon
from lp.services.openid.interfaces.openidconsumer import IOpenIDConsumerStore
+from lp.services.openid.model.openididentifier import OpenIdIdentifier
from lp.services.propertycache import cachedproperty
from lp.services.timeline.requesttimeline import get_request_timeline
from lp.services.webapp import canonical_url
@@ -275,12 +286,8 @@
[encode_utf8(value) for value in value_list])
-class OpenIDCallbackView(OpenIDLogin):
- """The OpenID callback page for logging into Launchpad.
-
- This is the page the OpenID provider will send the user's browser to,
- after the user has authenticated on the provider.
- """
+class FinishLoginMixin:
+ """A mixin for views that can finish the login process."""
suspended_account_template = ViewPageTemplateFile(
'templates/login-suspended-account.pt')
@@ -288,9 +295,6 @@
team_email_address_template = ViewPageTemplateFile(
'templates/login-team-email-address.pt')
- discharge_macaroon_template = ViewPageTemplateFile(
- 'templates/login-discharge-macaroon.pt')
-
def _gather_params(self, request):
params = dict(request.form)
for key, value in request.query_string_params.iteritems():
@@ -301,6 +305,31 @@
return params
+ def login(self, person, when=None):
+ loginsource = getUtility(IPlacelessLoginSource)
+ # We don't have a logged in principal, so we must remove the security
+ # proxy of the account's preferred email.
+ email = removeSecurityProxy(person.preferredemail).email
+ logInPrincipal(
+ self.request, loginsource.getPrincipalByLogin(email), email, when)
+
+ def _redirect(self):
+ target = self.params.get('starting_url')
+ if target is None:
+ target = self.request.getApplicationURL()
+ self.request.response.redirect(target, temporary_if_possible=True)
+
+
+class OpenIDCallbackView(FinishLoginMixin, OpenIDLogin):
+ """The OpenID callback page for logging into Launchpad.
+
+ This is the page the OpenID provider will send the user's browser to,
+ after the user has authenticated on the provider.
+ """
+
+ discharge_macaroon_template = ViewPageTemplateFile(
+ 'templates/login-discharge-macaroon.pt')
+
def _get_requested_url(self, request):
requested_url = request.getURL()
query_string = request.get('QUERY_STRING')
@@ -321,13 +350,28 @@
timeline_action.finish()
self.discharge_macaroon_raw = None
- def login(self, person, when=None):
- loginsource = getUtility(IPlacelessLoginSource)
- # We don't have a logged in principal, so we must remove the security
- # proxy of the account's preferred email.
- email = removeSecurityProxy(person.preferredemail).email
- logInPrincipal(
- self.request, loginsource.getPrincipalByLogin(email), email, when)
+ def loginInactive(self, when=None):
+ """Log an inactive person in.
+
+ This isn't a normal login, which we can't do while the person is
+ inactive. Instead, we store a few details about the OpenID response
+ in a separate part of the session database, which lets us render an
+ appropriate interstitial page and then activate the account properly
+ after the form on that page is submitted.
+ """
+ # Force a fresh session, per bug #828638.
+ client_id_manager = getUtility(IClientIdManager)
+ new_client_id = client_id_manager.generateUniqueId()
+ client_id_manager.setRequestId(self.request, new_client_id)
+ session = ISession(self.request)
+ authdata = session['launchpad.pendinguser']
+ authdata['identifier'] = self._getOpenIDIdentifier()
+ email_address, full_name = self._getEmailAddressAndFullName()
+ authdata['email'] = email_address
+ authdata['fullname'] = full_name
+ if when is None:
+ when = datetime.utcnow()
+ authdata['logintime'] = when
@cachedproperty
def sreg_response(self):
@@ -338,6 +382,10 @@
return macaroon.MacaroonResponse.fromSuccessResponse(
self.openid_response)
+ def _getOpenIDIdentifier(self):
+ identifier = self.openid_response.identity_url.split('/')[-1]
+ return identifier.decode('ascii')
+
def _getEmailAddressAndFullName(self):
# Here we assume the OP sent us the user's email address and
# full name in the response. Note we can only do that because
@@ -356,6 +404,49 @@
"No email address or full name found in sreg response.")
return email_address, full_name
+ def _maybeRedirectToInterstitial(self, openid_identifier, email_address):
+ """Redirect to an interstitial page in some cases.
+
+ If there is no existing account for this OpenID identifier or email
+ address, or if the existing account is in certain inactive states,
+ then instead of logging in straight away we redirect to an
+ interstitial page to confirm what the user wants to do.
+ """
+ redirect_view_names = {
+ AccountStatus.DEACTIVATED: '+reactivate-account',
+ AccountStatus.NOACCOUNT: '+new-account',
+ AccountStatus.PLACEHOLDER: '+new-account',
+ }
+ identifier = IStore(OpenIdIdentifier).find(
+ OpenIdIdentifier, identifier=openid_identifier).one()
+ if identifier is not None:
+ person = IPerson(identifier.account, None)
+ else:
+ email = getUtility(IEmailAddressSet).getByEmail(email_address)
+ person = email.person if email is not None else None
+
+ if (person is None or
+ (not person.is_team and
+ (not person.account or
+ person.account.status in redirect_view_names))):
+ self.loginInactive()
+ trust_root = allvhosts.configs['mainsite'].rooturl
+ url = urlappend(
+ trust_root,
+ redirect_view_names[
+ person.account.status if person
+ else AccountStatus.NOACCOUNT])
+ params = {}
+ target = self.params.get('starting_url')
+ if target is not None:
+ params['starting_url'] = target
+ if params:
+ url += "?%s" % urllib.urlencode(params)
+ self.request.response.redirect(url, temporary_if_possible=True)
+ return True
+ else:
+ return False
+
def processPositiveAssertion(self):
"""Process an OpenID response containing a positive assertion.
@@ -369,25 +460,9 @@
DB writes, to ensure subsequent requests use the master DB and see
the changes we just did.
"""
- identifier = self.openid_response.identity_url.split('/')[-1]
- identifier = identifier.decode('ascii')
+ identifier = self._getOpenIDIdentifier()
+ email_address, full_name = self._getEmailAddressAndFullName()
should_update_last_write = False
- # Force the use of the master database to make sure a lagged slave
- # doesn't fool us into creating a Person/Account when one already
- # exists.
- person_set = getUtility(IPersonSet)
- email_address, full_name = self._getEmailAddressAndFullName()
- try:
- person, db_updated = person_set.getOrCreateByOpenIDIdentifier(
- identifier, email_address, full_name,
- comment='when logging in to Launchpad.',
- creation_rationale=(
- PersonCreationRationale.OWNER_CREATED_LAUNCHPAD))
- should_update_last_write = db_updated
- except AccountSuspendedError:
- return self.suspended_account_template()
- except TeamEmailAddressError:
- return self.team_email_address_template()
if self.params.get('discharge_macaroon_field'):
if self.macaroon_response.discharge_macaroon_raw is None:
@@ -396,7 +471,30 @@
self.discharge_macaroon_raw = (
self.macaroon_response.discharge_macaroon_raw)
+ # Force the use of the master database to make sure a lagged slave
+ # doesn't fool us into creating a Person/Account when one already
+ # exists.
with MasterDatabasePolicy():
+ if self._maybeRedirectToInterstitial(identifier, email_address):
+ return None
+
+ # XXX cjwatson 2018-05-25: We should never create a Person at
+ # this point; any situation that would result in that should
+ # result in a redirection to an interstitial page. Guaranteeing
+ # that will require a bit more refactoring, though.
+ person_set = getUtility(IPersonSet)
+ try:
+ person, db_updated = person_set.getOrCreateByOpenIDIdentifier(
+ identifier, email_address, full_name,
+ comment='when logging in to Launchpad.',
+ creation_rationale=(
+ PersonCreationRationale.OWNER_CREATED_LAUNCHPAD))
+ should_update_last_write = db_updated
+ except AccountSuspendedError:
+ return self.suspended_account_template()
+ except TeamEmailAddressError:
+ return self.team_email_address_template()
+
self.login(person)
if self.params.get('discharge_macaroon_field'):
@@ -443,12 +541,6 @@
transaction.commit()
return retval
- def _redirect(self):
- target = self.params.get('starting_url')
- if target is None:
- target = self.request.getApplicationURL()
- self.request.response.redirect(target, temporary_if_possible=True)
-
class OpenIDLoginErrorView(LaunchpadView):
@@ -471,6 +563,81 @@
self.login_error = "Unknown error: %s" % openid_response
+class FinishLoginInterstitialView(FinishLoginMixin, LaunchpadFormView):
+
+ class schema(Interface):
+ pass
+
+ def initialize(self):
+ self.params = self._gather_params(self.request)
+ super(FinishLoginInterstitialView, self).initialize()
+
+ def _accept(self):
+ session = ISession(self.request)
+ authdata = session['launchpad.pendinguser']
+ try:
+ identifier = authdata['identifier']
+ email_address = authdata['email']
+ full_name = authdata['fullname']
+ except KeyError:
+ return OpenIDLoginErrorView(
+ self.context, self.request,
+ login_error=(
+ "Your session expired. Please try logging in again."))
+ should_update_last_write = False
+
+ # Force the use of the master database to make sure a lagged slave
+ # doesn't fool us into creating a Person/Account when one already
+ # exists.
+ with MasterDatabasePolicy():
+ person_set = getUtility(IPersonSet)
+ try:
+ person, db_updated = person_set.getOrCreateByOpenIDIdentifier(
+ identifier, email_address, full_name,
+ comment=(
+ 'when logging in to Launchpad, after accepting '
+ 'terms.'),
+ creation_rationale=(
+ PersonCreationRationale.OWNER_CREATED_LAUNCHPAD))
+ should_update_last_write = db_updated
+ except AccountSuspendedError:
+ return self.suspended_account_template()
+ except TeamEmailAddressError:
+ return self.team_email_address_template()
+
+ self.login(person)
+
+ if should_update_last_write:
+ # This is a GET request but we changed the database, so update
+ # session_data['last_write'] to make sure further requests use
+ # the master DB and thus see the changes we've just made.
+ session_data = ISession(self.request)['lp.dbpolicy']
+ session_data['last_write'] = datetime.utcnow()
+ self._redirect()
+ # No need to return anything as we redirect above.
+ return None
+
+
+class NewAccountView(FinishLoginInterstitialView):
+
+ page_title = label = 'Welcome to Launchpad!'
+ template = ViewPageTemplateFile('templates/login-new-account.pt')
+
+ @action('Accept terms and create account', name='accept')
+ def accept(self, action, data):
+ return self._accept()
+
+
+class ReactivateAccountView(FinishLoginInterstitialView):
+
+ page_title = label = 'Welcome back to Launchpad!'
+ template = ViewPageTemplateFile('templates/login-reactivate-account.pt')
+
+ @action('Accept terms and reactivate account', name='accept')
+ def accept(self, action, data):
+ return self._accept()
+
+
class AlreadyLoggedInView(LaunchpadView):
page_title = 'Already logged in'
=== added file 'lib/lp/services/webapp/templates/login-new-account.pt'
--- lib/lp/services/webapp/templates/login-new-account.pt 1970-01-01 00:00:00 +0000
+++ lib/lp/services/webapp/templates/login-new-account.pt 2018-05-26 07:32:51 +0000
@@ -0,0 +1,36 @@
+<html
+ xmlns="http://www.w3.org/1999/xhtml";
+ xmlns:tal="http://xml.zope.org/namespaces/tal";
+ xmlns:metal="http://xml.zope.org/namespaces/metal";
+ xmlns:i18n="http://xml.zope.org/namespaces/i18n";
+ metal:use-macro="view/macro:page/main_only"
+ i18n:domain="launchpad">
+
+ <body>
+ <div class="top-portlet" metal:fill-slot="main">
+ <div metal:use-macro="context/@@launchpad_form/form">
+ <div metal:fill-slot="extra_info">
+ <p>
+ Please read and accept the
+ <a href="/legal">Launchpad terms of service</a> and the
+ <a href="https://www.ubuntu.com/legal/dataprivacy";>data privacy
+ policy</a> before continuing. If you accept these terms,
+ Launchpad will create an account for you.
+ </p>
+
+ <p>
+ You may also
+ <a tal:attributes="href view/params/starting_url|string:/">return
+ to Launchpad without creating an account</a>.
+ </p>
+
+ <input
+ type="hidden"
+ name="starting_url"
+ tal:condition="view/params/starting_url|nothing"
+ tal:attributes="value view/params/starting_url" />
+ </div>
+ </div>
+ </div>
+ </body>
+</html>
=== added file 'lib/lp/services/webapp/templates/login-reactivate-account.pt'
--- lib/lp/services/webapp/templates/login-reactivate-account.pt 1970-01-01 00:00:00 +0000
+++ lib/lp/services/webapp/templates/login-reactivate-account.pt 2018-05-26 07:32:51 +0000
@@ -0,0 +1,43 @@
+<html
+ xmlns="http://www.w3.org/1999/xhtml";
+ xmlns:tal="http://xml.zope.org/namespaces/tal";
+ xmlns:metal="http://xml.zope.org/namespaces/metal";
+ xmlns:i18n="http://xml.zope.org/namespaces/i18n";
+ metal:use-macro="view/macro:page/main_only"
+ i18n:domain="launchpad">
+
+ <body>
+ <div class="top-portlet" metal:fill-slot="main">
+ <div metal:use-macro="context/@@launchpad_form/form">
+ <div metal:fill-slot="extra_info">
+ <p>
+ Please read and accept the
+ <a href="/legal">Launchpad terms of service</a> and the
+ <a href="https://www.ubuntu.com/legal/dataprivacy";>data privacy
+ policy</a> before continuing. If you accept these terms,
+ Launchpad will reactivate your account.
+ </p>
+
+ <p>
+ Reactivating your account will not restore any information that
+ was deleted when you deactivated it. You may start receiving
+ email notifications again related to information that was not
+ deleted, such as changes to any bugs you reported.
+ </p>
+
+ <p>
+ You may also
+ <a tal:attributes="href view/params/starting_url|string:/">return
+ to Launchpad without reactivating your account</a>.
+ </p>
+
+ <input
+ type="hidden"
+ name="starting_url"
+ tal:condition="view/params/starting_url|nothing"
+ tal:attributes="value view/params/starting_url" />
+ </div>
+ </div>
+ </div>
+ </body>
+</html>
=== modified file 'lib/lp/services/webapp/tests/test_login.py'
--- lib/lp/services/webapp/tests/test_login.py 2018-01-02 16:10:26 +0000
+++ lib/lp/services/webapp/tests/test_login.py 2018-05-26 07:32:51 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Test harness for running the new-login.txt tests."""
@@ -64,8 +64,10 @@
from lp.services.timeline.requesttimeline import get_request_timeline
from lp.services.webapp.interfaces import ILaunchpadApplication
from lp.services.webapp.login import (
+ NewAccountView,
OpenIDCallbackView,
OpenIDLogin,
+ ReactivateAccountView,
)
from lp.services.webapp.servers import LaunchpadTestRequest
from lp.testing import (
@@ -106,11 +108,11 @@
self.discharge_macaroon_raw = discharge_macaroon_raw
-class StubbedOpenIDCallbackView(OpenIDCallbackView):
+class StubLoginMixin:
login_called = False
def login(self, account):
- super(StubbedOpenIDCallbackView, self).login(account)
+ super(StubLoginMixin, self).login(account)
self.login_called = True
current_policy = getUtility(IStoreSelector).get_current()
if not isinstance(current_policy, MasterDatabasePolicy):
@@ -118,6 +120,18 @@
"Not using the master store: %s" % current_policy)
+class StubbedOpenIDCallbackView(StubLoginMixin, OpenIDCallbackView):
+ pass
+
+
+class StubbedNewAccountView(StubLoginMixin, NewAccountView):
+ pass
+
+
+class StubbedReactivateAccountView(StubLoginMixin, ReactivateAccountView):
+ pass
+
+
class FakeConsumer:
"""An OpenID consumer that stashes away arguments for test inspection."""
@@ -212,24 +226,27 @@
openid_response, view_class=view_class)
def _createAndRenderView(self, response,
- view_class=StubbedOpenIDCallbackView, form=None):
+ view_class=StubbedOpenIDCallbackView, form=None,
+ method='GET', **kwargs):
if form is None:
form = {'starting_url': 'http://launchpad.dev/after-login'}
- request = LaunchpadTestRequest(form=form, environ={'PATH_INFO': '/'})
+ request = LaunchpadTestRequest(
+ form=form, environ={'PATH_INFO': '/'}, method=method, **kwargs)
# The layer we use sets up an interaction (by calling login()), but we
# want to use our own request in the interaction, so we logout() and
# setup a newInteraction() using our request.
logout()
newInteraction(request)
view = view_class(object(), request)
- view.initialize()
- view.openid_response = response
# Monkey-patch getByOpenIDIdentifier() to make sure the view uses the
# master DB. This mimics the problem we're trying to avoid, where
# getByOpenIDIdentifier() doesn't find a newly created account because
# it looks in the slave database.
with IAccountSet_getByOpenIDIdentifier_monkey_patched():
- html = view.render()
+ view.initialize()
+ if response is not None:
+ view.openid_response = response
+ html = view.render() if method == 'GET' else None
return view, html
def test_full_fledged_account(self):
@@ -328,9 +345,7 @@
def test_unseen_identity(self):
# When we get a positive assertion about an identity URL we've never
- # seen, we automatically register an account with that identity
- # because someone who registered on login.lp.net or login.u.c should
- # be able to login here without any further steps.
+ # seen, we redirect to a confirmation page.
identifier = u'4w7kmzU'
account_set = getUtility(IAccountSet)
self.assertRaises(
@@ -340,15 +355,47 @@
email='non-existent@xxxxxxxxxxx', full_name='Foo User')
with SRegResponse_fromSuccessResponse_stubbed():
view, html = self._createAndRenderView(openid_response)
+ self.assertFalse(view.login_called)
+ response = view.request.response
+ self.assertEqual(httplib.TEMPORARY_REDIRECT, response.getStatus())
+ self.assertEqual(
+ 'http://launchpad.dev/+new-account?' + urllib.urlencode(
+ {'starting_url': view.request.form['starting_url']}),
+ response.getHeader('Location'))
+ self.assertRaises(
+ LookupError, account_set.getByOpenIDIdentifier, identifier)
+ self.assertThat(
+ ISession(view.request)['launchpad.pendinguser'],
+ ContainsDict({
+ 'identifier': Equals(identifier),
+ 'email': Equals('non-existent@xxxxxxxxxxx'),
+ 'fullname': Equals('Foo User'),
+ }))
+
+ # If the user accepts, we automatically register an account with
+ # that identity, since its existence on SSO is good enough for us.
+ cookie = response.getCookie('launchpad_tests')['value']
+ view, _ = self._createAndRenderView(
+ None, view_class=StubbedNewAccountView,
+ form={
+ 'starting_url': view.request.form['starting_url'],
+ 'field.actions.accept': 'Accept terms and create account',
+ },
+ method='POST', HTTP_COOKIE='launchpad_tests=%s' % cookie)
self.assertTrue(view.login_called)
+ response = view.request.response
+ self.assertEqual(httplib.SEE_OTHER, response.getStatus())
+ self.assertEqual(
+ view.request.form['starting_url'], response.getHeader('Location'))
account = account_set.getByOpenIDIdentifier(identifier)
- self.assertIsNot(None, account)
+ self.assertIsNotNone(account)
self.assertEqual(AccountStatus.ACTIVE, account.status)
person = IPerson(account, None)
- self.assertIsNot(None, person)
+ self.assertIsNotNone(person)
self.assertEqual('Foo User', person.displayname)
- self.assertEqual('non-existent@xxxxxxxxxxx',
- removeSecurityProxy(person.preferredemail).email)
+ self.assertEqual(
+ 'non-existent@xxxxxxxxxxx',
+ removeSecurityProxy(person.preferredemail).email)
# We also update the last_write flag in the session, to make sure
# further requests use the master DB and thus see the newly created
@@ -363,8 +410,7 @@
email = 'test@xxxxxxxxxxx'
person = self.factory.makePerson(
displayname='Test account', email=email,
- account_status=AccountStatus.DEACTIVATED,
- email_address_status=EmailAddressStatus.NEW)
+ account_status=AccountStatus.NOACCOUNT)
account = person.account
account_set = getUtility(IAccountSet)
self.assertRaises(
@@ -374,7 +420,38 @@
email=email, full_name='Foo User')
with SRegResponse_fromSuccessResponse_stubbed():
view, html = self._createAndRenderView(openid_response)
+ self.assertFalse(view.login_called)
+ response = view.request.response
+ self.assertEqual(httplib.TEMPORARY_REDIRECT, response.getStatus())
+ self.assertEqual(
+ 'http://launchpad.dev/+new-account?' + urllib.urlencode(
+ {'starting_url': view.request.form['starting_url']}),
+ response.getHeader('Location'))
+ self.assertRaises(
+ LookupError, account_set.getByOpenIDIdentifier, identifier)
+ self.assertEqual(AccountStatus.NOACCOUNT, account.status)
+ self.assertThat(
+ ISession(view.request)['launchpad.pendinguser'],
+ ContainsDict({
+ 'identifier': Equals(identifier),
+ 'email': Equals('test@xxxxxxxxxxx'),
+ 'fullname': Equals('Foo User'),
+ }))
+
+ # Accept the terms and proceed.
+ cookie = response.getCookie('launchpad_tests')['value']
+ view, _ = self._createAndRenderView(
+ None, view_class=StubbedNewAccountView,
+ form={
+ 'starting_url': view.request.form['starting_url'],
+ 'field.actions.accept': 'Accept terms and create account',
+ },
+ method='POST', HTTP_COOKIE='launchpad_tests=%s' % cookie)
self.assertTrue(view.login_called)
+ response = view.request.response
+ self.assertEqual(httplib.SEE_OTHER, response.getStatus())
+ self.assertEqual(
+ view.request.form['starting_url'], response.getHeader('Location'))
# The existing accounts had a new openid_identifier added, the
# account was reactivated and its preferred email was set, but
@@ -386,7 +463,7 @@
self.assertEqual(
email, removeSecurityProxy(person.preferredemail).email)
person = IPerson(account, None)
- self.assertIsNot(None, person)
+ self.assertIsNotNone(person)
self.assertEqual('Test account', person.displayname)
# We also update the last_write flag in the session, to make sure
@@ -396,7 +473,7 @@
def test_deactivated_account(self):
# The user has the account's password and is trying to login, so we'll
- # just re-activate their account.
+ # redirect them to a confirmation page.
email = 'foo@xxxxxxxxxxx'
person = self.factory.makePerson(
displayname='Test account', email=email,
@@ -409,11 +486,37 @@
status=SUCCESS, email=email, full_name=person.displayname)
with SRegResponse_fromSuccessResponse_stubbed():
view, html = self._createAndRenderView(openid_response)
+ self.assertFalse(view.login_called)
+ response = view.request.response
+ self.assertEqual(httplib.TEMPORARY_REDIRECT, response.getStatus())
+ self.assertEqual(
+ 'http://launchpad.dev/+reactivate-account?' + urllib.urlencode(
+ {'starting_url': view.request.form['starting_url']}),
+ response.getHeader('Location'))
+ self.assertEqual(AccountStatus.DEACTIVATED, person.account.status)
+ self.assertIsNone(person.preferredemail)
+ self.assertThat(
+ ISession(view.request)['launchpad.pendinguser'],
+ ContainsDict({
+ 'identifier': Equals(openid_identifier),
+ 'email': Equals(email),
+ 'fullname': Equals('Test account'),
+ }))
+
+ # If the user confirms the reactivation, we do it.
+ cookie = response.getCookie('launchpad_tests')['value']
+ view, _ = self._createAndRenderView(
+ None, view_class=StubbedReactivateAccountView,
+ form={
+ 'starting_url': view.request.form['starting_url'],
+ 'field.actions.accept': 'Accept terms and reactivate account',
+ },
+ method='POST', HTTP_COOKIE='launchpad_tests=%s' % cookie)
self.assertTrue(view.login_called)
response = view.request.response
- self.assertEqual(httplib.TEMPORARY_REDIRECT, response.getStatus())
- self.assertEqual(view.request.form['starting_url'],
- response.getHeader('Location'))
+ self.assertEqual(httplib.SEE_OTHER, response.getStatus())
+ self.assertEqual(
+ view.request.form['starting_url'], response.getHeader('Location'))
self.assertEqual(AccountStatus.ACTIVE, person.account.status)
self.assertEqual(email, person.preferredemail.email)
# We also update the last_write flag in the session, to make sure
@@ -423,12 +526,11 @@
def test_never_used_account(self):
# The account was created by one of our scripts but was never
- # activated, so we just activate it.
+ # activated, so we redirect to a confirmation page.
email = 'foo@xxxxxxxxxxx'
person = self.factory.makePerson(
displayname='Test account', email=email,
- account_status=AccountStatus.DEACTIVATED,
- email_address_status=EmailAddressStatus.NEW)
+ account_status=AccountStatus.NOACCOUNT)
openid_identifier = IStore(OpenIdIdentifier).find(
OpenIdIdentifier.identifier,
OpenIdIdentifier.account_id == person.account.id).order_by(
@@ -439,6 +541,32 @@
status=SUCCESS, email=email, full_name=person.displayname)
with SRegResponse_fromSuccessResponse_stubbed():
view, html = self._createAndRenderView(openid_response)
+ self.assertFalse(view.login_called)
+ response = view.request.response
+ self.assertEqual(httplib.TEMPORARY_REDIRECT, response.getStatus())
+ self.assertEqual(
+ 'http://launchpad.dev/+new-account?' + urllib.urlencode(
+ {'starting_url': view.request.form['starting_url']}),
+ response.getHeader('Location'))
+ self.assertEqual(AccountStatus.NOACCOUNT, person.account.status)
+ self.assertIsNone(person.preferredemail)
+ self.assertThat(
+ ISession(view.request)['launchpad.pendinguser'],
+ ContainsDict({
+ 'identifier': Equals(openid_identifier),
+ 'email': Equals(email),
+ 'fullname': Equals('Test account'),
+ }))
+
+ # If the user confirms the activation, we do it.
+ cookie = response.getCookie('launchpad_tests')['value']
+ view, _ = self._createAndRenderView(
+ None, view_class=StubbedNewAccountView,
+ form={
+ 'starting_url': view.request.form['starting_url'],
+ 'field.actions.accept': 'Accept terms and create account',
+ },
+ method='POST', HTTP_COOKIE='launchpad_tests=%s' % cookie)
self.assertTrue(view.login_called)
self.assertEqual(AccountStatus.ACTIVE, person.account.status)
self.assertEqual(email, person.preferredemail.email)
=== modified file 'lib/lp/testopenid/browser/server.py'
--- lib/lp/testopenid/browser/server.py 2015-07-08 16:05:11 +0000
+++ lib/lp/testopenid/browser/server.py 2018-05-26 07:32:51 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2011 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Test OpenID server."""
@@ -30,7 +30,10 @@
from zope.authentication.interfaces import IUnauthenticatedPrincipal
from zope.component import getUtility
from zope.interface import implementer
-from zope.security.proxy import isinstance as zisinstance
+from zope.security.proxy import (
+ isinstance as zisinstance,
+ removeSecurityProxy,
+ )
from zope.session.interfaces import ISession
from lp import _
@@ -39,11 +42,11 @@
LaunchpadFormView,
)
from lp.app.errors import UnexpectedFormData
-from lp.registry.interfaces.person import IPerson
-from lp.services.identity.interfaces.account import (
- AccountStatus,
- IAccountSet,
+from lp.registry.interfaces.person import (
+ IPerson,
+ IPersonSet,
)
+from lp.services.identity.interfaces.account import IAccountSet
from lp.services.openid.browser.openiddiscovery import (
XRDSContentNegotiationMixin,
)
@@ -52,13 +55,9 @@
get_property_cache,
)
from lp.services.webapp import LaunchpadView
-from lp.services.webapp.interfaces import (
- ICanonicalUrlData,
- IPlacelessLoginSource,
- )
+from lp.services.webapp.interfaces import ICanonicalUrlData
from lp.services.webapp.login import (
allowUnauthenticatedSession,
- logInPrincipal,
logoutPerson,
)
from lp.services.webapp.publisher import (
@@ -105,7 +104,7 @@
account = getUtility(IAccountSet).getByOpenIDIdentifier(name)
except LookupError:
account = None
- if account is None or account.status != AccountStatus.ACTIVE:
+ if account is None:
return None
return ITestOpenIDPersistentIdentity(account)
@@ -206,7 +205,7 @@
response.setHeader(header, value)
return webresponse.body
- def createPositiveResponse(self):
+ def createPositiveResponse(self, email):
"""Create a positive assertion OpenIDResponse.
This method should be called to create the response to
@@ -233,7 +232,7 @@
person = IPerson(self.account)
sreg_fields = dict(
nickname=person.name,
- email=person.preferredemail.email,
+ email=email,
fullname=self.account.displayname)
sreg_request = SRegRequest.fromOpenIDRequest(self.openid_request)
sreg_response = SRegResponse.extractResponse(
@@ -306,21 +305,34 @@
def validate(self, data):
"""Check that the email address is valid for login."""
- loginsource = getUtility(IPlacelessLoginSource)
- principal = loginsource.getPrincipalByLogin(data['email'])
- if principal is None:
- self.addError(
- _("Unknown email address."))
+ if data.get('username'):
+ person = getUtility(IPersonSet).getByName(data['username'])
+ if person is None:
+ self.setFieldError('username', _("Unknown username."))
+ elif person.preferredemail is not None:
+ email = removeSecurityProxy(person.preferredemail).email
+ if email != data['email']:
+ self.setFieldError(
+ 'email',
+ _("Email address for user '%s' is '%s', not '%s'.") %
+ (data['username'], email, data['email']))
+ elif getUtility(IPersonSet).getByEmail(data['email']) is None:
+ self.setFieldError('email', _("Unknown email address."))
@action('Continue', name='continue')
def continue_action(self, action, data):
email = data['email']
- principal = getUtility(IPlacelessLoginSource).getPrincipalByLogin(
- email)
- logInPrincipal(self.request, principal, email)
- # Update the attribute holding the cached user.
- self._account = principal.account
- return self.renderOpenIDResponse(self.createPositiveResponse())
+ username = data.get('username')
+ if username is not None:
+ person = getUtility(IPersonSet).getByName(username)
+ else:
+ person = getUtility(IPersonSet).getByEmail(email)
+ # Update the attribute holding the cached user. This fakes a login;
+ # we don't do a true login here, because we can get away without it
+ # and it allows testing the case of logging in as a user whose
+ # account status is not ACTIVE.
+ self._account = person.account
+ return self.renderOpenIDResponse(self.createPositiveResponse(email))
class PersistentIdentityView(
=== modified file 'lib/lp/testopenid/interfaces/server.py'
--- lib/lp/testopenid/interfaces/server.py 2015-07-21 09:04:01 +0000
+++ lib/lp/testopenid/interfaces/server.py 2018-05-26 07:32:51 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -23,7 +23,12 @@
class ITestOpenIDLoginForm(Interface):
- email = TextLine(title=u'What is your email address?', required=True)
+ email = TextLine(title=u'Your email address', required=True)
+ username = TextLine(
+ title=u'Your username', required=False,
+ description=(
+ u'This is only required if you are logging into a placeholder '
+ u'account for the first time.'))
class ITestOpenIDPersistentIdentity(IOpenIDPersistentIdentity):
=== modified file 'lib/lp/testopenid/stories/logging-in.txt'
--- lib/lp/testopenid/stories/logging-in.txt 2012-01-14 12:47:20 +0000
+++ lib/lp/testopenid/stories/logging-in.txt 2018-05-26 07:32:51 +0000
@@ -42,6 +42,7 @@
>>> for tag in find_tags_by_class(browser.contents, 'error'):
... print extract_text(tag)
There is 1 error.
+ Your email address:
Unknown email address.
If the email address matches an account, the user is logged in and
=== modified file 'utilities/make-lp-user'
--- utilities/make-lp-user 2015-05-07 09:29:30 +0000
+++ utilities/make-lp-user 2018-05-26 07:32:51 +0000
@@ -1,6 +1,6 @@
#!/usr/bin/python -S
#
-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Create a user for testing the local Launchpad.
@@ -29,6 +29,8 @@
production environments.
"""
+from __future__ import absolute_import, print_function
+
import _pythonpath
from optparse import OptionParser
@@ -49,6 +51,7 @@
GPGKeyAlgorithm,
IGPGHandler,
)
+from lp.services.identity.interfaces.account import AccountStatus
from lp.services.scripts import execute_zcml_for_scripts
from lp.services.timeout import set_default_timeout_function
from lp.testing.factory import LaunchpadObjectFactory
@@ -58,14 +61,23 @@
set_default_timeout_function(lambda: 100)
+
def make_person(username, email):
"""Create and return a person with the given username.
The email address for the user will be <username>@example.com.
"""
person = factory.makePerson(name=username, email=email)
- print "username: %s" % (username,)
- print "email: %s" % (email,)
+ print("username: %s" % (username,))
+ print("email: %s" % (email,))
+ return person
+
+
+def make_placeholder_person(username):
+ """Create and return a placeholder person with the given username."""
+ person = factory.makePerson(
+ name=username, account_status=AccountStatus.PLACEHOLDER)
+ print("username: %s" % (username,))
return person
@@ -82,15 +94,15 @@
for team_name in team_names:
team = person_set.getByName(team_name)
if team is None:
- print "ERROR: %s not found." % (team_name,)
+ print("ERROR: %s not found." % (team_name,))
continue
if not team.is_team:
- print "ERROR: %s is not a team." % (team_name,)
+ print("ERROR: %s is not a team." % (team_name,))
continue
team.addMember(
person, person, status=TeamMembershipStatus.APPROVED)
teams_joined.append(team_name)
- print "teams: %s" % ' '.join(teams_joined)
+ print("teams: %s" % ' '.join(teams_joined))
def add_ssh_public_keys(person):
@@ -111,10 +123,10 @@
except (OSError, IOError):
continue
key_set.new(person, public_key)
- print 'Registered SSH key: %s' % (filename,)
+ print('Registered SSH key: %s' % (filename,))
break
else:
- print 'No SSH key files found in %s' % ssh_dir
+ print('No SSH key files found in %s' % ssh_dir)
def parse_fingerprints(gpg_output):
@@ -143,7 +155,7 @@
command_line, env=env, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
stdout, stderr = pipe.communicate()
if stderr != '':
- print stderr
+ print(stderr)
if pipe.returncode != 0:
raise Exception('GPG error during "%s"' % ' '.join(command_line))
@@ -179,7 +191,7 @@
fingerprints = parse_fingerprints(output)
if len(fingerprints) == 0:
- print "No GPG key fingerprints found!"
+ print("No GPG key fingerprints found!")
for fingerprint in fingerprints:
add_gpg_key(person, fingerprint)
@@ -194,10 +206,13 @@
parser.add_option(
'-e', '--email', action='store', dest='email', default=None,
help="Email address; set to use real GPG key for this address.")
+ parser.add_option(
+ '--placeholder', action='store_true', default=False,
+ help="Create a placeholder account rather than a full user account.")
options, args = parser.parse_args(arguments)
if len(args) == 0:
- print __doc__
+ print(__doc__)
sys.exit(2)
options.username = args[0]
@@ -217,12 +232,15 @@
execute_zcml_for_scripts()
transaction.begin()
- person = make_person(options.username, email)
- add_person_to_teams(person, options.teams)
- add_ssh_public_keys(person)
+ if options.placeholder:
+ make_placeholder_person(options.username)
+ else:
+ person = make_person(options.username, email)
+ add_person_to_teams(person, options.teams)
+ add_ssh_public_keys(person)
- if options.email is not None:
- attach_gpg_keys(options.email, person)
+ if options.email is not None:
+ attach_gpg_keys(options.email, person)
transaction.commit()
Follow ups
