Skip to content
This repository has been archived by the owner on Jun 25, 2024. It is now read-only.

Remove totpKey during successful login after TOTP grace period (fix #62) #63

Merged
merged 4 commits into from
Jan 12, 2015

Conversation

bartekn
Copy link
Contributor

@bartekn bartekn commented Jan 12, 2015

We were cancelling TOTP grace period after successful login (by setting totpDisabledAt to null) but we haven't tested logging in after TOTP grace period. In this case we should remove totpKey from a wallet.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) when pulling 0009cf0 on fix-62 into c891560 on master.

@@ -262,11 +262,26 @@ walletV2.clearTotpRemovalRequestIfPossible = function(wallet) {
if(!wallet.totpDisabledAt) {
return Promise.resolve();
} else {
// set the totpDisabledAt to null
var now = Math.floor((new Date()).getTime() / 1000);
var disabledAt = Math.floor(wallet.totpDisabledAt.getTime() / 1000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't you just directly comparing dates?

if (wallet.totpDisabledAt > new Date())
...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied it from here. I was thinking about the same thing but I thought there's something important behind rounding it to seconds. Can change this.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling 25b3a5e on fix-62 into c891560 on master.

@nullstyle
Copy link
Contributor

@bartekn I've updated the tests. thoughts?


it("resets the totpKey and totpDisabled after the grace period has elapsed", function() {
var username = "mfa-disabled@stellar.org";
return this.submit({username:username, walletId:new Buffer(username).toString("base64")})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing I'd change is adding:

.expect(200)

here and in the second test. The rest is OK! 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

word

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling ed2bc82 on fix-62 into c891560 on master.

nullstyle added a commit that referenced this pull request Jan 12, 2015
Remove totpKey during successful login after TOTP grace period (fix #62)
@nullstyle nullstyle merged commit 6db65af into master Jan 12, 2015
@nullstyle nullstyle removed the pr label Jan 12, 2015
@nullstyle nullstyle deleted the fix-62 branch January 12, 2015 23:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants