Merge ~cjwatson/launchpad:login-interstitial into launchpad:master

[Colin Watson]

Add interstitial pages when creating or reactivating an account

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.

This is essentially the same as https://code.launchpad.net/~cjwatson/launchpad/login-interstitial/+merge/346908, converted to git and rebased on master.

Unmerged commits

55f540c... by Colin Watson on 2018-05-25

Add interstitial pages when creating or reactivating an account.

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.

diff --git a/lib/lp/app/browser/configure.zcml b/lib/lp/app/browser/configure.zcml
index 6573fb2..b908bba 100644
--- a/lib/lp/app/browser/configure.zcml
+++ b/lib/lp/app/browser/configure.zcml
@@ -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="*"
diff --git a/lib/lp/app/browser/launchpad.py b/lib/lp/app/browser/launchpad.py
index 5d2d6d6..f480b4a 100644
--- a/lib/lp/app/browser/launchpad.py
+++ b/lib/lp/app/browser/launchpad.py
@@ -618,7 +618,9 @@ class LoginStatus:
     @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):
diff --git a/lib/lp/services/webapp/login.py b/lib/lp/services/webapp/login.py
index 8dc179f..5f7d4ff 100644
--- a/lib/lp/services/webapp/login.py
+++ b/lib/lp/services/webapp/login.py
@@ -45,16 +45,27 @@ from zope.session.interfaces import (
     )
 
 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
@@ -271,12 +282,8 @@ class OpenIDLogin(LaunchpadView):
                 [six.ensure_binary(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')
@@ -284,9 +291,6 @@ class OpenIDCallbackView(OpenIDLogin):
     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():
@@ -297,6 +301,31 @@ class OpenIDCallbackView(OpenIDLogin):
 
         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')
@@ -317,13 +346,28 @@ class OpenIDCallbackView(OpenIDLogin):
             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):
@@ -334,6 +378,10 @@ class OpenIDCallbackView(OpenIDLogin):
         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
@@ -352,6 +400,49 @@ class OpenIDCallbackView(OpenIDLogin):
                 "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.
 
@@ -365,25 +456,9 @@ class OpenIDCallbackView(OpenIDLogin):
         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')
-        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)
+        identifier = self._getOpenIDIdentifier()
         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()
+        should_update_last_write = False
 
         if self.params.get('discharge_macaroon_field'):
             if self.macaroon_response.discharge_macaroon_raw is None:
@@ -392,7 +467,30 @@ class OpenIDCallbackView(OpenIDLogin):
             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'):
@@ -439,12 +537,6 @@ class OpenIDCallbackView(OpenIDLogin):
         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):
 
@@ -467,6 +559,81 @@ class OpenIDLoginErrorView(LaunchpadView):
             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'
diff --git a/lib/lp/services/webapp/templates/login-new-account.pt b/lib/lp/services/webapp/templates/login-new-account.pt
 new file mode 100644
index 0000000..6e5887f
--- /dev/null
+++ b/lib/lp/services/webapp/templates/login-new-account.pt
@@ -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>
diff --git a/lib/lp/services/webapp/templates/login-reactivate-account.pt b/lib/lp/services/webapp/templates/login-reactivate-account.pt
 new file mode 100644
index 0000000..3b02d0e
--- /dev/null
+++ b/lib/lp/services/webapp/templates/login-reactivate-account.pt
@@ -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>
diff --git a/lib/lp/services/webapp/tests/test_login.py b/lib/lp/services/webapp/tests/test_login.py
index 1a7dfad..28f9815 100644
--- a/lib/lp/services/webapp/tests/test_login.py
+++ b/lib/lp/services/webapp/tests/test_login.py
@@ -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.openid.model.openididentifier import OpenIdIdentifier
 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 @@ class FakeOpenIDResponse:
         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 @@ class StubbedOpenIDCallbackView(OpenIDCallbackView):
                 "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 @@ class TestOpenIDCallbackView(TestCaseWithFactory):
             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.test/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 @@ class TestOpenIDCallbackView(TestCaseWithFactory):
 
     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 @@ class TestOpenIDCallbackView(TestCaseWithFactory):
             email='[e-mail]', 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.test/+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('[e-mail]'),
+                '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('[e-mail]',
-                         removeSecurityProxy(person.preferredemail).email)
+        self.assertEqual(
+            '[e-mail]',
+            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 @@ class TestOpenIDCallbackView(TestCaseWithFactory):
         email = '[e-mail]'
         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 @@ class TestOpenIDCallbackView(TestCaseWithFactory):
             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.test/+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('[e-mail]'),
+                '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 @@ class TestOpenIDCallbackView(TestCaseWithFactory):
         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 @@ class TestOpenIDCallbackView(TestCaseWithFactory):
 
     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 = '[e-mail]'
         person = self.factory.makePerson(
             displayname='Test account', email=email,
@@ -409,11 +486,37 @@ class TestOpenIDCallbackView(TestCaseWithFactory):
             status=SUCCESS, email=email, full_name=person.displayname)
         with SRegResponse_fromSuccessResponse_stubbed():
             view, html = self._createAndRenderView(openid_response)
-        self.assertTrue(view.login_called)
+        self.assertFalse(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(
+            'http://launchpad.test/+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.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 @@ class TestOpenIDCallbackView(TestCaseWithFactory):
 
     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 = '[e-mail]'
         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 @@ class TestOpenIDCallbackView(TestCaseWithFactory):
             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.test/+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)
diff --git a/lib/lp/testopenid/browser/server.py b/lib/lp/testopenid/browser/server.py
index 18d4434..2e1305c 100644
--- a/lib/lp/testopenid/browser/server.py
+++ b/lib/lp/testopenid/browser/server.py
@@ -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 z3c.ptcompat import ViewPageTemplateFile
 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 @@ from lp.app.browser.launchpadform import (
     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 @@ from lp.services.propertycache import (
     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 @@ class TestOpenIDApplicationNavigation(Navigation):
             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 @@ class OpenIDMixin:
             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 @@ class OpenIDMixin:
         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 @@ class TestOpenIDLoginView(OpenIDMixin, LaunchpadFormView):
 
     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(
diff --git a/lib/lp/testopenid/interfaces/server.py b/lib/lp/testopenid/interfaces/server.py
index 434fee3..92bf847 100644
--- a/lib/lp/testopenid/interfaces/server.py
+++ b/lib/lp/testopenid/interfaces/server.py
@@ -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 ITestOpenIDApplication(ILaunchpadApplication):
 
 
 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):
diff --git a/lib/lp/testopenid/stories/logging-in.txt b/lib/lp/testopenid/stories/logging-in.txt
index 43ce1f1..7a9a8ed 100644
--- a/lib/lp/testopenid/stories/logging-in.txt
+++ b/lib/lp/testopenid/stories/logging-in.txt
@@ -42,6 +42,7 @@ If the email address isn't registered, an error is shown:
     >>> 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
diff --git a/utilities/make-lp-user b/utilities/make-lp-user
index 0f28da4..6cbea4c 100755
--- a/utilities/make-lp-user
+++ b/utilities/make-lp-user
@@ -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 @@ Please note that this script is for testing purposes only. Do NOT use it in
 production environments.
 """
 
+from __future__ import absolute_import, print_function
+
 import _pythonpath
 
 from optparse import OptionParser
@@ -50,6 +52,7 @@ from lp.services.gpg.interfaces import (
     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
@@ -59,14 +62,23 @@ factory = LaunchpadObjectFactory()
 
 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
 
 
@@ -83,15 +95,15 @@ def add_person_to_teams(person, team_names):
     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):
@@ -112,10 +124,10 @@ def add_ssh_public_keys(person):
         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):
@@ -144,7 +156,7 @@ def run_native_gpg(arguments):
         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 @@ def attach_gpg_keys(email, person):
 
     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 @@ def parse_args(arguments):
     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 @@ def main(arguments):
     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()