Custom getTokenInfo for authenticationMiddleware
shuhei opened this issue · 7 comments
Hi! my team started using authenticationMiddleware at work. We noticed that getTokenInfo was not using HTTP keep-alive and leaving thousands of TIME_WAIT TCP connections.
Because it is a crucial RPC call, I'd like to have the following reliability measures for getTokenInfo
- HTTP keep-alive (using HTTP agent)
- timeout
- retry
- metrics for monitoring
- circuit breaker (not sure if it makes sense for now)
But if authenticationMiddleware supports all of them directly, it will have too many options that are not directly related to authentication or the middleware. So I'd like to propose to make getTokenInfo injectable.
type GetTokenInfo<T> = (tokenInfoUrl: string, accessToken: string, logger?: Logger) => Promise<Token<T>>;
type AuthenticationMiddlewareOptions = {
tokenInfoEndpoint: string,
logger?: Logger,
onNotAuthenticatedHandler?: onNotAuthenticatedHandler,
publicEndpoints?: string[],
getTokenInfo?: GetTokenInfo<{}>
};If the getTokenInfo is not provided in the option, the middleware can use the default implementation.
I appreciate your feedback.
We discussed the issues and have to proposals:
1.) We do it as you proposed: we do not touch getTokenInfo and make it configurable in AuthenticationMiddlewareOptions. You would then not be able to "reuse" getTokenInfo but would have to implement it yourself (which is not too much, though).
2.) In addition to your proposal, we also change the signature of getTokenInfo to make it more configurable:
type GetTokenInfoOptions = {
logger?: Logger,
fetchOptions?: FetchOptions,
...
}
type GetTokenInfo<T> = (tokenInfoUrl: string, accessToken: string, options?: GetTokenInfoOptions) => Promise<Token<T>>;Then you would be able to specify things like a timeout via FetchOptions (could be something like https://www.npmjs.com/package/node-fetch#options). If you want to implement retries, monitoring, circuit breaking etc. you could reuse "our" getTokenInfo.
What is your opinion on that? For us the latter one seems more clean, we would like to configure things like timeout without reimplementing getTokenInfo and we would not mind to release a new major version (since the change would be breaking).
General remark: we should also keep in mind that there are also other places (like the TokenCache) where users want to achieve the same thing. We should think about adapting those places, too.
In my opinion, the option 1 is better at least for heavy users. The option 2 is nice for light users, but if it implements all the features that I listed above, the FetchOptions will grow very big like what perron has now, regardless of underlying library. Then authmosphere will need to change its API every time the HTTP client needs something new.
So I think the option 1 is the smallest change for most flexibility. Of course it won't hurt if the option 2 provides some nice defaults and some flexibility in addition to the option 1.
By the way, thank a lot for your quick response! I'm amazed how fast I get responses in this repo ❤️
but if it implements all the features that I listed above, the FetchOptions will grow very big like what perron has now, regardless of underlying library.
@bzums @Retro64 can we workaround this by making the GetTokenInfoOptions type open like we did for Token? Token could also be extended with other properties, we do this for uid for example.