♻️(backend) rename required claims to essential claims as per spec

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

@@ -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
 

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):

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": "[email protected]",
-        },
-        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": "[email protected]",
-        },
-        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": "[email protected]",
-        },
-        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"] == "[email protected]"
+    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 == "[email protected]"

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"],