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

Restart login flow if refresh_token is invalid #1135

Merged
merged 8 commits into from
Apr 24, 2023
Merged

Conversation

alexvuong
Copy link
Collaborator

@alexvuong alexvuong commented Apr 15, 2023

Description

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

  • (change1)

How to Test-Drive This PR

  • rebuild commerce-react-sdk
  • Run Retail App
  • Go to localStorage, delete access_token
  • Go to cookie, and change value in RefArch-cc-nx-g
  • Refresh the page
  • Observe that token returns an error, but the login flow started to fetch a new guest session

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)

@alexvuong alexvuong requested a review from a team as a code owner April 15, 2023 00:14
@alexvuong alexvuong self-assigned this Apr 15, 2023
wjhsf
wjhsf previously approved these changes Apr 17, 2023
wjhsf
wjhsf previously approved these changes Apr 18, 2023
kevinxh
kevinxh previously approved these changes Apr 21, 2023
@alexvuong alexvuong dismissed stale reviews from kevinxh and wjhsf via 0e2869d April 21, 2023 21:40
// We can't use `instanceof`, so instead we just check for the `response` property
// and assume it is a fetch Response.
const json = await (error['response'] as Response).json()
if (json.message === 'invalid refresh_token') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's not a lot of context for this change in the PR description.
I'm assuming we need to re-implement this because it's a functionality that got lost during the transition from commerce-api folder to commerce-sdk-react.

If that is the case, Do we want to also address the expired token scenario as we did in the commerce-api folder?

const retryErrors = [INVALID_TOKEN, EXPIRED_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.

What we did in commerce-api was to consider either for access_token and refresh_token. We handled that slightly different in commerce-react auth, we have checked for access_token expiration first, if it is still available, we don't need to check for refresh_token. In this PR, we only need to fix the bug when refresh_token is invalid

@alexvuong alexvuong merged commit 68cb474 into v3 Apr 24, 2023
bfeister added a commit that referenced this pull request Apr 25, 2023
* v3:
  [Spike]Replace watch with nodemon (#1146)
  Fix Page Designer ImageWithText Link component (#1092) (#1148)
  Upgrade deprecated dependencies (#1124)
  Restart login flow if refresh_token is invalid (#1135)
  Move some util to site-utils to avoid circular imports (#1133)
  Restore old file name. (#1140)
  Update eslint configuration (#1129)
  CI: clarify the environment variables (#1127)
  Remove react-query-devtools from production build (#1121)
  Update lerna.json (#1118)
  Parallelize lighthouse ci (#1126)
  Increase test timeouts only on CI env (#1123)
  Remove unused `request` deprecated dependency. (#1125)
  Upgrade msw to latest (#1100)
  Add mergeBasket hook (#1114)
  [V3][Hooks Integration 🪝] Manually update cache for ShopperCustomer (#1113)
  dont use callback on mutateAsync (#1119)
  2-spaces not 4-spaces (#1117)

# Conflicts:
#	packages/internal-lib-build/package-lock.json
#	packages/pwa-kit-create-app/package-lock.json
#	packages/pwa-kit-dev/package-lock.json
#	packages/pwa-kit-dev/package.json
#	packages/pwa-kit-dev/src/configs/webpack/config.js
#	packages/pwa-kit-dev/src/ssr/server/build-dev-server.js
#	packages/pwa-kit-react-sdk/package-lock.json
#	packages/pwa-kit-runtime/package-lock.json
#	packages/template-express-minimal/package-lock.json
#	packages/template-mrt-reference-app/package-lock.json
#	packages/template-retail-react-app/package-lock.json
#	packages/template-typescript-minimal/package-lock.json
#	packages/test-commerce-sdk-react/package-lock.json
bfeister added a commit that referenced this pull request Apr 25, 2023
…-rehaul

* feature/template-extensibility:
  [Spike]Replace watch with nodemon (#1146)
  Fix Page Designer ImageWithText Link component (#1092) (#1148)
  Upgrade deprecated dependencies (#1124)
  Restart login flow if refresh_token is invalid (#1135)
  Move some util to site-utils to avoid circular imports (#1133)
  Restore old file name. (#1140)
  Update eslint configuration (#1129)
  CI: clarify the environment variables (#1127)
  Remove react-query-devtools from production build (#1121)
  Update lerna.json (#1118)
  Parallelize lighthouse ci (#1126)
  Increase test timeouts only on CI env (#1123)
  Remove unused `request` deprecated dependency. (#1125)
  Upgrade msw to latest (#1100)
  Add mergeBasket hook (#1114)
  [V3][Hooks Integration 🪝] Manually update cache for ShopperCustomer (#1113)
  dont use callback on mutateAsync (#1119)
  2-spaces not 4-spaces (#1117)

# Conflicts:
#	packages/internal-lib-build/package-lock.json
#	packages/pwa-kit-dev/package-lock.json
#	packages/pwa-kit-react-sdk/package-lock.json
#	packages/pwa-kit-runtime/package-lock.json
#	packages/pwa-kit-runtime/package.json
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