launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/invisible-person into lp:launchpad.

Commit message:
Add AccountStatus.PLACEHOLDER to give usernames to SSO-only accounts.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/invisible-person/+merge/292940

Add AccountStatus.PLACEHOLDER to give usernames to SSO-only accounts. These accounts are invisible to mortals.

Username mastery should eventually move to SSO, but it is more expedient to create invisible placeholder Person records in LP for now.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/invisible-person into lp:launchpad.

=== modified file 'lib/lp/app/browser/launchpad.py'
--- lib/lp/app/browser/launchpad.py	2015-08-06 00:46:39 +0000
+++ lib/lp/app/browser/launchpad.py	2016-04-26 13:09:54 +0000
@@ -857,12 +857,15 @@
                 # team membership or Launchpad administration.
                 if (person.is_team and
                     not check_permission('launchpad.LimitedView', person)):
-                    raise NotFound(self.context, name)
+                    return None
                 # Only admins are permitted to see suspended users.
                 if person.account_status == AccountStatus.SUSPENDED:
                     if not check_permission('launchpad.Moderate', person):
                         raise GoneError(
                             'User is suspended: %s' % name)
+                if person.account_status == AccountStatus.PLACEHOLDER:
+                    if not check_permission('launchpad.Moderate', person):
+                        return None
                 return person
 
         # Dapper and Edgy shipped with https://launchpad.net/bazaar hard coded

=== modified file 'lib/lp/app/browser/tests/test_launchpad.py'
--- lib/lp/app/browser/tests/test_launchpad.py	2015-01-07 00:35:41 +0000
+++ lib/lp/app/browser/tests/test_launchpad.py	2016-04-26 13:09:54 +0000
@@ -370,6 +370,23 @@
         login_person(self.any_user)
         self.assertRaises(GoneError, self.traverse, segment, segment)
 
+    def test_placeholder_person_visibility(self):
+        # Verify a placeholder user is only traversable by an admin.
+        name = u'placeholder-person'
+        person = getUtility(IPersonSet).createPlaceholderPerson(name, name)
+        login_person(self.admin)
+        segment = '~%s' % name
+        # Admins can see the placeholder user.
+        traversed = self.traverse(segment, segment)
+        self.assertEqual(person, traversed)
+        # Registry experts can see the placeholder user.
+        login_person(self.registry_expert)
+        traversed = self.traverse(segment, segment)
+        self.assertEqual(person, traversed)
+        # Regular users cannot see the placeholder user.
+        login_person(self.any_user)
+        self.assertRaises(NotFound, self.traverse, segment, segment)
+
     def test_public_team(self):
         # Verify a public team is returned.
         name = 'public-team'

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2016-03-18 03:30:26 +0000
+++ lib/lp/registry/interfaces/person.py	2016-04-26 13:09:54 +0000
@@ -406,6 +406,14 @@
         and commercial archive) was made via Software Center.
         """)
 
+    USERNAME_PLACEHOLDER = DBItem(17, """
+        Created by setting a username in SSO.
+
+        Somebody without a Launchpad account set their username in SSO.
+        Since SSO doesn't store usernames directly, an invisible
+        placeholder Launchpad account is required.
+        """)
+
 
 class PersonNameField(BlacklistableContentNameField):
     """A `Person` team name, which is unique and performs psuedo blacklisting.
@@ -2117,6 +2125,19 @@
             used.
         """
 
+    def createPlaceholderPerson(openid_identifier, name):
+        """Create and return an SSO username placeholder `IPerson`.
+
+        The returned Person will have no email address, just a username and an
+        OpenID identifier.
+
+        :param openid_identifier: The SSO account's OpenID suffix.
+        :param name: The person's name.
+        :raises InvalidName: When the passed name isn't valid.
+        :raises NameAlreadyTaken: When the passed name has already been
+            used.
+        """
+
     def ensurePerson(email, displayname, rationale, comment=None,
                      registrant=None):
         """Make sure that there is a person in the database with the given

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2016-03-01 13:02:42 +0000
+++ lib/lp/registry/model/person.py	2016-04-26 13:09:54 +0000
@@ -3397,7 +3397,13 @@
                 raise NameAlreadyTaken(
                     "The account matching the identifier is inactive.")
             elif person.account.status in [AccountStatus.DEACTIVATED,
-                                           AccountStatus.NOACCOUNT]:
+                                           AccountStatus.NOACCOUNT,
+                                           AccountStatus.PLACEHOLDER]:
+                if person.account.status == AccountStatus.PLACEHOLDER:
+                    # Placeholder accounts were never visible to anyone
+                    # before, so make them appear fresh to the user.
+                    removeSecurityProxy(person).display_name = full_name
+                    removeSecurityProxy(person).datecreated = UTC_NOW
                 removeSecurityProxy(person.account).reactivate(comment)
                 if email is None:
                     email = getUtility(IEmailAddressSet).new(
@@ -3490,6 +3496,17 @@
             name, displayname, hide_email_addresses=True, rationale=rationale,
             comment=comment, registrant=registrant)
 
