Skip to content
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

Fix valid_totp to support upper bound on check_candidate #20

Merged
merged 2 commits into from
Oct 15, 2019

Conversation

nalundgaard
Copy link
Collaborator

Resolves #18. The logic of pot: valid_totp/2,3 incorrectly fails to evaluate the highest candidate when the window is set to a value greater than 0. This is because the logic of pot:check_candidate/5 short-circuits on Current == Last rather than Current > Last, which is what would be needed for it to work as expected.

This change alters pot:check_candidate/5 to break on Current > Last, and updates the unit tests accordingly. Note that, because pot:valid_hotp/2,3 also use pot:check_candidate/5, this change extends by 1 the number of candidate trials that it will check when evaluating a hotp Token. This appears to also be a bug fix, because if you set trials to 1, previously the next valid hotp Token would be rejected.

Unit tests have been added to cover the cases described above.

@coveralls
Copy link

coveralls commented Oct 15, 2019

Pull Request Test Coverage Report for Build 37

  • 5 of 6 (83.33%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.7%) to 98.561%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/pot.erl 5 6 83.33%
Totals Coverage Status
Change from base Build 34: 0.7%
Covered Lines: 411
Relevant Lines: 417

💛 - Coveralls

@coveralls
Copy link

coveralls commented Oct 15, 2019

Pull Request Test Coverage Report for Build 39

  • 5 of 6 (83.33%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.7%) to 98.561%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/pot.erl 5 6 83.33%
Totals Coverage Status
Change from base Build 34: 0.7%
Covered Lines: 411
Relevant Lines: 417

💛 - Coveralls

Resolves yuce#18. The logic of `pot: valid_totp/2,3` incorrectly fails to evaluate the highest candidate when the `window` is set to a value greater than `0`. This is because the logic of `pot:check_candidate/5` short-circuits on `Current == Last` rather than `Current > Last`, which is what would be needed for it to work as expected.

This change alters `pot:check_candidate/5` to break on `Current > Last`, and updates the unit tests accordingly. Note that, because `pot:valid_hotp/2,3` **also** use  `pot:check_candidate/5`, this change extends by 1 the number of candidate trials that it will check when evaluating a hotp Token. This appears to also be a bug fix, because if you set `trials` to `1`, previously the next valid hotp Token would be rejected.

Unit tests have been added to cover the cases described above.
@yuce
Copy link
Owner

yuce commented Oct 15, 2019

@nalundgaard Thanks for your contribution! Could you add your name to CONTRIBUTORS ?

@yuce yuce merged commit 07b5106 into yuce:master Oct 15, 2019
@nalundgaard nalundgaard deleted the fix_window branch October 15, 2019 17:47
@nalundgaard
Copy link
Collaborator Author

@yuce Thanks for the merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

addwindow doesn't work for totp
4 participants