From 119bf445ea8468542d21e1875ee9116cfa4a8e2d Mon Sep 17 00:00:00 2001 From: Nestor Qin Date: Thu, 20 Nov 2025 15:16:50 -0800 Subject: [PATCH] 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