From f06eec8a3525262ae86a27c64d80f8deadb3e420 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 9 May 2018 14:52:28 +0300 Subject: [PATCH] Fix issue where we didn't delete robots immediately under a namespace This could result in "hanging" robot accounts, although that would only leak the names of said accounts. Now we delete them immediately AND we proactively delete them before replacing the namespace (just to be sure) --- .../5b7503aada1b_cleanup_old_robots.py | 23 ++++++++++ data/model/test/test_user.py | 22 +++++++++- data/model/user.py | 26 +++++++---- util/migrate/cleanup_old_robots.py | 28 ++++++++++++ util/migrate/test/test_cleanup_old_robots.py | 43 +++++++++++++++++++ 5 files changed, 132 insertions(+), 10 deletions(-) create mode 100644 data/migrations/versions/5b7503aada1b_cleanup_old_robots.py create mode 100644 util/migrate/cleanup_old_robots.py create mode 100644 util/migrate/test/test_cleanup_old_robots.py diff --git a/data/migrations/versions/5b7503aada1b_cleanup_old_robots.py b/data/migrations/versions/5b7503aada1b_cleanup_old_robots.py new file mode 100644 index 000000000..d184790bc --- /dev/null +++ b/data/migrations/versions/5b7503aada1b_cleanup_old_robots.py @@ -0,0 +1,23 @@ +"""Cleanup old robots + +Revision ID: 5b7503aada1b +Revises: 224ce4c72c2f +Create Date: 2018-05-09 17:18:52.230504 + +""" + +# revision identifiers, used by Alembic. +revision = '5b7503aada1b' +down_revision = '224ce4c72c2f' + +from alembic import op +import sqlalchemy as sa + +from util.migrate.cleanup_old_robots import cleanup_old_robots + +def upgrade(tables): + cleanup_old_robots() + +def downgrade(tables): + # Nothing to do. + pass diff --git a/data/model/test/test_user.py b/data/model/test/test_user.py index ce0649322..1f004ba09 100644 --- a/data/model/test/test_user.py +++ b/data/model/test/test_user.py @@ -7,6 +7,8 @@ from mock import patch from data.database import EmailConfirmation, User, DeletedNamespace from data.model.user import create_user_noverify, validate_reset_code, get_active_users from data.model.user import mark_namespace_for_deletion, delete_namespace_via_marker +from data.model.user import create_robot, lookup_robot, list_namespace_robots +from data.model.user import InvalidRobotException from util.timedeltastring import convert_to_timedelta from data.queue import WorkQueue from test.fixtures import * @@ -48,12 +50,30 @@ def test_mark_namespace_for_deletion(initialized_db): # Create a user and then mark it for deletion. user = create_user_noverify('foobar', 'foo@example.com', email_required=False) + # Add some robots. + create_robot('foo', user) + create_robot('bar', user) + + assert lookup_robot('foobar+foo') is not None + assert lookup_robot('foobar+bar') is not None + assert len(list(list_namespace_robots('foobar'))) == 2 + # Mark the user for deletion. queue = WorkQueue('testgcnamespace', create_transaction) mark_namespace_for_deletion(user, [], queue) # Ensure the older user is still in the DB. - assert User.get(id=user.id).username != 'foobar' + older_user = User.get(id=user.id) + assert older_user.username != 'foobar' + + # Ensure the robots are deleted. + with pytest.raises(InvalidRobotException): + assert lookup_robot('foobar+foo') + + with pytest.raises(InvalidRobotException): + assert lookup_robot('foobar+bar') + + assert len(list(list_namespace_robots(older_user.username))) == 0 # Ensure we can create a user with the same namespace again. new_user = create_user_noverify('foobar', 'foo@example.com', email_required=False) diff --git a/data/model/user.py b/data/model/user.py index c742824e9..73d3936e1 100644 --- a/data/model/user.py +++ b/data/model/user.py @@ -68,7 +68,6 @@ def create_user_noverify(username, email, email_required=True, prompts=tuple(), try: existing = User.get((User.username == username) | (User.email == email)) - logger.info('Existing user with same username or email.') # A user already exists with either the same username or email @@ -77,11 +76,11 @@ def create_user_noverify(username, email, email_required=True, prompts=tuple(), username) raise InvalidEmailAddressException('Email has already been used: %s' % email) - except User.DoesNotExist: # This is actually the happy path logger.debug('Email and username are unique!') + # Create the user. try: default_expr_s = _convert_to_s(config.app_config['DEFAULT_TAG_EXPIRATION']) default_max_builds = config.app_config.get('DEFAULT_NAMESPACE_MAXIMUM_BUILD_COUNT') @@ -970,20 +969,29 @@ def delete_user(user, queues): def _delete_user_linked_data(user): if user.organization: # Delete the organization's teams. - for team in Team.select().where(Team.organization == user): - team.delete_instance(recursive=True) + with db_transaction(): + for team in Team.select().where(Team.organization == user): + team.delete_instance(recursive=True) # Delete any OAuth approvals and tokens associated with the user. - for app in OAuthApplication.select().where(OAuthApplication.organization == user): - app.delete_instance(recursive=True) + with db_transaction(): + for app in OAuthApplication.select().where(OAuthApplication.organization == user): + app.delete_instance(recursive=True) else: # Remove the user from any teams in which they are a member. TeamMember.delete().where(TeamMember.user == user).execute() # Delete any repository buildtriggers where the user is the connected user. - triggers = RepositoryBuildTrigger.select().where(RepositoryBuildTrigger.connected_user == user) - for trigger in triggers: - trigger.delete_instance(recursive=True, delete_nullable=False) + with db_transaction(): + triggers = RepositoryBuildTrigger.select().where(RepositoryBuildTrigger.connected_user == user) + for trigger in triggers: + trigger.delete_instance(recursive=True, delete_nullable=False) + + # Delete any robots owned by this user. + with db_transaction(): + robots = list(list_namespace_robots(user.username)) + for robot in robots: + robot.delete_instance(recursive=True, delete_nullable=True) # Null out any service key approvals. We technically lose information here, but its better than # falling and only occurs if a superuser is being deleted. diff --git a/util/migrate/cleanup_old_robots.py b/util/migrate/cleanup_old_robots.py new file mode 100644 index 000000000..3e726bcb4 --- /dev/null +++ b/util/migrate/cleanup_old_robots.py @@ -0,0 +1,28 @@ +from data.database import User +from util.names import parse_robot_username + +def cleanup_old_robots(page_size=50): + """ Deletes any robots that live under namespaces that no longer exist. """ + # Collect the robot accounts to delete. + page_number = 1 + to_delete = [] + + while True: + found_bots = False + for robot in list(User.select().where(User.robot == True).paginate(page_number, page_size)): + found_bots = True + namespace, _ = parse_robot_username(robot.username) + try: + User.get(username=namespace) + except User.DoesNotExist: + # Save the robot account for deletion. + to_delete.append(robot) + + if not found_bots: + break + + page_number = page_number + 1 + + # Cleanup any robot accounts whose corresponding namespace doesn't exist. + for robot in to_delete: + robot.delete_instance(recursive=True, delete_nullable=True) diff --git a/util/migrate/test/test_cleanup_old_robots.py b/util/migrate/test/test_cleanup_old_robots.py new file mode 100644 index 000000000..9d4fad0e3 --- /dev/null +++ b/util/migrate/test/test_cleanup_old_robots.py @@ -0,0 +1,43 @@ +import pytest + +from data.database import User +from util.migrate.cleanup_old_robots import cleanup_old_robots + +from test.fixtures import * + +def test_cleanup_old_robots(initialized_db): + before_robot_count = User.select().where(User.robot == True).count() + before_user_count = User.select().count() + + # Run the cleanup once, and ensure it does nothing. + cleanup_old_robots() + + after_robot_count = User.select().where(User.robot == True).count() + after_user_count = User.select().count() + + assert before_robot_count == after_robot_count + assert before_user_count == after_user_count + + # Create some orphan robots. + created = set() + for index in range(0, 50): + created.add('doesnotexist+a%s' % index) + created.add('anothernamespace+b%s' % index) + + User.create(username='doesnotexist+a%s' % index, robot=True) + User.create(username='anothernamespace+b%s' % index, robot=True) + + before_robot_count = User.select().where(User.robot == True).count() + before_user_count = User.select().count() + + cleanup_old_robots(page_size=10) + + after_robot_count = User.select().where(User.robot == True).count() + after_user_count = User.select().count() + + assert before_robot_count == after_robot_count + len(created) + assert before_user_count == after_user_count + len(created) + + for name in created: + with pytest.raises(User.DoesNotExist): + User.get(username=name)