Fixes infinite redirect issue on some errors in the OAuth flow #1089
Conversation
| @@ -103,6 +99,31 @@ export function readLocal(client: Client): LocalStorageState { | |||
| ); | |||
| } | |||
|
|
|||
| export function saveSession(client: Client, x: SessionStorageState) { | |||
There was a problem hiding this comment.
I left the above localStorage helpers just to ensure we still localStorage? due to the node.js restriction
| export type LocalStorageState = | ||
| export type LocalStorageState = { refresh_token?: string }; | ||
|
|
||
| export type SessionStorageState = |
There was a problem hiding this comment.
Here's my explanation on why this migration of all three of the below variables to session storage will not impact the current behavior:
If the refreshToken is present in localStorage, the contents of the session storage are largely disregarded. If the refresh flow fails or the refreshToken is not present, then we go through the entire auth flow or we handle the auth redirect, in which case the state, oldUrl, and codeVerifier are session specific and were generated by the loginRedirect flow. Since the refreshToken being present is disjointed from the use of the other variables, we can introduce the split here
| @@ -269,14 +274,15 @@ export function createPublicOauthClient( | |||
| && window.location.href !== loginPage | |||
| && window.location.pathname !== loginPage | |||
| ) { | |||
| saveLocal(client, { oldUrl: postLoginPage }); | |||
| saveSession(client, { oldUrl: postLoginPage }); | |||
There was a problem hiding this comment.
This code previously ensured that we were no longer storing refreshToken as it overwrote state but this line no longer does that. Is that intentional or should we be saving an empty object here
There was a problem hiding this comment.
Good catch. I don't think this has any behavioral effects as initiateLoginRedirect seems to only be called when there is no refresh token in local storage here. However, we should ensure we are always reflecting what the state should be and similarly follow what we do immediately below, so I'll add the fix here.
policy-bot
bot
dismissed
ericanderson’s stale review
Invalidated by push of 9acd1b0
…ated-permissions-cause-403-errors-when-user-loads-new-version-of-app-provide-way-to-get-new-token-with-new-permissions-on-load * origin/main: Fixes infinite redirect issue on some errors in the OAuth flow (#1089) Tighten return types (#924) Add typed lint checks and fix promise related things (#1081) Version Packages (beta) (#1090) Export hydrateObjectSetFromRid from internal (#1087) Fetch by RID experimental (#1088) Creates new internal export for helper methods we want to hide (#1084) Add's more descriptive error messages for 429 errors and other errors with no body (#1082) Update react template per discussion (#1077) OSDK client internal property is hidden from output .d.ts (#1079) Revert `api:x-read/write` back to `api:read/write-x` (#1076) Move from "views" naming to "widgets" (#1075)
Fixes #953. Implements the approach used in #389 for legacy oauth which moves the codeVerifier to session storage to avoid a bad state.
The error is caught on the authRedirect page and can be displayed to the user. The caveat here is that the "back" button is not going to redirect you to where the sign in began as the state is replaced. The expected flow here as discussed in the previous PR/offline discussions is that the user incorporates their own handling when something goes wrong here.