From 7d052eba007b97e18a14f5ed99220cdcf29fb279 Mon Sep 17 00:00:00 2001 From: Kevin Papst Date: Tue, 13 Jul 2021 23:23:18 +0200 Subject: [PATCH] make sure that timezone is properly validated (#2663) --- src/Command/ImportTimesheetCommand.php | 28 +++++++++++++++---- src/Entity/Timesheet.php | 1 + ...Constraint.php => TimesheetBudgetUsed.php} | 11 +------- .../TimesheetBudgetUsedValidator.php | 17 ++++++----- .../TimesheetLockdownValidator.php | 15 ++++++---- .../Constraints/TimesheetRestartValidator.php | 18 ++++-------- .../TimesheetBudgetUsedValidatorTest.php | 26 ++++++++--------- .../TimesheetLockdownValidatorTest.php | 6 ++-- .../TimesheetRestartValidatorTest.php | 8 ++++-- 9 files changed, 68 insertions(+), 62 deletions(-) rename src/Validator/Constraints/{TimesheetBudgetUsedConstraint.php => TimesheetBudgetUsed.php} (59%) rename src/Validator/{ => Constraints}/TimesheetBudgetUsedValidator.php (90%) rename tests/Validator/{ => Constraints}/TimesheetBudgetUsedValidatorTest.php (97%) diff --git a/src/Command/ImportTimesheetCommand.php b/src/Command/ImportTimesheetCommand.php index f107dbe8..63394219 100644 --- a/src/Command/ImportTimesheetCommand.php +++ b/src/Command/ImportTimesheetCommand.php @@ -206,6 +206,9 @@ class ImportTimesheetCommand extends Command default: try { + if (!\in_array($timezone, \DateTimeZone::listIdentifiers())) { + throw new \InvalidArgumentException('Not a known PHP timezone'); + } $timezone = new \DateTimeZone($timezone); } catch (\Exception $ex) { $io->error('Invalid timezone given, import canceled.'); @@ -245,7 +248,7 @@ class ImportTimesheetCommand extends Command $all = $csv->getRecords(); $total = iterator_count($all); - $io->text(sprintf('Found %s timesheets to import, validating now', $total)); + $io->text(sprintf('Found %s timesheets to import, pre-validating now', $total)); $records = []; $doImport = true; @@ -288,6 +291,7 @@ class ImportTimesheetCommand extends Command $progressBar->advance(); } $progressBar->finish(); + $io->writeln(''); if (!$ignoreErrors && !$doImport) { $io->caution(sprintf('Not importing, previous %s errors need to be fixed first.', $errors)); @@ -295,8 +299,8 @@ class ImportTimesheetCommand extends Command return 5; } - $io->text(sprintf('Validated %s rows.', $countAll)); - $io->text(sprintf('Importing %s of %s rows, skipping %s with validation errors.', \count($records), iterator_count($all), $errors)); + $io->writeln(''); + $io->text(sprintf('Processing %s of %s rows, skipping %s with pre-validation errors.', \count($records), iterator_count($all), $errors)); // values for new users $password = $input->getOption('password'); @@ -306,6 +310,8 @@ class ImportTimesheetCommand extends Command $durationParser = new Duration(); $row = 0; + $imported = 0; + $failed = 0; $isBatchUpdate = $input->getOption('batch'); $batches = []; @@ -401,10 +407,11 @@ class ImportTimesheetCommand extends Command } else { $this->timesheets->save($timesheet); } + + $imported++; } catch (\Exception $ex) { $io->error(sprintf('Failed importing timesheet row %s with: %s', $row, $ex->getMessage())); - - return 6; + $failed++; } $progressBar->advance(); @@ -416,6 +423,9 @@ class ImportTimesheetCommand extends Command $progressBar->finish(); + $io->writeln(''); + $io->writeln(''); + if ($this->createdUsers > 0) { $io->success(sprintf('Created %s users', $this->createdUsers)); } @@ -429,7 +439,13 @@ class ImportTimesheetCommand extends Command $io->success(sprintf('Created %s activities', $this->createdActivities)); } - $io->success(sprintf('Imported %s rows', $row)); + if ($failed > 0) { + $io->warning(sprintf('Failed validating %s rows', $failed)); + } + + if ($imported > 0) { + $io->success(sprintf('Imported %s rows', $imported)); + } return 0; } diff --git a/src/Entity/Timesheet.php b/src/Entity/Timesheet.php index d7f864d3..d9a63012 100644 --- a/src/Entity/Timesheet.php +++ b/src/Entity/Timesheet.php @@ -140,6 +140,7 @@ class Timesheet implements EntityWithMetaFields, ExportItemInterface * @internal for storing the timezone of "begin" and "end" date * * @ORM\Column(name="timezone", type="string", length=64, nullable=false) + * @Assert\Timezone */ private $timezone; /** diff --git a/src/Validator/Constraints/TimesheetBudgetUsedConstraint.php b/src/Validator/Constraints/TimesheetBudgetUsed.php similarity index 59% rename from src/Validator/Constraints/TimesheetBudgetUsedConstraint.php rename to src/Validator/Constraints/TimesheetBudgetUsed.php index 05a1941f..8ff7bc96 100644 --- a/src/Validator/Constraints/TimesheetBudgetUsedConstraint.php +++ b/src/Validator/Constraints/TimesheetBudgetUsed.php @@ -9,18 +9,9 @@ namespace App\Validator\Constraints; -use App\Validator\TimesheetBudgetUsedValidator; - -final class TimesheetBudgetUsedConstraint extends TimesheetConstraint +final class TimesheetBudgetUsed extends TimesheetConstraint { - public const BUDGET_SPENT = 'kimai-timesheet-budget-used-01'; - // same messages, so we can re-use the validation translation! public $messageRate = 'The budget is completely used.'; public $messageTime = 'The budget is completely used.'; - - public function validatedBy() - { - return TimesheetBudgetUsedValidator::class; - } } diff --git a/src/Validator/TimesheetBudgetUsedValidator.php b/src/Validator/Constraints/TimesheetBudgetUsedValidator.php similarity index 90% rename from src/Validator/TimesheetBudgetUsedValidator.php rename to src/Validator/Constraints/TimesheetBudgetUsedValidator.php index a347c4ad..03b41fb0 100644 --- a/src/Validator/TimesheetBudgetUsedValidator.php +++ b/src/Validator/Constraints/TimesheetBudgetUsedValidator.php @@ -7,7 +7,7 @@ * file that was distributed with this source code. */ -namespace App\Validator; +namespace App\Validator\Constraints; use App\Activity\ActivityStatisticService; use App\Configuration\SystemConfiguration; @@ -18,7 +18,6 @@ use App\Repository\TimesheetRepository; use App\Timesheet\RateServiceInterface; use App\Utils\Duration; use App\Utils\LocaleHelper; -use App\Validator\Constraints\TimesheetBudgetUsedConstraint; use Symfony\Component\Validator\Constraint; use Symfony\Component\Validator\ConstraintValidator; use Symfony\Component\Validator\Exception\UnexpectedTypeException; @@ -48,8 +47,8 @@ final class TimesheetBudgetUsedValidator extends ConstraintValidator */ public function validate($timesheet, Constraint $constraint) { - if (!($constraint instanceof TimesheetBudgetUsedConstraint)) { - throw new UnexpectedTypeException($constraint, TimesheetBudgetUsedConstraint::class); + if (!($constraint instanceof TimesheetBudgetUsed)) { + throw new UnexpectedTypeException($constraint, TimesheetBudgetUsed::class); } if (!\is_object($timesheet) || !($timesheet instanceof Timesheet)) { @@ -127,7 +126,7 @@ final class TimesheetBudgetUsedValidator extends ConstraintValidator } } - private function checkActivity(TimesheetBudgetUsedConstraint $constraint, Timesheet $timesheet, int $duration, float $rate): bool + private function checkActivity(TimesheetBudgetUsed $constraint, Timesheet $timesheet, int $duration, float $rate): bool { $activity = $timesheet->getActivity(); @@ -156,7 +155,7 @@ final class TimesheetBudgetUsedValidator extends ConstraintValidator return false; } - private function checkProject(TimesheetBudgetUsedConstraint $constraint, Timesheet $timesheet, int $duration, float $rate): bool + private function checkProject(TimesheetBudgetUsed $constraint, Timesheet $timesheet, int $duration, float $rate): bool { $project = $timesheet->getProject(); @@ -185,7 +184,7 @@ final class TimesheetBudgetUsedValidator extends ConstraintValidator return false; } - private function checkCustomer(TimesheetBudgetUsedConstraint $constraint, Timesheet $timesheet, int $duration, float $rate): bool + private function checkCustomer(TimesheetBudgetUsed $constraint, Timesheet $timesheet, int $duration, float $rate): bool { $customer = $timesheet->getProject()->getCustomer(); @@ -214,7 +213,7 @@ final class TimesheetBudgetUsedValidator extends ConstraintValidator return false; } - private function addBudgetViolation(TimesheetBudgetUsedConstraint $constraint, Timesheet $timesheet, string $field, float $budget, float $rate) + private function addBudgetViolation(TimesheetBudgetUsed $constraint, Timesheet $timesheet, string $field, float $budget, float $rate) { // using the locale of the assigned user is not the best solution, but allows to be independent from the request stack $helper = new LocaleHelper($timesheet->getUser()->getLanguage()); @@ -235,7 +234,7 @@ final class TimesheetBudgetUsedValidator extends ConstraintValidator ; } - private function addTimeBudgetViolation(TimesheetBudgetUsedConstraint $constraint, string $field, int $budget, int $duration) + private function addTimeBudgetViolation(TimesheetBudgetUsed $constraint, string $field, int $budget, int $duration) { $durationFormat = new Duration(); diff --git a/src/Validator/Constraints/TimesheetLockdownValidator.php b/src/Validator/Constraints/TimesheetLockdownValidator.php index 89396ac5..421e0e39 100644 --- a/src/Validator/Constraints/TimesheetLockdownValidator.php +++ b/src/Validator/Constraints/TimesheetLockdownValidator.php @@ -11,19 +11,19 @@ namespace App\Validator\Constraints; use App\Entity\Timesheet as TimesheetEntity; use App\Timesheet\LockdownService; -use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; +use Symfony\Component\Security\Core\Security; use Symfony\Component\Validator\Constraint; use Symfony\Component\Validator\ConstraintValidator; use Symfony\Component\Validator\Exception\UnexpectedTypeException; final class TimesheetLockdownValidator extends ConstraintValidator { - private $auth; private $lockdownService; + private $security; - public function __construct(AuthorizationCheckerInterface $auth, LockdownService $lockdownService) + public function __construct(Security $security, LockdownService $lockdownService) { - $this->auth = $auth; + $this->security = $security; $this->lockdownService = $lockdownService; } @@ -50,7 +50,7 @@ final class TimesheetLockdownValidator extends ConstraintValidator } // lockdown never takes effect for users with special permission - if ($this->auth->isGranted('lockdown_override_timesheet')) { + if (null !== $this->security->getUser() && $this->security->isGranted('lockdown_override_timesheet')) { return; } @@ -67,7 +67,10 @@ final class TimesheetLockdownValidator extends ConstraintValidator } } - $allowEditInGracePeriod = $this->auth->isGranted('lockdown_grace_timesheet'); + $allowEditInGracePeriod = false; + if (null !== $this->security->getUser() && $this->security->isGranted('lockdown_grace_timesheet')) { + $allowEditInGracePeriod = true; + } if ($this->lockdownService->isEditable($timesheet, $now, $allowEditInGracePeriod)) { return; diff --git a/src/Validator/Constraints/TimesheetRestartValidator.php b/src/Validator/Constraints/TimesheetRestartValidator.php index 1a36b864..a9f0c134 100644 --- a/src/Validator/Constraints/TimesheetRestartValidator.php +++ b/src/Validator/Constraints/TimesheetRestartValidator.php @@ -11,26 +11,20 @@ namespace App\Validator\Constraints; use App\Entity\Timesheet as TimesheetEntity; use App\Timesheet\TrackingModeService; -use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; +use Symfony\Component\Security\Core\Security; use Symfony\Component\Validator\Constraint; use Symfony\Component\Validator\ConstraintValidator; use Symfony\Component\Validator\Exception\UnexpectedTypeException; final class TimesheetRestartValidator extends ConstraintValidator { - /** - * @var TrackingModeService - */ private $trackingModeService; - /** - * @var AuthorizationCheckerInterface - */ - private $auth; + private $security; - public function __construct(TrackingModeService $service, AuthorizationCheckerInterface $auth) + public function __construct(Security $security, TrackingModeService $service) { + $this->security = $security; $this->trackingModeService = $service; - $this->auth = $auth; } /** @@ -58,7 +52,7 @@ final class TimesheetRestartValidator extends ConstraintValidator return; } - if ($this->auth->isGranted('start', $timesheet)) { + if (null !== $this->security->getUser() && $this->security->isGranted('start', $timesheet)) { return; } @@ -76,7 +70,5 @@ final class TimesheetRestartValidator extends ConstraintValidator ->setTranslationDomain('validators') ->setCode(TimesheetRestart::START_DISALLOWED) ->addViolation(); - - return; } } diff --git a/tests/Validator/TimesheetBudgetUsedValidatorTest.php b/tests/Validator/Constraints/TimesheetBudgetUsedValidatorTest.php similarity index 97% rename from tests/Validator/TimesheetBudgetUsedValidatorTest.php rename to tests/Validator/Constraints/TimesheetBudgetUsedValidatorTest.php index fb00a901..0173005d 100644 --- a/tests/Validator/TimesheetBudgetUsedValidatorTest.php +++ b/tests/Validator/Constraints/TimesheetBudgetUsedValidatorTest.php @@ -7,7 +7,7 @@ * file that was distributed with this source code. */ -namespace App\Tests\Validator; +namespace App\Tests\Validator\Constraints; use App\Activity\ActivityStatisticService; use App\Configuration\SystemConfiguration; @@ -25,16 +25,16 @@ use App\Repository\TimesheetRepository; use App\Timesheet\Rate; use App\Timesheet\RateService; use App\Timesheet\RateServiceInterface; -use App\Validator\Constraints\TimesheetBudgetUsedConstraint; -use App\Validator\TimesheetBudgetUsedValidator; +use App\Validator\Constraints\TimesheetBudgetUsed; +use App\Validator\Constraints\TimesheetBudgetUsedValidator; use DateTime; use Symfony\Component\Validator\Constraints\NotBlank; use Symfony\Component\Validator\Exception\UnexpectedTypeException; use Symfony\Component\Validator\Test\ConstraintValidatorTestCase; /** - * @covers \App\Validator\Constraints\TimesheetBudgetUsedConstraint - * @covers \App\Validator\TimesheetBudgetUsedValidator + * @covers \App\Validator\Constraints\TimesheetBudgetUsed + * @covers \App\Validator\Constraints\TimesheetBudgetUsedValidator */ class TimesheetBudgetUsedValidatorTest extends ConstraintValidatorTestCase { @@ -83,7 +83,7 @@ class TimesheetBudgetUsedValidatorTest extends ConstraintValidatorTestCase $this->validator->initialize($this->context); $this->context->addViolation('FOOOOOOOOO'); - $this->validator->validate(new Timesheet(), new TimesheetBudgetUsedConstraint()); + $this->validator->validate(new Timesheet(), new TimesheetBudgetUsed()); $this->buildViolation('FOOOOOOOOO')->assertRaised(); } @@ -91,7 +91,7 @@ class TimesheetBudgetUsedValidatorTest extends ConstraintValidatorTestCase { $this->expectException(UnexpectedTypeException::class); - $this->validator->validate('foo', new TimesheetBudgetUsedConstraint()); + $this->validator->validate('foo', new TimesheetBudgetUsed()); } public function testWithMissingEnd() @@ -99,7 +99,7 @@ class TimesheetBudgetUsedValidatorTest extends ConstraintValidatorTestCase $timesheet = new Timesheet(); $timesheet->setBegin(new DateTime()); - $this->validator->validate($timesheet, new TimesheetBudgetUsedConstraint()); + $this->validator->validate($timesheet, new TimesheetBudgetUsed()); $this->assertNoViolation(); } @@ -109,7 +109,7 @@ class TimesheetBudgetUsedValidatorTest extends ConstraintValidatorTestCase $timesheet->setBegin(new DateTime()); $timesheet->setEnd(new DateTime()); - $this->validator->validate($timesheet, new TimesheetBudgetUsedConstraint()); + $this->validator->validate($timesheet, new TimesheetBudgetUsed()); $this->assertNoViolation(); } @@ -120,7 +120,7 @@ class TimesheetBudgetUsedValidatorTest extends ConstraintValidatorTestCase $timesheet->setEnd(new DateTime()); $timesheet->setUser(new User()); - $this->validator->validate($timesheet, new TimesheetBudgetUsedConstraint()); + $this->validator->validate($timesheet, new TimesheetBudgetUsed()); $this->assertNoViolation(); } @@ -135,7 +135,7 @@ class TimesheetBudgetUsedValidatorTest extends ConstraintValidatorTestCase $timesheet->setUser(new User()); $timesheet->setProject($project); - $this->validator->validate($timesheet, new TimesheetBudgetUsedConstraint()); + $this->validator->validate($timesheet, new TimesheetBudgetUsed()); $this->assertNoViolation(); } @@ -161,7 +161,7 @@ class TimesheetBudgetUsedValidatorTest extends ConstraintValidatorTestCase $timesheet->setProject($project); $timesheet->setActivity($activity); - $this->validator->validate($timesheet, new TimesheetBudgetUsedConstraint()); + $this->validator->validate($timesheet, new TimesheetBudgetUsed()); $this->assertNoViolation(); } @@ -372,7 +372,7 @@ class TimesheetBudgetUsedValidatorTest extends ConstraintValidatorTestCase $this->validator = $this->createValidator(false, $activityStatistic, $projectStatistic, $customerStatistic, $rawData, $rate); $this->validator->initialize($this->context); - $this->validator->validate($timesheet, new TimesheetBudgetUsedConstraint()); + $this->validator->validate($timesheet, new TimesheetBudgetUsed()); if (null === $used && null === $budget && null === $free && $path === null) { $this->assertNoViolation(); diff --git a/tests/Validator/Constraints/TimesheetLockdownValidatorTest.php b/tests/Validator/Constraints/TimesheetLockdownValidatorTest.php index 36196f0c..9083a54f 100644 --- a/tests/Validator/Constraints/TimesheetLockdownValidatorTest.php +++ b/tests/Validator/Constraints/TimesheetLockdownValidatorTest.php @@ -12,10 +12,11 @@ namespace App\Tests\Validator\Constraints; use App\Configuration\ConfigLoaderInterface; use App\Configuration\SystemConfiguration; use App\Entity\Timesheet; +use App\Entity\User; use App\Timesheet\LockdownService; use App\Validator\Constraints\TimesheetLockdown; use App\Validator\Constraints\TimesheetLockdownValidator; -use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; +use Symfony\Component\Security\Core\Security; use Symfony\Component\Validator\Constraints\NotBlank; use Symfony\Component\Validator\Exception\UnexpectedTypeException; use Symfony\Component\Validator\Test\ConstraintValidatorTestCase; @@ -32,7 +33,8 @@ class TimesheetLockdownValidatorTest extends ConstraintValidatorTestCase protected function createMyValidator(bool $allowOverwriteFull, bool $allowOverwriteGrace, ?string $start, ?string $end, ?string $grace) { - $auth = $this->createMock(AuthorizationCheckerInterface::class); + $auth = $this->createMock(Security::class); + $auth->method('getUser')->willReturn(new User()); $auth->method('isGranted')->willReturnCallback( function ($attributes, $subject = null) use ($allowOverwriteFull, $allowOverwriteGrace) { switch ($attributes) { diff --git a/tests/Validator/Constraints/TimesheetRestartValidatorTest.php b/tests/Validator/Constraints/TimesheetRestartValidatorTest.php index 25a1d918..70f8bc57 100644 --- a/tests/Validator/Constraints/TimesheetRestartValidatorTest.php +++ b/tests/Validator/Constraints/TimesheetRestartValidatorTest.php @@ -13,11 +13,12 @@ use App\Entity\Activity; use App\Entity\Customer; use App\Entity\Project; use App\Entity\Timesheet; +use App\Entity\User; use App\Tests\Mocks\TrackingModeServiceFactory; use App\Validator\Constraints\TimesheetOverlapping; use App\Validator\Constraints\TimesheetRestart; use App\Validator\Constraints\TimesheetRestartValidator; -use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; +use Symfony\Component\Security\Core\Security; use Symfony\Component\Validator\Constraints\NotBlank; use Symfony\Component\Validator\Exception\UnexpectedTypeException; use Symfony\Component\Validator\Test\ConstraintValidatorTestCase; @@ -34,12 +35,13 @@ class TimesheetRestartValidatorTest extends ConstraintValidatorTestCase protected function createMyValidator(bool $allowed, string $trackingMode) { - $auth = $this->createMock(AuthorizationCheckerInterface::class); + $auth = $this->createMock(Security::class); + $auth->method('getUser')->willReturn(new User()); $auth->method('isGranted')->willReturn($allowed); $service = (new TrackingModeServiceFactory($this))->create($trackingMode); - return new TimesheetRestartValidator($service, $auth); + return new TimesheetRestartValidator($auth, $service); } public function testConstraintIsInvalid()