From acadd057d9a81e41cb4120802227c6b4a17b8031 Mon Sep 17 00:00:00 2001 From: bmedx Date: Wed, 8 Aug 2018 17:22:29 -0400 Subject: [PATCH 1/2] Bulk rehashing of retired usernames --- .../commands/bulk_rehash_retired_usernames.py | 84 +++++++++++++++++++ .../test_bulk_rehash_retired_usernames.py | 82 ++++++++++++++++++ 2 files changed, 166 insertions(+) create mode 100644 openedx/core/djangoapps/user_api/management/commands/bulk_rehash_retired_usernames.py create mode 100644 openedx/core/djangoapps/user_api/management/tests/test_bulk_rehash_retired_usernames.py diff --git a/openedx/core/djangoapps/user_api/management/commands/bulk_rehash_retired_usernames.py b/openedx/core/djangoapps/user_api/management/commands/bulk_rehash_retired_usernames.py new file mode 100644 index 0000000000..68ec4037b7 --- /dev/null +++ b/openedx/core/djangoapps/user_api/management/commands/bulk_rehash_retired_usernames.py @@ -0,0 +1,84 @@ +""" +One-off script to rehash all retired usernames in UserRetirementStatus and auth_user. +Background: We discovered that all prior retired usernames were generated based on +the exact capitalization of the original username, despite the fact that +usernames are considered case insensitive in practice. This led to the +possibility of users registering accounts with effectively retired usernames just +by changing the capitalization of the username, because the different +capitalization would hash to a different digest. +Solution: Rehash all usernames using the normalized-case (lowercase) +original usernames rather than the possibly mixed-case ones. This management +command likely cannot be re-used in the future because eventually we will need +to clean out the UserRetirementStatus table. +""" +from __future__ import print_function +from django.conf import settings +from django.db import transaction +from django.core.management.base import BaseCommand +from openedx.core.djangoapps.user_api.models import UserRetirementStatus +from user_util import user_util + + +class Command(BaseCommand): + """ + Implementation of the bulk_rehash_retired_usernames command. + """ + def add_arguments(self, parser): + parser.add_argument( + '--dry_run', + action='store_true', + help='Print proposed changes, but take no action.' + ) + + def handle(self, *args, **options): + """ + Execute the command. + """ + dry_run = options['dry_run'] + retirements = UserRetirementStatus.objects.all().select_related('user') + for retirement in retirements: + original_username = retirement.original_username + old_retired_username = retirement.retired_username + new_retired_username = user_util.get_retired_username( + original_username, + settings.RETIRED_USER_SALTS, + settings.RETIRED_USERNAME_FMT + ) + # Sanity check: + if retirement.user.username != old_retired_username: + print( + 'WARNING: Skipping UserRetirementStatus ID {} / User ID {} because the user does not appear to ' + 'have a retired username: {} != {}.'.format( + retirement.id, + retirement.user.id, + retirement.user.username, + old_retired_username + ) + ) + # If the original username was already normalized (or all lowercase), the old and new hashes would + # match: + elif old_retired_username == new_retired_username: + print( + 'Skipping UserRetirementStatus ID {} / User ID {} because the hash would not change.'.format( + retirement.id, + retirement.user.id, + ) + ) + # Found an username to update: + else: + print( + 'Updating UserRetirementStatus ID {} / User ID {} ' + 'to rehash their retired username: {} -> {}'.format( + retirement.id, + retirement.user.id, + old_retired_username, + new_retired_username + ) + ) + if not dry_run: + # Update and save both the user table and retirement queue table: + with transaction.atomic(): + retirement.user.username = new_retired_username + retirement.user.save() + retirement.retired_username = new_retired_username + retirement.save() diff --git a/openedx/core/djangoapps/user_api/management/tests/test_bulk_rehash_retired_usernames.py b/openedx/core/djangoapps/user_api/management/tests/test_bulk_rehash_retired_usernames.py new file mode 100644 index 0000000000..90d949db2c --- /dev/null +++ b/openedx/core/djangoapps/user_api/management/tests/test_bulk_rehash_retired_usernames.py @@ -0,0 +1,82 @@ +""" +Test the bulk_rehash_retired_usernames management command +""" +import pytest + +from django.core.management import call_command + +from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import RetirementTestCase, fake_retirement +from openedx.core.djangoapps.user_api.models import UserRetirementStatus +from openedx.core.djangolib.testing.utils import skip_unless_lms +from student.models import get_retired_username_by_username +from student.tests.factories import UserFactory + +pytestmark = pytest.mark.django_db + + +def _setup_users(): + """ + Creates and returns test users in the different states of needing rehash: + - Skipped: has not yet been retired + - Faked: has been fake-retired, but the retired username does not require updating + - Needing rehash: has been fake-retired and name changed so it triggers a hash update + """ + # When we loop through creating users, take additional action on these + user_indexes_to_be_fake_retired = (2, 4, 6, 8, 10) + user_indexes_to_be_rehashed = (4, 6) + + users_skipped = [] + users_faked = [] + users_needing_rehash = [] + retirements = {} + + # Create some test users with retirements + for i in range(1, 11): + user = UserFactory() + retirement = UserRetirementStatus.create_retirement(user) + retirements[user.id] = retirement + + if i in user_indexes_to_be_fake_retired: + fake_retirement(user) + + if i in user_indexes_to_be_rehashed: + # In order to need a rehash user.username must be the same as + # retirement.retired_username and NOT the same as the hash + # generated when the script is run. So we force that here. + retirement.retired_username = retirement.retired_username.upper() + user.username = retirement.retired_username + retirement.save() + user.save() + users_needing_rehash.append(user) + else: + users_faked.append(user) + else: + users_skipped.append(user) + return users_skipped, users_faked, users_needing_rehash, retirements + + +@skip_unless_lms +def test_successful_rehash(capsys): + """ + Run the command with users of all different hash statuses, expect success + """ + RetirementTestCase.setup_states() + users_skipped, users_faked, users_needing_rehash, retirements = _setup_users() + call_command('bulk_rehash_retired_usernames') + output = capsys.readouterr().out + + for user in users_skipped: + assert "User ID {} because the user does not appear to have a retired username:".format(user.id) in output + + for user in users_faked: + assert "User ID {} because the hash would not change.".format(user.id) in output + + for user in users_needing_rehash: + retirement = retirements[user.id] + user.refresh_from_db() + retirement.refresh_from_db() + new_retired_username = get_retired_username_by_username(retirement.original_username) + + assert "User ID {} to rehash their retired username".format(user.id) in output + assert new_retired_username == user.username + assert new_retired_username == retirement.retired_username From 8c93e424c0cdf0e97ec6e966a0ab9d734e784a72 Mon Sep 17 00:00:00 2001 From: bmedx Date: Fri, 10 Aug 2018 14:51:01 -0400 Subject: [PATCH 2/2] Add forums renaming to bulk username rehash command --- .../commands/bulk_rehash_retired_usernames.py | 47 ++++++++++++-- .../test_bulk_rehash_retired_usernames.py | 64 ++++++++++++++++++- 2 files changed, 101 insertions(+), 10 deletions(-) diff --git a/openedx/core/djangoapps/user_api/management/commands/bulk_rehash_retired_usernames.py b/openedx/core/djangoapps/user_api/management/commands/bulk_rehash_retired_usernames.py index 68ec4037b7..cc1ecf1653 100644 --- a/openedx/core/djangoapps/user_api/management/commands/bulk_rehash_retired_usernames.py +++ b/openedx/core/djangoapps/user_api/management/commands/bulk_rehash_retired_usernames.py @@ -12,9 +12,13 @@ command likely cannot be re-used in the future because eventually we will need to clean out the UserRetirementStatus table. """ from __future__ import print_function + from django.conf import settings from django.db import transaction from django.core.management.base import BaseCommand +from six import text_type + +from lms.lib import comment_client from openedx.core.djangoapps.user_api.models import UserRetirementStatus from user_util import user_util @@ -36,6 +40,8 @@ class Command(BaseCommand): """ dry_run = options['dry_run'] retirements = UserRetirementStatus.objects.all().select_related('user') + + failed_retirements = [] for retirement in retirements: original_username = retirement.original_username old_retired_username = retirement.retired_username @@ -64,7 +70,7 @@ class Command(BaseCommand): retirement.user.id, ) ) - # Found an username to update: + # Found an username to update else: print( 'Updating UserRetirementStatus ID {} / User ID {} ' @@ -76,9 +82,36 @@ class Command(BaseCommand): ) ) if not dry_run: - # Update and save both the user table and retirement queue table: - with transaction.atomic(): - retirement.user.username = new_retired_username - retirement.user.save() - retirement.retired_username = new_retired_username - retirement.save() + try: + # Update the forums first, that way if it fails the user can + # be re-run. It does not need to be in the same transaction, + # as the local db updates and can be slow, so keeping it + # outside to cut down on potential deadlocks. + cc_user = comment_client.User.from_django_user(retirement.user) + cc_user.retire(new_retired_username) + + # Update and save both the user table and retirement queue table: + with transaction.atomic(): + retirement.user.username = new_retired_username + retirement.user.save() + retirement.retired_username = new_retired_username + retirement.save() + except Exception as exc: # pylint: disable=broad-except + print( + 'UserRetirementStatus ID {} User ID {} failed rename'.format( + retirement.id, retirement.user.id + ) + ) + print(text_type(exc)) + failed_retirements.append(retirement) + + if failed_retirements: + print('------------------------------------------------------------') + print( + 'FAILED! {} retirements failed to rehash. Retirement IDs:\n{}'.format( + len(failed_retirements), + '\n'.join([text_type(r.id) for r in failed_retirements]) + ) + ) + else: + print('Success! {} retirements examined.'.format(len(retirements))) diff --git a/openedx/core/djangoapps/user_api/management/tests/test_bulk_rehash_retired_usernames.py b/openedx/core/djangoapps/user_api/management/tests/test_bulk_rehash_retired_usernames.py index 90d949db2c..6164a7f25c 100644 --- a/openedx/core/djangoapps/user_api/management/tests/test_bulk_rehash_retired_usernames.py +++ b/openedx/core/djangoapps/user_api/management/tests/test_bulk_rehash_retired_usernames.py @@ -1,14 +1,16 @@ """ Test the bulk_rehash_retired_usernames management command """ +from mock import call, patch import pytest +from django.conf import settings from django.core.management import call_command +from user_util.user_util import get_retired_username from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import RetirementTestCase, fake_retirement from openedx.core.djangoapps.user_api.models import UserRetirementStatus from openedx.core.djangolib.testing.utils import skip_unless_lms -from student.models import get_retired_username_by_username from student.tests.factories import UserFactory pytestmark = pytest.mark.django_db @@ -56,27 +58,83 @@ def _setup_users(): @skip_unless_lms -def test_successful_rehash(capsys): +@patch('lms.lib.comment_client.User.retire') +def test_successful_rehash(retire_user_forums, capsys): """ Run the command with users of all different hash statuses, expect success """ RetirementTestCase.setup_states() users_skipped, users_faked, users_needing_rehash, retirements = _setup_users() + call_command('bulk_rehash_retired_usernames') output = capsys.readouterr().out + # Make sure forums was called the correct number of times + assert retire_user_forums.call_count == 2 + for user in users_skipped: assert "User ID {} because the user does not appear to have a retired username:".format(user.id) in output for user in users_faked: assert "User ID {} because the hash would not change.".format(user.id) in output + expected_username_calls = [] for user in users_needing_rehash: retirement = retirements[user.id] user.refresh_from_db() retirement.refresh_from_db() - new_retired_username = get_retired_username_by_username(retirement.original_username) + new_retired_username = get_retired_username( + retirement.original_username, + settings.RETIRED_USER_SALTS, + settings.RETIRED_USERNAME_FMT + ) + expected_username_calls.append(call(new_retired_username)) assert "User ID {} to rehash their retired username".format(user.id) in output assert new_retired_username == user.username assert new_retired_username == retirement.retired_username + + retire_user_forums.assert_has_calls(expected_username_calls) + + +@skip_unless_lms +@patch('lms.lib.comment_client.User.retire') +def test_forums_failed(retire_user_forums, capsys): + """ + Run the command with users of all different hash statuses, expect success + """ + RetirementTestCase.setup_states() + users_skipped, users_faked, users_needing_rehash, retirements = _setup_users() + retire_user_forums.side_effect = Exception('something bad happened with forums') + + call_command('bulk_rehash_retired_usernames') + output = capsys.readouterr().out + + # Make sure forums was called the correct number of times + assert retire_user_forums.call_count == 2 + + for user in users_skipped: + assert "User ID {} because the user does not appear to have a retired username:".format(user.id) in output + + for user in users_faked: + assert "User ID {} because the hash would not change.".format(user.id) in output + + expected_username_calls = [] + for user in users_needing_rehash: + retirement = retirements[user.id] + user.refresh_from_db() + retirement.refresh_from_db() + new_retired_username = get_retired_username( + retirement.original_username, + settings.RETIRED_USER_SALTS, + settings.RETIRED_USERNAME_FMT + ) + expected_username_calls.append(call(new_retired_username)) + + assert "User ID {} to rehash their retired username".format(user.id) in output + # Confirm that the usernames are *not* updated, due to the forums error + assert new_retired_username != user.username + assert new_retired_username != retirement.retired_username + + assert "FAILED! 2 retirements failed to rehash. Retirement IDs:" in output + retire_user_forums.assert_has_calls(expected_username_calls)