fix: stop mutating token refresh errors into auth errors#329
fix: stop mutating token refresh errors into auth errors#329
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe changes fix issue Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 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 |
|
Related Documentation Checked 8 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #329 +/- ##
==========================================
+ Coverage 89.67% 89.96% +0.28%
==========================================
Files 25 25
Lines 1492 1534 +42
Branches 279 299 +20
==========================================
+ Hits 1338 1380 +42
+ Misses 94 90 -4
- Partials 60 64 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes token refresh failures being surfaced as mutated FgaApiError objects (breaking instanceof FgaApiAuthenticationError) by constructing and throwing a real FgaApiAuthenticationError, and adds a regression test for the failure mode described in #328.
Changes:
- Add a regression test asserting token refresh failures throw a real
FgaApiAuthenticationError. - Replace error-instance mutation in
Credentials.refreshAccessToken()with explicitFgaApiAuthenticationErrorconstruction, preserving auth context fields.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/credentials.test.ts | Adds a regression test ensuring token refresh failures reject with FgaApiAuthenticationError. |
| credentials/credentials.ts | Stops mutating FgaApiError instances and instead throws a newly constructed FgaApiAuthenticationError with auth context. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/credentials.test.ts
Outdated
| await expect(credentials.getAccessTokenHeader()).rejects.toThrow(FgaApiAuthenticationError); | ||
| expect(scope.isDone()).toBe(true); |
There was a problem hiding this comment.
This test triggers the full retry loop (4 requests) and will sleep for the exponential backoff delays (hundreds of ms up to >1s), which can noticeably slow the suite and add timing flakiness. Consider using Jest fake timers (or mocking the retry delay) so the retries complete without real time passing while still asserting the thrown error type.
| await expect(credentials.getAccessTokenHeader()).rejects.toThrow(FgaApiAuthenticationError); | |
| expect(scope.isDone()).toBe(true); | |
| jest.useFakeTimers(); | |
| try { | |
| const accessTokenPromise = credentials.getAccessTokenHeader(); | |
| jest.runAllTimers(); | |
| await expect(accessTokenPromise).rejects.toThrow(FgaApiAuthenticationError); | |
| expect(scope.isDone()).toBe(true); | |
| } finally { | |
| jest.useRealTimers(); | |
| } |
credentials/credentials.ts
Outdated
| status: err.statusCode, | ||
| statusText: err.statusText, | ||
| data: err.responseData, | ||
| headers: err.responseHeader, |
There was a problem hiding this comment.
FgaApiAuthenticationError’s constructor calls getResponseHeaders(err), which assumes err.response.headers is an object. Here headers is set to err.responseHeader, which is optional on FgaApiError; if it’s undefined this can throw at runtime. Consider defaulting headers to {} (and/or omitting the response object entirely when there is no response) before constructing the auth error.
| headers: err.responseHeader, | |
| headers: err.responseHeader ?? {}, |
credentials/credentials.ts
Outdated
| const authError = new FgaApiAuthenticationError({ | ||
| response: { | ||
| status: err.statusCode, | ||
| statusText: err.statusText, | ||
| data: err.responseData, | ||
| headers: err.responseHeader, | ||
| }, | ||
| config: { | ||
| url: err.requestURL, | ||
| method: err.method, | ||
| data: JSON.stringify({ | ||
| client_id: clientCredentials.clientId, | ||
| audience: clientCredentials.apiAudience, | ||
| grant_type: "client_credentials", | ||
| }), | ||
| }, | ||
| } as any); |
There was a problem hiding this comment.
This creates an AxiosError-like object and then casts it to any to satisfy new FgaApiAuthenticationError(...). That removes type safety and makes it easy to drift from AxiosError shape over time. A more robust approach would be to introduce a small helper/factory (or overload) that builds an FgaApiAuthenticationError directly from an existing FgaApiError + auth context, avoiding as any and manual AxiosError shaping.
|
Addressed the review comments and pushed commit What changed:
Validated locally:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@errors.ts`:
- Around line 217-221: The constructor for the error class currently ignores the
optional context when err is an AxiosError; update the AxiosError branch in the
constructor to apply context overrides (clientId, audience, grantType) after
extracting values from the parsed request data so that if
context.clientId/audience/grantType is provided it replaces the parsed value;
locate the AxiosError handling block in the constructor and merge values (e.g.,
finalClientId = context?.clientId ?? parsedClientId) for each of the three
fields and use those merged values where the code currently uses the parsed
ones.
In `@tests/credentials.test.ts`:
- Around line 541-569: Add assertions to the existing test that checks thrown
error properties: after awaiting
expect(credentials.getAccessTokenHeader()).rejects.toThrow(FgaApiAuthenticationError)
also capture the rejected error and assert its context fields and status code
are set correctly (e.g., verify error.clientId equals OPENFGA_CLIENT_ID,
error.audience equals OPENFGA_API_AUDIENCE, error.grantType equals
CredentialsMethod.ClientCredentials or the literal grant type string used, and
error.statusCode equals 404). Use the same test-local variables (apiTokenIssuer,
OPENFGA_CLIENT_ID, OPENFGA_API_AUDIENCE) and the
Credentials.getAccessTokenHeader / FgaApiAuthenticationError symbols to locate
where to add these assertions.
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@errors.ts`: - Around line 217-221: The constructor for the error class currently ignores the optional context when err is an AxiosError; update the AxiosError branch in the constructor to apply context overrides (clientId, audience, grantType) after extracting values from the parsed request data so that if context.clientId/audience/grantType is provided it replaces the parsed value; locate the AxiosError handling block in the constructor and merge values (e.g., finalClientId = context?.clientId ?? parsedClientId) for each of the three fields and use those merged values where the code currently uses the parsed ones. In `@tests/credentials.test.ts`: - Around line 541-569: Add assertions to the existing test that checks thrown error properties: after awaiting expect(credentials.getAccessTokenHeader()).rejects.toThrow(FgaApiAuthenticationError) also capture the rejected error and assert its context fields and status code are set correctly (e.g., verify error.clientId equals OPENFGA_CLIENT_ID, error.audience equals OPENFGA_API_AUDIENCE, error.grantType equals CredentialsMethod.ClientCredentials or the literal grant type string used, and error.statusCode equals 404). Use the same test-local variables (apiTokenIssuer, OPENFGA_CLIENT_ID, OPENFGA_API_AUDIENCE) and the Credentials.getAccessTokenHeader / FgaApiAuthenticationError symbols to locate where to add these assertions.errors.ts (1)
217-221:contextparameter is silently ignored whenerris anAxiosError.The AxiosError branch (lines 264–272) extracts
clientId,audience, andgrantTypeonly from parsed request data, ignoring thecontextargument. Today no caller passes context with an AxiosError, but a future caller could hit this silently. Consider applying context overrides in the AxiosError branch as well, or documenting that context is only used forFgaApiErrorinputs.Proposed fix
- this.clientId = data?.client_id; - this.audience = data?.audience; - this.grantType = data?.grant_type; + this.clientId = context?.clientId || data?.client_id; + this.audience = context?.audience || data?.audience; + this.grantType = context?.grantType || data?.grant_type;Also applies to: 253-272
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@errors.ts` around lines 217 - 221, The constructor for the error class currently ignores the optional context when err is an AxiosError; update the AxiosError branch in the constructor to apply context overrides (clientId, audience, grantType) after extracting values from the parsed request data so that if context.clientId/audience/grantType is provided it replaces the parsed value; locate the AxiosError handling block in the constructor and merge values (e.g., finalClientId = context?.clientId ?? parsedClientId) for each of the three fields and use those merged values where the code currently uses the parsed ones.tests/credentials.test.ts (1)
541-569: Good regression test — consider asserting error properties too.The
instanceofcheck directly validates the fix for#328. For stronger coverage, you could also verify that context fields (clientId,audience,grantType) andstatusCodeare correctly propagated on the thrown error.Example: assert error properties
- await expect(credentials.getAccessTokenHeader()).rejects.toThrow(FgaApiAuthenticationError); + await expect(credentials.getAccessTokenHeader()).rejects.toSatisfy((err: FgaApiAuthenticationError) => { + expect(err).toBeInstanceOf(FgaApiAuthenticationError); + expect(err.statusCode).toBe(404); + expect(err.clientId).toBe(OPENFGA_CLIENT_ID); + expect(err.audience).toBe(OPENFGA_API_AUDIENCE); + expect(err.grantType).toBe("client_credentials"); + return true; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/credentials.test.ts` around lines 541 - 569, Add assertions to the existing test that checks thrown error properties: after awaiting expect(credentials.getAccessTokenHeader()).rejects.toThrow(FgaApiAuthenticationError) also capture the rejected error and assert its context fields and status code are set correctly (e.g., verify error.clientId equals OPENFGA_CLIENT_ID, error.audience equals OPENFGA_API_AUDIENCE, error.grantType equals CredentialsMethod.ClientCredentials or the literal grant type string used, and error.statusCode equals 404). Use the same test-local variables (apiTokenIssuer, OPENFGA_CLIENT_ID, OPENFGA_API_AUDIENCE) and the Credentials.getAccessTokenHeader / FgaApiAuthenticationError symbols to locate where to add these assertions.
|
Addressed the remaining CodeRabbit suggestions in commit Changes made:
Validation run:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
errors.ts
Outdated
| this.clientId = context?.clientId || data?.client_id; | ||
| this.audience = context?.audience || data?.audience; | ||
| this.grantType = context?.grantType || data?.grant_type; |
There was a problem hiding this comment.
In the FgaApiError wrapping branch, the context fallbacks use || (e.g., context?.clientId || data?.client_id). This treats empty strings as “unset” and will fall back to request data, which is inconsistent with the AxiosError branch below that uses nullish coalescing (??). Prefer using ?? here as well for consistent semantics and to avoid surprising overrides when a context value is intentionally provided but falsy.
| this.clientId = context?.clientId || data?.client_id; | |
| this.audience = context?.audience || data?.audience; | |
| this.grantType = context?.grantType || data?.grant_type; | |
| this.clientId = context?.clientId ?? data?.client_id; | |
| this.audience = context?.audience ?? data?.audience; | |
| this.grantType = context?.grantType ?? data?.grant_type; |
|
Followed up on Copilot review #329 (review) in commit Actionable fixes applied:
Validation run:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (err: unknown) { | ||
| if (err instanceof FgaApiError) { | ||
| (err as any).constructor = FgaApiAuthenticationError; | ||
| (err as any).name = "FgaApiAuthenticationError"; | ||
| (err as any).clientId = clientCredentials.clientId; | ||
| (err as any).audience = clientCredentials.apiAudience; | ||
| (err as any).grantType = "client_credentials"; | ||
| throw new FgaApiAuthenticationError(err, { | ||
| clientId: clientCredentials.clientId, | ||
| audience: clientCredentials.apiAudience, | ||
| grantType: "client_credentials", | ||
| }); | ||
| } | ||
|
|
||
| throw err; |
There was a problem hiding this comment.
refreshAccessToken() only wraps errors that are instanceof FgaApiError. However attemptHttpRequest() throws FgaApiAuthenticationError directly for 401/403, so token-refresh failures with those statuses will bypass this wrapper and lose the intended auth context (clientId, audience, grantType). Consider also catching/wrapping FgaApiAuthenticationError here (or adjusting the retry helper) so token endpoint 401/403 include the same context as other failures.
errors.ts
Outdated
| this.grantType = data?.grant_type; | ||
| this.clientId = context?.clientId ?? data?.client_id; | ||
| this.audience = context?.audience ?? data?.audience; | ||
| this.grantType = context?.grantType ?? data?.grant_type; |
There was a problem hiding this comment.
In the AxiosError branch, the code assumes err.config.data is JSON and parses it with JSON.parse(). For token refresh requests, axios commonly stores this as a URL-encoded string, so parsing fails and the error won’t capture clientId/audience/grantType unless context was explicitly passed. Consider parsing err.config.data as URLSearchParams when it’s a string and JSON parsing fails.
credentials/credentials.ts
Outdated
| throw new FgaApiAuthenticationError(err, { | ||
| clientId: clientCredentials.clientId, | ||
| audience: clientCredentials.apiAudience, | ||
| grantType: "client_credentials", |
There was a problem hiding this comment.
Minor: grantType is passed as the string literal "client_credentials" here. Using CredentialsMethod.ClientCredentials would keep this aligned with the enum and the test expectation, and avoids duplicating the raw value in multiple places.
| grantType: "client_credentials", | |
| grantType: CredentialsMethod.ClientCredentials, |
|
Addressed actionable feedback from #329 (review) in commit Applied changes:
Validation run:
|
2 participants
Summary
Replaces error-instance mutation in credential token refresh with construction of a real
FgaApiAuthenticationError.Changes
toThrow(FgaApiAuthenticationError)when token endpoint returns 500FgaApiAuthenticationErrorclient_id,audience,grant_type) in the generated error payloadPotential Breaking Change
FgaApiAuthenticationErrorinstance rather than a mutated API error object.Fixes #328
Summary by CodeRabbit
Bug Fixes
Tests