fix: attach Authorization header to streaming and ApiExecutor requests (#330)#331
fix: attach Authorization header to streaming and ApiExecutor requests (#330)#331cportcvent wants to merge 10 commits intoopenfga:mainfrom
Conversation
…try point for attaching OAuth2 Authorization header. Includes lazy OAuth2Client initialization on detected CLIENT_CREDENTIALS use.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughCentralizes and applies outbound authentication for all request paths: adds ApiClient.applyAuthHeader(...), updates request builders (streaming and non-streaming) to call it, moves OAuth2 token management into ApiClient with caching, removes duplicate auth logic in OpenFgaApi, and adds tests for auth header behavior and concurrency. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ApiClient
participant RequestBuilder
participant OAuth2Issuer
participant RemoteAPI
Client->>RequestBuilder: buildHttpRequest(configuration)
RequestBuilder->>ApiClient: applyAuthHeader(requestBuilder, configuration)
alt credentials method = API_TOKEN
ApiClient-->>RequestBuilder: add Authorization: Bearer <api_token>
else credentials method = CLIENT_CREDENTIALS
ApiClient->>OAuth2Issuer: (cached?) getAccessToken()
alt cached valid token
OAuth2Issuer-->>ApiClient: return token
else no valid token
OAuth2Issuer->>OAuth2Issuer: in-flight dedupe + exchange
OAuth2Issuer->>OAuth2Issuer: update TokenSnapshot
OAuth2Issuer-->>ApiClient: return token
end
ApiClient-->>RequestBuilder: add Authorization: Bearer <exchanged_token>
else no credentials
ApiClient-->>RequestBuilder: no Authorization header
end
RequestBuilder->>RemoteAPI: send HttpRequest (with headers)
RemoteAPI-->>Client: response / stream of data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/dev/openfga/sdk/api/BaseStreamingApi.java (1)
167-181:⚠️ Potential issue | 🟠 MajorPreserve auth exceptions instead of re-wrapping them.
Line 171 can now throw
ApiExceptionorFgaInvalidParameterException, but the broad catch on Line 180 wraps both as a newApiException. That loses the original auth failure type/details for streaming requests.Proposed fix
try { byte[] bodyBytes = objectMapper.writeValueAsBytes(body); HttpRequest.Builder requestBuilder = ApiClient.requestBuilder(method, path, bodyBytes, configuration); apiClient.applyAuthHeader(requestBuilder, configuration); // Apply request interceptors if any var interceptor = apiClient.getRequestInterceptor(); if (interceptor != null) { interceptor.accept(requestBuilder); } return requestBuilder.build(); + } catch (ApiException | FgaInvalidParameterException e) { + throw e; } catch (Exception e) { throw new ApiException(e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/openfga/sdk/api/BaseStreamingApi.java` around lines 167 - 181, The catch-all in BaseStreamingApi around objectMapper.writeValueAsBytes / ApiClient.requestBuilder / apiClient.applyAuthHeader is re-wrapping auth-related exceptions (e.g., FgaInvalidParameterException) into a generic ApiException and losing original details; update the catch block to rethrow the original exception when it's already an ApiException or FgaInvalidParameterException (or other auth-specific exception types), and only wrap unknown exceptions into a new ApiException so callers of the BaseStreamingApi methods receive the original auth failure type and message.
🧹 Nitpick comments (1)
src/test/java/dev/openfga/sdk/api/client/StreamingApiExecutorTest.java (1)
360-390: Optional: extend streaming regression toCLIENT_CREDENTIALS.Issue
#330specifically called outstreamedListObjectsfailing withClientCredentials. The API_TOKEN test is a solid guard, but the exact reproduction scenario (CLIENT_CREDENTIALSon the streaming path) is only covered transitively viaApiExecutorTest.rawApi_appliesClientCredentialsAuthHeader+ApiClientTestcaching. A WireMock-backed streaming variant (similar torawApi_appliesClientCredentialsAuthHeader) would give a direct regression guard for the exact originally-reported flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/dev/openfga/sdk/api/client/StreamingApiExecutorTest.java` around lines 360 - 390, Add a new streaming unit test mirroring stream_appliesApiTokenAuthHeader but using ClientCredentials: create a ClientConfiguration with credentials(new Credentials(new ClientCredentials(clientId, clientSecret))) (or the existing ClientCredentials fixture), call doCallRealMethod().when(mockApiClient).applyAuthHeader(...) as in the API_TOKEN test, construct an OpenFgaClient with that config, mock the HttpClient.sendAsync to return a Stream response, call streamingApiExecutor(...).stream(...).get(), capture the HttpRequest via ArgumentCaptor and assert the Authorization header produced by the streaming path equals the expected bearer token created/attached by applyAuthHeader (i.e., the same pattern and assertions as in stream_appliesApiTokenAuthHeader but with ClientCredentials to directly guard the CLIENT_CREDENTIALS streaming regression).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/dev/openfga/sdk/api/client/ApiClient.java`:
- Around line 390-397: The cached OAuth2Client is currently returned regardless
of the passed Configuration; change ensureOAuth2Client to validate the existing
oAuth2Client against the credential identity from the incoming Configuration
(e.g. clientId, clientSecret/token issuer/audience or any combination that
defines the credential identity) and only reuse it if the identity matches;
otherwise construct a new OAuth2Client and atomically replace the cached
reference (use a compare-and-set loop on the oAuth2Client atomic reference) so
callers using ConfigurationOverride or different credentials get a client tied
to those credentials; implement a small identity/key extractor on Configuration
and/or add a getter on OAuth2Client to compare the identity before returning the
cached instance.
- Around line 372-375: The OAuth token refresh path is not thread-safe:
ensureOAuth2Client() returns a shared OAuth2Client whose getAccessToken()
mutates shared fields (token, expiresAt) and can be invoked concurrently; update
OAuth2Client.getAccessToken() to serialize refreshes (e.g., add a private final
ReentrantLock or synchronized block inside getAccessToken(), or implement a
single-flight promise so only one thread calls exchangeToken() while others
wait) and ensure access to token and expiresAt is guarded; keep
ensureOAuth2Client() as-is but protect the mutating calls (exchangeToken()) and
the subsequent writes to token/expiresAt inside the lock so concurrent requests
don’t perform duplicate exchanges or corrupt state.
In `@src/main/java/dev/openfga/sdk/api/client/ApiExecutorRequestBuilder.java`:
- Around line 209-211: The builder currently appends user headers with
headers.forEach(httpRequestBuilder::header) then calls
apiClient.applyAuthHeader(httpRequestBuilder, configuration), which can produce
duplicate Authorization headers; update either ApiExecutorRequestBuilder or the
applyAuthHeader implementation to avoid duplication: detect existing
"Authorization" on the HttpRequest.Builder (or in the headers map passed to
ApiExecutorRequestBuilder) and skip adding the SDK token if present, or
conversely remove/override any user-supplied Authorization before calling
apiClient.applyAuthHeader; refer to the headers.forEach loop, the
httpRequestBuilder::header usage, and the apiClient.applyAuthHeader(...) method
when making the change and add a short Javadoc note on Authorization being
SDK-managed if you choose to reserve the header instead.
---
Outside diff comments:
In `@src/main/java/dev/openfga/sdk/api/BaseStreamingApi.java`:
- Around line 167-181: The catch-all in BaseStreamingApi around
objectMapper.writeValueAsBytes / ApiClient.requestBuilder /
apiClient.applyAuthHeader is re-wrapping auth-related exceptions (e.g.,
FgaInvalidParameterException) into a generic ApiException and losing original
details; update the catch block to rethrow the original exception when it's
already an ApiException or FgaInvalidParameterException (or other auth-specific
exception types), and only wrap unknown exceptions into a new ApiException so
callers of the BaseStreamingApi methods receive the original auth failure type
and message.
---
Nitpick comments:
In `@src/test/java/dev/openfga/sdk/api/client/StreamingApiExecutorTest.java`:
- Around line 360-390: Add a new streaming unit test mirroring
stream_appliesApiTokenAuthHeader but using ClientCredentials: create a
ClientConfiguration with credentials(new Credentials(new
ClientCredentials(clientId, clientSecret))) (or the existing ClientCredentials
fixture), call doCallRealMethod().when(mockApiClient).applyAuthHeader(...) as in
the API_TOKEN test, construct an OpenFgaClient with that config, mock the
HttpClient.sendAsync to return a Stream response, call
streamingApiExecutor(...).stream(...).get(), capture the HttpRequest via
ArgumentCaptor and assert the Authorization header produced by the streaming
path equals the expected bearer token created/attached by applyAuthHeader (i.e.,
the same pattern and assertions as in stream_appliesApiTokenAuthHeader but with
ClientCredentials to directly guard the CLIENT_CREDENTIALS streaming
regression).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39bae369-0896-409a-ae4f-5939c2a72286
📒 Files selected for processing (8)
src/main/java/dev/openfga/sdk/api/BaseStreamingApi.javasrc/main/java/dev/openfga/sdk/api/OpenFgaApi.javasrc/main/java/dev/openfga/sdk/api/client/ApiClient.javasrc/main/java/dev/openfga/sdk/api/client/ApiExecutorRequestBuilder.javasrc/test/java/dev/openfga/sdk/api/client/ApiClientTest.javasrc/test/java/dev/openfga/sdk/api/client/ApiExecutorTest.javasrc/test/java/dev/openfga/sdk/api/client/OpenFgaClientTest.javasrc/test/java/dev/openfga/sdk/api/client/StreamingApiExecutorTest.java
There was a problem hiding this comment.
Pull request overview
Fixes missing Authorization: Bearer <token> propagation on streaming and ApiExecutor request paths by centralizing auth header application in ApiClient, ensuring authenticated requests across OpenFgaApi, BaseStreamingApi, and ApiExecutorRequestBuilder.
Changes:
- Add
ApiClient.applyAuthHeader(...)with lazyOAuth2Clientinitialization/caching forCLIENT_CREDENTIALS. - Route
OpenFgaApi, streaming request building, andApiExecutorRequestBuilderthroughapplyAuthHeader(...). - Add/adjust unit + WireMock tests to cover auth header attachment and token exchange caching.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/dev/openfga/sdk/api/client/ApiClient.java | Introduces centralized auth header application + cached OAuth2 client. |
| src/main/java/dev/openfga/sdk/api/OpenFgaApi.java | Delegates auth header logic to ApiClient.applyAuthHeader(...). |
| src/main/java/dev/openfga/sdk/api/BaseStreamingApi.java | Ensures streaming request builder applies auth header. |
| src/main/java/dev/openfga/sdk/api/client/ApiExecutorRequestBuilder.java | Applies auth header for ApiExecutor-built requests and updates throws list. |
| src/test/java/dev/openfga/sdk/api/client/ApiClientTest.java | Adds unit tests for applyAuthHeader behaviors + OAuth2 exchange caching. |
| src/test/java/dev/openfga/sdk/api/client/ApiExecutorTest.java | Adds WireMock regression tests for auth headers on ApiExecutor path. |
| src/test/java/dev/openfga/sdk/api/client/StreamingApiExecutorTest.java | Adds regression guard verifying auth header on streaming path. |
| src/test/java/dev/openfga/sdk/api/client/OpenFgaClientTest.java | Switches to a real ApiClient in tests to exercise auth wiring. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (38.34%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #331 +/- ##
============================================
+ Coverage 38.05% 38.34% +0.28%
- Complexity 1259 1266 +7
============================================
Files 198 199 +1
Lines 7646 7717 +71
Branches 885 901 +16
============================================
+ Hits 2910 2959 +49
- Misses 4598 4610 +12
- Partials 138 148 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…e Authorization header
…to protect multi-tenant credentials
…ingle-flight pattern in OAuth2Client
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/test/java/dev/openfga/sdk/api/auth/OAuth2ClientTest.java (1)
174-210: Solid concurrency test; consider pooling threads for CI stability.The test faithfully reproduces the race and correctly asserts single-flight via
verify(1, postRequestedFor(...)). Two small nits if you want to harden it:
- Using
new Thread(...).start()without joining means threads may still be running when the test returns on timeout; wrapping in atry/finallythat interrupts survivors on failure would avoid leaked threads across tests.- A small
ExecutorServicewith ashutdownNow()in finally would be cleaner than raw threads.Neither is blocking — the
donelatch is sufficient for the assertion semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/dev/openfga/sdk/api/auth/OAuth2ClientTest.java` around lines 174 - 210, The test exchangeOAuth2Token_concurrentRequests_singleExchange creates raw Threads that may leak across tests; replace the manual thread creation with an ExecutorService (e.g., fixed thread pool of size threadCount) and submit the same runnables that await startGate and count down done, then in a finally block call executor.shutdownNow() and await termination to ensure all worker threads are stopped; alternatively, if you prefer to keep Threads, store them in a list and after done.await(...) join each thread and interrupt any still-alive threads in a finally block so no threads leak between tests (refer to startGate, done, tokens, failures, and the test method name).src/main/java/dev/openfga/sdk/api/auth/TokenSnapshot.java (1)
25-38: Per-call randomized jitter makesisValid()non-deterministic across successive calls.Because
ThreadLocalRandom.current().nextInt(EXPIRY_JITTER_SECS)is evaluated on every invocation, two calls a second apart near the expiry boundary can return different verdicts (one valid, the next invalid). Combined with theinFlightsingle-flight gate this is benign — the first losing call triggers a refresh that others piggyback on — but it does mean a burst of callers can deterministically force an early refresh even when the token is still well within its buffer window.If the intent is "randomize the refresh boundary per token" rather than "per call", consider capturing the jitter once (e.g., compute an
effectiveExpiresAtin the compact constructor) so all callers agree on validity for a given snapshot:♻️ Sketch
-record TokenSnapshot(String token, Instant expiresAt) { +record TokenSnapshot(String token, Instant expiresAt, Instant effectiveExpiresAt) { ... TokenSnapshot { - expiresAt = expiresAt != null ? expiresAt.truncatedTo(ChronoUnit.SECONDS) : null; + expiresAt = expiresAt != null ? expiresAt.truncatedTo(ChronoUnit.SECONDS) : null; + effectiveExpiresAt = expiresAt == null ? null : expiresAt + .minusSeconds(EXPIRY_BUFFER_SECS) + .minusSeconds(ThreadLocalRandom.current().nextInt(EXPIRY_JITTER_SECS)); } boolean isValid() { if (isNullOrWhitespace(token)) return false; - if (expiresAt == null) return true; - ... + if (effectiveExpiresAt == null) return true; + return Instant.now().isBefore(effectiveExpiresAt); } }Separately, if
TOKEN_EXPIRY_JITTER_IN_SECis ever set to 0,nextInt(0)throwsIllegalArgumentException— a defensiveMath.max(1, EXPIRY_JITTER_SECS)or an explicit guard would prevent a nasty regression if the constant is tuned down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/openfga/sdk/api/auth/TokenSnapshot.java` around lines 25 - 38, The isValid() method in TokenSnapshot uses ThreadLocalRandom.nextInt(EXPIRY_JITTER_SECS) on every call making validity checks non-deterministic; instead compute a per-snapshot jitter once (e.g., add an effectiveExpiresAt field or compute and store jitter in the TokenSnapshot constructor) and have isValid() use that stored value (use expiresAt, EXPIRY_BUFFER_SECS and the stored jitter to derive effectiveExpiresAt) so all calls agree; also defensively handle EXPIRY_JITTER_SECS == 0 by using Math.max(1, EXPIRY_JITTER_SECS) or an explicit guard when computing the one-time jitter.src/test/java/dev/openfga/sdk/api/auth/AccessTokenTest.java (1)
41-43: LGTM — parameterized cases correctly migrated toTokenSnapshot.The parameter set still covers the same validity boundaries and the
TokenSnapshotconstructor applies second-truncation consistently for these cases.Consider renaming the class to
TokenSnapshotTestin a follow-up since the class is no longer exercisingAccessToken.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/dev/openfga/sdk/api/auth/AccessTokenTest.java` around lines 41 - 43, The test class name still references AccessToken but now tests TokenSnapshot; rename the test class from AccessTokenTest to TokenSnapshotTest (update the public class declaration and the file name accordingly) so the test class name matches the exercised type (TokenSnapshot) while leaving the parameterized test method testTokenValid and its TokenSnapshot usage unchanged.src/test/java/dev/openfga/sdk/api/client/ApiClientTest.java (1)
58-270: Good coverage of the newapplyAuthHeadercontract.The suite exercises the essential branches (NONE, null credentials, null method, API_TOKEN, token replacement on retry, CC failure →
ApiException, CC success, CC caching, CC per-credential isolation). The replace-header test directly guards against the regression that led tosetHeaderbeing used in production — nice regression coverage.Minor:
mockHttpClientBuilderonly stubsbuild()andexecutor(...). IfApiClientever starts invoking otherHttpClient.Builderfluent methods (e.g.,connectTimeout,version) in the ctor, those will silently returnnulland NPE at the next chained call. Not a problem today, but a one-lineMockito.when(builder.<anyMethod>(any())).thenReturn(builder)default or using a realHttpClient.newBuilder()with a mockedhttpClientwould be more future-proof.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/dev/openfga/sdk/api/client/ApiClientTest.java` around lines 58 - 270, Test helper mockHttpClientBuilder currently only stubs build() and executor(...), so if ApiClient's constructor ever calls other HttpClient.Builder fluent methods (e.g., connectTimeout, version, followRedirects) the chain will return null and cause NPEs; update mockHttpClientBuilder to either (a) use a real HttpClient.newBuilder() and spy it while stubbing build() to return the HttpClientMock, or (b) add a default Mockito stub that returns the same mock builder for any fluent method (e.g., Mockito.when(builder.connectTimeout(any())).thenReturn(builder), Mockito.when(builder.version(any())).thenReturn(builder), or a catch-all pattern to return builder) so the builder remains chainable when used by ApiClient (see mockHttpClientBuilder, ApiClient constructor, and HttpClient.Builder fluent methods).src/main/java/dev/openfga/sdk/api/client/ApiClient.java (1)
396-449: Cache key design looks good; minor suggestion on lookup path.SHA-256 hashing the secret (rather than retaining the raw value as a key field) is the right call, and including
clientId, issuer, audience, and scopes gives clean tenant isolation. TheConcurrentHashMap+putIfAbsentpattern is correct.Optional:
computeIfAbsentwould collapse the two-step get/put and avoid constructing a throwawayOAuth2Clienton the losing side of a race, at the cost of needing to wrapFgaInvalidParameterException(e.g., via atry/catch+ sneaky-throw helper or an unchecked sentinel). Given how infrequently this path races in practice, the current form is fine.Also note: entries are never evicted. If an application rotates client secrets at runtime (a new
Configurationwith the same clientId/issuer/audience/scopes but a new secret), the map grows by one per rotation. Not a leak in typical usage, but worth calling out if long-running servers do key rotation without restarting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/openfga/sdk/api/client/ApiClient.java` around lines 396 - 449, The ensureOAuth2Client two-step get/putIfAbsent should be simplified to use oAuth2Clients.computeIfAbsent to avoid constructing a throwaway OAuth2Client on races: change ensureOAuth2Client to call oAuth2Clients.computeIfAbsent(key, k -> { try { return new OAuth2Client(configuration, this); } catch (FgaInvalidParameterException e) { throw new RuntimeException(e); } }) and then unwrap the RuntimeException to rethrow the original FgaInvalidParameterException; additionally consider replacing the plain ConcurrentHashMap with a bounded/expiring cache (e.g., Caffeine) or add TTL/eviction logic for CredentialsCacheKey to avoid unbounded growth on client-secret rotations (referencing ensureOAuth2Client, CredentialsCacheKey, oAuth2Clients, and OAuth2Client).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/dev/openfga/sdk/api/client/ApiClient.java`:
- Around line 373-391: Unwrap the ExecutionException returned by
ensureOAuth2Client(...).getAccessToken().get() so the original cause (e.g.,
FgaApi*Error or ApiException from the HTTP attempt) is propagated to callers: if
ExecutionException.getCause() is an ApiException rethrow it, otherwise wrap the
cause in a new ApiException preserving the cause; also preserve
InterruptedException handling as-is. Add defensive null checks before
dereferencing credentials.getApiToken() and
configuration.getCredentials().getClientCredentials() (and their
getToken()/token fields) and throw a clear validation exception (e.g.,
FgaInvalidParameterException or ApiException with a descriptive message) when
those sub-objects or tokens are null instead of letting an NPE surface;
reference the switch block in ApiClient.java, the calls to
credentials.getApiToken().getToken(),
ensureOAuth2Client(...).getAccessToken().get(), and
configuration.getCredentials().getClientCredentials() when making these changes.
---
Nitpick comments:
In `@src/main/java/dev/openfga/sdk/api/auth/TokenSnapshot.java`:
- Around line 25-38: The isValid() method in TokenSnapshot uses
ThreadLocalRandom.nextInt(EXPIRY_JITTER_SECS) on every call making validity
checks non-deterministic; instead compute a per-snapshot jitter once (e.g., add
an effectiveExpiresAt field or compute and store jitter in the TokenSnapshot
constructor) and have isValid() use that stored value (use expiresAt,
EXPIRY_BUFFER_SECS and the stored jitter to derive effectiveExpiresAt) so all
calls agree; also defensively handle EXPIRY_JITTER_SECS == 0 by using
Math.max(1, EXPIRY_JITTER_SECS) or an explicit guard when computing the one-time
jitter.
In `@src/main/java/dev/openfga/sdk/api/client/ApiClient.java`:
- Around line 396-449: The ensureOAuth2Client two-step get/putIfAbsent should be
simplified to use oAuth2Clients.computeIfAbsent to avoid constructing a
throwaway OAuth2Client on races: change ensureOAuth2Client to call
oAuth2Clients.computeIfAbsent(key, k -> { try { return new
OAuth2Client(configuration, this); } catch (FgaInvalidParameterException e) {
throw new RuntimeException(e); } }) and then unwrap the RuntimeException to
rethrow the original FgaInvalidParameterException; additionally consider
replacing the plain ConcurrentHashMap with a bounded/expiring cache (e.g.,
Caffeine) or add TTL/eviction logic for CredentialsCacheKey to avoid unbounded
growth on client-secret rotations (referencing ensureOAuth2Client,
CredentialsCacheKey, oAuth2Clients, and OAuth2Client).
In `@src/test/java/dev/openfga/sdk/api/auth/AccessTokenTest.java`:
- Around line 41-43: The test class name still references AccessToken but now
tests TokenSnapshot; rename the test class from AccessTokenTest to
TokenSnapshotTest (update the public class declaration and the file name
accordingly) so the test class name matches the exercised type (TokenSnapshot)
while leaving the parameterized test method testTokenValid and its TokenSnapshot
usage unchanged.
In `@src/test/java/dev/openfga/sdk/api/auth/OAuth2ClientTest.java`:
- Around line 174-210: The test
exchangeOAuth2Token_concurrentRequests_singleExchange creates raw Threads that
may leak across tests; replace the manual thread creation with an
ExecutorService (e.g., fixed thread pool of size threadCount) and submit the
same runnables that await startGate and count down done, then in a finally block
call executor.shutdownNow() and await termination to ensure all worker threads
are stopped; alternatively, if you prefer to keep Threads, store them in a list
and after done.await(...) join each thread and interrupt any still-alive threads
in a finally block so no threads leak between tests (refer to startGate, done,
tokens, failures, and the test method name).
In `@src/test/java/dev/openfga/sdk/api/client/ApiClientTest.java`:
- Around line 58-270: Test helper mockHttpClientBuilder currently only stubs
build() and executor(...), so if ApiClient's constructor ever calls other
HttpClient.Builder fluent methods (e.g., connectTimeout, version,
followRedirects) the chain will return null and cause NPEs; update
mockHttpClientBuilder to either (a) use a real HttpClient.newBuilder() and spy
it while stubbing build() to return the HttpClientMock, or (b) add a default
Mockito stub that returns the same mock builder for any fluent method (e.g.,
Mockito.when(builder.connectTimeout(any())).thenReturn(builder),
Mockito.when(builder.version(any())).thenReturn(builder), or a catch-all pattern
to return builder) so the builder remains chainable when used by ApiClient (see
mockHttpClientBuilder, ApiClient constructor, and HttpClient.Builder fluent
methods).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d860d14-bf0f-4a09-bcfd-519efb78f367
📒 Files selected for processing (6)
src/main/java/dev/openfga/sdk/api/auth/OAuth2Client.javasrc/main/java/dev/openfga/sdk/api/auth/TokenSnapshot.javasrc/main/java/dev/openfga/sdk/api/client/ApiClient.javasrc/test/java/dev/openfga/sdk/api/auth/AccessTokenTest.javasrc/test/java/dev/openfga/sdk/api/auth/OAuth2ClientTest.javasrc/test/java/dev/openfga/sdk/api/client/ApiClientTest.java
|
It seems to work well! |
…lyAuthHeader exception propogation
SoulPancake
left a comment
SoulPancake
left a comment
There was a problem hiding this comment.
Since accessToken.java is now unused, maybe we can remove it?
And rename AccessTokenTest to TokenSnapshotTest
| @Test | ||
| public void stream_appliesApiTokenAuthHeader() throws Exception { | ||
| // Regression guard for openfga/java-sdk#330: the streaming path must attach | ||
| // Authorization: Bearer <token> when the configuration uses API_TOKEN credentials. |
There was a problem hiding this comment.
For this test
can we replace the doCallRealMethod() pattern with real ApiClient constructed from a fresh mock builder backed by mockHttpClient ?
| // after inFlight becomes null immediately sees a valid token. | ||
| snapshot.set(new TokenSnapshot(token, Instant.now().plusSeconds(response.getExpiresInSeconds()))); | ||
|
|
||
| telemetry.metrics().credentialsRequest(1L, new HashMap<>()); |
There was a problem hiding this comment.
Can we move this telemetry.metrics().credentialsRequest(...) to after promise.complete(token) so a slow/throwing telemetry call can't stall waiting threads.
…g a forked TokenSnapshot Also improve stream API executor tests per reviewer comments.
… stability during OAuth2 exchange handling
Good catch - I went with the other direction: preserve the existing AccessToken history, but make it immutable (essentially roll TokenSnapshot into AccessToken as a modest refactor rather than a new concept) Addressed in 6298cb6 |
Description
What problem is being solved?
OpenFgaClient#streamedListObjects— and every other code path that flows throughStreamingApiExecutor,ApiExecutorRequestBuilder, orBaseStreamingApi— sends requests without theAuthorization: Bearer <token>header. Against any FGA deployment that requires auth (FGA Cloud, or any self-hosted OpenFGA with bearer auth), every streaming call fails withApiException: API error: 401, even though non-streaming calls on the sameOpenFgaClientwith the sameClientCredentialssucceed.Closes #330.
How is it being solved?
Auth used to live in exactly one place —
OpenFgaApi.buildHttpRequestWithPublisher— which also privately owned the onlyOAuth2Client. Any request builder that did not route throughOpenFgaApi(streaming, the newerApiExecutor) had no way to reach that code and shipped unauthenticated.This PR centralizes auth on
ApiClientas the single entry point and routes every request-builder path through it:ApiClient#applyAuthHeader(HttpRequest.Builder, Configuration)appliesAuthorization: Bearer <token>based onCredentials:NONE(or null credentials / null method) → no header.API_TOKEN→ static token from configuration.CLIENT_CREDENTIALS→ lazily creates anOAuth2Clienton first use, cached on theApiClientin anAtomicReference, so the exchanged token is reused across every request path that uses the sameApiClient.OpenFgaApidrops its privateOAuth2Clientfield andgetAccessToken(...)helper and delegates toapiClient.applyAuthHeader(...).BaseStreamingApi.buildHttpRequestandApiExecutorRequestBuilder.buildHttpRequestnow callapiClient.applyAuthHeader(...)before running user-supplied request interceptors — fixing the 401 forstreamedListObjectsand every other streaming /ApiExecutor-backed call.No public API is removed.
ApiClientgains one new method;OpenFgaApi's public surface is unchanged. Lazy initialization means existingApiClientconsumers that never useCLIENT_CREDENTIALSpay nothing.What changes are made to solve it?
src/main/java/dev/openfga/sdk/api/client/ApiClient.java— addsapplyAuthHeader(...)and a lazyAtomicReference<OAuth2Client>with double-checked initialization viacompareAndSet.src/main/java/dev/openfga/sdk/api/OpenFgaApi.java— removesoAuth2Clientfield andgetAccessToken(...); replaces inline header logic with a call toapiClient.applyAuthHeader(httpRequest, configuration).src/main/java/dev/openfga/sdk/api/BaseStreamingApi.java— invokesapiClient.applyAuthHeader(requestBuilder, configuration)immediately after building the request.src/main/java/dev/openfga/sdk/api/client/ApiExecutorRequestBuilder.java— invokesapiClient.applyAuthHeader(...)before running request interceptors; method now declaresApiExceptionin addition toFgaInvalidParameterExceptionandJsonProcessingException.ApiClientTest— unit coverage forapplyAuthHeaderacrossNONE,API_TOKEN, andCLIENT_CREDENTIALS, including verification that the OAuth2 token exchange happens exactly once and is cached for subsequent calls.ApiExecutorTest— WireMock-backed regression tests that prove theApiExecutorrequest path attachesAuthorizationfor bothAPI_TOKENandCLIENT_CREDENTIALS.StreamingApiExecutorTest— regression guard tied directly to streamedListObjects omits Authorization header — 401 against FGA Cloud with ClientCredentials #330: the streaming path now attachesAuthorization: Bearer <token>whenAPI_TOKENcredentials are configured.OpenFgaClientTest— switches from a mockedApiClientto a realApiClientwrapping a mockedHttpClient.Builder, so tests exercise the realapplyAuthHeaderwiring.References
Review Checklist
OpenFgaClient/OpenFgaApisurface is unchanged.main— base ismain.OpenFgaApi,ApiExecutor,StreamingApiExecutor) plus a direct#330regression guard inStreamingApiExecutorTest.Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor
Tests