From a8b79740e96e23fb698611616318afa5b763a1a4 Mon Sep 17 00:00:00 2001 From: lebaudantoine Date: Tue, 2 Jun 2026 11:34:22 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B(backend)=20prevent=20duplicate=20p?= =?UTF-8?q?ending=20users=20on=20concurrent=20requests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a race condition in the external viewset where concurrent requests could create multiple pending users with the same email address. No database constraint enforced email uniqueness for pending users, allowing duplicates to be created under load. This caused issues during user reconciliation, which expects a single matching pending user and raises a warning when multiple records are found. Add a narrowly scoped migration to enforce the uniqueness constraint and address the identified issue. --- CHANGELOG.md | 4 ++ ...0019_user_unique_email_when_sub_is_null.py | 19 +++++++ src/backend/core/models.py | 7 +++ .../tests/authentication/test_backends.py | 7 ++- src/backend/core/tests/test_models_users.py | 50 +++++++++++++++++++ 5 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 src/backend/core/migrations/0019_user_unique_email_when_sub_is_null.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e8b98ee..0d801a02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ and this project adheres to - 🔧(backend) backport logging configuration from docs +### Fixed + +- 🐛(backend) prevent duplicate pending users on concurrent requests + ## [1.17.0] - 2026-05-31 ### Added diff --git a/src/backend/core/migrations/0019_user_unique_email_when_sub_is_null.py b/src/backend/core/migrations/0019_user_unique_email_when_sub_is_null.py new file mode 100644 index 00000000..154128fe --- /dev/null +++ b/src/backend/core/migrations/0019_user_unique_email_when_sub_is_null.py @@ -0,0 +1,19 @@ +# Generated by Django 5.2.14 on 2026-06-02 17:31 + +import django.db.models.functions.text +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('auth', '0012_alter_user_first_name_max_length'), + ('core', '0018_rename_active_application_is_active'), + ] + + operations = [ + migrations.AddConstraint( + model_name='user', + constraint=models.UniqueConstraint(django.db.models.functions.text.Lower('email'), condition=models.Q(('sub__isnull', True)), name='unique_email_when_sub_is_null'), + ), + ] diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 45fb271f..32d17db8 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -211,6 +211,13 @@ class User(AbstractBaseUser, BaseModel, auth_models.PermissionsMixin): ordering = ("-created_at",) verbose_name = _("user") verbose_name_plural = _("users") + constraints = [ + models.UniqueConstraint( + models.functions.Lower("email"), + condition=models.Q(sub__isnull=True), + name="unique_email_when_sub_is_null", + ) + ] def __str__(self): return self.email or self.admin_email or str(self.id) diff --git a/src/backend/core/tests/authentication/test_backends.py b/src/backend/core/tests/authentication/test_backends.py index 4e6d649d..2f707120 100644 --- a/src/backend/core/tests/authentication/test_backends.py +++ b/src/backend/core/tests/authentication/test_backends.py @@ -346,8 +346,11 @@ def test_authentication_getter_existing_user_change_fields( # One and only one additional update query when a field has changed # Note: .save() triggers uniqueness validation queries for unique fields, - # adding extra SELECT queries before the UPDATE (e.g., checking unique=True on 'sub') - with django_assert_num_queries(3): + # adding extra SELECT queries before the UPDATE: + # - unique=True on 'sub' + # - unique=True on 'admin_email' + # - partial unique index 'unique_email_when_sub_is_null' + with django_assert_num_queries(5): authenticated_user = klass.get_or_create_user( access_token="test-token", id_token=None, payload=None ) diff --git a/src/backend/core/tests/test_models_users.py b/src/backend/core/tests/test_models_users.py index edea5bb9..cb23cecd 100644 --- a/src/backend/core/tests/test_models_users.py +++ b/src/backend/core/tests/test_models_users.py @@ -44,3 +44,53 @@ def test_models_users_send_mail_main_missing(): user.email_user("my subject", "my message") assert str(excinfo.value) == "User has no email address." + + +def test_models_users_email_unique_when_sub_is_null(): + """Email should be unique among users with no sub (pending users).""" + user = factories.UserFactory(sub=None, email="test@example.com") + with pytest.raises( + ValidationError, match="Constraint “unique_email_when_sub_is_null” is violated." + ): + factories.UserFactory(sub=None, email=user.email) + + +def test_models_users_email_unique_case_insensitive_when_sub_is_null(): + """Email uniqueness should be case-insensitive among users with no sub (pending users).""" + factories.UserFactory(sub=None, email="Test@example.com") + with pytest.raises( + ValidationError, match="Constraint “unique_email_when_sub_is_null” is violated." + ): + factories.UserFactory(sub=None, email="test@example.com") + + +def test_models_users_email_not_unique_when_sub_is_set(): + """Email uniqueness should not be enforced when users have a sub.""" + user = factories.UserFactory(sub="sub-1", email="test@example.com") + user2 = factories.UserFactory(sub="sub-2", email=user.email) + assert user2.email == user.email + + +def test_models_users_email_not_unique_between_sub_null_and_sub_set(): + """A user with a sub and a pending user (sub=None) can share the same email.""" + user = factories.UserFactory(sub="sub-1", email="test@example.com") + user2 = factories.UserFactory(sub=None, email=user.email) + assert user2.email == user.email + + +def test_models_users_email_unique_constraint_allows_multiple_null_emails(): + """Multiple users with sub=None and email=None should be allowed.""" + factories.UserFactory(sub=None, email=None) + factories.UserFactory(sub=None, email=None) + + +def test_models_users_sub_null_email_null_does_not_prevent_creation(): + """Multiple pending users (sub=None, email=None) can be created without conflict. + + sub=None is not unique-constrained. email uniqueness is only enforced among + sub=None users with a non-null email, so email=None bypasses it (NULL != NULL in SQL). + """ + # Ghost row can still appear from bad code path + u1 = factories.UserFactory(sub=None, email=None) + u2 = factories.UserFactory(sub=None, email=None) + assert u1.pk != u2.pk