Skip to content
This repository has been archived by the owner on Oct 11, 2022. It is now read-only.

Fix broken authentication #3392

Merged
merged 2 commits into from
Jun 26, 2018
Merged

Fix broken authentication #3392

merged 2 commits into from
Jun 26, 2018

Conversation

mxstbr
Copy link
Contributor

@mxstbr mxstbr commented Jun 26, 2018

Status

  • WIP
  • Ready for review
  • Needs testing

Deploy after merge (delete what needn't be deployed)

  • hyperion (frontend)

The ServiceWorker would automatically return our app shell for all navigation requests, including to /auth/xyz—which meant that users were served the app instead of getting redirected to their auth provider of choice.

This fixes it by disabling the built-in appShell option and then copying exactly what it does BUT filtering out the /auth route first. (and the /api route for good measure)

/cc @NekR it would be nice if there was a built-in option for this behavior (excluding certain routes from the app shell) or if the default appShell cache map would be added as the first cache map , not as the last cache map, so it can be overridden.
https://github.com/NekR/offline-plugin/blob/ddec2aa93a7ea1cf8f0338518f96cf9f521607c9/src/index.js#L130-L140 It says here that it's added before the custom ones, but it's actually added after them as far as I can tell

The ServiceWorker would automatically return our app shell for all
navigation requests, including to `/auth/xyz`—which meant that users
were served the app instead of getting redirected to their auth provider
of choice.

This fixes it by disabling the built-in `appShell` option, copying
exactly what it does but filtering out the `/auth` route. (and the
`/api` route for good measure)
@spectrum-bot
Copy link

spectrum-bot bot commented Jun 26, 2018

Warnings
⚠️

These modified files do not have Flow enabled:

  • config-overrides.js

Generated by 🚫 dangerJS

@mxstbr
Copy link
Contributor Author

mxstbr commented Jun 26, 2018

This was tested locally on the production setup and works beautifully. Will get a deploy on alpha too.

@mxstbr
Copy link
Contributor Author

mxstbr commented Jun 26, 2018

Found a small bug, fixed in 1693267. This is live on alpha and works perfectly! Everything is served from the ServiceWorker, and authentication works perfectly fine 👌

caches: process.env.NODE_ENV === 'development' ? {} : 'all',
externals,
autoUpdate: true,
// NOTE(@mxstbr): Normally this is handled by setting
Copy link
Contributor

Choose a reason for hiding this comment

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

helpful, describable comments. nice work😀

@brianlovin brianlovin merged commit fffd967 into alpha Jun 26, 2018
@brianlovin brianlovin deleted the fix-sw-for-real-for-real branch June 26, 2018 17:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants