diff --git a/openedx_lti_tool_plugin/models.py b/openedx_lti_tool_plugin/models.py index 2ec10a5..1d750d5 100644 --- a/openedx_lti_tool_plugin/models.py +++ b/openedx_lti_tool_plugin/models.py @@ -3,10 +3,12 @@ import uuid from typing import TypeVar +import shortuuid from django.contrib.auth import get_user_model from django.contrib.auth.models import AbstractBaseUser from django.core.exceptions import ValidationError from django.db import models, transaction +from django.db.models import Q from django.utils.translation import gettext_lazy as _ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey @@ -82,33 +84,92 @@ def __init__(self, *args: tuple, **kwargs: dict): self._initial_pii = self.pii @property - def username(self) -> str: - """str: Username.""" - return f'{app_config.name}.{self.uuid}' + def short_uuid(self) -> str: + """str: Short UUID.""" + return shortuuid.encode(self.uuid)[:8] @property - def email(self) -> str: - """str: Email address.""" - return f'{self.uuid}@{app_config.name}' + def given_name(self) -> str: + """str: Given name.""" + return self.pii.get('given_name', '') + + @property + def middle_name(self) -> str: + """str: Middle name.""" + return self.pii.get('middle_name', '') + + @property + def family_name(self) -> str: + """str: Family name.""" + return self.pii.get('family_name', '') @property def name(self) -> str: """str: Name.""" - name = self.pii.get('name', '') - given_name = self.pii.get('given_name', '') - middle_name = self.pii.get('middle_name', '') - family_name = self.pii.get('family_name', '') + if self.given_name and self.middle_name and self.family_name: + return f'{self.given_name} {self.middle_name} {self.family_name}' - if name: - return name + if self.given_name and self.family_name: + return f'{self.given_name} {self.family_name}' - if given_name and middle_name and family_name: - return f'{given_name} {middle_name} {family_name}' + return self.given_name - if given_name and family_name: - return f'{given_name} {family_name}' + @property + def username(self) -> str: + """str: Username.""" + # Get username from user. + if getattr(self, 'user', None): + return self.user.username + # Generate username string without name. + if not self.given_name: + return f'{self.short_uuid}' + # Generate username string with name. + return ( + f'{self.given_name.lower()[:30]}' + f'{self.family_name.lower()[:1]}' + f'.{self.short_uuid}' + ) - return given_name + @property + def email(self) -> str: + """str: Email address.""" + # Get email from user. + if getattr(self, 'user', None): + return self.user.email + # Generate email string. + return f'{self.uuid}@{app_config.name}' + + def user_collision(self) -> bool: + """Check for user collision. + + Returns: + True if a user collides with another user. + False if no user collision is found. + + """ + return get_user_model().objects.filter( + Q(email=self.email) + | Q(username=self.username) + ).exclude( + email=self.email, + username=self.username, + ).exists() + + def configure_user(self): + """Configure user.""" + # Configure if no user is set. + if getattr(self, 'user', None): + return + # Regenerate uuid on user collision. + while self.user_collision(): + self.uuid = uuid.uuid4() + # Get or create user. + self.user, _created = get_user_model().objects.get_or_create( + email=self.email, + username=self.username, + ) + # Set unusable user password. + self.user.set_unusable_password() @transaction.atomic def save(self, *args: tuple, **kwargs: dict): @@ -121,18 +182,13 @@ def save(self, *args: tuple, **kwargs: dict): """ # Merge initial instance PII data with new PII data. self.pii = {**self._initial_pii, **self.pii} - # Get or create user. - if not getattr(self, 'user', None): - self.user, _created = get_user_model().objects.get_or_create( - username=self.username, - email=self.email, - ) - # Set unusable user password. - self.user.set_unusable_password() + # Configure user. + self.configure_user() # Update or create user profile. user_profile().objects.update_or_create( user=self.user, - defaults={'name': self.name}, + # Truncate name to max_length limit. + defaults={'name': self.name[:255]}, ) return super().save(*args, **kwargs) diff --git a/openedx_lti_tool_plugin/tests/test_models.py b/openedx_lti_tool_plugin/tests/test_models.py index 6ebcf88..4afbe2a 100644 --- a/openedx_lti_tool_plugin/tests/test_models.py +++ b/openedx_lti_tool_plugin/tests/test_models.py @@ -1,4 +1,7 @@ """Test models module.""" +import random +import string +import uuid from unittest.mock import MagicMock, call, patch import ddt @@ -19,6 +22,7 @@ GIVEN_NAME = 'random-given-name' MIDDLE_NAME = 'random-middle-name' FAMILY_NAME = 'random-family-name' +GIVEN_NAME_LARGER = ''.join(random.choices(string.ascii_lowercase, k=300)) @ddt.ddt @@ -29,7 +33,7 @@ def setUp(self): """Set up test fixtures.""" super().setUp() self.pii = {'x': 'x'} - self.new_pii = {'y': 'y'} + self.new_pii = {'y': 'y', 'name': GIVEN_NAME_LARGER} self.lti_profile = LtiProfile.objects.create( platform_id=ISS, client_id=AUD, @@ -42,83 +46,160 @@ def test_class_instance_attributes(self): """Test class instance attributes.""" self.assertEqual(self.lti_profile._initial_pii, self.pii) - @patch(f'{MODULE_PATH}.super') - @patch(f'{MODULE_PATH}.user_profile') + @patch.object(get_user_model().objects, 'filter') + @patch(f'{MODULE_PATH}.Q') + def test_user_collision( + self, + q_mock: MagicMock, + user_filter_mock: MagicMock, + ): + """Test user_collision method.""" + result = self.lti_profile.user_collision() + + q_mock.assert_has_calls([ + call(email=self.lti_profile.email), + call(username=self.lti_profile.username), + ]) + user_filter_mock.assert_called_once_with(q_mock() | q_mock()) + user_filter_mock().exclude.assert_called_once_with( + email=self.lti_profile.email, + username=self.lti_profile.username, + ) + user_filter_mock().exclude().exists.assert_called_once_with() + self.assertEqual(result, user_filter_mock().exclude().exists()) + @patch.object(get_user_model(), 'set_unusable_password') @patch.object(get_user_model().objects, 'get_or_create') - @patch(f'{MODULE_PATH}.getattr', return_value=False) - def test_save_without_user( + @patch(f'{MODULE_PATH}.uuid.uuid4', return_value=uuid.uuid4()) + @patch.object(LtiProfile, 'user_collision') + @patch(f'{MODULE_PATH}.getattr', return_value=None) + def test_configure_user_without_user_with_user_collision( self, getattr_mock: MagicMock, + user_collision_mock: MagicMock, + uuid4_mock: MagicMock, user_get_or_create_mock: MagicMock, user_set_unusable_password_mock: MagicMock, - user_profile_mock: MagicMock, - super_mock: MagicMock, ): - """Test save method without LtiProfile.user attribute value.""" + """Test configure_user method without user with user collision.""" + user_collision_mock.side_effect = [True, False] user_get_or_create_mock.return_value = self.lti_profile.user, None - self.lti_profile.pii = self.new_pii - self.lti_profile.save([], {}) + self.lti_profile.configure_user() - self.assertEqual(self.lti_profile.pii, {**self.pii, **self.new_pii}) - getattr_mock.assert_called_once_with(self.lti_profile, 'user', None) + getattr_mock.assert_has_calls([ + call(self.lti_profile, 'user', None), + call(self.lti_profile, 'user', None), + call(self.lti_profile, 'user', None), + ]) + user_collision_mock.assert_has_calls([call(), call()]) + uuid4_mock.assert_called_once_with() user_get_or_create_mock.assert_called_once_with( - username=self.lti_profile.username, email=self.lti_profile.email, + username=self.lti_profile.username, ) user_set_unusable_password_mock.assert_called_once_with() - user_profile_mock().objects.update_or_create.assert_called_once_with( - user=self.lti_profile.user, - defaults={'name': self.lti_profile.name}, + + @patch.object(get_user_model(), 'set_unusable_password') + @patch.object(get_user_model().objects, 'get_or_create') + @patch(f'{MODULE_PATH}.uuid.uuid4') + @patch.object(LtiProfile, 'user_collision', return_value=False) + @patch(f'{MODULE_PATH}.getattr', return_value=None) + def test_configure_user_without_user_witout_user_collision( + self, + getattr_mock: MagicMock, + user_collision_mock: MagicMock, + uuid4_mock: MagicMock, + user_get_or_create_mock: MagicMock, + user_set_unusable_password_mock: MagicMock, + ): + """Test configure_user method without user without user collision.""" + user_get_or_create_mock.return_value = self.lti_profile.user, None + + self.lti_profile.configure_user() + + getattr_mock.assert_has_calls([ + call(self.lti_profile, 'user', None), + call(self.lti_profile, 'user', None), + call(self.lti_profile, 'user', None), + ]) + user_collision_mock.assert_called_once_with() + uuid4_mock.assert_not_called() + user_get_or_create_mock.assert_called_once_with( + email=self.lti_profile.email, + username=self.lti_profile.username, ) - super_mock().save.assert_called_once_with([], {}) + user_set_unusable_password_mock.assert_called_once_with() - @patch(f'{MODULE_PATH}.super') - @patch(f'{MODULE_PATH}.user_profile') @patch.object(get_user_model(), 'set_unusable_password') @patch.object(get_user_model().objects, 'get_or_create') + @patch.object(get_user_model().objects, 'filter') @patch(f'{MODULE_PATH}.getattr') - def test_save_with_user( + def test_configure_user_with_user( self, getattr_mock: MagicMock, + user_filter_mock: MagicMock, user_get_or_create_mock: MagicMock, user_set_unusable_password_mock: MagicMock, + ): + """Test configure_user method with user.""" + user_get_or_create_mock.return_value = self.lti_profile.user, None + + self.lti_profile.configure_user() + + getattr_mock.assert_called_once_with(self.lti_profile, 'user', None) + user_filter_mock.assert_not_called() + user_filter_mock().exclude.assert_not_called() + user_get_or_create_mock.assert_not_called() + user_set_unusable_password_mock.assert_not_called() + + @patch(f'{MODULE_PATH}.super') + @patch(f'{MODULE_PATH}.user_profile') + @patch.object(LtiProfile, 'configure_user') + def test_save( + self, + configure_user_mock: MagicMock, user_profile_mock: MagicMock, super_mock: MagicMock, ): - """Test save method with LtiProfile.user attribute value.""" - user_get_or_create_mock.return_value = self.lti_profile.user, None + """Test save method.""" self.lti_profile.pii = self.new_pii self.lti_profile.save([], {}) self.assertEqual(self.lti_profile.pii, {**self.pii, **self.new_pii}) - getattr_mock.assert_called_once_with(self.lti_profile, 'user', None) - user_get_or_create_mock.assert_not_called() - user_set_unusable_password_mock.assert_not_called() + configure_user_mock.assert_called_once_with() user_profile_mock().objects.update_or_create.assert_called_once_with( user=self.lti_profile.user, - defaults={'name': self.lti_profile.name}, + defaults={'name': self.lti_profile.name[:255]}, ) super_mock().save.assert_called_once_with([], {}) - def test_username_property(self): - """Test username property.""" - self.assertEqual( - self.lti_profile.username, - f'{app_config.name}.{self.lti_profile.uuid}', - ) + @patch(f'{MODULE_PATH}.shortuuid.encode') + def test_short_uuid_property(self, encode_mock: MagicMock): + """Test short_uuid property.""" + self.assertEqual(self.lti_profile.short_uuid, encode_mock.return_value[:8]) + encode_mock.assert_called_once_with(self.lti_profile.uuid) - def test_email_property(self): - """Test email property.""" - self.assertEqual( - self.lti_profile.email, - f'{self.lti_profile.uuid}@{app_config.name}', - ) + def test_given_name_property(self): + """Test given_name property.""" + self.lti_profile.pii = {'given_name': GIVEN_NAME} + + self.assertEqual(self.lti_profile.given_name, GIVEN_NAME) + + def test_middle_name_property(self): + """Test middle_name property.""" + self.lti_profile.pii = {'middle_name': MIDDLE_NAME} + + self.assertEqual(self.lti_profile.middle_name, MIDDLE_NAME) + + def test_family_name_property(self): + """Test family_name property.""" + self.lti_profile.pii = {'family_name': FAMILY_NAME} + + self.assertEqual(self.lti_profile.family_name, FAMILY_NAME) @ddt.data( - ({'name': NAME}, NAME), ( { 'given_name': GIVEN_NAME, @@ -144,6 +225,45 @@ def test_name_property(self, name_data: dict, name_return: str): self.assertEqual(self.lti_profile.name, name_return) + @ddt.data( + ( + False, + { + 'given_name': GIVEN_NAME_LARGER, + }, + f'{GIVEN_NAME_LARGER.lower()[:30]}.', + ), + ( + False, + { + 'given_name': GIVEN_NAME_LARGER, + 'family_name': FAMILY_NAME, + }, + f'{GIVEN_NAME_LARGER.lower()[:30]}{FAMILY_NAME.lower()[:1]}.', + ), + (False, {}, ''), + (True, {}, ''), + ) + @ddt.unpack + def test_username_property(self, has_user: bool, name_data: dict, name_return: str): + """Test username property.""" + self.lti_profile.pii = name_data + + if not has_user: + self.lti_profile.user = None + + self.assertEqual( + self.lti_profile.username, + f'{name_return}{self.lti_profile.short_uuid}', + ) + + def test_email_property(self): + """Test email property.""" + self.assertEqual( + self.lti_profile.email, + f'{self.lti_profile.uuid}@{app_config.name}', + ) + def test_str_method(self): """Test __str__ method.""" self.assertEqual( diff --git a/requirements/base.in b/requirements/base.in index eb4b3e5..96b1abe 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -7,3 +7,4 @@ pylti1p3 # LTI 1.3 Advantage Tool implementation edx-opaque-keys # Open edX course and XBlock identities API edx-toggles # Library and utilities for implementing and reporting on feature toggles. celery # Asynchronous task execution library +shortuuid # Library that generates concise, unambiguous, URL-safe UUIDs diff --git a/requirements/base.txt b/requirements/base.txt index de901eb..8c5728b 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -168,6 +168,8 @@ requests==2.28.1 # via # -c requirements/constraints.txt # pylti1p3 +shortuuid==1.0.13 + # via -r requirements/base.in six==1.16.0 # via # -c requirements/constraints.txt diff --git a/requirements/test.txt b/requirements/test.txt index 847ae1d..dbcaa95 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -277,6 +277,8 @@ rpds-py==0.10.6 # via # jsonschema # referencing +shortuuid==1.0.13 + # via -r requirements/base.txt six==1.16.0 # via # -c requirements/constraints.txt