Skip to content
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.

Sign up for GitHub

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

[Hybrid] PWA Kit should have a mechanism for replacing the access token when a SFRA login state is changed #1171

Merged
merged 12 commits into from
May 5, 2023
Merged

[Hybrid] PWA Kit should have a mechanism for replacing the access token when a SFRA login state is changed #1171

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9097789
Implement mechanism to store refresh token copy and compare with sfra
shethj May 4, 2023
78dd3f0
Update tests and mocks for util function to check SFRA login state
shethj May 4, 2023
ad0c215
Merge branch 'develop' into feat/sync-login-state-hybrid
shethj May 4, 2023
2eeed45
Fix linting issues
shethj May 4, 2023
b0d82c9
FIx param types for util functionn
shethj May 4, 2023
8f8b0c5
Rename old isTokenValid to isTokenExpired
shethj May 4, 2023
ffd6ebf
Remove expiry for refresh_token in localstorage
shethj May 4, 2023
f09ba06
Update packages/template-retail-react-app/app/commerce-api/utils.js
shethj May 4, 2023
ef654db
fix test
kevinxh May 4, 2023
b1157d9
Fix linting on use-auth-modal.test.js
shethj May 4, 2023
5f6df2d
Update hasSFRAStateChanged logic to compare keys and values
shethj May 5, 2023
04733e8
Fix linting
shethj May 5, 2023
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
48 changes: 33 additions & 15 deletions packages/template-retail-react-app/app/commerce-api/auth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,20 @@
import {getAppOrigin} from 'pwa-kit-react-sdk/utils/url'
import {HTTPError} from 'pwa-kit-react-sdk/ssr/universal/errors'
import {createCodeVerifier, generateCodeChallenge} from './pkce'
import {isTokenValid, createGetTokenBody} from './utils'
import {isTokenExpired, createGetTokenBody, hasSFRAAuthStateChanged} from './utils'
import {
usidStorageKey,
cidStorageKey,
encUserIdStorageKey,
tokenStorageKey,
refreshTokenRegisteredStorageKey,
refreshTokenGuestStorageKey,
oidStorageKey,
dwSessionIdKey,
REFRESH_TOKEN_COOKIE_AGE,
EXPIRED_TOKEN,
INVALID_TOKEN
} from './constants'
import fetch from 'cross-fetch'
import Cookies from 'js-cookie'

Expand All @@ -26,19 +39,6 @@ import Cookies from 'js-cookie'
* @typedef {Object} Customer
*/

const usidStorageKey = 'usid'
const cidStorageKey = 'cid'
const encUserIdStorageKey = 'enc-user-id'
const tokenStorageKey = 'token'
const refreshTokenRegisteredStorageKey = 'cc-nx'
const refreshTokenGuestStorageKey = 'cc-nx-g'
const oidStorageKey = 'oid'
const dwSessionIdKey = 'dwsid'
const REFRESH_TOKEN_COOKIE_AGE = 90 // 90 days. This value matches SLAS cartridge.

const EXPIRED_TOKEN = 'EXPIRED_TOKEN'
const INVALID_TOKEN = 'invalid refresh_token'

/**
* A class that provides auth functionality for the retail react app.
*/
Expand All @@ -48,6 +48,7 @@ class Auth {
this._api = api
this._config = api._config
this._onClient = typeof window !== 'undefined'
this._storageCopy = this._onClient ? new LocalStorage() : new Map()
kevinxh marked this conversation as resolved.
Show resolved Hide resolved

// To store tokens as cookies
// change the next line to
Expand Down Expand Up @@ -139,23 +140,40 @@ class Auth {
this._storage.set(oidStorageKey, oid)
}

get isTokenValid() {
return (
!isTokenExpired(this.authToken) &&
!hasSFRAAuthStateChanged(this._storage, this._storageCopy)
)
}

/**
* Save refresh token in designated storage.
*
* @param {string} token The refresh token.
* @param {USER_TYPE} type Type of the user.
*/
_saveRefreshToken(token, type) {
/**
* For hybrid deployments, We store a copy of the refresh_token
* to update access_token whenever customer auth state changes on SFRA.
*/
if (type === Auth.USER_TYPE.REGISTERED) {
this._storage.set(refreshTokenRegisteredStorageKey, token, {
expires: REFRESH_TOKEN_COOKIE_AGE
})
this._storage.delete(refreshTokenGuestStorageKey)

this._storageCopy.set(refreshTokenRegisteredStorageKey, token)
this._storageCopy.delete(refreshTokenGuestStorageKey)
return
}

this._storage.set(refreshTokenGuestStorageKey, token, {expires: REFRESH_TOKEN_COOKIE_AGE})
this._storage.delete(refreshTokenRegisteredStorageKey)

this._storageCopy.set(refreshTokenGuestStorageKey, token)
this._storageCopy.delete(refreshTokenRegisteredStorageKey)
}

/**
Expand Down Expand Up @@ -230,7 +248,7 @@ class Auth {
let authorizationMethod = '_loginAsGuest'
if (credentials) {
authorizationMethod = '_loginWithCredentials'
} else if (isTokenValid(this.authToken)) {
} else if (this.isTokenValid) {
authorizationMethod = '_reuseCurrentLogin'
} else if (this.refreshToken) {
authorizationMethod = '_refreshAccessToken'
Expand Down
18 changes: 18 additions & 0 deletions packages/template-retail-react-app/app/commerce-api/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* Copyright (c) 2021, salesforce.com, inc.
* All rights reserved.
* SPDX-License-Identifier: BSD-3-Clause
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause
*/

export const usidStorageKey = 'usid'
export const cidStorageKey = 'cid'
export const encUserIdStorageKey = 'enc-user-id'
export const tokenStorageKey = 'token'
export const refreshTokenRegisteredStorageKey = 'cc-nx'
export const refreshTokenGuestStorageKey = 'cc-nx-g'
export const oidStorageKey = 'oid'
export const dwSessionIdKey = 'dwsid'
export const REFRESH_TOKEN_COOKIE_AGE = 90 // 90 days. This value matches SLAS cartridge.
export const EXPIRED_TOKEN = 'EXPIRED_TOKEN'
export const INVALID_TOKEN = 'invalid refresh_token'
4 changes: 2 additions & 2 deletions packages/template-retail-react-app/app/commerce-api/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import * as sdk from 'commerce-sdk-isomorphic'
import {getAppOrigin} from 'pwa-kit-react-sdk/utils/url'
import ShopperBaskets from './shopper-baskets'
import OcapiShopperOrders from './ocapi-shopper-orders'
import {getTenantId, isError, isTokenValid} from './utils'
import {getTenantId, isError} from './utils'
import Auth from './auth'
import EinsteinAPI from './einstein'

Expand Down Expand Up @@ -199,7 +199,7 @@ class CommerceAPI {
// If the token is invalid (missing, past/nearing expiration), we issue
// a login call, which will attempt to refresh the token or get a new
// guest token. Once login is complete, we can proceed.
if (!isTokenValid(this.auth.authToken)) {
if (!this.auth.isTokenValid) {
// NOTE: Login will update `this.auth.authToken` with a fresh token
vcua-mobify marked this conversation as resolved.
Show resolved Hide resolved
await this.auth.login()
}
Expand Down
2 changes: 1 addition & 1 deletion packages/template-retail-react-app/app/commerce-api/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ describe('CommerceAPI', () => {
})
test('Use same customer if token is valid', async () => {
const Utils = require('./utils')
jest.spyOn(Utils, 'isTokenValid').mockReturnValue(true)
jest.spyOn(Utils, 'isTokenExpired').mockReturnValue(false)
const _CommerceAPI = require('./index').default
const api = new _CommerceAPI(apiConfig)

Expand Down
29 changes: 25 additions & 4 deletions packages/template-retail-react-app/app/commerce-api/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import jwtDecode from 'jwt-decode'
import {getAppOrigin} from 'pwa-kit-react-sdk/utils/url'
import {HTTPError} from 'pwa-kit-react-sdk/ssr/universal/errors'
import {refreshTokenGuestStorageKey, refreshTokenRegisteredStorageKey} from './constants'
import fetch from 'cross-fetch'

/**
Expand All @@ -16,18 +17,18 @@ import fetch from 'cross-fetch'
* @param {string} token - The JWT bearer token to be inspected
* @returns {boolean}
*/
export function isTokenValid(token) {
export function isTokenExpired(token) {
if (!token) {
return false
return true
}
const {exp, iat} = jwtDecode(token.replace('Bearer ', ''))
const validTimeSeconds = exp - iat - 60
const tokenAgeSeconds = Date.now() / 1000 - iat
if (validTimeSeconds > tokenAgeSeconds) {
return true
return false
}

return false
return true
}

// Returns fomrulated body for SopperLogin getToken endpoint
Expand Down Expand Up @@ -271,3 +272,23 @@ export const convertSnakeCaseToSentenceCase = (text) => {
* Usually used as default for event handlers.
*/
export const noop = () => {}

/**
* WARNING: This function is relevant to be used in Hybrid deployments only.
* Compares the refresh_token keys for guest('cc-nx-g') and registered('cc-nx') login from the cookie received from SFRA with the copy stored in localstorage on PWA Kit
* to determine if the login state of the shopper on SFRA site has changed.
* @param {Storage} storage Cookie storage on PWA Kit in hybrid deployment.
* @param {LocalStorage} storageCopy Local storage holding the copy of the refresh_token in hybrid deployment.
* @returns {boolean} true if the keys do not match (login state changed), false otherwise.
*/
export function hasSFRAAuthStateChanged(storage, storageCopy) {
let refreshTokenKey =
(storage.get(refreshTokenGuestStorageKey) && refreshTokenGuestStorageKey) ||
(storage.get(refreshTokenRegisteredStorageKey) && refreshTokenRegisteredStorageKey)

let refreshTokenCopyKey =
(storageCopy.get(refreshTokenGuestStorageKey) && refreshTokenGuestStorageKey) ||
(storageCopy.get(refreshTokenRegisteredStorageKey) && refreshTokenRegisteredStorageKey)

return refreshTokenKey !== refreshTokenCopyKey
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happens if user stays on PWA kit and keeps refreshing the page? (no SFRA)

It seems to me hasSFRAAuthStateChanged always evaluate to true and PWA will always use refresh token, and never re-use access token?

Copy link
Collaborator

Choose a reason for hiding this comment

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

if cookie doesn't exist, this function should return true

Copy link
Collaborator

Choose a reason for hiding this comment

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

also, let's add a test case for when refresh cookies does not exist.

Copy link
Collaborator Author

@shethj shethj May 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

what happens if user stays on PWA kit and keeps refreshing the page? (no SFRA)

I'm guessing by no SFRA you mean in case of a PWA Kit only setup:
In this case, the storage and storageCopy would both be pointing to LocalStorage and refreshTokenKey === refreshTokenKeyCopy so hasSFRAAuthStateChanged will be false

If by no SFRA you mean Hybrid Setup but the user hasn't yet navigated to SFRA,
Then storage will point to CookieStorage and storageCopy will point to LocalStorage in which case PWA Kit itself will write the refresh_token in both places. And if the user keeps refreshing on PWA kit without navigating to SFRA again refreshTokenKey === refreshTokenKeyCopy so hasSFRAAuthStateChanged will be false

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to be a stickler but why are we only comparing the keys and not the values of the refresh token?

By comparing just the keys we open ourselves to the following edge case in hybrid:

  1. User signs in as a guest via SFRA
  2. User goes to PWA - this sets an access token PWA side
  3. User adds something to their cart via PWA
  4. User goes back to SFRA, logs in (gets a cc-nx) then logs out (goes back to cc-nx-g)
  5. User goes back to PWA

In the above, SFRA has changed the login state a couple of times but settled on a cc-nx-g when transitioning to PWA. If we compare only the keys, in this case we are not discarding the old access token.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated hasSFRALoginStateChanged(...) to compare values if refershToken keys are same.

Thanks @vcua-mobify for catching that 🎉

}
42 changes: 32 additions & 10 deletions packages/template-retail-react-app/app/commerce-api/utils.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@
import jwt from 'njwt'
import {
camelCaseKeysToUnderscore,
isTokenValid,
isTokenExpired,
keysToCamel,
convertSnakeCaseToSentenceCase,
handleAsyncError
handleAsyncError,
hasSFRAAuthStateChanged
} from './utils'

const createJwt = (secondsToExp) => {
Expand All @@ -26,20 +27,20 @@ jest.mock('./utils', () => {
}
})

describe('isTokenValid', () => {
test('returns false when no token given', () => {
expect(isTokenValid()).toBe(false)
describe('isTokenExpired', () => {
test('returns true when no token given', () => {
expect(isTokenExpired()).toBe(true)
})

test('returns true for valid token', () => {
test('returns false for valid token', () => {
const token = createJwt(600)
const bearerToken = `Bearer ${token}`
expect(isTokenValid(token)).toBe(true)
expect(isTokenValid(bearerToken)).toBe(true)
expect(isTokenExpired(token)).toBe(false)
expect(isTokenExpired(bearerToken)).toBe(false)
})

test('returns false if token expires within 60 econds', () => {
expect(isTokenValid(createJwt(59))).toBe(false)
test('returns true if token expires within 60 econds', () => {
expect(isTokenExpired(createJwt(59))).toBe(true)
})
})

Expand Down Expand Up @@ -244,3 +245,24 @@ describe('handleAsyncError', () => {
expect(await handleAsyncError(func)()).toBe(1)
})
})

describe('hasSFRAAuthStateChanged', () => {
test('returns true when refresh_token keys are different', () => {
const storage = new Map()
const storageCopy = new Map()

storage.set('cc-nx-g', 'testRefreshToken1')
storageCopy.set('cc-nx', 'testRefreshToken2')

expect(hasSFRAAuthStateChanged(storage, storageCopy)).toBe(true)
})
test('returns false when refresh_token keys are the same', () => {
const storage = new Map()
const storageCopy = new Map()

storage.set('cc-nx-g', 'testRefreshToken1')
storageCopy.set('cc-nx-g', 'testRefreshToken2')

expect(hasSFRAAuthStateChanged(storage, storageCopy)).toBe(false)
})
})
shethj marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 2 additions & 1 deletion packages/template-retail-react-app/app/hooks/use-auth-modal.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ jest.mock('../commerce-api/utils', () => {
const originalModule = jest.requireActual('../commerce-api/utils')
return {
...originalModule,
isTokenValid: jest.fn().mockReturnValue(true),
isTokenExpired: jest.fn().mockReturnValue(false),
hasSFRAAuthStateChanged: jest.fn().mockReturnValue(false),
createGetTokenBody: jest.fn().mockReturnValue({
grantType: 'test',
code: 'test',
Expand Down
3 changes: 2 additions & 1 deletion packages/template-retail-react-app/app/pages/checkout/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ jest.mock('../../commerce-api/utils', () => {
const originalModule = jest.requireActual('../../commerce-api/utils')
return {
...originalModule,
isTokenValid: jest.fn().mockReturnValue(true),
isTokenExpired: jest.fn().mockReturnValue(false),
hasSFRAAuthStateChanged: jest.fn().mockReturnValue(false),
createGetTokenBody: jest.fn().mockReturnValue({
grantType: 'test',
code: 'test',
Expand Down
3 changes: 2 additions & 1 deletion packages/template-retail-react-app/app/pages/login/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ jest.mock('../../commerce-api/utils', () => {
const originalModule = jest.requireActual('../../commerce-api/utils')
return {
...originalModule,
isTokenValid: jest.fn().mockReturnValue(true),
isTokenExpired: jest.fn().mockReturnValue(false),
hasSFRAAuthStateChanged: jest.fn().mockReturnValue(false),
createGetTokenBody: jest.fn().mockReturnValue({
grantType: 'test',
code: 'test',
Expand Down
3 changes: 2 additions & 1 deletion packages/template-retail-react-app/app/pages/registration/index.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ jest.mock('../../commerce-api/utils', () => {
const originalModule = jest.requireActual('../../commerce-api/utils')
return {
...originalModule,
isTokenValid: jest.fn().mockReturnValue(true),
isTokenExpired: jest.fn().mockReturnValue(false),
hasSFRAAuthStateChanged: jest.fn().mockReturnValue(false),
createGetTokenBody: jest.fn().mockReturnValue({
grantType: 'test',
code: 'test',
Expand Down
4 changes: 2 additions & 2 deletions packages/template-retail-react-app/jest-setup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,12 +84,12 @@ jest.mock('pwa-kit-runtime/utils/ssr-config', () => {
}
})

// Mock isTokenValid globally
// Mock isTokenExpired globally
jest.mock('./app/commerce-api/utils', () => {
const originalModule = jest.requireActual('./app/commerce-api/utils')
return {
...originalModule,
isTokenValid: jest.fn().mockReturnValue(true)
isTokenExpired: jest.fn().mockReturnValue(false)
}
})

Expand Down