Fix valid_totp to support upper bound on check_candidate

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.

src/pot.erl

@@ -111,10 +111,12 @@ valid_totp(Token, Secret, Opts) ->
                         false ->
                             false;
                         _ ->
-                            true end end;
+                            true
+                    end
+            end;
         _ ->
-            false end.
-
+            false
+    end.
 
 -spec time_interval(proplist()) -> time().
 time_interval(Opts) ->
@@ -123,17 +125,16 @@ time_interval(Opts) ->
   {MegaSecs, Secs, _} = proplists:get_value(timestamp, Opts, os:timestamp()),
   trunc((MegaSecs * 1000000 + (Secs + AddSeconds)) / IntervalLength).
 
+-spec check_candidate(token(), secret(), time(), time(), proplist()) -> time() | false.
 check_candidate(Token, Secret, Current, Last, Opts) when Current =< Last ->
-    case Current of
-        Last ->
-            false;
+    case hotp(Secret, Current, Opts) of
+        Token ->
+            Current;
         _ ->
-            Candidate = hotp(Secret, Current, Opts),
-            case Candidate of
-                Token ->
-                    Current;
-                _ ->
-                    check_candidate(Token, Secret, Current + 1, Last, Opts) end end.
+            check_candidate(Token, Secret, Current + 1, Last, Opts)
+    end;
+check_candidate(_Token, _Secret, _Current, _Last, _Opts) ->
+    false.
 
 -spec prepend_zeros(token(), non_neg_integer()) -> token().
 prepend_zeros(Token, N) ->

test/hotp_validity_tests.erl

@@ -5,67 +5,64 @@
 
 
 checking_hotp_validity_without_range_test_() ->
-    {setup, fun start/0,
-            fun stop/1,
-            fun checking_hotp_validity_without_range/1}.
+    {foreach,
+     fun start/0,
+     fun stop/1,
+     [fun checking_hotp_validity_without_range/1,
+      fun checking_hotp_validity_max_default_range/1,
+      fun checking_hotp_validity_past_max_default_range/1,
+      fun validating_correct_hotp_after_exhaustion/1,
+      fun validating_correct_totp_as_hotp/1,
+      fun retrieving_proper_interval_from_validator/1,
+      fun hotp_for_range_exact_match/1,
+      fun hotp_for_range_preceding_match/1]}.
 
 
-validating_correct_hotp_after_exhaustion_test_() ->
-    {setup, fun start/0,
-            fun stop/1,
-            fun validating_correct_hotp_after_exhaustion/1}.
-
-
-validating_correct_totp_as_hotp_test_() ->
-    {setup, fun start/0,
-            fun stop/1,
-            fun validating_correct_totp_as_hotp/1}.
-
-
-retrieving_proper_interval_from_validator_test_() ->
-    {setup, fun start/0,
-            fun stop/1,
-            fun retrieving_proper_interval_from_validator/1}.
+start() ->
+    _Secret = <<"MFRGGZDFMZTWQ2LK">>.
 
 
-hotp_for_range_preceding_match_test_() ->
-    {setup, fun start/0,
-            fun stop/1,
-            fun hotp_for_range_preceding_match/1}.
+stop(_) ->
+    ok.
 
 
-start() ->
-    ok.
+checking_hotp_validity_without_range(Secret) ->
+    [?_assert(pot:valid_hotp(pot:hotp(Secret, 123), Secret))].
 
 
-stop(_) ->
-    ok.
+checking_hotp_validity_max_default_range(Secret) ->
+    [{"hotp at the max # of trials (1000) past the start (1) is valid",
+      ?_assert(pot:valid_hotp(pot:hotp(Secret, 1001), Secret))}].
 
 
-checking_hotp_validity_without_range(_) ->
-    Secret = <<"MFRGGZDFMZTWQ2LK">>,
-    [?_assertEqual(pot:valid_hotp(pot:hotp(Secret, 123), Secret), true)].
+checking_hotp_validity_past_max_default_range(Secret) ->
+    [{"hotp beyond the max # of trials (1000) past the start (1) is invalid",
+      ?_assertNot(pot:valid_hotp(pot:hotp(Secret, 1002), Secret))}].
 
 
-validating_correct_hotp_after_exhaustion(_) ->
-    Secret = <<"MFRGGZDFMZTWQ2LK">>,
-     [?_assertEqual(pot:valid_hotp(
-            pot:hotp(Secret, 123), Secret, [{last, 123}]), false)].
+validating_correct_hotp_after_exhaustion(Secret) ->
+     [?_assertNot(pot:valid_hotp(pot:hotp(Secret, 123), Secret, [{last, 123}]))].
 
 
-validating_correct_totp_as_hotp(_) ->
-    Secret = <<"MFRGGZDFMZTWQ2LK">>,
-    [?_assertEqual(pot:valid_hotp(pot:totp(Secret), Secret), false)].
+validating_correct_totp_as_hotp(Secret) ->
+    [?_assertNot(pot:valid_hotp(pot:totp(Secret), Secret))].
 
 
-retrieving_proper_interval_from_validator(_) ->
-    Secret = <<"MFRGGZDFMZTWQ2LK">>,
+retrieving_proper_interval_from_validator(Secret) ->
     Totp = <<"713385">>,
     Result = pot:valid_hotp(Totp, Secret, [{last, 1}, {trials, 5}]),
-    [?_assertEqual(Result, true),
+    [?_assert(Result),
      ?_assertEqual(pot:hotp(Secret, 4), Totp)].
 
 
+hotp_for_range_exact_match(_) ->
+    Secret = <<"MFRGGZDFMZTWQ2LK">>,
+    Totp = <<"816065">>,
+    Result = pot:valid_hotp(Totp, Secret, [{last, 1}, {trials, 1}]),
+    [?_assert(Result),
+     ?_assertEqual(pot:hotp(Secret, 2), Totp)].
+
+
 hotp_for_range_preceding_match(_) ->
     Secret = <<"MFRGGZDFMZTWQ2LK">>,
-    [?_assertEqual(pot:valid_hotp(<<"713385">>, Secret, [{last, 1}, {trials, 2}]), false)].
+    [?_assertNot(pot:valid_hotp(<<"713385">>, Secret, [{last, 1}, {trials, 2}]))].

test/totp_generation_tests.erl

@@ -23,16 +23,13 @@ stop(_) ->
 
 generating_current_totp_and_validating(_) ->
     Secret = <<"MFRGGZDFMZTWQ2LK">>,
-    IntervalsNo = trunc(time_now() / 30),
+    IntervalOpts = [{timestamp, os:timestamp()}],
+    IntervalsNo = pot:time_interval(IntervalOpts),
     Hotp = pot:hotp(Secret, IntervalsNo),
-    Totp = pot:totp(Secret),
+    Totp = pot:totp(Secret, IntervalOpts),
     [?_assertEqual(Hotp, Totp)].
 
 generating_totp_for_given_timestamp_and_compare(_) ->
     Secret = <<"MFRGGZDFMZTWQ2LK">>,
     Totp = pot:totp(Secret, [{timestamp, {1518, 179058, 919315}}]),
     [?_assertEqual(Totp, <<"151469">>)].
-
-time_now() ->
-    {MegaSecs, Secs, _} = os:timestamp(),
-    MegaSecs * 1000000 + Secs.

test/totp_validity_tests.erl

@@ -3,22 +3,15 @@
 -include_lib("eunit/include/eunit.hrl").
 
 
-validating_totp_for_same_secret_test_() ->
-    {setup, fun start/0,
-            fun stop/1,
-            fun validating_totp_for_same_secret/1}.
-
-
-validating_invalid_totp_for_same_secret_test_() ->
-    {setup, fun start/0,
-            fun stop/1,
-            fun validating_invalid_totp_for_same_secret/1}.
-
-
-validating_correct_hotp_as_totp_test_() ->
-    {setup, fun start/0,
-            fun stop/1,
-            fun validating_correct_hotp_as_totp/1}.
+totp_validity_test_() ->
+    {foreach,
+     fun start/0,
+     fun stop/1,
+     [fun validating_totp_for_same_secret/1,
+      fun validating_invalid_totp_for_same_secret/1,
+      fun validating_correct_hotp_as_totp/1,
+      fun validating_past_future_totp_too_small_window/1,
+      fun validating_past_future_totp_with_window/1]}.
 
 
 start() ->
@@ -31,14 +24,34 @@ stop(_) ->
 
 validating_totp_for_same_secret(_) ->
     Secret = <<"MFRGGZDFMZTWQ2LK">>,
-    [?_assertEqual(pot:valid_totp(pot:totp(Secret), Secret), true)].
+    [?_assert(pot:valid_totp(pot:totp(Secret), Secret))].
 
 
 validating_invalid_totp_for_same_secret(_) ->
     Secret = <<"MFRGGZDFMZTWQ2LK">>,
-    [?_assertEqual(pot:valid_totp(<<"123456">>, Secret), false)].
+    [?_assertNot(pot:valid_totp(<<"123456">>, Secret))].
 
 
 validating_correct_hotp_as_totp(_) ->
     Secret = <<"MFRGGZDFMZTWQ2LK">>,
-    [?_assertEqual(pot:valid_totp(pot:hotp(Secret, 1), Secret), false)].
+    [?_assertNot(pot:valid_totp(pot:hotp(Secret, 1), Secret))].
+
+
+validating_past_future_totp_too_small_window(_) ->
+    Secret = <<"MFRGGZDFMZTWQ2LK">>,
+    IntervalOpts = [{timestamp, os:timestamp()}],
+    N = 5,
+    [?_assertNot(pot:valid_totp(pot:totp(Secret, [{addwindow, AW} | IntervalOpts]),
+                                Secret,
+                                [{window, W} | IntervalOpts]))
+     || W <- lists:seq(0, N), AW <- lists:seq(-N, N), W < abs(AW)].
+
+
+validating_past_future_totp_with_window(_) ->
+    Secret = <<"MFRGGZDFMZTWQ2LK">>,
+    IntervalOpts = [{timestamp, os:timestamp()}],
+    N = 5,
+    [?_assert(pot:valid_totp(pot:totp(Secret, [{addwindow, AW} | IntervalOpts]),
+                             Secret,
+                             [{window, abs(W)} | IntervalOpts]))
+     || W <- lists:seq(-N, N), AW <- lists:seq(-N, N), W >= abs(AW)].