From fba5f9794fc42b19039bdbd1b108b1bb74984145 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20B=C3=B6hmer?= Date: Fri, 27 Dec 2019 15:20:06 +0100 Subject: [PATCH] Added an service for generating Backup codes and added some tests. --- config/services.yaml | 5 ++ src/Entity/UserSystem/User.php | 33 +++++-- src/Services/TFA/BackupCodeGenerator.php | 60 +++++++++++++ .../AbstractAdminControllerTest.php | 1 + .../AttachmentTypeControllerTest.php | 1 + .../AdminPages/CategoryControllerTest.php | 1 + .../AdminPages/DeviceControllerTest.php | 1 + .../AdminPages/FootprintControllerTest.php | 1 + .../AdminPages/ManufacturerControllerTest.php | 1 + .../MeasurementUnitControllerTest.php | 1 + .../StorelocationControllerTest.php | 1 + .../AdminPages/SupplierControllerTest.php | 1 + tests/Controller/RedirectControllerTest.php | 1 + tests/Entity/UserSystem/UserTest.php | 85 +++++++++++++++++++ tests/Services/EntityImporterTest.php | 3 + .../Services/TFA/BackupCodeGeneratorTest.php | 56 ++++++++++++ 16 files changed, 245 insertions(+), 7 deletions(-) create mode 100644 src/Services/TFA/BackupCodeGenerator.php create mode 100644 tests/Services/TFA/BackupCodeGeneratorTest.php diff --git a/config/services.yaml b/config/services.yaml index 3a4e69cd..7fa8ebf3 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -120,6 +120,11 @@ services: arguments: $mimeTypes: '@mime_types' + App\Services\TFA\BackupCodeGenerator: + arguments: + $code_length: 8 + $code_count: 10 + App\Services\TranslationExtractor\PermissionExtractor: tags: - { name: 'translation.extractor', alias: 'permissionExtractor'} \ No newline at end of file diff --git a/src/Entity/UserSystem/User.php b/src/Entity/UserSystem/User.php index ac935920..a03aa85a 100644 --- a/src/Entity/UserSystem/User.php +++ b/src/Entity/UserSystem/User.php @@ -191,7 +191,7 @@ class User extends AttachmentContainingDBElement implements UserInterface, HasPe * @var string[]|null A list of backup codes that can be used, if the user has no access to its Google Authenticator device * @ORM\Column(type="json") */ - protected $backupCodes; + protected $backupCodes = []; /** @var \DateTime The time when the backup codes were generated * @ORM\Column(type="datetime", nullable=true) @@ -201,7 +201,7 @@ class User extends AttachmentContainingDBElement implements UserInterface, HasPe /** @var int The version of the trusted device cookie. Used to invalidate all trusted device cookies at once. * @ORM\Column(type="integer") */ - protected $trustedDeviceCookieVersion; + protected $trustedDeviceCookieVersion = 0; /** @var Collection * @ORM\OneToMany(targetEntity="App\Entity\UserSystem\U2FKey", mappedBy="user") @@ -775,15 +775,19 @@ class User extends AttachmentContainingDBElement implements UserInterface, HasPe public function setBackupCodes(array $codes) : self { $this->backupCodes = $codes; - $this->backupCodesGenerationDate = new \DateTime(); + if(empty($codes)) { + $this->backupCodesGenerationDate = null; + } else { + $this->backupCodesGenerationDate = new \DateTime(); + } return $this; } /** * Return the date when the backup codes were generated. - * @return \DateTime + * @return \DateTime|null */ - public function getBackupCodesGenerationDate() : \DateTime + public function getBackupCodesGenerationDate() : ?\DateTime { return $this->backupCodesGenerationDate; } @@ -806,24 +810,39 @@ class User extends AttachmentContainingDBElement implements UserInterface, HasPe $this->trustedDeviceCookieVersion++; } + /** + * Check if U2F is enabled + * @return bool + */ public function isU2FAuthEnabled(): bool { return count($this->u2fKeys) > 0; } - /** @return Collection */ + /** + * Get all U2F Keys that are associated with this user + * @return Collection + */ public function getU2FKeys(): Collection { return $this->u2fKeys; } + /** + * Add a U2F key to this user. + * @param TwoFactorKeyInterface $key + */ public function addU2FKey(TwoFactorKeyInterface $key): void { $this->u2fKeys->add($key); } + /** + * Remove a U2F key from this user. + * @param TwoFactorKeyInterface $key + */ public function removeU2FKey(TwoFactorKeyInterface $key): void { - $this->u2fKeys->remove($key); + $this->u2fKeys->removeElement($key); } } diff --git a/src/Services/TFA/BackupCodeGenerator.php b/src/Services/TFA/BackupCodeGenerator.php new file mode 100644 index 00000000..a180dae0 --- /dev/null +++ b/src/Services/TFA/BackupCodeGenerator.php @@ -0,0 +1,60 @@ + 32) { + throw new \RuntimeException('Backup code can have maximum 32 digits!'); + } + if ($code_length < 6) { + throw new \RuntimeException('Code must have at least 6 digits to ensure security!'); + } + + $this->code_count = $code_count; + $this->code_length = $code_length; + } + + /** + * Generates a single backup code. + * It is a random hexadecimal value with the digit count configured in constructor + * @return string The generated backup code (e.g. 1f3870be2) + * @throws \Exception If no entropy source is available. + */ + public function generateSingleCode() : string + { + $bytes = random_bytes(32); + return substr(md5($bytes), 0, $this->code_length); + } + + + /** + * Returns a full backup code set. The code count can be configured in the constructor + * @return string[] An array containing different backup codes. + * @throws \Exception If no entropy source is available + */ + public function generateCodeSet() : array + { + $array = []; + for($n=0; $n<$this->code_count; $n++) { + $array[] = $this->generateSingleCode(); + } + + return $array; + } +} \ No newline at end of file diff --git a/tests/Controller/AdminPages/AbstractAdminControllerTest.php b/tests/Controller/AdminPages/AbstractAdminControllerTest.php index 4a191b59..a9da3b73 100644 --- a/tests/Controller/AdminPages/AbstractAdminControllerTest.php +++ b/tests/Controller/AdminPages/AbstractAdminControllerTest.php @@ -26,6 +26,7 @@ use Symfony\Component\Security\Core\Exception\AccessDeniedException; /** * @group slow + * @group DB */ abstract class AbstractAdminControllerTest extends WebTestCase { diff --git a/tests/Controller/AdminPages/AttachmentTypeControllerTest.php b/tests/Controller/AdminPages/AttachmentTypeControllerTest.php index d7cfd48c..952211a6 100644 --- a/tests/Controller/AdminPages/AttachmentTypeControllerTest.php +++ b/tests/Controller/AdminPages/AttachmentTypeControllerTest.php @@ -25,6 +25,7 @@ use App\Entity\Attachments\AttachmentType; /** * @group slow + * @group DB */ class AttachmentTypeControllerTest extends AbstractAdminControllerTest { diff --git a/tests/Controller/AdminPages/CategoryControllerTest.php b/tests/Controller/AdminPages/CategoryControllerTest.php index 48a5cff0..12460519 100644 --- a/tests/Controller/AdminPages/CategoryControllerTest.php +++ b/tests/Controller/AdminPages/CategoryControllerTest.php @@ -25,6 +25,7 @@ use App\Entity\Parts\Category; /** * @group slow + * @group DB */ class CategoryControllerTest extends AbstractAdminControllerTest { diff --git a/tests/Controller/AdminPages/DeviceControllerTest.php b/tests/Controller/AdminPages/DeviceControllerTest.php index a608ace9..d7fa1ab0 100644 --- a/tests/Controller/AdminPages/DeviceControllerTest.php +++ b/tests/Controller/AdminPages/DeviceControllerTest.php @@ -25,6 +25,7 @@ use App\Entity\Devices\Device; /** * @group slow + * @group DB */ class DeviceControllerTest extends AbstractAdminControllerTest { diff --git a/tests/Controller/AdminPages/FootprintControllerTest.php b/tests/Controller/AdminPages/FootprintControllerTest.php index 14e8ecb1..10c3bbc2 100644 --- a/tests/Controller/AdminPages/FootprintControllerTest.php +++ b/tests/Controller/AdminPages/FootprintControllerTest.php @@ -25,6 +25,7 @@ use App\Entity\Parts\Footprint; /** * @group slow + * @group DB */ class FootprintControllerTest extends AbstractAdminControllerTest { diff --git a/tests/Controller/AdminPages/ManufacturerControllerTest.php b/tests/Controller/AdminPages/ManufacturerControllerTest.php index 134ef8a5..7c9151a3 100644 --- a/tests/Controller/AdminPages/ManufacturerControllerTest.php +++ b/tests/Controller/AdminPages/ManufacturerControllerTest.php @@ -25,6 +25,7 @@ use App\Entity\Parts\Manufacturer; /** * @group slow + * @group DB */ class ManufacturerControllerTest extends AbstractAdminControllerTest { diff --git a/tests/Controller/AdminPages/MeasurementUnitControllerTest.php b/tests/Controller/AdminPages/MeasurementUnitControllerTest.php index ac576409..090ca2f3 100644 --- a/tests/Controller/AdminPages/MeasurementUnitControllerTest.php +++ b/tests/Controller/AdminPages/MeasurementUnitControllerTest.php @@ -25,6 +25,7 @@ use App\Entity\Parts\MeasurementUnit; /** * @group slow + * @group DB */ class MeasurementUnitControllerTest extends AbstractAdminControllerTest { diff --git a/tests/Controller/AdminPages/StorelocationControllerTest.php b/tests/Controller/AdminPages/StorelocationControllerTest.php index b92f5bf9..e1bd0e9f 100644 --- a/tests/Controller/AdminPages/StorelocationControllerTest.php +++ b/tests/Controller/AdminPages/StorelocationControllerTest.php @@ -25,6 +25,7 @@ use App\Entity\Parts\Storelocation; /** * @group slow + * @group DB */ class StorelocationControllerTest extends AbstractAdminControllerTest { diff --git a/tests/Controller/AdminPages/SupplierControllerTest.php b/tests/Controller/AdminPages/SupplierControllerTest.php index a085bfae..31559c42 100644 --- a/tests/Controller/AdminPages/SupplierControllerTest.php +++ b/tests/Controller/AdminPages/SupplierControllerTest.php @@ -25,6 +25,7 @@ use App\Entity\Parts\Supplier; /** * @group slow + * @group DB */ class SupplierControllerTest extends AbstractAdminControllerTest { diff --git a/tests/Controller/RedirectControllerTest.php b/tests/Controller/RedirectControllerTest.php index 58bbb2c9..098ee1fb 100644 --- a/tests/Controller/RedirectControllerTest.php +++ b/tests/Controller/RedirectControllerTest.php @@ -27,6 +27,7 @@ use Symfony\Bundle\FrameworkBundle\Test\WebTestCase; /** * @group slow + * @group DB */ class RedirectControllerTest extends WebTestCase { diff --git a/tests/Entity/UserSystem/UserTest.php b/tests/Entity/UserSystem/UserTest.php index 0ed8fdf2..f201348f 100644 --- a/tests/Entity/UserSystem/UserTest.php +++ b/tests/Entity/UserSystem/UserTest.php @@ -21,6 +21,7 @@ namespace App\Tests\Entity\UserSystem; +use App\Entity\UserSystem\U2FKey; use App\Entity\UserSystem\User; use PHPUnit\Framework\TestCase; @@ -36,4 +37,88 @@ class UserTest extends TestCase $this->assertEquals('John Doe', $user->getFullName(false)); $this->assertEquals('John Doe (username)', $user->getFullName(true)); } + + public function googleAuthenticatorEnabledDataProvider() : array + { + return [ + [null, false], + ['', false], + ['SSSk38498', true] + ]; + } + + /** + * @dataProvider googleAuthenticatorEnabledDataProvider + */ + public function testIsGoogleAuthenticatorEnabled(?string $secret, bool $expected) + { + $user = new User(); + $user->setGoogleAuthenticatorSecret($secret); + $this->assertSame($expected ,$user->isGoogleAuthenticatorEnabled()); + } + + public function testSetBackupCodes() + { + $user = new User(); + $codes = ["test", "invalid", "test"]; + $user->setBackupCodes($codes); + // Backup Codes generation date must be changed! + $this->assertEquals(new \DateTime(), $user->getBackupCodesGenerationDate(), '', 0.1); + $this->assertEquals($codes, $user->getBackupCodes()); + + //Test what happens if we delete the backup keys + $user->setBackupCodes([]); + $this->assertEmpty($user->getBackupCodes()); + $this->assertNull($user->getBackupCodesGenerationDate()); + } + + public function testIsBackupCode() + { + $user = new User(); + $codes = ['aaaa', 'bbbb', 'cccc', 'dddd']; + $user->setBackupCodes($codes); + + $this->assertTrue($user->isBackupCode('aaaa')); + $this->assertTrue($user->isBackupCode('cccc')); + + $this->assertFalse($user->isBackupCode('')); + $this->assertFalse($user->isBackupCode('zzzz')); + } + + public function testInvalidateBackupCode() + { + $user = new User(); + $codes = ['aaaa', 'bbbb', 'cccc', 'dddd']; + $user->setBackupCodes($codes); + + //Ensure the code is valid + $this->assertTrue($user->isBackupCode('aaaa')); + $this->assertTrue($user->isBackupCode('bbbb')); + //Invalidate code, afterwards the code has to be invalid! + $user->invalidateBackupCode('bbbb'); + $this->assertFalse($user->isBackupCode('bbbb')); + $this->assertTrue($user->isBackupCode('aaaa')); + + //No exception must happen, when we try to invalidate an not existing backup key! + $user->invalidateBackupCode('zzzz'); + } + + public function testInvalidateTrustedDeviceTokens() + { + $user = new User(); + $old_value = $user->getTrustedTokenVersion(); + //To invalidate the token, the new value must be bigger than the old value + $user->invalidateTrustedDeviceTokens(); + $this->assertGreaterThan($old_value, $user->getTrustedTokenVersion()); + } + + public function testIsU2fEnabled() + { + $user = new User(); + $user->addU2FKey(new U2FKey()); + $this->assertTrue($user->isU2FAuthEnabled()); + + $user->getU2FKeys()->clear(); + $this->assertFalse($user->isU2FAuthEnabled()); + } } diff --git a/tests/Services/EntityImporterTest.php b/tests/Services/EntityImporterTest.php index a16e5475..4bdf4063 100644 --- a/tests/Services/EntityImporterTest.php +++ b/tests/Services/EntityImporterTest.php @@ -28,6 +28,9 @@ use App\Services\ElementTypeNameGenerator; use App\Services\EntityImporter; use Symfony\Bundle\FrameworkBundle\Test\WebTestCase; +/** + * @group DB + */ class EntityImporterTest extends WebTestCase { /** diff --git a/tests/Services/TFA/BackupCodeGeneratorTest.php b/tests/Services/TFA/BackupCodeGeneratorTest.php new file mode 100644 index 00000000..b7b1108c --- /dev/null +++ b/tests/Services/TFA/BackupCodeGeneratorTest.php @@ -0,0 +1,56 @@ +expectException(\RuntimeException::class); + new BackupCodeGenerator(33, 10); + } + + /** + * Test if an exception is thrown if you are using a too high code length + */ + public function testLengthLowerLimit() + { + $this->expectException(\RuntimeException::class); + new BackupCodeGenerator(4, 10); + } + + + public function codeLengthDataProvider() + { + return [[6], [8], [10], [16]]; + } + + /** + * @dataProvider codeLengthDataProvider + */ + public function testGenerateSingleCode(int $code_length) + { + $generator = new BackupCodeGenerator($code_length, 10); + $this->assertRegExp("/^([a-f0-9]){{$code_length}}\$/", $generator->generateSingleCode()); + } + + public function codeCountDataProvider() + { + return [[2], [8], [10]]; + } + + /** + * @dataProvider codeCountDataProvider + */ + public function testGenerateCodeSet(int $code_count) + { + $generator = new BackupCodeGenerator(8, $code_count); + $this->assertCount($code_count, $generator->generateCodeSet()); + } +}