Send Basic auth on firm OAuth token requests only when staging requires it#257
Send Basic auth on firm OAuth token requests only when staging requires it#257Benjvandam wants to merge 2 commits into
Conversation
…es it silverfin-cli attaches the SF_BASIC_AUTH header to every staging request, including the OAuth /token POSTs. On a staging environment whose Basic Auth gateway is disabled, that header makes Doorkeeper read the gateway credentials as the OAuth client and reject the request with "unknown client" - breaking `silverfin authorize` and token refresh. Add AxiosFactory.createTokenInstanceForFirm, which probes the host once for a WWW-Authenticate: Basic challenge and attaches the Basic header only when the gateway actually requires it. Route all three firm /oauth/token requests through it: initial authorize, manual refreshFirm, and the automatic 401-refresh interceptor. Staging data calls keep the Basic header; production is unchanged.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe firm OAuth/token request path now uses a token-only axios client that conditionally adds Basic auth for staging after a host probe. ChangesFirm token client flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 docstrings
🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/api/axiosFactory.js`:
- Around line 84-94: The basic-gate probe in the host-checking logic should not
cache a false “gateway disabled” result when the request fails ambiguously
without a response. Update the probe around the axios.get call in the basic-gate
method so that only a confirmed non-Basic response sets and stores false, while
timeout/DNS/TLS-style failures leave the host uncached or return an
indeterminate state. Use the existing `#basicGateByHost` cache and the current
response/error header inspection to distinguish confirmed Basic challenges from
probe failures before writing to the cache.
In `@tests/lib/api/silverfinAuthorizer.test.js`:
- Line 79: The mock for createTokenInstanceForFirm in SilverfinAuthorizer tests
is synchronous even though the real method returns a Promise, so it can hide
missing await bugs. Update the test setup around
AxiosFactory.createTokenInstanceForFirm to return an async-resolving mock (using
the same mockAxiosInstance) so SilverfinAuthorizer is exercised with the real
async behavior.
🪄 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: 64551d47-8a64-47b7-9747-82d9c47ec0f1
📒 Files selected for processing (4)
lib/api/axiosFactory.jslib/api/silverfinAuthorizer.jstests/lib/api/axiosFactory.test.jstests/lib/api/silverfinAuthorizer.test.js
michieldegezelle
left a comment
michieldegezelle
left a comment
There was a problem hiding this comment.
🟠 Major — check-cli-version CI is failing: package.json / package-lock.json are still at 1.56.1 (same as main), so the version-bump step fails before the changelog check runs. Bump the version (e.g. 1.56.2 for a bugfix) and add a matching ## [1.56.2] entry to CHANGELOG.md describing the conditional staging Basic-auth behaviour — unless the PR description checkbox to skip the version check is intentional.
💡 Suggestion — tests/TESTS.md is stale: lines 360–362 still document AxiosFactory.createAuthInstanceForFirm and the old single “always Basic on staging” behaviour; update to createTokenInstanceForFirm and the gateway-probe cases added in this PR.
- axiosFactory: re-throw (instead of caching) on a transport-level Basic-gate probe failure with no HTTP response, so one transient blip can't suppress the Basic header for the rest of the process and break every following token exchange on a gated staging; add a regression test that asserts the next call re-probes. - silverfinAuthorizer test: mock createTokenInstanceForFirm with mockResolvedValue (the method is async) so a dropped await would be caught. - Bump version 1.56.1 -> 1.56.2 and add a CHANGELOG entry (fixes check-cli-version CI). - TESTS.md: replace stale createAuthInstanceForFirm rows with the current createTokenInstanceForFirm tests, including the gateway-probe cases.
|
@michieldegezelle both done in 86cff50 — updated to 1.56.2 with a changelog entry, and updated the stale TESTS.md rows to createTokenInstanceForFirm. |
|
|
||
| it("should not thrown an error for missing tokens", () => { | ||
| firmCredentials.getHost.mockReturnValue(mockHost); | ||
| describe("Create token instance for firm", () => { |
There was a problem hiding this comment.
💡 Suggestion: The static AxiosFactory.#basicGateByHost cache persists across all test cases in this file — jest.clearAllMocks() in beforeEach doesn't reset static class fields. These tests only stay isolated because each uses a distinct hostname (gated./open./flaky.staging…). If a future staging token-instance test reuses an existing host (e.g. the widely-used https://test-api.staging.getsilverfin.com), it'll silently false-pass/fail depending on run order, and the toHaveBeenCalledTimes(2) assertion is similarly run-order sensitive. Consider a test-only reset to make isolation explicit rather than relying on unique hostnames — e.g. expose static _resetBasicGateCache() { this.#basicGateByHost = {}; } and call it in beforeEach.
michieldegezelle
left a comment
michieldegezelle
left a comment
There was a problem hiding this comment.
Claude made 1 more comment, but approved already
2 participants
Problem
On staging, the CLI attaches the
SF_BASIC_AUTHHTTP Basic header to every firm request, including the OAuth/tokenPOSTs (initialauthorize, manualrefreshFirm, and the automatic 401-refresh interceptor).When a staging environment has its Basic Auth gateway disabled (e.g. set up with the "disable HTTP basic auth" option), that header breaks authentication: Doorkeeper treats the gateway credentials as the OAuth client, finds no matching client, and responds
401 "Client authentication failed ... unknown client". As a resultsilverfin authorizeand token refresh fail on such stagings.Confirmed empirically: the same
/oauth/tokenrequest, with the sameclient_id/client_secret, succeeds when the Basic header is omitted.Fix
Add
AxiosFactory.createTokenInstanceForFirm(), which builds the token-endpoint axios instance and attaches the Basic header only when the staging gateway actually requires it — detected by a one-time, per-host probe for aWWW-Authenticate: Basicchallenge (an unauthenticated request to a Basic-protected host returns401 WWW-Authenticate: Basic).client_id/client_secret, as Doorkeeper expects.All three firm
/oauth/tokenpaths now route through this method (initial authorize,refreshFirm, and the 401-refresh interceptor). Staging data calls keep the Basic header; production behaviour is unchanged.Tests
Updated
tests/lib/api/axiosFactory.test.jsandtests/lib/api/silverfinAuthorizer.test.js, including new coverage for gateway-present vs gateway-disabled staging. All api-layer tests pass (91/91).