Add middleware to schedule sign-out #2904

jimbo · 2020-12-08T22:19:44Z

Description

When a user signs in, we get a token that we store in both app state (Redux store's user slice) and local storage (signin_token, namespaced). This token has a limited lifetime before it becomes invalid (currently hardcoded to one hour), and an invalid token is removed from storage the first time it is accessed after expiration.

Unfortunately, the storage interface can't notify app state when the token expires or when it removes an expired token, since it doesn't have access to the dispatch function. This leads app state and storage to fall out of sync, such that components start to throw errors when app state tells them the user is signed in but the token has actually been invalidated.

This PR adds a middleware that schedules the app state's signOut workflow to run whenever a token is set (sign-in or re-auth). Just as the token expires, the app will sign the user out, clear the token and all user state, and refresh the page. In the future, we can remove the page refresh, but that will require some amount of component rework and regression.

Related Issue

PWA-1105

Acceptance

Verification Stakeholders

Specification

Verification Steps

Sign in to a user account.
Open developer tools and edit the signin_token in storage to have a ttl of 30000 (30 seconds).
Refresh your browser tab (to simulate having waited approximately one hour).
Wait 30 seconds.
Verify that you are automatically signed out and the page automatically refreshes.
Verify that there are no JS errors in the console.
Repeat this process on the checkout page.

Screenshots / Screen Captures (if appropriate)

Checklist

I have added tests to cover my changes, if necessary.
I have added translations for new strings, if necessary.
I have updated the documentation accordingly, if necessary.

PWAStudioBot · 2020-12-08T22:29:45Z

	Fails
🚫	A version label is required. A maintainer must add one.

	Messages
📖	Associated JIRA tickets: PWA-1105.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against 2ab4ad0

sirugh · 2020-12-14T17:11:07Z

packages/peregrine/lib/store/middleware/auth.js

+const CLEAR_TOKEN = userActions.clearToken.toString();
+const GET_DETAILS = userActions.getDetails.request.toString();
+
+const isSigningIn = type => type === SET_TOKEN || type === GET_DETAILS;


Why || GET_DETAILS? Is this a safeguard against tokens invalidating between the token set and the details fetch?

When a user starts up the application, if they have a valid token in storage, we sign them in. Since they already have a token, a SET_TOKEN action is never dispatched. Instead, the first user action is GET_DETAILS.

So it's really "is signing in or is getting user details". And in both cases we set a timer based on the time remaining on the TTL. Got it.

sirugh · 2020-12-14T17:20:51Z

packages/peregrine/lib/store/middleware/auth.js

+const scheduleSignOut = store => next => action => {
+    const { dispatch } = store;
+
+    if (isSigningIn(action.type)) {


So when we sign in, we create a timer to remove the token based on the TTL of the token just set? So this is similar to what getItem does but is also able to dispatch the signOut action and clear the token from redux (persisted as M2_VENIA_BROWSER_PERSISTENCE__signin_token).

The current iteration of the auth link also uses the getItem mechanism which should delete the token if expired.

Both seem to handle similar situations related to client-side invalidation of the token, but I still don't know if we gracefully handle a user token that invalidates server-side before the client-side expiry.

Correct, getItem can't dispatch signOut, so the Redux store contains stale state (isSignedIn: true) when there's an expired token or no token in storage.

This PR doesn't handle a server-side invalidation, but I believe those are going to be much less frequent. Scheduled expiration should be the 99% case.

This PR doesn't handle a server-side invalidation, but I believe those are going to be much less frequent. Scheduled expiration should be the 99% case.

Right, Tommy's solution gets us there, assuming the error messages are correct. That said, I don't know enough about auth invalidation so I'll trust that you when you say that a timeout expiry is the most common scenario.

sirugh

We need to solve for the cache clearing just like we do in the other sign out handlers. :(

jimbo · 2021-01-06T23:01:08Z

We need to solve for the cache clearing just like we do in the other sign out handlers. :(

The signOut async action now calls the other cache clearing functions. 👍

jimbo · 2021-01-06T23:03:56Z

packages/peregrine/lib/context/cart.js

        cartApi.getCartDetails({
-            apolloClient,


Passing things that can't be serialized, such as functions or objects containing functions, in action payloads is an anti-pattern in Redux. I understand the thought behind putting apolloClient in the payload, but we shouldn't have been doing this.

jimbo · 2021-01-06T23:04:57Z

packages/peregrine/lib/store/actions/cart/asyncActions.js


-    return async function thunk(dispatch, getState) {
+    return async function thunk(dispatch, getState, { apolloClient }) {


Now that our thunk middleware includes an extraArgument object with the Apollo client, we can access it from any thunk.

jimbo · 2021-01-06T23:05:29Z

packages/peregrine/lib/store/actions/user/asyncActions.js

@@ -23,6 +25,8 @@ export const signOut = (payload = {}) =>
        await dispatch(clearToken());
        await dispatch(actions.reset());
        await clearCheckoutDataFromStorage();
+        await clearCartDataFromCache(apolloClient);
+        await clearCustomerDataFromCache(apolloClient);


Here, we clear the rest of the cache data.

Beautiful - plan on applying this to the other actions like cart?

Also, now that signOut handles this, can we remove it from the sign out handlers like here?

It looks like maybe we have to add it to the signIn async action as well in order to remove it from other handlers.

jimbo · 2021-01-06T23:07:33Z

packages/peregrine/lib/store/middleware/thunk.js

+import thunk from 'redux-thunk';
+
+export const extraArgument = {};
+export default thunk.withExtraArgument(extraArgument);


Fortunately for us, redux-thunk has already considered our exact situation and provides a method that makes it easy to provide our API client to every thunk.

jimbo · 2021-01-06T23:14:16Z

packages/venia-ui/lib/drivers/adapter.js

-        apolloClient = apollo.client;
-    } else {
-        apolloClient = new ApolloClient({
+    const apolloClient = useMemo(() => {


This should always have been memoized. Performing side effects in the body of a render function (component) has always been an anti-pattern; here, the original code was creating multiple Apollo clients as Adapter is executed several times during initial rendering.

An easy mistake to make, but a very important one to avoid.

jimbo · 2021-01-06T23:16:45Z

packages/venia-ui/lib/drivers/adapter.js

+            await apolloClient.persistor.restore();
+
+            // attach the Apollo client to the Redux store
+            attachClient(apolloClient);


This is the only real change to this file. Ideally, we would create the Apollo client at the same time we create the Redux store and provide it directly to the middleware, but since we've delegated client creation to Adapter, we have to wait and add it here.

sirugh · 2021-01-07T21:48:57Z

packages/peregrine/lib/store/actions/cart/asyncActions.js

-                // Clear the cart data from apollo client if we get here and
-                // have an apolloClient.
-                if (apolloClient) {
-                    await clearCartDataFromCache(apolloClient);


Why did we remove this?

sirugh

Excellent work on this solution. And nice find on the adapter bug.

I do have concerns about alerting the user that they are being signed out, but I believe we are going to handle that detail in another ticket. The main point of this effort was to not leave the user in a stuck state where their token was invalid but not cleared.

dpatil-magento · 2021-01-14T17:03:39Z

@jimbo Pls check 2 observations -

Even if user is actively working is session, they would be automatically logged out after 60mins? (I tested it by changing ttl to 100 secs).
On Mobile I kept session open for 60mins and dont see automatic sign-out but on refresh it did signout. And on trying to click Proceed to Checkout after session has expired > User get error Toast and after 2-3 secs user gets auto signed-out.
Note - Desktop chrome browser/simulator is working fine.

dpatil-magento · 2021-01-15T02:56:44Z

@jimbo Looks like issue is happening on desktop browser as well, if you leave idle for an hour then auto sign-out does not happen unless new call is made.

dpatil-magento · 2021-01-22T05:33:11Z

QA Approved.

Add middleware to schedule sign-out

42058c5

m2-community-project bot added the Progress: ready for review label Dec 8, 2020

PWAStudioBot added the pkg:peregrine label Dec 8, 2020

Augment persistence API

33f0b90

sirugh self-requested a review December 14, 2020 16:55

sirugh reviewed Dec 14, 2020

View reviewed changes

m2-community-project bot assigned sirugh Dec 14, 2020

m2-community-project bot added Progress: review and removed Progress: ready for review labels Dec 14, 2020

sirugh requested changes Dec 18, 2020

View reviewed changes

m2-community-project bot added Progress: changes requested and removed Progress: review labels Dec 18, 2020

Provide client to thunk middleware

c7e62f8

PWAStudioBot added the pkg:venia-ui label Jan 6, 2021

Fix tests

7aa638a

jimbo marked this pull request as ready for review January 6, 2021 23:00

Merge branch 'develop' into jimbo/PWA-1105

be96477

jimbo commented Jan 6, 2021

View reviewed changes

m2-community-project bot added Progress: review and removed Progress: changes requested labels Jan 6, 2021

Cover all sign-out cases

c1bda3d

sirugh reviewed Jan 7, 2021

View reviewed changes

Revert cart change

423011e

sirugh previously approved these changes Jan 8, 2021

View reviewed changes

m2-community-project bot added Progress: approved and removed Progress: review labels Jan 8, 2021

Merge branch 'develop' into jimbo/PWA-1105

84a6a8a

Add interval for mobile

ddd26b9

jimbo dismissed sirugh’s stale review via ddd26b9 January 15, 2021 23:09

Merge branch 'develop' into jimbo/PWA-1105

a31a59f

sirugh approved these changes Jan 21, 2021

View reviewed changes

Merge branch 'develop' into jimbo/PWA-1105

2ab4ad0

dpatil-magento merged commit 7dd0edd into develop Jan 22, 2021

dpatil-magento deleted the jimbo/PWA-1105 branch January 22, 2021 16:51

m2-community-project bot added Progress: done and removed Progress: approved labels Jan 22, 2021

jimbo added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Jan 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add middleware to schedule sign-out #2904

Add middleware to schedule sign-out #2904

jimbo commented Dec 8, 2020 •

edited

Loading

PWAStudioBot commented Dec 8, 2020 •

edited

Loading

sirugh Dec 14, 2020

jimbo Dec 14, 2020

sirugh Dec 15, 2020

sirugh Dec 14, 2020

jimbo Dec 14, 2020 •

edited

Loading

sirugh Dec 15, 2020

sirugh left a comment

jimbo commented Jan 6, 2021

jimbo Jan 6, 2021

jimbo Jan 6, 2021

jimbo Jan 6, 2021

sirugh Jan 7, 2021 •

edited

Loading

jimbo Jan 7, 2021

jimbo Jan 6, 2021

jimbo Jan 6, 2021

jimbo Jan 6, 2021

sirugh Jan 7, 2021

sirugh left a comment

dpatil-magento commented Jan 14, 2021 •

edited

Loading

dpatil-magento commented Jan 15, 2021 •

edited

Loading

dpatil-magento commented Jan 22, 2021


		return async function thunk(dispatch, getState) {
		return async function thunk(dispatch, getState, { apolloClient }) {

Add middleware to schedule sign-out #2904

Add middleware to schedule sign-out #2904

Conversation

jimbo commented Dec 8, 2020 • edited Loading

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimbo Dec 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirugh left a comment

Choose a reason for hiding this comment

jimbo commented Jan 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirugh Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirugh left a comment

Choose a reason for hiding this comment

dpatil-magento commented Jan 14, 2021 • edited Loading

dpatil-magento commented Jan 15, 2021 • edited Loading

dpatil-magento commented Jan 22, 2021

jimbo commented Dec 8, 2020 •

edited

Loading

PWAStudioBot commented Dec 8, 2020 •

edited

Loading

jimbo Dec 14, 2020 •

edited

Loading

sirugh Jan 7, 2021 •

edited

Loading

dpatil-magento commented Jan 14, 2021 •

edited

Loading

dpatil-magento commented Jan 15, 2021 •

edited

Loading