🐛(backend) fix create document for user when sub does not match

When creating a document on behalf of a user via the server-to-server
API, a special edge case was broken that should should never happen
but happens in our OIDC federation because one of the provider modifies
the users "sub" each time they login.

We end-up with existing users for who the email matches but not the sub.
They were not correctly handled.

I made a few additional fixes and improvements to the endpoint.

Author: sampaccoud

CHANGELOG.md

@@ -29,6 +29,9 @@ and this project adheres to
 - 💄(frontend) update doc versioning ui #463
 - 💄(frontend) update doc summary ui #473
 
+## Fixed
+
+- 🐛(backend) fix create document via s2s if sub unknown but email found #543
 
 ## [1.10.0] - 2024-12-17
 

src/backend/core/api/serializers.py

@@ -264,13 +264,17 @@ def create(self, validated_data):
         """Create the document and associate it with the user or send an invitation."""
         language = validated_data.get("language", settings.LANGUAGE_CODE)
 
-        # Get the user based on the sub (unique identifier)
+        # Get the user on its sub (unique identifier). Default on email if allowed in settings
+        email = validated_data["email"]
+
         try:
-            user = models.User.objects.get(sub=validated_data["sub"])
-        except (models.User.DoesNotExist, KeyError):
-            user = None
-            email = validated_data["email"]
-        else:
+            user = models.User.objects.get_user_by_sub_or_email(
+                validated_data["sub"], email
+            )
+        except models.DuplicateEmailError as err:
+            raise serializers.ValidationError({"email": [err.message]}) from err
+
+        if user:
             email = user.email
             language = user.language or language
 
@@ -279,7 +283,9 @@ def create(self, validated_data):
                 validated_data["content"]
             )
         except ConversionError as err:
-            raise exceptions.APIException(detail="could not convert content") from err
+            raise serializers.ValidationError(
+                {"content": ["Could not convert content"]}
+            ) from err
 
         document = models.Document.objects.create(
             title=validated_data["title"],
@@ -302,7 +308,11 @@ def create(self, validated_data):
                 role=models.RoleChoices.OWNER,
             )
 
-        # Notify the user about the newly created document
+        self._send_email_notification(document, validated_data, email, language)
+        return document
+
+    def _send_email_notification(self, document, validated_data, email, language):
+        """Notify the user about the newly created document."""
         subject = validated_data.get("subject") or _(
             "A new document was created on your behalf!"
         )
@@ -313,8 +323,6 @@ def create(self, validated_data):
         }
         document.send_email(subject, [email], context, language)
 
-        return document
-
     def update(self, instance, validated_data):
         """
         This serializer does not support updates.

src/backend/core/authentication/backends.py

@@ -11,7 +11,7 @@
     OIDCAuthenticationBackend as MozillaOIDCAuthenticationBackend,
 )
 
-from core.models import User
+from core.models import DuplicateEmailError, User
 
 logger = logging.getLogger(__name__)
 
@@ -98,7 +98,10 @@ def get_or_create_user(self, access_token, id_token, payload):
             "short_name": short_name,
         }
 
-        user = self.get_existing_user(sub, email)
+        try:
+            user = User.objects.get_user_by_sub_or_email(sub, email)
+        except DuplicateEmailError as err:
+            raise SuspiciousOperation(err.message) from err
 
         if user:
             if not user.is_active:
@@ -117,16 +120,6 @@ def compute_full_name(self, user_info):
         )
         return full_name or None
 
-    def get_existing_user(self, sub, email):
-        """Fetch an existing user by sub (or email as a fallback respecting fallback setting."""
-        try:
-            return User.objects.get(sub=sub)
-        except User.DoesNotExist:
-            if email and settings.OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION:
-                return User.objects.filter(email=email).first()
-
-        return None
-
     def update_user_if_needed(self, user, claims):
         """Update user claims if they have changed."""
         has_changed = any(

src/backend/core/models.py

@@ -1,6 +1,7 @@
 """
 Declare and configure the models for the impress core application
 """
+# pylint: disable=too-many-lines
 
 import hashlib
 import smtplib
@@ -89,6 +90,16 @@ class LinkReachChoices(models.TextChoices):
     PUBLIC = "public", _("Public")  # Even anonymous users can access the document
 
 
+class DuplicateEmailError(Exception):
+    """Raised when an email is already associated with a pre-existing user."""
+
+    def __init__(self, message=None, email=None):
+        """Set message and email to describe the exception."""
+        self.message = message
+        self.email = email
+        super().__init__(self.message)
+
+
 class BaseModel(models.Model):
     """
     Serves as an abstract base model for other models, ensuring that records are validated
@@ -126,6 +137,35 @@ def save(self, *args, **kwargs):
         super().save(*args, **kwargs)
 
 
+class UserManager(auth_models.UserManager):
+    """Custom manager for User model with additional methods."""
+
+    def get_user_by_sub_or_email(self, sub, email):
+        """Fetch existing user by sub or email."""
+        try:
+            return self.get(sub=sub)
+        except self.model.DoesNotExist as err:
+            if not email:
+                return None
+
+            if settings.OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION:
+                try:
+                    return self.get(email=email)
+                except self.model.DoesNotExist:
+                    pass
+            elif (
+                self.filter(email=email).exists()
+                and not settings.OIDC_ALLOW_DUPLICATE_EMAILS
+            ):
+                raise DuplicateEmailError(
+                    _(
+                        "We couldn't find a user with this sub but the email is already "
+                        "associated with a registered user."
+                    )
+                ) from err
+        return None
+
+
 class User(AbstractBaseUser, BaseModel, auth_models.PermissionsMixin):
     """User model to work with OIDC only authentication."""
 
@@ -192,7 +232,7 @@ class User(AbstractBaseUser, BaseModel, auth_models.PermissionsMixin):
         ),
     )
 
