Merge lp:~cjwatson/launchpad/close-account-perms into lp:launchpad

[Colin Watson]

Make close-account work as a non-superuser.

I also Stormified and generally modernised close-account.

[William Grant (wgrant)]

I think we need to look over all Person FKs and work out what to do. The script has only been used a handful of times and then probably not for more than a decade, and it has weird stuff that bypasses audit trails, sets invalid questions to solved, wipes the audit trail in Account.status_comment, etc.

The port to Storm seems fine, but this can't go near production yet.

review: Approve (code)

[Colin Watson (cjwatson)]

I've proposed https://code.launchpad.net/~cjwatson/launchpad/close-account-polish/+merge/359865 to try to make this safer and somewhat less weird.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2018-10-16 10:10:10 +0000
+++ database/schema/security.cfg	2018-11-28 18:58:57 +0000
@@ -333,7 +333,9 @@
 [launchpad]
 groups=launchpad_main
 type=user
+public.karma                            = SELECT, INSERT, UPDATE, DELETE
 public.sharingjob                       = SELECT, INSERT, UPDATE, DELETE
+public.signedcodeofconduct              = SELECT, INSERT, UPDATE, DELETE
 
 [script]
 public.accessartifact                   = SELECT

=== added file 'lib/lp/registry/scripts/closeaccount.py'
--- lib/lp/registry/scripts/closeaccount.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/scripts/closeaccount.py	2018-11-28 18:58:57 +0000
@@ -0,0 +1,193 @@
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Remove personal details of a user from the database, leaving a stub."""
+
+__metaclass__ = type
+__all__ = ['CloseAccountScript']
+
+from storm.expr import (
+    LeftJoin,
+    Lower,
+    Or,
+    )
+
+from lp.answers.enums import QuestionStatus
+from lp.answers.model.question import Question
+from lp.bugs.model.bugtask import BugTask
+from lp.registry.interfaces.person import PersonCreationRationale
+from lp.registry.model.person import Person
+from lp.services.database.interfaces import IMasterStore
+from lp.services.identity.model.account import Account
+from lp.services.identity.model.emailaddress import EmailAddress
+from lp.services.scripts.base import (
+    LaunchpadScript,
+    LaunchpadScriptFailure,
+    )
+
+
+def close_account(username, log):
+    """Close a person's account.
+
+    Return True on success, or log an error message and return False
+    """
+    store = IMasterStore(Person)
+
+    row = store.using(
+        Person,
+        LeftJoin(EmailAddress, Person.id == EmailAddress.personID)
+    ).find(
+        (Person.id, Person.accountID, Person.name, Person.teamownerID),
+        Or(Person.name == username,
+           Lower(EmailAddress.email) == Lower(username))).one()
+    if row is None:
+        raise LaunchpadScriptFailure("User %s does not exist" % username)
+    person_id, account_id, username, teamowner_id = row
+
+    # We don't do teams
+    if teamowner_id is not None:
+        raise LaunchpadScriptFailure("%s is a team" % username)
+
+    log.info("Closing %s's account" % username)
+
+    def table_notification(table):
+        log.debug("Handling the %s table" % table)
+
+    # All names starting with 'removed' are blacklisted, so this will always
+    # succeed.
+    new_name = 'removed%d' % person_id
+
+    # Remove the EmailAddress. This is the most important step, as
+    # people requesting account removal seem to primarily be interested
+    # in ensuring we no longer store this information.
+    table_notification('EmailAddress')
+    store.find(EmailAddress, EmailAddress.personID == person_id).remove()
+
+    # Clean out personal details from the Person table
+    table_notification('Person')
+    store.find(Person, Person.id == person_id).set(
+        display_name='Removed by request',
+        name=new_name,
+        accountID=None,
+        homepage_content=None,
+        iconID=None,
+        mugshotID=None,
+        hide_email_addresses=True,
+        registrantID=None,
+        logoID=None,
+        creation_rationale=PersonCreationRationale.UNKNOWN,
+        creation_comment=None)
+
+    # Remove the Account. We don't set the status to deactivated,
+    # as this script is used to satisfy people who insist on us removing
+    # all their personal details from our systems. This includes any
+    # identification tokens like email addresses or openid identifiers.
+    # So the Account record would be unusable, and contain no useful
+    # information.
+    table_notification('Account')
+    if account_id is not None:
+        store.find(Account, Account.id == account_id).remove()
+
+    # Reassign their bugs
+    table_notification('BugTask')
+    store.find(BugTask, BugTask.assigneeID == person_id).set(assigneeID=None)
+
+    # Reassign questions assigned to the user, and close all their questions
+    # since nobody else can
+    table_notification('Question')
+    store.find(Question, Question.assigneeID == person_id).set(assigneeID=None)
+    store.find(Question, Question.ownerID == person_id).set(
+        status=QuestionStatus.SOLVED,
+        whiteboard=(
+            'Closed by Launchpad due to owner requesting account removal'))
+
+    # Remove rows from tables in simple cases in the given order
+    removals = [
+        # Trash their email addresses. People who request complete account
+        # removal would be unhappy if they reregistered with their old email
+        # address and this resurrected their deleted account, as the email
+        # address is probably the piece of data we store that they were most
+        # concerned with being removed from our systems.
+        ('EmailAddress', 'person'),
+
+        # Trash their codes of conduct and GPG keys
+        ('SignedCodeOfConduct', 'owner'),
+        ('GpgKey', 'owner'),
+
+        # Subscriptions
+        ('BranchSubscription', 'person'),
+        ('GitSubscription', 'person'),
+        ('BugSubscription', 'person'),
+        ('QuestionSubscription', 'person'),
+        ('SpecificationSubscription', 'person'),
+
+        # Personal stuff, freeing up the namespace for others who want to play
+        # or just to remove any fingerprints identifying the user.
+        ('IrcId', 'person'),
+        ('JabberId', 'person'),
+        ('WikiName', 'person'),
+        ('PersonLanguage', 'person'),
+        ('PersonLocation', 'person'),
+        ('SshKey', 'person'),
+
+        # Karma
+        ('Karma', 'person'),
+        ('KarmaCache', 'person'),
+        ('KarmaTotalCache', 'person'),
+
+        # Team memberships
+        ('TeamMembership', 'person'),
+        ('TeamParticipation', 'person'),
+
+        # Contacts
+        ('AnswerContact', 'person'),
+
+        # Pending items in queues
+        ('POExportRequest', 'person'),
+
+        # Access grants
+        ('GitRuleGrant', 'grantee'),
+        ]
+    for table, person_id_column in removals:
+        table_notification(table)
+        store.execute("""
+            DELETE FROM %(table)s WHERE %(person_id_column)s = ?
+            """ % {
+                'table': table,
+                'person_id_column': person_id_column,
+                },
+            (person_id,))
+
+    # Trash Sprint Attendance records in the future.
+    table_notification('SprintAttendance')
+    store.execute("""
+        DELETE FROM SprintAttendance
+        USING Sprint
+        WHERE Sprint.id = SprintAttendance.sprint
+            AND attendee = ?
+            AND Sprint.time_starts > CURRENT_TIMESTAMP AT TIME ZONE 'UTC'
+        """, (person_id,))
+
+    return True
+
+
+class CloseAccountScript(LaunchpadScript):
+
+    usage = '%prog [options] (username|email) [...]'
+    description = (
+        "Close a person's account, deleting as much personal information "
+        "as possible.")
+
+    def main(self):
+        if not self.args:
+            raise LaunchpadScriptFailure(
+                "Must specify username (Person.name)")
+
+        for username in self.args:
+            try:
+                close_account(unicode(username), self.logger)
+            except Exception:
+                self.txn.abort()
+                raise
+        self.logger.debug("Committing changes")
+        self.txn.commit()

=== added file 'lib/lp/registry/scripts/tests/test_closeaccount.py'
--- lib/lp/registry/scripts/tests/test_closeaccount.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/scripts/tests/test_closeaccount.py	2018-11-28 18:58:57 +0000
@@ -0,0 +1,113 @@
+# Copyright 2018 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test the close-account script."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+
+from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
+
+from lp.registry.interfaces.person import IPersonSet
+from lp.registry.scripts.closeaccount import CloseAccountScript
+from lp.services.identity.interfaces.account import (
+    AccountStatus,
+    IAccountSet,
+    )
+from lp.services.log.logger import DevNullLogger
+from lp.services.scripts.base import LaunchpadScriptFailure
+from lp.testing import TestCaseWithFactory
+from lp.testing.dbuser import dbuser
+from lp.testing.faketransaction import FakeTransaction
+from lp.testing.layers import ZopelessDatabaseLayer
+
+
+class TestCloseAccount(TestCaseWithFactory):
+    """Test the close-account script.
+
+    Unfortunately, we have no way of detecting schema updates containing new
+    information that needs to be removed or sanitized on account closure
+    apart from reviewers noticing and prompting developers to update this
+    script.  See Bug #120506 for more details.
+    """
+
+    layer = ZopelessDatabaseLayer
+
+    def makeScript(self, test_args):
+        script = CloseAccountScript(test_args=test_args)
+        script.logger = DevNullLogger()
+        script.txn = FakeTransaction()
+        return script
+
+    def getSampleUser(self, name, email):
+        """Return a sampledata account with some personal information."""
+        person = getUtility(IPersonSet).getByEmail(email)
+        account = removeSecurityProxy(person.account)
+        self.assertEqual(AccountStatus.ACTIVE, account.status)
+        self.assertEqual(name, person.name)
+        return person.id, account.id
+
+    def assertRemoved(self, account_id, person_id):
+        # We can't just set the account to DEACTIVATED, as the
+        # close-account.py script is used to satisfy people who insist on us
+        # removing all their personal details from our system.  The Account
+        # has been removed entirely.
+        self.assertRaises(
+            LookupError, getUtility(IAccountSet).get, account_id)
+
+        # The Person record still exists to maintain links with information
+        # that won't be removed, such as bug comments, but has been
+        # anonymized.
+        person = getUtility(IPersonSet).get(person_id)
+        self.assertStartsWith(person.name, 'removed')
+        self.assertEqual('Removed by request', person.display_name)
+
+    def assertNotRemoved(self, account_id, person_id):
+        account = getUtility(IAccountSet).get(account_id)
+        self.assertEqual(AccountStatus.ACTIVE, account.status)
+        person = getUtility(IPersonSet).get(person_id)
+        self.assertIsNotNone(person.account)
+
+    def test_nonexistent(self):
+        script = self.makeScript(['nonexistent-person'])
+        with dbuser('launchpad'):
+            self.assertRaisesWithContent(
+                LaunchpadScriptFailure,
+                'User nonexistent-person does not exist',
+                script.main)
+
+    def test_team(self):
+        team = self.factory.makeTeam()
+        script = self.makeScript([team.name])
+        with dbuser('launchpad'):
+            self.assertRaisesWithContent(
+                LaunchpadScriptFailure,
+                '%s is a team' % team.name,
+                script.main)
+
+    def test_single_by_name(self):
+        person_id, account_id = self.getSampleUser('mark', '[e-mail]')
+        script = self.makeScript(['mark'])
+        with dbuser('launchpad'):
+            script.main()
+        self.assertRemoved(account_id, person_id)
+
+    def test_single_by_email(self):
+        person_id, account_id = self.getSampleUser('mark', '[e-mail]')
+        script = self.makeScript(['[e-mail]'])
+        with dbuser('launchpad'):
+            script.main()
+        self.assertRemoved(account_id, person_id)
+
+    def test_multiple(self):
+        persons = [self.factory.makePerson() for _ in range(3)]
+        person_ids = [person.id for person in persons]
+        account_ids = [person.account.id for person in persons]
+        script = self.makeScript([persons[0].name, persons[1].name])
+        with dbuser('launchpad'):
+            script.main()
+        self.assertRemoved(account_ids[0], person_ids[0])
+        self.assertRemoved(account_ids[1], person_ids[1])
+        self.assertNotRemoved(account_ids[2], person_ids[2])

=== removed file 'lib/lp/services/identity/doc/close-account.txt'
--- lib/lp/services/identity/doc/close-account.txt	2011-12-29 05:29:36 +0000
+++ lib/lp/services/identity/doc/close-account.txt	1970-01-01 00:00:00 +0000
@@ -1,83 +0,0 @@
-We have a command line script that can be used to close accounts.
-
-Unfortunately, we have no way of detecting schema updates containing new
-information that needs to be removed or sanitized on account closure apart
-from reviewers noticing and prompting developers to update this script.
-
-See Bug #120506 for more details.
-
-
-Get Mark's account and person entries.
->>> from lp.registry.interfaces.person import IPersonSet
->>> from zope.security.proxy import removeSecurityProxy
->>> mark_person = getUtility(IPersonSet).getByEmail('[e-mail]')
->>> mark_account = removeSecurityProxy(mark_person.account)
-
-
-Mark's account is active and contains personal information.
-
->>> print mark_account.status.name
-ACTIVE
->>> print mark_account.displayname
-Mark Shuttleworth
->>> print mark_person.name
-mark
->>> print mark_person.displayname
-Mark Shuttleworth
-
-
-Store the id's so we can retrieve the records later.
-
->>> mark_person_id = mark_person.id
->>> mark_account_id = mark_account.id
-
-
-Lets close his account.
-
->>> import os.path
->>> from lp.services.config import config
->>> from lp.testing.script import run_script
->>> script = os.path.join(config.root, 'scripts', 'close-account.py')
->>> (result, out, err) = run_script(script, args=['[e-mail]'])
->>> print result
-0
->>> print out
->>> print err
-INFO...Closing mark's account
-
-
-Now, start a new transaction so we can see the changes the script made.
-
->>> from lp.testing.layers import LaunchpadZopelessLayer
->>> LaunchpadZopelessLayer.abort()
-
-
-We can't just set the account to DEACTIVATED, as the close-account.py
-script is used to satisty people who insist on us removing all their
-personal details from our system. The Account has been removed entirely.
-
->>> from lp.services.identity.model.account import Account
->>> Account.get(mark_account_id)
-Traceback (most recent call last):
-...
-SQLObjectNotFound: ...
-
-
-The Person record still exists to maintain links with information that won't
-be removed, such as bug comments, but has been anonymized.
-
->>> from lp.registry.model.person import Person
->>> mark_person = Person.get(mark_person_id)
->>> print mark_person.name
-removed...
->>> print mark_person.displayname
-Removed by request
-
-
-Flag the database as dirty since it has been modified without the test suite
-knowing.
-
->>> from lp.testing.layers import DatabaseLayer
->>> DatabaseLayer.force_dirty_database()
-
-

=== modified file 'lib/lp/services/identity/tests/test_doc.py'
--- lib/lp/services/identity/tests/test_doc.py	2012-01-01 02:58:52 +0000
+++ lib/lp/services/identity/tests/test_doc.py	2018-11-28 18:58:57 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 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).
 
 """
