-
Notifications
You must be signed in to change notification settings - Fork 0
feat(auth): add refresh-token support to TokenStore + PKCE provider #36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
0a7d655
2a92956
db8235d
d4cd62e
05463f7
c8d224e
a7fd8e8
0fca19e
aca2702
ae34bb6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,19 +22,43 @@ import { execFile } from 'node:child_process' | |
| import { readFileSync } from 'node:fs' | ||
| import openBrowserModule from 'open' | ||
| import { type RunOAuthFlowOptions, runOAuthFlow } from './flow.js' | ||
| import type { AuthProvider, TokenStore } from './types.js' | ||
| import type { AuthProvider, TokenBundle, TokenStore } from './types.js' | ||
|
|
||
| type Account = { id: string; label?: string; email: string } | ||
| type FakeStoreState = { | ||
| account: Account | ||
| bundle: TokenBundle | ||
| /** Literal options arg seen by `setBundle` (`undefined` when omitted). */ | ||
| setBundleOptions: { promoteDefault?: boolean } | undefined | ||
| } | ||
|
|
||
| /** Tiny in-memory `TokenStore` so the flow tests don't need disk I/O. */ | ||
| function fakeStore(): TokenStore<Account> & { last?: { account: Account; token: string } } { | ||
| const state: { last?: { account: Account; token: string } } = {} | ||
| /** | ||
| * Tiny in-memory `TokenStore` so the flow tests don't need disk I/O. Keeps | ||
| * the full `TokenBundle` and the literal `setBundle` options arg so tests | ||
| * can verify that `runOAuthFlow` persists refresh + expiry AND opts into | ||
| * `promoteDefault: true` on the explicit login path. | ||
| */ | ||
| function fakeStore(): TokenStore<Account> & { last?: FakeStoreState } { | ||
| const state: { last?: FakeStoreState } = {} | ||
| return { | ||
| async active() { | ||
| return state.last ?? null | ||
| return state.last | ||
| ? { | ||
| token: state.last.bundle.accessToken, | ||
| bundle: state.last.bundle, | ||
| account: state.last.account, | ||
| } | ||
| : null | ||
| }, | ||
| async set(account, token) { | ||
| state.last = { account, token } | ||
| state.last = { | ||
| account, | ||
| bundle: { accessToken: token }, | ||
| setBundleOptions: undefined, | ||
| } | ||
| }, | ||
| async setBundle(account, bundle, setOptions) { | ||
| state.last = { account, bundle, setBundleOptions: setOptions } | ||
| }, | ||
| async clear() { | ||
| state.last = undefined | ||
|
|
@@ -151,10 +175,93 @@ describe('runOAuthFlow', () => { | |
| expect(openBrowser).toHaveBeenCalledTimes(1) | ||
| expect(await store.active()).toEqual({ | ||
| token: 'tok-1', | ||
| bundle: { | ||
| accessToken: 'tok-1', | ||
| refreshToken: undefined, | ||
| accessTokenExpiresAt: undefined, | ||
| refreshTokenExpiresAt: undefined, | ||
| }, | ||
| account: { id: '1', email: 'a@b' }, | ||
| }) | ||
| }) | ||
|
|
||
| it('persists the full bundle (refresh + access expiry + refresh expiry) when exchangeCode returns them', async () => { | ||
| const accessExpiresAt = Date.now() + 3_600_000 | ||
| const refreshExpiresAt = Date.now() + 30 * 24 * 3_600_000 | ||
| const { provider, getRedirect } = instrument({ | ||
| exchangeCode: async () => ({ | ||
| accessToken: 'tok-1', | ||
| refreshToken: 'rt-1', | ||
| expiresAt: accessExpiresAt, | ||
| refreshTokenExpiresAt: refreshExpiresAt, | ||
| }), | ||
| }) | ||
| const store = fakeStore() | ||
|
|
||
| await runOAuthFlow<Account>( | ||
| flowOptions({ provider, store, openBrowser: driveCallback(getRedirect) }), | ||
| ) | ||
|
|
||
| // Regression guard: if `runOAuthFlow` drops any of refresh / access | ||
| // expiry / refresh expiry on the floor, the snapshot would lose | ||
| // that field and silent re-auth would break. | ||
| expect(store.last?.bundle).toEqual({ | ||
| accessToken: 'tok-1', | ||
| refreshToken: 'rt-1', | ||
| accessTokenExpiresAt: accessExpiresAt, | ||
| refreshTokenExpiresAt: refreshExpiresAt, | ||
| }) | ||
| // Login is the canonical default-promotion trigger — the helper | ||
| // must thread `promoteDefault: true` through. A regression that | ||
| // dropped the flag would let an empty-default config silently | ||
| // stay un-pinned after the first login. | ||
| expect(store.last?.setBundleOptions).toEqual({ promoteDefault: true }) | ||
| }) | ||
|
|
||
| it('falls back to set(token) when the store does not implement setBundle (custom-store back-compat)', async () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P3] This back-compat case replays the whole OAuth flow just to prove the |
||
| // Custom `TokenStore` implementations predating refresh-token | ||
| // support only ship `set(account, token)`. The shared | ||
| // `persistBundle` helper degrades gracefully — login still works, | ||
| // but refresh metadata is lost (subsequent refreshes throw | ||
| // AUTH_REFRESH_UNAVAILABLE). Without this test the back-compat | ||
| // path is unexercised and a regression that always required | ||
| // `setBundle` would silently break consumers. | ||
| const setSpy = vi.fn(async (_account: Account, _token: string) => {}) | ||
| const legacyStore: TokenStore<Account> = { | ||
| async active() { | ||
| return null | ||
| }, | ||
| set: setSpy, | ||
| // No setBundle — that's the whole point. | ||
| async clear() {}, | ||
| async list() { | ||
| return [] | ||
| }, | ||
| async setDefault() {}, | ||
| } | ||
| const { provider, getRedirect } = instrument({ | ||
| exchangeCode: async () => ({ | ||
| accessToken: 'tok-legacy', | ||
| refreshToken: 'rt-1', | ||
| expiresAt: Date.now() + 3_600_000, | ||
| }), | ||
| }) | ||
|
|
||
| await runOAuthFlow<Account>( | ||
| flowOptions({ | ||
| provider, | ||
| store: legacyStore, | ||
| openBrowser: driveCallback(getRedirect), | ||
| }), | ||
| ) | ||
|
|
||
| // The legacy `set` path receives just the access token — refresh + | ||
| // expiry get dropped (correct degraded behaviour for stores that | ||
| // can't persist them). | ||
| expect(setSpy).toHaveBeenCalledTimes(1) | ||
| expect(setSpy.mock.calls[0][1]).toBe('tok-legacy') | ||
| }) | ||
|
|
||
| it('skips validateToken when exchangeCode returns an account', async () => { | ||
| const validateToken = vi.fn(async () => ({ id: 'WRONG', email: 'x@x' })) | ||
| const { provider, getRedirect } = instrument({ | ||
|
|
@@ -249,11 +356,17 @@ describe('runOAuthFlow', () => { | |
| expect(openBrowser).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('halts via AbortSignal: aborting before the callback rejects with AUTH_OAUTH_FAILED and skips store.set', async () => { | ||
| it('halts via AbortSignal: aborting before the callback rejects with AUTH_OAUTH_FAILED and skips persistence', async () => { | ||
| const controller = new AbortController() | ||
| const { provider, getRedirect } = instrument() | ||
| const store = fakeStore() | ||
| // Spy on BOTH `set` and `setBundle` so a regression where | ||
| // `runOAuthFlow` persists via `setBundle` after abort (now the | ||
| // default path with refresh-token support) wouldn't slip through. | ||
| // Reading `store.last` is the strongest guard: it stays undefined | ||
| // iff neither method ran. | ||
| const setSpy = vi.spyOn(store, 'set') | ||
| const setBundleSpy = vi.spyOn(store, 'setBundle') | ||
|
|
||
| await expect( | ||
| runOAuthFlow<Account>( | ||
|
|
@@ -272,6 +385,8 @@ describe('runOAuthFlow', () => { | |
| ), | ||
| ).rejects.toMatchObject({ code: 'AUTH_OAUTH_FAILED' }) | ||
| expect(setSpy).not.toHaveBeenCalled() | ||
| expect(setBundleSpy).not.toHaveBeenCalled() | ||
| expect(store.last).toBeUndefined() | ||
| }) | ||
|
|
||
| it('always surfaces the authorize URL via onAuthorizeUrl, even when openBrowser succeeds', async () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| export type { AuthErrorCode } from './errors.js' | ||
| export { runOAuthFlow } from './flow.js' | ||
| export type { RunOAuthFlowOptions, RunOAuthFlowResult } from './flow.js' | ||
| export { refreshAccessToken } from './refresh.js' | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P0] |
||
| export type { RefreshAccessTokenOptions } from './refresh.js' | ||
| export { attachLoginCommand } from './login.js' | ||
| export type { AttachLoginCommandOptions, AttachLoginContext } from './login.js' | ||
| export { attachLogoutCommand } from './logout.js' | ||
|
|
@@ -32,6 +34,8 @@ export type { | |
| ExchangeResult, | ||
| PrepareInput, | ||
| PrepareResult, | ||
| RefreshInput, | ||
| TokenBundle, | ||
| TokenStore, | ||
| ValidateInput, | ||
| } from './types.js' | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] This new persistence regression test still leaves
refreshTokenExpiresAtunexercised. SincerunOAuthFlownow forwards that field too, a regression dropping only the refresh-token expiry would still pass here. Add a concreterefreshTokenExpiresAtin the fixture and assert it instore.last?.bundle.