do not allow multiple uses of the same key

owncloud · Sep 25, 2018 · 6e83277 · 6e83277
1 parent 8fd672f
commit 6e83277
Show file tree

Hide file tree

Showing 5 changed files with 169 additions and 7 deletions.
diff --git a/appinfo/database.xml b/appinfo/database.xml
@@ -27,6 +27,11 @@
                 <type>clob</type>
                 <notnull>true</notnull>
             </field>
+            <field>
+                <name>last_validated_key</name>
+                <type>text</type>
+                <length>32</length>
+            </field>
             <field>
                 <name>verified</name>
                 <type>boolean</type>

diff --git a/lib/Db/TotpSecret.php b/lib/Db/TotpSecret.php
@@ -2,6 +2,7 @@
 
 /**
  * @author Christoph Wurst <christoph@winzerhof-wurst.at>
+ * @author Semih Serhat Karakaya <karakayasemi@itu.edu.tr>
  *
  * Two-factor TOTP
  *
@@ -26,7 +27,6 @@
 /**
  * @method string getUserId()
  * @method void setUserId(string $userId)
- * @method string getSecret()
  * @method void setSecret(string $secret)
  * @method boolean getVerified()
  * @method void setVerified(bool $verified)
@@ -39,7 +39,31 @@ class TotpSecret extends Entity {
     /** @var string */
     protected $secret;
 
+    /** @var string */
+    protected $lastValidatedKey;
+
     /** @var boolean */
     protected $verified;
 
+    /**
+     * @return string
+     */
+    public function getSecret() {
+        return $this->secret;
+    }
+
+    /**
+     * @return string
+     */
+    public function getLastValidatedKey() {
+        return $this->lastValidatedKey;
+    }
+
+    /**
+     * @param string $key
+     */
+    public function setLastValidatedKey($key) {
+        $this->lastValidatedKey = $key;
+    }
+
 }
diff --git a/lib/Db/TotpSecretMapper.php b/lib/Db/TotpSecretMapper.php
@@ -43,7 +43,7 @@ public function getSecret(IUser $user) {
         /* @var $qb IQueryBuilder */
         $qb = $this->db->getQueryBuilder();
 
-        $qb->select('id', 'user_id', 'secret', 'verified')
+        $qb->select('id', 'user_id', 'secret', 'verified', 'last_validated_key')
                 ->from('twofactor_totp_secrets')
                 ->where($qb->expr()->eq('user_id', $qb->createNamedParameter($user->getUID())));
         $result = $qb->execute();

diff --git a/lib/Service/Totp.php b/lib/Service/Totp.php
@@ -40,9 +40,13 @@ class Totp implements ITotp {
     /** @var ICrypto */
     private $crypto;
 
-    public function __construct(TotpSecretMapper $secretMapper, ICrypto $crypto) {
+    /** @var Otp */
+    private  $otp;
+
+    public function __construct(TotpSecretMapper $secretMapper, ICrypto $crypto, Otp $otp) {
         $this->secretMapper = $secretMapper;
         $this->crypto = $crypto;
+        $this->otp = $otp;
     }
 
     public function hasSecret(IUser $user) {
@@ -109,10 +113,18 @@ public function validateSecret(IUser $user, $key) {
             throw new NoTotpSecretFoundException();
         }
 
-        $secret = $this->crypto->decrypt($dbSecret->getSecret());
-
-        $otp = new Otp();
-        return $otp->checkTotp(Base32::decode($secret), $key, 3);
+        /**
+         * Check last validated key to prevent consecutive usage of the same key
+         */
+        if ($dbSecret->getLastValidatedKey() !== $key) {
+            $secret = $this->crypto->decrypt($dbSecret->getSecret());
+            if ($this->otp->checkTotp(Base32::decode($secret), $key, 3) === true) {
+                $dbSecret->setLastValidatedKey($key);
+                $this->secretMapper->update($dbSecret);
+                return true;
+            }
+        }
+        return false;
     }
 
     /**

diff --git a/tests/unit/Service/TotpTest.php b/tests/unit/Service/TotpTest.php
@@ -0,0 +1,121 @@
+<?php
+/**
+ * @author Semih Serhat Karakaya <karakayasemi@itu.edu.tr>
+ *
+ * Two-factor TOTP
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace OCA\TwoFactor_Totp\Tests\Service;
+
+use OCA\TwoFactor_Totp\Db\TotpSecret;
+use OCA\TwoFactor_Totp\Db\TotpSecretMapper;
+use OCA\TwoFactor_Totp\Exception\NoTotpSecretFoundException;
+use OCA\TwoFactor_Totp\Service\Totp;
+use OCP\AppFramework\Db\DoesNotExistException;
+use OCP\IUser;
+use OCP\Security\ICrypto;
+use Otp\Otp;
+use Test\TestCase;
+
+/**
+ * Class TotpTest
+ */
+class TotpTest extends TestCase {
+
+	/** @var TotpSecretMapper | \PHPUnit_Framework_MockObject_MockObject */
+	private $secretMapper;
+
+	/** @var ICrypto | \PHPUnit_Framework_MockObject_MockObject */
+	private $crypto;
+
+	/** @var Otp | \PHPUnit_Framework_MockObject_MockObject  */
+	private  $otp;
+
+	/** @var Totp */
+	private  $totp;
+
+	protected function setUp() {
+		parent::setUp();
+
+		$this->secretMapper = $this->createMock(TotpSecretMapper::class);
+		$this->crypto = $this->createMock(ICrypto::class);
+		$this->otp = $this->createMock(Otp::class);
+
+		$this->totp = new Totp($this->secretMapper, $this->crypto, $this->otp);
+
+	}
+
+	/**
+	 * @dataProvider validationProvider
+	 * @param string $lastKey
+	 * @param string $key
+	 * @param boolean $validationResult
+	 * @param boolean $expectedResult
+	 */
+	public function testValidateSecret($lastKey, $key, $validationResult, $expectedResult) {
+		/** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user  */
+		$user = $this->createMock(IUser::class);
+		$dbSecret = $this->createMock(TotpSecret::class);
+
+		$dbSecret->expects($this->once())
+			->method('getLastValidatedKey')
+			->will($this->returnValue($lastKey));
+		$this->secretMapper->expects($this->once())
+			->method('getSecret')
+			->with($user)
+			->will($this->returnValue($dbSecret));
+
+		if($lastKey !== $key) {
+			$dbSecret->expects($this->once())
+				->method('getSecret')
+				->will($this->returnValue('secret'));
+			$this->otp->expects($this->once())
+				->method('checkTotp')
+				->will($this->returnValue($validationResult));
+		}
+		if($expectedResult === true) {
+			$dbSecret->expects($this->once())
+				->method('setLastValidatedKey')
+				->with($key);
+			$this->secretMapper->expects($this->once())
+				->method('update')
+				->with($dbSecret);
+		}
+		$this->assertEquals($this->totp->validateSecret($user, $key), $expectedResult);
+	}
+
+	public function validationProvider() {
+		return [
+			[ '000000', '000000', true, false ],
+			[ '000000', '000000', false, false ],
+			[ '000000', '000001', false, false ],
+			[ '000000', '000001', true, true ]
+		];
+	}
+
+	public function testValidateSecretNoSecret() {
+		/** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user  */
+		$user = $this->createMock(IUser::class);
+
+		$this->secretMapper->expects($this->once())
+			->method('getSecret')
+			->with($user)
+			->will($this->throwException(new DoesNotExistException('')));
+		$this->expectException(NoTotpSecretFoundException::class);
+		$this->totp->validateSecret($user, 'testkey');
+	}
+
+}