@W-15490276@ Improved auth error logging for PWA Kit v2 #2031
Conversation
| @@ -201,6 +202,7 @@ class Auth { | |||
| const response = await this._api.shopperLogin.getAccessToken(options) | |||
| // Check for error response before handling the token | |||
| if (response.status_code) { | |||
There was a problem hiding this comment.
Let's add a comment here that clarifies the expectation that any status_code at all is a signal of error (because success omits status_code
There was a problem hiding this comment.
A comment has been added.
Also, after doing some testing, only SLAS getAccessToken will return a status_code that is a string.
The other endpoints where an auth related HTTPError can be produced have a status code that is a number.
So rather than change HTTPError to cast the status to a number, we set a number here.
| @@ -11,7 +11,7 @@ export class HTTPError extends Error { | |||
| this.constructor = HTTPError | |||
| this.__proto__ = HTTPError.prototype | |||
| this.message = message | |||
| this.status = status | |||
| this.status = Number(status) | |||
There was a problem hiding this comment.
We need to parse this string to split at the first space character and properly coerce the left-hand side of the split to a number because SLAS sometimes sends things like '400 - DESCRIPTION of PROBLEM'
There was a problem hiding this comment.
I removed this casting here and updated the place in Auth where a string status can be passed in (which was just the getToken call)
| @@ -201,6 +202,7 @@ class Auth { | |||
| const response = await this._api.shopperLogin.getAccessToken(options) | |||
| // Check for error response before handling the token | |||
| if (response.status_code) { | |||
| console.log(`Auth Error: HTTP ${response.status_code} - ${response.message}`) | |||
There was a problem hiding this comment.
Do we want to use console.error instead?
| this._clearAuth() | ||
| console.log(`Auth Error: HTTP ${response.status_code} - ${response.message}`) | ||
| throw new HTTPError(response.status_code, response.message) |
There was a problem hiding this comment.
Since we've handled the error here (by clearing auth), I think we better not throw the error too 🤔 Usually we'd throw errors, so that the caller is able to catch and handle it. But in this case, we've proactively handled the error ourselves.
There was a problem hiding this comment.
The thrown error has been removed in favour of an automatic fallback to the new guest login.
| // fall back to new guest login | ||
| return this._loginAsGuest() |
There was a problem hiding this comment.
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.
|
@vcua-mobify What are the incentives of merging into v2.8? The change is in the template itself, existing customers are not able to consume these changes from upgrading npm, and there will be no new customers using v2. What we can do is refer customers to this PR so they can make change if needed, we don't need to merge it |
|
As the change is to the template-retail-react-app, we will not merge as we do not expect any new projects to begin on v2. Existing v2 projects should refer to this PR to see how refresh token and new guest login errors can be handled. |
This is the v2 version of #2028 and is 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, when the refresh token call fails, we do not clear the bad refresh token from local / cookie storage. As a result, when the user tries to log in again, the system will re-try with the same bad refresh token until the refresh token expires after 90 days or is manually cleared.
This change clears the refresh token when the refresh token fails.
This change also updates logging so that calls to the SLAS /authorize, /authenticate, and /token endpoints are logging error messages when they fail.
For v2, I updated SLAS calls to include the channel_id property
Testing the changes:
Refresh login (note: the refresh login request happens client side only):
commerce-sdk-react ERROR invalid organizationIdNew guest login: