diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e72850..a2d01ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to ### Fixed - 🩹(mailbox) fix status of current mailboxes +- 🚑️(backend) fix claim contains non-user field #548 ## [1.6.0] - 2024-11-20 diff --git a/src/backend/core/authentication/backends.py b/src/backend/core/authentication/backends.py index d51decb..d9f2102 100644 --- a/src/backend/core/authentication/backends.py +++ b/src/backend/core/authentication/backends.py @@ -220,13 +220,11 @@ class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend): def update_user_if_needed(self, user, claims): """Update user claims if they have changed.""" - has_changed = any( - value and value != getattr(user, key) - for key, value in claims.items() - if key != "sub" - ) - if has_changed: - updated_claims = { - key: value for key, value in claims.items() if value and key != "sub" - } + updated_claims = {} + for key in ["email", "name"]: + claim_value = claims.get(key) + if claim_value and claim_value != getattr(user, key): + updated_claims[key] = claim_value + + if updated_claims: self.UserModel.objects.filter(sub=user.sub).update(**updated_claims) diff --git a/src/backend/core/tests/authentication/test_backends.py b/src/backend/core/tests/authentication/test_backends.py index 522be7a..70e79c1 100644 --- a/src/backend/core/tests/authentication/test_backends.py +++ b/src/backend/core/tests/authentication/test_backends.py @@ -106,6 +106,39 @@ def test_authentication_getter_existing_user_change_fields( assert user.name == f"{first_name:s} {last_name:s}" +def test_authentication_getter_existing_user_keep_fields( + django_assert_num_queries, monkeypatch +): + """ + Falsy values in claim should not update the user's fields. + """ + klass = OIDCAuthenticationBackend() + user = factories.UserFactory( + name="John Doe", email="john.doe@example.com", with_organization=True + ) + + def get_userinfo_mocked(*args): + return { + "sub": user.sub, + "email": None, + "first_name": "", + "last_name": "", + } + + monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked) + + # No field changed no more query + with django_assert_num_queries(1): + 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 == "john.doe@example.com" + assert user.name == "John Doe" + + def test_authentication_getter_existing_user_via_email( django_assert_num_queries, monkeypatch ): @@ -381,3 +414,42 @@ def test_authentication_getter_existing_user_via_email_update_organization( assert user == db_user assert user.organization is not None assert user.organization.domain_list == ["my-domain.com"] + + +def test_authentication_getter_existing_user_with_registration_id( + monkeypatch, + settings, +): + """ + Authenticate a user who already exists with an organization registration ID. + + This asserts the "update_user_if_needed" does not fail when the claim + contains the organization registration ID (or any value not present in the + User model). + """ + settings.OIDC_ORGANIZATION_REGISTRATION_ID_FIELD = "registration_number" + + klass = OIDCAuthenticationBackend() + _existing_user = factories.UserFactory( + sub="123", + name="John Doe", + email="people@example.com", + ) + + def get_userinfo_mocked(*args): + return { + "sub": "123", + "email": "people@example.com", + "first_name": "John", + "last_name": "Doe", + "registration_number": "12345678901234", + } + + monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked) + + user = klass.get_or_create_user( + access_token="test-token", id_token=None, payload=None + ) + + assert user.organization is not None + assert user.organization.registration_id_list == ["12345678901234"]