From 9e28da11f2e9c3be32a9ae11111b17d7774e846d Mon Sep 17 00:00:00 2001 From: Nestor Qin Date: Wed, 19 Nov 2025 19:02:31 +0000 Subject: [PATCH 1/4] fix Azure token refresh issue --- src/fastmcp/server/auth/oauth_proxy.py | 24 ++++++++++- src/fastmcp/server/auth/providers/azure.py | 50 +++++++++++++++++++--- 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/src/fastmcp/server/auth/oauth_proxy.py b/src/fastmcp/server/auth/oauth_proxy.py index f8babfa2e..c526ddcd3 100644 --- a/src/fastmcp/server/auth/oauth_proxy.py +++ b/src/fastmcp/server/auth/oauth_proxy.py @@ -1273,6 +1273,23 @@ class OAuthProxy(OAuthProvider): # Refresh Token Flow # ------------------------------------------------------------------------- + def _prepare_scopes_for_upstream_refresh(self, scopes: list[str]) -> list[str]: + """Prepare scopes for upstream token refresh request. + + Override this method to transform scopes before sending to upstream provider. + For example, Azure needs to prefix scopes and add additional Graph scopes. + + The scopes parameter represents what should be stored in the RefreshToken. + This method returns what should be sent to the upstream provider. + + Args: + scopes: Base scopes that will be stored in RefreshToken + + Returns: + Scopes to send to upstream provider (may be transformed/augmented) + """ + return scopes + async def load_refresh_token( self, client: OAuthClientInformationFull, @@ -1333,12 +1350,17 @@ class OAuthProxy(OAuthProvider): timeout=HTTP_TIMEOUT_SECONDS, ) + # Allow child classes to transform scopes before sending to upstream + # This enables provider-specific scope formatting (e.g., Azure prefixing) + # while keeping original scopes in storage + upstream_scopes = self._prepare_scopes_for_upstream_refresh(scopes) + try: logger.debug("Refreshing upstream token (jti=%s)", refresh_jti[:8]) token_response: dict[str, Any] = await oauth_client.refresh_token( # type: ignore[misc] url=self._upstream_token_endpoint, refresh_token=upstream_token_set.refresh_token, - scope=" ".join(scopes) if scopes else None, + scope=" ".join(upstream_scopes) if upstream_scopes else None, **self._extra_token_params, ) logger.debug("Successfully refreshed upstream token") diff --git a/src/fastmcp/server/auth/providers/azure.py b/src/fastmcp/server/auth/providers/azure.py index 4c7cb8359..e206ed6ec 100644 --- a/src/fastmcp/server/auth/providers/azure.py +++ b/src/fastmcp/server/auth/providers/azure.py @@ -6,9 +6,11 @@ using the OAuth Proxy pattern for non-DCR OAuth flows. from __future__ import annotations -from typing import TYPE_CHECKING, Any +from typing import Any from key_value.aio.protocols import AsyncKeyValue +from mcp.server.auth.provider import AuthorizationParams +from mcp.shared.auth import OAuthClientInformationFull from pydantic import SecretStr, field_validator from pydantic_settings import BaseSettings, SettingsConfigDict @@ -19,10 +21,6 @@ from fastmcp.utilities.auth import parse_scopes from fastmcp.utilities.logging import get_logger from fastmcp.utilities.types import NotSet, NotSetT -if TYPE_CHECKING: - from mcp.server.auth.provider import AuthorizationParams - from mcp.shared.auth import OAuthClientInformationFull - logger = get_logger(__name__) @@ -358,3 +356,45 @@ class AzureProvider(OAuthProxy): # Let parent build the URL with prefixed scopes return super()._build_upstream_authorize_url(txn_id, modified_transaction) + + def _prepare_scopes_for_upstream_refresh(self, scopes: list[str]) -> list[str]: + """Prepare scopes for Azure token refresh. + + Azure requires: + 1. Fully-qualified custom scopes (e.g., "api://xxx/read" not "read") + 2. Microsoft Graph scopes (e.g., "User.Read", "openid") sent as-is + 3. Additional scopes from provider config (additional_authorize_scopes) + + This method transforms base client scopes for Azure while keeping them + unprefixed in storage to prevent accumulation. + + Args: + scopes: Base scopes from RefreshToken (unprefixed, e.g., ["read"]) + + Returns: + Scopes formatted for Azure token endpoint + """ + logger.debug(f"Base scopes from storage: {scopes}") + + # Filter out any additional_authorize_scopes that may have been stored + # (they shouldn't be in storage, but clean them up if they are) + additional_scopes_set = set(self.additional_authorize_scopes or []) + base_scopes = [s for s in scopes if s not in additional_scopes_set] + + # Prefix base scopes with identifier_uri for Azure + prefixed_scopes = [] + for scope in base_scopes: + if "://" in scope or "/" in scope: + # Already fully-qualified (e.g., "api://xxx/read") + prefixed_scopes.append(scope) + else: + # Unprefixed client scope - prefix with identifier_uri + prefixed_scopes.append(f"{self.identifier_uri}/{scope}") + + # Add additional scopes (Graph + OIDC) for the Azure request + # These are NOT stored in RefreshToken, only sent to Azure + if self.additional_authorize_scopes: + prefixed_scopes.extend(self.additional_authorize_scopes) + + logger.debug(f"Scopes for Azure token endpoint: {prefixed_scopes}") + return prefixed_scopes From 463652a878bc7dc57801e484fd5ef8524498580b Mon Sep 17 00:00:00 2001 From: Nestor Qin Date: Thu, 20 Nov 2025 15:08:50 -0800 Subject: [PATCH 2/4] Add unit tests --- src/fastmcp/server/auth/providers/azure.py | 8 +- tests/server/auth/providers/test_azure.py | 173 +++++++++++++++++++++ 2 files changed, 178 insertions(+), 3 deletions(-) diff --git a/src/fastmcp/server/auth/providers/azure.py b/src/fastmcp/server/auth/providers/azure.py index e206ed6ec..42b252eb2 100644 --- a/src/fastmcp/server/auth/providers/azure.py +++ b/src/fastmcp/server/auth/providers/azure.py @@ -6,11 +6,9 @@ using the OAuth Proxy pattern for non-DCR OAuth flows. from __future__ import annotations -from typing import Any +from typing import TYPE_CHECKING, Any from key_value.aio.protocols import AsyncKeyValue -from mcp.server.auth.provider import AuthorizationParams -from mcp.shared.auth import OAuthClientInformationFull from pydantic import SecretStr, field_validator from pydantic_settings import BaseSettings, SettingsConfigDict @@ -21,6 +19,10 @@ from fastmcp.utilities.auth import parse_scopes from fastmcp.utilities.logging import get_logger from fastmcp.utilities.types import NotSet, NotSetT +if TYPE_CHECKING: + from mcp.server.auth.provider import AuthorizationParams + from mcp.shared.auth import OAuthClientInformationFull + logger = get_logger(__name__) diff --git a/tests/server/auth/providers/test_azure.py b/tests/server/auth/providers/test_azure.py index 168384eb0..894cd7531 100644 --- a/tests/server/auth/providers/test_azure.py +++ b/tests/server/auth/providers/test_azure.py @@ -487,3 +487,176 @@ class TestAzureProvider: parsed = urlparse(provider._upstream_authorization_endpoint) assert parsed.netloc == "login.microsoftonline.us" assert "/organizations/" in parsed.path + + def test_prepare_scopes_for_upstream_refresh_basic_prefixing(self): + """Test that unprefixed scopes are correctly prefixed for Azure token refresh.""" + provider = AzureProvider( + client_id="test_client", + client_secret="test_secret", + tenant_id="test-tenant", + identifier_uri="api://my-api", + required_scopes=["read", "write"], + jwt_signing_key="test-secret", + ) + + # Unprefixed scopes from storage should be prefixed + result = provider._prepare_scopes_for_upstream_refresh(["read", "write"]) + + assert "api://my-api/read" in result + assert "api://my-api/write" in result + assert len(result) == 2 + + def test_prepare_scopes_for_upstream_refresh_already_prefixed(self): + """Test that already-prefixed scopes remain unchanged.""" + provider = AzureProvider( + client_id="test_client", + client_secret="test_secret", + tenant_id="test-tenant", + identifier_uri="api://my-api", + required_scopes=["read"], + jwt_signing_key="test-secret", + ) + + # Already prefixed scopes should pass through unchanged + result = provider._prepare_scopes_for_upstream_refresh( + ["api://my-api/read", "api://other-api/admin"] + ) + + assert "api://my-api/read" in result + assert "api://other-api/admin" in result + assert len(result) == 2 + + def test_prepare_scopes_for_upstream_refresh_with_additional_scopes(self): + """Test that additional_authorize_scopes are added during token refresh.""" + provider = AzureProvider( + client_id="test_client", + client_secret="test_secret", + tenant_id="test-tenant", + identifier_uri="api://my-api", + required_scopes=["read"], + additional_authorize_scopes=[ + "User.Read", + "openid", + "profile", + "offline_access", + ], + jwt_signing_key="test-secret", + ) + + # Base scopes should be prefixed, additional scopes appended + result = provider._prepare_scopes_for_upstream_refresh(["read", "write"]) + + assert "api://my-api/read" in result + assert "api://my-api/write" in result + assert "User.Read" in result + assert "openid" in result + assert "profile" in result + assert "offline_access" in result + assert len(result) == 6 + + def test_prepare_scopes_for_upstream_refresh_filters_duplicate_additional_scopes( + self, + ): + """Test that accidentally stored additional_authorize_scopes are filtered out.""" + provider = AzureProvider( + client_id="test_client", + client_secret="test_secret", + tenant_id="test-tenant", + identifier_uri="api://my-api", + required_scopes=["read"], + additional_authorize_scopes=["User.Read", "openid"], + jwt_signing_key="test-secret", + ) + + # If additional scopes were accidentally stored, they should be filtered + # to prevent accumulation + result = provider._prepare_scopes_for_upstream_refresh( + ["read", "User.Read", "openid"] + ) + + # Should have: api://my-api/read (prefixed) + User.Read + openid (added once) + assert "api://my-api/read" in result + assert result.count("User.Read") == 1 + assert result.count("openid") == 1 + assert len(result) == 3 + + def test_prepare_scopes_for_upstream_refresh_mixed_scopes(self): + """Test mixed scenario with both prefixed and unprefixed scopes.""" + provider = AzureProvider( + client_id="test_client", + client_secret="test_secret", + tenant_id="test-tenant", + identifier_uri="api://my-api", + required_scopes=["read"], + additional_authorize_scopes=["User.Read"], + jwt_signing_key="test-secret", + ) + + # Mix of prefixed and unprefixed scopes + result = provider._prepare_scopes_for_upstream_refresh( + ["read", "api://other-api/admin", "write"] + ) + + assert "api://my-api/read" in result + assert "api://other-api/admin" in result # Already prefixed, unchanged + assert "api://my-api/write" in result + assert "User.Read" in result + assert len(result) == 4 + + def test_prepare_scopes_for_upstream_refresh_scope_with_slash(self): + """Test that scopes containing '/' are not prefixed.""" + provider = AzureProvider( + client_id="test_client", + client_secret="test_secret", + tenant_id="test-tenant", + identifier_uri="api://my-api", + required_scopes=["read"], + jwt_signing_key="test-secret", + ) + + # Scopes with "/" should not be prefixed (already fully qualified) + result = provider._prepare_scopes_for_upstream_refresh( + ["read", "https://graph.microsoft.com/.default"] + ) + + assert "api://my-api/read" in result + assert ( + "https://graph.microsoft.com/.default" in result + ) # Not prefixed (contains ://) + + def test_prepare_scopes_for_upstream_refresh_empty_scopes(self): + """Test behavior with empty scopes list.""" + provider = AzureProvider( + client_id="test_client", + client_secret="test_secret", + tenant_id="test-tenant", + identifier_uri="api://my-api", + required_scopes=["read"], + additional_authorize_scopes=["User.Read", "openid"], + jwt_signing_key="test-secret", + ) + + # Empty scopes should still add additional_authorize_scopes + result = provider._prepare_scopes_for_upstream_refresh([]) + + assert "User.Read" in result + assert "openid" in result + assert len(result) == 2 + + def test_prepare_scopes_for_upstream_refresh_no_additional_scopes(self): + """Test behavior when no additional_authorize_scopes are configured.""" + provider = AzureProvider( + client_id="test_client", + client_secret="test_secret", + tenant_id="test-tenant", + identifier_uri="api://my-api", + required_scopes=["read"], + jwt_signing_key="test-secret", + ) + + # Should only prefix base scopes, no additional scopes added + result = provider._prepare_scopes_for_upstream_refresh(["read", "write"]) + + assert "api://my-api/read" in result + assert "api://my-api/write" in result + assert len(result) == 2 From 119bf445ea8468542d21e1875ee9116cfa4a8e2d Mon Sep 17 00:00:00 2001 From: Nestor Qin Date: Thu, 20 Nov 2025 15:16:50 -0800 Subject: [PATCH 3/4] Resolve comments --- src/fastmcp/server/auth/providers/azure.py | 52 +++++++++++++--------- tests/server/auth/providers/test_azure.py | 49 ++++++++++++++++++++ 2 files changed, 81 insertions(+), 20 deletions(-) diff --git a/src/fastmcp/server/auth/providers/azure.py b/src/fastmcp/server/auth/providers/azure.py index 42b252eb2..035fe59b9 100644 --- a/src/fastmcp/server/auth/providers/azure.py +++ b/src/fastmcp/server/auth/providers/azure.py @@ -327,6 +327,28 @@ class AzureProvider(OAuthProxy): separator = "&" if "?" in auth_url else "?" return f"{auth_url}{separator}prompt=select_account" + def _prefix_scopes_for_azure(self, scopes: list[str]) -> list[str]: + """Prefix unprefixed scopes with identifier_uri for Azure. + + This helper centralizes the scope prefixing logic used in both + authorization and token refresh flows. + + Args: + scopes: List of scopes, may be prefixed or unprefixed + + Returns: + List of scopes with identifier_uri prefix applied where needed + """ + prefixed = [] + for scope in scopes: + if "://" in scope or "/" in scope: + # Already fully-qualified (e.g., "api://xxx/read" or "User.Read") + prefixed.append(scope) + else: + # Unprefixed client scope - prefix with identifier_uri + prefixed.append(f"{self.identifier_uri}/{scope}") + return prefixed + def _build_upstream_authorize_url( self, txn_id: str, transaction: dict[str, Any] ) -> str: @@ -339,14 +361,7 @@ class AzureProvider(OAuthProxy): unprefixed_scopes = transaction.get("scopes") or self.required_scopes or [] # Prefix scopes for Azure authorization request - prefixed_scopes = [] - for scope in unprefixed_scopes: - if "://" in scope or "/" in scope: - # Already a full URI or path (e.g., "api://xxx/read" or "User.Read") - prefixed_scopes.append(scope) - else: - # Unprefixed scope name - prefix it with identifier_uri - prefixed_scopes.append(f"{self.identifier_uri}/{scope}") + prefixed_scopes = self._prefix_scopes_for_azure(unprefixed_scopes) # Add Microsoft Graph scopes (not validated, not prefixed) if self.additional_authorize_scopes: @@ -374,7 +389,7 @@ class AzureProvider(OAuthProxy): scopes: Base scopes from RefreshToken (unprefixed, e.g., ["read"]) Returns: - Scopes formatted for Azure token endpoint + Deduplicated list of scopes formatted for Azure token endpoint """ logger.debug(f"Base scopes from storage: {scopes}") @@ -383,20 +398,17 @@ class AzureProvider(OAuthProxy): additional_scopes_set = set(self.additional_authorize_scopes or []) base_scopes = [s for s in scopes if s not in additional_scopes_set] - # Prefix base scopes with identifier_uri for Azure - prefixed_scopes = [] - for scope in base_scopes: - if "://" in scope or "/" in scope: - # Already fully-qualified (e.g., "api://xxx/read") - prefixed_scopes.append(scope) - else: - # Unprefixed client scope - prefix with identifier_uri - prefixed_scopes.append(f"{self.identifier_uri}/{scope}") + # Prefix base scopes with identifier_uri for Azure using shared helper + prefixed_scopes = self._prefix_scopes_for_azure(base_scopes) # Add additional scopes (Graph + OIDC) for the Azure request # These are NOT stored in RefreshToken, only sent to Azure if self.additional_authorize_scopes: prefixed_scopes.extend(self.additional_authorize_scopes) - logger.debug(f"Scopes for Azure token endpoint: {prefixed_scopes}") - return prefixed_scopes + # Deduplicate while preserving order (in case older tokens have duplicates) + # Use dict.fromkeys() for O(n) deduplication with order preservation + deduplicated_scopes = list(dict.fromkeys(prefixed_scopes)) + + logger.debug(f"Scopes for Azure token endpoint: {deduplicated_scopes}") + return deduplicated_scopes diff --git a/tests/server/auth/providers/test_azure.py b/tests/server/auth/providers/test_azure.py index 894cd7531..70098725a 100644 --- a/tests/server/auth/providers/test_azure.py +++ b/tests/server/auth/providers/test_azure.py @@ -660,3 +660,52 @@ class TestAzureProvider: assert "api://my-api/read" in result assert "api://my-api/write" in result assert len(result) == 2 + + def test_prepare_scopes_for_upstream_refresh_deduplicates_scopes(self): + """Test that duplicate scopes are deduplicated while preserving order.""" + provider = AzureProvider( + client_id="test_client", + client_secret="test_secret", + tenant_id="test-tenant", + identifier_uri="api://my-api", + required_scopes=["read"], + additional_authorize_scopes=["User.Read", "openid"], + jwt_signing_key="test-secret", + ) + + # Test with duplicate base scopes and duplicate additional scopes + result = provider._prepare_scopes_for_upstream_refresh( + ["read", "write", "read", "User.Read", "openid"] + ) + + # Should have deduplicated results in order + assert result == [ + "api://my-api/read", + "api://my-api/write", + "User.Read", + "openid", + ] + assert len(result) == 4 + + def test_prepare_scopes_for_upstream_refresh_deduplicates_prefixed_variants(self): + """Test that both prefixed and unprefixed variants are deduplicated.""" + provider = AzureProvider( + client_id="test_client", + client_secret="test_secret", + tenant_id="test-tenant", + identifier_uri="api://my-api", + required_scopes=["read"], + jwt_signing_key="test-secret", + ) + + # Test with both prefixed and unprefixed variants of same scope + result = provider._prepare_scopes_for_upstream_refresh( + ["read", "api://my-api/read", "write"] + ) + + # Should deduplicate - first occurrence wins (api://my-api/read from "read") + assert "api://my-api/read" in result + assert "api://my-api/write" in result + # Should only have 2 items (read processed twice, but deduplicated) + assert len(result) == 2 + assert result.count("api://my-api/read") == 1 From 2852f2cb1335b1bced8e50786859b4340b6e4122 Mon Sep 17 00:00:00 2001 From: Nestor Qin Date: Thu, 20 Nov 2025 15:31:40 -0800 Subject: [PATCH 4/4] Use lazy logging --- src/fastmcp/server/auth/providers/azure.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/fastmcp/server/auth/providers/azure.py b/src/fastmcp/server/auth/providers/azure.py index 035fe59b9..c216e351a 100644 --- a/src/fastmcp/server/auth/providers/azure.py +++ b/src/fastmcp/server/auth/providers/azure.py @@ -391,7 +391,7 @@ class AzureProvider(OAuthProxy): Returns: Deduplicated list of scopes formatted for Azure token endpoint """ - logger.debug(f"Base scopes from storage: {scopes}") + logger.debug("Base scopes from storage: %s", scopes) # Filter out any additional_authorize_scopes that may have been stored # (they shouldn't be in storage, but clean them up if they are) @@ -410,5 +410,5 @@ class AzureProvider(OAuthProxy): # Use dict.fromkeys() for O(n) deduplication with order preservation deduplicated_scopes = list(dict.fromkeys(prefixed_scopes)) - logger.debug(f"Scopes for Azure token endpoint: {deduplicated_scopes}") + logger.debug("Scopes for Azure token endpoint: %s", deduplicated_scopes) return deduplicated_scopes