-
Notifications
You must be signed in to change notification settings - Fork 493
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
Add __enableImpersonation flag to enable impersonation again #689
Conversation
var transactionStateMatchesState = transactionState === state; | ||
if (!state || !transactionStateMatchesState) { | ||
var shouldBypassStateChecking = !state && !transactionState && options.__enableImpersonation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
state
and transactionState
are missing for every impersonation call?
Also I don't know how this boolean checks works on JS but probably options. __enableImpersonation
will be false 99% of the cases, so I'd move that check to the beginning of the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if the user provides a value in that field (image you shared). Where does that end up being added, options.state (transaction state) or the request state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the value from the screen will be inside parsedHash.state
. Then it's up to the developer to call parseHash({state: 'previously-set-state'})
(this behavior wasn't altered)
@@ -182,8 +182,11 @@ WebAuth.prototype.validateAuthenticationResponse = function(options, parsedHash, | |||
var state = parsedHash.state; | |||
var transaction = this.transactionManager.getStoredTransaction(state); | |||
var transactionState = options.state || (transaction && transaction.state) || null; | |||
|
|||
var transactionStateMatchesState = transactionState === state; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this be true
for both values of null
? e.g. null === null
-> transactionStateMatchesState=true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I thought about all the combinations possible and it looks fine. Might need a second pair of eyes here @cocojoe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed internally.
fix #683