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)