Fix Azure OAuth token refresh with unprefixed scopes#2462
Fix Azure OAuth token refresh with unprefixed scopes#2462jlowin merged 5 commits intoPrefectHQ:mainfrom
Conversation
WalkthroughA new hook method Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/fastmcp/server/auth/providers/azure.py (1)
360-400: Azure-specific scope transformation for refresh is correct; consider minor hardeningThe logic here matches the design goal:
- Treats
scopesas unprefixed client/base scopes from storage.- Strips out any
additional_authorize_scopesthat may have leaked into storage.- Prefixes remaining client scopes with
identifier_uriunless they already look fully qualified.- Re-appends
additional_authorize_scopesso Azure still receives Graph/OIDC scopes on refresh.- Leaves stored scopes unchanged, so client-facing behavior remains backward compatible.
This should fix the invalid_grant on Azure refresh while preventing unbounded scope growth.
Two optional refinements you might consider (non-blocking):
- Deduplicate
prefixed_scopesbefore returning, in case older tokens already contain prefixed variants (avoids redundant entries in thescopestring).- Centralize the prefixing heuristic in a small helper shared with
_build_upstream_authorize_urlso future Azure scope rules stay in sync across authorize and refresh paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/fastmcp/server/auth/oauth_proxy.py(2 hunks)src/fastmcp/server/auth/providers/azure.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/fastmcp/server/auth/oauth_proxy.py (1)
src/fastmcp/server/auth/providers/azure.py (1)
_prepare_scopes_for_upstream_refresh(360-400)
src/fastmcp/server/auth/providers/azure.py (1)
src/fastmcp/server/auth/oauth_proxy.py (1)
_prepare_scopes_for_upstream_refresh(1276-1291)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: label-issue-or-pr
🔇 Additional comments (3)
src/fastmcp/server/auth/providers/azure.py (1)
9-13: Imports for type annotations are consistent with usageThe added imports (
Any,AuthorizationParams,OAuthClientInformationFull) match howauthorize()is typed later in the file and improve clarity without changing behavior.src/fastmcp/server/auth/oauth_proxy.py (2)
1276-1292: Hook for provider-specific refresh scopes is well-scoped and backward compatibleThe new
_prepare_scopes_for_upstream_refreshhook is clearly documented, and returningscopesunchanged in the base implementation preserves existing behavior for all current providers. It gives derived providers (like Azure) a precise interception point without altering the contract ofexchange_refresh_token.
1353-1357: Using transformed scopes only for the upstream call cleanly isolates provider behaviorDeriving
upstream_scopesvia_prepare_scopes_for_upstream_refresh(scopes)and using only those for the upstreamscopeparameter while keeping the originalscopesfor FastMCP-issued tokens achieves the intended separation:
- Providers can adjust what Azure/others see during refresh (e.g., prefix + add Graph/OIDC).
- Stored/advertised scopes for MCP clients remain the base, untransformed set.
This wires the hook into the refresh flow without regressing other providers.
Also applies to: 1363-1363
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/fastmcp/server/auth/providers/azure.py (1)
394-394: Use lazy logging for better performance.The f-string formatting in debug log statements evaluates even when debug logging is disabled, creating unnecessary overhead.
Apply this diff to use lazy evaluation:
- logger.debug(f"Base scopes from storage: {scopes}") + logger.debug("Base scopes from storage: %s", scopes)And:
- logger.debug(f"Scopes for Azure token endpoint: {deduplicated_scopes}") + logger.debug("Scopes for Azure token endpoint: %s", deduplicated_scopes)Also applies to: 413-413
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/server/auth/providers/test_azure.pyis excluded by none and included by none
📒 Files selected for processing (1)
src/fastmcp/server/auth/providers/azure.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/fastmcp/server/auth/providers/azure.py (1)
src/fastmcp/server/auth/oauth_proxy.py (1)
_prepare_scopes_for_upstream_refresh(1276-1291)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run tests: Python 3.10 on windows-latest
🔇 Additional comments (3)
src/fastmcp/server/auth/providers/azure.py (3)
330-350: LGTM! Clean scope prefixing logic.The helper method correctly centralizes scope prefixing for both authorization and refresh flows. The detection logic using
"://" in scope or "/" in scopeappropriately handles Azure's scope formats:
- Fully-qualified scopes like
api://xxx/readare preserved- Unprefixed scopes like
readget prefixed with identifier_uri- Already-prefixed scopes from migrations are handled correctly
The approach relies on Azure's well-defined scope format and the provider's documented requirement for unprefixed scope names in
required_scopes.
364-364: Good refactoring to use centralized helper.Using
_prefix_scopes_for_azureimproves maintainability by ensuring consistent scope prefixing logic across authorization and refresh flows.
377-414: Excellent fix for Azure token refresh scope handling.The implementation correctly addresses the AADSTS65001 error by:
- Defensive filtering (lines 398-399): Cleans up any accidentally stored additional_authorize_scopes
- Dynamic prefixing (line 402): Transforms base scopes to fully-qualified format for Azure
- Scope augmentation (lines 406-407): Includes Graph/OIDC scopes in the refresh request
- Deduplication (line 411): Handles legacy tokens with duplicate scopes efficiently
The approach maintains backward compatibility by keeping stored scopes unprefixed while transforming them only when sending to Azure. This prevents scope accumulation and aligns with Azure AD v2.0 requirements.
Test Failure AnalysisSummary: The Windows test job failed due to a pytest-xdist worker crash during the Root Cause: The worker crash is triggered by a This occurs at ~35% test completion when the test spawns multiple Python subprocess-based MCP servers using Why This Is Not PR-Related:
Known Windows Issues: The codebase already acknowledges Windows process lifecycle issues:
Suggested Solution: Option 1 (Recommended): Mark @pytest.mark.skipif(
sys.platform.startswith("win32"),
reason="Windows has process lifecycle issues with pytest-xdist workers"
)
async def test_multi_client_with_transforms(tmp_path: Path):
...Option 2: Mark the test as flaky to allow retries: @pytest.mark.flaky(retries=3)
async def test_multi_client_with_transforms(tmp_path: Path):
...(Note: Option 3: Investigate the underlying Windows subprocess cleanup issue, but this is a larger effort that should be tracked separately from this PR. Detailed AnalysisRelevant Log ExcerptsThe failure shows the worker crashed during test execution: The underlying cause is a connection reset in pytest-retry: Test StructureThe failing test:
This multi-process subprocess pattern with Related Files
|
Azure OAuth token refresh was failing because the proxy was sending unprefixed scopes (e.g., read) during refresh, which is inconsistent with how we process initial authentication token exchange. Azure AD requires fully-qualified scopes (e.g.,
api://xxx/read) for both authorization and token refresh.The Problem
After successful initial authorization, the first token refresh would fail with:
The issue: during initial authorization, scopes were correctly prefixed (api://xxx/read User.Read openid profile offline_access), but during refresh, only the unprefixed base scope (read) was sent.
The Solution
Introduced a hook method
_prepare_scopes_for_upstream_refresh()in OAuthProxy that allows provider-specific scope transformation before sending to upstream token endpoints, while keeping base scopes unprefixed in storage.The Azure provider now overrides this to:
additional_authorize_scopes(e.g., User.Read, openid, profile, offline_access)additional_authorize_scopeswill show in stored token's base scopes next time and the scope string will grow longer and longer).This approach maintains backward compatibility—unprefixed scopes remain stored in tokens, and prefixing happens dynamically only when sending to Azure.
Contributors Checklist
Review Checklist