From 3b76699c9c49f50268cc42ad7f0b5b9bba577663 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Mon, 16 Jan 2017 11:24:03 +0100 Subject: [PATCH 1/2] Increase 2fa backup codes entropy Signed-off-by: Christoph Wurst --- .../lib/Service/BackupCodeStorage.php | 11 ++++++++++- .../tests/Unit/Service/BackupCodeStorageTest.php | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/apps/twofactor_backupcodes/lib/Service/BackupCodeStorage.php b/apps/twofactor_backupcodes/lib/Service/BackupCodeStorage.php index ecb16305e92..487112c7c17 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, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789'); $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..43cca6fc602 100644 --- a/apps/twofactor_backupcodes/tests/Unit/Service/BackupCodeStorageTest.php +++ b/apps/twofactor_backupcodes/tests/Unit/Service/BackupCodeStorageTest.php @@ -77,7 +77,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') From 6726c98bbd7a6744bbf7d22a22224fe25b945ee1 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Mon, 16 Jan 2017 13:14:15 +0100 Subject: [PATCH 2/2] use constant and clean up unit test Signed-off-by: Christoph Wurst --- .../lib/Service/BackupCodeStorage.php | 2 +- .../Unit/Service/BackupCodeStorageTest.php | 35 +++++++++---------- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/apps/twofactor_backupcodes/lib/Service/BackupCodeStorage.php b/apps/twofactor_backupcodes/lib/Service/BackupCodeStorage.php index 487112c7c17..1cf62d18801 100644 --- a/apps/twofactor_backupcodes/lib/Service/BackupCodeStorage.php +++ b/apps/twofactor_backupcodes/lib/Service/BackupCodeStorage.php @@ -78,7 +78,7 @@ class BackupCodeStorage { $uid = $user->getUID(); foreach (range(1, min([$number, 20])) as $i) { - $code = $this->random->generate(self::$CODE_LENGTH, '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 43cca6fc602..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); @@ -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');