ramu11 commented on PR #29592: URL: https://github.com/apache/camel/pull/29592#issuecomment-6792012794
@davsclaus Thanks for the detailed review. I've pushed a separate follow-up commit addressing the review findings. Details below. ### Important findings **1. OAuth configuration silently ignored / fail-open — Addressed** Moved the cross-option validation into `McpServerBridge` initialization so it applies to programmatic as well as Camel Main configurations. - Required scopes without an OAuth profile are rejected. - A resource URI without an OAuth profile is rejected. - An OAuth profile without a resource URI is rejected. - Added an explicit `supportsOAuthConfiguration()` capability to `McpServerEngine`. Engines that do not support OAuth reject these settings instead of silently ignoring them. - The Vert.x engine declares OAuth support. - Removed duplicate OAuth profile validation from the Main factory; the HTTP engine performs profile validation when creating its security support. - Preserved the existing stdio warn-and-ignore behavior. Added tests for unsupported engines and invalid programmatic configurations. **2. Non-root `camel.server.path` breaks RFC 9728 discovery — Addressed** Added startup validation that rejects a non-root `camel.server.path` when MCP OAuth is enabled. The exception explains that the public path prefix must be configured through `camel.server.mcp-path`. Added a regression test and documented this restriction. **3. MCP component documentation — Addressed** Updated `mcp-server.adoc` with: - The three OAuth configuration options. - A Camel Main / JBang configuration example, including expected audience. - HTTPS resource URI requirements and path matching. - RFC 9728 metadata discovery. - 401, 403 and 503 behavior. - `CamelMcpOAuthSubject`, `CamelMcpOAuthScopes`, `CamelMcpOAuthClaims` and the standard `CamelOAuthTokenValidationResult` exchange property. - Principal role extraction and stdio behavior. - The distinction between MCP OAuth configuration and global HTTP authentication. Also updated the relevant descriptions in `main.adoc`. **4. `scp` scope behavior affects all camel-oauth users — Addressed** Documented the support for both `scope` and `scp` claims in `oauth.adoc` and added an entry to the Camel 4.24 upgrade guide. The documentation explains string and array claim formats, merging and duplicate removal. ### Suggestions and nits **5. Module-wide s390x integration-test skip — Addressed** Removed the module-wide `skipITs.s390x` property and applied `@DisabledOnOs(architectures = "s390x")` only to `McpServerMainOAuthKeycloakIT`. Other integration tests in the module are no longer excluded by this setting. **6. Include `resource_metadata` in the 403 challenge — Addressed** The `insufficient_scope` response now includes the RFC 9728 `resource_metadata` parameter, along with the required scopes. Added assertions in both the lightweight OAuth test and the real Keycloak integration test. **7. Subject-bound sessions and standard OAuth validation result — Addressed** Implemented both suggestions: - Stores the validated token subject (`sub`) when an MCP session is initialized. - Requires the same authenticated subject for subsequent POST, GET and DELETE requests. - Returns the same 404 response used for unknown sessions when another subject attempts to access the session. - Rejects initialization when an OAuth-protected session has no non-empty subject. - Exposes the full `OAuthTokenValidationResult` through the standard `CamelOAuthTokenValidationResult` exchange property, alongside the MCP-specific identity properties. Added tests covering subject-less tokens, cross-subject access through all three operations, and continued access by the original subject. **8. Compatibility constructors and record Javadocs — Addressed** Removed the compatibility constructors for signatures that had not shipped in a released version: - The OAuth-aware compatibility constructor in `McpServerInfo`. - The 8-argument and 10-argument compatibility constructors in `VertxMcpStreamableServerTransportProvider`. Added the missing `@param` documentation for the OAuth fields in `McpToolCallContext`. ### Question / clarification **9. HTTPS resource URI and local development — Pending clarification** I have kept the MCP resource URI HTTPS-only for now. The `allow-insecure-http` option continues to apply only to authorization-server discovery, JWKS and introspection endpoints; it does not permit an HTTP MCP resource URI. I have not introduced a loopback HTTP exception pending agreement on the intended behavior. Would you prefer local development to require TLS on the embedded MCP endpoint (or a local TLS-terminating proxy), or should HTTP be permitted specifically for loopback resource URIs when `allow-insecure-http=true`? ### Validation The following tests have passed: - MCP Server unit tests: **108 passed, 0 failures, 0 errors**. - Keycloak OAuth integration tests: **16 passed, 0 failures, 0 errors**. The Keycloak integration test was run independently after building/installing the required reactor dependencies to avoid running unrelated Camel OpenAI integration tests. @davsclaus Thanks again for the review. I have kept item 9 unchanged pending your guidance. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
