Skip to content
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

fix: use provided fetcher to fetch jwks #37

Merged

Conversation

bukowskiadam
Copy link
Contributor

I had to do some magic tricks to keep the fetcher's interface the same because JwksClient expects to just return the jwks.

This PR is more for a discussion, maybe we should use a different fetch implementation for this purpose. I've checked it and JwksClient uses node http or https agent.

@joawan
Copy link
Member

joawan commented Apr 23, 2024

JwksClient also supports overriding requestAgent but I'd say the fetcher is more flexible. I see the magic, but looks alright. It's possible to add some extra magic, to not send in { fetcher: false }, but might be be unnecessary.

I noticed that the custom fetcher removes support for timeout, requestHeaders, requestAgent. timeout is a configurable via the jwks_timeout setting of the lib. This change will always ignore that timeout, since fetch is always set in options when using main entry point.

This lib uses node-fetch v2 at the comment and it supports timeout but recommends using AbortSignal instead. Timeout option is removed in node-fetch v3.

I'll explore a bit what to do with the timeout option. Could be we just add it as an option and for the v2 case.

@bukowskiadam
Copy link
Contributor Author

I get the point of the timeout and using our node-fetch always in JwksClient and that's changing too much. So maybe something like this:
image
image
Separate fetch key from the fetch jwks?

@joawan
Copy link
Member

joawan commented Apr 23, 2024

That's another option that limits unforeseen consequences a lot. Would just need to make a note in the docs that timeout isn't respected when using custom jwks fetcher and that those would be needed to be handled by the custom fetcher.

@bukowskiadam
Copy link
Contributor Author

Sorry, I'm pretty busy with other stuff. I'll redo it as we agreed ☝🏻 and push commits here

Copy link
Member

@joawan joawan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joawan joawan merged commit 6e1461c into schibsted:master May 15, 2024
1 check passed
@bukowskiadam bukowskiadam deleted the fix/use-provided-fetcher-to-fetch-jwk branch May 15, 2024 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants