Merge pull request #3078 from nextcloud/2fa-backup-codes-entropy

Increase 2fa backup codes entropy

2 files changed, 28 insertions, 20 deletions

diff --git a/apps/twofactor_backupcodes/lib/Service/BackupCodeStorage.php b/apps/twofactor_backupcodes/lib/Service/BackupCodeStorage.php
index ecb16305e92..1cf62d18801 100644
--- a/apps/twofactor_backupcodes/lib/Service/BackupCodeStorage.php
+++ b/apps/twofactor_backupcodes/lib/Service/BackupCodeStorage.php
@@ -33,6 +33,8 @@ use OCP\Security\ISecureRandom;
 
 class BackupCodeStorage {
 
+	private static $CODE_LENGTH = 16;
+
 	/** @var BackupCodeMapper */
 	private $mapper;
 
@@ -48,6 +50,13 @@ class BackupCodeStorage {
 	/** @var ILogger */
 	private $logger;
 
+	/**
+	 * @param BackupCodeMapper $mapper
+	 * @param ISecureRandom $random
+	 * @param IHasher $hasher
+	 * @param IManager $activityManager
+	 * @param ILogger $logger
+	 */
 	public function __construct(BackupCodeMapper $mapper, ISecureRandom $random, IHasher $hasher,
 		IManager $activityManager, ILogger $logger) {
 		$this->mapper = $mapper;
@@ -69,7 +78,7 @@ class BackupCodeStorage {
 
 		$uid = $user->getUID();
 		foreach (range(1, min([$number, 20])) as $i) {
-			$code = $this->random->generate(10, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789');
+			$code = $this->random->generate(self::$CODE_LENGTH, ISecureRandom::CHAR_UPPER . ISecureRandom::CHAR_DIGITS);
 
 			$dbCode = new BackupCode();
 			$dbCode->setUserId($uid);
diff --git a/apps/twofactor_backupcodes/tests/Unit/Service/BackupCodeStorageTest.php b/apps/twofactor_backupcodes/tests/Unit/Service/BackupCodeStorageTest.php
index 54738f74600..109db7f688c 100644
--- a/apps/twofactor_backupcodes/tests/Unit/Service/BackupCodeStorageTest.php
+++ b/apps/twofactor_backupcodes/tests/Unit/Service/BackupCodeStorageTest.php
@@ -31,23 +31,24 @@ use OCP\ILogger;
 use OCP\IUser;
 use OCP\Security\IHasher;
 use OCP\Security\ISecureRandom;
+use PHPUnit_Framework_MockObject_MockObject;
 use Test\TestCase;
 
 class BackupCodeStorageTest extends TestCase {
 
-	/** @var BackupCodeMapper|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var BackupCodeMapper|PHPUnit_Framework_MockObject_MockObject */
 	private $mapper;
 
-	/** @var ISecureRandom|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var ISecureRandom|PHPUnit_Framework_MockObject_MockObject */
 	private $random;
 
-	/** @var IHasher|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var IHasher|PHPUnit_Framework_MockObject_MockObject */
 	private $hasher;
 
-	/** @var IManager|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var IManager|PHPUnit_Framework_MockObject_MockObject */
 	private $activityManager;
 
-	/** @var ILogger|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var ILogger|PHPUnit_Framework_MockObject_MockObject */
 	private $logger;
 
 	/** @var BackupCodeStorage */
@@ -56,11 +57,9 @@ class BackupCodeStorageTest extends TestCase {
 	protected function setUp() {
 		parent::setUp();
 
-		$this->mapper = $this->getMockBuilder(BackupCodeMapper::class)
-			->disableOriginalConstructor()
-			->getMock();
-		$this->random = $this->getMockBuilder(ISecureRandom::class)->getMock();
-		$this->hasher = $this->getMockBuilder(IHasher::class)->getMock();
+		$this->mapper = $this->createMock(BackupCodeMapper::class);
+		$this->random = $this->createMock(ISecureRandom::class);
+		$this->hasher = $this->createMock(IHasher::class);
 		$this->activityManager = $this->createMock(IManager::class);
 		$this->logger = $this->createMock(ILogger::class);
 
@@ -68,7 +67,7 @@ class BackupCodeStorageTest extends TestCase {
 	}
 
 	public function testCreateCodes() {
-		$user = $this->getMockBuilder(IUser::class)->getMock();
+		$user = $this->createMock(IUser::class);
 		$number = 5;
 		$event = $this->createMock(IEvent::class);
 
@@ -77,7 +76,7 @@ class BackupCodeStorageTest extends TestCase {
 			->will($this->returnValue('fritz'));
 		$this->random->expects($this->exactly($number))
 			->method('generate')
-			->with(10, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789')
+			->with(16, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789')
 			->will($this->returnValue('CODEABCDEF'));
 		$this->hasher->expects($this->exactly($number))
 			->method('hash')
@@ -121,7 +120,7 @@ class BackupCodeStorageTest extends TestCase {
 	}
 
 	public function testHasBackupCodes() {
-		$user = $this->getMockBuilder(IUser::class)->getMock();
+		$user = $this->createMock(IUser::class);
 		$codes = [
 			new BackupCode(),
 			new BackupCode(),
@@ -136,7 +135,7 @@ class BackupCodeStorageTest extends TestCase {
 	}
 
 	public function testHasBackupCodesNoCodes() {
-		$user = $this->getMockBuilder(IUser::class)->getMock();
+		$user = $this->createMock(IUser::class);
 		$codes = [];
 
 		$this->mapper->expects($this->once())
@@ -148,7 +147,7 @@ class BackupCodeStorageTest extends TestCase {
 	}
 
 	public function testGetBackupCodeState() {
-		$user = $this->getMockBuilder(IUser::class)->getMock();
+		$user = $this->createMock(IUser::class);
 
 		$code1 = new BackupCode();
 		$code1->setUsed(1);
@@ -173,7 +172,7 @@ class BackupCodeStorageTest extends TestCase {
 	}
 
 	public function testGetBackupCodeDisabled() {
-		$user = $this->getMockBuilder(IUser::class)->getMock();
+		$user = $this->createMock(IUser::class);
 
 		$codes = [];
 
@@ -191,7 +190,7 @@ class BackupCodeStorageTest extends TestCase {
 	}
 
 	public function testValidateCode() {
-		$user = $this->getMockBuilder(IUser::class)->getMock();
+		$user = $this->createMock(IUser::class);
 		$code = new BackupCode();
 		$code->setUsed(0);
 		$code->setCode('HASHEDVALUE');
@@ -217,7 +216,7 @@ class BackupCodeStorageTest extends TestCase {
 	}
 
 	public function testValidateUsedCode() {
-		$user = $this->getMockBuilder(IUser::class)->getMock();
+		$user = $this->createMock(IUser::class);
 		$code = new BackupCode();
 		$code->setUsed('1');
 		$code->setCode('HASHEDVALUE');
@@ -238,7 +237,7 @@ class BackupCodeStorageTest extends TestCase {
 	}
 
 	public function testValidateCodeWithWrongHash() {
-		$user = $this->getMockBuilder(IUser::class)->getMock();
+		$user = $this->createMock(IUser::class);
 		$code = new BackupCode();
 		$code->setUsed(0);
 		$code->setCode('HASHEDVALUE');