fix(totp): add totp code window valid config #2371
Conversation
ghost
added
the
vbudhram
changed the title
vbudhram
force-pushed
the
fix-test-failures
branch
from
1d8b477
to
cfc01e1
Compare
test/remote/totp_tests.js
Outdated
|
|
||
| it('should verify totp one outside one code window', () => { | ||
| const code = otplib.authenticator.generate() | ||
| return P.delay(4000) |
There was a problem hiding this comment.
Looking through previous remote tests, each consistently finishing in under 2 seconds, so having a 4 second delay here seemed like a safe choice for a code window.
There was a problem hiding this comment.
note, this can also be a problem for humans, since we're much slower than machines ;-)
usually for a 30s TOTP window, the verifiers allow for 90s of actual window
There was a problem hiding this comment.
we cannot wait for a test to be done in 30 secs, even 4 secs is very long time for this test. Can we avoid the 4 secs and test some other way?
There was a problem hiding this comment.
Generate TOTPs that are 20s ahead and 20s behind window (to check the window is ~90s) and/or 31s ahead/31s behind (to check it fails if outside 90s window), then verify them - should be faster than the 4s delay
At least, that's what I would do - I don't know if it makes sense for you or not
There was a problem hiding this comment.
Updated this to remove the delay and check against future and previous code windows.
|
Test failures fixed with mozilla/fxa-auth-db-mysql#330 |
vbudhram
force-pushed
the
fix-test-failures
branch
from
b912cb7
to
da100bf
Compare
|
@mozilla/fxa-devs r? |
config/index.js
Outdated
| doc: 'Tokens in the previous x-windows that should be considered valid', | ||
| default: 1, | ||
| format: 'nat', | ||
| env: 'TOTP_CODE_WINDOw' |
There was a problem hiding this comment.
nit: can we name this totpWindow ?
- env var is called
..._CODE_WINDOW
(2. and alsowindowis a browser global which is a bit trippy)
| @@ -18,7 +18,8 @@ module.exports = (log, db, mailer, customs, config) => { | |||
| // Default options for TOTP | |||
| otplib.authenticator.options = { | |||
| encoding: 'hex', | |||
| step: config.step | |||
| step: config.step, | |||
| window: config.window | |||
There was a problem hiding this comment.
if the nit is meh above, I think you can also do
endoing: 'hex',
step,
window
vbudhram
force-pushed
the
fix-test-failures
branch
3 times, most recently
from
d9908ff
to
ff3eec8
Compare
| @@ -64,7 +65,9 @@ module.exports = (log, db, mailer, customs, config) => { | |||
| .then(createResponse) | |||
| .then(() => reply(response), reply) | |||
|
|
|||
| const secret = otplib.authenticator.generateSecret() | |||
| const authenticator = new otplib.authenticator.Authenticator() | |||
There was a problem hiding this comment.
Not directly related to code window size, but changed this to be consistent with how we use otplib.authenticator in other files and repos.
| const P = require('../../../lib/promise') | ||
| const sinon = require('sinon') | ||
| const proxyquire = require('proxyquire').noPreserveCache() |
There was a problem hiding this comment.
Benefit from https://github.com/mozilla/fxa-auth-server/pull/2371/files#r178127428, is that we no longer need proxyrequire.
vbudhram
force-pushed
the
fix-test-failures
branch
from
a794ab2
to
a0ba10b
Compare
|
Thank you! @vladikoff @gdestuynder |
This PR adds a configurable window for TOTP codes to be valid, defaulting to 1.
Fixes mozilla/fxa-content-server#6005