-
Notifications
You must be signed in to change notification settings - Fork 36
feat: configure transparent OAuth token refresh via auth.oauth #1323
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
base: main
Are you sure you want to change the base?
Changes from all commits
be45796
dcbafe4
a7982d0
3e0750f
0c4b143
28fe7d0
3cbe983
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| <!-- auto-generated --> | ||
| --- | ||
| '@sanity/client': minor | ||
| --- | ||
|
|
||
| feat: configure transparent OAuth token refresh via auth.oauth |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| import type {EventSourceFetchInit, FetchLikeResponse} from 'eventsource' | ||
| import type {FetchFunction, FetchInit} from 'get-it' | ||
|
|
||
| import type {InitializedClientConfig} from '../types' | ||
| import {resolveOAuthToken} from '../http/oauthRefreshHandler' | ||
| import type {InitializedClientConfig, OAuthTokenSetup} from '../types' | ||
|
|
||
| /** @internal */ | ||
| export interface EventSourceFetchOptions { | ||
|
|
@@ -11,6 +12,23 @@ export interface EventSourceFetchOptions { | |
| * etc. — things the native EventSource API has no equivalent for. | ||
| */ | ||
| headers?: Record<string, string> | ||
| /** | ||
| * Last event id from a prior EventSource instance. Used when the client has | ||
| * to construct a fresh EventSource itself (eg after an OAuth refresh on a | ||
| * rejected reconnect) so the new instance can resume from the previous | ||
| * position. The `eventsource` package's own reconnect header, when present, | ||
| * still wins. | ||
| */ | ||
| lastEventId?: string | ||
| /** | ||
| * OAuth token setup to resolve an `Authorization` header from. Resolved via | ||
| * `resolveOAuthToken()` on every request — not once per connection — so the | ||
| * `eventsource` package's reconnects pick up a refreshed token, and a token | ||
| * about to expire is refreshed proactively. 401-driven `refresh()` still | ||
| * lives upstream in `reconnectOnConnectionFailure`. Config `headers` take | ||
| * precedence, mirroring the string-token merge order. | ||
| */ | ||
| tokenSetup?: OAuthTokenSetup | ||
| /** | ||
| * If the client was configured with `withCredentials: true`, the | ||
| * resolved fetch forwards `credentials: 'include'` so the browser | ||
|
|
@@ -50,19 +68,29 @@ export function resolveEventSourceFetch( | |
| options: EventSourceFetchOptions = {}, | ||
| ): EventSourceFetch { | ||
| const extraHeaders = options.headers | ||
| const lastEventId = options.lastEventId | ||
| const tokenSetup = options.tokenSetup | ||
| const credentials: FetchInit['credentials'] = options.withCredentials ? 'include' : undefined | ||
|
|
||
| return function eventSourceFetch(url, init) { | ||
| return async function eventSourceFetch(url, init) { | ||
| const baseFetch = pickBaseFetch(config) | ||
|
|
||
| // Extra `EventSourceFetchInit` fields get-it's `FetchInit` doesn't | ||
| // declare (`mode`, `cache`) survive the spread and reach whichever | ||
| // fetch implementation is effective. | ||
| const mergedInit: FetchInit = {...init} | ||
| if (extraHeaders) { | ||
| if (extraHeaders || tokenSetup || lastEventId) { | ||
| const headers = new Headers(init?.headers) | ||
| for (const [key, value] of Object.entries(extraHeaders)) { | ||
| headers.set(key, value) | ||
| if (lastEventId && !headers.has('last-event-id')) { | ||
| headers.set('Last-Event-ID', lastEventId) | ||
|
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. EventSource instances also tracks Last-Event-ID internally, how will these interact? We need to be extra careful here so we don't lose events or get duplicated delivery. Find it a bit hard to judge whether that's a real concern though 😅
Member
Author
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.
I've (claude) tried to add tests to cover this. Beyond that i'm not sure how else we can test this – open to ideas. |
||
| } | ||
| if (tokenSetup) { | ||
| headers.set('Authorization', `Bearer ${await resolveOAuthToken(tokenSetup)}`) | ||
| } | ||
| if (extraHeaders) { | ||
| for (const [key, value] of Object.entries(extraHeaders)) { | ||
| headers.set(key, value) | ||
| } | ||
| } | ||
| mergedInit.headers = headers | ||
| } | ||
|
|
@@ -71,7 +99,7 @@ export function resolveEventSourceFetch( | |
| } | ||
| // get-it's `FetchResponse` is a structural superset of the package's | ||
| // `FetchLikeResponse`, so it can be handed over as-is. | ||
| return baseFetch(typeof url === 'string' ? url : url.href, mergedInit) | ||
| return await baseFetch(typeof url === 'string' ? url : url.href, mergedInit) | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
I think this will break resumability as it will trigger a new listener request, which instantiates a new EventSource rather than reusing the existing one, bypassing the existing EventSource's built-in error/retry mechanism (which sends the Last-Event-ID header on reconnect, telling the server where to resume from).
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.
I've had a look at producing tests which hopefully disproves your concern - let me know if i've missed a case worth having!