From 5ef6359b7c24eb9960ec1af448287b858a2e90a3 Mon Sep 17 00:00:00 2001 From: lebaudantoine Date: Mon, 4 Nov 2024 13:40:09 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=82(backend)=20fallback=20to=20email?= =?UTF-8?q?=20matching=20when=20OIDC=20sub=20is=20not=20found?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When OIDC providers return random values in the "sub" field instead of stable identifiers, implement email-based user matching as fallback. Note: Current implementation needs improvement. Tests forthcoming. Original: @sampaccoud (ff7914f) on Impress --- src/backend/core/authentication/backends.py | 34 +++++--- .../tests/authentication/test_backends.py | 83 ++++++++++++++++--- src/backend/meet/settings.py | 4 + 3 files changed, 97 insertions(+), 24 deletions(-) diff --git a/src/backend/core/authentication/backends.py b/src/backend/core/authentication/backends.py index ae1a2b2f..9e2ada71 100644 --- a/src/backend/core/authentication/backends.py +++ b/src/backend/core/authentication/backends.py @@ -1,5 +1,6 @@ """Authentication Backends for the Meet core app.""" +from django.conf import settings from django.core.exceptions import SuspiciousOperation from django.utils.translation import gettext_lazy as _ @@ -71,22 +72,31 @@ class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend): _("User info contained no recognizable user identification") ) - try: - user = User.objects.get(sub=sub) - except User.DoesNotExist: - if self.get_settings("OIDC_CREATE_USER", True): - user = User.objects.create( - sub=sub, - email=user_info.get("email"), - password="!", # noqa: S106 - ) - else: - user = None + email = user_info.get("email") + user = self.get_existing_user(sub, email) - if not user: + if not user and self.get_settings("OIDC_CREATE_USER", True): + user = User.objects.create( + sub=sub, + email=email, + password="!", # noqa: S106 + ) + elif not user: return None if not user.is_active: raise SuspiciousOperation(_("User account is disabled")) return user + + def get_existing_user(self, sub, email): + """Fetch existing user by sub or email.""" + try: + return User.objects.get(sub=sub) + except User.DoesNotExist: + if email and settings.OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION: + try: + return User.objects.get(email=email) + except User.DoesNotExist: + pass + return None diff --git a/src/backend/core/tests/authentication/test_backends.py b/src/backend/core/tests/authentication/test_backends.py index c63dc8df..5ae41de5 100644 --- a/src/backend/core/tests/authentication/test_backends.py +++ b/src/backend/core/tests/authentication/test_backends.py @@ -11,32 +11,35 @@ from core.factories import UserFactory pytestmark = pytest.mark.django_db -def test_authentication_getter_existing_user_no_email( - django_assert_num_queries, monkeypatch -): +def test_authentication_getter_existing_user(monkeypatch): """ - If an existing user matches the user's info sub, the user should be returned. + If an existing user matches, the user should be returned. """ klass = OIDCAuthenticationBackend() - db_user = UserFactory() + db_user = UserFactory(email="foo@mail.com") def get_userinfo_mocked(*args): - return {"sub": db_user.sub} + return {"sub": db_user.sub, "email": "some@mail.com"} + + def get_existing_user(*args): + return db_user monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked) + monkeypatch.setattr( + OIDCAuthenticationBackend, "get_existing_user", get_existing_user + ) - with django_assert_num_queries(1): - user = klass.get_or_create_user( - access_token="test-token", id_token=None, payload=None - ) + user = klass.get_or_create_user( + access_token="test-token", id_token=None, payload=None + ) assert user == db_user def test_authentication_getter_new_user_no_email(monkeypatch): """ - If no user matches the user's info sub, a user should be created. + If no user matches, a user should be created. User's info doesn't contain an email, created user's email should be empty. """ klass = OIDCAuthenticationBackend() @@ -58,7 +61,7 @@ def test_authentication_getter_new_user_no_email(monkeypatch): def test_authentication_getter_new_user_with_email(monkeypatch): """ - If no user matches the user's info sub, a user should be created. + If no user matches, a user should be created. User's email and name should be set on the identity. The "email" field on the User model should not be set as it is reserved for staff users. """ @@ -143,3 +146,59 @@ def test_authentication_get_inactive_user(monkeypatch): ), ): klass.get_or_create_user(access_token="test-token", id_token=None, payload=None) + + +def test_finds_user_by_sub(django_assert_num_queries): + """Should return user when found by sub, and email is matching.""" + + klass = OIDCAuthenticationBackend() + db_user = UserFactory(email="foo@mail.com") + + with django_assert_num_queries(1): + user = klass.get_existing_user(db_user.sub, db_user.email) + + assert user == db_user + + +def test_finds_user_when_email_fallback_disabled(django_assert_num_queries, settings): + """Should not return a user when not found by sub, and email fallback is disabled.""" + + settings.OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION = False + + klass = OIDCAuthenticationBackend() + db_user = UserFactory(email="foo@mail.com") + + with django_assert_num_queries(1): + user = klass.get_existing_user("wrong-sub", db_user.email) + + assert user is None + + +def test_finds_user_when_email_is_none(django_assert_num_queries, settings): + """Should not return a user when not found by sub, and email is empty.""" + + settings.OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION = True + + klass = OIDCAuthenticationBackend() + UserFactory(email="foo@mail.com") + + empty_email = "" + + with django_assert_num_queries(1): + user = klass.get_existing_user("wrong-sub", empty_email) + + assert user is None + + +def test_finds_user_by_email(django_assert_num_queries, settings): + """Should return user when found by email, and sub is not matching.""" + + settings.OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION = True + + klass = OIDCAuthenticationBackend() + db_user = UserFactory(email="foo@mail.com") + + with django_assert_num_queries(2): + user = klass.get_existing_user("wrong-sub", db_user.email) + + assert user == db_user diff --git a/src/backend/meet/settings.py b/src/backend/meet/settings.py index d083df51..7bb9116d 100755 --- a/src/backend/meet/settings.py +++ b/src/backend/meet/settings.py @@ -327,6 +327,10 @@ class Base(Configuration): default=True, environ_name="OIDC_CREATE_USER", ) + OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION = values.BooleanValue( + default=False, + environ_name="OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION", + ) OIDC_RP_SIGN_ALGO = values.Value( "RS256", environ_name="OIDC_RP_SIGN_ALGO", environ_prefix=None )