@@ -8,27 +8,12 @@
 import os
 
 from lp.services.testing import build_test_suite
-from lp.testing.layers import (
-    DatabaseFunctionalLayer,
-    LaunchpadZopelessLayer,
-    )
-from lp.testing.systemdocs import (
-    LayeredDocFileSuite,
-    setUp,
-    tearDown,
-    )
+from lp.testing.layers import DatabaseFunctionalLayer
 
 
 here = os.path.dirname(os.path.realpath(__file__))
 
 
-special = {
-    'close-account.txt': LayeredDocFileSuite(
-    '../doc/close-account.txt', setUp=setUp, tearDown=tearDown,
-    layer=LaunchpadZopelessLayer),
-    }
-
-
 def test_suite():
-    suite = build_test_suite(here, special, layer=DatabaseFunctionalLayer)
+    suite = build_test_suite(here, layer=DatabaseFunctionalLayer)
     return suite

=== modified file 'scripts/close-account.py'
--- scripts/close-account.py	2018-09-12 11:53:08 +0000
+++ scripts/close-account.py	2018-11-28 18:58:57 +0000
@@ -5,219 +5,11 @@
 
 """Remove personal details of a user from the database, leaving a stub."""
 
-__metaclass__ = type
-__all__ = []
-
 import _pythonpath
 
