From 80992e5e6149f2bf1c1c256dc31e5df5fdfd7a06 Mon Sep 17 00:00:00 2001 From: Atul Varma Date: Thu, 25 Oct 2018 07:51:46 -0400 Subject: [PATCH 1/2] Generate random usernames, use them to id users. Fixes #340. --- airtable/tests/test_sync.py | 8 ++++---- legacy_tenants/auth.py | 2 +- legacy_tenants/tests/test_auth.py | 2 +- onboarding/schema.py | 2 +- users/models.py | 22 ++++++++++++++++---- users/tests/test_models.py | 34 +++++++++++++++++++++++++------ users/tests/test_signals.py | 10 ++++----- 7 files changed, 58 insertions(+), 22 deletions(-) diff --git a/airtable/tests/test_sync.py b/airtable/tests/test_sync.py index aa13f6948..a895b1b7c 100644 --- a/airtable/tests/test_sync.py +++ b/airtable/tests/test_sync.py @@ -35,13 +35,13 @@ def resync(): syncer.sync_users(stdout=io) return io.getvalue() - assert resync() == '5551234567 (Boop Jones) does not exist in Airtable, adding them.\n' + assert resync() == 'boop does not exist in Airtable, adding them.\n' assert airtable.get(user.pk).fields_.last_name == 'Jones' - assert resync() == '5551234567 (Boop Jones) is already synced.\n' + assert resync() == 'boop is already synced.\n' user.last_name = 'Denver' user.save() - assert resync() == 'Updating 5551234567 (Boop Denver).\n' + assert resync() == 'Updating boop.\n' assert airtable.get(user.pk).fields_.last_name == 'Denver' @@ -87,7 +87,7 @@ def test_it_works(self, settings): call_command('syncairtable', stdout=io) assert io.getvalue().split('\n') == [ 'Retrieving current Airtable...', - '5551234567 (Boop Jones) does not exist in Airtable, adding them.', + 'boop does not exist in Airtable, adding them.', 'Finished synchronizing with Airtable!', '' ] diff --git a/legacy_tenants/auth.py b/legacy_tenants/auth.py index 19b814018..aaae5c360 100644 --- a/legacy_tenants/auth.py +++ b/legacy_tenants/auth.py @@ -81,7 +81,7 @@ def authenticate(self, request, phone_number: Optional[str]=None, user = JustfixUser.objects.get(phone_number=phone_number) except JustfixUser.DoesNotExist: user = JustfixUser( - username=f"legacy_{phone_number}", + username=JustfixUser.objects.generate_random_username('legacy_'), phone_number=phone_number ) user.save() diff --git a/legacy_tenants/tests/test_auth.py b/legacy_tenants/tests/test_auth.py index efc073ee7..8ac5d10d3 100644 --- a/legacy_tenants/tests/test_auth.py +++ b/legacy_tenants/tests/test_auth.py @@ -51,7 +51,7 @@ def test_backend_creates_tenant_user_if_they_do_not_exist(settings): user = backend.authenticate(None, '1234567890', 'password') get_user.assert_called_with('1234567890') assert isinstance(user, JustfixUser) - assert user.username == 'legacy_1234567890' + assert user.username.startswith('legacy_') assert user.legacy_info.role == 'TENANT' assert user.first_name == 'Testy' assert user.last_name == 'Test' diff --git a/onboarding/schema.py b/onboarding/schema.py index 7a3d36cee..05cd13c3a 100644 --- a/onboarding/schema.py +++ b/onboarding/schema.py @@ -133,7 +133,7 @@ def perform_mutate(cls, form: forms.OnboardingStep4Form, info: ResolveInfo): cls.log(info, "User has not completed previous steps, aborting mutation.") return cls.make_error("You haven't completed all the previous steps yet.") user = JustfixUser.objects.create_user( - username=phone_number, + username=JustfixUser.objects.generate_random_username(), first_name=prev_steps['first_name'], last_name=prev_steps['last_name'], phone_number=phone_number, diff --git a/users/models.py b/users/models.py index 97993aed1..cde36b97b 100644 --- a/users/models.py +++ b/users/models.py @@ -1,6 +1,7 @@ import logging from django.db import models -from django.contrib.auth.models import AbstractUser +from django.contrib.auth.models import AbstractUser, UserManager +from django.utils.crypto import get_random_string from project import twilio from project.util.site_util import absolute_reverse @@ -14,6 +15,17 @@ logger = logging.getLogger(__name__) +class JustfixUserManager(UserManager): + def generate_random_username(self, prefix='') -> str: + while True: + username = prefix + get_random_string( + length=12, + allowed_chars='abcdefghijklmnopqrstuvwxyz' + ) + if not self.filter(username=username).exists(): + return username + + class JustfixUser(AbstractUser): phone_number = models.CharField( 'Phone number', @@ -22,6 +34,8 @@ class JustfixUser(AbstractUser): help_text="A U.S. phone number without parentheses or hyphens, e.g. \"5551234567\"." ) + objects = JustfixUserManager() + USERNAME_FIELD = 'phone_number' REQUIRED_FIELDS = ['username', 'email'] @@ -48,6 +62,6 @@ def admin_url(self): return absolute_reverse('admin:users_justfixuser_change', args=[self.pk]) def __str__(self): - if self.full_name: - return f"{self.phone_number} ({self.full_name})" - return self.phone_number + if self.username: + return self.username + return '' diff --git a/users/tests/test_models.py b/users/tests/test_models.py index b84f43869..55c10bee6 100644 --- a/users/tests/test_models.py +++ b/users/tests/test_models.py @@ -1,3 +1,4 @@ +from unittest.mock import patch import pytest from ..models import JustfixUser @@ -5,6 +6,27 @@ from onboarding.tests.factories import OnboardingInfoFactory +@pytest.mark.django_db +class TestGenerateRandomUsername: + def generate(self, prefix='', **kwargs): + user = JustfixUser.objects.create_user( + username=JustfixUser.objects.generate_random_username(prefix=prefix), + **kwargs + ) + return user + + def test_it_applies_a_prefix_if_provided(self): + with patch('users.models.get_random_string', side_effect=['boop']): + assert self.generate(prefix='bleh_').username == 'bleh_boop' + + def test_it_retries_until_a_unique_one_is_found(self): + with patch('users.models.get_random_string', side_effect=['boop', 'boop', 'blap']): + user = self.generate(phone_number='1234567890') + assert user.username == 'boop' + user2 = self.generate(phone_number='1234567891') + assert user2.username == 'blap' + + def test_formatted_phone_number_works(): assert JustfixUser().formatted_phone_number() == '' @@ -21,14 +43,14 @@ def test_admin_url_works(): assert user.admin_url == f'https://example.com/admin/users/justfixuser/{user.pk}/change/' -def test_str_works_when_only_phone_number_is_available(): - user = JustfixUser(phone_number='5551234567') - assert str(user) == '5551234567' +def test_str_works_when_username_is_available(): + user = JustfixUser(username='boop') + assert str(user) == 'boop' -def test_str_works_when_full_name_is_available(): - user = UserFactory.build(phone_number='5551234567', full_name='boop jones') - assert str(user) == '5551234567 (boop jones)' +def test_str_works_when_username_is_unavailable(): + user = JustfixUser() + assert str(user) == '' def test_full_name_only_renders_if_both_first_and_last_are_present(): diff --git a/users/tests/test_signals.py b/users/tests/test_signals.py index 908850f2e..f9d595eaf 100644 --- a/users/tests/test_signals.py +++ b/users/tests/test_signals.py @@ -20,7 +20,7 @@ def test_permission_change_is_logged(): with patch('users.signals.logger') as mock: create_user_with_perm() mock.info.assert_called_once_with( - "permissions given to user '5551234567 (Boop Jones)': " + "permissions given to user 'boop': " "[]." ) @@ -31,7 +31,7 @@ def test_permission_remove_is_logged(): with patch('users.signals.logger') as mock: user.user_permissions.remove(perm) mock.info.assert_called_once_with( - "permissions removed from user '5551234567 (Boop Jones)': " + "permissions removed from user 'boop': " "[]." ) @@ -42,7 +42,7 @@ def test_permission_clear_is_logged(): with patch('users.signals.logger') as mock: user.user_permissions.clear() mock.info.assert_called_once_with( - "All permissions removed from user '5551234567 (Boop Jones)'." + "All permissions removed from user 'boop'." ) @@ -77,7 +77,7 @@ def test_logentry_change_is_logged(): def test_login_is_logged(client): with patch('users.signals.logger') as mock: client.force_login(UserFactory()) - mock.info.assert_called_once_with("5551234567 (Boop Jones) logged in.") + mock.info.assert_called_once_with("boop logged in.") @pytest.mark.django_db @@ -95,4 +95,4 @@ def test_logout_is_logged(client): client.force_login(UserFactory()) with patch('users.signals.logger') as mock: client.logout() - mock.info.assert_called_once_with("5551234567 (Boop Jones) logged out.") + mock.info.assert_called_once_with("boop logged out.") From bf25e81c3a3486690cb7a19d096c50fbdf466606 Mon Sep 17 00:00:00 2001 From: Atul Varma Date: Thu, 25 Oct 2018 07:59:11 -0400 Subject: [PATCH 2/2] Show username in admin list view. --- users/admin.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/users/admin.py b/users/admin.py index 42d7bf858..875c2bf88 100644 --- a/users/admin.py +++ b/users/admin.py @@ -23,15 +23,18 @@ class JustfixUserAdmin(UserAdmin): add_form = JustfixUserCreationForm form = JustfixUserChangeForm model = JustfixUser - list_display = ['phone_number', 'first_name', 'last_name', 'issue_count', 'mailing_needed'] + list_display = [ + 'username', 'phone_number', 'first_name', 'last_name', 'issue_count', 'mailing_needed'] fieldsets = ( (_('Personal info'), {'fields': ('first_name', 'last_name', 'email', 'phone_number')}), ('Username and password', { 'fields': ('username', 'password'), 'description': ( - "Note that the username is largely useless, and is an artifact of Django. " - "We don't really use it anywhere, but nonetheless, it must exist, and it " - "must be unique." + "Note that the username is never visible to users, but it is used to " + "identify users in server logs. Therefore, it doesn't need to be " + "very human-friendly, and ideally it should be devoid of any " + "personally identifiable information such as a user's real name " + "or phone number." ) }), (_('Permissions'), {'fields': ('is_active', 'is_staff', 'is_superuser',