Add option to pass custom fetch implementation

[vabatta]

Hi,

I was wondering if it would be possible to add an option to method which uses fetch under the hood to actually have a custom fetch implementation be passed. This would be useful for cases where the fetch is actually enriched with custom functionalities (for example in SSR in SvelteKit).

I had a quick look at the source code, and my idea would have been to add an extra optional parameter with the default being fetch to all method which uses it, making the change retro-compatible.

export async function discoveryRequest(
  issuerIdentifier: URL,
  options?: DiscoveryRequestOptions,
  fetch: typeof fetch = fetch
): Promise<Response> {

  // ...

  // this will use shadowed `fetch` parameter instead of global `fetch`
  return fetch(url.href, {
    headers,
    method: 'GET',
    redirect: 'manual',
    signal: options?.signal ? signal(options.signal) : null,
  }).then(processDpopNonce)
}

[panva]

Hi @vabatta,

I don't plan allowing this right now, mostly because I don't see how useful that is. Could you elaborate what this would actually allow you to do?

[nicklascarnegie]

I'm still not seeing what this would bring to this module if you're mostly talking to third party providers...

For SvelteKit users there's at least one reason why this would be useful, and that is to get rid of this warning in the console;

Loading http://auth.server/realms/myrealm/.well-known/openid-configuration using `window.fetch`. For best results, use the `fetch` that is passed to your `load` function: https://kit.svelte.dev/docs/load#making-fetch-requests

[nicklascarnegie]

So, adding this feature will probably not add any value to this module directly.. but it will surely make users of SvelteKit + oauth4webapi a bit happier.

[panva]

Can't you overwrite the fetch and Response globals as a one-time all module fixing solution?

Providing own fetch doesn't help if the returned Response instance is not also instanceof global Response.

[nicklascarnegie]

Can't you overwrite the fetch and Response globals as a one-time all module fixing solution?

Absolutely, and perhaps that's the way to go here.. ?

[nicklascarnegie]

It would feel a bit more clean to be able to pass it in instead of overwriting the global fetch.

[panva]

Absolutely, and perhaps that's the way to go here.. ?

I think it absolutely is.

[melbourne2991]

There are additional reasons for wanting to pass in a custom fetch implementation

• logging/tracing of Request/Response pairs.
• caching - for instance Deno and Cloudflare workers support a Cache API that mirrors the browser's api, which is keyed by Request. Caching of .well-known & JWKs endpoints is likely to be a common requirement.

Because I do not have access to the Request object created by the library and cannot provide a custom fetch implementation this is impossible without

• Creating an "artificial request" to serve as a key
• Messing around with globals
• Forking the repo and patching

The library would be much ergonomic if this was possible.

This would also solve: #79 and #20

I would be happy to make the changes and submit a PR if greenlighted

[panva]

Well okay, why don't you start with a proposal of what the surface and use of this would look like. I'm looking for three examples

• logging/tracing of any Request/Response pairs
• caching well-known and jwks in deno (and separate examplr with cf workers)

[panva]

@melbourne2991 ping

Imagine the HttpRequestOptions interface having a fetch-like option that takes a Request instance and is supposed to return Promise<Response> where Response is an instanceof globalThis.Response.

How would that enable

• logging/tracing of any Request/Response pairs
• caching well-known and jwks in deno (and separate example with cf workers)

[melbourne2991]

Yep that would work.

You could use a fetch-like API but the fetch api is pretty simple and mostly consistent across environments.

The below code would work across all three environments (cf, deno, browser)

const trace_input = (input: RequestInfo | URL) => {
  // ...
}

const trace_response = (response: Response) => {
  // ...
}

const should_cache = (response: Response): boolean => {
  // ...
}

const cached_response_still_valid = (response: Response): boolean => {
  // ...
}

const cache = await caches.open("jwks-cache");

const custom_fetch: typeof fetch = async (input, init) => {
  const req = new Request(input, init);

  trace_input(req);

  const cached_response = await cache.match(req);

  if (cached_response) {
    const still_valid = cached_response_still_valid(cached_response);

    if (still_valid) {
      trace_response(cached_response);
      return cached_response;
    } else {
      cache.delete(req);
    }
  }

  const response = await fetch(req);

  trace_response(response);

  if (should_cache(response)) {
    cache.put(req, response).catch(err => console.log('Caching failed'))
  }

  return response;
}

With the caveat that Cloudflare respects cache headers whereas the browser and Deno do not (up to you to invalidate cache entries).

If you wanted to use a fetch-like interface that explicitly takes only a Request object and returns a Response the code is nearly identical except you can just remove the line that normalizes to Request for the cache key:

type FetchLike = (req: Request) => Promise<Response>;

const custom_fetch: FetchLike = async (req) => {
  trace_input(req);

  const cached_response = await cache.match(req);

  if (cached_response) {
    const still_valid = cached_response_still_valid(cached_response);

    if (still_valid) {
      trace_response(cached_response);
      return cached_response;
    } else {
      cache.delete(req);
    }
  }

  const response = await fetch(req);

  trace_response(response);

  if (should_cache(response)) {
    cache.put(req, response).catch(err => console.log('Caching failed'))
  }

  return response;
}

This is how I implemented the patch in my fork if it's helpful

https://github.com/panva/oauth4webapi/compare/main...melbourne2991:oauth4webapi-anyfetch:main?diff=split&w=#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80L1965

[panva]

I'm looking for feedback for this request in #94