From 0e48bc0f90eac3bc71e4a501277dcc8f65db5beb Mon Sep 17 00:00:00 2001 From: Marie PUPO JEAMMET Date: Fri, 4 Oct 2024 18:59:32 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=82(backend)=20match=20email=20if=20no?= =?UTF-8?q?=20existing=20user=20matches=20the=20sub?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some OIDC identity providers may provide a random value in the "sub" field instead of an identifying ID. In this case, it may be a good idea to fallback to matching the user on its email field. --- CHANGELOG.md | 4 ++ src/backend/core/authentication/backends.py | 67 +++++++++++++------ .../tests/authentication/test_backends.py | 55 +++++++++++++++ src/backend/people/settings.py | 5 ++ 4 files changed, 112 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6299b3b..b522b9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ and this project adheres to - ✨(backend) manage roles on domain admin view +### Changed + +- 🛂(backend) match email if no existing user matches the sub + ## [1.2.1] - 2024-10-03 ### Fixed diff --git a/src/backend/core/authentication/backends.py b/src/backend/core/authentication/backends.py index 86c82c1..3dfdafa 100644 --- a/src/backend/core/authentication/backends.py +++ b/src/backend/core/authentication/backends.py @@ -1,6 +1,7 @@ """Authentication Backends for the People core app.""" from django.conf import settings +from django.contrib.auth import get_user_model from django.core.exceptions import SuspiciousOperation from django.utils.translation import gettext_lazy as _ @@ -9,6 +10,8 @@ from mozilla_django_oidc.auth import ( OIDCAuthenticationBackend as MozillaOIDCAuthenticationBackend, ) +User = get_user_model() + class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend): """Custom OpenID Connect (OIDC) Authentication Backend. @@ -48,7 +51,7 @@ class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend): return userinfo def get_or_create_user(self, access_token, id_token, payload): - """Return a User based on userinfo. Get or create a new user if no user matches the Sub. + """Return a User based on userinfo. Create a new user if no match is found. Parameters: - access_token (str): The access token. @@ -64,30 +67,27 @@ class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend): user_info = self.get_userinfo(access_token, id_token, payload) - # Compute user name from OIDC name fields as defined in settings - names_list = [ - user_info[field] - for field in settings.USER_OIDC_FIELDS_TO_NAME - if user_info.get(field) - ] - user_info["name"] = " ".join(names_list) or None + # Get user's full name from OIDC fields defined in settings + full_name = self.compute_full_name(user_info) + email = user_info.get("email") + + claims = { + "email": email, + "name": full_name, + } sub = user_info.get("sub") - if sub is None: + if not sub: raise SuspiciousOperation( _("User info contained no recognizable user identification") ) - try: - user = self.UserModel.objects.get(sub=sub, is_active=True) - except self.UserModel.DoesNotExist: - if self.get_settings("OIDC_CREATE_USER", True): - user = self.create_user(user_info) - else: - email = user_info.get("email") - name = user_info.get("name") - if email and email != user.email or name and name != user.name: - self.UserModel.objects.filter(sub=sub).update(email=email, name=name) + # if sub is absent, try matching on email + user = self.get_existing_user(sub, email) + if user: + self.update_user_if_needed(user, claims) + elif self.get_settings("OIDC_CREATE_USER", True): + user = User.objects.create(sub=sub, password="!", **claims) # noqa: S106 return user @@ -105,3 +105,32 @@ class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend): email=claims.get("email"), name=claims.get("name"), ) + + def compute_full_name(self, user_info): + """Compute user's full name based on OIDC fields in settings.""" + name_fields = settings.USER_OIDC_FIELDS_TO_NAME + full_name = " ".join( + user_info[field] for field in name_fields if user_info.get(field) + ) + return full_name or None + + def get_existing_user(self, sub, email): + """Fetch existing user by sub or email.""" + try: + return User.objects.get(sub=sub, is_active=True) + except User.DoesNotExist: + if email and settings.OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION: + try: + return User.objects.get(email=email, is_active=True) + except User.DoesNotExist: + pass + return None + + 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 has_changed: + updated_claims = {key: value for key, value in claims.items() if value} + 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 090cf8d..bc19c95 100644 --- a/src/backend/core/tests/authentication/test_backends.py +++ b/src/backend/core/tests/authentication/test_backends.py @@ -1,5 +1,6 @@ """Unit tests for the Authentication Backends.""" +from django.contrib.auth import get_user_model from django.core.exceptions import SuspiciousOperation import pytest @@ -8,6 +9,7 @@ from core import factories, models from core.authentication.backends import OIDCAuthenticationBackend pytestmark = pytest.mark.django_db +User = get_user_model() def test_authentication_getter_existing_user_no_email( @@ -101,6 +103,59 @@ def test_authentication_getter_existing_user_change_fields( assert user.name == f"{first_name:s} {last_name:s}" +def test_authentication_getter_existing_user_via_email( + django_assert_num_queries, monkeypatch +): + """ + If an existing user doesn't match the sub but matches the email, + the user should be returned. + """ + + klass = OIDCAuthenticationBackend() + db_user = factories.UserFactory() + + def get_userinfo_mocked(*args): + return {"sub": "123", "email": db_user.email} + + monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked) + + with django_assert_num_queries(2): + user = klass.get_or_create_user( + access_token="test-token", id_token=None, payload=None + ) + + assert user == db_user + + +def test_authentication_getter_existing_user_no_fallback_to_email( + 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 = factories.UserFactory() + + # Set the setting to False + settings.OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION = False + + def get_userinfo_mocked(*args): + return {"sub": "123", "email": db_user.email} + + 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 doesn'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_new_user_no_email(monkeypatch): """ If no user matches the user's info sub, a user should be created. diff --git a/src/backend/people/settings.py b/src/backend/people/settings.py index d8d91a8..1d1bbbf 100755 --- a/src/backend/people/settings.py +++ b/src/backend/people/settings.py @@ -413,6 +413,11 @@ class Base(Configuration): ) OIDC_TIMEOUT = values.Value(None, environ_name="OIDC_TIMEOUT", environ_prefix=None) + OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION = values.BooleanValue( + default=True, + environ_name="OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION", + environ_prefix=None, + ) # MAILBOX-PROVISIONING API WEBMAIL_URL = values.Value(