From 8eec2f3b2196f081fe22c2cf769eb6d42a53a6cd Mon Sep 17 00:00:00 2001 From: doobry Date: Sat, 30 Mar 2024 13:17:43 +0100 Subject: [PATCH 1/4] refactor: Rename Voucher constraint to VoucherExists --- config/validator/validation.yaml | 2 +- .../{Voucher.php => VoucherExists.php} | 2 +- ...lidator.php => VoucherExistsValidator.php} | 13 ++------ ...est.php => VoucherExistsValidatorTest.php} | 30 +++++++++---------- 4 files changed, 20 insertions(+), 27 deletions(-) rename src/Validator/Constraints/{Voucher.php => VoucherExists.php} (94%) rename src/Validator/Constraints/{VoucherValidator.php => VoucherExistsValidator.php} (86%) rename tests/Validator/Constraints/{VoucherValidatorTest.php => VoucherExistsValidatorTest.php} (72%) diff --git a/config/validator/validation.yaml b/config/validator/validation.yaml index 31bdddc0..ae5e8fb0 100644 --- a/config/validator/validation.yaml +++ b/config/validator/validation.yaml @@ -53,7 +53,7 @@ App\Form\Model\Registration: minLength: 3 maxLength: 32 voucher: - - App\Validator\Constraints\Voucher: + - App\Validator\Constraints\VoucherExists: exists: true plainPassword: - App\Validator\Constraints\PasswordPolicy: ~ diff --git a/src/Validator/Constraints/Voucher.php b/src/Validator/Constraints/VoucherExists.php similarity index 94% rename from src/Validator/Constraints/Voucher.php rename to src/Validator/Constraints/VoucherExists.php index 03cc7666..9209fa0d 100644 --- a/src/Validator/Constraints/Voucher.php +++ b/src/Validator/Constraints/VoucherExists.php @@ -5,7 +5,7 @@ use Symfony\Component\Validator\Constraint; use Symfony\Component\Validator\Exception\MissingOptionsException; -class Voucher extends Constraint +class VoucherExists extends Constraint { /** * @var bool|null diff --git a/src/Validator/Constraints/VoucherValidator.php b/src/Validator/Constraints/VoucherExistsValidator.php similarity index 86% rename from src/Validator/Constraints/VoucherValidator.php rename to src/Validator/Constraints/VoucherExistsValidator.php index 1f702a4f..d3351f10 100644 --- a/src/Validator/Constraints/VoucherValidator.php +++ b/src/Validator/Constraints/VoucherExistsValidator.php @@ -4,22 +4,15 @@ use App\Entity\Voucher; use App\Repository\VoucherRepository; -use App\Validator\Constraints\Voucher as VoucherConstraint; use Doctrine\ORM\EntityManagerInterface; use Symfony\Component\Validator\Constraint; use Symfony\Component\Validator\ConstraintValidator; use Symfony\Component\Validator\Exception\UnexpectedTypeException; -/** - * Class VoucherValidator. - */ -class VoucherValidator extends ConstraintValidator +class VoucherExistsValidator extends ConstraintValidator { private readonly VoucherRepository $voucherRepository; - /** - * VoucherValidator constructor. - */ public function __construct(EntityManagerInterface $manager) { $this->voucherRepository = $manager->getRepository(Voucher::class); @@ -33,8 +26,8 @@ public function __construct(EntityManagerInterface $manager) */ public function validate($value, Constraint $constraint): void { - if (!$constraint instanceof VoucherConstraint) { - throw new UnexpectedTypeException($constraint, Voucher::class); + if (!$constraint instanceof VoucherExists) { + throw new UnexpectedTypeException($constraint, VoucherExists::class); } if (!is_scalar($value) && !(\is_object($value) && method_exists($value, '__toString'))) { diff --git a/tests/Validator/Constraints/VoucherValidatorTest.php b/tests/Validator/Constraints/VoucherExistsValidatorTest.php similarity index 72% rename from tests/Validator/Constraints/VoucherValidatorTest.php rename to tests/Validator/Constraints/VoucherExistsValidatorTest.php index b20ee450..1ee86be1 100644 --- a/tests/Validator/Constraints/VoucherValidatorTest.php +++ b/tests/Validator/Constraints/VoucherExistsValidatorTest.php @@ -2,20 +2,20 @@ namespace App\Tests\Validator\Constraints; -use stdClass; use App\Entity\Voucher; use App\Repository\VoucherRepository; -use App\Validator\Constraints\Voucher as VoucherConstraint; -use App\Validator\Constraints\VoucherValidator; +use App\Validator\Constraints\VoucherExists; +use App\Validator\Constraints\VoucherExistsValidator; use Doctrine\ORM\EntityManagerInterface; +use stdClass; use Symfony\Component\Validator\Constraints\Valid; use Symfony\Component\Validator\Exception\MissingOptionsException; use Symfony\Component\Validator\Exception\UnexpectedTypeException; use Symfony\Component\Validator\Test\ConstraintValidatorTestCase; -class VoucherValidatorTest extends ConstraintValidatorTestCase +class VoucherExistsValidatorTest extends ConstraintValidatorTestCase { - protected function createValidator(): VoucherValidator + protected function createValidator(): VoucherExistsValidator { $voucher = new Voucher(); $voucher->setCode('code'); @@ -28,7 +28,7 @@ protected function createValidator(): VoucherValidator $manager = $this->getMockBuilder(EntityManagerInterface::class)->getMock(); $manager->method('getRepository')->willReturn($repository); - return new VoucherValidator($manager); + return new VoucherExistsValidator($manager); } public function testExpectsVoucherType(): void @@ -40,12 +40,12 @@ public function testExpectsVoucherType(): void public function testNullIsInvalid(): void { $this->expectException(UnexpectedTypeException::class); - $this->validator->validate(null, new VoucherConstraint(true)); + $this->validator->validate(null, new VoucherExists(true)); } public function testEmptyStringIsInvalid(): void { - $this->validator->validate('', new VoucherConstraint(true)); + $this->validator->validate('', new VoucherExists(true)); $this->buildViolation('registration.voucher-invalid') ->assertRaised(); @@ -54,43 +54,43 @@ public function testEmptyStringIsInvalid(): void public function testExpectsStringCompatibleType(): void { $this->expectException(UnexpectedTypeException::class); - $this->validator->validate(new stdClass(), new VoucherConstraint(true)); + $this->validator->validate(new stdClass(), new VoucherExists(true)); } public function testConstraintMissingOptions(): void { $this->expectException(MissingOptionsException::class); - new VoucherConstraint(); + new VoucherExists(); } public function testConstraintGetDefaultOption(): void { - $constraint = new VoucherConstraint(true); + $constraint = new VoucherExists(true); self::assertEquals(true, $constraint->exists); } public function testValidateVoucherInvalid(): void { - $this->validator->validate('code2', new VoucherConstraint(true)); + $this->validator->validate('code2', new VoucherExists(true)); $this->buildViolation('registration.voucher-invalid') ->assertRaised(); } public function testValidateVoucherUnused(): void { - $this->validator->validate('code', new VoucherConstraint(true)); + $this->validator->validate('code', new VoucherExists(true)); $this->assertNoViolation(); } public function testValidateVoucherNew(): void { - $this->validator->validate('new', new VoucherConstraint(false)); + $this->validator->validate('new', new VoucherExists(false)); $this->assertNoViolation(); } public function testValidateVoucherNewExists(): void { - $this->validator->validate('code', new VoucherConstraint(false)); + $this->validator->validate('code', new VoucherExists(false)); $this->buildViolation('registration.voucher-exists') ->assertRaised(); } From 313621ea5ac0630f83e29bd5d84c6c3edfdd6c56 Mon Sep 17 00:00:00 2001 From: doobry Date: Sat, 30 Mar 2024 16:16:29 +0100 Subject: [PATCH 2/4] feat(voucher): Check whether user is suspicious before creating voucher --- config/validator/validation.yaml | 4 +++ default_translations/de/validators.de.yml | 3 ++ default_translations/en/validators.en.yml | 3 ++ src/Command/VoucherCreationCommand.php | 22 ++++++------ src/Validator/Constraints/VoucherUser.php | 13 +++++++ .../Constraints/VoucherUserValidator.php | 36 +++++++++++++++++++ 6 files changed, 70 insertions(+), 11 deletions(-) create mode 100644 src/Validator/Constraints/VoucherUser.php create mode 100644 src/Validator/Constraints/VoucherUserValidator.php diff --git a/config/validator/validation.yaml b/config/validator/validation.yaml index ae5e8fb0..6d6a9dca 100644 --- a/config/validator/validation.yaml +++ b/config/validator/validation.yaml @@ -42,6 +42,10 @@ App\Entity\User: constraints: - App\Validator\Constraints\EmailDomain: ~ +App\Entity\Voucher: + constraints: + - App\Validator\Constraints\VoucherUser: ~ + App\Form\Model\Registration: properties: email: diff --git a/default_translations/de/validators.de.yml b/default_translations/de/validators.de.yml index e93707c3..824a8484 100644 --- a/default_translations/de/validators.de.yml +++ b/default_translations/de/validators.de.yml @@ -20,3 +20,6 @@ registration: email-too-long: Der eingegebene Benutzername darf nicht mehr als %max% Zeichen enthalten. email-unexpected-characters: 'Der eingegebene Benutzername enthält nicht erlaubte Zeichen. Erlaubt sind: Buchstaben und Zahlen, sowie -, _ und .' + +voucher: + suspicious-user: "Nicht erlaubt, Einladungscodes für verdächtige Benutzer:innen anzulegen." diff --git a/default_translations/en/validators.en.yml b/default_translations/en/validators.en.yml index 5f87d8fc..935f2625 100644 --- a/default_translations/en/validators.en.yml +++ b/default_translations/en/validators.en.yml @@ -21,3 +21,6 @@ registration: email-too-short: "The username must contain at least %min% characters." email-too-long: "The username must contain at most %max% characters." email-unexpected-characters: "The username contains unexpected characters. Only valid: Letters and numbers, as well as -, _ and ." + +voucher: + suspicious-user: "Not allowed to create voucher for suspicious users." diff --git a/src/Command/VoucherCreationCommand.php b/src/Command/VoucherCreationCommand.php index 4260345c..bea95786 100644 --- a/src/Command/VoucherCreationCommand.php +++ b/src/Command/VoucherCreationCommand.php @@ -2,8 +2,8 @@ namespace App\Command; +use App\Creator\VoucherCreator; use App\Entity\User; -use App\Factory\VoucherFactory; use Doctrine\ORM\EntityManagerInterface; use Symfony\Component\Console\Attribute\AsCommand; use Symfony\Component\Console\Command\Command; @@ -16,7 +16,11 @@ #[AsCommand(name: 'app:voucher:create')] class VoucherCreationCommand extends Command { - public function __construct(private readonly EntityManagerInterface $manager, private readonly RouterInterface $router, private readonly string $appUrl) + public function __construct( + private readonly EntityManagerInterface $manager, + private readonly RouterInterface $router, + private readonly VoucherCreator $creator, + private readonly string $appUrl) { parent::__construct(); } @@ -25,10 +29,10 @@ protected function configure(): void { $this ->setDescription('Create voucher for a specific user') - ->addOption('user', 'u', InputOption::VALUE_REQUIRED, 'User who get the voucher(s)') - ->addOption('count', 'c', InputOption::VALUE_OPTIONAL, 'Count of the voucher which will created', 3) - ->addOption('print', 'p', InputOption::VALUE_NONE, 'Print out vouchers') - ->addOption('print-links', 'l', InputOption::VALUE_NONE, 'Print out links to vouchers'); + ->addOption('user', 'u', InputOption::VALUE_REQUIRED, 'User who gets the voucher(s) assigned') + ->addOption('count', 'c', InputOption::VALUE_OPTIONAL, 'How many voucher to create', 3) + ->addOption('print', 'p', InputOption::VALUE_NONE, 'Show vouchers') + ->addOption('print-links', 'l', InputOption::VALUE_NONE, 'Show links to vouchers'); } protected function execute(InputInterface $input, OutputInterface $output): int @@ -44,7 +48,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int } for ($i = 1; $i <= $input->getOption('count'); ++$i) { - $voucher = VoucherFactory::create($user); + $voucher = $this->creator->create($user); if (true === $input->getOption('print-links')) { $output->write(sprintf("%s\n", $this->router->generate( 'register_voucher', @@ -53,12 +57,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int } elseif (true === $input->getOption('print')) { $output->write(sprintf("%s\n", $voucher->getCode())); } - - $this->manager->persist($voucher); } - $this->manager->flush(); - return 0; } } diff --git a/src/Validator/Constraints/VoucherUser.php b/src/Validator/Constraints/VoucherUser.php new file mode 100644 index 00000000..3383cb0e --- /dev/null +++ b/src/Validator/Constraints/VoucherUser.php @@ -0,0 +1,13 @@ +getUser(); + if ($user->hasRole(Roles::SUSPICIOUS)) { + $this->context->addViolation('voucher.suspicious-user'); + } + } +} From a6c652035a7a5352d1b74e566a210d87994e9550 Mon Sep 17 00:00:00 2001 From: doobry Date: Sun, 31 Mar 2024 13:40:26 +0200 Subject: [PATCH 3/4] test: Add VoucherCreateCommandTest and VoucherUserValidatorTest --- config/services.yaml | 2 +- ...d.feature => voucherCreateCommand.feature} | 2 +- ...onCommand.php => VoucherCreateCommand.php} | 2 +- tests/Command/VoucherCreateCommandTest.php | 135 ++++++++++++++++++ .../Constraints/VoucherUserValidatorTest.php | 53 +++++++ 5 files changed, 191 insertions(+), 3 deletions(-) rename features/{voucherCreationCommand.feature => voucherCreateCommand.feature} (95%) rename src/Command/{VoucherCreationCommand.php => VoucherCreateCommand.php} (98%) create mode 100644 tests/Command/VoucherCreateCommandTest.php create mode 100644 tests/Validator/Constraints/VoucherUserValidatorTest.php diff --git a/config/services.yaml b/config/services.yaml index dca183a6..82dfa053 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -75,7 +75,7 @@ services: arguments: $mailLocation: "%env(DOVECOT_MAIL_LOCATION)%" - App\Command\VoucherCreationCommand: + App\Command\VoucherCreateCommand: arguments: $appUrl: "%env(APP_URL)%" diff --git a/features/voucherCreationCommand.feature b/features/voucherCreateCommand.feature similarity index 95% rename from features/voucherCreationCommand.feature rename to features/voucherCreateCommand.feature index a146dad9..acc7987c 100644 --- a/features/voucherCreationCommand.feature +++ b/features/voucherCreateCommand.feature @@ -1,4 +1,4 @@ -Feature: VoucherCreationCommand +Feature: VoucherCreateCommand Background: Given the database is clean diff --git a/src/Command/VoucherCreationCommand.php b/src/Command/VoucherCreateCommand.php similarity index 98% rename from src/Command/VoucherCreationCommand.php rename to src/Command/VoucherCreateCommand.php index bea95786..4c9b7afc 100644 --- a/src/Command/VoucherCreationCommand.php +++ b/src/Command/VoucherCreateCommand.php @@ -14,7 +14,7 @@ use Symfony\Component\Security\Core\Exception\UserNotFoundException; #[AsCommand(name: 'app:voucher:create')] -class VoucherCreationCommand extends Command +class VoucherCreateCommand extends Command { public function __construct( private readonly EntityManagerInterface $manager, diff --git a/tests/Command/VoucherCreateCommandTest.php b/tests/Command/VoucherCreateCommandTest.php new file mode 100644 index 00000000..2f46dbf8 --- /dev/null +++ b/tests/Command/VoucherCreateCommandTest.php @@ -0,0 +1,135 @@ +createMock(EntityManagerInterface::class); + $this->repository = $this->createMock(UserRepository::class); + $manager->method('getRepository')->willReturn($this->repository); + + $this->router = $this->createMock(RouterInterface::class); + + $this->creator = $this->createMock(VoucherCreator::class); + + $this->command = new VoucherCreateCommand($manager, $this->router, $this->creator, $this->baseUrl); + } + + public function testExecuteWithUnknownUser(): void + { + $this->repository->method('findByEmail') + ->willReturn(null); + + $application = new Application(); + $application->add($this->command); + + $command = $application->find('app:voucher:create'); + $commandTester = new CommandTester($command); + + $this->expectException(UserNotFoundException::class); + $commandTester->execute([ + '--user' => 'user@example.org', + '--count' => 1, + '--print' + ]); + + $output = $commandTester->getDisplay(); + self::assertStringContainsString('', $output); + } + + public function testExecuteWithUser(): void + { + $user = new User(); + $user->setEmail('user@example.org'); + $this->repository->method('findByEmail') + ->willReturn($user); + + $this->router->method('generate') + ->willReturn($this->baseUrl . '/' . $this->voucherCode); + + $voucher = new Voucher(); + $voucher->setCode($this->voucherCode); + $this->creator->method('create') + ->willReturn($voucher); + + $application = new Application(); + $application->add($this->command); + + $command = $application->find('app:voucher:create'); + $commandTester = new CommandTester($command); + + // Test show vouchers + + $commandTester->execute([ + '--user' => $user->getEmail(), + '--count' => 1, + '--print' => true, + ]); + + $commandTester->assertCommandIsSuccessful(); + + $output = $commandTester->getDisplay(); + $this->assertStringContainsString($this->voucherCode, $output); + + // Test show links to vouchers + + $commandTester->execute([ + '--user' => $user->getEmail(), + '--count' => 1, + '--print-links' => true, + ]); + + $commandTester->assertCommandIsSuccessful(); + + $output = $commandTester->getDisplay(); + $this->assertStringContainsString($this->baseUrl . '/' . $this->voucherCode, $output); + } + + public function testExecuteWithSuspiciousUser(): void + { + $user = new User(); + $user->setEmail('suspicious@example.org'); + $this->repository->method('findByEmail') + ->willReturn($user); + + $voucher = new Voucher(); + $voucher->setCode($this->voucherCode); + $exception = $this->createMock(ValidationException::class); + $this->creator->method('create') + ->willThrowException($exception); + + $application = new Application(); + $application->add($this->command); + + $command = $application->find('app:voucher:create'); + $commandTester = new CommandTester($command); + + $this->expectException(ValidationException::class); + $commandTester->execute([ + '--user' => $user->getEmail(), + '--count' => 1, + ]); + } +} diff --git a/tests/Validator/Constraints/VoucherUserValidatorTest.php b/tests/Validator/Constraints/VoucherUserValidatorTest.php new file mode 100644 index 00000000..c4a78ce2 --- /dev/null +++ b/tests/Validator/Constraints/VoucherUserValidatorTest.php @@ -0,0 +1,53 @@ +voucher = new Voucher(); + $this->voucher->setCode('code'); + + return new VoucherUserValidator(); + } + + public function testExpectsVoucherUserType(): void + { + $this->expectException(UnexpectedTypeException::class); + $this->validator->validate('string', new Valid()); + } + + public function testNoSuspiciousUser(): void + { + $user = new User(); + $user->setRoles([Roles::USER, Roles::SUSPICIOUS]); + $this->voucher->setUser($user); + $this->validator->validate($this->voucher, new VoucherUser()); + $this->buildViolation('voucher.suspicious-user') + ->assertRaised(); + } + + public function testValidVoucherUser(): void + { + $user = new User(); + $user->setRoles([Roles::USER]); + $this->voucher->setUser($user); + $this->validator->validate($this->voucher, new VoucherUser()); + $this->assertNoViolation(); + } +} From 0806ccd5a48b0819396a1ca4016d75ccfcb1fd60 Mon Sep 17 00:00:00 2001 From: doobry Date: Sun, 31 Mar 2024 14:04:55 +0200 Subject: [PATCH 4/4] chore: split long line in VoucherExists --- src/Validator/Constraints/VoucherExists.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Validator/Constraints/VoucherExists.php b/src/Validator/Constraints/VoucherExists.php index 9209fa0d..01c5f5d8 100644 --- a/src/Validator/Constraints/VoucherExists.php +++ b/src/Validator/Constraints/VoucherExists.php @@ -26,7 +26,9 @@ public function __construct($options = null) parent::__construct($options); if (null === $this->exists) { - throw new MissingOptionsException(sprintf('Option "exists" must be given for constraint %s', __CLASS__), ['min', 'max']); + throw new MissingOptionsException( + sprintf('Option "exists" must be given for constraint %s', __CLASS__), ['min', 'max'] + ); } } }