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 11 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:
WalkthroughThe PR centralizes authentication header application by introducing Changes
Sequence DiagramsequenceDiagram
participant Client as Client Code
participant Builder as Request Builder
participant ApiClient as ApiClient
participant OAuth2 as OAuth2Client
participant HttpClient as HTTP Client
participant TokenEndpoint as OAuth Token Endpoint
Client->>Builder: buildHttpRequest(configuration, apiClient)
Builder->>Builder: Create HttpRequest.Builder<br/>(headers, body, etc.)
Builder->>ApiClient: applyAuthHeader(requestBuilder, configuration)
alt Configuration.Credentials == null or NONE
ApiClient->>Builder: (no modification)
else CredentialsMethod == API_TOKEN
ApiClient->>ApiClient: Extract static token
ApiClient->>Builder: Set Authorization: Bearer {token}
else CredentialsMethod == CLIENT_CREDENTIALS
ApiClient->>ApiClient: Check cache by CredentialsCacheKey
alt Token in cache and valid
ApiClient->>Builder: Set Authorization: Bearer {cached_token}
else Token missing or expired
ApiClient->>OAuth2: getAccessToken()
OAuth2->>OAuth2: Check/create inFlight ConcurrentMap entry
alt Atomic winner (first thread to set inFlight)
OAuth2->>TokenEndpoint: POST /oauth/token<br/>(client_credentials, client_id, secret)
TokenEndpoint->>OAuth2: {access_token, expires_in}
OAuth2->>OAuth2: Create immutable AccessToken snapshot<br/>with computed expiry
OAuth2->>OAuth2: Cache in per-ApiClient ConcurrentMap
OAuth2->>OAuth2: Clear inFlight entry
else Atomic loser (subsequent threads)
OAuth2->>OAuth2: Reuse in-flight CompletableFuture
end
OAuth2->>ApiClient: CompletableFuture.get() {access_token}
ApiClient->>Builder: Set Authorization: Bearer {token}
end
end
Builder->>Builder: Apply request interceptors
Builder->>Builder: Build HttpRequest
Builder->>HttpClient: send/sendAsync(request)
HttpClient->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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.44%) 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.44% +0.38%
- Complexity 1259 1265 +6
============================================
Files 198 198
Lines 7646 7705 +59
Branches 885 900 +15
============================================
+ Hits 2910 2962 +52
+ Misses 4598 4594 -4
- Partials 138 149 +11 ☔ 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
…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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/main/java/dev/openfga/sdk/api/client/ApiClient.java (3)
341-361: Nit: document theassertValid()precondition on the public method.
applyAuthHeaderis public and dereferencescredentials.getApiToken().getToken()(line 376) andcredentials.getClientCredentials()insideensureOAuth2Client(line 401) without null guards. In-tree this is safe because every call site invokesconfiguration.assertValid()first (see learning), but since this is a new public surface, a one-line Javadoc note would prevent external callers from tripping on an NPE if they ever reach this method without having validated the configuration.Based on learnings:
Credentials.assertValid()ensuresapiToken != nullforAPI_TOKENandclientCredentials != nullforCLIENT_CREDENTIALS, and all in-tree call sites forApiClient.applyAuthHeader()invokeconfiguration.assertValid()beforehand.🤖 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 341 - 361, applyAuthHeader is a public API that dereferences credential fields without null checks; update its Javadoc to require callers to call configuration.assertValid() (or equivalently document that Configuration.assertValid() / Credentials.assertValid() must have been invoked) so external callers won't hit NPEs; reference the method name applyAuthHeader and the helper ensureOAuth2Client (which uses getClientCredentials) and the use of getApiToken/getToken in the doc comment to make the precondition explicit.
385-391: Minor: also unwrapFgaInvalidParameterExceptionfromExecutionException.getCause().
applyAuthHeaderalready declaresthrows FgaInvalidParameterException, andOAuth2Client.exchangeToken()declares it too — so it's possible (e.g., a config defect surfaced asynchronously) for anFgaInvalidParameterExceptionto be the cause. Today it gets wrapped inApiException, which loses the precise type for callers that distinguish the two. Cheap to fix.♻️ Proposed change
} catch (ExecutionException e) { Throwable cause = e.getCause(); if (cause instanceof ApiException) { throw (ApiException) cause; } + if (cause instanceof FgaInvalidParameterException) { + throw (FgaInvalidParameterException) cause; + } throw new ApiException(cause != null ? cause : 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/client/ApiClient.java` around lines 385 - 391, In the ExecutionException catch block inside ApiClient (e.g., in applyAuthHeader where OAuth2Client.exchangeToken() may be called), check if e.getCause() is an instance of FgaInvalidParameterException and if so rethrow it directly before the ApiException handling; otherwise keep the existing behavior of rethrowing ApiException when cause is ApiException or wrapping into a new ApiException for other throwables. This preserves the specific FgaInvalidParameterException type for callers that rely on it.
412-453: LGTM onCredentialsCacheKey.Hashing the client secret (rather than retaining plaintext in the key) is a nice touch for cache-isolation hygiene, and the
Arrays.equals/Arrays.hashCodehandling on the byte array is correct. One thing to keep in mind operationally: if your deployment ever rotates client credentials in-place against the sameApiClientinstance, stale entries will accumulate inoAuth2Clients— likely not a concern in typical usage (one credential per app), but worth noting if the SDK is ever embedded in a multi-tenant credential-rotating context.🤖 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 412 - 453, The review notes stale cache entries can accumulate in the oAuth2Clients map when client credentials are rotated (new secret => new CredentialsCacheKey) — fix by giving the cache eviction semantics: replace the plain Map with an expiring/loading cache (e.g., Caffeine or a ConcurrentHashMap with scheduled TTL eviction) or implement explicit removal when credentials are updated; locate usages of oAuth2Clients and the CredentialsCacheKey class and change the cache implementation to support time-based expiry or an update/remove pathway so rotating client secrets do not leak stale entries.src/main/java/dev/openfga/sdk/api/auth/OAuth2Client.java (1)
60-65: Optional: consider a CAS retry loop instead of recursion.The fallback
existing != null ? existing : getAccessToken()at line 64 recurses when the racing thread clearedinFlightbefore ourget(). In practice the recursion terminates immediately (snapshot is set before the in-flight gate is cleared on success), but a tight CAS-retry loop would be more idiomatic and remove the (theoretical) stack-growth concern under sustained failure churn.♻️ Sketch
- public CompletableFuture<String> getAccessToken() throws FgaInvalidParameterException, ApiException { - AccessToken current = snapshot.get(); - if (current.isValid()) { - return CompletableFuture.completedFuture(current.token()); - } - - CompletableFuture<String> promise = new CompletableFuture<>(); - if (!inFlight.compareAndSet(null, promise)) { - // Another thread won the race — join its exchange rather than starting a new one. - CompletableFuture<String> existing = inFlight.get(); - return existing != null ? existing : getAccessToken(); - } + public CompletableFuture<String> getAccessToken() throws FgaInvalidParameterException, ApiException { + CompletableFuture<String> promise; + while (true) { + AccessToken current = snapshot.get(); + if (current.isValid()) { + return CompletableFuture.completedFuture(current.token()); + } + promise = new CompletableFuture<>(); + if (inFlight.compareAndSet(null, promise)) { + break; + } + CompletableFuture<String> existing = inFlight.get(); + if (existing != null) { + return existing; + } + }🤖 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/OAuth2Client.java` around lines 60 - 65, The fallback in OAuth2Client.getAccessToken currently recurses via "existing != null ? existing : getAccessToken()" when inFlight was cleared between the compareAndSet and the subsequent get(), which risks theoretical stack growth; change this to a CAS-retry loop: allocate a new CompletableFuture<String> promise, attempt inFlight.compareAndSet(null, promise) and if it fails obtain the current inFlight via inFlight.get() and if that is non-null return it, otherwise retry the compareAndSet until it succeeds (then proceed with the token exchange and complete/clear inFlight), using the existing inFlight, promise, compareAndSet and getAccessToken logic but without recursion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/dev/openfga/sdk/api/auth/OAuth2Client.java`:
- Around line 60-65: The fallback in OAuth2Client.getAccessToken currently
recurses via "existing != null ? existing : getAccessToken()" when inFlight was
cleared between the compareAndSet and the subsequent get(), which risks
theoretical stack growth; change this to a CAS-retry loop: allocate a new
CompletableFuture<String> promise, attempt inFlight.compareAndSet(null, promise)
and if it fails obtain the current inFlight via inFlight.get() and if that is
non-null return it, otherwise retry the compareAndSet until it succeeds (then
proceed with the token exchange and complete/clear inFlight), using the existing
inFlight, promise, compareAndSet and getAccessToken logic but without recursion.
In `@src/main/java/dev/openfga/sdk/api/client/ApiClient.java`:
- Around line 341-361: applyAuthHeader is a public API that dereferences
credential fields without null checks; update its Javadoc to require callers to
call configuration.assertValid() (or equivalently document that
Configuration.assertValid() / Credentials.assertValid() must have been invoked)
so external callers won't hit NPEs; reference the method name applyAuthHeader
and the helper ensureOAuth2Client (which uses getClientCredentials) and the use
of getApiToken/getToken in the doc comment to make the precondition explicit.
- Around line 385-391: In the ExecutionException catch block inside ApiClient
(e.g., in applyAuthHeader where OAuth2Client.exchangeToken() may be called),
check if e.getCause() is an instance of FgaInvalidParameterException and if so
rethrow it directly before the ApiException handling; otherwise keep the
existing behavior of rethrowing ApiException when cause is ApiException or
wrapping into a new ApiException for other throwables. This preserves the
specific FgaInvalidParameterException type for callers that rely on it.
- Around line 412-453: The review notes stale cache entries can accumulate in
the oAuth2Clients map when client credentials are rotated (new secret => new
CredentialsCacheKey) — fix by giving the cache eviction semantics: replace the
plain Map with an expiring/loading cache (e.g., Caffeine or a ConcurrentHashMap
with scheduled TTL eviction) or implement explicit removal when credentials are
updated; locate usages of oAuth2Clients and the CredentialsCacheKey class and
change the cache implementation to support time-based expiry or an update/remove
pathway so rotating client secrets do not leak stale entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e483e90-b78e-4e7e-bd66-e375203f1813
📒 Files selected for processing (5)
src/main/java/dev/openfga/sdk/api/auth/AccessToken.javasrc/main/java/dev/openfga/sdk/api/auth/OAuth2Client.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/client/StreamingApiExecutorTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/dev/openfga/sdk/api/auth/AccessTokenTest.java
…tor to strict mutex; includes regression test
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
Refactor
Tests