#6656: Refresh partner token on failed requests #8466
Conversation
Playwright test results
Details
Failed testschrome › tests/bricks/sidebarEffects.spec.ts › sidebar effect bricks › toggle sidebar brick Flaky testsedgeSetup › auth.setup.ts › authenticate Skipped testsedge › tests/bricks/sidebarEffects.spec.ts › sidebar effect bricks › toggle sidebar brick |
Playwright test results - MV2
Details
Failed testschrome › tests/bricks/sidebarEffects.spec.ts › sidebar effect bricks › toggle sidebar brick Skipped testschrome › tests/extensionConsoleActivation.spec.ts › can activate a mod with built-in integration |
| /** | ||
| * Refresh an Automation Anywhere JWT. NOOP if a JWT refresh token is not available. | ||
| */ | ||
| export async function _refreshPartnerToken(): Promise<void> { |
There was a problem hiding this comment.
Moved to a new file to prevent a circular import
| @@ -253,31 +254,6 @@ async function proxyRequest<T>( | |||
| }; | |||
| } | |||
|
|
|||
| function isAuthenticationError(error: Pick<AxiosError, "response">): boolean { | |||
There was a problem hiding this comment.
Moved to apiClient.ts
| @@ -60,15 +93,7 @@ export async function getLinkedApiClient(): Promise<AxiosInstance> { | |||
| throw new ExtensionNotLinkedError(); | |||
| } | |||
|
|
|||
| return axios.create({ | |||
There was a problem hiding this comment.
This code was exactly the same as the code in getApiClient so I'm using that to DRY out the code. And so we don't have to create the interceptor multiple times, see createAuthRefreshInterceptor.
We may actually want to merge getApiClient and getLinkedApiClient into a single function (and introduce a flag for throwing an error if the extension is not linked) because they're the same besides that check.
There was a problem hiding this comment.
We may actually want to merge getApiClient and getLinkedApiClient into a single function (and introduce a flag for throwing an error if the extension is not linked) because they're the same besides that check.
I had them as two separate methods so the intent is explicit. I'll defer to engineering on their preference, but my preference would be to keep them separate. (If we do use a single method, I'd want the flag to be required)
johnnymetz
force-pushed
the
feature/6656-refresh-partner-token-on-failed-requests
branch
from
a8d0ba6
to
fbbcfbe
Compare
twschiller
added
enhancement
|
There's a fundamental architecture problem here that we probably need to re-think. The api client auth interceptor is trying to use the partner integration to refresh it's auth token, but that calls the integrations registry, which tries to refresh the remote registry, which tries to use the same api client (the same code, different call to I'm also pretty concerned that we're configuring and instantiating a new instance of api client on every call to |
That's a good callout. Some thoughts:
Could you clarify your concern - is it performance? There should be negligible overhead for creating new instances. Whether or not it's a single seems to be an implementation detail to me. The caller doesn't care, it just needs to be able to get a client that meets its requirements (i.e., whether it needs to make an authenticated call) @johnnymetz @grahamlangford let's make sure engineering is aligned on the architecture concerns before going too far down the particular implementation |
My concern is just that it feels like a very non-standard way to use an API client. I would have to look into it, but my instinct is that there may be some sort of caching happening in the axios instance that we're losing by doing this. I'm also concerned that, although it may seem negligible now, the more api calls our application makes the more impact this will have will creating all the new objects every time there's an api call. I'm concerned that this is a "foot-gun" that we're just leaving in the product because it doesn't seem like it makes a difference one way or another right now. Is there a chance this would be a memory leak with all the instances if you use the extension over a long period of time on one session? I'm not sure I see what the difference is with just changing this function into a module constant in the same place? If there's a reason we should leave it like this I'm open to discussion though. Can also be addressed separate from this refresh token PR for sure. |
Yeah, I think this is probably what we'll have to do for now, unless we want to save the entire config somewhere that's associated with the refresh token. That, to me, would be in the spirit of the concept of a "refresh token" where it's something that can be used to start a new auth session. But it sort of goes against our registry concept, and creates a place where the stored config could get out of sync with the registry possibly. We'll probably need to split out a separate locator for "local integration config locator" or something so that we can decouple it from anything that uses |
Someone had asked in the axios project and there's no mention of performance considerations: axios/axios#3365 Agree it makes sense to understand if there's any tradeoff. My instinct is that caching is done by the browser, not axios (fwiw, there is a separate caching adapter https://www.npmjs.com/package/axios-cache-adapter)
I wouldn't bet on it currently. We've been using the existing approach for 2+ years. But yes, there's always a chance we might introduce something that keeps a reference to the client instance
As mentioned, I'm fine using a singleton. I just don't see any urgency around it (engineering can prioritize for DevEx). At the end of the day, the caller cares about:
|
Sure, probably not any urgency related to anything broken, I agree |
This comment has been minimized.
This comment has been minimized.
What does this PR do?
Discussion
Demo
https://www.loom.com/share/372cd8d5bacc4fa18ae2e5d847363204
Team Coordination
Checklist