#6656 Partner refresh tokens #8650
Conversation
Playwright test results
Details
Skipped testschrome › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options |
src/auth/authStorage.ts
Outdated
| // // https://developer.chrome.com/docs/extensions/reference/identity/#method-clearAllCachedAuthTokens | ||
| // await chromeP.identity.clearAllCachedAuthTokens(); | ||
|
|
||
| const partnerAuthData = await partnerTokenStorage.get(); | ||
| if (partnerAuthData?.token) { | ||
| console.debug( | ||
| "Clearing partner auth for authId: " + partnerAuthData.authId, | ||
| ); | ||
| await chromeP.identity.removeCachedAuthToken({ | ||
| token: partnerAuthData.token, | ||
| }); | ||
| } |
There was a problem hiding this comment.
I need to test this some more to make sure it works properly, but I think we might be able to get away with only clearing the partner token from the identity cache, so that other oauth2 sessions are not invalidated also, like a gsuite login for ex.
…lso some refactoring to help with testing
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/background/auth/partnerIntegrations/launchAuthIntegration.test.ts
Dismissed
Show resolved
Hide resolved
| console.debug( | ||
| "Clearing partner auth for authId: " + partnerAuthData.authId, | ||
| ); | ||
| await removeOAuth2Token(partnerAuthData.token); |
There was a problem hiding this comment.
Maybe leave a comment as to why we want this instead of clearAllCachedAuthTokens which we used previously
| refreshToken: newRefreshToken as string, | ||
| refreshParamPayload: { | ||
| ...refreshParamPayload, | ||
| refresh_token: newRefreshToken as string, |
There was a problem hiding this comment.
To DRY this out, maybe just add the refresh token to postData above so we're not duplicating the refresh token value
|
|
||
| let apiClientSetupPromise: Promise<void> | null = null; | ||
|
|
||
| async function safeSetupClient(): Promise<void> { |
There was a problem hiding this comment.
Two questions:
- Why do we need the try/catch block? What kind of errors do we expect to run into? And if there is an error, why not throw it?
- Why return a promise instead of just returning the axios instance?
There was a problem hiding this comment.
You're probably right, not sure what error would ever be thrown from the setup function, so probably don't need the try-catch around that. I had an error being thrown explicitly from inside the setup function at one point when I was working on this, I think that's where this came from.
For the promise question, are you talking about combining the promise and client instance and making it just one stored Promise<AxiosInstance>? Or am I misunderstanding what you are talking about? I suppose I could do it that way with one promise, I'll have to change the code and see if I like how it turns out or not 🤔
There was a problem hiding this comment.
Nevermind about the promise question. I wasn't understanding it correctly
|
@BLoe it looks like there's some merge conflicts now |
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
What does this PR do?
Demo
https://www.loom.com/share/ecdd2d683cb74279aa9fa98fe7351c04
Reviewer Tips
apiClient.ts,launchAuthIntegration, andrefreshPartnerAuthenticationChecklist