Repository: Spomky-Labs/otphp (php)

Issue:
Why is label required for the constructor?
When only verifying a TOTP code, it doesn't look like label is needed anywhere, yet it is a required constructor parameter for the TOTP class.

This means passing in a blank string or random junk for the sake of making it happy.

Can this be changed to make it neater? Otherwise a simple static function could be use that eliminates the need to construct a TOTP object just to call verify.


Discussion:
Hi @lukos,

You are right: the label is only needed when the provisioning URI is computed.
Depending on the schemen (TOTP/HOTP), you just need the secret, the digest, the digits and additional parameters required by the scheme (counter, interval).

I will not add a static method, from my POV it should be easier to modify the [setLabel](https://github.com/Spomky-Labs/otphp/blob/master/src/ParameterTrait.php#L85) method by allowing a null label and verify that label in the [generateURI](https://github.com/Spomky-Labs/otphp/blob/master/src/OTP.php#L97) one.

WDYT?

Patch:
diff --git a/src/Factory.php b/src/Factory.php
--- a/src/Factory.php
+++ b/src/Factory.php
@@ -20,7 +20,7 @@ final class Factory
      *
      * @throws \InvalidArgumentException
      *
-     * @return \OTPHP\TOTP|\OTPHP\HOTP
+     * @return \OTPHP\TOTPInterface|\OTPHP\HOTPInterface
      */
     public static function loadFromProvisioningUri($uri)
     {
diff --git a/src/HOTP.php b/src/HOTP.php
--- a/src/HOTP.php
+++ b/src/HOTP.php
@@ -18,13 +18,13 @@ final class HOTP extends OTP implements HOTPInterface
     /**
      * HOTP constructor.
      *
-     * @param string      $label
+     * @param string|null $label
      * @param string|null $secret
      * @param int         $counter
      * @param string      $digest
      * @param int         $digits
      */
-    public function __construct($label, $secret = null, $counter = 0, $digest = 'sha1', $digits = 6)
+    public function __construct($label = null, $secret = null, $counter = 0, $digest = 'sha1', $digits = 6)
     {
         parent::__construct($label, $secret, $digest, $digits);
         $this->setCounter($counter);
diff --git a/src/OTP.php b/src/OTP.php
--- a/src/OTP.php
+++ b/src/OTP.php
@@ -11,6 +11,7 @@
 
 namespace OTPHP;
 
+use Assert\Assertion;
 use Base32\Base32;
 
 abstract class OTP implements OTPInterface
@@ -20,7 +21,7 @@ abstract class OTP implements OTPInterface
     /**
      * OTP constructor.
      *
-     * @param string      $label
+     * @param string|null $label
      * @param string|null $secret
      * @param string      $digest
      * @param int         $digits
@@ -56,11 +57,7 @@ protected function generateOTP($input)
             $hmac[] = hexdec($hex);
         }
         $offset = $hmac[count($hmac) - 1] & 0xF;
-        $code = ($hmac[$offset + 0] & 0x7F) << 24 |
-            ($hmac[$offset + 1] & 0xFF) << 16 |
-            ($hmac[$offset + 2] & 0xFF) << 8 |
-            ($hmac[$offset + 3] & 0xFF);
-
+        $code = ($hmac[$offset + 0] & 0x7F) << 24 | ($hmac[$offset + 1] & 0xFF) << 16 | ($hmac[$offset + 2] & 0xFF) << 8 | ($hmac[$offset + 3] & 0xFF);
         $otp = $code % pow(10, $this->getDigits());
 
         return str_pad((string) $otp, $this->getDigits(), '0', STR_PAD_LEFT);
@@ -96,22 +93,14 @@ protected function filterOptions(array &$options)
      */
     protected function generateURI($type, array $options)
     {
+        $label = $this->getLabel();
+        Assertion::string($label, 'The label is not set.');
+        Assertion::false($this->hasColon($label), 'Label must not contain a colon.');
         $options = array_merge($options, $this->getParameters());
-
         $this->filterOptions($options);
+        $params = str_replace(['+', '%7E'], ['%20', '~'], http_build_query($options));
 
-        $params = str_replace(
-            ['+', '%7E'],
-            ['%20', '~'],
-            http_build_query($options)
-        );
-
-        return sprintf(
-            'otpauth://%s/%s?%s',
-            $type,
-            rawurlencode((null !== $this->getIssuer() ? $this->getIssuer().':' : '').$this->getLabel()),
-            $params
-        );
+        return sprintf('otpauth://%s/%s?%s', $type, rawurlencode((null !== $this->getIssuer() ? $this->getIssuer().':' : '').$label), $params);
     }
 
     /**
diff --git a/src/ParameterTrait.php b/src/ParameterTrait.php
--- a/src/ParameterTrait.php
+++ b/src/ParameterTrait.php
@@ -80,12 +80,11 @@ public function getLabel()
     }
 
     /**
-     * @param string $label
+     * @param string|null $label
      */
     private function setLabel($label)
     {
-        Assertion::string($label, 'Label must be a string.');
-        Assertion::false($this->hasColon($label), 'Label must not contain a colon.');
+        Assertion::nullOrString($label, 'Label must be null or a string.');
 
         $this->label = $label;
     }
@@ -165,6 +164,8 @@ private function setDigest($digest)
 
     /**
      * @param string $parameter
+     *
+     * @return bool
      */
     public function hasParameter($parameter)
     {
diff --git a/src/TOTP.php b/src/TOTP.php
--- a/src/TOTP.php
+++ b/src/TOTP.php
@@ -18,13 +18,13 @@ final class TOTP extends OTP implements TOTPInterface
     /**
      * TOTP constructor.
      *
-     * @param string      $label
+     * @param string|null $label
      * @param string|null $secret
      * @param int         $period
      * @param string      $digest
      * @param int         $digits
      */
-    public function __construct($label, $secret = null, $period = 30, $digest = 'sha1', $digits = 6)
+    public function __construct($label = null, $secret = null, $period = 30, $digest = 'sha1', $digits = 6)
     {
         parent::__construct($label, $secret, $digest, $digits);
         $this->setPeriod($period);


Test patch:
diff --git a/tests/HOTPTest.php b/tests/HOTPTest.php
--- a/tests/HOTPTest.php
+++ b/tests/HOTPTest.php
@@ -15,11 +15,22 @@ class HOTPTest extends \PHPUnit_Framework_TestCase
 {
     /**
      * @expectedException \InvalidArgumentException
-     * @expectedExceptionMessage Label must be a string.
+     * @expectedExceptionMessage Label must be null or a string.
+     */
+    public function testLabelNotNullAndNotAStringDefined()
+    {
+        new HOTP(1234, 'JDDK4U6G3BJLEZ7Y', 0, 'sha512', 8);
+    }
+
+    /**
+     * @expectedException \InvalidArgumentException
+     * @expectedExceptionMessage The label is not set.
      */
     public function testLabelNotDefined()
     {
-        new HOTP(null, 'JDDK4U6G3BJLEZ7Y', 0, 'sha512', 8);
+        $hotp = new HOTP();
+        $this->assertTrue(is_string($hotp->at(0)));
+        $hotp->getProvisioningUri();
     }
 
     /**
@@ -48,7 +59,8 @@ public function testIssuerHasColon2()
      */
     public function testLabelHasColon()
     {
-        new HOTP('foo%3Abar', 'JDDK4U6G3BJLEZ7Y', 0, 'sha512', 8);
+        $hotp = new HOTP('foo%3Abar', 'JDDK4U6G3BJLEZ7Y', 0, 'sha512', 8);
+        $hotp->getProvisioningUri();
     }
 
     /**
@@ -57,7 +69,8 @@ public function testLabelHasColon()
      */
     public function testLabelHasColon2()
     {
-        new HOTP('foo:bar', 'JDDK4U6G3BJLEZ7Y', 0, 'sha512', 8);
+        $hotp = new HOTP('foo:bar', 'JDDK4U6G3BJLEZ7Y', 0, 'sha512', 8);
+        $hotp->getProvisioningUri();
     }
 
     /**
diff --git a/tests/TOTPTest.php b/tests/TOTPTest.php
--- a/tests/TOTPTest.php
+++ b/tests/TOTPTest.php
@@ -14,6 +14,17 @@
 
 class TOTPTest extends \PHPUnit_Framework_TestCase
 {
+    /**
+     * @expectedException \InvalidArgumentException
+     * @expectedExceptionMessage The label is not set.
+     */
+    public function testLabelNotDefined()
+    {
+        $hotp = new TOTP();
+        $this->assertTrue(is_string($hotp->now()));
+        $hotp->getProvisioningUri();
+    }
+
     public function testCustomParameter()
     {
         $otp = new TOTP('alice@foo.bar', 'JDDK4U6G3BJLEZ7Y', 20, 'sha512', 8);