Added Provider for OCI IAM service. #2317
Conversation
WalkthroughAdds an OCI OIDC authentication provider: new Changes
Sequence Diagram(s)sequenceDiagram
participant App as FastMCP App
participant OCIProv as OCIProvider
participant Settings as OCIProviderSettings
participant OIDC as OIDCProxy
Note over App,OCIProv: Provider instantiation
App->>OCIProv: instantiate OCIProvider(...)
OCIProv->>Settings: construct settings (env FASTMCP_SERVER_AUTH_OCI_)
Settings-->>OCIProv: validated settings (scopes parsed)
OCIProv->>OIDC: super().__init__(mapped values, client_secret plain)
OIDC-->>OCIProv: initialized
OCIProv-->>App: ready (logs client_id & scopes)
Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
marvin-context-protocol
bot
added
enhancement
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/fastmcp/server/auth/providers/ociprovider.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use Python ≥ 3.10 and provide full type annotations for library code
Files:
src/fastmcp/server/auth/providers/ociprovider.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Never use bare except; always catch specific exception types
Files:
src/fastmcp/server/auth/providers/ociprovider.py
🧬 Code graph analysis (1)
src/fastmcp/server/auth/providers/ociprovider.py (3)
src/fastmcp/server/auth/oidc_proxy.py (1)
OIDCProxy(172-384)src/fastmcp/utilities/auth.py (1)
parse_scopes(9-34)src/fastmcp/utilities/logging.py (1)
get_logger(14-26)
🪛 GitHub Actions: Run static analysis
src/fastmcp/server/auth/providers/ociprovider.py
[error] 97-97: B006 Do not use mutable data structures for argument defaults. Replace with None; initialize within function. (ruff-check)
⏰ 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). (3)
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Run tests with lowest-direct dependencies
- GitHub Check: label-issue-or-pr
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/fastmcp/server/auth/providers/ociprovider.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use Python ≥ 3.10 and provide full type annotations for library code
Files:
src/fastmcp/server/auth/providers/ociprovider.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Never use bare except; always catch specific exception types
Files:
src/fastmcp/server/auth/providers/ociprovider.py
🧬 Code graph analysis (1)
src/fastmcp/server/auth/providers/ociprovider.py (3)
src/fastmcp/server/auth/oidc_proxy.py (1)
OIDCProxy(172-384)src/fastmcp/utilities/auth.py (1)
parse_scopes(9-34)src/fastmcp/utilities/logging.py (1)
get_logger(14-26)
⏰ 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 with lowest-direct dependencies
- GitHub Check: Run tests: Python 3.10 on windows-latest
🔇 Additional comments (3)
src/fastmcp/server/auth/providers/ociprovider.py (3)
1-35: LGTM! Well-structured module with clear documentation.The module docstring provides a clear example, imports are well-organized, and the logger follows FastMCP conventions.
119-157: LGTM! Solid validation logic with helpful error messages.The settings construction correctly filters out
NotSetvalues to allow environment variable fallbacks, and the required field validation provides clear guidance on how to set missing values.
158-179: LGTM! Clean initialization with proper value mapping.The parent constructor receives correctly mapped values, including secure extraction of the client secret and proper handling of the jwt_signing_key (which correctly supports both str and bytes). The debug logging provides useful information for troubleshooting.
| @field_validator("required_scopes", mode="before") | ||
| @classmethod | ||
| def _parse_scopes(cls, v): | ||
| return parse_scopes(v) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add return type annotation to the validator.
For library code, full type annotations improve type safety and code clarity. The validator should explicitly annotate its return type.
Apply this diff:
@field_validator("required_scopes", mode="before")
@classmethod
- def _parse_scopes(cls, v):
+ def _parse_scopes(cls, v) -> list[str] | None:
return parse_scopes(v)As per coding guidelines
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @field_validator("required_scopes", mode="before") | |
| @classmethod | |
| def _parse_scopes(cls, v): | |
| return parse_scopes(v) | |
| @field_validator("required_scopes", mode="before") | |
| @classmethod | |
| def _parse_scopes(cls, v) -> list[str] | None: | |
| return parse_scopes(v) |
🤖 Prompt for AI Agents
In src/fastmcp/server/auth/providers/ociprovider.py around lines 58 to 61, the
Pydantic field validator _parse_scopes lacks an explicit return type; update its
signature to include the correct return type that matches parse_scopes (for
example -> list[str] or whatever parse_scopes returns), and add any necessary
typing imports (or use from __future__ import annotations) so the module remains
fully typed.
| """Initialize OCI OIDC provider. | ||
|
|
||
| Args: | ||
| config_url: OCI OIDC Discovery URL | ||
| client_id: OCI IAM Domain Integrated Application client id | ||
| client_secret: OCI Integrated Application client secret | ||
| audience: OCI API audience (optional) | ||
| base_url: Public URL where OIDC endpoints will be accessible (includes any mount path) | ||
| issuer_url: Issuer URL for OCI IAM Domain metadata. This will override issuer URL from the discovery URL. | ||
| required_scopes: Required OCI scopes (defaults to ["openid"]) | ||
| redirect_path: Redirect path configured in OCI IAM Domain Integrated Application. | ||
| The default is "/auth/callback". | ||
| allowed_client_redirect_uris: List of allowed redirect URI patterns for MCP clients. | ||
| """ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Complete the docstring with missing parameter documentation.
The docstring is missing documentation for three parameters: client_storage, jwt_signing_key, and require_authorization_consent. These are important configuration options (especially require_authorization_consent which has security implications) and should be documented for library users.
Consider adding documentation similar to the parent OIDCProxy class:
redirect_path: Redirect path configured in OCI IAM Domain Integrated Application.
The default is "/auth/callback".
allowed_client_redirect_uris: List of allowed redirect URI patterns for MCP clients.
+ Patterns support wildcards (e.g., "http://localhost:*", "https://*.example.com/*").
+ If None (default), only localhost redirect URIs are allowed.
+ client_storage: Storage backend for OAuth state (client registrations, encrypted tokens).
+ If None, a DiskStore will be created in the data directory.
+ jwt_signing_key: Secret for signing FastMCP JWT tokens (any string or bytes).
+ If not provided, the client secret will be used to derive a key.
+ require_authorization_consent: Whether to require user consent before authorizing clients (default True).
+ When False, authorization proceeds without user confirmation.
+ SECURITY WARNING: Only disable for local development or testing environments.
"""🤖 Prompt for AI Agents
In src/fastmcp/server/auth/providers/ociprovider.py around lines 105 to 118, the
docstring omits documentation for the parameters client_storage,
jwt_signing_key, and require_authorization_consent; update the docstring to add
brief descriptions for each: document client_storage (what storage
implementation is expected and its role), jwt_signing_key (purpose for signing
JWTs and expected format), and require_authorization_consent (boolean behavior
and security implication when true/false), mirroring style and phrasing used in
the parent OIDCProxy class so callers understand defaults and security impact.
|
It appears that this doesn't actually "do" anything -- the arguments are passed directly to the OIDC proxy provider without defaults, validation, or any other OCI-specific logic. [The Auth0 provider does something similar, I see]. Is there a strong reason to introduce this class rather than simply encourage users to use the OIDC proxy? For example, the docstring shows |
|
That is right. I will also be able to add documentation specific to OCI (How do users create an Integrated application (OAuth Client) in OCI IAM service) in the Integrations section for OCI. |
2 participants
Description
Contributors Checklist
Review Checklist
Summary by CodeRabbit