+    def createPlaceholderPerson(self, openid_identifier, name):
+        """See `IPersonSet`."""
+        account = getUtility(IAccountSet).new(
+            AccountCreationRationale.USERNAME_PLACEHOLDER, name,
+            openid_identifier=openid_identifier,
+            status=AccountStatus.PLACEHOLDER)
+        return self._newPerson(
+            name, name, True,
+            rationale=PersonCreationRationale.USERNAME_PLACEHOLDER,
+            comment="when setting a username in SSO", account=account)
+
     def _newPerson(self, name, displayname, hide_email_addresses,
                    rationale, comment=None, registrant=None, account=None):
         """Create and return a new Person with the given attributes."""

=== modified file 'lib/lp/registry/tests/test_personset.py'
--- lib/lp/registry/tests/test_personset.py	2015-10-01 10:25:19 +0000
+++ lib/lp/registry/tests/test_personset.py	2016-04-26 13:09:54 +0000
@@ -6,7 +6,10 @@
 __metaclass__ = type
 
 
-from testtools.matchers import LessThan
+from testtools.matchers import (
+    GreaterThan,
+    LessThan,
+    )
 import transaction
 from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
@@ -424,6 +427,25 @@
         self.assertEqual(AccountStatus.ACTIVE, self.person.account_status)
         self.assertEqual(addr, self.person.preferredemail.email)
 
+    def testPlaceholderAccount(self):
+        # Logging into a username placeholder account activates the
+        # account and adds the email address.
+        email = u'placeholder@xxxxxxxxxxx'
+        openid = u'placeholder-id'
+        name = u'placeholder'
+        person = self.person_set.createPlaceholderPerson(openid, name)
+        self.assertEqual(AccountStatus.PLACEHOLDER, person.account.status)
+        original_created = person.datecreated
+        transaction.commit()
+        found, updated = self.person_set.getOrCreateByOpenIDIdentifier(
+            openid, email, 'New Name', PersonCreationRationale.UNKNOWN,
+            'No Comment')
+        self.assertEqual(person, found)
+        self.assertEqual(AccountStatus.ACTIVE, person.account.status)
+        self.assertEqual(name, person.name)
+        self.assertEqual('New Name', person.display_name)
+        self.assertThat(person.datecreated, GreaterThan(original_created))
+
 
 class TestCreatePersonAndEmail(TestCase):
     """Test `IPersonSet`.createPersonAndEmail()."""

=== modified file 'lib/lp/services/identity/interfaces/account.py'
--- lib/lp/services/identity/interfaces/account.py	2016-01-26 15:47:37 +0000
+++ lib/lp/services/identity/interfaces/account.py	2016-04-26 13:09:54 +0000
@@ -50,6 +50,16 @@
 class AccountStatus(DBEnumeratedType):
     """The status of an account."""
 
+    PLACEHOLDER = DBItem(5, """
+        Placeholder
+
+        The account was created automatically by SSO and exists solely
+        to hold a username. It is visible only to staff.
+
+        XXX wgrant 2016-03-03: This is a hack until usernames are moved
+            into SSO properly.
+        """)
+
     NOACCOUNT = DBItem(10, """
         Unactivated
 
@@ -76,7 +86,8 @@
 
 
 INACTIVE_ACCOUNT_STATUSES = [
-    AccountStatus.DEACTIVATED, AccountStatus.SUSPENDED]
+    AccountStatus.PLACEHOLDER, AccountStatus.DEACTIVATED,
+    AccountStatus.SUSPENDED]
 
 
 class AccountCreationRationale(DBEnumeratedType):
@@ -206,6 +217,14 @@
         and commercial archive) was made via Software Center.
         """)
 
+    USERNAME_PLACEHOLDER = DBItem(17, """
+        Created by setting a username in SSO.
+
+        Somebody without a Launchpad account set their username in SSO.
+        Since SSO doesn't store usernames directly, an invisible
+        placeholder Launchpad account is required.
+        """)
+
 
 class AccountStatusError(LaunchpadValidationError):
     """The account status cannot change to the proposed status."""
@@ -215,6 +234,8 @@
     """A valid status and transition."""
 
     transitions = {
+        AccountStatus.PLACEHOLDER: [
+            AccountStatus.NOACCOUNT, AccountStatus.ACTIVE],
         AccountStatus.NOACCOUNT: [AccountStatus.ACTIVE],
         AccountStatus.ACTIVE: [
             AccountStatus.DEACTIVATED, AccountStatus.SUSPENDED],
@@ -224,7 +245,7 @@
 
     def constraint(self, value):
         """See `IField`."""
-        if not IAccount.providedBy(self.context):
+        if removeSecurityProxy(self.context)._SO_creating:
             # This object is initializing.
             return True
         if self.context.status == value:

=== modified file 'lib/lp/services/identity/model/account.py'
--- lib/lp/services/identity/model/account.py	2015-07-08 16:05:11 +0000
+++ lib/lp/services/identity/model/account.py	2016-04-26 13:09:54 +0000
@@ -92,11 +92,13 @@
 class AccountSet:
     """See `IAccountSet`."""
 
-    def new(self, rationale, displayname, openid_identifier=None):
+    def new(self, rationale, displayname, openid_identifier=None,
+            status=AccountStatus.NOACCOUNT):
         """See `IAccountSet`."""
-
+        assert status in (AccountStatus.NOACCOUNT, AccountStatus.PLACEHOLDER)
         account = Account(
-            displayname=displayname, creation_rationale=rationale)
+            displayname=displayname, creation_rationale=rationale,
+            status=status)
 
         # Create an OpenIdIdentifier record if requested.
         if openid_identifier is not None: