diff --git a/CHANGELOG.md b/CHANGELOG.md index aebe5541..fb27510c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,8 +11,8 @@ and this project adheres to ## Added -🔧(backend) add option to configure list of required OIDC claims #525 -🔧(helm) add option to disable default tls setting by @dominikkaminski #519 +- 🔧(backend) add option to configure list of essential OIDC claims #525 & #531 +- 🔧(helm) add option to disable default tls setting by @dominikkaminski #519 ## Changed diff --git a/src/backend/core/authentication/backends.py b/src/backend/core/authentication/backends.py index 4d7f80e9..09936aa8 100644 --- a/src/backend/core/authentication/backends.py +++ b/src/backend/core/authentication/backends.py @@ -1,5 +1,7 @@ """Authentication Backends for the Impress core app.""" +import logging + from django.conf import settings from django.core.exceptions import SuspiciousOperation from django.utils.translation import gettext_lazy as _ @@ -11,6 +13,8 @@ from mozilla_django_oidc.auth import ( from core.models import User +logger = logging.getLogger(__name__) + class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend): """Custom OpenID Connect (OIDC) Authentication Backend. @@ -57,24 +61,31 @@ class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend): _("Invalid response format or token verification failed") ) from e - # Validate required claims - missing_claims = [ - claim - for claim in settings.USER_OIDC_REQUIRED_CLAIMS - if claim not in userinfo - ] - if missing_claims: - raise SuspiciousOperation( - _("Missing required claims in user info: %(claims)s") - % {"claims": ", ".join(missing_claims)} - ) - return userinfo + def verify_claims(self, claims): + """ + Verify the presence of essential claims and the "sub" (which is mandatory as defined + by the OIDC specification) to decide if authentication should be allowed. + """ + essential_claims = settings.USER_OIDC_ESSENTIAL_CLAIMS + missing_claims = [claim for claim in essential_claims if claim not in claims] + + if missing_claims: + logger.error("Missing essential claims: %s", missing_claims) + return False + + return True + def get_or_create_user(self, access_token, id_token, payload): """Return a User based on userinfo. Create a new user if no match is found.""" user_info = self.get_userinfo(access_token, id_token, payload) + + if not self.verify_claims(user_info): + raise SuspiciousOperation("Claims verification failed.") + + sub = user_info["sub"] email = user_info.get("email") # Get user's full name from OIDC fields defined in settings @@ -87,12 +98,6 @@ class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend): "short_name": short_name, } - sub = user_info.get("sub") - if not sub: - raise SuspiciousOperation( - _("User info contained no recognizable user identification") - ) - user = self.get_existing_user(sub, email) if user: @@ -113,15 +118,13 @@ class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend): return full_name or None def get_existing_user(self, sub, email): - """Fetch existing user by sub or email.""" + """Fetch an existing user by sub (or email as a fallback respecting fallback setting.""" 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 User.objects.filter(email=email).first() + return None def update_user_if_needed(self, user, claims): diff --git a/src/backend/core/tests/authentication/test_backends.py b/src/backend/core/tests/authentication/test_backends.py index dc937da6..dcc01101 100644 --- a/src/backend/core/tests/authentication/test_backends.py +++ b/src/backend/core/tests/authentication/test_backends.py @@ -1,6 +1,8 @@ """Unit tests for the Authentication Backends.""" import re +from logging import Logger +from unittest import mock from django.core.exceptions import SuspiciousOperation from django.test.utils import override_settings @@ -213,29 +215,6 @@ def test_authentication_getter_new_user_with_email(monkeypatch): assert models.User.objects.count() == 1 -def test_authentication_getter_invalid_token(django_assert_num_queries, monkeypatch): - """The user's info doesn't contain a sub.""" - klass = OIDCAuthenticationBackend() - - def get_userinfo_mocked(*args): - return { - "test": "123", - } - - monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked) - - with ( - django_assert_num_queries(0), - pytest.raises( - SuspiciousOperation, - match="User info contained no recognizable user identification", - ), - ): - klass.get_or_create_user(access_token="test-token", id_token=None, payload=None) - - assert models.User.objects.exists() is False - - @override_settings(OIDC_OP_USER_ENDPOINT="http://oidc.endpoint.test/userinfo") @responses.activate def test_authentication_get_userinfo_json_response(): @@ -341,7 +320,7 @@ def test_authentication_getter_existing_disabled_user_via_email( django_assert_num_queries, monkeypatch ): """ - If an existing user does not matches the sub but matches the email and is disabled, + If an existing user does not match the sub but matches the email and is disabled, an error should be raised and a user should not be created. """ @@ -367,85 +346,100 @@ def test_authentication_getter_existing_disabled_user_via_email( assert models.User.objects.count() == 1 -# Required claims +# Essential claims -@override_settings( - OIDC_OP_USER_ENDPOINT="http://oidc.endpoint.test/userinfo", - USER_OIDC_REQUIRED_CLAIMS=["email", "sub", "address"], -) -@responses.activate -def test_authentication_get_userinfo_required_claims_missing(): - """Ensure SuspiciousOperation is raised if required claims are missing.""" +def test_authentication_verify_claims_default(django_assert_num_queries, monkeypatch): + """The sub claim should be mandatory by default.""" + klass = OIDCAuthenticationBackend() - responses.add( - responses.GET, - re.compile(r".*/userinfo"), - json={ - "last_name": "Doe", - "email": "john.doe@example.com", - }, - status=200, - ) + def get_userinfo_mocked(*args): + return { + "test": "123", + } - oidc_backend = OIDCAuthenticationBackend() + monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked) - with pytest.raises( - SuspiciousOperation, match="Missing required claims in user info: sub, address" + with ( + django_assert_num_queries(0), + pytest.raises( + KeyError, + match="sub", + ), ): - oidc_backend.get_userinfo("fake_access_token", None, None) + klass.get_or_create_user(access_token="test-token", id_token=None, payload=None) + + assert models.User.objects.exists() is False -@override_settings( - OIDC_OP_USER_ENDPOINT="http://oidc.endpoint.test/userinfo", - USER_OIDC_REQUIRED_CLAIMS=["email", "Sub"], +@pytest.mark.parametrize( + "essential_claims, missing_claims", + [ + (["email", "sub"], ["email"]), + (["Email", "sub"], ["Email"]), # Case sensitivity + ], ) -@responses.activate -def test_authentication_get_userinfo_required_claims_case_sensitivity(): - """Ensure the system respects case sensitivity for required claims.""" +@override_settings(OIDC_OP_USER_ENDPOINT="http://oidc.endpoint.test/userinfo") +@mock.patch.object(Logger, "error") +def test_authentication_verify_claims_essential_missing( + mock_logger, + essential_claims, + missing_claims, + django_assert_num_queries, + monkeypatch, +): + """Ensure SuspiciousOperation is raised if essential claims are missing.""" - responses.add( - responses.GET, - re.compile(r".*/userinfo"), - json={ + klass = OIDCAuthenticationBackend() + + def get_userinfo_mocked(*args): + return { "sub": "123", "last_name": "Doe", - "email": "john.doe@example.com", - }, - status=200, - ) + } - oidc_backend = OIDCAuthenticationBackend() + monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked) - with pytest.raises( - SuspiciousOperation, match="Missing required claims in user info: Sub" + with ( + django_assert_num_queries(0), + pytest.raises( + SuspiciousOperation, + match="Claims verification failed", + ), + override_settings(USER_OIDC_ESSENTIAL_CLAIMS=essential_claims), ): - oidc_backend.get_userinfo("fake_access_token", None, None) + klass.get_or_create_user(access_token="test-token", id_token=None, payload=None) + + assert models.User.objects.exists() is False + mock_logger.assert_called_once_with("Missing essential claims: %s", missing_claims) @override_settings( OIDC_OP_USER_ENDPOINT="http://oidc.endpoint.test/userinfo", - USER_OIDC_REQUIRED_CLAIMS=["email", "sub"], + USER_OIDC_ESSENTIAL_CLAIMS=["email", "last_name"], ) -@responses.activate -def test_authentication_get_userinfo_required_claims_success(): - """Ensure user is authenticated when required claims are present.""" +def test_authentication_verify_claims_success(django_assert_num_queries, monkeypatch): + """Ensure user is authenticated when all essential claims are present.""" - responses.add( - responses.GET, - re.compile(r".*/userinfo"), - json={ - "sub": "123", - "last_name": "Doe", + klass = OIDCAuthenticationBackend() + + def get_userinfo_mocked(*args): + return { "email": "john.doe@example.com", - }, - status=200, - ) + "last_name": "Doe", + "sub": "123", + } - oidc_backend = OIDCAuthenticationBackend() - result = oidc_backend.get_userinfo("fake_access_token", None, None) + monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked) - assert result["sub"] == "123" - assert result.get("first_name") is None - assert result["last_name"] == "Doe" - assert result["email"] == "john.doe@example.com" + with django_assert_num_queries(6): + user = klass.get_or_create_user( + access_token="test-token", id_token=None, payload=None + ) + + assert models.User.objects.filter(id=user.id).exists() + + assert user.sub == "123" + assert user.full_name == "Doe" + assert user.short_name is None + assert user.email == "john.doe@example.com" diff --git a/src/backend/impress/settings.py b/src/backend/impress/settings.py index f29dce1a..41557bc2 100755 --- a/src/backend/impress/settings.py +++ b/src/backend/impress/settings.py @@ -474,8 +474,8 @@ class Base(Configuration): environ_prefix=None, ) - USER_OIDC_REQUIRED_CLAIMS = values.ListValue( - default=[], environ_name="USER_OIDC_REQUIRED_CLAIMS", environ_prefix=None + USER_OIDC_ESSENTIAL_CLAIMS = values.ListValue( + default=[], environ_name="USER_OIDC_ESSENTIAL_CLAIMS", environ_prefix=None ) USER_OIDC_FIELDS_TO_FULLNAME = values.ListValue( default=["first_name", "last_name"],