Add OAuth2ClientInterceptor to handle client credentials flow#2969
Add OAuth2ClientInterceptor to handle client credentials flow#2969christiangoerdes wants to merge 10 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds OAuth2ClientInterceptor that performs client-credentials token acquisition (optional scope), caches tokens until near expiry, injects Authorization: Bearer into outbound requests, and returns 502 ProblemDetails on token acquisition failure. ChangesOAuth2 Client Credentials Token Injection
Sequence DiagramsequenceDiagram
participant Client
participant OAuth2ClientInterceptor
participant RouterHttpClient
participant AuthorizationServer
Client->>OAuth2ClientInterceptor: Incoming request
OAuth2ClientInterceptor->>OAuth2ClientInterceptor: getAccessToken() (check cache)
alt token missing/expired
OAuth2ClientInterceptor->>RouterHttpClient: POST tokenUrl (form body, Basic auth)
RouterHttpClient->>AuthorizationServer: POST /token
AuthorizationServer->>RouterHttpClient: 200 { access_token, expires_in }
RouterHttpClient->>OAuth2ClientInterceptor: token response
OAuth2ClientInterceptor->>OAuth2ClientInterceptor: parse JSON, update cache
end
OAuth2ClientInterceptor->>Client: Forward request with Authorization: Bearer <token>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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. Comment |
|
/ok-to-test |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2ClientInterceptor.java`:
- Around line 61-66: The current Bad Gateway ProblemDetails built in
OAuth2ClientInterceptor (the gateway(...).title("Bad Gateway").status(502)...
.detail("Could not obtain an OAuth2 access token from
%s.".formatted(tokenUrl)).buildAndSetResponse(exc)) leaks backend topology by
including tokenUrl in the client-visible detail; replace that formatted detail
with a generic message (e.g. "Could not obtain an OAuth2 access token.") so the
call to .detail(...) does not contain tokenUrl or any internal hostnames/URLs,
and keep the original exception logged server-side (exc) for diagnostics without
returning internal addresses to the client.
- Around line 112-114: The buildBasicAuthorization() method currently joins raw
clientId and clientSecret with ":" before Base64, which fails for credentials
requiring form-encoding; update it to call the existing
encodeClientCredential(...) (or implement it if missing) to percent-encode each
component using application/x-www-form-urlencoded (e.g. URLEncoder.encode(value,
UTF_8.name())), then join encodedClientId + ":" + encodedClientSecret and pass
that to getEncoder().encodeToString(...), preserving the "Basic " prefix and
UTF_8 usage.
- Around line 53-56: handleRequest currently calls fetchAccessToken() on every
request; change OAuth2ClientInterceptor to cache the access token and its expiry
(use fields like cachedToken and tokenExpiryEpochMillis) and only call
fetchAccessToken() when the cached token is missing or about to expire (e.g.
within a small refresh window of now >= tokenExpiryEpochMillis -
refreshMarginMillis). Protect token reads/writes with synchronization (or a
ReentrantLock) so only one thread performs fetchAccessToken() and updates
cachedToken/tokenExpiryEpochMillis while others wait or use the previous token
if still valid; ensure fetchAccessToken() returns both token and expires_in so
you compute tokenExpiryEpochMillis from current time + expires_in*1000.
🪄 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: 0e35c861-8fe8-49ae-9460-e12d17aa5509
📒 Files selected for processing (1)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2ClientInterceptor.java
|
This pull request needs "/ok-to-test" from an authorized committer. |
…edundant token requests
… handling of special characters
… handling: extract `extractAccessToken()` method and simplify token request creation.
…ion with `URLParamUtil.createQueryStringOmitNullValues` for improved readability and reusability
…henticationUtil.createAuthorizationHeader` and replace redundant implementations across modules
Summary by CodeRabbit
New Features
Refactor