-    objects = auth_models.UserManager()
+    objects = UserManager()
 
     USERNAME_FIELD = "admin_email"
     REQUIRED_FIELDS = []
@@ -939,7 +979,10 @@ def clean(self):
         super().clean()
 
         # Check if an identity already exists for the provided email
-        if User.objects.filter(email=self.email).exists():
+        if (
+            User.objects.filter(email=self.email).exists()
+            and not settings.OIDC_ALLOW_DUPLICATE_EMAILS
+        ):
             raise exceptions.ValidationError(
                 {"email": _("This email is already associated to a registered user.")}
             )

src/backend/core/tests/authentication/test_backends.py

@@ -1,5 +1,6 @@
 """Unit tests for the Authentication Backends."""
 
+import random
 import re
 from logging import Logger
 from unittest import mock
@@ -64,7 +65,33 @@ def get_userinfo_mocked(*args):
     assert user == db_user
 
 
-def test_authentication_getter_existing_user_no_fallback_to_email(
+def test_authentication_getter_email_none(monkeypatch):
+    """
+    If no user is found with the sub and no email is provided, a new user should be created.
+    """
+
+    klass = OIDCAuthenticationBackend()
+    db_user = UserFactory(email=None)
+
+    def get_userinfo_mocked(*args):
+        user_info = {"sub": "123"}
+        if random.choice([True, False]):
+            user_info["email"] = None
+        return user_info
+
+    monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked)
+
+    user = klass.get_or_create_user(
+        access_token="test-token", id_token=None, payload=None
+    )
+
+    # Since the sub and email didn't match, it should create a new user
+    assert models.User.objects.count() == 2
+    assert user != db_user
+    assert user.sub == "123"
+
+
+def test_authentication_getter_existing_user_no_fallback_to_email_allow_duplicate(
     settings, monkeypatch
 ):
     """
@@ -77,6 +104,7 @@ def test_authentication_getter_existing_user_no_fallback_to_email(
 
     # Set the setting to False
     settings.OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION = False
+    settings.OIDC_ALLOW_DUPLICATE_EMAILS = True
 
     def get_userinfo_mocked(*args):
         return {"sub": "123", "email": db_user.email}
@@ -93,6 +121,39 @@ def get_userinfo_mocked(*args):
     assert user.sub == "123"
 
 
+def test_authentication_getter_existing_user_no_fallback_to_email_no_duplicate(
+    settings, monkeypatch
+):
+    """
+    When the "OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION" setting is set to False,
+    the system should not match users by email, even if the email matches.
+    """
+
+    klass = OIDCAuthenticationBackend()
+    db_user = UserFactory()
+
+    # Set the setting to False
+    settings.OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION = False
+    settings.OIDC_ALLOW_DUPLICATE_EMAILS = False
+
+    def get_userinfo_mocked(*args):
+        return {"sub": "123", "email": db_user.email}
+
+    monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked)
+
+    with pytest.raises(
+        SuspiciousOperation,
+        match=(
+            "We couldn't find a user with this sub but the email is already associated "
+            "with a registered user."
+        ),
+    ):
+        klass.get_or_create_user(access_token="test-token", id_token=None, payload=None)
+
+    # Since the sub doesn't match, it should not create a new user
+    assert models.User.objects.count() == 1
+
+
 def test_authentication_getter_existing_user_with_email(
     django_assert_num_queries, monkeypatch
 ):