Clear local state when checkSession call fails #758
Conversation
| } | ||
| var parsedHash = eventData.event.data.response; | ||
| _this.webAuth.validateAuthenticationResponse(options, parsedHash, cb); | ||
| _this.webAuth.transactionManager.clearTransaction(error.state); |
There was a problem hiding this comment.
OK. This is the goal of this PR. However this clearTransaction won't get called if error!=null (see line 66, happy path). Is that fine?
There was a problem hiding this comment.
Yes, because then we'll call validateAuthenticationResponse, which needs the transaction to work (and it will be removed later)
| @@ -58,22 +59,22 @@ WebMessageHandler.prototype.run = function(options, cb) { | |||
| ) { | |||
| var error = err; | |||
There was a problem hiding this comment.
this could be
var error = err ? err : eventData.event.data.response.error
if (!error) {
parsedHash =..
return validateAuthenticationResponse(...);
}
//continue handling errors
There was a problem hiding this comment.
see error = eventData.event.data.response;the eventData.event.data.response.error property is a string.
| @@ -2402,7 +2404,7 @@ describe('auth0.WebAuth', function() { | |||
| stub(IframeHandler.prototype, 'init', function() { | |||
| this.timeoutCallback(); | |||
| }); | |||
| this.auth0.checkSession({}, function(err, data) { | |||
| this.auth0.checkSession({ state: 'foobar' }, function(err, data) { | |||
There was a problem hiding this comment.
why has this changed? If I read the tests below (line 2452) it seems it's using this key as default state key value to remove when no error is provided (because of timeout). What would happen if this value is not given, like in the previous diff, when an error is also not provided (because of timeout)?
There was a problem hiding this comment.
Because the underlying code now depends on a state and my stub for the transactionManager.process method doesn't return a state:
auth0.js/test/web-auth/web-auth.test.js
Line 2303 in 6ac59c4
Since I already tested (elsewhere)[https://github.com/auth0/auth0.js/blob/6ac59c4faedc003820f3909aef5b5d57a572fad7/test/web-auth/web-auth.test.js#L2362] that we handle those params correctly, there's no need to stub the method again, so I just added the state
| @@ -22,7 +22,8 @@ function runWebMessageFlow(authorizeUrl, options, callback) { | |||
| timeoutCallback: function() { | |||
| callback({ | |||
| error: 'timeout', | |||
| error_description: 'Timeout during executing web_message communication' | |||
| error_description: 'Timeout during executing web_message communication', | |||
| state: options.state | |||
There was a problem hiding this comment.
I find odd having an error object having "extra values" just for the sake of your code simplicity. It's easy to forget to filter the attributes returned to the user later.
There was a problem hiding this comment.
the server also sends the same object {error, error_description, state}. I'm just replicating what the server does
170: Update dependency auth0-js to v9.6.0 r=rehandalal a=renovate[bot] This Pull Request updates dependency [auth0-js](https://github.com/auth0/auth0.js) from `v9.5.1` to `v9.6.0` <details> <summary>Release Notes</summary> ### [`v9.6.0`](https://github.com/auth0/auth0.js/blob/master/CHANGELOG.md#v960httpsgithubcomauth0auth0jstreev960-2018-05-28) [Compare Source](auth0/auth0.js@v9.5.1...v9.6.0) [Full Changelog](auth0/auth0.js@v9.5.1...v9.6.0) **Changed** - Added `access_type` and `display` to the parameters-whitelist [\#​760](`https://github.com/auth0/auth0.js/pull/760`) ([lordnox]) **Fixed** - Clear local state when checkSession call fails [\#​758](`https://github.com/auth0/auth0.js/pull/758`) ([luisrudge]) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com). Co-authored-by: Renovate Bot <bot@renovateapp.com>
partial fix for #402