fix auth #154
Open
fix auth #154
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughThis PR adds intelligent fallback logic for authentication provider selection. When no explicit default provider is set, the system automatically selects from available providers (preferring a single registered provider or the "api_key" provider if multiple exist). Additionally, authentication configuration is auto-detected from environment variables when not explicitly configured. Minor logging enhancements support diagnostic purposes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthPort
participant Environment
participant AuthManager
participant Loaders
Client->>AuthPort: resolve_auth_port(no explicit config)
AuthPort->>Environment: Check for explicit auth_config
Environment-->>AuthPort: None
AuthPort->>Environment: Check auth_env flag
Environment-->>AuthPort: None
AuthPort->>Environment: Auto-detect MCP_* variables
Environment-->>AuthPort: Found (e.g., MCP_API_KEY)
AuthPort->>Loaders: setup_auth_from_env()
Loaders->>AuthManager: Check registered providers
AuthManager-->>Loaders: List of providers
Loaders->>Loaders: Select default (sole/api_key)
Loaders-->>AuthPort: Auth configuration
AuthPort-->>Client: Resolved auth config
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #154 +/- ##
==========================================
- Coverage 95.80% 95.77% -0.04%
==========================================
Files 150 150
Lines 8923 8943 +20
==========================================
+ Hits 8549 8565 +16
- Misses 374 378 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@mcp_fuzzer/auth/manager.py`:
- Around line 46-69: Add unit tests for get_default_auth_headers on the Auth
manager to lock the new fallback logic: 1) "single provider returns its headers"
— create a manager with one auth provider (mock or simple stub implementing
get_auth_headers) and assert returned dict equals that provider's headers; 2)
"multiple providers with api_key prefer api_key" — add at least two providers
including a key "api_key" and assert headers come from
auth_providers["api_key"]; 3) "multiple providers without api_key returns empty
dict" — add multiple providers none named "api_key" and assert the return is {}.
Use the same class/method names as in the diff (get_default_auth_headers and
auth_providers) and mock get_auth_headers to return distinct marker dicts for
clear assertions.
In `@mcp_fuzzer/client/main.py`:
- Around line 139-143: The two debug f-strings exceed line length; replace them
with logger-style formatting and break long calls across lines: change
logging.debug(f"Calling fuzz_all_tools with runs={config.get('runs', 10)}") to
logging.debug("Calling fuzz_all_tools with runs=%s", config.get("runs", 10)) and
change logging.debug(f"fuzz_all_tools completed, got {len(tool_results)} tool
results") to logging.debug("fuzz_all_tools completed, got %s tool results",
len(tool_results)); also ensure the client.fuzz_all_tools call (and its
runs_per_tool=...) is wrapped over multiple lines to keep each line under the
limit (refer to client.fuzz_all_tools, runs_per_tool, tool_results, config.get).
In `@mcp_fuzzer/client/tool_client.py`:
- Around line 236-241: The debug log f-strings in fuzz_all_tools exceed the
line-length limit; replace them with logger-style formatting and avoid eager
interpolation: in the fuzz_all_tools method (and the debug after calling
_get_tools_from_server) stop using f-strings and instead call self._logger.debug
with a format string and separate arguments (use %s/%d placeholders and pass
runs_per_tool and tool_timeout, and pass the computed tools count expression as
an argument), keeping the info/warning calls as-is and ensuring lines are
wrapped under the 88-char limit.
Comment on lines
46
to
+69
| def get_default_auth_headers(self) -> dict[str, str]: | ||
| """Get auth headers from default provider for transport authentication. | ||
|
|
||
| If no default provider is set, tries to use: | ||
| 1. Provider named "api_key" (if exists) | ||
| 2. First available provider (if only one exists) | ||
| 3. Empty dict (if multiple providers and no default) | ||
|
|
||
| Returns: | ||
| Dict of auth headers, or empty dict if no default provider is set | ||
| Dict of auth headers, or empty dict if no provider available | ||
| """ | ||
| # Use explicitly set default provider | ||
| if self.default_provider and self.default_provider in self.auth_providers: | ||
| return self.auth_providers[self.default_provider].get_auth_headers() | ||
|
|
||
| # Fallback: if only one provider exists, use it | ||
| if len(self.auth_providers) == 1: | ||
| provider = next(iter(self.auth_providers.values())) | ||
| return provider.get_auth_headers() | ||
|
|
||
| # Fallback: prefer "api_key" provider if it exists | ||
| if "api_key" in self.auth_providers: | ||
| return self.auth_providers["api_key"].get_auth_headers() | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add unit coverage for the new fallback selection paths.
The new default-selection logic is subtle (single provider vs. api_key vs. empty). Please add tests to lock behavior and prevent regressions in future refactors. Suggested cases: single provider returns its headers; multiple providers with api_key prefer api_key; multiple providers without api_key returns empty dict.
🤖 Prompt for AI Agents
In `@mcp_fuzzer/auth/manager.py` around lines 46 - 69, Add unit tests for
get_default_auth_headers on the Auth manager to lock the new fallback logic: 1)
"single provider returns its headers" — create a manager with one auth provider
(mock or simple stub implementing get_auth_headers) and assert returned dict
equals that provider's headers; 2) "multiple providers with api_key prefer
api_key" — add at least two providers including a key "api_key" and assert
headers come from auth_providers["api_key"]; 3) "multiple providers without
api_key returns empty dict" — add multiple providers none named "api_key" and
assert the return is {}. Use the same class/method names as in the diff
(get_default_auth_headers and auth_providers) and mock get_auth_headers to
return distinct marker dicts for clear assertions.
Comment on lines
+139
to
+143
| logging.debug(f"Calling fuzz_all_tools with runs={config.get('runs', 10)}") | ||
| tool_results = await client.fuzz_all_tools( | ||
| runs_per_tool=config.get("runs", 10) | ||
| ) | ||
| logging.debug(f"fuzz_all_tools completed, got {len(tool_results)} tool results") |
Contributor
There was a problem hiding this comment.
Fix Ruff E501 by wrapping debug logs.
These new f-strings exceed the line-length limit and fail CI. Use logger formatting and line wrapping.
✅ Proposed fix
- logging.debug(f"Calling fuzz_all_tools with runs={config.get('runs', 10)}")
+ logging.debug(
+ "Calling fuzz_all_tools with runs=%s",
+ config.get("runs", 10),
+ )
@@
- logging.debug(f"fuzz_all_tools completed, got {len(tool_results)} tool results")
+ logging.debug(
+ "fuzz_all_tools completed, got %d tool results",
+ len(tool_results),
+ )📝 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.
Suggested change
| logging.debug(f"Calling fuzz_all_tools with runs={config.get('runs', 10)}") | |
| tool_results = await client.fuzz_all_tools( | |
| runs_per_tool=config.get("runs", 10) | |
| ) | |
| logging.debug(f"fuzz_all_tools completed, got {len(tool_results)} tool results") | |
| logging.debug( | |
| "Calling fuzz_all_tools with runs=%s", | |
| config.get("runs", 10), | |
| ) | |
| tool_results = await client.fuzz_all_tools( | |
| runs_per_tool=config.get("runs", 10) | |
| ) | |
| logging.debug( | |
| "fuzz_all_tools completed, got %d tool results", | |
| len(tool_results), | |
| ) |
🧰 Tools
🪛 GitHub Actions: Lint
[error] 139-139: Ruff check failed: E501 Line too long (95 > 88) in mcp_fuzzer/client/main.py:139. Command: 'ruff check mcp_fuzzer tests'.
[error] 143-143: Ruff check failed: E501 Line too long (100 > 88) in mcp_fuzzer/client/main.py:143. Command: 'ruff check mcp_fuzzer tests'.
🤖 Prompt for AI Agents
In `@mcp_fuzzer/client/main.py` around lines 139 - 143, The two debug f-strings
exceed line length; replace them with logger-style formatting and break long
calls across lines: change logging.debug(f"Calling fuzz_all_tools with
runs={config.get('runs', 10)}") to logging.debug("Calling fuzz_all_tools with
runs=%s", config.get("runs", 10)) and change logging.debug(f"fuzz_all_tools
completed, got {len(tool_results)} tool results") to
logging.debug("fuzz_all_tools completed, got %s tool results",
len(tool_results)); also ensure the client.fuzz_all_tools call (and its
runs_per_tool=...) is wrapped over multiple lines to keep each line under the
limit (refer to client.fuzz_all_tools, runs_per_tool, tool_results, config.get).
Comment on lines
+236
to
+241
| self._logger.debug(f"fuzz_all_tools called with runs_per_tool={runs_per_tool}, tool_timeout={tool_timeout}") | ||
| self._logger.info("Fetching tools from server...") | ||
| tools = await self._get_tools_from_server() | ||
| self._logger.debug(f"_get_tools_from_server returned {len(tools) if tools else 0} tools") | ||
| if not tools: | ||
| self._logger.warning("No tools available for fuzzing") |
Contributor
There was a problem hiding this comment.
Fix Ruff E501 by wrapping debug logs and using logger formatting.
Current f-strings exceed the 88-char limit and fail CI. Switching to logger formatting resolves lint and avoids eager formatting.
✅ Proposed fix
- self._logger.debug(f"fuzz_all_tools called with runs_per_tool={runs_per_tool}, tool_timeout={tool_timeout}")
+ self._logger.debug(
+ "fuzz_all_tools called with runs_per_tool=%s, tool_timeout=%s",
+ runs_per_tool,
+ tool_timeout,
+ )
@@
- self._logger.debug(f"_get_tools_from_server returned {len(tools) if tools else 0} tools")
+ self._logger.debug(
+ "_get_tools_from_server returned %d tools",
+ len(tools) if tools else 0,
+ )🧰 Tools
🪛 GitHub Actions: Lint
[error] 236-236: Ruff check failed: E501 Line too long (116 > 88) in mcp_fuzzer/client/tool_client.py:236. Command: 'ruff check mcp_fuzzer tests'.
[error] 239-239: Ruff check failed: E501 Line too long (97 > 88) in mcp_fuzzer/client/tool_client.py:239. Command: 'ruff check mcp_fuzzer tests'.
🤖 Prompt for AI Agents
In `@mcp_fuzzer/client/tool_client.py` around lines 236 - 241, The debug log
f-strings in fuzz_all_tools exceed the line-length limit; replace them with
logger-style formatting and avoid eager interpolation: in the fuzz_all_tools
method (and the debug after calling _get_tools_from_server) stop using f-strings
and instead call self._logger.debug with a format string and separate arguments
(use %s/%d placeholders and pass runs_per_tool and tool_timeout, and pass the
computed tools count expression as an argument), keeping the info/warning calls
as-is and ensuring lines are wrapped under the 88-char limit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Summary: Authentication Enhancements with Auto-Detection and Fallback Logic
Overview
This PR introduces automatic authentication provider selection and environment variable auto-detection, allowing authentication to work with minimal configuration. Changes span authentication initialization, manager fallback logic, transport resolution, and enhanced logging.
Design Changes and Patterns
1. Implicit Auto-Detection Pattern
The PR adds implicit auto-detection across multiple layers:
resolve_auth_port(): Auto-detects auth from environment variables without explicit--auth-envflagsetup_auth_from_env(): Auto-selects default provider based on available providersget_default_auth_headers(): Provides runtime fallback to api_key provider or sole providerThis follows a Convention-over-Configuration pattern, reducing explicit configuration burden but introducing implicit behavior.
2. Fallback/Cascade Strategy Pattern
Multiple fallback chains now exist:
resolve_auth_port(): auth_config → auth_env flag → auto-detect env vars → Nonesetup_auth_from_env(): explicit MCP_DEFAULT_AUTH_PROVIDER → single provider → api_key providerget_default_auth_headers(): explicit default → single provider → api_key provider → empty dictThis implements a Priority-Based Resolver pattern for graceful degradation.
Design and Code Smells
SOLID Violations:
Single Responsibility Principle (SRP) - Medium Violation
setup_auth_from_env()now handles: reading env vars, creating providers, and selecting default provider (3 responsibilities)resolve_auth_port()handles: reading CLI args, checking env vars, and dispatching to appropriate loaderOpen/Closed Principle (OCP) - Medium Violation
resolve_auth_port():["MCP_API_KEY", "MCP_USERNAME", ...]Dependency Inversion Principle (DIP) - Minor Violation
resolve_auth_port()directly depends on concrete function names (load_auth_config,setup_auth_from_env)Code Smells:
Duplicate Logic (DRY Violation) - High Severity
setup_auth_from_env()(lines 68-77 of loaders.py)get_default_auth_headers()(lines 46-70 of manager.py)Magic String Literals - Medium Severity
AUTH_PROVIDER_NAMES,AUTH_ENV_VARIABLES)Feature Envy - Minor
get_default_auth_headers()directly accessesauth_providersdict internalsImplicit Behavior - Low to Medium Severity
Implicit Coupling Issues:
get_default_auth_headers()fallback logic depends on whetherdefault_providerwas previously setPositive Aspects
✅ Improved UX: Single provider or api_key scenarios require no explicit configuration
✅ Dependency Injection: AuthManager properly injected throughout
✅ Strategy Pattern: AuthProvider interface well-designed with concrete implementations
✅ Enhanced Observability: Added debug/info logging for troubleshooting (lines 139-143 in main.py, lines 236-242 in tool_client.py)
✅ Documentation: Updated docstrings reflect new behavior
✅ Backward Compatibility: Explicit configuration still works as before
✅ Defensive Coding: Null checks and fallbacks prevent AttributeErrors
Recommendations for Improvement
_select_default_provider(providers: dict) -> str | Noneutility function to DRY default selection and ensure consistency--auto-detect-authflag or configuration option with clear logging when auto-detection triggersmcp_fuzzer/auth/constants.py)AuthResolutionFactoryorAuthResolutionStrategyto encapsulate resolution logic and eliminate conditional logic inresolve_auth_port()Summary
The PR successfully reduces configuration friction through intelligent defaults but introduces implicit behavior and duplicated logic that violates DRY and increases maintenance burden. The changes prioritize developer UX over code clarity, which is acceptable for a fuzzer tool but should be addressed through refactoring and test coverage before adding more features. The most critical issue is the duplicated default provider selection logic that exists in two places—this should be extracted to a shared utility immediately.