mirror of
https://github.com/PrefectHQ/fastmcp.git
synced 2026-08-22 13:34:17 +02:00
fix: use intent-based flag for OIDC scope patch in load_access_token (#3465)
When OIDCProxy has verify_id_token=True and the IdP issues the same JWT for both access_token and id_token, the value-equality check `verification_token != upstream_token_set.access_token` evaluated to False, skipping the scope patch entirely. This left AccessToken.scopes empty, causing RequireAuthMiddleware to return 403 insufficient_scope. Replace the value-equality check with an intent-based virtual method `_uses_alternate_verification()` that OIDCProxy overrides to return `self._verify_id_token`. The base OAuthProxy returns False (preserving existing behavior for non-OIDC providers). Fixes #3461 Co-authored-by: voidborne-d <voidborne-d@users.noreply.github.com>
This commit is contained in:
parent
f08205789e
commit
33c3acfc87
3 changed files with 126 additions and 4 deletions
|
|
@ -1381,6 +1381,22 @@ class OAuthProxy(OAuthProvider, ConsentMixin):
|
|||
"""
|
||||
return upstream_token_set.access_token
|
||||
|
||||
def _uses_alternate_verification(self) -> bool:
|
||||
"""Whether this provider verifies a different token than the access token.
|
||||
|
||||
When True, ``load_access_token`` patches the validated result with
|
||||
the upstream access token, scopes, and expiry so that the returned
|
||||
``AccessToken`` reflects the access token rather than the
|
||||
verification token.
|
||||
|
||||
The default implementation compares token values, but subclasses
|
||||
should override this to use an intent-based flag so the patch is
|
||||
applied even when the verification token and access token happen to
|
||||
carry the same value (e.g., some OIDC providers issue identical
|
||||
JWTs for both).
|
||||
"""
|
||||
return False
|
||||
|
||||
async def load_access_token(self, token: str) -> AccessToken | None: # type: ignore[override]
|
||||
"""Validate FastMCP JWT by swapping for upstream token.
|
||||
|
||||
|
|
@ -1429,11 +1445,14 @@ class OAuthProxy(OAuthProvider, ConsentMixin):
|
|||
logger.debug("Upstream token validation failed")
|
||||
return None
|
||||
|
||||
# When the verification token differs from the access token
|
||||
# (e.g., id_token verification), ensure the returned AccessToken
|
||||
# When alternate verification is in use (e.g., id_token
|
||||
# verification in OIDCProxy), ensure the returned AccessToken
|
||||
# carries the upstream access token and its scopes, not the
|
||||
# verification token's values.
|
||||
if verification_token != upstream_token_set.access_token:
|
||||
# verification token's values. We use an intent-based check
|
||||
# rather than value equality because some IdPs issue identical
|
||||
# JWTs for both access_token and id_token, which would cause
|
||||
# the scope patch to be skipped even though it's needed.
|
||||
if self._uses_alternate_verification():
|
||||
validated = validated.model_copy(
|
||||
update={
|
||||
"token": upstream_token_set.access_token,
|
||||
|
|
|
|||
|
|
@ -429,6 +429,15 @@ class OIDCProxy(OAuthProxy):
|
|||
return id_token
|
||||
return upstream_token_set.access_token
|
||||
|
||||
def _uses_alternate_verification(self) -> bool:
|
||||
"""Return True when id_token verification is enabled.
|
||||
|
||||
This ensures ``load_access_token`` always patches the validated
|
||||
result with upstream scopes, even when the IdP issues the same
|
||||
JWT for both ``access_token`` and ``id_token``.
|
||||
"""
|
||||
return self._verify_id_token
|
||||
|
||||
def get_oidc_configuration(
|
||||
self,
|
||||
config_url: AnyHttpUrl,
|
||||
|
|
|
|||
|
|
@ -329,3 +329,97 @@ class TestVerifyIdToken:
|
|||
"read",
|
||||
"write",
|
||||
]
|
||||
|
||||
|
||||
class TestUsesAlternateVerification:
|
||||
"""Tests for _uses_alternate_verification intent-based flag."""
|
||||
|
||||
def test_disabled_by_default(self, valid_oidc_configuration_dict):
|
||||
"""OIDCProxy without verify_id_token returns False."""
|
||||
with patch(
|
||||
"fastmcp.server.auth.oidc_proxy.OIDCConfiguration.get_oidc_configuration"
|
||||
) as mock_get:
|
||||
oidc_config = OIDCConfiguration.model_validate(
|
||||
valid_oidc_configuration_dict
|
||||
)
|
||||
mock_get.return_value = oidc_config
|
||||
|
||||
proxy = OIDCProxy(
|
||||
config_url=TEST_CONFIG_URL,
|
||||
client_id=TEST_CLIENT_ID,
|
||||
client_secret=TEST_CLIENT_SECRET,
|
||||
base_url=TEST_BASE_URL,
|
||||
jwt_signing_key="test-secret",
|
||||
)
|
||||
|
||||
assert proxy._uses_alternate_verification() is False
|
||||
|
||||
def test_enabled_with_verify_id_token(self, valid_oidc_configuration_dict):
|
||||
"""OIDCProxy with verify_id_token=True returns True."""
|
||||
with patch(
|
||||
"fastmcp.server.auth.oidc_proxy.OIDCConfiguration.get_oidc_configuration"
|
||||
) as mock_get:
|
||||
oidc_config = OIDCConfiguration.model_validate(
|
||||
valid_oidc_configuration_dict
|
||||
)
|
||||
mock_get.return_value = oidc_config
|
||||
|
||||
proxy = OIDCProxy(
|
||||
config_url=TEST_CONFIG_URL,
|
||||
client_id=TEST_CLIENT_ID,
|
||||
client_secret=TEST_CLIENT_SECRET,
|
||||
base_url=TEST_BASE_URL,
|
||||
jwt_signing_key="test-secret",
|
||||
verify_id_token=True,
|
||||
)
|
||||
|
||||
assert proxy._uses_alternate_verification() is True
|
||||
|
||||
def test_scope_patch_applied_when_tokens_identical(
|
||||
self, valid_oidc_configuration_dict
|
||||
):
|
||||
"""Regression test: scopes must be patched even when id_token and
|
||||
access_token carry the same JWT value (fixes #3461)."""
|
||||
with patch(
|
||||
"fastmcp.server.auth.oidc_proxy.OIDCConfiguration.get_oidc_configuration"
|
||||
) as mock_get:
|
||||
oidc_config = OIDCConfiguration.model_validate(
|
||||
valid_oidc_configuration_dict
|
||||
)
|
||||
mock_get.return_value = oidc_config
|
||||
|
||||
proxy = OIDCProxy(
|
||||
config_url=TEST_CONFIG_URL,
|
||||
client_id=TEST_CLIENT_ID,
|
||||
client_secret=TEST_CLIENT_SECRET,
|
||||
base_url=TEST_BASE_URL,
|
||||
jwt_signing_key="test-secret",
|
||||
verify_id_token=True,
|
||||
)
|
||||
|
||||
# Same JWT for both access_token and id_token — the scenario
|
||||
# that triggered the bug.
|
||||
same_jwt = "eyJhbGciOiJSUzI1NiJ9.identical-token"
|
||||
token_set = UpstreamTokenSet(
|
||||
upstream_token_id="test-id",
|
||||
access_token=same_jwt,
|
||||
refresh_token=None,
|
||||
refresh_token_expires_at=None,
|
||||
expires_at=9999999999.0,
|
||||
token_type="Bearer",
|
||||
scope="openid offline_access",
|
||||
client_id="test-client",
|
||||
created_at=1000000000.0,
|
||||
raw_token_data={
|
||||
"access_token": same_jwt,
|
||||
"id_token": same_jwt,
|
||||
},
|
||||
)
|
||||
|
||||
# _uses_alternate_verification should be True regardless of
|
||||
# token value equality
|
||||
assert proxy._uses_alternate_verification() is True
|
||||
# _get_verification_token returns the id_token (same value)
|
||||
assert proxy._get_verification_token(token_set) == same_jwt
|
||||
# The key point: even though the tokens are equal, the intent
|
||||
# flag ensures load_access_token will patch scopes
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue