From 67d5cba09fc5e97a28021fdfe15e7dfa77da6114 Mon Sep 17 00:00:00 2001 From: Kevin Papst Date: Sun, 17 Jan 2021 17:50:37 +0100 Subject: [PATCH] improve permission checks for timesheets with activated lockdown (#2271) --- src/Security/RolePermissionManager.php | 11 ++ src/Timesheet/LockdownService.php | 96 +++++++++++ src/Timesheet/UserDateTimeFactory.php | 1 + .../TimesheetLockdownValidator.php | 66 ++------ src/Voter/AbstractVoter.php | 2 + src/Voter/ActivityVoter.php | 19 ++- src/Voter/CustomerVoter.php | 19 ++- src/Voter/ProjectVoter.php | 19 ++- src/Voter/RolePermissionVoter.php | 15 +- src/Voter/TeamVoter.php | 15 +- src/Voter/TimesheetVoter.php | 85 +++++++++- src/Voter/UserVoter.php | 21 ++- tests/Security/RolePermissionManagerTest.php | 7 + tests/Timesheet/LockdownServiceTest.php | 151 ++++++++++++++++++ .../TimesheetLockdownValidatorTest.php | 3 +- tests/Voter/AbstractVoterTest.php | 21 +-- tests/Voter/ActivityVoterTest.php | 2 +- tests/Voter/CustomerVoterTest.php | 2 +- tests/Voter/DeprecatedAbstractVoterTest.php | 80 ++++++++++ tests/Voter/ProjectVoterTest.php | 2 +- tests/Voter/RolePermissionVoterTest.php | 2 +- tests/Voter/TeamVoterTest.php | 2 +- tests/Voter/TimesheetVoterTest.php | 63 +++++++- tests/Voter/UserVoterTest.php | 2 +- 24 files changed, 595 insertions(+), 111 deletions(-) create mode 100644 src/Timesheet/LockdownService.php create mode 100644 tests/Timesheet/LockdownServiceTest.php create mode 100644 tests/Voter/DeprecatedAbstractVoterTest.php diff --git a/src/Security/RolePermissionManager.php b/src/Security/RolePermissionManager.php index 565a984b..b50c8acb 100644 --- a/src/Security/RolePermissionManager.php +++ b/src/Security/RolePermissionManager.php @@ -90,6 +90,17 @@ final class RolePermissionManager return \in_array($permission, $this->permissions[$role]); } + public function hasRolePermission(User $user, string $permission) + { + foreach ($user->getRoles() as $role) { + if ($this->hasPermission($role, $permission)) { + return true; + } + } + + return false; + } + /** * Only permissions which were registered through the Symfony configuration stack will be returned here. * diff --git a/src/Timesheet/LockdownService.php b/src/Timesheet/LockdownService.php new file mode 100644 index 00000000..d18e5081 --- /dev/null +++ b/src/Timesheet/LockdownService.php @@ -0,0 +1,96 @@ +configuration = $configuration; + } + + public function isLockdownActive(): bool + { + if ($this->isActive === null) { + $this->isActive = $this->configuration->isTimesheetLockdownActive(); + } + + return $this->isActive; + } + + /** + * Does not check if the current user is allowed to edit timesheets in lockdown situations. + * This needs to be performed earlier by yourself (see TimesheetVoter or LockdownValidator). + * + * @param Timesheet $timesheet + * @param \DateTime $now + * @param bool $allowEditInGracePeriod + * @return bool + */ + public function isEditable(Timesheet $timesheet, \DateTime $now, bool $allowEditInGracePeriod = false) + { + if (!$this->isLockdownActive()) { + return true; + } + + $timesheetStart = $timesheet->getBegin(); + + if (null === $timesheetStart) { + return true; + } + + $lockedStart = $this->configuration->getTimesheetLockdownPeriodStart(); + $lockedEnd = $this->configuration->getTimesheetLockdownPeriodEnd(); + + $gracePeriod = $this->configuration->getTimesheetLockdownGracePeriod(); + if (!empty($gracePeriod)) { + $gracePeriod = $gracePeriod . ' '; + } + + try { + $lockdownStart = new \DateTime($lockedStart, $timesheetStart->getTimezone()); + $lockdownEnd = new \DateTime($lockedEnd, $timesheetStart->getTimezone()); + $lockdownGrace = new \DateTime($gracePeriod . $lockdownEnd->format('Y-m-d'), $timesheetStart->getTimezone()); + } catch (\Exception $ex) { + // should not happen, but ... if parsing of datetimes fails: skip validation + return true; + } + + // misconfiguration detected, skip validation + if ($lockdownEnd < $lockdownStart) { + return true; + } + + // validate only entries added before the end of lockdown period + if ($timesheetStart > $lockdownEnd) { + return true; + } + + // further validate entries inside of the most recent lockdown + if ($timesheetStart > $lockdownStart && $timesheetStart < $lockdownEnd) { + // if grace period is still in effect, validation succeeds + if ($now < $lockdownGrace) { + return true; + } + + if ($allowEditInGracePeriod) { + return true; + } + } + + return false; + } +} diff --git a/src/Timesheet/UserDateTimeFactory.php b/src/Timesheet/UserDateTimeFactory.php index 288109f0..296ace6a 100644 --- a/src/Timesheet/UserDateTimeFactory.php +++ b/src/Timesheet/UserDateTimeFactory.php @@ -14,6 +14,7 @@ use App\Security\CurrentUser; use DateTimeZone; /** + * @codeCoverageIgnore * @deprecated will be removed with 2.0 */ class UserDateTimeFactory extends DateTimeFactory diff --git a/src/Validator/Constraints/TimesheetLockdownValidator.php b/src/Validator/Constraints/TimesheetLockdownValidator.php index 206ad1a9..89396ac5 100644 --- a/src/Validator/Constraints/TimesheetLockdownValidator.php +++ b/src/Validator/Constraints/TimesheetLockdownValidator.php @@ -9,8 +9,8 @@ namespace App\Validator\Constraints; -use App\Configuration\SystemConfiguration; use App\Entity\Timesheet as TimesheetEntity; +use App\Timesheet\LockdownService; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; use Symfony\Component\Validator\Constraint; use Symfony\Component\Validator\ConstraintValidator; @@ -18,19 +18,13 @@ use Symfony\Component\Validator\Exception\UnexpectedTypeException; final class TimesheetLockdownValidator extends ConstraintValidator { - /** - * @var AuthorizationCheckerInterface - */ private $auth; - /** - * @var SystemConfiguration - */ - private $configuration; + private $lockdownService; - public function __construct(AuthorizationCheckerInterface $auth, SystemConfiguration $configuration) + public function __construct(AuthorizationCheckerInterface $auth, LockdownService $lockdownService) { $this->auth = $auth; - $this->configuration = $configuration; + $this->lockdownService = $lockdownService; } /** @@ -47,40 +41,11 @@ final class TimesheetLockdownValidator extends ConstraintValidator throw new UnexpectedTypeException($timesheet, TimesheetEntity::class); } - $timesheetStart = $timesheet->getBegin(); - - if (null === $timesheetStart) { + if (!$this->lockdownService->isLockdownActive()) { return; } - if (!$this->configuration->isTimesheetLockdownActive()) { - return; - } - - $lockedStart = $this->configuration->getTimesheetLockdownPeriodStart(); - $lockedEnd = $this->configuration->getTimesheetLockdownPeriodEnd(); - - $gracePeriod = $this->configuration->getTimesheetLockdownGracePeriod(); - if (!empty($gracePeriod)) { - $gracePeriod = $gracePeriod . ' '; - } - - try { - $lockdownStart = new \DateTime($lockedStart, $timesheetStart->getTimezone()); - $lockdownEnd = new \DateTime($lockedEnd, $timesheetStart->getTimezone()); - $lockdownGrace = new \DateTime($gracePeriod . $lockdownEnd->format('Y-m-d'), $timesheetStart->getTimezone()); - } catch (\Exception $ex) { - // should not happen, but ... if parsing of datetimes fails: skip validation - return; - } - - // misconfiguration detected, skip validation - if ($lockdownEnd < $lockdownStart) { - return; - } - - // validate only entries added before the end of lockdown period - if ($timesheetStart > $lockdownEnd) { + if (null === ($timesheetStart = $timesheet->getBegin())) { return; } @@ -89,6 +54,8 @@ final class TimesheetLockdownValidator extends ConstraintValidator return; } + $now = new \DateTime('now', $timesheetStart->getTimezone()); + if (!empty($constraint->now)) { if ($constraint->now instanceof \DateTime) { $now = $constraint->now; @@ -100,21 +67,10 @@ final class TimesheetLockdownValidator extends ConstraintValidator } } - if (empty($now)) { - $now = new \DateTime('now', $timesheetStart->getTimezone()); - } + $allowEditInGracePeriod = $this->auth->isGranted('lockdown_grace_timesheet'); - // further validate entries inside of the most recent lockdown - if ($timesheetStart > $lockdownStart && $timesheetStart < $lockdownEnd) { - // if grace period is still in effect, validation succeeds - if ($now < $lockdownGrace) { - return; - } - - // if user has special role, validation succeeds - if ($this->auth->isGranted('lockdown_grace_timesheet')) { - return; - } + if ($this->lockdownService->isEditable($timesheet, $now, $allowEditInGracePeriod)) { + return; } // raise a violation for all entries before the start of lockdown period diff --git a/src/Voter/AbstractVoter.php b/src/Voter/AbstractVoter.php index 310c0eb8..3edbe170 100644 --- a/src/Voter/AbstractVoter.php +++ b/src/Voter/AbstractVoter.php @@ -17,6 +17,8 @@ use Symfony\Component\Security\Core\Authorization\Voter\Voter; /** * Abstract voter to help with checking user permissions. + * @codeCoverageIgnore + * @deprecated since 1.13 */ abstract class AbstractVoter extends Voter { diff --git a/src/Voter/ActivityVoter.php b/src/Voter/ActivityVoter.php index 35fa54bc..7b69bf0e 100644 --- a/src/Voter/ActivityVoter.php +++ b/src/Voter/ActivityVoter.php @@ -12,17 +12,19 @@ namespace App\Voter; use App\Entity\Activity; use App\Entity\Team; use App\Entity\User; +use App\Security\RolePermissionManager; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\Voter\Voter; /** * A voter to check permissions on Activities. */ -class ActivityVoter extends AbstractVoter +class ActivityVoter extends Voter { /** * support rules based on the given activity */ - public const ALLOWED_ATTRIBUTES = [ + private const ALLOWED_ATTRIBUTES = [ 'view', 'edit', 'budget', @@ -30,6 +32,13 @@ class ActivityVoter extends AbstractVoter 'permissions', ]; + private $permissionManager; + + public function __construct(RolePermissionManager $permissionManager) + { + $this->permissionManager = $permissionManager; + } + /** * @param string $attribute * @param Activity $subject @@ -62,7 +71,7 @@ class ActivityVoter extends AbstractVoter return false; } - if ($this->hasRolePermission($user, $attribute . '_activity')) { + if ($this->permissionManager->hasRolePermission($user, $attribute . '_activity')) { return true; } @@ -71,8 +80,8 @@ class ActivityVoter extends AbstractVoter return false; } - $hasTeamleadPermission = $this->hasRolePermission($user, $attribute . '_teamlead_activity'); - $hasTeamPermission = $this->hasRolePermission($user, $attribute . '_team_activity'); + $hasTeamleadPermission = $this->permissionManager->hasRolePermission($user, $attribute . '_teamlead_activity'); + $hasTeamPermission = $this->permissionManager->hasRolePermission($user, $attribute . '_team_activity'); if (!$hasTeamleadPermission && !$hasTeamPermission) { return false; diff --git a/src/Voter/CustomerVoter.php b/src/Voter/CustomerVoter.php index 948f3d8b..5d0f81b7 100644 --- a/src/Voter/CustomerVoter.php +++ b/src/Voter/CustomerVoter.php @@ -12,17 +12,19 @@ namespace App\Voter; use App\Entity\Customer; use App\Entity\Team; use App\Entity\User; +use App\Security\RolePermissionManager; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\Voter\Voter; /** * A voter to check authorization on Customers. */ -class CustomerVoter extends AbstractVoter +class CustomerVoter extends Voter { /** * supported attributes/rules based on the given customer */ - public const ALLOWED_ATTRIBUTES = [ + private const ALLOWED_ATTRIBUTES = [ 'view', 'create', 'edit', @@ -34,6 +36,13 @@ class CustomerVoter extends AbstractVoter 'details', ]; + private $permissionManager; + + public function __construct(RolePermissionManager $permissionManager) + { + $this->permissionManager = $permissionManager; + } + /** * @param string $attribute * @param Customer $subject @@ -66,7 +75,7 @@ class CustomerVoter extends AbstractVoter return false; } - if ($this->hasRolePermission($user, $attribute . '_customer')) { + if ($this->permissionManager->hasRolePermission($user, $attribute . '_customer')) { return true; } @@ -75,8 +84,8 @@ class CustomerVoter extends AbstractVoter return false; } - $hasTeamleadPermission = $this->hasRolePermission($user, $attribute . '_teamlead_customer'); - $hasTeamPermission = $this->hasRolePermission($user, $attribute . '_team_customer'); + $hasTeamleadPermission = $this->permissionManager->hasRolePermission($user, $attribute . '_teamlead_customer'); + $hasTeamPermission = $this->permissionManager->hasRolePermission($user, $attribute . '_team_customer'); if (!$hasTeamleadPermission && !$hasTeamPermission) { return false; diff --git a/src/Voter/ProjectVoter.php b/src/Voter/ProjectVoter.php index 0eb4db53..7ec7c421 100644 --- a/src/Voter/ProjectVoter.php +++ b/src/Voter/ProjectVoter.php @@ -12,17 +12,19 @@ namespace App\Voter; use App\Entity\Project; use App\Entity\Team; use App\Entity\User; +use App\Security\RolePermissionManager; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\Voter\Voter; /** * A voter to check permissions on Projects. */ -class ProjectVoter extends AbstractVoter +class ProjectVoter extends Voter { /** * support rules based on the given project */ - public const ALLOWED_ATTRIBUTES = [ + private const ALLOWED_ATTRIBUTES = [ 'view', 'edit', 'budget', @@ -33,6 +35,13 @@ class ProjectVoter extends AbstractVoter 'details', ]; + private $permissionManager; + + public function __construct(RolePermissionManager $permissionManager) + { + $this->permissionManager = $permissionManager; + } + /** * @param string $attribute * @param Project $subject @@ -65,7 +74,7 @@ class ProjectVoter extends AbstractVoter return false; } - if ($this->hasRolePermission($user, $attribute . '_project')) { + if ($this->permissionManager->hasRolePermission($user, $attribute . '_project')) { return true; } @@ -74,8 +83,8 @@ class ProjectVoter extends AbstractVoter return false; } - $hasTeamleadPermission = $this->hasRolePermission($user, $attribute . '_teamlead_project'); - $hasTeamPermission = $this->hasRolePermission($user, $attribute . '_team_project'); + $hasTeamleadPermission = $this->permissionManager->hasRolePermission($user, $attribute . '_teamlead_project'); + $hasTeamPermission = $this->permissionManager->hasRolePermission($user, $attribute . '_team_project'); if (!$hasTeamleadPermission && !$hasTeamPermission) { return false; diff --git a/src/Voter/RolePermissionVoter.php b/src/Voter/RolePermissionVoter.php index b6e8e25f..3333e8fd 100644 --- a/src/Voter/RolePermissionVoter.php +++ b/src/Voter/RolePermissionVoter.php @@ -11,13 +11,22 @@ namespace App\Voter; use App\Entity\Activity; use App\Entity\User; +use App\Security\RolePermissionManager; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\Voter\Voter; /** * A voter to check the free-configurable permission from "kimai.permissions". */ -class RolePermissionVoter extends AbstractVoter +class RolePermissionVoter extends Voter { + private $permissionManager; + + public function __construct(RolePermissionManager $permissionManager) + { + $this->permissionManager = $permissionManager; + } + /** * @param string $attribute * @param mixed $subject @@ -30,7 +39,7 @@ class RolePermissionVoter extends AbstractVoter return false; } - return $this->isRegisteredPermission($attribute); + return $this->permissionManager->isRegisteredPermission($attribute); } /** @@ -47,6 +56,6 @@ class RolePermissionVoter extends AbstractVoter return false; } - return $this->hasRolePermission($user, $attribute); + return $this->permissionManager->hasRolePermission($user, $attribute); } } diff --git a/src/Voter/TeamVoter.php b/src/Voter/TeamVoter.php index 7d21c735..edef7581 100644 --- a/src/Voter/TeamVoter.php +++ b/src/Voter/TeamVoter.php @@ -11,19 +11,28 @@ namespace App\Voter; use App\Entity\Team; use App\Entity\User; +use App\Security\RolePermissionManager; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\Voter\Voter; -class TeamVoter extends AbstractVoter +class TeamVoter extends Voter { /** * support rules based on the given $subject (here: Team) */ - public const ALLOWED_ATTRIBUTES = [ + private const ALLOWED_ATTRIBUTES = [ 'view', 'edit', 'delete', ]; + private $permissionManager; + + public function __construct(RolePermissionManager $permissionManager) + { + $this->permissionManager = $permissionManager; + } + /** * @param string $attribute * @param Team $subject @@ -56,6 +65,6 @@ class TeamVoter extends AbstractVoter return false; } - return $this->hasRolePermission($user, $attribute . '_team'); + return $this->permissionManager->hasRolePermission($user, $attribute . '_team'); } } diff --git a/src/Voter/TimesheetVoter.php b/src/Voter/TimesheetVoter.php index 8ea790a0..ddcb624e 100644 --- a/src/Voter/TimesheetVoter.php +++ b/src/Voter/TimesheetVoter.php @@ -11,12 +11,15 @@ namespace App\Voter; use App\Entity\Timesheet; use App\Entity\User; +use App\Security\RolePermissionManager; +use App\Timesheet\LockdownService; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\Voter\Voter; /** * A voter to check permissions on Timesheets. */ -class TimesheetVoter extends AbstractVoter +class TimesheetVoter extends Voter { public const VIEW = 'view'; public const START = 'start'; @@ -31,7 +34,7 @@ class TimesheetVoter extends AbstractVoter /** * support rules based on the given $subject (here: Timesheet) */ - public const ALLOWED_ATTRIBUTES = [ + private const ALLOWED_ATTRIBUTES = [ self::VIEW, self::START, self::STOP, @@ -44,6 +47,20 @@ class TimesheetVoter extends AbstractVoter 'duplicate' ]; + private $permissionManager; + private $lockdownService; + + private $lockdownGrace; + private $lockdownOverride; + private $editExported; + private $now; + + public function __construct(RolePermissionManager $permissionManager, LockdownService $lockdownService) + { + $this->permissionManager = $permissionManager; + $this->lockdownService = $lockdownService; + } + /** * @param string $attribute * @param mixed $subject @@ -101,6 +118,9 @@ class TimesheetVoter extends AbstractVoter break; case 'duplicate': + if (!$this->canDuplicate($user, $subject)) { + return false; + } $permission = self::EDIT; break; @@ -128,7 +148,7 @@ class TimesheetVoter extends AbstractVoter $permission .= '_timesheet'; - return $this->hasRolePermission($user, $permission); + return $this->permissionManager->hasRolePermission($user, $permission); } protected function canStart(Timesheet $timesheet): bool @@ -158,7 +178,11 @@ class TimesheetVoter extends AbstractVoter protected function canEdit(User $user, Timesheet $timesheet): bool { - if ($timesheet->isExported() && !$this->hasRolePermission($user, 'edit_exported_timesheet')) { + if (!$this->isAllowedExported($user, $timesheet)) { + return false; + } + + if (!$this->isAllowedInLockdown($user, $timesheet)) { return false; } @@ -167,10 +191,61 @@ class TimesheetVoter extends AbstractVoter protected function canDelete(User $user, Timesheet $timesheet): bool { - if ($timesheet->isExported() && !$this->hasRolePermission($user, 'edit_exported_timesheet')) { + if (!$this->isAllowedExported($user, $timesheet)) { + return false; + } + + if (!$this->isAllowedInLockdown($user, $timesheet)) { return false; } return true; } + + protected function canDuplicate(User $user, Timesheet $timesheet): bool + { + if (!$this->isAllowedInLockdown($user, $timesheet)) { + return false; + } + + return true; + } + + private function isAllowedExported(User $user, Timesheet $timesheet): bool + { + if (!$timesheet->isExported()) { + return true; + } + + if ($this->editExported === null) { + $this->editExported = $this->permissionManager->hasRolePermission($user, 'edit_exported_timesheet'); + } + + return $this->editExported; + } + + private function isAllowedInLockdown(User $user, Timesheet $timesheet): bool + { + if (!$this->lockdownService->isLockdownActive()) { + return true; + } + + if ($this->lockdownOverride === null) { + $this->lockdownOverride = $this->permissionManager->hasRolePermission($user, 'lockdown_override_timesheet'); + } + + if ($this->lockdownOverride) { + return true; + } + + if ($this->lockdownGrace === null) { + $this->lockdownGrace = $this->permissionManager->hasRolePermission($user, 'lockdown_grace_timesheet'); + } + + if ($this->now === null) { + $this->now = new \DateTime('now', new \DateTimeZone($user->getTimezone())); + } + + return $this->lockdownService->isEditable($timesheet, $this->now, $this->lockdownGrace); + } } diff --git a/src/Voter/UserVoter.php b/src/Voter/UserVoter.php index 218fb051..94f718ca 100644 --- a/src/Voter/UserVoter.php +++ b/src/Voter/UserVoter.php @@ -10,14 +10,16 @@ namespace App\Voter; use App\Entity\User; +use App\Security\RolePermissionManager; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\Voter\Voter; /** * A voter to check permissions on user profiles. */ -class UserVoter extends AbstractVoter +class UserVoter extends Voter { - public const ALLOWED_ATTRIBUTES = [ + private const ALLOWED_ATTRIBUTES = [ 'view', 'edit', 'roles', @@ -29,6 +31,13 @@ class UserVoter extends AbstractVoter 'hourly-rate', ]; + private $permissionManager; + + public function __construct(RolePermissionManager $permissionManager) + { + $this->permissionManager = $permissionManager; + } + /** * @param string $attribute * @param mixed $subject @@ -66,8 +75,10 @@ class UserVoter extends AbstractVoter return false; } - return $this->hasRolePermission($user, 'delete_user'); - } elseif ($attribute === 'password') { + return $this->permissionManager->hasRolePermission($user, 'delete_user'); + } + + if ($attribute === 'password') { if (!$subject->isInternalUser()) { return false; } @@ -84,6 +95,6 @@ class UserVoter extends AbstractVoter $permission .= '_profile'; - return $this->hasRolePermission($user, $permission); + return $this->permissionManager->hasRolePermission($user, $permission); } } diff --git a/tests/Security/RolePermissionManagerTest.php b/tests/Security/RolePermissionManagerTest.php index bd20703f..b1ba83af 100644 --- a/tests/Security/RolePermissionManagerTest.php +++ b/tests/Security/RolePermissionManagerTest.php @@ -9,6 +9,7 @@ namespace App\Tests\Security; +use App\Entity\User; use App\Repository\RolePermissionRepository; use App\Security\RolePermissionManager; use PHPUnit\Framework\TestCase; @@ -89,11 +90,17 @@ class RolePermissionManagerTest extends TestCase 'USER_ROLE' => ['foo', 'bar'] ]); + $user = new User(); + $user->addRole('TEST_ROLE'); + $user->addRole('FFOOOOOO'); + self::assertTrue($sut->isRegisteredPermission('foo')); self::assertTrue($sut->isRegisteredPermission('bar')); self::assertEquals(['role_permissions', 'view_user', 'create_user', 'foo2', 'foo', 'bar'], array_values($sut->getPermissions())); self::assertTrue($sut->hasPermission('TEST_ROLE', 'foo2')); + self::assertTrue($sut->hasRolePermission($user, 'foo2')); + self::assertFalse($sut->hasRolePermission($user, 'foo')); self::assertFalse($sut->hasPermission('TEST_ROLE', 'foo')); self::assertFalse($sut->hasPermission('USER_ROLE', 'foo')); self::assertTrue($sut->hasPermission('USER_ROLE', 'bar')); diff --git a/tests/Timesheet/LockdownServiceTest.php b/tests/Timesheet/LockdownServiceTest.php new file mode 100644 index 00000000..bb37e295 --- /dev/null +++ b/tests/Timesheet/LockdownServiceTest.php @@ -0,0 +1,151 @@ +createMock(ConfigLoaderInterface::class); + $config = new SystemConfiguration($loader, [ + 'timesheet' => [ + 'rules' => [ + 'lockdown_period_start' => $start, + 'lockdown_period_end' => $end, + 'lockdown_grace_period' => $grace, + ], + ] + ]); + + return new LockdownService($config); + } + + public function testValidatorWithoutNowConstraint() + { + $sut = $this->createService('first day of last month', 'last day of last month', '+10 days'); + + $begin = new \DateTime('first day of last month'); + $begin->modify('-5 days'); + $timesheet = new Timesheet(); + $timesheet->setBegin($begin); + + self::assertFalse($sut->isEditable($timesheet, new \DateTime(), false)); + } + + public function testValidatorWithEmptyTimesheet() + { + $sut = $this->createService('first day of last month', 'last day of last month', '+10 days'); + + self::assertTrue($sut->isEditable(new Timesheet(), new \DateTime(), false)); + } + + public function testValidatorWithoutNowStringConstraint() + { + $sut = $this->createService('first day of last month', 'last day of last month', '+10 days'); + + $begin = new \DateTime('first day of last month'); + $begin->modify('+5 days'); + $timesheet = new Timesheet(); + $timesheet->setBegin($begin); + + self::assertTrue($sut->isEditable($timesheet, new \DateTime('first day of this month'), false)); + } + + public function testValidatorWithEndBeforeStartPeriod() + { + $sut = $this->createService('first day of this month', 'last day of last month', '+10 days'); + + $begin = new \DateTime('first day of last month'); + $begin->modify('+5 days'); + $timesheet = new Timesheet(); + $timesheet->setBegin($begin); + + self::assertTrue($sut->isEditable($timesheet, new \DateTime('first day of this month'), false)); + } + + /** + * @dataProvider getTestData + */ + public function testLockdown(bool $allowOverwriteGrace, string $beginModifier, string $nowModifier, bool $isViolation) + { + $sut = $this->createService('first day of last month', 'last day of last month', '+10 days'); + + $begin = new \DateTime('first day of last month'); + $begin->modify($beginModifier); + $timesheet = new Timesheet(); + $timesheet->setBegin($begin); + + $now = new \DateTime('first day of this month'); + $now->modify($nowModifier); + + $result = $sut->isEditable($timesheet, $now, $allowOverwriteGrace); + if ($isViolation) { + self::assertFalse($result); + } else { + self::assertTrue($result); + } + } + + public function getTestData() + { + // changing before last dockdown period is not allowed + yield [false, '-5 days', '+5 days', true]; + // changing before last dockdown period is not allowed with grace permission + yield [true, '-5 days', '+5 days', true]; + // changing a value in the last lockdown period is allowed during grace period + yield [false, '+5 days', '+5 days', false]; + // changing outside grace period is not allowed + yield [false, '+5 days', '+11 days', true]; + // changing outside grace period is allowed with grace and full permission + yield [true, '+5 days', '+11 days', false]; + } + + /** + * @dataProvider getConfigTestData + */ + public function testLockdownConfig(bool $allowOverwriteGrace, ?string $lockdownBegin, ?string $lockdownEnd, ?string $grace, bool $isViolation) + { + $sut = $this->createService($lockdownBegin, $lockdownEnd, $grace); + + $begin = new \DateTime('first day of last month'); + $begin->modify('+5 days'); + $timesheet = new Timesheet(); + $timesheet->setBegin($begin); + + $now = new \DateTime('first day of this month'); + + $result = $sut->isEditable($timesheet, $now, $allowOverwriteGrace); + + if ($isViolation) { + self::assertFalse($result); + } else { + self::assertTrue($result); + } + } + + public function getConfigTestData() + { + yield [false, null, null, null, false]; + yield [false, '+5 days', null, null, false]; + yield [false, null, '+5 days', null, false]; + + yield [true, 'öööö', '+11 days', null, false]; + yield [true, '+5 days', '+5 of !!!!', null, false]; + } +} diff --git a/tests/Validator/Constraints/TimesheetLockdownValidatorTest.php b/tests/Validator/Constraints/TimesheetLockdownValidatorTest.php index 690a3ee0..36196f0c 100644 --- a/tests/Validator/Constraints/TimesheetLockdownValidatorTest.php +++ b/tests/Validator/Constraints/TimesheetLockdownValidatorTest.php @@ -12,6 +12,7 @@ namespace App\Tests\Validator\Constraints; use App\Configuration\ConfigLoaderInterface; use App\Configuration\SystemConfiguration; use App\Entity\Timesheet; +use App\Timesheet\LockdownService; use App\Validator\Constraints\TimesheetLockdown; use App\Validator\Constraints\TimesheetLockdownValidator; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; @@ -56,7 +57,7 @@ class TimesheetLockdownValidatorTest extends ConstraintValidatorTestCase ] ]); - return new TimesheetLockdownValidator($auth, $config); + return new TimesheetLockdownValidator($auth, new LockdownService($config)); } public function testConstraintIsInvalid() diff --git a/tests/Voter/AbstractVoterTest.php b/tests/Voter/AbstractVoterTest.php index cb49b9c5..60e5147c 100644 --- a/tests/Voter/AbstractVoterTest.php +++ b/tests/Voter/AbstractVoterTest.php @@ -11,29 +11,18 @@ namespace App\Tests\Voter; use App\Entity\User; use App\Repository\RolePermissionRepository; -use App\Security\AclDecisionManager; use App\Security\RolePermissionManager; -use App\Voter\AbstractVoter; use PHPUnit\Framework\TestCase; +use Symfony\Component\Security\Core\Authorization\Voter\Voter; abstract class AbstractVoterTest extends TestCase { - /** - * @param string $voterClass - * @param User $user - * @return AbstractVoter - * @throws \ReflectionException - */ - protected function getVoter(string $voterClass, User $user) + protected function getVoter(string $voterClass): Voter { - $isAuthenticated = empty($user->getRoles()); - $accessManager = $this->getMockBuilder(AclDecisionManager::class)->disableOriginalConstructor()->getMock(); - $accessManager->method('isFullyAuthenticated')->willReturn($isAuthenticated); - $class = new \ReflectionClass($voterClass); - /** @var AbstractVoter $voter */ - $voter = $class->newInstance($accessManager, $this->getRolePermissionManager()); - self::assertInstanceOf(AbstractVoter::class, $voter); + /** @var Voter $voter */ + $voter = $class->newInstance($this->getRolePermissionManager()); + self::assertInstanceOf(Voter::class, $voter); return $voter; } diff --git a/tests/Voter/ActivityVoterTest.php b/tests/Voter/ActivityVoterTest.php index 3d80c9ef..2c1cff5a 100644 --- a/tests/Voter/ActivityVoterTest.php +++ b/tests/Voter/ActivityVoterTest.php @@ -34,7 +34,7 @@ class ActivityVoterTest extends AbstractVoterTest protected function assertVote(User $user, $subject, $attribute, $result) { $token = new UsernamePasswordToken($user, 'foo', 'bar', $user->getRoles()); - $sut = $this->getVoter(ActivityVoter::class, $user); + $sut = $this->getVoter(ActivityVoter::class); $this->assertEquals($result, $sut->vote($token, $subject, [$attribute])); } diff --git a/tests/Voter/CustomerVoterTest.php b/tests/Voter/CustomerVoterTest.php index 0cd3ee41..e8c9c80b 100644 --- a/tests/Voter/CustomerVoterTest.php +++ b/tests/Voter/CustomerVoterTest.php @@ -24,7 +24,7 @@ class CustomerVoterTest extends AbstractVoterTest protected function assertVote(User $user, $subject, $attribute, $result) { $token = new UsernamePasswordToken($user, 'foo', 'bar', $user->getRoles()); - $sut = $this->getVoter(CustomerVoter::class, $user); + $sut = $this->getVoter(CustomerVoter::class); $actual = $sut->vote($token, $subject, [$attribute]); $this->assertEquals($result, $actual, sprintf('Failed voting "%s" for User with roles %s.', $attribute, implode(', ', $user->getRoles()))); diff --git a/tests/Voter/DeprecatedAbstractVoterTest.php b/tests/Voter/DeprecatedAbstractVoterTest.php new file mode 100644 index 00000000..31947ef1 --- /dev/null +++ b/tests/Voter/DeprecatedAbstractVoterTest.php @@ -0,0 +1,80 @@ +getMockBuilder(AclDecisionManager::class)->disableOriginalConstructor()->getMock(); + $accessManager->method('isFullyAuthenticated')->willReturn(true); + + $class = new \ReflectionClass($voterClass); + /** @var AbstractVoter $voter */ + $voter = $class->newInstance($accessManager, $this->getRolePermissionManager()); + self::assertInstanceOf(AbstractVoter::class, $voter); + + return $voter; + } + + protected function assertVote(User $user, $subject, $attribute, $result) + { + $token = new UsernamePasswordToken($user, 'foo', 'bar', $user->getRoles()); + $sut = $this->getVoter(DeprecatedVoter::class); + + $actual = $sut->vote($token, $subject, [$attribute]); + $this->assertEquals($result, $actual, sprintf('Failed voting "%s" for User with roles %s.', $attribute, implode(', ', $user->getRoles()))); + } + + public function testMuuu() + { + $userStandard = $this->getUser(1, User::ROLE_USER); + $this->assertVote($userStandard, null, 'view_own_timesheet', true); + } +} + +class DeprecatedVoter extends AbstractVoter +{ + protected function supports($attribute, $subject) + { + return true; + } + + protected function voteOnAttribute($attribute, $subject, TokenInterface $token) + { + if (!$this->isRegisteredPermission($attribute)) { + return false; + } + + if (!$this->hasPermission('ROLE_USER', $attribute)) { + return false; + } + + /** @var User $user */ + $user = $token->getUser(); + + if (!$this->hasRolePermission($user, $attribute)) { + return false; + } + + return $this->isFullyAuthenticated($token); + } +} diff --git a/tests/Voter/ProjectVoterTest.php b/tests/Voter/ProjectVoterTest.php index 5913339a..6ae1404a 100644 --- a/tests/Voter/ProjectVoterTest.php +++ b/tests/Voter/ProjectVoterTest.php @@ -25,7 +25,7 @@ class ProjectVoterTest extends AbstractVoterTest protected function assertVote(User $user, $subject, $attribute, $result) { $token = new UsernamePasswordToken($user, 'foo', 'bar', $user->getRoles()); - $sut = $this->getVoter(ProjectVoter::class, $user); + $sut = $this->getVoter(ProjectVoter::class); if ($subject instanceof Project && null === $subject->getCustomer()) { $subject->setCustomer(new Customer()); diff --git a/tests/Voter/RolePermissionVoterTest.php b/tests/Voter/RolePermissionVoterTest.php index f01d7c69..591d1061 100644 --- a/tests/Voter/RolePermissionVoterTest.php +++ b/tests/Voter/RolePermissionVoterTest.php @@ -26,7 +26,7 @@ class RolePermissionVoterTest extends AbstractVoterTest public function testVote(User $user, $subject, $attribute, $result) { $token = new UsernamePasswordToken($user, 'foo', 'bar', $user->getRoles()); - $sut = $this->getVoter(RolePermissionVoter::class, $user); + $sut = $this->getVoter(RolePermissionVoter::class); $actual = $sut->vote($token, $subject, [$attribute]); $this->assertEquals($result, $actual, sprintf('Failed voting "%s" for User with roles %s.', $attribute, implode(', ', $user->getRoles()))); diff --git a/tests/Voter/TeamVoterTest.php b/tests/Voter/TeamVoterTest.php index 29f32dfc..6f3b6388 100644 --- a/tests/Voter/TeamVoterTest.php +++ b/tests/Voter/TeamVoterTest.php @@ -26,7 +26,7 @@ class TeamVoterTest extends AbstractVoterTest public function testVote(User $user, $subject, $attribute, $result) { $token = new UsernamePasswordToken($user, 'foo', 'bar', $user->getRoles()); - $sut = $this->getVoter(TeamVoter::class, $user); + $sut = $this->getVoter(TeamVoter::class); $this->assertEquals($result, $sut->vote($token, $subject, [$attribute])); } diff --git a/tests/Voter/TimesheetVoterTest.php b/tests/Voter/TimesheetVoterTest.php index 2cff09e5..e981855c 100644 --- a/tests/Voter/TimesheetVoterTest.php +++ b/tests/Voter/TimesheetVoterTest.php @@ -9,13 +9,17 @@ namespace App\Tests\Voter; +use App\Configuration\ConfigLoaderInterface; +use App\Configuration\SystemConfiguration; use App\Entity\Activity; use App\Entity\Customer; use App\Entity\Project; use App\Entity\Timesheet; use App\Entity\User; +use App\Timesheet\LockdownService; use App\Voter\TimesheetVoter; use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken; +use Symfony\Component\Security\Core\Authorization\Voter\Voter; use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; /** @@ -23,10 +27,15 @@ use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; */ class TimesheetVoterTest extends AbstractVoterTest { + protected function getVoter(string $voterClass): Voter + { + return $this->getLockdownVoter(); + } + protected function assertVote(User $user, $subject, $attribute, $result) { $token = new UsernamePasswordToken($user, 'foo', 'bar', $user->getRoles()); - $sut = $this->getVoter(TimesheetVoter::class, $user); + $sut = $this->getVoter(TimesheetVoter::class); $this->assertEquals($result, $sut->vote($token, $subject, [$attribute])); } @@ -87,6 +96,36 @@ class TimesheetVoterTest extends AbstractVoterTest } } + /** + * @dataProvider getLockDownTestData + */ + public function testWithLockdown(string $permission, int $expected, string $beginModifier, string $lockdownBegin, string $lockdownEnd, ?string $lockdownGrace) + { + $user = $this->getUser(1, User::ROLE_USER); + + $begin = new \DateTime('now'); + $begin->modify($beginModifier); + + $timesheet = new Timesheet(); + $timesheet->setBegin($begin); + $timesheet->setUser($user); + + $token = new UsernamePasswordToken($user, 'foo', 'bar', $user->getRoles()); + $sut = $this->getLockdownVoter($lockdownBegin, $lockdownEnd, $lockdownGrace); + + self::assertEquals($expected, $sut->vote($token, $timesheet, [$permission])); + } + + public function getLockDownTestData() + { + yield ['view', VoterInterface::ACCESS_GRANTED, '+1 days', 'first day of this month', 'last day of this month', '+10 days']; + yield ['duplicate', VoterInterface::ACCESS_GRANTED, '+1 days', 'first day of this month', 'last day of this month', '+10 days']; + yield ['delete', VoterInterface::ACCESS_GRANTED, '+1 days', 'first day of this month', 'last day of this month', '+10 days']; + yield ['edit', VoterInterface::ACCESS_DENIED, '-50 days', 'first day of last month', 'last day of last month', '+1 days']; + yield ['duplicate', VoterInterface::ACCESS_DENIED, '-50 days', 'first day of last month', 'last day of last month', '+1 days']; + yield ['delete', VoterInterface::ACCESS_DENIED, '-50 days', 'first day of last month', 'last day of last month', '+1 days']; + } + public function testSpecialCases() { $user1 = $this->getUser(1, User::ROLE_USER); @@ -154,10 +193,30 @@ class TimesheetVoterTest extends AbstractVoterTest */ protected function getUser($id, $role) { - $user = $this->getMockBuilder(User::class)->getMock(); + $user = $this->createMock(User::class); $user->method('getId')->willReturn($id); $user->method('getRoles')->willReturn([$role]); + $user->method('getTimezone')->willReturn(date_default_timezone_get()); return $user; } + + protected function getLockdownVoter(?string $lockdownBegin = null, ?string $lockdownEnd = null, ?string $lockdownGrace = null): Voter + { + $loader = $this->createMock(ConfigLoaderInterface::class); + $config = new SystemConfiguration($loader, [ + 'timesheet' => [ + 'rules' => [ + 'lockdown_period_start' => $lockdownBegin, + 'lockdown_period_end' => $lockdownEnd, + 'lockdown_grace_period' => $lockdownGrace, + ], + ] + ]); + + $voter = new TimesheetVoter($this->getRolePermissionManager(), new LockdownService($config)); + self::assertInstanceOf(Voter::class, $voter); + + return $voter; + } } diff --git a/tests/Voter/UserVoterTest.php b/tests/Voter/UserVoterTest.php index 636679a8..466d31de 100644 --- a/tests/Voter/UserVoterTest.php +++ b/tests/Voter/UserVoterTest.php @@ -26,7 +26,7 @@ class UserVoterTest extends AbstractVoterTest public function testVote(User $user, $subject, $attribute, $result) { $token = new UsernamePasswordToken($user, 'foo', 'bar', $user->getRoles()); - $sut = $this->getVoter(UserVoter::class, $user); + $sut = $this->getVoter(UserVoter::class); $this->assertEquals($result, $sut->vote($token, $subject, [$attribute])); }