launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #23109
[Merge] lp:~cjwatson/launchpad/close-account-perms into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/close-account-perms into lp:launchpad.
Commit message:
Make close-account work as a non-superuser.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/close-account-perms/+merge/359773
I also Stormified and generally modernised close-account.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/close-account-perms into lp:launchpad.
=== 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', 'mark@xxxxxxxxxxx')
+ 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', 'mark@xxxxxxxxxxx')
+ script = self.makeScript(['mark@xxxxxxxxxxx'])
+ 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('mark@xxxxxxxxxxx')
->>> 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=['mark@xxxxxxxxxxx'])
->>> 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()
Follow ups
