feat(totp): initial recovery codes #319
Conversation
ghost
added
the
vbudhram
force-pushed
the
feature.recovery
branch
from
a12487c to
c831990
Compare
| ## createTotpToken(uid, sharedSecret, epoch) | ||
|
|
||
| Creates a new TOTP token for the user. | ||
| Creates a new TOTP token for the user. |
There was a problem hiding this comment.
Noticed TOTP section didn't have correct indention, fixed.
| const INSERT_RECOVERY_CODE = 'CALL createRecoveryCode_1(?, ?)' | ||
| MySql.prototype.replaceRecoveryCodes = function (uid, count) { | ||
|
|
||
| // Because of the hashing requirements the process of replacing |
There was a problem hiding this comment.
Open to suggestions here. Instead of building out one dynamic insert statement, I choose to insert each recovery code individually. This solution, while less efficient, seemed to be a good compromise on readability and security.
The stored procedure I used for dynamic insertion, was technically executing an arbitrary sql statement.
After #320, I will update this to use MySql.prototype.writeMultiple.
vbudhram
force-pushed
the
feature.recovery
branch
from
c831990 to
3aac51c
Compare
|
@mozilla/fxa-devs This is ready for review. Couple of notes
|
db-server/index.js
Outdated
| var bufferize = require('./lib/bufferize') | ||
| var version = require('../package.json').version | ||
| var errors = require('./lib/error') | ||
| const RECOVERY_CODE_COUNT = 8 |
There was a problem hiding this comment.
COUNT seems like the wrong suffix here, unless I misunderstood. Should it be LENGTH?
There was a problem hiding this comment.
Nope, I misunderstood. It should be COUNT. 😊
There was a problem hiding this comment.
Will we ever want to change this from 8? Any case to be made for it coming from config?
(...and even if it doesn't come from config, this module feels like the wrong place for it I think, it doesn't seem API-related)
There was a problem hiding this comment.
Will we ever want to change this from 8
👍🏽, the procedure is flexible so I'll push passing the count from the auth-server.
vbudhram
force-pushed
the
feature.recovery
branch
2 times, most recently
from
04bf9e4 to
2c259ab
Compare
|
@philbooth Rebased and updated! |
lib/db/mem.js
Outdated
| .then(() => { | ||
| let codes = [] | ||
| return dbUtil.generateRecoveryCodes(count) | ||
| .then((result) => { |
There was a problem hiding this comment.
If codes moved up a level we could flatten this promise chain.
lib/db/mem.js
Outdated
| codeHash: dbUtil.createHashSha512(code) | ||
| } | ||
| }) | ||
| return Promise.resolve(codes) |
lib/db/mem.js
Outdated
| const codes = recoveryCodes[uid] | ||
|
|
||
| if (! codes) { | ||
| return Promise.reject(error.notFound()) |
There was a problem hiding this comment.
Or throw error.notFound().
lib/db/mem.js
Outdated
|
|
||
| codes.splice(foundIndex, 1) | ||
|
|
||
| return Promise.resolve({ |
lib/db/mysql.js
Outdated
| return this.readFirstResult(CONSUME_RECOVERY_CODE, [uid, dbUtil.createHashSha512(code)]) | ||
| .then((result) => { | ||
| return P.resolve({ | ||
| remaining: result['COUNT(*)'] |
There was a problem hiding this comment.
Not sure if it's worth it, but would this be a simple result.count if you used SELECT COUNT(*) AS count in the stored procedure?
| randomByteCodes.push(randomBytes(4)) | ||
| } | ||
|
|
||
| return P.all(randomByteCodes) |
lib/db/util.js
Outdated
| codes = result.map((randomCode) => { | ||
| return randomCode.toString('hex') | ||
| }) | ||
| return Promise.resolve(codes) |
philbooth
left a comment
philbooth
left a comment
There was a problem hiding this comment.
Looks good! Feel free to ignore the inline comments, they're kind of bike-sheddy.
vbudhram
force-pushed
the
feature.recovery
branch
from
2c259ab to
c29aebc
Compare
vbudhram
force-pushed
the
feature.recovery
branch
from
c29aebc to
f39a2f4
Compare
|
@philbooth Thanks! Circle is having a bad day, tests pass, but it is not pushing to docker hub because of AWS degraded service. Merging |
Fixes #302