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.

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

Conversation

shethj
Copy link
Collaborator

@shethj shethj commented May 4, 2023

On login, PWA Kit stores several properties such as the access token and refresh token. Once an access token is present, PWA Kit will continue to use the same access token until it expires, where it will then use the refresh token to acquire a new access token.

In hybrid sites, SFRA logins are handled by plugin_SLAS. However, plugin_SLAS only has the ability to modify the refresh token on login. Since it is unable to modify/revoke the access token directly, situations can occur where a user with a PWA Kit access token navigates to SFRA, logs in via SFRA, then goes back to PWA Kit where their login status would remain unchanged since the access token from before the SFRA login will continue to be in use.

For PWA Kit to know whether plugin_SLAS has changed a user's login state, PWA needs to know when the refresh token changes.

Description

  • All PWA Kit login scenarios (guest, registered, and refresh token) should store the refresh token in a 2nd property in addition to cc-nx-g / cc-nx
  • On every request to SCAPI, before we check for the existence of an access token, run a check comparing the value of the refresh token stored in cc-nx-g / cc-nx with the 2nd copy. If the values are equal, continue to use the access token. If the values are different, throw out (or revoke) the existing access token and trigger a new login using the refresh token stored in cc-nx-g / cc-nx

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

Changes

  • Implemented hasSFRAAuthStateChanged(...) to compare refresh_token keys between SFRA and PWA
  • Moved constants from commerce-api/auth.js to commerce-api/constants.js for shared usage
  • updated auth.js with a getter function for isTokenValid to combine token validation and sfra login state changed checks
  • Updated if conditions to use this.auth.isTokenValid from auth.js instead of using the util function isTokenValid(...)

How to Test-Drive This PR

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

@shethj shethj requested a review from a team as a code owner May 4, 2023 19:45
bfeister
bfeister previously approved these changes May 4, 2023
Copy link
Collaborator

@bfeister bfeister left a comment

Choose a reason for hiding this comment

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

LGTM

