From 3554b2eb53023c51e627194eef78a5401bec5fe6 Mon Sep 17 00:00:00 2001 From: lebaudantoine Date: Tue, 2 Jun 2026 16:20:40 +0200 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F(backend)=20defend=20user=20p?= =?UTF-8?q?rovisioning=20against=20race=20condition?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move user provisioning logic out of the external token viewset into a dedicated service to keep the viewset lightweight and easier to maintain. While extracting the logic, refactor user object handling to improve robustness and make the provisioning workflow easier to reason about. Defend against race conditions when concurrent requests attempt to provision the same user. Rely on the existing database constraints to guarantee uniqueness and gracefully handle integrity errors raised by concurrent creations. --- src/backend/core/external_api/viewsets.py | 49 +++----- .../core/services/provisional_user_service.py | 110 ++++++++++++++++++ .../core/tests/test_external_api_token.py | 88 ++++++++++++++ 3 files changed, 212 insertions(+), 35 deletions(-) create mode 100644 src/backend/core/services/provisional_user_service.py diff --git a/src/backend/core/external_api/viewsets.py b/src/backend/core/external_api/viewsets.py index a96fd283..07ecf4f8 100644 --- a/src/backend/core/external_api/viewsets.py +++ b/src/backend/core/external_api/viewsets.py @@ -4,7 +4,7 @@ from logging import getLogger from django.conf import settings from django.contrib.auth.hashers import check_password -from django.core.exceptions import SuspiciousOperation, ValidationError +from django.core.exceptions import ValidationError from django.core.validators import validate_email from lasuite.oidc_resource_server.authentication import ResourceServerAuthentication @@ -23,6 +23,11 @@ from core import api, models from core.api.feature_flag import FeatureFlag from core.services.jwt_token import JwtTokenService +from ..services.provisional_user_service import ( + ProvisionalUserCreationDisabledError, + ProvisionalUserIntegrityError, + ProvisionalUserService, +) from . import authentication, permissions, serializers logger = getLogger(__name__) @@ -94,40 +99,14 @@ class ApplicationViewSet(viewsets.ViewSet): ) try: - user = models.User.objects.get(email__iexact=email) - except models.User.DoesNotExist as e: - if ( - settings.APPLICATION_ALLOW_USER_CREATION - and settings.OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION - and not settings.OIDC_USER_SUB_FIELD_IMMUTABLE - ): - # Create a provisional user without `sub`, identified by email only. - # - # This relies on Django LaSuite implicitly updating the `sub` field on the - # user's first successful OIDC authentication. If this stops working, - # check for behavior changes in Django LaSuite. - # - # `OIDC_USER_SUB_FIELD_IMMUTABLE` comes from Django LaSuite and prevents `sub` - # updates. We override its default value to allow setting `sub` for - # provisional users. - user = models.User( - sub=None, - email=email, - ) - user.set_unusable_password() - user.save() - logger.info( - "Provisional user created via application: user_id=%s, email=%s, client_id=%s", - user.id, - email, - application.client_id, - ) - else: - raise drf_exceptions.NotFound("User not found.") from e - except models.User.MultipleObjectsReturned as e: - raise SuspiciousOperation( - "Multiple user accounts share a common email." - ) from e + user, _ = ProvisionalUserService().get_or_create(email, client_id) + except ProvisionalUserCreationDisabledError as not_found_error: + raise drf_exceptions.NotFound("User not found.") from not_found_error + except ProvisionalUserIntegrityError: + return drf_response.Response( + {"error": "Failed to create or retrieve provisional user."}, + status=drf_status.HTTP_409_CONFLICT, + ) scope = " ".join(application.scopes or []) diff --git a/src/backend/core/services/provisional_user_service.py b/src/backend/core/services/provisional_user_service.py new file mode 100644 index 00000000..dbe5e4f3 --- /dev/null +++ b/src/backend/core/services/provisional_user_service.py @@ -0,0 +1,110 @@ +"""Service for provisional user creation.""" + +import logging + +from django.conf import settings +from django.core.exceptions import SuspiciousOperation, ValidationError +from django.db import IntegrityError + +from core import models + +logger = logging.getLogger(__name__) + + +class ProvisionalUserError(Exception): + """Base exception for provisional user service errors.""" + + +class ProvisionalUserCreationDisabledError(ProvisionalUserError): + """Raised when provisional user creation is disabled by configuration.""" + + +class ProvisionalUserIntegrityError(ProvisionalUserError): + """Raised when a provisional user cannot be created or retrieved after a race condition.""" + + +class ProvisionalUserService: + """Handles creation and retrieval of provisional users. + + A provisional user is created without a `sub`, identified by email only. + The `sub` is set on first successful OIDC authentication via Django LaSuite. + """ + + def __init__(self): + """Initialize the service.""" + + # `OIDC_USER_SUB_FIELD_IMMUTABLE` comes from Django LaSuite and prevents `sub` + # updates. We override its default value to allow setting `sub` for + # provisional users. + + self._is_creation_enabled = ( + settings.APPLICATION_ALLOW_USER_CREATION + and settings.OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION + and not settings.OIDC_USER_SUB_FIELD_IMMUTABLE + ) + + def _get_by_email(self, email: str) -> models.User | None: + """Return the user with this email, or None if not found.""" + try: + return models.User.objects.get(email__iexact=email) + except models.User.DoesNotExist: + return None + except models.User.MultipleObjectsReturned as e: + raise SuspiciousOperation( + "Multiple user accounts share a common email." + ) from e + + def get_or_create( + self, email: str, client_id: str + ) -> tuple[models.User | None, bool]: + """Get or create a provisional user identified by email. + + Args: + email: The email address to identify the user. + client_id: The application client_id, used for audit logging only. + + Returns: + A (user, created) tuple — mirrors get_or_create conventions. + + Raises: + ProvisionalUserError: If creation and retrieval both fail. + """ + user = self._get_by_email(email) + if user: + return user, False + + if not self._is_creation_enabled: + raise ProvisionalUserCreationDisabledError( + "Provisional user creation is disabled by configuration." + ) + + # Create a provisional user without `sub`, identified by email only. + # This relies on Django LaSuite implicitly updating the `sub` field on the + # user's first successful OIDC authentication. If this stops working, + # check for behavior changes in Django LaSuite. + try: + user = models.User(sub=None, email=email) + user.set_unusable_password() + user.save() + logger.info( + "Provisional user created via application: user_id=%s, email=%s, client_id=%s", + user.id, + email, + client_id, + ) + return user, True + except (IntegrityError, ValidationError) as e: + logger.warning( + "Race condition on provisional user creation, fetching existing: " + "email=%s, client_id=%s", + email, + client_id, + ) + user = self._get_by_email(email) + + if user: + return user, False + + raise ProvisionalUserIntegrityError( + "Failed to create or retrieve provisional user." + ) from e diff --git a/src/backend/core/tests/test_external_api_token.py b/src/backend/core/tests/test_external_api_token.py index 9a8683ae..6c13a884 100644 --- a/src/backend/core/tests/test_external_api_token.py +++ b/src/backend/core/tests/test_external_api_token.py @@ -4,6 +4,8 @@ Tests for external API /token endpoint # pylint: disable=W0621 +from unittest import mock + import jwt import pytest from freezegun import freeze_time @@ -15,6 +17,7 @@ from core.factories import ( UserFactory, ) from core.models import ApplicationScope, User +from core.services import provisional_user_service pytestmark = pytest.mark.django_db @@ -436,3 +439,88 @@ def test_api_applications_token_existing_user(settings): "delegated": True, "scope": "rooms:list rooms:create", } + + +@mock.patch.object(provisional_user_service.ProvisionalUserService, "_get_by_email") +def test_api_applications_token_new_user_race_condition(mock_get_by_email, settings): + """Should handle race condition where two concurrent requests create the same user.""" + settings.APPLICATION_ALLOW_USER_CREATION = True + settings.OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION = True + settings.OIDC_USER_SUB_FIELD_IMMUTABLE = False + + application = ApplicationFactory( + is_active=True, scopes=[ApplicationScope.ROOMS_LIST] + ) + plain_secret = "test-secret-123" + application.client_secret = plain_secret + application.save() + + email = "john.doe@example.com" + + # First call: lie and say user doesn't exist, simulating the race window + # Second call (recovery path): return the real user + existing_user = UserFactory(sub=None, email=email) + mock_get_by_email.side_effect = [None, existing_user] + + client = APIClient() + response = client.post( + "/external-api/v1.0/application/token/", + { + "client_id": application.client_id, + "client_secret": plain_secret, + "grant_type": "client_credentials", + "scope": email, + }, + format="json", + ) + + assert response.status_code == 200 + assert mock_get_by_email.call_count == 2 + + token = response.data["access_token"] + payload = jwt.decode( + token, + settings.APPLICATION_JWT_SECRET_KEY, + algorithms=[settings.APPLICATION_JWT_ALG], + issuer=settings.APPLICATION_JWT_ISSUER, + audience=settings.APPLICATION_JWT_AUDIENCE, + ) + assert payload["user_id"] == str(existing_user.id) + assert User.objects.filter(email=email).count() == 1 + + +@mock.patch.object( + provisional_user_service.ProvisionalUserService, + "get_or_create", + side_effect=provisional_user_service.ProvisionalUserIntegrityError, +) +def test_api_applications_token_new_user_race_condition_unrecoverable( + mock_get_or_create, settings +): + """Should return 500 when ProvisionalUserIntegrityError is raised.""" + + settings.APPLICATION_ALLOW_USER_CREATION = True + settings.OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION = True + settings.OIDC_USER_SUB_FIELD_IMMUTABLE = False + + application = ApplicationFactory( + is_active=True, scopes=[ApplicationScope.ROOMS_LIST] + ) + plain_secret = "test-secret-123" + application.client_secret = plain_secret + application.save() + + client = APIClient() + response = client.post( + "/external-api/v1.0/application/token/", + { + "client_id": application.client_id, + "client_secret": plain_secret, + "grant_type": "client_credentials", + "scope": "john.doe@example.com", + }, + format="json", + ) + + assert response.status_code == 409 + assert mock_get_or_create.call_count == 1