Skip to content

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

Merged
yuce merged 2 commits intoyuce:masterfrom
zbigniewpekala:fix-checking-hotp-token
Jul 9, 2019
Merged

Return boolean on pot:valid_hotp/2 and pot:valid_hotp/3#15
yuce merged 2 commits intoyuce:masterfrom
zbigniewpekala:fix-checking-hotp-token

Conversation

@zbigniewpekala
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Contributor Author

zbigniewpekala commented Jul 9, 2019

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
Copy Markdown
Contributor Author

zbigniewpekala commented Jul 9, 2019

/cc @yuce

@yuce
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

zbigniewpekala commented Jul 9, 2019

@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
Copy Markdown
Contributor Author

zbigniewpekala commented Jul 9, 2019

@yuce All passed now! 🍻

@yuce
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

zbigniewpekala commented Jul 9, 2019

@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
Copy Markdown
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