@W-15490276@ Better refresh token error logging for V3 #2028

vcua-mobify · 2024-09-25T23:23:01Z

Related to #1829

There are a few ways in which a call to SLAS to consume a refresh token can fail. For example, the client id the refresh token is associated with might be different from the client id included with the request.

Before this change, the error logging for a failed refresh token call only triggered when SLAS returned the error message 'invalid refresh token'.

This change opens up the logging so that any failed refresh token call error message is logged.

This change also updates new guest login to also log error messages.

Testing the changes:
Refresh login (note: the refresh login request happens client side only):

Start the app with these changes locally (this will start the app against zzrf-001)
Delete the access token in your local store
On a different browser window, open up https://test.phased-launch-testing.com/ and grab the refresh token
Copy the refresh token from test.phased-launch-testing into the cc-nx-g cookie on the tab that is open on zzrf-001
Refresh the tab on zzrf-001
See in the browser console that there is a log entry for commerce-sdk-react ERROR invalid organizationId
Verify that your refresh token was replaced with a new one that is usable for zzrf-001

New guest login:

In default.js, set an invalid client ID
Start the app (make sure you don't have an existing refresh token or access token)
In server logs, see that there is a log entry for 400 Bad Request with a descriptive SLAS error message (ie. client_id must be 36 chars)
In browser console, see the same message.

vmarta · 2024-09-27T18:05:05Z

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

+ await this.logTokenRequestError(error as Error)
+ throw error


Because error.response has been consumed with json(), when we throw an error here, the caller can no longer read what's inside error.response anymore.

Oh, inspired by what's seen in your other PR: rather than reusing the same error, let's create a new error (with HTTPError) and throw that one instead.

I've updated this to throw a new Error (since commerce-sdk-react currently does not have access to HTTPError).

vmarta · 2024-09-27T18:08:12Z

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

+ const json = await (error['response'] as Response).json()
+ const status_code: string = json.status_code
+ const responseMessage: string = json.message
+
+ this.logger.error(`${status_code} ${responseMessage}`)


Similar to the previous comment, do we really need to log the response details?

I think as long as we handle any refresh token errors (rather than one with a specific error message), then this is already better error handling than before.

Another way to put it is that in the case of refresh token errors, IMO gracefully handling those errors (by clearing storage and then restarting the login flow) is more important than the logging part.

I think we do need to log the response details because there are a variety of different types of problems that SLAS could respond with and we don't want this to happen in a place where developers can't get into our SDK to get those clarifying response error messages that are missing

In "development mode" they can look at the Network tab in Chrome, but if a customer hits an error and can't reproduce it / the dev tools weren't already open, seeing the verbose message would become critical in catching and fixing errors in production

vmarta

FYI done reviewing.

vmarta · 2024-09-27T18:19:07Z

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

+ expect(refreshAccessTokenSpy).toHaveBeenCalled()
+ expect(helpers.loginGuestUser).toHaveBeenCalled()


Do we also want to check whether the storage or refresh token has been updated properly afterwards?

I've added a check to verify that the storage has been cleared after the refresh attempt.

Testing that the loginGuestUser has updated the refresh token isn't too valuable since it's a mock method that we'd define and control in the test. Testing that the token has been replaced is better done in an e2e test that involves actual SLAS calls.

I think for our unit tests, it is enough to verify that loginGuestUser has been invoked.

vmarta · 2024-09-27T18:20:29Z

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

+ try {
+ return await this.queueRequest(callback, isGuest)
+ } catch (error) {


Thank you for catching and handling this additional scenario 👍

joeluong-sfcc · 2024-09-27T19:51:33Z

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

+ // ie. 'Bad Request' for 400. We need to drill specifically into the ResponseError
+ // to get a more descriptive error message from SLAS
+ if (error instanceof Error && 'response' in error) {
+ // commerce-sdk-isomorphic throws a `ResponseError`, but doesn't export the class.


We control this package so there's nothing stopping from exporting it :D

but the release wouldn't make it in time for this PR, so maybe we can export it in the next release and follow up in another PR if we really want to

vmarta · 2024-09-27T22:57:48Z

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

+ // We catch the error here to do logging but we still need to
+ // throw an error to stop the login flow from continuing.
+ await this.logTokenRequestError(err)
+ throw new Error(`New guest user could not be logged in. ${err.message}`)


This way the caller would also not have access to read the response details. How can we pass along those details to them?

I've made changes for extracting the status code and message from the ResponseError so that they can be passed around.

vmarta · 2024-09-27T23:19:06Z

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

+ // clean up storage and restart the login flow
+ this.clearStorage()


Minor: how about us console.loging here to tell others that we've handled the error by doing a guest login? I think it'd be useful for them to see that in the log too... not just the errors.

alexvuong · 2024-09-27T23:43:44Z

packages/commerce-sdk-react/CHANGELOG.md

@@ -1,5 +1,6 @@
 ## v3.1.0-dev (Aug 08, 2024)

+- Better refresh token error logging [#2028](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/2028)


Suggested change

- Better refresh token error logging [#2028](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/2028)

- Improve refresh token error logging [#2028](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/2028)

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

vcua-mobify added 2 commits September 25, 2024 15:35

Always log a message if refresh login fails

ac6b399

Add test for fallback to guest login

b1b4084

vcua-mobify requested a review from a team as a code owner September 25, 2024 23:23

vcua-mobify added 5 commits September 25, 2024 16:24

Update CHANGELOG.md

2278669

Add status code to error message

bd49986

Add logging for guest login

9982821

Lint fix

89dba9f

Test guest login returns error

58eb95c

vcua-mobify requested a review from vmarta September 26, 2024 19:57

Merge branch 'develop' into handle-refresh-token-error

443116d

vcua-mobify mentioned this pull request Sep 26, 2024

@W-15490276@ Improved auth error logging for PWA Kit v2 #2031

Closed

bfeister changed the title ~~@W-15490276@ Better refresh token error logging~~ @W-15490276@ Better refresh token error logging for V3 Sep 27, 2024

vmarta reviewed Sep 27, 2024

View reviewed changes

joeluong-sfcc reviewed Sep 27, 2024

View reviewed changes

vcua-mobify added 2 commits September 27, 2024 15:12

Check that refresh token is cleared

a397c1d

Update thrown error

b297f19

vcua-mobify requested review from vmarta, bfeister and joeluong-sfcc September 27, 2024 22:26

vmarta reviewed Sep 27, 2024

View reviewed changes

alexvuong reviewed Sep 27, 2024

View reviewed changes

Apply feedback

2eb5fec

vcua-mobify requested review from vmarta and alexvuong October 1, 2024 18:20

alexvuong reviewed Oct 1, 2024

View reviewed changes

packages/commerce-sdk-react/src/auth/index.ts Show resolved Hide resolved

vmarta approved these changes Oct 1, 2024

View reviewed changes

Merge branch 'develop' into handle-refresh-token-error

bb2ee7c

alexvuong approved these changes Oct 1, 2024

View reviewed changes

Merge branch 'develop' into handle-refresh-token-error

8aa99a5

vcua-mobify enabled auto-merge (squash) October 1, 2024 20:25

vcua-mobify merged commit ccb17bf into develop Oct 1, 2024
23 of 29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@W-15490276@ Better refresh token error logging for V3 #2028

@W-15490276@ Better refresh token error logging for V3 #2028

vcua-mobify commented Sep 25, 2024 •

edited

Loading

vmarta Sep 27, 2024

vmarta Sep 27, 2024

vcua-mobify Sep 27, 2024

vmarta Sep 27, 2024 •

edited

Loading

bfeister Sep 27, 2024 •

edited

Loading

vmarta left a comment

vmarta Sep 27, 2024

vcua-mobify Sep 27, 2024 •

edited

Loading

vmarta Sep 27, 2024

joeluong-sfcc Sep 27, 2024

vmarta Sep 27, 2024

vcua-mobify Oct 1, 2024

vmarta Sep 27, 2024 •

edited

Loading

alexvuong Sep 27, 2024

vcua-mobify Oct 1, 2024

		expect(refreshAccessTokenSpy).toHaveBeenCalled()
		expect(helpers.loginGuestUser).toHaveBeenCalled()

		// clean up storage and restart the login flow
		this.clearStorage()

		@@ -1,5 +1,6 @@
		## v3.1.0-dev (Aug 08, 2024)

		- Better refresh token error logging [#2028](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/2028)

	- Better refresh token error logging [#2028](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/2028)
	- Improve refresh token error logging [#2028](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/2028)

@W-15490276@ Better refresh token error logging for V3 #2028

@W-15490276@ Better refresh token error logging for V3 #2028

Conversation

vcua-mobify commented Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmarta Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

bfeister Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

vmarta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcua-mobify Sep 27, 2024 • 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

vmarta Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcua-mobify commented Sep 25, 2024 •

edited

Loading

vmarta Sep 27, 2024 •

edited

Loading

bfeister Sep 27, 2024 •

edited

Loading

vcua-mobify Sep 27, 2024 •

edited

Loading

vmarta Sep 27, 2024 •

edited

Loading