AxaFrance / oidc-client

Light, Secure, Pure Javascript OIDC (Open ID Connect) Client. We provide also a REACT wrapper (compatible NextJS, etc.).
MIT License
582 stars 160 forks source link

Setup example with existing service worker? #914

Open yann-combarnous opened 1 year ago

yann-combarnous commented 1 year ago

Issue and Steps to Reproduce

We already use in our project a service worker, similar to https://create-react-app.dev/docs/making-a-progressive-web-app/.

As your example for react uses create-react-app, would it be possible to get an example on how to use this library with the default service worker provided by CRA?

Versions

Screenshots

Expected

Actual

Additional Details

guillaume-chervet commented 1 year ago

thank you @yann-combarnous for the issue. yes definitlety we need this :)

darkowic commented 1 year ago

We just realized we are facing the same issue. We will most likely work on figuring out something.

kosciolek commented 1 year ago

1. No service worker

Looks like you can do service_worker_relative_url: '', and then the client will work without a SW, so you may use your own. This has security drawbacks however.

2. With a service worker (the current hacky way)

Since it's not possible to have multiple service workers on the same origin, we've got to compose them with importScripts:

// main-service-worker.js
importScripts('first-service-worker.js');
importScripts('second-service-worker.js');
importScripts('OidcServiceWorker.js');

Let's register it:

navigator.serviceWorker.register('main-service-worker.js');

However, there's a problem: axa's oidc will call navigator.serviceWorker.register later anyway, overriding our service worker.

https://github.com/AxaFrance/react-oidc/blob/9cc11ba9c9daa1a5fc1943d4a00081b887704df8/packages/react/src/oidc/vanilla/initWorker.ts#L179

Set configuration.service_worker_relative_url to main-service-worker.js, and:

  1. Do not call navigator.serviceWorker.register ourselves, but wait till axa does it.
  2. Do call navigator.serviceWorker.register ourselves (if we need to register earlier), as loading the same service worker a second time is a noop, so axa's registration will do nothing. There's also the fact that this way the oidc service worker is now loaded earlier than intended (the moment you call navigator.serviceWorker.register, rather than when axa's client calls it), but it seems to work just fine.

However, this is feels hacky and I assume it's not the intended solution, and something somewhere might break. Please correct me if this actually is the intended solution.

3. With a service worker (the proper solution)

I think the simplest way is to delegate SW registration to the user with something like a service_worker_no_auto_register param. I do not know if we'll need it yet, but if so, I'll be willing to implement it myself.

@guillaume-chervet What are your thoughts? Would you please tell me if you'd accept such a PR? Do you think this would be a complex change? As far as I see, it already works if you register the SW yourself earlier than axa does it, but maybe you can foresee some obstacles here.

A quick PR, just to facilitate discussion: #1081

Rough to-do

Some things to be taken care of when delegating the service worker to the user:

  1. Don't register the SW, let the user do it, and skip all logic related to registration/unregistration in the oidc worker https://github.com/AxaFrance/react-oidc/blob/1d96c895b19d8f42486b44fa3cffc620b1e7ee28/packages/react/src/oidc/vanilla/initWorker.ts#L179
  2. Axa's keep-alive mechanism (since the browser often deactivates idle service workers after some time) perhaps should be moved to the user. https://github.com/AxaFrance/react-oidc/blob/1d96c895b19d8f42486b44fa3cffc620b1e7ee28/packages/react/src/oidc/vanilla/initWorker.ts#L110
  3. Worker updates should most likely be handled by the user, and this should be removed https://github.com/AxaFrance/react-oidc/blob/1d96c895b19d8f42486b44fa3cffc620b1e7ee28/packages/react/src/oidc/vanilla/initWorker.ts#L191
  4. The oidc service worker receives objects of type { type: string, ...otherStuff }. It's a common pattern, and all event types most likely should be prefixed to avoid collisions: https://github.com/AxaFrance/react-oidc/blob/1d96c895b19d8f42486b44fa3cffc620b1e7ee28/packages/react/service_worker/types.ts#L35
  5. Check if Axa's fetch interception won't collide with some other service worker that's used to, for example, cache assets.