From 945f55f50dec026f66f90a5cd3cadb9fa4ade249 Mon Sep 17 00:00:00 2001
From: Samuel Paccoud - DINUM <samuel.paccoud@mail.numerique.gouv.fr>
Date: Fri, 3 Jan 2025 17:37:41 +0100
Subject: [PATCH] =?UTF-8?q?=E2=9C=85(backend)=20add=20test=20to=20secure?=
 =?UTF-8?q?=20updating=20user=20when=20matched=20on=20email?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We had doubts that the user was correctly updated in the case where
its identity was matched on the email and not on the sub. I added
a test and confirmed that it was working correctly. I still modified
the backend to update the user based on its "id" instead of its "sub"
because it was confusing, but both actually work the same.
---
 src/backend/core/authentication/backends.py   |  2 +-
 .../tests/authentication/test_backends.py     | 47 ++++++++++++++++++-
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/src/backend/core/authentication/backends.py b/src/backend/core/authentication/backends.py
index 09936aa8..63932858 100644
--- a/src/backend/core/authentication/backends.py
+++ b/src/backend/core/authentication/backends.py
@@ -134,4 +134,4 @@ class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend):
         )
         if has_changed:
             updated_claims = {key: value for key, value in claims.items() if value}
-            self.UserModel.objects.filter(sub=user.sub).update(**updated_claims)
+            self.UserModel.objects.filter(id=user.id).update(**updated_claims)
diff --git a/src/backend/core/tests/authentication/test_backends.py b/src/backend/core/tests/authentication/test_backends.py
index dcc01101..c73099a6 100644
--- a/src/backend/core/tests/authentication/test_backends.py
+++ b/src/backend/core/tests/authentication/test_backends.py
@@ -130,11 +130,12 @@ def test_authentication_getter_existing_user_with_email(
         ("Jack", "Duy", "jack.duy@example.com"),
     ],
 )
-def test_authentication_getter_existing_user_change_fields(
+def test_authentication_getter_existing_user_change_fields_sub(
     first_name, last_name, email, django_assert_num_queries, monkeypatch
 ):
     """
-    It should update the email or name fields on the user when they change.
+    It should update the email or name fields on the user when they change
+    and the user was identified by its "sub".
     """
     klass = OIDCAuthenticationBackend()
     user = UserFactory(
@@ -164,6 +165,48 @@ def test_authentication_getter_existing_user_change_fields(
     assert user.short_name == first_name
 
 
+@pytest.mark.parametrize(
+    "first_name, last_name, email",
+    [
+        ("Jack", "Doe", "john.doe@example.com"),
+        ("John", "Duy", "john.doe@example.com"),
+    ],
+)
+def test_authentication_getter_existing_user_change_fields_email(
+    first_name, last_name, email, django_assert_num_queries, monkeypatch
+):
+    """
+    It should update the name fields on the user when they change
+    and the user was identified by its "email" as fallback.
+    """
+    klass = OIDCAuthenticationBackend()
+    user = UserFactory(
+        full_name="John Doe", short_name="John", email="john.doe@example.com"
+    )
+
+    def get_userinfo_mocked(*args):
+        return {
+            "sub": "123",
+            "email": user.email,
+            "first_name": first_name,
+            "last_name": last_name,
+        }
+
+    monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked)
+
+    # One and only one additional update query when a field has changed
+    with django_assert_num_queries(3):
+        authenticated_user = klass.get_or_create_user(
+            access_token="test-token", id_token=None, payload=None
+        )
+
+    assert user == authenticated_user
+    user.refresh_from_db()
+    assert user.email == email
+    assert user.full_name == f"{first_name:s} {last_name:s}"
+    assert user.short_name == first_name
+
+
 def test_authentication_getter_new_user_no_email(monkeypatch):
     """
     If no user matches the user's info sub, a user should be created.