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