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.

Sign up for GitHub

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

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()}`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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