From fb22ba44ecff88cf2cbfcc494b04a9e2e143de76 Mon Sep 17 00:00:00 2001 From: Samuel Paccoud - DINUM Date: Fri, 27 Dec 2024 20:49:55 +0100 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F(backend)=20rename=20required?= =?UTF-8?q?=20claims=20to=20essential=20claims=20as=20per=20spec?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It was pointed by @lebaudantoine that the OIDC specification uses the term "essential claims" for what we called required claims. Further more, the Mozilla OIDC library that we use, validates claims in a method called "verify_claims". Let's override this method. --- CHANGELOG.md | 4 +- src/backend/core/authentication/backends.py | 41 ++--- .../tests/authentication/test_backends.py | 171 ++++++++++-------- src/backend/impress/settings.py | 4 +- 4 files changed, 115 insertions(+), 105 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aebe5541f..fb27510c7 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 4d7f80e99..e4745a258 100644 --- a/src/backend/core/authentication/backends.py +++ b/src/backend/core/authentication/backends.py @@ -57,24 +57,27 @@ def get_userinfo(self, access_token, id_token, payload): _("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 to decide if authentication should be allowed. + """ + essential_claims = settings.USER_OIDC_ESSENTIAL_CLAIMS + if not "sub" in essential_claims: + raise SuspiciousOperation('Essential claims should include "sub"') + + return all(claim in claims for claim in essential_claims) + 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 +90,6 @@ def get_or_create_user(self, access_token, id_token, payload): "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 +110,13 @@ def compute_full_name(self, user_info): 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 dc937da67..cab232836 100644 --- a/src/backend/core/tests/authentication/test_backends.py +++ b/src/backend/core/tests/authentication/test_backends.py @@ -213,29 +213,6 @@ def get_userinfo_mocked(*args): 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 +318,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 +344,123 @@ def get_userinfo_mocked(*args): assert models.User.objects.count() == 1 -# Required claims +# Essential claims + + +def test_authentication_verify_claims_default(django_assert_num_queries, monkeypatch): + """The sub claim should be declared as essential by default.""" + 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="Claims verification failed", + ), + ): + 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", "address"], + USER_OIDC_ESSENTIAL_CLAIMS=[], ) -@responses.activate -def test_authentication_get_userinfo_required_claims_missing(): - """Ensure SuspiciousOperation is raised if required claims are missing.""" +def test_authentication_verify_claims_essential_empty( + django_assert_num_queries, monkeypatch +): + """An error should be raised if the configured claims don't include "sub".""" + klass = OIDCAuthenticationBackend() - responses.add( - responses.GET, - re.compile(r".*/userinfo"), - json={ + def get_userinfo_mocked(*args): + return { "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, address" + with ( + django_assert_num_queries(0), + pytest.raises( + SuspiciousOperation, + match='Essential claims should include "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", + [ + ["email", "sub"], + ["Email", "sub"], # 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") +def test_authentication_verify_claims_essential_missing( + essential_claims, django_assert_num_queries, monkeypatch +): + """Ensure SuspiciousOperation is raised if required 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 @override_settings( OIDC_OP_USER_ENDPOINT="http://oidc.endpoint.test/userinfo", - USER_OIDC_REQUIRED_CLAIMS=["email", "sub"], + USER_OIDC_ESSENTIAL_CLAIMS=["email", "sub"], ) -@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 f29dce1ac..4a9c55035 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=["sub"], environ_name="USER_OIDC_ESSENTIAL_CLAIMS", environ_prefix=None ) USER_OIDC_FIELDS_TO_FULLNAME = values.ListValue( default=["first_name", "last_name"],