From 82bb5f0f8b76dfcd1bdfa971892c092638b73c16 Mon Sep 17 00:00:00 2001 From: lebaudantoine Date: Wed, 13 Nov 2024 12:18:41 +0100 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8(backend)=20persist=20OIDC=20first=20n?= =?UTF-8?q?ame=20and=20last=20name=20while=20authenticating?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Inspired by @sampaccoud's eee2003 commit on impress, adapt the code to be more Pythonic. Add basic test coverage for user name synchronization on login. User name fields now update automatically at each login when new data is available. Note: current logic doesn't handle the case where a user with existing names logs in with missing first/last names - should we clear the names then? Removing a field that was present in the initial form is not a valid update operation. --- src/backend/core/authentication/backends.py | 38 +++++- .../tests/authentication/test_backends.py | 126 ++++++++++++++++++ src/backend/meet/settings.py | 10 ++ 3 files changed, 173 insertions(+), 1 deletion(-) diff --git a/src/backend/core/authentication/backends.py b/src/backend/core/authentication/backends.py index 7ce8799c..202a5293 100644 --- a/src/backend/core/authentication/backends.py +++ b/src/backend/core/authentication/backends.py @@ -75,11 +75,16 @@ class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend): email = user_info.get("email") user = self.get_existing_user(sub, email) + claims = { + "email": email, + "full_name": self.compute_full_name(user_info), + "short_name": user_info.get(settings.OIDC_USERINFO_SHORTNAME_FIELD), + } if not user and self.get_settings("OIDC_CREATE_USER", True): user = User.objects.create( sub=sub, - email=email, password="!", # noqa: S106 + **claims, ) elif not user: return None @@ -87,6 +92,8 @@ class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend): if not user.is_active: raise SuspiciousOperation(_("User account is disabled")) + self.update_user_if_needed(user, claims) + return user def get_existing_user(self, sub, email): @@ -104,3 +111,32 @@ class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend): _("Multiple user accounts share a common email.") ) from e return None + + @staticmethod + def compute_full_name(user_info): + """Compute user's full name based on OIDC fields in settings.""" + full_name = " ".join( + filter( + None, + ( + user_info.get(field) + for field in settings.OIDC_USERINFO_FULLNAME_FIELDS + ), + ) + ) + return full_name or None + + @staticmethod + def update_user_if_needed(user, claims): + """Update user claims if they have changed.""" + user_fields = vars(user.__class__) # Get available model fields + updated_claims = { + key: value + for key, value in claims.items() + if value and key in user_fields and value != getattr(user, key) + } + + if not updated_claims: + return + + User.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 73600f43..1a27ead5 100644 --- a/src/backend/core/tests/authentication/test_backends.py +++ b/src/backend/core/tests/authentication/test_backends.py @@ -85,6 +85,32 @@ def test_authentication_getter_new_user_with_email(monkeypatch): assert models.User.objects.count() == 1 +@pytest.mark.parametrize("email", [None, "johndoe@foo.com"]) +def test_authentication_getter_new_user_with_names(monkeypatch, email): + """ + If no user matches, a user should be created. + User's info contains name-related field, created user's full and short names should be filled, + whether the email is filled + """ + klass = OIDCAuthenticationBackend() + + def get_userinfo_mocked(*args): + return {"sub": "123", "first_name": "John", "last_name": "Doe", "email": email} + + 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.sub == "123" + assert user.email == email + assert user.full_name == "John Doe" + assert user.short_name == "John" + assert user.password == "!" + assert models.User.objects.count() == 1 + + def test_models_oidc_user_getter_invalid_token(django_assert_num_queries, monkeypatch): """The user's info doesn't contain a sub.""" klass = OIDCAuthenticationBackend() @@ -286,3 +312,103 @@ def test_authentication_getter_existing_user_email_tricky(email, monkeypatch, se ) assert user != db_user + + +@pytest.mark.parametrize( + "first_name, last_name, email", + [ + ("Jack", "Doe", "john.doe@example.com"), + ("John", "Duy", "john.doe@example.com"), + ("John", "Doe", "jack.duy@example.com"), + ("Jack", "Duy", "jack.duy@example.com"), + ], +) +def test_authentication_getter_existing_user_change_fields( + first_name, last_name, email, django_assert_num_queries, monkeypatch +): + """It should update the email or name fields on the user when they change.""" + + klass = OIDCAuthenticationBackend() + user = UserFactory( + full_name="John Doe", short_name="John", email="john.doe@example.com" + ) + + def get_userinfo_mocked(*args): + return { + "sub": user.sub, + "email": 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(2): + 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 + + +@pytest.mark.parametrize( + "user_info, expected_name", + [ + ({"given_name": "John", "family_name": "Doe"}, "John Doe"), + ( + {"given_name": "John", "middle_name": "M", "family_name": "Doe"}, + "John M Doe", + ), + ({"family_name": "Doe"}, "Doe"), + ({"given_name": "", "family_name": ""}, None), + ({}, None), + ], +) +def test_compute_full_name(user_info, expected_name, settings): + """Test full name computation from OIDC user info fields.""" + settings.OIDC_USERINFO_FULLNAME_FIELDS = [ + "given_name", + "middle_name", + "family_name", + ] + klass = OIDCAuthenticationBackend() + assert klass.compute_full_name(user_info) == expected_name + + +def test_compute_full_name_no_fields(settings): + """Test full name computation with empty field configuration.""" + settings.OIDC_USERINFO_FULLNAME_FIELDS = [] + klass = OIDCAuthenticationBackend() + assert klass.compute_full_name({"given_name": "John"}) is None + + +@pytest.mark.parametrize( + "claims", + [ + {"email": "john.doe@example.com"}, # Same data - no change needed + {"email": ""}, # Empty strings should not override + {"non_related_field": "foo"}, # Unrelated fields should be ignored + {}, # Empty claims should not affect user + {"email": None}, # None values should be ignored + ], +) +def test_update_user_when_no_update_needed(django_assert_num_queries, claims): + """Test that user attributes remain unchanged when claims don't require updates.""" + + user = UserFactory( + full_name="John Doe", short_name="John", email="john.doe@example.com" + ) + + klass = OIDCAuthenticationBackend() + + with django_assert_num_queries(0): + klass.update_user_if_needed(user, claims) + + user.refresh_from_db() + + assert user.email == "john.doe@example.com" diff --git a/src/backend/meet/settings.py b/src/backend/meet/settings.py index e04095d5..e81448bf 100755 --- a/src/backend/meet/settings.py +++ b/src/backend/meet/settings.py @@ -390,6 +390,16 @@ class Base(Configuration): OIDC_REDIRECT_FIELD_NAME = values.Value( "returnTo", environ_name="OIDC_REDIRECT_FIELD_NAME", environ_prefix=None ) + OIDC_USERINFO_FULLNAME_FIELDS = values.ListValue( + default=["first_name", "last_name"], + environ_name="OIDC_USERINFO_FULLNAME_FIELDS", + environ_prefix=None, + ) + OIDC_USERINFO_SHORTNAME_FIELD = values.Value( + default="first_name", + environ_name="OIDC_USERINFO_SHORTNAME_FIELD", + environ_prefix=None, + ) # Video conference configuration LIVEKIT_CONFIGURATION = {