Updated according to code review feedback.

5 files changed, 67 insertions, 21 deletions

diff --git a/cryptography/exceptions.py b/cryptography/exceptions.py
index e2542a1f..f9849e2f 100644
--- a/cryptography/exceptions.py
+++ b/cryptography/exceptions.py
@@ -42,3 +42,7 @@ class InternalError(Exception):
 
 class InvalidKey(Exception):
     pass
+
+
+class InvalidToken(Exception):
+    pass
diff --git a/cryptography/hazmat/oath/hotp.py b/cryptography/hazmat/oath/hotp.py
index a04d0d49..a1f62746 100644
--- a/cryptography/hazmat/oath/hotp.py
+++ b/cryptography/hazmat/oath/hotp.py
@@ -12,28 +12,37 @@
 # limitations under the License.
 
 import struct
+from cryptography.exceptions import InvalidToken
 import six
 
-from cryptography.hazmat.primitives import constant_time
+from cryptography.hazmat.primitives import constant_time, hmac
 from cryptography.hazmat.primitives.hashes import SHA1
 
 
 class HOTP(object):
-    def __init__(self, secret, length, backend):
-        self.secret = secret
-        self.length = length
-        self.backend = backend
+    def __init__(self, key, length, backend):
+
+        if len(key) < 16:
+            raise ValueError("Key length has to be at least 128 bits.")
+
+        if length < 6:
+            raise ValueError("Length of HOTP has to be at least 6.")
+
+        self._key = key
+        self._length = length
+        self._backend = backend
 
     def generate(self, counter):
-        sbit = self._dynamic_truncate(counter)
-        foo = sbit % (10**self.length)
-        return ('%s' % foo).zfill(self.length).encode()
+        truncated_value = self._dynamic_truncate(counter)
+        hotp = truncated_value % (10**self._length)
+        return "{0:0{1}}".format(hotp, self._length).encode()
 
     def verify(self, hotp, counter):
-        return constant_time.bytes_eq(self.generate(counter), hotp)
+        if not constant_time.bytes_eq(self.generate(counter), hotp):
+            raise InvalidToken("Supplied HOTP value does not match")
 
     def _dynamic_truncate(self, counter):
-        ctx = self.backend.create_hmac_ctx(self.secret, SHA1)
+        ctx = hmac.HMAC(self._key, SHA1(), self._backend)
         ctx.update(struct.pack(">Q", counter))
         hmac_value = ctx.finalize()
 
diff --git a/docs/hazmat/oath/hotp.rst b/docs/hazmat/oath/hotp.rst
index 614933f9..1dee26b0 100644
--- a/docs/hazmat/oath/hotp.rst
+++ b/docs/hazmat/oath/hotp.rst
@@ -17,18 +17,20 @@ values based on Hash-based message authentication codes (HMAC).
 
     This is an implementation of :rfc:`4226`.
 
-    .. code-block:: python
+    .. doctest::
 
+        >>> import os
         >>> from cryptography.hazmat.backends import default_backend
         >>> from cryptography.hazmat.oath.hotp import HOTP
-        >>> hotp = HOTP(secret, 6, backend=default_backend)
+
+        >>> key = "12345678901234567890"
+        >>> hotp = HOTP(key, 6, backend=default_backend())
         >>> hotp.generate(0)
-        958695
-        >>> hotp.verify("958695", 0)
-        True
+        '755224'
+        >>> hotp.verify("755224", 0)
 
-    :param secret: Secret key as ``bytes``.
-    :param length: Length of generated one time password as ``int``.
+    :param bytes secret: Secret key as ``bytes``.
+    :param int length: Length of generated one time password as ``int``.
     :param backend: A
         :class:`~cryptography.hazmat.backends.interfaces.HMACBackend`
         provider.
@@ -36,7 +38,7 @@ values based on Hash-based message authentication codes (HMAC).
     .. method:: generate(counter)
 
         :param int counter: The counter value used to generate the one time password.
-        :return: A one time password value.
+        :return bytes: A one time password value.
 
     .. method:: verify(hotp, counter)
 
diff --git a/pytest.ini b/pytest.ini
index 77360d16..3f65e30e 100644
--- a/pytest.ini
+++ b/pytest.ini
@@ -7,4 +7,3 @@ markers =
     pbkdf2hmac: this test requires a backend providing PBKDF2HMACBackend
     rsa: this test requires a backend providing RSABackend
     supported: parametrized test requiring only_if and skip_message
-    oath: this test requires a backend providing HMACBackend
diff --git a/tests/hazmat/oath/test_hotp.py b/tests/hazmat/oath/test_hotp.py
index cd06c79f..8a5aebd3 100644
--- a/tests/hazmat/oath/test_hotp.py
+++ b/tests/hazmat/oath/test_hotp.py
@@ -10,18 +10,41 @@
 # implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+from cryptography.exceptions import InvalidToken
+
+import os
 
 import pytest
+
 from cryptography.hazmat.oath.hotp import HOTP
+from cryptography.hazmat.primitives import hashes
 from tests.utils import load_vectors_from_file, load_nist_vectors
 
 vectors = load_vectors_from_file(
     "oath/rfc-4226.txt", load_nist_vectors)
 
 
-@pytest.mark.oath
+@pytest.mark.supported(
+    only_if=lambda backend: backend.hmac_supported(hashes.SHA1()),
+    skip_message="Does not support HMAC-SHA1."
+)
+@pytest.mark.hmac
 class TestHOTP(object):
 
+    def test_invalid_key_length(self, backend):
+        secret = os.urandom(10)
+
+        with pytest.raises(ValueError):
+            hotp = HOTP(secret, 6, backend)
+            hotp.generate(0)
+
+    def test_invalid_hotp_length(self, backend):
+        secret = os.urandom(16)
+
+        with pytest.raises(ValueError):
+            hotp = HOTP(secret, 4, backend)
+            hotp.generate(0)
+
     @pytest.mark.parametrize("params", vectors)
     def test_truncate(self, backend, params):
         secret = params["secret"]
@@ -50,4 +73,13 @@ class TestHOTP(object):
 
         hotp = HOTP(secret, 6, backend)
 
-        assert hotp.verify(hotp_value, counter) is True
+        assert hotp.verify(hotp_value, counter) is None
+
+    def test_invalid_verify(self, backend):
+        secret = b"12345678901234567890"
+        counter = 0
+
+        hotp = HOTP(secret, 6, backend)
+
+        with pytest.raises(InvalidToken):
+            hotp.verify(b"123456", counter)