From 682681afa58f7d4584026a87aa0941179809473f Mon Sep 17 00:00:00 2001 From: Kevin Papst Date: Sat, 4 Jul 2020 13:29:03 +0200 Subject: [PATCH] added domain logic for permission checks (#1803) --- src/Entity/Activity.php | 1 - src/Entity/Customer.php | 1 - src/Entity/EntityWithMetaFields.php | 1 - src/Entity/Project.php | 1 - src/Entity/Timesheet.php | 1 - src/Entity/User.php | 10 ++++++++++ .../AjaxAuthenticationSubscriber.php | 2 +- src/Form/Type/TeamMemberType.php | 2 +- src/Repository/ActivityRepository.php | 2 +- src/Repository/CustomerRepository.php | 2 +- src/Repository/InvoiceRepository.php | 2 +- src/Repository/ProjectRepository.php | 2 +- src/Repository/TeamRepository.php | 2 +- src/Repository/TimesheetRepository.php | 2 +- src/Repository/UserRepository.php | 2 +- tests/Entity/UserTest.php | 20 +++++++++++++++++++ 16 files changed, 39 insertions(+), 14 deletions(-) diff --git a/src/Entity/Activity.php b/src/Entity/Activity.php index 8e95014d..616d1e3b 100644 --- a/src/Entity/Activity.php +++ b/src/Entity/Activity.php @@ -150,7 +150,6 @@ class Activity implements EntityWithMetaFields } /** - * @internal only here for symfony forms * @return Collection|MetaTableTypeInterface[] */ public function getMetaFields(): Collection diff --git a/src/Entity/Customer.php b/src/Entity/Customer.php index ed3b43fe..d0d30b65 100644 --- a/src/Entity/Customer.php +++ b/src/Entity/Customer.php @@ -393,7 +393,6 @@ class Customer implements EntityWithMetaFields } /** - * @internal only here for symfony forms * @return Collection|MetaTableTypeInterface[] */ public function getMetaFields(): Collection diff --git a/src/Entity/EntityWithMetaFields.php b/src/Entity/EntityWithMetaFields.php index 4cb5c899..e138065c 100644 --- a/src/Entity/EntityWithMetaFields.php +++ b/src/Entity/EntityWithMetaFields.php @@ -14,7 +14,6 @@ use Doctrine\Common\Collections\Collection; interface EntityWithMetaFields { /** - * @internal only here for symfony forms * @return Collection|MetaTableTypeInterface[] */ public function getMetaFields(): Collection; diff --git a/src/Entity/Project.php b/src/Entity/Project.php index 6d655d3e..ffb20c04 100644 --- a/src/Entity/Project.php +++ b/src/Entity/Project.php @@ -298,7 +298,6 @@ class Project implements EntityWithMetaFields } /** - * @internal only here for symfony forms * @return Collection|MetaTableTypeInterface[] */ public function getMetaFields(): Collection diff --git a/src/Entity/Timesheet.php b/src/Entity/Timesheet.php index a522fe2c..cbece5b8 100644 --- a/src/Entity/Timesheet.php +++ b/src/Entity/Timesheet.php @@ -509,7 +509,6 @@ class Timesheet implements EntityWithMetaFields, ExportItemInterface } /** - * @internal only here for symfony forms * @return Collection|MetaTableTypeInterface[] */ public function getMetaFields(): Collection diff --git a/src/Entity/User.php b/src/Entity/User.php index 585ee071..43233daa 100644 --- a/src/Entity/User.php +++ b/src/Entity/User.php @@ -354,6 +354,11 @@ class User extends BaseUser implements UserInterface $team->removeUser($this); } + public function hasTeamAssignment(): bool + { + return !$this->getTeams()->isEmpty(); + } + /** * @return Collection */ @@ -372,6 +377,11 @@ class User extends BaseUser implements UserInterface return $team->getTeamLead() === $this; } + public function canSeeAllData(): bool + { + return $this->isSuperAdmin() || $this->isAdmin(); + } + public function isTeamlead(): bool { return $this->hasRole(static::ROLE_TEAMLEAD); diff --git a/src/EventSubscriber/AjaxAuthenticationSubscriber.php b/src/EventSubscriber/AjaxAuthenticationSubscriber.php index 78b6e326..1ce0bed0 100644 --- a/src/EventSubscriber/AjaxAuthenticationSubscriber.php +++ b/src/EventSubscriber/AjaxAuthenticationSubscriber.php @@ -31,7 +31,7 @@ class AjaxAuthenticationSubscriber implements EventSubscriberInterface $request = $event->getRequest(); if ($request->isXmlHttpRequest()) { - $exception = $event->getException(); + $exception = $event->getThrowable(); if ($exception instanceof AuthenticationExpiredException) { $event->setResponse(new Response('Session expired', 403, ['Login-Required' => true])); } elseif ($exception instanceof AuthenticationException) { diff --git a/src/Form/Type/TeamMemberType.php b/src/Form/Type/TeamMemberType.php index e20f5848..813f54a3 100644 --- a/src/Form/Type/TeamMemberType.php +++ b/src/Form/Type/TeamMemberType.php @@ -45,7 +45,7 @@ class TeamMemberType extends AbstractType /** @var User $user */ $user = $options['user']; - if (null !== $user && !$user->getTeams()->isEmpty() && !$user->isSuperAdmin() && !$user->isAdmin()) { + if (null !== $user && $user->hasTeamAssignment() && !$user->canSeeAllData()) { $qb ->leftJoin('u.teams', 'teams') ->leftJoin('teams.users', 'users') diff --git a/src/Repository/ActivityRepository.php b/src/Repository/ActivityRepository.php index 50f7a4c0..19680317 100644 --- a/src/Repository/ActivityRepository.php +++ b/src/Repository/ActivityRepository.php @@ -122,7 +122,7 @@ class ActivityRepository extends EntityRepository } // make sure that admins see all activities - if (null !== $user && ($user->isSuperAdmin() || $user->isAdmin())) { + if (null !== $user && $user->canSeeAllData()) { return; } diff --git a/src/Repository/CustomerRepository.php b/src/Repository/CustomerRepository.php index 2d197598..b21254bf 100644 --- a/src/Repository/CustomerRepository.php +++ b/src/Repository/CustomerRepository.php @@ -139,7 +139,7 @@ class CustomerRepository extends EntityRepository } // make sure that admins see all customers - if (null !== $user && ($user->isSuperAdmin() || $user->isAdmin())) { + if (null !== $user && $user->canSeeAllData()) { return; } diff --git a/src/Repository/InvoiceRepository.php b/src/Repository/InvoiceRepository.php index a885add8..53c5966a 100644 --- a/src/Repository/InvoiceRepository.php +++ b/src/Repository/InvoiceRepository.php @@ -92,7 +92,7 @@ class InvoiceRepository extends EntityRepository } // make sure that admins see all projects - if (null !== $user && ($user->isSuperAdmin() || $user->isAdmin())) { + if (null !== $user && $user->canSeeAllData()) { return; } diff --git a/src/Repository/ProjectRepository.php b/src/Repository/ProjectRepository.php index b4aeacd2..90d97c47 100644 --- a/src/Repository/ProjectRepository.php +++ b/src/Repository/ProjectRepository.php @@ -131,7 +131,7 @@ class ProjectRepository extends EntityRepository } // make sure that admins see all projects - if (null !== $user && ($user->isSuperAdmin() || $user->isAdmin())) { + if (null !== $user && $user->canSeeAllData()) { return; } diff --git a/src/Repository/TeamRepository.php b/src/Repository/TeamRepository.php index 10b446db..db12e7b1 100644 --- a/src/Repository/TeamRepository.php +++ b/src/Repository/TeamRepository.php @@ -174,7 +174,7 @@ class TeamRepository extends EntityRepository } // make sure that admins see all user - if (null !== $user && ($user->isSuperAdmin() || $user->isAdmin())) { + if (null !== $user && $user->canSeeAllData()) { return; } diff --git a/src/Repository/TimesheetRepository.php b/src/Repository/TimesheetRepository.php index effd070a..29f5705b 100644 --- a/src/Repository/TimesheetRepository.php +++ b/src/Repository/TimesheetRepository.php @@ -549,7 +549,7 @@ class TimesheetRepository extends EntityRepository } // make sure that admins see all timesheet records - if (null !== $user && ($user->isSuperAdmin() || $user->isAdmin())) { + if (null !== $user && $user->canSeeAllData()) { return; } diff --git a/src/Repository/UserRepository.php b/src/Repository/UserRepository.php index fdeeeef8..c448dab4 100644 --- a/src/Repository/UserRepository.php +++ b/src/Repository/UserRepository.php @@ -159,7 +159,7 @@ class UserRepository extends EntityRepository implements UserLoaderInterface } // make sure that admins see all user - if (null !== $user && ($user->isSuperAdmin() || $user->isAdmin())) { + if (null !== $user && $user->canSeeAllData()) { return; } diff --git a/tests/Entity/UserTest.php b/tests/Entity/UserTest.php index 4f482929..895dc57c 100644 --- a/tests/Entity/UserTest.php +++ b/tests/Entity/UserTest.php @@ -32,6 +32,8 @@ class UserTest extends TestCase self::assertNull($user->getApiToken()); self::assertNull($user->getPlainApiToken()); self::assertEquals(User::DEFAULT_LANGUAGE, $user->getLocale()); + self::assertFalse($user->hasTeamAssignment()); + self::assertFalse($user->canSeeAllData()); $user->setAvatar('https://www.gravatar.com/avatar/00000000000000000000000000000000?d=retro&f=y'); self::assertEquals('https://www.gravatar.com/avatar/00000000000000000000000000000000?d=retro&f=y', $user->getAvatar()); @@ -145,6 +147,7 @@ class UserTest extends TestCase self::assertCount(1, $sut->getTeams()); self::assertSame($team, $sut->getTeams()[0]); self::assertSame($sut, $team->getUsers()[0]); + self::assertTrue($sut->hasTeamAssignment()); self::assertFalse($sut->isTeamleadOf($team)); self::assertTrue($sut->isInTeam($team)); @@ -160,18 +163,35 @@ class UserTest extends TestCase self::assertCount(2, $sut->getTeams()); $sut->removeTeam($team); self::assertCount(1, $sut->getTeams()); + self::assertTrue($sut->hasTeamAssignment()); $sut->removeTeam($team2); self::assertCount(0, $sut->getTeams()); + self::assertFalse($sut->hasTeamAssignment()); } public function testRoles() { $sut = new User(); + self::assertFalse($sut->canSeeAllData()); + self::assertFalse($sut->isAdmin()); self::assertFalse($sut->isTeamlead()); + $sut->addRole(User::ROLE_ADMIN); + self::assertTrue($sut->canSeeAllData()); + self::assertTrue($sut->isAdmin()); self::assertFalse($sut->isTeamlead()); + $sut->addRole(User::ROLE_TEAMLEAD); self::assertTrue($sut->isTeamlead()); + + $sut->removeRole(User::ROLE_ADMIN); + self::assertFalse($sut->canSeeAllData()); + self::assertFalse($sut->isAdmin()); + + $sut->addRole(User::ROLE_SUPER_ADMIN); + self::assertTrue($sut->canSeeAllData()); + self::assertFalse($sut->isAdmin()); + self::assertTrue($sut->isSuperAdmin()); } /**