From 1c8f5e30e9ebd3a0d1434bdcc8febe258b4b7a0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20Paccoud=20-=20France=20Universit=C3=A9=20Num?= =?UTF-8?q?=C3=A9rique?= Date: Mon, 6 Feb 2023 12:47:05 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B(users)=20allow=20several=20users?= =?UTF-8?q?=20with=20empty=20email?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We allowed empty emails but it breaks the unique constraint if we don't also allow several users with empty email. This commit also takes the opportunity to block user creation from a logged-in user as it does not seem legitimate. --- .../core/migrations/0002_alter_user_email.py | 18 +++ src/magnify/apps/core/models.py | 2 +- src/magnify/apps/core/permissions.py | 5 +- tests/apps/core/test_core_api_users.py | 147 +++++++++--------- tests/apps/core/test_core_models_users.py | 50 +++++- 5 files changed, 139 insertions(+), 83 deletions(-) create mode 100644 src/magnify/apps/core/migrations/0002_alter_user_email.py diff --git a/src/magnify/apps/core/migrations/0002_alter_user_email.py b/src/magnify/apps/core/migrations/0002_alter_user_email.py new file mode 100644 index 000000000..bbb5d873a --- /dev/null +++ b/src/magnify/apps/core/migrations/0002_alter_user_email.py @@ -0,0 +1,18 @@ +# Generated by Django 4.1.6 on 2023-02-04 17:15 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("core", "0001_initial"), + ] + + operations = [ + migrations.AlterField( + model_name="user", + name="email", + field=models.EmailField(blank=True, max_length=255, null=True), + ), + ] diff --git a/src/magnify/apps/core/models.py b/src/magnify/apps/core/models.py index 3b1c4c70d..56d555b92 100644 --- a/src/magnify/apps/core/models.py +++ b/src/magnify/apps/core/models.py @@ -92,7 +92,7 @@ class User(BaseModel, AbstractBaseUser, PermissionsMixin): blank=True, null=True, ) - email = models.EmailField(max_length=255, unique=True, blank=True, null=True) + email = models.EmailField(max_length=255, blank=True, null=True) language = models.CharField( max_length=10, choices=lazy(lambda: settings.LANGUAGES, tuple)(), diff --git a/src/magnify/apps/core/permissions.py b/src/magnify/apps/core/permissions.py index b45b01d56..ab49beb5c 100644 --- a/src/magnify/apps/core/permissions.py +++ b/src/magnify/apps/core/permissions.py @@ -40,7 +40,10 @@ def has_permission(self, request, view): of its administrator. """ if view.action == "create": - return getattr(settings, "ALLOW_API_USER_CREATE", False) + return ( + getattr(settings, "ALLOW_API_USER_CREATE", False) + and not request.user.is_authenticated + ) if view.action in ["list", "retrieve"]: return request.user.is_authenticated diff --git a/tests/apps/core/test_core_api_users.py b/tests/apps/core/test_core_api_users.py index 08cf3ccfe..5440e071d 100644 --- a/tests/apps/core/test_core_api_users.py +++ b/tests/apps/core/test_core_api_users.py @@ -358,36 +358,35 @@ def test_api_users_create_anonymous_successful(self): }, ) - def test_api_users_create_authenticated_forbidden(self): + @override_settings(ALLOW_API_USER_CREATE=True) + def test_api_users_create_anonymous_existing_username(self): """ - Authenticated users should not be able to create users via the API if not allowed. + Trying to create a user with a username that already exists should receive a 400 error. """ user = UserFactory() - jwt_token = AccessToken.for_user(user) - with mock.patch( - "magnify.apps.core.utils.get_tokens_for_user", return_value=MOCK_TOKENS - ): - response = self.client.post( - "/api/users/", - { - "email": "thomas.jeffersion@example.com", - "language": "fr", - "name": "Thomas Jefferson", - "username": "thomas", - "password": "mypassword", - }, - HTTP_AUTHORIZATION=f"Bearer {jwt_token}", - ) - self.assertEqual(response.status_code, 403) - self.assertEqual(User.objects.count(), 1) + response = self.client.post( + "/api/users/", + { + "email": "thomas.jeffersion@example.com", + "language": "fr", + "name": "Thomas Jefferson", + "username": user.username, + "password": "mypassword", + }, + ) + + self.assertEqual(response.status_code, 400) + self.assertEqual( + response.json(), {"username": ["A user with that username already exists."]} + ) @override_settings(ALLOW_API_USER_CREATE=True) - def test_api_users_create_authenticated_successful(self): - """Authenticated users should be able to create users.""" + def test_api_users_create_anonymous_existing_email(self): + """ + It should be possible to create a user with an email that already exists. + """ user = UserFactory() - jwt_token = AccessToken.for_user(user) - is_device = random.choice([True, False]) with mock.patch( "magnify.apps.core.utils.get_tokens_for_user", return_value=MOCK_TOKENS @@ -395,33 +394,32 @@ def test_api_users_create_authenticated_successful(self): response = self.client.post( "/api/users/", { - "email": "thomas.jeffersion@example.com", - "is_device": is_device, + "email": user.email, "language": "fr", "name": "Thomas Jefferson", "username": "thomas", "password": "mypassword", }, - HTTP_AUTHORIZATION=f"Bearer {jwt_token}", ) + self.assertEqual(response.status_code, 201) - self.assertEqual(User.objects.count(), 2) - user = User.objects.get(username="thomas") - self.assertEqual(user.email, "thomas.jeffersion@example.com") - self.assertEqual(user.name, "Thomas Jefferson") - self.assertEqual(user.language, "fr") - self.assertEqual(user.is_device, is_device) + self.assertEqual(User.objects.filter(email=user.email).count(), 2) + new_user = User.objects.get(username="thomas") + self.assertEqual(new_user.email, user.email) + self.assertEqual(new_user.name, "Thomas Jefferson") + self.assertEqual(new_user.language, "fr") + self.assertFalse(new_user.is_device) - self.assertIn("pbkdf2_sha256", user.password) - self.assertTrue(check_password("mypassword", user.password)) + self.assertIn("pbkdf2_sha256", new_user.password) + self.assertTrue(check_password("mypassword", new_user.password)) self.assertEqual( response.json(), { - "id": str(user.id), - "email": "thomas.jeffersion@example.com", - "is_device": is_device, + "id": str(new_user.id), + "email": user.email, + "is_device": False, "language": "fr", "name": "Thomas Jefferson", "username": "thomas", @@ -429,57 +427,54 @@ def test_api_users_create_authenticated_successful(self): }, ) - @override_settings(ALLOW_API_USER_CREATE=True) - def test_api_users_create_authenticated_existing_username(self): + def test_api_users_create_authenticated_settings_forbidden(self): """ - A user trying to create a user with a username that already exists - should receive a 400 error. + Authenticated users should not be able to create users via the API if settings allow. """ user = UserFactory() jwt_token = AccessToken.for_user(user) - response = self.client.post( - "/api/users/", - { - "email": "thomas.jeffersion@example.com", - "language": "fr", - "name": "Thomas Jefferson", - "username": user.username, - "password": "mypassword", - }, - HTTP_AUTHORIZATION=f"Bearer {jwt_token}", - ) - - self.assertEqual(response.status_code, 400) - self.assertEqual( - response.json(), {"username": ["A user with that username already exists."]} - ) + with mock.patch( + "magnify.apps.core.utils.get_tokens_for_user", return_value=MOCK_TOKENS + ): + response = self.client.post( + "/api/users/", + { + "email": "thomas.jeffersion@example.com", + "language": "fr", + "name": "Thomas Jefferson", + "username": "thomas", + "password": "mypassword", + }, + HTTP_AUTHORIZATION=f"Bearer {jwt_token}", + ) + self.assertEqual(response.status_code, 403) + self.assertEqual(User.objects.count(), 1) @override_settings(ALLOW_API_USER_CREATE=True) - def test_api_users_create_authenticated_existing_email(self): + def test_api_users_create_authenticated_settings_authorized(self): """ - A user trying to create a user with an email that already exists - should receive a 400 error. + Authenticated users should not be able to create users via the API even settings allow. """ user = UserFactory() jwt_token = AccessToken.for_user(user) - response = self.client.post( - "/api/users/", - { - "email": user.email, - "language": "fr", - "name": "Thomas Jefferson", - "username": "thomas", - "password": "mypassword", - }, - HTTP_AUTHORIZATION=f"Bearer {jwt_token}", - ) - - self.assertEqual(response.status_code, 400) - self.assertEqual( - response.json(), {"email": ["User with this email already exists."]} - ) + with mock.patch( + "magnify.apps.core.utils.get_tokens_for_user", return_value=MOCK_TOKENS + ): + response = self.client.post( + "/api/users/", + { + "email": "thomas.jeffersion@example.com", + "language": "fr", + "name": "Thomas Jefferson", + "username": "thomas", + "password": "mypassword", + }, + HTTP_AUTHORIZATION=f"Bearer {jwt_token}", + ) + self.assertEqual(response.status_code, 403) + self.assertEqual(User.objects.count(), 1) def test_api_users_update_authenticated_self(self): """ diff --git a/tests/apps/core/test_core_models_users.py b/tests/apps/core/test_core_models_users.py index c1bd75e9b..3a2668b9c 100644 --- a/tests/apps/core/test_core_models_users.py +++ b/tests/apps/core/test_core_models_users.py @@ -18,12 +18,36 @@ def test_models_users_str(self): user = UserFactory() self.assertEqual(str(user), user.username) - def test_models_users_email_normalization(self): - """The email field should be automatically normalized upon saving.""" + def test_models_users_username_null(self): + """The "username" field should not be null.""" + with self.assertRaises(ValidationError) as context: + UserFactory(username=None) + + self.assertEqual( + context.exception.messages, + ["This field cannot be null."], + ) + + def test_models_users_username_empty(self): + """The "username" field should not be empty.""" + with self.assertRaises(ValidationError) as context: + UserFactory(username="") + + self.assertEqual( + context.exception.messages, + ["This field cannot be blank."], + ) + + def test_models_users_username_unique(self): + """The "username" field should be unique.""" user = UserFactory() - user.email = "Thomas.Jefferson@Example.com" - user.save() - self.assertEqual(user.email, "Thomas.Jefferson@example.com") + with self.assertRaises(ValidationError) as context: + UserFactory(username=user.username) + + self.assertEqual( + context.exception.messages, + ["A user with that username already exists."], + ) def test_models_users_username_max_length(self): """The username field should be 30 characters maximum.""" @@ -72,6 +96,22 @@ def test_models_users_username_ascii(self): ], ) + def test_models_users_email_empty(self): + """The "email" field can be empty.""" + UserFactory(email="") + + def test_models_users_email_unique(self): + """The "email" field is not unique.""" + user = UserFactory() + UserFactory(email=user.email) + + def test_models_users_email_normalization(self): + """The email field should be automatically normalized upon saving.""" + user = UserFactory() + user.email = "Thomas.Jefferson@Example.com" + user.save() + self.assertEqual(user.email, "Thomas.Jefferson@example.com") + def test_models_users_ordering(self): """Users should be returned ordered by their username.""" UserFactory.create_batch(3)