Skip to content

fix: correctly send resource when exchanging code for the upstream to…#3013

Merged
jlowin merged 2 commits intoPrefectHQ:mainfrom
JonasKs:fix/azure-token-exchange-scopes
Jan 29, 2026
Merged

fix: correctly send resource when exchanging code for the upstream to…#3013
jlowin merged 2 commits intoPrefectHQ:mainfrom
JonasKs:fix/azure-token-exchange-scopes

Conversation

@JonasKs
Copy link
Copy Markdown
Contributor

@JonasKs JonasKs commented Jan 28, 2026

As explained here, if an MCP server wants to use OBO flow for multiple resources, and use consent flow, this does not work today.

This PR fixes this by correctly sending the correct scopes when exchanging auth code for the upstream token.

Note: Tests have been written by Claude Sonnet4.5 and reviewed by codex and gemini.

Contributors Checklist

  • My change closes comment
  • I have followed the repository's development workflow
  • I have tested my changes manually and by adding relevant tests
  • I have performed all required documentation updates

Review Checklist

  • I have self-reviewed my changes
  • My Pull Request is ready for review

@marvin-context-protocol marvin-context-protocol Bot added bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. auth Related to authentication (Bearer, JWT, OAuth, WorkOS) for client or server. server Related to FastMCP server implementation or server-side functionality. labels Jan 28, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d7f52dddc3

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/fastmcp/server/auth/providers/azure.py Outdated
@marvin-context-protocol
Copy link
Copy Markdown
Contributor

marvin-context-protocol Bot commented Jan 28, 2026

Test Failure Analysis

Summary: Static analysis failed due to type checking errors in files unrelated to this PR's changes.

Root Cause: The type checker () found legitimate type errors in src/fastmcp/utilities/skills.py where hasattr() checks don't properly narrow union types. When accessing .text on a union type TextResourceContents | BlobResourceContents, the type checker infers object instead of str because hasattr() doesn't provide type narrowing.

Suggested Solution: These errors exist on main and are not introduced by this PR. The PR only modifies Azure authentication files. The type errors should be fixed in a separate PR:

  1. Fix src/fastmcp/utilities/skills.py - Use isinstance() checks instead of hasattr():

    # Line 106: Instead of hasattr(content, "text")
    if isinstance(content, mcp.types.TextResourceContents):
        manifest_data = json.loads(content.text)
    
    # Line 201: Same pattern
    if isinstance(content, mcp.types.TextResourceContents):
        file_path.write_text(content.text)
    elif isinstance(content, mcp.types.BlobResourceContents):
        # Handle blob...
  2. Fix tests/server/test_dependencies.py - Remove unused # type: ignore[assignment] comments on lines 70 and 201 (they're no longer needed).

This PR should be mergeable - The failures are pre-existing issues on main, not regressions from these changes.

Detailed Analysis

CI Errors Reported:

  1. src/fastmcp/utilities/skills.py:106 - content.text typed as object instead of str
  2. src/fastmcp/utilities/skills.py:201 - Same issue with content.text
  3. tests/server/test_dependencies.py:70,201 - Unused type ignore comments

Why These Aren't False Positives:

The type checker is correctly identifying that when you use hasattr(content, 'text') on a union type TextResourceContents | BlobResourceContents, Python's type system doesn't narrow the type. The .text attribute access returns type object, not str.

Why This PR Isn't at Fault:

  • ✅ This PR only modifies: oauth_proxy/proxy.py, providers/azure.py, and test_azure.py
  • ✅ The failing files (skills.py, test_dependencies.py) are unchanged by this PR
  • ✅ The errors exist on main branch (can be verified by checking out main)

Why Tests Pass Locally:

Type checking may pass locally with cached/incremental checking, but --all-files in CI runs a fresh check that catches these issues consistently.

Related Files

Files modified in this PR (all Azure auth related):

  • src/fastmcp/server/auth/oauth_proxy/proxy.py - Added scope preparation hooks
  • src/fastmcp/server/auth/providers/azure.py - Implemented Azure scope handling
  • tests/server/auth/providers/test_azure.py - Tests for scope handling

Files with pre-existing type errors (not in this PR):

  • src/fastmcp/utilities/skills.py:106,201 - Need isinstance() instead of hasattr()
  • tests/server/test_dependencies.py:70,201 - Need to remove unused type ignore comments

Updated: 2026-01-28 - Corrected analysis after further investigation. Previous comment incorrectly attributed errors to stale cache.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 28, 2026

Walkthrough

Introduces a hook to prepare scopes used for the authorization-code → token exchange: OAuthProxy adds a private _prepare_scopes_for_token_exchange(self, scopes: list[str]) -> list[str] and uses it during IdP callback to set the upstream scope when non-empty. AzureProvider implements _prepare_scopes_for_token_exchange to prefix resource scopes with the Azure identifier URI, include only OIDC/Graph scopes from additional authorize scopes, deduplicate the final list, and log the result. Upstream authorize/refresh scope preparation was adjusted to align with Azure token rules.

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main fix: correctly sending the resource when exchanging code for the upstream token, which aligns with the core objective of the changeset.
Description check ✅ Passed The description covers the issue, the solution, testing approach, and all contributor checklist items are marked complete. It references the related GitHub issue and provides sufficient context.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@JonasKs JonasKs marked this pull request as draft January 28, 2026 12:37
@JonasKs JonasKs marked this pull request as ready for review January 28, 2026 13:37
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 398f88f405

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/fastmcp/server/auth/providers/azure.py
@JonasKs
Copy link
Copy Markdown
Contributor Author

JonasKs commented Jan 29, 2026

I've resolved the comments and this is ready for review @jlowin. Let me know if you want anything changed or more information. :)

…ken, allowing for consent flow with multiple resources

# Conflicts:
#	tests/server/auth/providers/test_azure.py
@JonasKs JonasKs force-pushed the fix/azure-token-exchange-scopes branch from 37e5b4f to 132d32e Compare January 29, 2026 12:06
result = provider._prepare_scopes_for_upstream_refresh([])

assert "User.Read" in result
assert "User.Read" not in result # Not OIDC
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct btw, not an LLM oversight. We only want scopes for the appreg resource when we fetch or refresh tokens.

@JonasKs
Copy link
Copy Markdown
Contributor Author

JonasKs commented Jan 29, 2026

I've rebased main to resolve all the conflicts.

@jlowin
Copy link
Copy Markdown
Member

jlowin commented Jan 29, 2026

Thanks @JonasKs! FWIW my claude agreed with your pushback on codex but apparently I didn't post that comment though I thought I did.

@jlowin jlowin merged commit 46e0e01 into PrefectHQ:main Jan 29, 2026
9 checks passed
@JonasKs
Copy link
Copy Markdown
Contributor Author

JonasKs commented Jan 29, 2026

@jlowin , I love when Claude agrees with me!!

Thanks for quick reviews and merging everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auth Related to authentication (Bearer, JWT, OAuth, WorkOS) for client or server. bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. server Related to FastMCP server implementation or server-side functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants