Simplify OAuth refreshToken middleware by using OAuth service in the middleware#13805
Simplify OAuth refreshToken middleware by using OAuth service in the middleware#13805rickyrombo wants to merge 16 commits intomjp-oauth-net-newfrom
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the SDK’s OAuth/token-refresh flow to reduce dependencies on generated API clients and to allow OAuth to be instantiated earlier so it can be injected into middleware.
Changes:
- Remove
UsersApidependency fromOAuth(dropisWriteAccessGranted/verifyToken) and perform token verification via direct HTTP request. - Simplify
addTokenRefreshMiddlewareto delegate refresh behavior toOAuth.refreshAccessToken()and retry with the refreshed access token. - Initialize
OAuthearlier increateSdkWithoutServicesand update middleware unit tests to mockOAuthrather thanfetch.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk/src/sdk/oauth/OAuth.ts | Removes UsersApi coupling; implements manual token verification and changes refresh API to return the new access token. |
| packages/sdk/src/sdk/middleware/addTokenRefreshMiddleware.ts | Switches refresh logic from inline fetch/validation to calling oauth.refreshAccessToken(). |
| packages/sdk/src/sdk/middleware/addTokenRefreshMiddleware.test.ts | Updates tests to mock OAuth.refreshAccessToken() instead of mocking cross-fetch. |
| packages/sdk/src/sdk/createSdkWithoutServices.ts | Instantiates OAuth earlier and injects it into the token refresh middleware. |
| packages/sdk/src/sdk/createSdkWithServices.ts | Removes usersApi being passed into OAuth (but currently doesn’t supply basePath). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| apiKey, | ||
| usersApi: apis.users, | ||
| logger: services.logger | ||
| }) |
There was a problem hiding this comment.
OAuth now requires basePath for implicit token verification and PKCE exchanges (it calls /users/verify_token and /oauth/token via this.config.basePath). In createSdkWithServices, OAuth is instantiated without basePath, so oauth.login() will always surface "basePath is required for token verification." and fail in the browser. Pass the same basePath used for API clients into the OAuth constructor (and consider also wiring a tokenStore if PKCE/refresh is expected here).
| @@ -87,12 +97,10 @@ export const createSdkWithoutServices = (config: SdkConfig) => { | |||
| } | |||
|
|
|||
| // Auto-refresh middleware — intercepts 401s and retries with a fresh token. | |||
| if (apiKey) { | |||
| if (apiKey && oauth) { | |||
| middleware.push( | |||
| addTokenRefreshMiddleware({ | |||
| tokenStore, | |||
| apiKey, | |||
| basePath | |||
| oauth | |||
| }) | |||
| ) | |||
| } | |||
There was a problem hiding this comment.
This change gates the token-refresh middleware on oauth being defined, but oauth is only created when window exists. That means auto-refresh on 401 no longer runs in Node/SSR even if a refresh token is present in OAuthTokenStore (previously it did). If server-side refresh is still a supported use case, consider keeping a non-browser refresh path (e.g., allow the middleware to accept tokenStore/apiKey/basePath again, or refactor refresh into a window-independent helper/service).
| export const addTokenRefreshMiddleware = ({ | ||
| tokenStore, | ||
| apiKey, | ||
| basePath | ||
| oauth | ||
| }: { | ||
| tokenStore: OAuthTokenStore | ||
| apiKey: string | ||
| basePath: string | ||
| oauth: OAuth | ||
| }): Middleware => { |
There was a problem hiding this comment.
addTokenRefreshMiddleware now depends on an OAuth instance, but OAuth cannot be constructed outside the browser (constructor throws when window is undefined). This effectively prevents using this middleware in non-browser environments unless callers type-cast a mock. Consider loosening the dependency to a small interface (e.g., something with refreshAccessToken(): Promise<string | null>) so server-side consumers can still supply their own refresh implementation.
| @@ -534,19 +511,19 @@ export class OAuth { | |||
| }) | |||
| }) | |||
There was a problem hiding this comment.
refreshAccessToken() sends client_id: this.apiKey, but this.apiKey can be null (e.g., OAuth constructed without an apiKey and called directly). That will yield a malformed refresh request and makes failures harder to diagnose. Add an explicit guard that surfaces an error and returns null when this.apiKey is missing.
| const tokens = await res.json() | ||
| if (tokens.access_token && tokens.refresh_token) { | ||
| this.config.tokenStore.setTokens( | ||
| tokens.access_token, | ||
| tokens.refresh_token | ||
| ) | ||
| return true | ||
| return tokens.access_token | ||
| } | ||
| return false | ||
| return null |
There was a problem hiding this comment.
refreshAccessToken() only checks truthiness of tokens.access_token / tokens.refresh_token before storing them. This is weaker than the previous middleware validation and could store non-string or empty token values (e.g., { access_token: {}, refresh_token: [] }). Validate that both fields are non-empty strings before updating the token store and returning the new access token.
| @@ -534,19 +511,19 @@ export class OAuth { | |||
| }) | |||
| }) | |||
| if (!res.ok) { | |||
| return false | |||
| return null | |||
| } | |||
| const tokens = await res.json() | |||
| if (tokens.access_token && tokens.refresh_token) { | |||
| this.config.tokenStore.setTokens( | |||
| tokens.access_token, | |||
| tokens.refresh_token | |||
| ) | |||
| return true | |||
| return tokens.access_token | |||
| } | |||
| return false | |||
| return null | |||
| } catch { | |||
| return false | |||
| return null | |||
| } | |||
| } | |||
There was a problem hiding this comment.
The refresh-token exchange logic moved from addTokenRefreshMiddleware (previously covered by unit tests for invalid JSON / missing fields / etc.) into OAuth.refreshAccessToken(), but there are no tests covering the new behavior. Add focused unit tests for refreshAccessToken() (success path updates the token store + returns the access token; failure paths like non-OK response, invalid JSON, missing/invalid fields, and missing apiKey/basePath/tokenStore).
| post: async (context: ResponseContext): Promise<Response | void> => { | ||
| if (context.response.status !== 401) { | ||
| return context.response | ||
| } | ||
|
|
||
| // Snapshot the refresh token — it may be cleared concurrently. | ||
| const currentRefreshToken = tokenStore.refreshToken | ||
| if (!currentRefreshToken) { | ||
| return context.response | ||
| } | ||
|
|
||
| // Coalesce concurrent 401s into a single refresh call. | ||
| if (!refreshInFlight) { | ||
| refreshInFlight = exchangeRefreshToken( | ||
| currentRefreshToken, | ||
| apiKey, | ||
| basePath | ||
| ).finally(() => { | ||
| refreshInFlight = null | ||
| }) | ||
| refreshInFlight = oauth | ||
| .refreshAccessToken() | ||
| .catch(() => null) | ||
| .finally(() => { | ||
| refreshInFlight = null | ||
| }) | ||
| } | ||
|
|
||
| const newTokens = await refreshInFlight | ||
| if (!newTokens) { | ||
| // Refresh failed — surface the original 401. | ||
| const newAccessToken = await refreshInFlight | ||
| if (!newAccessToken) { | ||
| return context.response |
There was a problem hiding this comment.
Behavior change: the middleware now calls oauth.refreshAccessToken() on every 401, even when the client is unauthenticated. Since refreshAccessToken() surfaces errors for expected states like "No refresh token available", this can generate noisy logs/error callbacks on normal 401s. Consider adding a cheap guard before attempting refresh (e.g., expose a hasRefreshToken/getRefreshToken on OAuth/tokenStore, or make refreshAccessToken() silent for missing refresh token and simply return null).
Fixes UI for tracks/collections that dont have artwork
Currently when a user comes back from guest account sign up, they enter a sign up purgatory loop where they can't finish their account
- `publishTracks` returns string track IDs, which were being incorrectly parsed as though they were numbers that needed converting. This was changed behavior from recent SDK changes made to match the POST endpoints as this was working previously - `createPlaylist` wasn't equipped to handle using a preset `playlistId` like our client expects, rejecting calls that had `playlistId` already set in the metadata (which would happen on our creation of playlists from scratch). - `createPlaylistInternal` was being passed parsed parameters in the `createPlaylist` case, and unparsed in the `uploadPlaylist` case, and used types that made it hard to squeeze both callsites in. This was resulting in incorrectly setting some IDs to hash IDs (eg in `playlistContents`) and was uncovered when fixing the playlistId bug above - `updatePlaylist` had incorrect schema still referencing `coverArtCid` instead of `playlistImageSizesMultihash`, blocking any playlist updates that included an image update --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1. Top nav CTA If the user is unauthenticated, the button should say “Sign Up.” If the user is authenticated, the button should say “Launch.” 2. Hero CTA The main hero CTA should link to /trending (not Sign Up). 3. Bottom CTA Change the copy from “Get Started” to “Start Exploring.” Link this button to /explore.
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @audius/sdk@14.0.0 ### Major Changes - d864806: Remove getPlaylistByHandleAndSlug in favor of getBulkPlaylists - Removes `sdk.playlists.getPlaylistByHandleAndSlug()` in favor of calling `sdk.playlists.getBulkPlaylists({ permalink: ['/handle/playlist/playlist-name-slug'] })` - Changes return values of `CommentsAPI` to match other APIs, removing `success` param. ### Minor Changes - 71bb31b: Add programmable distribution config to stream_conditions ### Patch Changes - 71bb31b: Fix missing bearer token for PUT /users - a7a9e17: Fix create/upload/update playlist in legacy path - `publishTracks` returns string track IDs, which were being incorrectly parsed as though they were numbers that needed converting. This was changed behavior from recent SDK changes made to match the POST endpoints as this was working previously - `createPlaylist` wasn't equipped to handle using a preset `playlistId` like our client expects, rejecting calls that had `playlistId` already set in the metadata (which would happen on our creation of playlists from scratch). - `createPlaylistInternal` was being passed parsed parameters in the `createPlaylist` case, and unparsed in the `uploadPlaylist` case, and used types that made it hard to squeeze both callsites in. This was resulting in incorrectly setting some IDs to hash IDs (eg in `playlistContents`) and was uncovered when fixing the playlistId bug above - `updatePlaylist` had incorrect schema still referencing `coverArtCid` instead of `playlistImageSizesMultihash`, blocking any playlist updates that included an image update - 8f12bb7: Fix cover art CID metadata properties for playlists and tracks. - 6cb4b6f: Fix UploadsApi to make start() a function ## @audius/sdk-legacy@6.0.21 ### Patch Changes - Updated dependencies [71bb31b] - Updated dependencies [71bb31b] - Updated dependencies [a7a9e17] - Updated dependencies [d864806] - Updated dependencies [8f12bb7] - Updated dependencies [6cb4b6f] - @audius/sdk@14.0.0 ## @audius/sp-actions@1.0.25 ### Patch Changes - @audius/sdk-legacy@6.0.21 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
… entity manager (at least for now) and add support to services-based init + oauth
Removes the deps on usersApi by removing the
isWriteAccessGrantedmethod (which is redundant to the devApps APIs) and theverifyTokenmethod (which is redundant with the usersApi) and calling fetch manually internally.Then, this allows the OAuth service to be initialized before other APIs so that it can be used in the config for middlewares to all other APIs.