fix: Harden Entra ID OIDC claim handling from review

Refine the initial Entra ID support in response to review: - Merge roles with list(dict.fromkeys(...)), the codebase's order- preserving dedup idiom (feast/utils.py), so duplicates within the top-level roles claim are also collapsed, not only cross-claim ones. - Skip username claims whose value is null or non-string instead of returning them, so a present-but-null early claim no longer shadows a usable later claim; the -> str contract now holds. - Correct the _extract_username_or_raise_error docstring: the raise fires only when a token provides none of the five claims as a string. Extend the tests: intra-claim role de-duplication, human-claim precedence over appid/sub, and rejection of tokens whose only identity claims are null or non-string. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
ntkathole · Jul 23, 2026 · Jul 22, 2026 · Jul 22, 2026 · Jul 22, 2026 · Jul 22, 2026
commit 2f52a80753529fd70b98fb0ae00e4f1948c7fbb1
@@ -61,17 +61,23 @@ async def _validate_token(self, access_token: str):
 
     @staticmethod
     def _extract_username_or_raise_error(data: dict) -> str:
-        """Extract the username from the decoded JWT. Raises if missing — identity is mandatory.
+        """Extract the username from the decoded JWT, or raise if the token
+        carries none of the recognised identity claims.
 
         Human identity claims take precedence: ``preferred_username`` (Keycloak
         default), then ``upn`` (Azure AD / Entra ID). Client-credentials
         (app-only) tokens carry neither, so the calling application's own
         identity is used instead: ``azp`` (Entra ID v2 tokens), ``appid``
         (Entra ID v1 tokens), and finally ``sub``, which every issuer sets.
+        A claim present with a null or non-string value is skipped rather than
+        returned, so it never shadows a usable later claim. Because ``sub`` is a
+        mandatory string JWT claim, the raise below fires only for a malformed
+        token that provides none of the five as a string.
         """
         for claim in ("preferred_username", "upn", "azp", "appid", "sub"):
-            if claim in data:
-                return data[claim]
+            value = data.get(claim)
+            if isinstance(value, str):
+                return value
         raise AuthenticationError(
             "Missing username claim in access token: expected one of "
             "preferred_username, upn, azp, appid or sub."
@@ -199,10 +205,8 @@ async def user_details_from_access_token(self, access_token: str) -> User:
             )
             # Entra ID emits app roles in the top-level `roles` claim instead of
             # Keycloak's nested `resource_access.<client_id>.roles`. Merge both
-            # shapes, preserving order and dropping duplicates.
-            roles = roles + [
-                r for r in self._extract_claim(data, "roles") if r not in roles
-            ]
+            # shapes for every issuer, preserving order and dropping duplicates.
+            roles = list(dict.fromkeys(roles + self._extract_claim(data, "roles")))
             groups = self._extract_claim(data, "groups")
 
             logger.info(

@@ -200,6 +200,16 @@ def test_oidc_token_extracts_groups_and_roles(
             {"upn": "my-name@example.com", "azp": "my-client-app"},
             "my-name@example.com",
         ),
+        # A human claim still wins when accompanied only by application claims.
+        (
+            {
+                "preferred_username": "my-name",
+                "appid": "my-v1-client-app",
+                "sub": "my-subject",
+            },
+            "my-name",
+        ),
+        ({"upn": "my-name@example.com", "sub": "my-subject"}, "my-name@example.com"),
         # Entra ID client-credentials (app-only) tokens carry no human claim.
         (
             {"azp": "my-client-app", "appid": "my-v1-client-app", "sub": "my-subject"},
@@ -247,14 +257,30 @@ def test_oidc_token_username_claim_precedence(
         assertpy.assert_that(user.username).is_equal_to(expected_username)
 
 
+@pytest.mark.parametrize(
+    "token_claims",
+    [
+        # No identity claim at all.
+        {"resource_access": {_CLIENT_ID: {"roles": ["reader"]}}},
+        # Identity claims present but null are skipped, not accepted.
+        {"preferred_username": None, "upn": None, "azp": None, "sub": None},
+        # A non-string identity claim is skipped, not accepted.
+        {"sub": 12345},
+    ],
+)
 @patch(
     "feast.permissions.auth.oidc_token_parser.OAuth2AuthorizationCodeBearer.__call__"
 )
 @patch("feast.permissions.auth.oidc_token_parser.PyJWKClient.get_signing_key_from_jwt")
 @patch("feast.permissions.auth.oidc_token_parser.jwt.decode")
 @patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data")
-def test_oidc_token_without_any_username_claim_fails(
-    mock_discovery_data, mock_jwt, mock_signing_key, mock_oauth2, oidc_config
+def test_oidc_token_without_usable_username_claim_fails(
+    mock_discovery_data,
+    mock_jwt,
+    mock_signing_key,
+    mock_oauth2,
+    token_claims,
+    oidc_config,
 ):
     signing_key = MagicMock()
     signing_key.key = "a-key"
@@ -266,7 +292,7 @@ def test_oidc_token_without_any_username_claim_fails(
         "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs",
     }
 
-    mock_jwt.return_value = {"resource_access": {_CLIENT_ID: {"roles": ["reader"]}}}
+    mock_jwt.return_value = token_claims
 
     access_token = "aaa-bbb-ccc"
     token_parser = OidcTokenParser(auth_config=oidc_config)
@@ -281,6 +307,8 @@ def test_oidc_token_without_any_username_claim_fails(
     [
         # Entra ID app roles arrive in the top-level `roles` claim.
         ({"roles": ["reader", "writer"]}, ["reader", "writer"]),
+        # Duplicates within the top-level claim are collapsed.
+        ({"roles": ["reader", "reader", "writer"]}, ["reader", "writer"]),
         # Keycloak's nested claim keeps working on its own.
         ({"resource_access": {_CLIENT_ID: {"roles": ["reader"]}}}, ["reader"]),
         # Both shapes present: merged, in order, without duplicates.