luigidemasi commented on PR #23866: URL: https://github.com/apache/camel/pull/23866#issuecomment-4659982524
> ## Review Summary > This PR adds a well-designed public SPI for validating incoming OAuth 2.0 Bearer tokens (JWT via JWKS + opaque via RFC 7662 introspection), integrates it into `platform-http` via an `oauthProfile` endpoint option, and provides a default implementation in `camel-oauth`. > > ### Security > The security posture is strong: > > * HTTPS enforcement by default (`allowInsecureHttp = false`) > * Symmetric algorithm rejection (HS256/384/512 blocked) > * Bearer token character validation + bounded token length (8192) > * Generic error messages (no token details leaked) > * Authorization header removed before route processing > * Validation result stored as exchange property (not externally visible header) > > ### SPI Design > The SPI design is clean and extensible: > > * `OAuthTokenValidationFactory` resolved via FactoryFinder > * Profile-based config supports multi-IdP deployments > * `PlatformHttpEngine.createConsumer` overload is backward-compatible (default method) > * Vert.x integration runs auth before body handler, on blocking thread pool > > ### Test Coverage > Comprehensive test coverage including JWT validation, OIDC discovery, introspection, caching, error paths, platform-http unit tests, and Vert.x integration tests with WireMock IdP. > > ### Minor Suggestions (non-blocking) > 1. **Property resolution in the interface:** `OAuthTokenValidationFactory.resolveProfileConfigWithPrefix()` is a substantial private static method in the SPI interface. Custom implementations can't customize property resolution. Consider whether this should move to a separate helper class. Not blocking since custom impls can override `validateToken(CamelContext, String, String)` directly. > 2. **`CamelOAuthTokenValidationResult` vs `Exchange.AUTHENTICATION`:** The test explicitly asserts `Exchange.AUTHENTICATION` is null — this seems intentional to avoid conflicts with other auth mechanisms. A brief Javadoc note on the rationale would help future contributors. > 3. **`JwksCache` singleton:** `JwksCache.getInstance()` returns a JVM-wide singleton shared across CamelContexts. A brief Javadoc note about the shared scope would be helpful. > 4. **`OAuthTokenRequest.refreshTokenGrant` fix:** The fix (sending `refresh_token` instead of `token` parameter) is correct per RFC 6749 Section 6 but is a behavior change on an existing method — consider a sentence in the upgrade guide entry. > > This review does not replace specialized AI review tools (CodeRabbit, Sourcery) or static analysis (SonarCloud). > > _This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying._ @davsclaus I addressed these. The profile property resolution is now in a separate resolver class instead of the SPI interface, with the SPI default methods just delegating to it. I also added the notes around Exchange.AUTHENTICATION and the JVM-wide JWKS cache scope, and documented the refresh_token request parameter fix in the 4.21 upgrade guide. -- 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]
