-
Notifications
You must be signed in to change notification settings - Fork 413
Allow refresh: true in getAccessToken() #2055
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2055 +/- ##
==========================================
+ Coverage 80.02% 82.53% +2.51%
==========================================
Files 21 21
Lines 1907 1941 +34
Branches 315 342 +27
==========================================
+ Hits 1526 1602 +76
+ Misses 376 333 -43
- Partials 5 6 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Thanks for prioritising this @tusharpandey13. I have it as a blocker for upgrading at the moment which blocks our NextJS 15 upgrade. Just wondering if you have a rough idea of when this might get completed and released? |
We'll release this soon, ETA might be tomorrow EOD |
Any more updates on when we can expect to see this go in? Right now it's a blocker on NextJS 15 upgrade which due to atrocious dev server performance in NextJS 14 is impacting our team's productivity heavily. Frustrating to see what feels ike a very simple change being dragged out. |
these changes have been added
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.
Pull Request Overview
This PR introduces a new option (refresh) to force a token refresh in getAccessToken(), addressing issue #1884. Key changes include updating method overloads in the Auth0Client, adding a new forceRefresh parameter to the AuthClient’s getTokenSet method, and updating integration tests and documentation to cover this new behavior.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/server/client.ts | Updated getAccessToken overloads to accept refresh option. |
src/server/client.test.ts | Added integration tests for forcing token refresh. |
src/server/auth-client.ts | Updated getTokenSet to support forced refresh logic. |
EXAMPLES.md | Added documentation examples for forcing token refresh. |
Comments suppressed due to low confidence (1)
src/server/client.ts:349
- [nitpick] The parameter names 'arg1', 'arg2', and 'arg3' are ambiguous. Consider renaming them to more descriptive names (e.g., 'reqOrOptions', 'res', 'options') to improve code clarity.
arg1?: PagesRouterRequest | NextRequest | GetAccessTokenOptions,
@@ -0,0 +1,472 @@ | |||
/** | |||
* @fileoverview |
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 the seperate file is a good idea, but I would propose to:
- move the file to
client/get-access-token.test.ts
- Also, as above, drop the
integration
in the file name.
* 1. `AuthClient.getTokenSet` correctly uses the refresh token grant to obtain a new | ||
* token set from the authorization server. |
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.
This shouldnt matter for this test, as this is an implementation detail.
* @param {string} [options.refreshedRefreshToken="refreshed-refresh-token"] - Refresh token to return on refresh. | ||
* @returns {Promise<vi.Mock>} A Vitest mock function simulating `fetch`. | ||
*/ | ||
async function getMockAuthorizationServer({ |
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 would recommend to look into MSW to mock Http calls, see https://github.com/auth0/auth0-auth-js/blob/early-access/packages/auth0-server-js/src/server-client.spec.ts
vi.spyOn(Auth0Client.prototype as any, "getSession").mockResolvedValue( | ||
initialSession | ||
); |
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.
Let's refrain from mocking the getSession, instead, mock the actual session store.
initialSession | ||
); | ||
const mockSaveToSession = vi | ||
.spyOn(Auth0Client.prototype as any, "saveToSession") |
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.
Let's refrain from mocking the saveToSession, but mock the session store.
* it refreshes the token and saves the updated session. | ||
*/ | ||
it("should refresh token for pages-router overload when refresh is true", async () => { | ||
const initialExpiresAt = Math.floor(Date.now() / 1000) - 60; |
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.
Why is this test working around an expired token, rather than testing that this works when the token is not expired?
* so the test asserts this observed behavior. | ||
*/ | ||
it("should refresh token for app-router overload when refresh is true", async () => { | ||
const initialExpiresAt = Math.floor(Date.now() / 1000) - 60; |
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.
Why is the token expired in the past, when this test is explicitly to test non-expired token refresh?
const fetchCalls = mockFetch.mock.calls; | ||
expect(fetchCalls.length).toBeGreaterThan(0); | ||
const tokenEndpointCall = fetchCalls.find((call: any) => { | ||
let urlString: string; | ||
if (call[0] instanceof URL) { | ||
urlString = call[0].toString(); | ||
} else if (call[0] instanceof Request) { | ||
urlString = call[0].url; | ||
} else { | ||
urlString = call[0]; | ||
} | ||
return urlString.endsWith("/oauth/token"); | ||
}); |
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.
We do not need to assert this, we know this is the case by asserting the returned values. If we want to assert this, I would propose to do it differently, as this doesnt look like what we want.
expect(result).not.toBeNull(); | ||
expect(result?.token).toBe(expectedRefreshedAccessToken); | ||
expect(result?.expiresAt).toBeGreaterThan(initialExpiresAt); | ||
const expectedExpiresAtRough = | ||
Math.floor(Date.now() / 1000) + expectedRefreshedExpiresIn; | ||
expect(result?.expiresAt).toBeGreaterThanOrEqual( | ||
expectedExpiresAtRough - 5 | ||
); | ||
expect(result?.expiresAt).toBeLessThanOrEqual(expectedExpiresAtRough + 5); | ||
expect(result?.scope).toBe(initialTokenSet.scope); // Scope remains initial | ||
|
||
// 2. Assert saveToSession WAS called (matches current behavior) | ||
expect(mockSaveToSession).toHaveBeenCalledOnce(); | ||
const savedSession = mockSaveToSession.mock.calls[0][0] as SessionData; | ||
expect(savedSession.tokenSet.accessToken).toBe( | ||
expectedRefreshedAccessToken | ||
); | ||
expect(savedSession.tokenSet.refreshToken).toBe( | ||
expectedRefreshedRefreshToken | ||
); | ||
expect(savedSession.tokenSet.expiresAt).toBe(result?.expiresAt); | ||
expect(savedSession.tokenSet.scope).toBe(initialTokenSet.scope); | ||
|
||
// 3. Assert mockFetch was called | ||
const fetchCalls = mockFetch.mock.calls; | ||
expect(fetchCalls.length).toBeGreaterThan(0); | ||
const tokenEndpointCall = fetchCalls.find((call: any) => { | ||
let urlString: string; | ||
if (call[0] instanceof URL) { | ||
urlString = call[0].toString(); | ||
} else if (call[0] instanceof Request) { | ||
urlString = call[0].url; | ||
} else { | ||
urlString = call[0]; | ||
} | ||
return urlString.endsWith("/oauth/token"); | ||
}); | ||
expect(tokenEndpointCall).toBeDefined(); | ||
}); |
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.
This is a test that tests and asserts a lot of things. We can easily split this in several tests to make it more clear what is being tested.
Fixes : #1884
Changes
refresh?: boolean
option togetAccessToken()
Testing
PASSING
Usage
App Router (Server Components, Route Handlers, Server Actions):
When calling
getAccessToken
without request and response objects, you can pass an options object as the first argument. Set therefresh
property totrue
to force a token refresh.Pages Router (getServerSideProps, API Routes):
When calling
getAccessToken
with request and response objects (fromgetServerSideProps
context or an API route), the options object is passed as the third argument.