From 5e9c95b635af03c219e3035dd88f88709ed74449 Mon Sep 17 00:00:00 2001 From: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> Date: Wed, 22 Jul 2026 13:51:21 -0500 Subject: [PATCH 1/7] fix: Support Entra ID (Azure AD) token claims in OIDC auth The OIDC token parser was written against Keycloak's token shape, which rejected Microsoft Entra ID tokens in two independent ways. Username extraction accepted only preferred_username or upn. Entra client-credentials (app-only) tokens carry neither, so every machine-to-machine caller failed authentication. Fall back to the calling application's own identity when no human claim is present: azp on v2 tokens, appid on v1, then sub, which every issuer sets. A token with none of the five claims still raises AuthenticationError. Roles were read only from Keycloak's nested resource_access..roles. Entra emits app roles in the top-level roles claim, so the extracted list was always empty and RoleBasedPolicy could never match. Merge the top-level claim into the existing extraction, preserving order and dropping duplicates. Both changes are additive, so Keycloak behavior is unchanged: the username fallbacks only fire when preferred_username and upn are both absent, and the merge only adds roles. Token validation is untouched. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> --- .../permissions/auth/oidc_token_parser.py | 23 ++- .../permissions/auth/test_token_parser.py | 150 ++++++++++++++++++ 2 files changed, 166 insertions(+), 7 deletions(-) diff --git a/sdk/python/feast/permissions/auth/oidc_token_parser.py b/sdk/python/feast/permissions/auth/oidc_token_parser.py index 7940c9d7525..42a99544271 100644 --- a/sdk/python/feast/permissions/auth/oidc_token_parser.py +++ b/sdk/python/feast/permissions/auth/oidc_token_parser.py @@ -63,15 +63,18 @@ async def _validate_token(self, access_token: str): def _extract_username_or_raise_error(data: dict) -> str: """Extract the username from the decoded JWT. Raises if missing — identity is mandatory. - Checks ``preferred_username`` first (Keycloak default), then falls back - to ``upn`` (Azure AD / Entra ID). + 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. """ - if "preferred_username" in data: - return data["preferred_username"] - if "upn" in data: - return data["upn"] + for claim in ("preferred_username", "upn", "azp", "appid", "sub"): + if claim in data: + return data[claim] raise AuthenticationError( - "Missing preferred_username or upn field in access token." + "Missing username claim in access token: expected one of " + "preferred_username, upn, azp, appid or sub." ) @staticmethod @@ -194,6 +197,12 @@ async def user_details_from_access_token(self, access_token: str) -> User: if self._auth_config.client_id else [] ) + # Entra ID emits app roles in the top-level `roles` claim instead of + # Keycloak's nested `resource_access..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 + ] groups = self._extract_claim(data, "groups") logger.info( diff --git a/sdk/python/tests/unit/permissions/auth/test_token_parser.py b/sdk/python/tests/unit/permissions/auth/test_token_parser.py index fdec71e109a..7e8b074e6a5 100644 --- a/sdk/python/tests/unit/permissions/auth/test_token_parser.py +++ b/sdk/python/tests/unit/permissions/auth/test_token_parser.py @@ -184,6 +184,156 @@ def test_oidc_token_extracts_groups_and_roles( assertpy.assert_that(user.has_matching_group(["banking-admin"])).is_true() +@pytest.mark.parametrize( + "token_claims, expected_username", + [ + # Human identity claims keep precedence over the application claims. + ( + { + "preferred_username": "my-name", + "upn": "my-name@example.com", + "azp": "my-client-app", + }, + "my-name", + ), + ( + {"upn": "my-name@example.com", "azp": "my-client-app"}, + "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"}, + "my-client-app", + ), + ({"appid": "my-v1-client-app", "sub": "my-subject"}, "my-v1-client-app"), + ({"sub": "my-subject"}, "my-subject"), + ], +) +@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_username_claim_precedence( + mock_discovery_data, + mock_jwt, + mock_signing_key, + mock_oauth2, + token_claims, + expected_username, + oidc_config, +): + signing_key = MagicMock() + signing_key.key = "a-key" + mock_signing_key.return_value = signing_key + + mock_discovery_data.return_value = { + "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", + "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", + "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", + } + + mock_jwt.return_value = token_claims + + access_token = "aaa-bbb-ccc" + token_parser = OidcTokenParser(auth_config=oidc_config) + user = asyncio.run( + token_parser.user_details_from_access_token(access_token=access_token) + ) + + assertpy.assert_that(user).is_type_of(User) + if isinstance(user, User): + assertpy.assert_that(user.username).is_equal_to(expected_username) + + +@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 +): + signing_key = MagicMock() + signing_key.key = "a-key" + mock_signing_key.return_value = signing_key + + mock_discovery_data.return_value = { + "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", + "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", + "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", + } + + mock_jwt.return_value = {"resource_access": {_CLIENT_ID: {"roles": ["reader"]}}} + + access_token = "aaa-bbb-ccc" + token_parser = OidcTokenParser(auth_config=oidc_config) + with pytest.raises(AuthenticationError): + asyncio.run( + token_parser.user_details_from_access_token(access_token=access_token) + ) + + +@pytest.mark.parametrize( + "token_claims, expected_roles", + [ + # Entra ID app roles arrive in the top-level `roles` claim. + ({"roles": ["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. + ( + { + "resource_access": {_CLIENT_ID: {"roles": ["reader", "writer"]}}, + "roles": ["writer", "admin"], + }, + ["reader", "writer", "admin"], + ), + ], +) +@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_merges_top_level_and_resource_access_roles( + mock_discovery_data, + mock_jwt, + mock_signing_key, + mock_oauth2, + token_claims, + expected_roles, + oidc_config, +): + signing_key = MagicMock() + signing_key.key = "a-key" + mock_signing_key.return_value = signing_key + + mock_discovery_data.return_value = { + "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", + "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", + "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", + } + + mock_jwt.return_value = {"preferred_username": "my-name", **token_claims} + + access_token = "aaa-bbb-ccc" + token_parser = OidcTokenParser(auth_config=oidc_config) + user = asyncio.run( + token_parser.user_details_from_access_token(access_token=access_token) + ) + + assertpy.assert_that(user).is_type_of(User) + if isinstance(user, User): + assertpy.assert_that(user.roles).is_equal_to(expected_roles) + for role in expected_roles: + assertpy.assert_that(user.has_matching_role([role])).is_true() + assertpy.assert_that(user.has_matching_role(["updater"])).is_false() + + @patch( "feast.permissions.auth.oidc_token_parser.OAuth2AuthorizationCodeBearer.__call__" ) From 0e551851e0109f6a2e0ec02e81f3a1dce9364872 Mon Sep 17 00:00:00 2001 From: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> Date: Wed, 22 Jul 2026 13:52:03 -0500 Subject: [PATCH 2/7] docs: Document Entra ID claim support in OIDC authorization The OIDC assumptions listed for the auth manager described only Keycloak's token shape. Record that roles are also read from the top-level roles claim and merged, and that the username falls back through upn, azp, appid and sub when preferred_username is absent. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> --- docs/getting-started/components/authz_manager.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/getting-started/components/authz_manager.md b/docs/getting-started/components/authz_manager.md index eae3fece50b..12ff7a1a9b5 100644 --- a/docs/getting-started/components/authz_manager.md +++ b/docs/getting-started/components/authz_manager.md @@ -44,9 +44,9 @@ The server, in turn, uses the same OIDC server to validate the token and extract Some assumptions are made in the OIDC server configuration: * The OIDC token refers to a client with roles matching the RBAC roles of the configured `Permission`s (*) -* The roles are exposed in the access token under `resource_access..roles` +* The roles are exposed in the access token under `resource_access..roles` (Keycloak) or in the top-level `roles` claim (Entra ID app roles). Roles found in both are merged. * The JWT token is expected to have a verified signature and not be expired. The Feast OIDC token parser logic validates for `verify_signature` and `verify_exp` so make sure that the given OIDC provider is configured to meet these requirements. -* The `preferred_username` should be part of the JWT token claim. +* The username is read from the first of `preferred_username`, `upn`, `azp`, `appid`, `sub` present in the token. Entra ID client-credentials (app-only) tokens carry no user claim, so they authenticate as the calling application. * For `GroupBasedPolicy` support, the `groups` claim should be present in the access token (requires a "Group Membership" protocol mapper in Keycloak). (*) Please note that **the role match is case-sensitive**, e.g. the name of the role in the OIDC server and in the `Permission` configuration From 2f52a80753529fd70b98fb0ae00e4f1948c7fbb1 Mon Sep 17 00:00:00 2001 From: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> Date: Wed, 22 Jul 2026 14:22:21 -0500 Subject: [PATCH 3/7] 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> --- .../permissions/auth/oidc_token_parser.py | 18 ++++++---- .../permissions/auth/test_token_parser.py | 34 +++++++++++++++++-- 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/sdk/python/feast/permissions/auth/oidc_token_parser.py b/sdk/python/feast/permissions/auth/oidc_token_parser.py index 42a99544271..2bbe8f69942 100644 --- a/sdk/python/feast/permissions/auth/oidc_token_parser.py +++ b/sdk/python/feast/permissions/auth/oidc_token_parser.py @@ -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..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( diff --git a/sdk/python/tests/unit/permissions/auth/test_token_parser.py b/sdk/python/tests/unit/permissions/auth/test_token_parser.py index 7e8b074e6a5..fbb79f84d9c 100644 --- a/sdk/python/tests/unit/permissions/auth/test_token_parser.py +++ b/sdk/python/tests/unit/permissions/auth/test_token_parser.py @@ -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. From 0c689db850725b86ac1918de16689a2d5ad07c24 Mon Sep 17 00:00:00 2001 From: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> Date: Wed, 22 Jul 2026 14:22:31 -0500 Subject: [PATCH 4/7] docs: Add Entra ID token example and group-claim caveat Add an app-only (client-credentials) Entra ID token example alongside the Keycloak one, and note that Entra emits group object IDs (GUIDs) rather than names and omits the groups claim under the overage limit, so GroupBasedPolicy on Entra must reference those IDs. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> --- docs/getting-started/components/authz_manager.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/docs/getting-started/components/authz_manager.md b/docs/getting-started/components/authz_manager.md index 12ff7a1a9b5..b24c7abd4c7 100644 --- a/docs/getting-started/components/authz_manager.md +++ b/docs/getting-started/components/authz_manager.md @@ -47,7 +47,7 @@ Some assumptions are made in the OIDC server configuration: * The roles are exposed in the access token under `resource_access..roles` (Keycloak) or in the top-level `roles` claim (Entra ID app roles). Roles found in both are merged. * The JWT token is expected to have a verified signature and not be expired. The Feast OIDC token parser logic validates for `verify_signature` and `verify_exp` so make sure that the given OIDC provider is configured to meet these requirements. * The username is read from the first of `preferred_username`, `upn`, `azp`, `appid`, `sub` present in the token. Entra ID client-credentials (app-only) tokens carry no user claim, so they authenticate as the calling application. -* For `GroupBasedPolicy` support, the `groups` claim should be present in the access token (requires a "Group Membership" protocol mapper in Keycloak). +* For `GroupBasedPolicy` support, the `groups` claim should be present in the access token (requires a "Group Membership" protocol mapper in Keycloak). Entra ID emits group object IDs (GUIDs) in this claim rather than names, and omits it once a principal exceeds Entra's group overage limit, so on Entra a `GroupBasedPolicy` must reference those IDs and cannot rely on the claim for very large group memberships. (*) Please note that **the role match is case-sensitive**, e.g. the name of the role in the OIDC server and in the `Permission` configuration must be exactly the same. @@ -69,6 +69,16 @@ For example, the access token for a client `app` of a user with `reader` role an } ``` +A Microsoft Entra ID (Azure AD) client-credentials (app-only) token has no user claim; the application authenticates as itself, and its app roles arrive in the top-level `roles` claim: +```json +{ + "azp": "11111111-2222-3333-4444-555555555555", + "roles": [ + "reader" + ] +} +``` + #### Server-Side Configuration The server requires `auth_discovery_url` and `client_id` to validate incoming JWT tokens via JWKS: From 06afa5b000e0c496bf5f31d7b0324b923be253a0 Mon Sep 17 00:00:00 2001 From: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> Date: Wed, 22 Jul 2026 14:27:52 -0500 Subject: [PATCH 5/7] test: Cover OIDC verified-decode sourcing and trim redundant asserts Add a test that gives the unverified routing decode and the verified decode different payloads and asserts identity and roles come from the verified one, so a regression that read claims from the unverified decode would be caught. Drop the has_matching_role assertions in the roles-merge test: the exact roles-list equality already pins the result, and has_matching_role has its own coverage in test_user.py. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> --- .../permissions/auth/test_token_parser.py | 50 +++++++++++++++++-- 1 file changed, 47 insertions(+), 3 deletions(-) diff --git a/sdk/python/tests/unit/permissions/auth/test_token_parser.py b/sdk/python/tests/unit/permissions/auth/test_token_parser.py index fbb79f84d9c..4706a763139 100644 --- a/sdk/python/tests/unit/permissions/auth/test_token_parser.py +++ b/sdk/python/tests/unit/permissions/auth/test_token_parser.py @@ -357,9 +357,53 @@ def test_oidc_token_merges_top_level_and_resource_access_roles( assertpy.assert_that(user).is_type_of(User) if isinstance(user, User): assertpy.assert_that(user.roles).is_equal_to(expected_roles) - for role in expected_roles: - assertpy.assert_that(user.has_matching_role([role])).is_true() - assertpy.assert_that(user.has_matching_role(["updater"])).is_false() + + +@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_identity_and_roles_come_from_verified_decode( + mock_discovery_data, mock_jwt, mock_signing_key, mock_oauth2, oidc_config +): + """Identity and roles must come from the signature-verified decode, not the + unverified decode used only for routing.""" + signing_key = MagicMock() + signing_key.key = "a-key" + mock_signing_key.return_value = signing_key + + mock_discovery_data.return_value = { + "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", + "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", + "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", + } + + def decode(token, key=None, *args, **kwargs): + if kwargs.get("options", {}).get("verify_signature") is False: + # Unverified decode, used only for routing; must not drive identity. + return { + "preferred_username": "spoofed-user", + "resource_access": {_CLIENT_ID: {"roles": ["spoofed-role"]}}, + } + return { + "preferred_username": "verified-user", + "resource_access": {_CLIENT_ID: {"roles": ["reader"]}}, + } + + mock_jwt.side_effect = decode + + access_token = "aaa-bbb-ccc" + token_parser = OidcTokenParser(auth_config=oidc_config) + user = asyncio.run( + token_parser.user_details_from_access_token(access_token=access_token) + ) + + assertpy.assert_that(user).is_type_of(User) + if isinstance(user, User): + assertpy.assert_that(user.username).is_equal_to("verified-user") + assertpy.assert_that(user.roles).is_equal_to(["reader"]) @patch( From 456f740245cb9b2c82a42e38cea91c829f4d964f Mon Sep 17 00:00:00 2001 From: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> Date: Wed, 22 Jul 2026 14:30:45 -0500 Subject: [PATCH 6/7] test: Extract shared OIDC discovery and signing-key fixtures The OIDC token-parser tests each re-declared the same mock discovery document and JWKS signing key. Move both into conftest.py fixtures and have the tests consume them, removing the repeated setup blocks. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> --- .../tests/unit/permissions/auth/conftest.py | 18 +++ .../permissions/auth/test_token_parser.py | 143 ++++++++---------- 2 files changed, 77 insertions(+), 84 deletions(-) diff --git a/sdk/python/tests/unit/permissions/auth/conftest.py b/sdk/python/tests/unit/permissions/auth/conftest.py index 1ad5a30fe7d..1fd2f34bbfd 100644 --- a/sdk/python/tests/unit/permissions/auth/conftest.py +++ b/sdk/python/tests/unit/permissions/auth/conftest.py @@ -1,3 +1,5 @@ +from unittest.mock import MagicMock + import pytest from kubernetes import client @@ -62,6 +64,22 @@ def oidc_config() -> OidcAuthConfig: ) +@pytest.fixture +def discovery_data() -> dict: + return { + "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", + "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", + "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", + } + + +@pytest.fixture +def signing_key() -> MagicMock: + key = MagicMock() + key.key = "a-key" + return key + + @pytest.fixture( scope="module", params=[ diff --git a/sdk/python/tests/unit/permissions/auth/test_token_parser.py b/sdk/python/tests/unit/permissions/auth/test_token_parser.py index 4706a763139..41f201672dd 100644 --- a/sdk/python/tests/unit/permissions/auth/test_token_parser.py +++ b/sdk/python/tests/unit/permissions/auth/test_token_parser.py @@ -23,17 +23,16 @@ @patch("feast.permissions.auth.oidc_token_parser.jwt.decode") @patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data") def test_oidc_token_validation_success( - mock_discovery_data, mock_jwt, mock_signing_key, mock_oauth2, oidc_config + mock_discovery_data, + mock_jwt, + mock_signing_key, + mock_oauth2, + oidc_config, + discovery_data, + signing_key, ): - signing_key = MagicMock() - signing_key.key = "a-key" mock_signing_key.return_value = signing_key - - mock_discovery_data.return_value = { - "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", - "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", - "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", - } + mock_discovery_data.return_value = discovery_data user_data = { "preferred_username": "my-name", @@ -67,17 +66,16 @@ def test_oidc_token_validation_success( @patch("feast.permissions.auth.oidc_token_parser.jwt.decode") @patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data") def test_oidc_token_missing_roles_key_returns_empty( - mock_discovery_data, mock_jwt, mock_signing_key, mock_oauth2, oidc_config + mock_discovery_data, + mock_jwt, + mock_signing_key, + mock_oauth2, + oidc_config, + discovery_data, + signing_key, ): - signing_key = MagicMock() - signing_key.key = "a-key" mock_signing_key.return_value = signing_key - - mock_discovery_data.return_value = { - "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", - "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", - "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", - } + mock_discovery_data.return_value = discovery_data user_data = { "preferred_username": "my-name", @@ -104,17 +102,16 @@ def test_oidc_token_missing_roles_key_returns_empty( @patch("feast.permissions.auth.oidc_token_parser.jwt.decode") @patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data") def test_oidc_token_extracts_groups( - mock_discovery_data, mock_jwt, mock_signing_key, mock_oauth2, oidc_config + mock_discovery_data, + mock_jwt, + mock_signing_key, + mock_oauth2, + oidc_config, + discovery_data, + signing_key, ): - signing_key = MagicMock() - signing_key.key = "a-key" mock_signing_key.return_value = signing_key - - mock_discovery_data.return_value = { - "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", - "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", - "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", - } + mock_discovery_data.return_value = discovery_data user_data = { "preferred_username": "my-name", @@ -146,17 +143,16 @@ def test_oidc_token_extracts_groups( @patch("feast.permissions.auth.oidc_token_parser.jwt.decode") @patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data") def test_oidc_token_extracts_groups_and_roles( - mock_discovery_data, mock_jwt, mock_signing_key, mock_oauth2, oidc_config + mock_discovery_data, + mock_jwt, + mock_signing_key, + mock_oauth2, + oidc_config, + discovery_data, + signing_key, ): - signing_key = MagicMock() - signing_key.key = "a-key" mock_signing_key.return_value = signing_key - - mock_discovery_data.return_value = { - "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", - "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", - "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", - } + mock_discovery_data.return_value = discovery_data user_data = { "preferred_username": "my-name", @@ -233,16 +229,11 @@ def test_oidc_token_username_claim_precedence( token_claims, expected_username, oidc_config, + discovery_data, + signing_key, ): - signing_key = MagicMock() - signing_key.key = "a-key" mock_signing_key.return_value = signing_key - - mock_discovery_data.return_value = { - "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", - "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", - "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", - } + mock_discovery_data.return_value = discovery_data mock_jwt.return_value = token_claims @@ -281,16 +272,11 @@ def test_oidc_token_without_usable_username_claim_fails( mock_oauth2, token_claims, oidc_config, + discovery_data, + signing_key, ): - signing_key = MagicMock() - signing_key.key = "a-key" mock_signing_key.return_value = signing_key - - mock_discovery_data.return_value = { - "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", - "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", - "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", - } + mock_discovery_data.return_value = discovery_data mock_jwt.return_value = token_claims @@ -335,16 +321,11 @@ def test_oidc_token_merges_top_level_and_resource_access_roles( token_claims, expected_roles, oidc_config, + discovery_data, + signing_key, ): - signing_key = MagicMock() - signing_key.key = "a-key" mock_signing_key.return_value = signing_key - - mock_discovery_data.return_value = { - "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", - "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", - "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", - } + mock_discovery_data.return_value = discovery_data mock_jwt.return_value = {"preferred_username": "my-name", **token_claims} @@ -366,19 +347,18 @@ def test_oidc_token_merges_top_level_and_resource_access_roles( @patch("feast.permissions.auth.oidc_token_parser.jwt.decode") @patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data") def test_oidc_identity_and_roles_come_from_verified_decode( - mock_discovery_data, mock_jwt, mock_signing_key, mock_oauth2, oidc_config + mock_discovery_data, + mock_jwt, + mock_signing_key, + mock_oauth2, + oidc_config, + discovery_data, + signing_key, ): """Identity and roles must come from the signature-verified decode, not the unverified decode used only for routing.""" - signing_key = MagicMock() - signing_key.key = "a-key" mock_signing_key.return_value = signing_key - - mock_discovery_data.return_value = { - "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", - "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token", - "jwks_uri": "https://localhost:8080/realms/master/protocol/openid-connect/certs", - } + mock_discovery_data.return_value = discovery_data def decode(token, key=None, *args, **kwargs): if kwargs.get("options", {}).get("verify_signature") is False: @@ -623,14 +603,10 @@ def test_k8s_inter_server_comm( @patch("feast.permissions.auth.oidc_token_parser.jwt.decode") @patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data") def test_oidc_parser_handles_sa_token_via_token_review( - mock_discovery_data, mock_jwt_decode, oidc_config + mock_discovery_data, mock_jwt_decode, oidc_config, discovery_data ): """When a token contains kubernetes.io claim, _handle_sa_token is called (not the OIDC JWKS path).""" - mock_discovery_data.return_value = { - "authorization_endpoint": "https://localhost:8080/auth", - "token_endpoint": "https://localhost:8080/token", - "jwks_uri": "https://localhost:8080/certs", - } + mock_discovery_data.return_value = discovery_data mock_jwt_decode.return_value = { "kubernetes.io": {"namespace": "feast"}, @@ -669,18 +645,17 @@ def test_oidc_parser_handles_sa_token_via_token_review( @patch("feast.permissions.auth.oidc_token_parser.jwt.decode") @patch("feast.permissions.oidc_service.OIDCDiscoveryService._fetch_discovery_data") def test_oidc_parser_routes_keycloak_token_normally( - mock_discovery_data, mock_jwt_decode, mock_signing_key, mock_oauth2, oidc_config + mock_discovery_data, + mock_jwt_decode, + mock_signing_key, + mock_oauth2, + oidc_config, + discovery_data, + signing_key, ): """When a token does NOT contain kubernetes.io claim, it should follow the OIDC path.""" - signing_key = MagicMock() - signing_key.key = "a-key" mock_signing_key.return_value = signing_key - - mock_discovery_data.return_value = { - "authorization_endpoint": "https://localhost:8080/auth", - "token_endpoint": "https://localhost:8080/token", - "jwks_uri": "https://localhost:8080/certs", - } + mock_discovery_data.return_value = discovery_data keycloak_payload = { "preferred_username": "testuser", From 4b4c564325c469db248e928ea97cbe15baad62bf Mon Sep 17 00:00:00 2001 From: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> Date: Thu, 23 Jul 2026 08:01:24 -0500 Subject: [PATCH 7/7] fix: Address review nitpicks on Entra OIDC PR - Type the discovery_data test fixture as Dict[str, str]. - Split the Entra group-claim caveat into its own bullet in authz_manager.md. - Log the token's claim keys at debug level before raising on a missing username claim, to aid diagnosis (keys only, no values). Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> --- docs/getting-started/components/authz_manager.md | 3 ++- sdk/python/feast/permissions/auth/oidc_token_parser.py | 3 +++ sdk/python/tests/unit/permissions/auth/conftest.py | 3 ++- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/getting-started/components/authz_manager.md b/docs/getting-started/components/authz_manager.md index b24c7abd4c7..7dbb5470f5f 100644 --- a/docs/getting-started/components/authz_manager.md +++ b/docs/getting-started/components/authz_manager.md @@ -47,7 +47,8 @@ Some assumptions are made in the OIDC server configuration: * The roles are exposed in the access token under `resource_access..roles` (Keycloak) or in the top-level `roles` claim (Entra ID app roles). Roles found in both are merged. * The JWT token is expected to have a verified signature and not be expired. The Feast OIDC token parser logic validates for `verify_signature` and `verify_exp` so make sure that the given OIDC provider is configured to meet these requirements. * The username is read from the first of `preferred_username`, `upn`, `azp`, `appid`, `sub` present in the token. Entra ID client-credentials (app-only) tokens carry no user claim, so they authenticate as the calling application. -* For `GroupBasedPolicy` support, the `groups` claim should be present in the access token (requires a "Group Membership" protocol mapper in Keycloak). Entra ID emits group object IDs (GUIDs) in this claim rather than names, and omits it once a principal exceeds Entra's group overage limit, so on Entra a `GroupBasedPolicy` must reference those IDs and cannot rely on the claim for very large group memberships. +* For `GroupBasedPolicy` support, the `groups` claim should be present in the access token (requires a "Group Membership" protocol mapper in Keycloak). +* **Entra ID limitation**: Group claims use object IDs (GUIDs) instead of names, and are omitted entirely when a user exceeds the group overage threshold. GroupBasedPolicy must reference GUIDs and cannot be used for principals with large group memberships. (*) Please note that **the role match is case-sensitive**, e.g. the name of the role in the OIDC server and in the `Permission` configuration must be exactly the same. diff --git a/sdk/python/feast/permissions/auth/oidc_token_parser.py b/sdk/python/feast/permissions/auth/oidc_token_parser.py index 2bbe8f69942..0f4f1b6e676 100644 --- a/sdk/python/feast/permissions/auth/oidc_token_parser.py +++ b/sdk/python/feast/permissions/auth/oidc_token_parser.py @@ -78,6 +78,9 @@ def _extract_username_or_raise_error(data: dict) -> str: value = data.get(claim) if isinstance(value, str): return value + logger.debug( + f"No usable username claim; token claims present: {list(data.keys())}" + ) raise AuthenticationError( "Missing username claim in access token: expected one of " "preferred_username, upn, azp, appid or sub." diff --git a/sdk/python/tests/unit/permissions/auth/conftest.py b/sdk/python/tests/unit/permissions/auth/conftest.py index 1fd2f34bbfd..cf25fc970a0 100644 --- a/sdk/python/tests/unit/permissions/auth/conftest.py +++ b/sdk/python/tests/unit/permissions/auth/conftest.py @@ -1,3 +1,4 @@ +from typing import Dict from unittest.mock import MagicMock import pytest @@ -65,7 +66,7 @@ def oidc_config() -> OidcAuthConfig: @pytest.fixture -def discovery_data() -> dict: +def discovery_data() -> Dict[str, str]: return { "authorization_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/auth", "token_endpoint": "https://localhost:8080/realms/master/protocol/openid-connect/token",