fix(verify): verifyTokenWithMethod should update already verified session status
#329
Conversation
ghost
added
the
| UPDATE `sessionTokens` SET verificationMethod = verificationMethodArg, verifiedAt = verifiedAtArg | ||
| WHERE tokenId = tokenIdArg; | ||
|
|
||
| SET @updateCount = (SELECT ROW_COUNT()); |
There was a problem hiding this comment.
The count returned should be the the number of session's updated, previously it was number of tokens verified.
db-server/test/backend/db_tests.js
Outdated
| return db.sessionToken(tokenId) | ||
| }, assert.fail) | ||
| .then((token) => { | ||
| assert.equal(!! token.mustVerify, false, 'mustVerify is null') |
There was a problem hiding this comment.
The comment here suggest we expect it to be null, but then for the test we coerce it to a boolean and compare it to false, which seems kinda funny.
If the comment is true why aren't we comparing to null? Or if the comment is wrong and we expect a boolean, why are we coercing it first? Is it expected that it might be null or false? Or can we guarantee that it will always be a boolean and eliminate the coercion? The stricter the tests are the better, is all I'm thinking.
There was a problem hiding this comment.
Whoops you are right, this doesn't need coercing. It just needs to check that the value is false.
vbudhram
force-pushed
the
update-verify-with-method
branch
from
dc06bc8 to
d91ffbd
Compare
From debugging mozilla/fxa-auth-server#2351, I found that the
verifyTokenWithMethodstored procedure was not correctly updating an already verified session's verificationMethod. This could help explain why mozilla/fxa-auth-server#2346 (comment) happened.Either way, this PR fixes the issue and matches how the mem db's method works.