kevinxh
kevinxh previously approved these changes May 4, 2023
Comment on lines 167 to 169
this._storageCopy.set(refreshTokenRegisteredStorageKey, token, {
expires: REFRESH_TOKEN_COOKIE_AGE
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

expires is for cookies, it doesn't work for localstorage

Copy link
Contributor

Choose a reason for hiding this comment

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

Dumb question: What does this look like if both the original and copy are stored in local store? They're stored separately still correct? Otherwise, since they have the same name, one would override the other.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@vcua-mobify The original and copy would both point to LocalStorage only in case of standalone setups in which case the token values will be the same and hasSFRAAuthStateChanged does not check values it only compares the keys. which again will be the same in this case. So we should be good.

Copy link
Contributor

@vcua-mobify vcua-mobify May 4, 2023

Choose a reason for hiding this comment

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

I think we want to be actively comparing the values since we open ourselves up to an edge case if we just look at the keys.

On a PWA Kit only environment (the only scenario where both point the original and copy are in local store) I think it's fine if we compare the value with itself or if the copy overrides the original since they should never diverge in this scenario in the first place.

On a hybrid environment, the original will be in the cookie store while the copy is in local store so we should have different values then.

(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

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 🎉

Comment on lines 167 to 169
this._storageCopy.set(refreshTokenRegisteredStorageKey, token, {
expires: REFRESH_TOKEN_COOKIE_AGE
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Dumb question: What does this look like if both the original and copy are stored in local store? They're stored separately still correct? Otherwise, since they have the same name, one would override the other.

kevinxh
kevinxh previously approved these changes May 4, 2023
kevinxh
kevinxh previously approved these changes May 4, 2023
@shethj shethj requested review from bfeister and vcua-mobify May 4, 2023 22:21
bfeister
bfeister previously approved these changes May 4, 2023
(storageCopy.get(refreshTokenGuestStorageKey) && refreshTokenGuestStorageKey) ||
(storageCopy.get(refreshTokenRegisteredStorageKey) && refreshTokenRegisteredStorageKey)

return refreshTokenKey !== refreshTokenCopyKey
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.

@shethj shethj dismissed stale reviews from bfeister and kevinxh via b1157d9 May 4, 2023 23:03
@bfeister bfeister self-requested a review May 5, 2023 14:36
bfeister
bfeister previously approved these changes May 5, 2023
Copy link
Contributor

@vcua-mobify vcua-mobify left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the edge case @shethj. This PR looks good to me

@bfeister bfeister self-requested a review May 5, 2023 16:33
@shethj shethj added this pull request to the merge queue May 5, 2023
Merged via the queue into develop with commit 744f9e4 May 5, 2023
vcua-mobify added a commit that referenced this pull request May 11, 2023
* Update `develop` with `release-v2.7.0` (#1033)

* Starting release process for 2.7.0

* Update Create MRT credentials file (#1025)

* Release 2.7: fix order of hooks (#1027)

* Return early only after all of the hooks are called

* Bumper version

---------

Co-authored-by: Ben Chypak <[email protected]>

* Quick fix for einstein data (#1028)

* Quick fix for einstein data

* Bump Version

* Re-lock package-lock files with npm 8

* Update package-lock.json

* Update einstein.js

* Regenerate lock files (#1030)

* Regenerate lock files

* Bump version to 2.7.0-alpha.3

* Bump to 2.7.0 (#1032)

* Begin development on 2.8.0

---------

Co-authored-by: Adam Raya <[email protected]>
Co-authored-by: Vincent Marta <[email protected]>

* Move the MRT reference app to the SDKs, so that we can verify eg. Node support (#966)

* BUG: Changed type of the phone number field to bring up numberic keyboard on mobile devices - W-9871940 (#1016)

Co-authored-by: Ben Chypak <[email protected]>

* Update Retail React App Page Designer integration README (#1041)

Co-authored-by: Richard Sexton <[email protected]>

* Implement `updateCustomerPassword` as no-op. (#1031)

* Allow query hook parameters to be `null`. (#1046)

* Remove unused util.

* Allow query hook parameters to be `null`.

* Fix addresses not having preferred address first. (#1051)

* Fix addresses not having preferred address first.

* Include all addresses, not just preferred address twice.

* Correctly include preferred address.

* Make `mergeBasket` Conditional More Robust (#1048)

* Update merge logic

* Update CHANGELOG.md

* Lint

* PR feedback

* Rename isNewlyRegisters to isNew for simplicity

* Lint

* [commerce-sdk-react] Decode pre-fetched token and save auth data in storage (#1052)

* add test

* decode jwt data

* lint

* Update packages/commerce-sdk-react/src/auth/index.ts

Co-authored-by: Will Harney <[email protected]>

* rename parseSlasJWT

* Update packages/commerce-sdk-react/src/auth/index.test.ts

Co-authored-by: Will Harney <[email protected]>

* convert all .thens to await

* make fake token in tests more clear

* lint

---------

Co-authored-by: Will Harney <[email protected]>

* Prevent modal to open when it fails to add an item to cart (#1053)

* prevent modal to open when it fails to add an item to cart

* Update comment

---------

Co-authored-by: Alex Vuong <[email protected]>

* Fix `getConfig` referencing config from incorrect location (#1049)

* Initial Commit

* Update CHANGELOG.md

* Revert some testing code

* Get test coverage back up

* Test build directory folder before proceeding.

* fix(template-retail-react-app): product-list refinements  (#957)

* fix(template-retail-react-app): product-list refinements

+ Ensure that the `selectedFilters` provided to the `Refinements` value components is always an array. And update said value components (checkbox, color, radio & size) to handle always receiving an array.
+ Apply some loose equality checks, catering for cases where a refinement URL param is parsed as a number but the refinement `.value` is a stringified number.
+ Fix bug where non-multiple filters could not be unchecked by selecting themselves (e.g. clicking an already checked checkbox).
+ Fix bug where a refinement URL param is parsed as a number, but was handled as a string (with `.split()`).
+ Fix bug where radio refinements would still appear as checked after clearing the value via `SelectedRefinements`.
+ Deprecate unused proptypes in `SizeRefinements`

* feat(retail-react-app): update `CheckboxRefinements`

+ Adjust the `onChange` logic so that multiple checkbox refinements can be selected

---------

Co-authored-by: Ben Chypak <[email protected]>

* update docs for shopper-experience scope (#1059)

* add docs for experience scope

* Add `merge_group` event to trigger GA builds

---------

Co-authored-by: Will Harney <[email protected]>
Co-authored-by: Adam Raya Navarro <[email protected]>

* Update lockfiles to reflect current version. (#1071)

* [commerce-sdk-react] Implement remaining Shopper Baskets cache logic (#1070)

* Refactor Shopper Basket cache following new pattern

* Fix types

* Update packages/commerce-sdk-react/src/hooks/ShopperBaskets/cache.ts

Co-authored-by: Will Harney <[email protected]>

* PR Feedback

* Clean up unused utils and types after refactor

* Add missing response basketId to queryKey

* Implement cache for the remaining mutations

* Tests WIP

* PR Feedback & Use query key path in `deleteBasket` cache

* Add tests for mutations returning void response

* PR Feedback

* Remove unused `and` hooks util

* Revert "Remove unused `and` hooks util"

This reverts commit c0a364a.

---------

Co-authored-by: Will Harney <[email protected]>

* [commerce-sdk-react] Fix Shopper Baskets Test case (#1082)

* Fix Shopper Basket empty response test cases

* lint

* feat(pwa-kit-dev): minor performance improvements and added comments (#974)

* feat(pwa-kit-dev): minor performance improvements and added comments

* docs(pwa-kit-dev): clean up comments

* refactor(pwa-kit-dev): update condition from PR feedback

---------

Co-authored-by: Will Harney <[email protected]>

* Update dependencies. (#1079)

* Remove internal packages to bypass lerna nonsense.

Include output.json to make restoring deps easier.

* Disable build scripts to make changing deps easier.

* Update root deps.

* Fix typescript issue.

* Create restore script to restore internal deps/scripts.

* Update commerce-sdk-react deps.

* Update test-commerce-sdk-react deps.

* Update template-typescript-minimal deps

* Improve restore script.

* Fix trailing comma.

* Update template-retail-react-app deps

* npm prune everything

* Update pwa-kit-runtime deps

* Update pwa-kit-react-sdk deps

* Update pwa-kit-create-app deps

* Update pwa-kit-dev deps (except eslint)

* Update pwa-kit-dev eslint deps

* Update internal-lib-build deps (except eslint)

* Update pwa-kit eslint deps

* Restore internal deps.

* Restore build scripts.

* Remove temporary helper files.

* Bump ua-parser-js to avoid vulnerable version.

* Anchor cross-env common dep to version used by internal-lib-build.

* Re-enable eslint.

* Implement Cache Logic for Shopper APIs (Contexts/Customers/Login/Orders) (#1073)

* Initial commit

* Update packages/commerce-sdk-react/src/hooks/useAuthHelper.ts

Co-authored-by: Will Harney <[email protected]>

* Update packages/commerce-sdk-react/src/hooks/ShopperOrders/cache.ts

Co-authored-by: Will Harney <[email protected]>

* Update packages/commerce-sdk-react/src/hooks/ShopperOrders/cache.ts

Co-authored-by: Will Harney <[email protected]>

* Added root to all query keys, use remove over clear

* Remove previous impemented "clear" from utils

* Initial tests for shoppercontexts

* Update ShopperLogin tests

* Fix order tests

* Update packages/commerce-sdk-react/src/hooks/ShopperContexts/cache.ts

Co-authored-by: Will Harney <[email protected]>

* Update cache.ts

* Lint!

* Update Json.tsx

* Lint!

* Testing race condition in tests

* Re-add tests in other order.

* Update CHANGELOG.md

* Add todo to complete context cache work

* Update packages/commerce-sdk-react/src/hooks/ShopperBaskets/mutation.test.ts

Co-authored-by: Will Harney <[email protected]>

* Update packages/commerce-sdk-react/src/components/ShopperExperience/Page/index.tsx

Co-authored-by: Will Harney <[email protected]>

* Update useAuthHelper.ts

* Update packages/commerce-sdk-react/src/hooks/ShopperCustomers/cache.ts

Co-authored-by: Adam Raya <[email protected]>

---------

Co-authored-by: Will Harney <[email protected]>
Co-authored-by: Adam Raya <[email protected]>

* remove site alias and locale from location.state.directedFrom path (#1065)

* remove site alias and locale from location.state.directedFrom path

* moving call to removeSiteLocaleFromPath into use-navigation hook

* fixing failing test, added tests for removeSiteLocaleFromPath

* per code review, skipping failed test instead of using mocking

* Fix Page Designer ImageWithText Link component (#1092)

* Fix PD ImageWithText Link component by using Chakra UI Link

* Use isAbsoluteURL to use react-router Link or Chakra Link component

* PR Feedback

* Clean up

* Update packages/template-retail-react-app/app/page-designer/assets/image-with-text/index.jsx

Co-authored-by: Ben Chypak <[email protected]>

* Remove temporal page-viewer page to facilitate review

---------

Co-authored-by: Ben Chypak <[email protected]>

* split ssr build on local (#1155)

* add suffix to ssr build files (#1157)

* Added session bridge call to login for phased launch (#1159)

* Added session bridge call to login for phased launch

* Fix code smell for session-bridge in hybrid

* Fix multi-value query params being lost (#1150)

* Fix multi-value query params being lost

* Update CHANGELOG.md

* Snyk dependency updates (#1169)

* Dependency updates

* Update runtime package lock

* Bump cosmiconfig version to latest

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

* Implement mechanism to store refresh token copy and compare with sfra

* Update tests and mocks for util function to check SFRA login state

* Fix linting issues

* FIx param types for util functionn

* Rename old isTokenValid to isTokenExpired

* Remove expiry for refresh_token in localstorage

* Update packages/template-retail-react-app/app/commerce-api/utils.js

Co-authored-by: Kevin He <[email protected]>

* fix test

* Fix linting on use-auth-modal.test.js

* Update hasSFRAStateChanged logic to compare keys and values

* Fix linting

---------

Co-authored-by: Kevin He <[email protected]>

* Add a redirect to login page after user signs out from checkout page (#1172)

* Add a redirect to login page after user signs out from checkout page

* Update CHANGELOG.md

* Remove history since navigate handles similarly

* Bump version number to 2.7.1-alpha.0

* Update changelogs

* #1174 Replace invalid value for wrap property (#1179)

* Update changelogs

* Version bump to 2.7.1-preview.0

* Revert "Version bump to 2.7.1-preview.0"

This reverts commit 985a7e0.

* Update CHANGELOG.md

* Rebuild lock files and fix ShopperLogin mutation test

* Revert "Rebuild lock files and fix ShopperLogin mutation test"

This reverts commit d9cfe50.

* Add additional properties to ShopperLogin test types (#1185)

* [V2] Re-generate lock files and fix hook lib tests (#1186)

* re-generate lock files and fix test

* Update packages/commerce-sdk-react/src/hooks/ShopperBaskets/index.test.ts

* Rebuild lockfiles using node 14 npm 8

* Revert "Rebuild lockfiles using node 14 npm 8"

This reverts commit 3d5c0cb.

* Use pwa-kit-dev for lint and format

* Revert "Use pwa-kit-dev for lint and format"

This reverts commit f46d83e.

* Add typescript to internal-lib-build and rebuild lock files

---------

Co-authored-by: vcua-mobify <[email protected]>
Co-authored-by: vcua-mobify <[email protected]>

* [V2] Internal lib build typescript dev dependency (#1194)

* Move typescript to dev and peer dependency

* Update package lock file

* Lockfile updates

---------

Co-authored-by: Ben Chypak <[email protected]>
Co-authored-by: Adam Raya <[email protected]>
Co-authored-by: Vincent Marta <[email protected]>
Co-authored-by: Oliver Brook <[email protected]>
Co-authored-by: echessman <[email protected]>
Co-authored-by: John Boxall <[email protected]>
Co-authored-by: Richard Sexton <[email protected]>
Co-authored-by: Will Harney <[email protected]>
Co-authored-by: Kevin He <[email protected]>
Co-authored-by: Alex Vuong <[email protected]>
Co-authored-by: Brad Adams <[email protected]>
Co-authored-by: Charles Lavery <[email protected]>
Co-authored-by: Adam Raya Navarro <[email protected]>
Co-authored-by: ecRobertEngel <[email protected]>
Co-authored-by: Sandra Golden <[email protected]>
Co-authored-by: Jainam Sheth <[email protected]>
Co-authored-by: mdenchev-aiopsgroup <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants