Skip to content

Add middleware to schedule sign-out #2904

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Jan 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/peregrine/lib/Apollo/attachClientToStore.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { extraArgument } from '../store/middleware/thunk';

const attachClientToStore = apolloClient => {
Object.assign(extraArgument, { apolloClient });
};

export default attachClientToStore;
10 changes: 3 additions & 7 deletions packages/peregrine/lib/context/cart.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, { createContext, useContext, useEffect, useMemo } from 'react';
import { connect } from 'react-redux';
import { useApolloClient, useMutation } from '@apollo/client';
import { useMutation } from '@apollo/client';
import gql from 'graphql-tag';

import { useAwaitQuery } from '@magento/peregrine/lib/hooks/useAwaitQuery';
Expand Down Expand Up @@ -55,20 +55,16 @@ const CartContextProvider = props => {
derivedCartState
]);

const apolloClient = useApolloClient();
const [fetchCartId] = useMutation(CREATE_CART_MUTATION);
const fetchCartDetails = useAwaitQuery(CART_DETAILS_QUERY);

useEffect(() => {
// cartApi.getCartDetails initializes the cart if there isn't one. Also, we pass
// apolloClient to wipe the store in event of auth token expiry which
// will only happen if the user refreshes.
// cartApi.getCartDetails initializes the cart if there isn't one.
cartApi.getCartDetails({
apolloClient,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

fetchCartId,
fetchCartDetails
});
}, [apolloClient, cartApi, fetchCartDetails, fetchCartId]);
}, [cartApi, fetchCartDetails, fetchCartId]);

return (
<CartContext.Provider value={contextValue}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,13 @@ const getState = jest.fn(() => ({
cart: { cartId: 'CART_ID' },
user: { isSignedIn: false }
}));
const thunkArgs = [dispatch, getState];
const thunkArgs = [
dispatch,
getState,
{
apolloClient: {}
}
];

describe('createCart', () => {
test('it returns a thunk', () => {
Expand Down
13 changes: 5 additions & 8 deletions packages/peregrine/lib/store/actions/cart/asyncActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -317,9 +317,9 @@ export const removeItemFromCart = payload => {
};

export const getCartDetails = payload => {
const { apolloClient, fetchCartId, fetchCartDetails } = payload;
const { fetchCartId, fetchCartDetails } = payload;

return async function thunk(dispatch, getState) {
return async function thunk(dispatch, getState, { apolloClient }) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

const { cart, user } = getState();
const { cartId } = cart;
const { isSignedIn } = user;
Expand Down Expand Up @@ -367,13 +367,10 @@ export const getCartDetails = payload => {
await dispatch(removeCart());
}

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

Choose a reason for hiding this comment

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

Why did we remove this?

}
// Clear cart data from Apollo cache
await clearCartDataFromCache(apolloClient);

// Create a new one
// Create a new cart
try {
await dispatch(
createCart({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,14 @@ const dispatch = jest.fn();
const getState = jest.fn(() => ({
user: { isSignedIn: false }
}));
const thunkArgs = [dispatch, getState];
const thunkArgs = [
dispatch,
getState,
{
apolloClient: {}
}
];

const fetchUserDetails = jest
.fn()
.mockResolvedValue({ data: { customer: {} } });
Expand Down Expand Up @@ -102,7 +109,7 @@ describe('signOut', () => {
});

test('signOut thunk invokes revokeToken and dispatchs actions', async () => {
await signOut({ revokeToken })(dispatch);
await signOut({ revokeToken })(...thunkArgs);

expect(revokeToken).toHaveBeenCalledTimes(1);
expect(dispatch).toHaveBeenCalledTimes(3);
Expand All @@ -112,7 +119,7 @@ describe('signOut', () => {
const consoleSpy = jest.spyOn(console, 'error');
revokeToken.mockRejectedValueOnce(new Error('Revoke Token Error'));

await signOut({ revokeToken })(dispatch);
await signOut({ revokeToken })(...thunkArgs);

expect(revokeToken).toHaveBeenCalledTimes(1);
expect(consoleSpy).toHaveBeenCalledTimes(1);
Expand Down
6 changes: 5 additions & 1 deletion packages/peregrine/lib/store/actions/user/asyncActions.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import BrowserPersistence from '../../../util/simplePersistence';
import { clearCartDataFromCache } from '../../../Apollo/clearCartDataFromCache';
import { clearCustomerDataFromCache } from '../../../Apollo/clearCustomerDataFromCache';
import { removeCart } from '../cart';
import { clearCheckoutDataFromStorage } from '../checkout';

Expand All @@ -7,7 +9,7 @@ import actions from './actions';
const storage = new BrowserPersistence();

export const signOut = (payload = {}) =>
async function thunk(dispatch) {
async function thunk(dispatch, getState, { apolloClient }) {
const { revokeToken } = payload;

if (revokeToken) {
Expand All @@ -23,6 +25,8 @@ export const signOut = (payload = {}) =>
await dispatch(clearToken());
await dispatch(actions.reset());
await clearCheckoutDataFromStorage();
await clearCartDataFromCache(apolloClient);
await clearCustomerDataFromCache(apolloClient);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, we clear the rest of the cache data.

Copy link
Contributor

@sirugh sirugh Jan 7, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


// Now that we're signed out, forget the old (customer) cart.
// We don't need to create a new cart here because we're going to refresh
Expand Down
5 changes: 3 additions & 2 deletions packages/peregrine/lib/store/enhancers/middleware.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { applyMiddleware } from 'redux';
import thunk from 'redux-thunk';

import auth from '../middleware/auth';
import log from '../middleware/log';
import thunk from '../middleware/thunk';

const middleware = [thunk];
const middleware = [thunk, auth];

if (process.env.NODE_ENV !== 'production') {
middleware.push(log);
Expand Down
82 changes: 82 additions & 0 deletions packages/peregrine/lib/store/middleware/auth.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import BrowserPersistence from '../../util/simplePersistence';
import userActions, { signOut } from '../actions/user';

const timeouts = new Map();
const intervals = new Map();
const storage = new BrowserPersistence();
const SET_TOKEN = userActions.setToken.toString();
const CLEAR_TOKEN = userActions.clearToken.toString();
const GET_DETAILS = userActions.getDetails.request.toString();

const isSigningIn = type => type === SET_TOKEN || type === GET_DETAILS;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

const isSigningOut = type => type === CLEAR_TOKEN;

/**
* This function adheres to Redux's middleware pattern.
*
* @param {Store} store The store to augment.
* @returns {Function}
*/
const scheduleSignOut = store => next => action => {
const { dispatch } = store;

if (isSigningIn(action.type)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jimbo jimbo Dec 14, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// `BrowserPersistence.getItem()` only returns the value
// but we need the full item with timestamp and ttl
const item = storage.getRawItem('signin_token');

// exit if there's nothing in storage
if (!item) return next(action);

const { timeStored, ttl, value } = JSON.parse(item);
const parsedValue = JSON.parse(value);
const preciseTTL = ttl * 1000;
const elapsed = Date.now() - timeStored;
const expiry = Math.max(preciseTTL - elapsed, 0);

// establish a sign-out routine
const callback = () => {
dispatch(signOut()).then(() => {
timeouts.delete(parsedValue);
intervals.delete(parsedValue);

// refresh the page, important for checkout
history.go(0);
});
};

// set a timeout that runs once when the token expires
if (!timeouts.has(parsedValue)) {
const timeoutId = setTimeout(callback, expiry);

timeouts.set(parsedValue, timeoutId);
}

// then set an interval that runs once per second
// on mobile, the timeout won't fire if the tab is inactive
if (!intervals.has(parsedValue)) {
const intervalId = setInterval(() => {
const hasExpired = Date.now() - timeStored > preciseTTL;

if (hasExpired) callback();
}, 1000);

intervals.set(parsedValue, intervalId);
}
} else if (isSigningOut(action.type)) {
for (const timeoutId of timeouts) {
clearTimeout(timeoutId);
}

for (const intervalId of intervals) {
clearInterval(intervalId);
}

timeouts.clear();
intervals.clear();
}

return next(action);
};

export default scheduleSignOut;
4 changes: 4 additions & 0 deletions packages/peregrine/lib/store/middleware/thunk.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import thunk from 'redux-thunk';

export const extraArgument = {};
export default thunk.withExtraArgument(extraArgument);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

9 changes: 2 additions & 7 deletions packages/peregrine/lib/talons/AuthModal/useAuthModal.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
import { useCallback, useEffect, useState } from 'react';
import { useHistory } from 'react-router-dom';
import { useApolloClient, useMutation } from '@apollo/client';
import { useMutation } from '@apollo/client';

import mergeOperations from '../../util/shallowMerge';
import { clearCartDataFromCache } from '../../Apollo/clearCartDataFromCache';
import { clearCustomerDataFromCache } from '../../Apollo/clearCustomerDataFromCache';
import { useUserContext } from '../../context/user';
import DEFAULT_OPERATIONS from './authModal.gql';

Expand Down Expand Up @@ -48,7 +46,6 @@ export const useAuthModal = props => {
const operations = mergeOperations(DEFAULT_OPERATIONS, props.operations);
const { signOutMutation } = operations;

const apolloClient = useApolloClient();
const [isSigningOut, setIsSigningOut] = useState(false);
const [username, setUsername] = useState('');
const [{ currentUser, isSignedIn }, { signOut }] = useUserContext();
Expand Down Expand Up @@ -89,14 +86,12 @@ export const useAuthModal = props => {

// Delete cart/user data from the redux store.
await signOut({ revokeToken });
await clearCartDataFromCache(apolloClient);
await clearCustomerDataFromCache(apolloClient);

// Refresh the page as a way to say "re-initialize". An alternative
// would be to call apolloClient.resetStore() but that would require
// a large refactor.
history.go(0);
}, [apolloClient, history, revokeToken, signOut]);
}, [history, revokeToken, signOut]);

return {
handleCancel,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,6 @@ jest.mock('@magento/peregrine/lib/hooks/useDropdown', () => ({
})
}));

jest.mock('@magento/peregrine/lib/Apollo/clearCartDataFromCache', () => ({
clearCartDataFromCache: jest.fn().mockResolvedValue()
}));

jest.mock('@magento/peregrine/lib/Apollo/clearCustomerDataFromCache', () => ({
clearCustomerDataFromCache: jest.fn().mockResolvedValue()
}));

const defaultProps = {
accountMenuIsOpen: false,
setAccountMenuIsOpen: jest.fn()
Expand Down
9 changes: 2 additions & 7 deletions packages/peregrine/lib/talons/Header/useAccountMenu.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import { useState, useCallback, useEffect } from 'react';
import { useHistory, useLocation } from 'react-router-dom';
import { useApolloClient, useMutation } from '@apollo/client';
import { useMutation } from '@apollo/client';

import { clearCartDataFromCache } from '../../Apollo/clearCartDataFromCache';
import { clearCustomerDataFromCache } from '../../Apollo/clearCustomerDataFromCache';
import mergeOperations from '../../util/shallowMerge';
import { useUserContext } from '../../context/user';

Expand Down Expand Up @@ -36,7 +34,6 @@ export const useAccountMenu = props => {
const [view, setView] = useState('SIGNIN');
const [username, setUsername] = useState('');

const apolloClient = useApolloClient();
const history = useHistory();
const location = useLocation();
const [revokeToken] = useMutation(signOutMutation);
Expand All @@ -48,14 +45,12 @@ export const useAccountMenu = props => {

// Delete cart/user data from the redux store.
await signOut({ revokeToken });
await clearCartDataFromCache(apolloClient);
await clearCustomerDataFromCache(apolloClient);

// Refresh the page as a way to say "re-initialize". An alternative
// would be to call apolloClient.resetStore() but that would require
// a large refactor.
history.go(0);
}, [apolloClient, history, revokeToken, setAccountMenuIsOpen, signOut]);
}, [history, revokeToken, setAccountMenuIsOpen, signOut]);

const handleForgotPassword = useCallback(() => {
setView('FORGOT_PASSWORD');
Expand Down
3 changes: 3 additions & 0 deletions packages/peregrine/lib/util/simplePersistence.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ export default class BrowserPersistence {
this.constructor.KEY || BrowserPersistence.KEY
);
}
getRawItem(name) {
return this.storage.getItem(name);
}
getItem(name) {
const now = Date.now();
const item = this.storage.getItem(name);
Expand Down
Loading