-
Notifications
You must be signed in to change notification settings - Fork 38
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 addwindow #19
Fix addwindow #19
Conversation
- fixes yuce#18
Pull Request Test Coverage Report for Build 36
💛 - Coveralls |
@@ -106,7 +106,7 @@ valid_totp(Token, Secret, Opts) -> | |||
Token -> | |||
true; | |||
_ -> | |||
Window = proplists:get_value(window, Opts, 0), | |||
Window = proplists:get_value(addwindow, Opts, 0), | |||
case check_candidate(Token, Secret, IntervalsNo - Window, IntervalsNo + Window, Opts) of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't addwidow
a number that should multiply the interval_length
? This is now subtracting/adding the result of time_interval(Opts)
(big number like 52357080) by a relatively small number, Window = addwindow
(-2-1,0,1,2,3).
Also if you really plan to remove support of window
option, can you update the references of it in the README?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addwindow
is supposed to offset the interval for checking the totp, not change the interval_length
.
@@ -119,21 +119,20 @@ valid_totp(Token, Secret, Opts) -> | |||
-spec time_interval(proplist()) -> time(). | |||
time_interval(Opts) -> | |||
IntervalLength = proplists:get_value(interval_length, Opts, 30), | |||
AddSeconds = proplists:get_value(addwindow, Opts, 0) * proplists:get_value(interval_length, Opts, 30), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain to me why this behavior should be changed? We extensively use this in our application tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks to me like this change breaks the window
parameter and alters addwindow
to work the way window
already does. I think that this PR and issue #18 should be closed.
@@ -106,7 +106,7 @@ valid_totp(Token, Secret, Opts) -> | |||
Token -> | |||
true; | |||
_ -> | |||
Window = proplists:get_value(window, Opts, 0), | |||
Window = proplists:get_value(addwindow, Opts, 0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong to me. window
is the parameter that expands the number of allowed interval tokens to be used (basically +/- window
intervals are considered valid). addwindow
is supposed to offset the interval that you check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @nalundgaard, that's very useful to know! It would be great if the docs described these options in a bit more detail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danturn Indeed! It's definitely worth tracking separately. The examples hint at what the options do, but they really don't explain in detail their intention, so you kind of have to piece it together from reading the code and being familiar with the hotp/totp RFC nomenclature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup I was misunderstanding how this works, since it's different from the Python library referenced in the README. Bit scary that no tests failed. Will close. |
Fixes #18