feat(auth): add auth headers provider and interceptor options#255
feat(auth): add auth headers provider and interceptor options#255
Conversation
| @@ -0,0 +1,6 @@ | |||
| export type AuthHeadersProvider = () => Promise<Record<string, string>> | Record<string, string>; | |||
|
|
|||
| export interface AuthInterceptorOptions { | |||
There was a problem hiding this comment.
since this type is exactly the same as ClientOptions
you could do something like export type AuthInterceptorOptions = Omit<ClientOptions, 'baseUrl'> or even request it like this in the param where its used
| if (options.authHeadersProvider) { | ||
| instance.interceptors.request.use(createRequestInterceptor(options.authHeadersProvider)); | ||
| } | ||
|
|
||
| if (options.onUnauthorized) { |
There was a problem hiding this comment.
Having a look a this, does it really need to be optional these handler?
There was a problem hiding this comment.
It is necessary to keep it that way; the auth client does not send those parameters. In the future, we should try to unify those clients; it is very rare to have so many.
| function getHeaderValue(headers: unknown, key: string): string | undefined { | ||
| if (!headers || typeof headers !== 'object') { | ||
| return undefined; | ||
| } | ||
|
|
||
| if ('get' in headers && typeof headers.get === 'function') { | ||
| const value = headers.get(key); | ||
| return typeof value === 'string' ? value : undefined; | ||
| } | ||
|
|
||
| for (const [headerKey, headerValue] of Object.entries(headers as Record<string, unknown>)) { | ||
| if (headerKey.toLowerCase() === key.toLowerCase() && typeof headerValue === 'string') { | ||
| return headerValue; | ||
| } | ||
| } | ||
|
|
||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Why is this function actually needed?
There was a problem hiding this comment.
It is better to keep the function, avoid problems with duplicate headers or replacement of headers sent directly from the request.
| }); | ||
| }); | ||
|
|
||
| it('should not overwrite a header already present in the request', async () => { |
There was a problem hiding this comment.
Then, we would have no way to refresh the token?
There was a problem hiding this comment.
There is no way to do this. We would have to verify for each request whether the token is about to expire, and due to race conditions, it is possible that two requests could refresh and end up revoking each other's tokens, causing problems.
There was a problem hiding this comment.
You could try having some shared state across requests to work around the race condition right?
| expect((result.headers as Record<string, string>).Authorization).toBe('Bearer existing'); | ||
| }); | ||
|
|
||
| it('should resolve async authHeadersProvider', async () => { |
There was a problem hiding this comment.
So this is the counterpart test of the last one right? if no header then add it
|
|
||
| const onRejected = (error: AxiosError): Promise<never> => { | ||
| if (axios.isAxiosError(error) && error.response?.status === 401) { | ||
| onUnauthorized(); |
There was a problem hiding this comment.
So, we are not going to refresh the token?
There was a problem hiding this comment.
There is no way to do this. We would have to verify for each request whether the token is about to expire, and due to race conditions, it is possible that two requests could refresh and end up revoking each other's tokens, causing problems.
|
2 participants
What is Changed / Added
The assignment of authentication headers has been centralized so that it does not have to be repeated for each request.
Why