Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Return boolean on pot:valid_hotp/2 and pot:valid_hotp/3 #15

Merged
merged 2 commits into from
Jul 9, 2019
Merged

Return boolean on pot:valid_hotp/2 and pot:valid_hotp/3 #15

merged 2 commits into from
Jul 9, 2019

Conversation

zbigniewpekala
Copy link
Contributor

@zbigniewpekala zbigniewpekala commented Jul 9, 2019 •
edited
Loading

Based on the specs https://github.com/yuce/pot/blob/master/src/pot.erl#L76 pot:valid_hotp/2 and pot: valid_hotp/3 should return boolean.

@coveralls
Copy link

coveralls commented Jul 9, 2019 •
edited
Loading

Pull Request Test Coverage Report for Build 18

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 97.872%
Totals Coverage Status
Change from base Build 16: 0.03%
Covered Lines: 138
Relevant Lines: 141
💛 - Coveralls

@zbigniewpekala
Copy link
Contributor Author

There is an error on Erlang 17.5 on the CI

===> Running coveralls...
rebar_coveralls:Exporting cover data from _build/test/cover/eunit.coverdata using service travis-ci and jobid 556217714
Analysis includes data from imported files
["/home/travis/build/yuce/pot/_build/test/cover/eunit.coverdata"]
Analysis includes data from imported files
["/home/travis/build/yuce/pot/_build/test/cover/eunit.coverdata"]
===> Uncaught error in rebar_core. Run with DEBUG=1 to see stacktrace or consult rebar3.crashdump
===> When submitting a bug report, please include the output of `rebar3 report "your command"`
make: *** [coveralls] Error 1
The command "make coveralls" exited with 2.
Done. Your build exited with 1.

I don't have OTP 17.5 locally, so cannot reproduce this atm. I have no idea how this change may fail only on OTP 17.5 though.

@zbigniewpekala
Copy link
Contributor Author

/cc @yuce

@yuce
Copy link
Owner

yuce commented Jul 9, 2019

@zbigniewpekala Probably kerl doesn't support 17.5 anymore. Do you mind removing 17.5 from .travis.yml and adding 21.3 and 22.0 ?

@yuce
Copy link
Owner

yuce commented Jul 9, 2019

Thanks for this fix, not sure how it wasn't noticed before. Could you add your name to CONTRIBUTORS ?

@zbigniewpekala
Copy link
Contributor Author

@yuce Will do. I'm also curious why dialyzer did not see this problem, but it was found by dialyzer in Elixir project where we used the lib.

@zbigniewpekala
Copy link
Contributor Author

@yuce All passed now! 🍻

@yuce
Copy link
Owner

yuce commented Jul 9, 2019

Great! Thanks for your contribution!

@yuce yuce merged commit 18c7a94 into yuce:master Jul 9, 2019
@zbigniewpekala zbigniewpekala deleted the fix-checking-hotp-token branch July 9, 2019 10:20
@zbigniewpekala
Copy link
Contributor Author

@yuce Any ideas when I can expect new release with the fix? I guess 0.9.8 version https://hex.pm/packages/pot?

@yuce
Copy link
Owner

yuce commented Jul 9, 2019

0.9.8 is live on hex.

nalundgaard added a commit that referenced this pull request Feb 19, 2020
@nalundgaard
Add return_interval option to valid_hotp
400b0e3 
There is a flaw in the `valid_hotp` logic introduced in #15 that prevents it from being effectively used: because it returns a raw `boolean`, there is no way to know *which* interval matched: the next one, the 1000th one, or somewhere in between? In order to effectively implement a proper HOTP scheme, you need to track the interval number of the last valid HOTP to prevent its reuse, OR the reuse of a HOTP from any previous interval.

For backwards compatibility, I have retained the default `boolean()` response, but if the `return_interval` option is set, the interval will be returned when a hotp is valid, like `{true, interval()}`.
@nalundgaard nalundgaard mentioned this pull request Feb 19, 2020
Merged
nalundgaard added a commit that referenced this pull request Feb 19, 2020
@nalundgaard
Add return_interval option to valid_hotp
21dbd07 
There is a flaw in the `valid_hotp` logic introduced in #15 that prevents it from being effectively used: because it returns a raw `boolean`, there is no way to know *which* interval matched: the next one, the 1000th one, or somewhere in between? In order to effectively implement a proper HOTP scheme, you need to track the interval number of the last valid HOTP to prevent its reuse, OR the reuse of a HOTP from any previous interval.

For backwards compatibility, I have retained the default `boolean()` response, but if the `return_interval` option is set, the interval will be returned when a hotp is valid, like `{true, interval()}`.
nalundgaard added a commit that referenced this pull request Feb 19, 2020
@nalundgaard
Add return_interval option to valid_hotp
62b8289 
There is a flaw in the `valid_hotp` logic introduced in #15 that prevents it from being effectively used: because it returns a raw `boolean`, there is no way to know *which* interval matched: the next one, the 1000th one, or somewhere in between? In order to effectively implement a proper HOTP scheme, you need to track the interval number of the last valid HOTP to prevent its reuse, OR the reuse of a HOTP from any previous interval.

For backwards compatibility, I have retained the default `boolean()` response, but if the `return_interval` option is set, the interval will be returned when a hotp is valid, like `{true, interval()}`.
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zbigniewpekala @coveralls @yuce