Merge pull request #341 from JustFixNYC/random-username

 Generate random usernames by default and use them to identify users in logs.

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!',
             ''
         ]

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()

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'

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,

users/admin.py

@@ -25,15 +25,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_LABEL, {'fields': ('is_active', 'is_staff', 'is_superuser',

users/models.py

@@ -1,88 +1,101 @@
-import logging
-from django.db import models
-from django.contrib.auth.models import AbstractUser
-from django.contrib.auth.models import Permission
-
-from project import twilio
-from project.util.site_util import absolute_reverse
-
-
-PHONE_NUMBER_LEN = 10
-
-FULL_NAME_MAXLEN = 150
-
-VIEW_LETTER_REQUEST_PERMISSION = 'loc.view_letterrequest'
-
-ROLES = {}
-
-ROLES['Outreach Coordinators'] = set([
-    'users.add_justfixuser',
-    'users.change_justfixuser',
-    'legacy_tenants.change_legacyuserinfo',
-    'loc.add_accessdate',
-    'loc.change_accessdate',
-    'loc.delete_accessdate',
-    'loc.add_landlorddetails',
-    'loc.change_landlorddetails',
-    'loc.add_letterrequest',
-    'loc.change_letterrequest',
-    'loc.delete_letterrequest',
-    VIEW_LETTER_REQUEST_PERMISSION,
-    'onboarding.add_onboardinginfo',
-    'onboarding.change_onboardinginfo',
-])
-
-
-logger = logging.getLogger(__name__)
-
-
-def get_permissions_from_ns_codenames(ns_codenames):
-    '''
-    Returns a list of Permission objects for the specified namespaced codenames
-    '''
-
-    splitnames = [ns_codename.split('.') for ns_codename in ns_codenames]
-    return [
-        Permission.objects.get(codename=codename,
-                               content_type__app_label=app_label)
-        for app_label, codename in splitnames
-    ]
-
-
-class JustfixUser(AbstractUser):
-    phone_number = models.CharField(
-        'Phone number',
-        max_length=PHONE_NUMBER_LEN,
-        unique=True,
-        help_text="A U.S. phone number without parentheses or hyphens, e.g. \"5551234567\"."
-    )
-
-    USERNAME_FIELD = 'phone_number'
-    REQUIRED_FIELDS = ['username', 'email']
-
-    @property
-    def full_name(self) -> str:
-        if self.first_name and self.last_name:
-            return ' '.join([self.first_name, self.last_name])
-        return ''
-
-    def formatted_phone_number(self) -> str:
-        if len(self.phone_number) != PHONE_NUMBER_LEN:
-            return self.phone_number
-        area_code = self.phone_number[0:3]
-        first_three_digits = self.phone_number[3:6]
-        last_digits = self.phone_number[6:]
-        return f"({area_code}) {first_three_digits}-{last_digits}"
-
-    def send_sms(self, body: str, fail_silently=True):
-        if hasattr(self, 'onboarding_info') and self.onboarding_info.can_we_sms:
-            twilio.send_sms(self.phone_number, body, fail_silently=fail_silently)
-
-    @property
-    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
+import logging
+from django.db import models
+from django.contrib.auth.models import AbstractUser, UserManager, Permission
+from django.utils.crypto import get_random_string
+
+from project import twilio
+from project.util.site_util import absolute_reverse
+
+
+PHONE_NUMBER_LEN = 10
+
+FULL_NAME_MAXLEN = 150
+
+VIEW_LETTER_REQUEST_PERMISSION = 'loc.view_letterrequest'
+
+ROLES = {}
+
+ROLES['Outreach Coordinators'] = set([
+    'users.add_justfixuser',
+    'users.change_justfixuser',
+    'legacy_tenants.change_legacyuserinfo',
+    'loc.add_accessdate',
+    'loc.change_accessdate',
+    'loc.delete_accessdate',
+    'loc.add_landlorddetails',
+    'loc.change_landlorddetails',
+    'loc.add_letterrequest',
+    'loc.change_letterrequest',
+    'loc.delete_letterrequest',
+    VIEW_LETTER_REQUEST_PERMISSION,
+    'onboarding.add_onboardinginfo',
+    'onboarding.change_onboardinginfo',
+])
+
+
+logger = logging.getLogger(__name__)
+
+
+def get_permissions_from_ns_codenames(ns_codenames):
+    '''
+    Returns a list of Permission objects for the specified namespaced codenames
+    '''
+
+    splitnames = [ns_codename.split('.') for ns_codename in ns_codenames]
+    return [
+        Permission.objects.get(codename=codename,
+                               content_type__app_label=app_label)
+        for app_label, codename in splitnames
+    ]
+
+
+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',
+        max_length=PHONE_NUMBER_LEN,
+        unique=True,
+        help_text="A U.S. phone number without parentheses or hyphens, e.g. \"5551234567\"."
+    )
+
+    objects = JustfixUserManager()
+
+    USERNAME_FIELD = 'phone_number'
+    REQUIRED_FIELDS = ['username', 'email']
+
+    @property
+    def full_name(self) -> str:
+        if self.first_name and self.last_name:
+            return ' '.join([self.first_name, self.last_name])
+        return ''
+
+    def formatted_phone_number(self) -> str:
+        if len(self.phone_number) != PHONE_NUMBER_LEN:
+            return self.phone_number
+        area_code = self.phone_number[0:3]
+        first_three_digits = self.phone_number[3:6]
+        last_digits = self.phone_number[6:]
+        return f"({area_code}) {first_three_digits}-{last_digits}"
+
+    def send_sms(self, body: str, fail_silently=True):
+        if hasattr(self, 'onboarding_info') and self.onboarding_info.can_we_sms:
+            twilio.send_sms(self.phone_number, body, fail_silently=fail_silently)
+
+    @property
+    def admin_url(self):
+        return absolute_reverse('admin:users_justfixuser_change', args=[self.pk])
+
+    def __str__(self):
+        if self.username:
+            return self.username
+        return '<unnamed user>'

users/tests/test_models.py

@@ -1,10 +1,32 @@
+from unittest.mock import patch
 import pytest
 
 from ..models import JustfixUser
 from .factories import UserFactory
 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) == '<unnamed user>'
 
 
 def test_full_name_only_renders_if_both_first_and_last_are_present():

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': "
             "[<Permission: users | user | Can change user>]."
         )
 
@@ -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': "
             "[<Permission: users | user | Can change user>]."
         )
 
@@ -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.")