-from optparse import OptionParser
-import sys
-
-from lp.answers.enums import QuestionStatus
-from lp.registry.interfaces.person import PersonCreationRationale
-from lp.services.database.sqlbase import (
-    connect,
-    sqlvalues,
-    )
-from lp.services.scripts import (
-    db_options,
-    logger,
-    logger_options,
-    )
-
-
-def close_account(con, log, username):
-    """Close a person's account.
-
-    Return True on success, or log an error message and return False
-    """
-    cur = con.cursor()
-    cur.execute("""
-        SELECT Person.id, Person.account, name, teamowner
-        FROM Person
-        LEFT OUTER JOIN EmailAddress ON Person.id = EmailAddress.person
-        WHERE name = %(username)s OR lower(email) = lower(%(username)s)
-        """, vars())
-    try:
-        person_id, account_id, username, teamowner = cur.fetchone()
-    except TypeError:
-        log.fatal("User %s does not exist" % username)
-        return False
-
-    # We don't do teams
-    if teamowner is not None:
-        log.fatal("%s is a team" % username)
-        return False
-
-    log.info("Closing %s's account" % username)
-
-    def table_notification(table):
-        log.debug("Handling the %s table" % table)
-
-    # All names starting with 'removed' are blacklisted, so this will always
-    # succeed.
-    new_name = 'removed%d' % person_id
-
-    # Remove the EmailAddress. This is the most important step, as
-    # people requesting account removal seem to primarily be interested
-    # in ensuring we no longer store this information.
-    table_notification('EmailAddress')
-    cur.execute("""
-        DELETE FROM EmailAddress WHERE person = %s
-        """ % sqlvalues(person_id))
-
-    # Clean out personal details from the Person table
-    table_notification('Person')
-    unknown_rationale = PersonCreationRationale.UNKNOWN.value
-    cur.execute("""
-        UPDATE Person
-        SET
-            displayname = 'Removed by request',
-            name=%(new_name)s,
-            language = NULL,
-            account = NULL,
-            homepage_content = NULL,
-            icon = NULL,
-            mugshot = NULL,
-            hide_email_addresses = TRUE,
-            registrant = NULL,
-            logo = NULL,
-            creation_rationale = %(unknown_rationale)s,
-            creation_comment = NULL
-        WHERE id = %(person_id)s
-        """, vars())
-
-    # Remove the Account. We don't set the status to deactivated,
-    # as this script is used to satisfy people who insist on us removing
-    # all their personal details from our systems. This includes any
-    # identification tokens like email addresses or openid identifiers.
-    # So the Account record would be unusable, and contain no useful
-    # information.
-    table_notification('Account')
-    if account_id is not None:
-        cur.execute("""
-            DELETE FROM Account WHERE id = %s
-            """ % sqlvalues(account_id))
-
-    # Reassign their bugs
-    table_notification('BugTask')
-    cur.execute("""
-        UPDATE BugTask SET assignee = NULL WHERE assignee = %(person_id)s
-        """, vars())
-
-    # Reassign questions assigned to the user, and close all their questions
-    # since nobody else can
-    table_notification('Question')
-    cur.execute("""
-        UPDATE Question SET assignee=NULL WHERE assignee=%(person_id)s
-        """, vars())
-    closed_question_status = QuestionStatus.SOLVED.value
-    cur.execute("""
-        UPDATE Question
-        SET status=%(closed_question_status)s, whiteboard=
-            'Closed by Launchpad due to owner requesting account removal'
-        WHERE owner=%(person_id)s
-        """, vars())
-
-    # Remove rows from tables in simple cases in the given order
-    removals = [
-        # Trash their email addresses. People who request complete account
-        # removal would be unhappy if they reregistered with their old email
-        # address and this resurrected their deleted account, as the email
-        # address is probably the piece of data we store that they were most
-        # concerned with being removed from our systems.
-        ('EmailAddress', 'person'),
-
-        # Trash their codes of conduct and GPG keys
-        ('SignedCodeOfConduct', 'owner'),
-        ('GpgKey', 'owner'),
-
-        # Subscriptions
-        ('BranchSubscription', 'person'),
-        ('GitSubscription', 'person'),
-        ('BugSubscription', 'person'),
-        ('QuestionSubscription', 'person'),
-        ('SpecificationSubscription', 'person'),
-
-        # Personal stuff, freeing up the namespace for others who want to play
-        # or just to remove any fingerprints identifying the user.
-        ('IrcId', 'person'),
-        ('JabberId', 'person'),
-        ('WikiName', 'person'),
-        ('PersonLanguage', 'person'),
-        ('PersonLocation', 'person'),
-        ('SshKey', 'person'),
-
-        # Karma
-        ('Karma', 'person'),
-        ('KarmaCache', 'person'),
-        ('KarmaTotalCache', 'person'),
-
-        # Team memberships
-        ('TeamMembership', 'person'),
-        ('TeamParticipation', 'person'),
-
-        # Contacts
-        ('AnswerContact', 'person'),
-
-        # Pending items in queues
-        ('POExportRequest', 'person'),
-
-        # Access grants
-        ('GitRuleGrant', 'grantee'),
-        ]
-    for table, person_id_column in removals:
-        table_notification(table)
-        cur.execute("""
-                DELETE FROM %(table)s WHERE %(person_id_column)s=%(person_id)d
-                """ % vars())
-
-    # Trash Sprint Attendance records in the future.
-    table_notification('SprintAttendance')
-    cur.execute("""
-        DELETE FROM SprintAttendance
-        USING Sprint
-        WHERE Sprint.id = SprintAttendance.sprint
-            AND attendee=%(person_id)s
-            AND Sprint.time_starts > CURRENT_TIMESTAMP AT TIME ZONE 'UTC'
-        """, vars())
-
-    return True
-
-
-def main():
-    parser = OptionParser(
-            '%prog [options] (username|email) [...]'
-            )
-    db_options(parser)
-    logger_options(parser)
-
-    (options, args) = parser.parse_args()
-
-    if len(args) == 0:
-        parser.error("Must specify username (Person.name)")
-
-    log = logger(options)
-
-    con = None
-    try:
-        log.debug("Connecting to database")
-        con = connect()
-        for username in args:
-            if not close_account(con, log, username):
-                log.debug("Rolling back")
-                con.rollback()
-                return 1
-        log.debug("Committing changes")
-        con.commit()
-        return 0
-    except:
-        log.exception("Unhandled exception")
-        log.debug("Rolling back")
-        if con is not None:
-            con.rollback()
-        return 1
+from lp.registry.scripts.closeaccount import CloseAccountScript
 
 
 if __name__ == '__main__':
-    sys.exit(main())
+    script = CloseAccountScript('close-account', dbuser='launchpad')
+    script.run()