Fix adding www-authenticate header#13821
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoved MCP-specific logic that modified the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
Pull request overview
This PR adjusts MCP authentication failure handling in the API Gateway, focusing on how the WWW-Authenticate header is produced (and where that logic lives), while also cleaning up related gateway utilities and tests.
Changes:
- Removed MCP-specific
WWW-Authenticateheader mutation logic fromAPIAuthenticationHandler.handleAuthFailure(...)(delegating toGatewayUtils.handleAuthFailure(...)only). - Removed an unused
apiTypeconstant fromGatewayUtils. - Updated
APIAuthenticationHandlerTestCaseby deleting MCP-related tests and modifying metric-timer related assertions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.../APIAuthenticationHandlerTestCase.java |
Removes MCP tests and changes assertions in metric timer tests (currently reducing meaningful coverage). |
.../GatewayUtils.java |
Removes an unused static apiType constant. |
.../APIAuthenticationHandler.java |
Deletes in-handler MCP WWW-Authenticate header construction logic, relying on shared auth-failure handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.gateway/src/test/java/org/wso2/carbon/apimgt/gateway/handlers/security/APIAuthenticationHandlerTestCase.java`:
- Around line 377-380: The test currently uses a no-op assertion; replace it
with a real verification that stopMetricTimer delegates to the context by
verifying the mocked context's stop() was invoked (e.g., use
Mockito.verify(context, times(1)).stop() or equivalent) after calling
APIAuthenticationHandler.stopMetricTimer(context) so the test fails if stop() is
not called; reference APIAuthenticationHandler.stopMetricTimer and the mocked
context.stop() in the assertion.
- Around line 325-328: Replace the hardcoded error code in the test helper with
the production constant: in APIAuthenticationHandlerTestCase::isAuthenticate
where it currently throws new APISecurityException(1000, "test"), use
APISecurityConstants.API_AUTH_INVALID_CREDENTIALS instead (or add a comment if a
different code was intentional) so the test mirrors production error-code usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 89417889-5f0a-4889-b8e4-386e66d2dc98
📒 Files selected for processing (3)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/APIAuthenticationHandler.javacomponents/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.javacomponents/apimgt/org.wso2.carbon.apimgt.gateway/src/test/java/org/wso2/carbon/apimgt/gateway/handlers/security/APIAuthenticationHandlerTestCase.java
💤 Files with no reviewable changes (2)
- components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java
- components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/APIAuthenticationHandler.java
This pull request introduces several improvements and fixes related to MCP (Multi-Channel Protocol) request handling, authentication, and protocol validation in the API Gateway. The changes primarily focus on consistent usage of constants, improving authentication flows for MCP requests, refining error responses, and enhancing protocol version validation. Below are the most important changes grouped by theme:
MCP Request Handling and Protocol Validation:
Authentication Flow and Error Handling:
Scopes and Resource Management:
These changes collectively improve the reliability, maintainability, and interoperability of MCP request handling and authentication in the API Gateway.
Resolves: wso2/api-manager#4681