diff --git a/UPGRADING.md b/UPGRADING.md index 85b19995..aaa8e540 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -14,6 +14,8 @@ Perform EACH version specific task between your version and the new one, otherwi - Invoice renderer `CSV` was removed - Sessions are now stored in the database (all users have to re-login after upgrade) +- New permissions: `lockdown_grace_timesheet`, `lockdown_override_timesheet`, `view_all_data` +- Fixed team permissions on user queries: depending on your previous team & permission setup your users might see less data (SUPER_ADMINS see all data, but new: ADMINS only see all data if they own the `view_all_data` permission) ### Developer diff --git a/config/packages/kimai.yaml b/config/packages/kimai.yaml index 793d4cb4..fddec87f 100644 --- a/config/packages/kimai.yaml +++ b/config/packages/kimai.yaml @@ -111,8 +111,8 @@ kimai: # some single default definitions for roles SINGLE_USER: ['view_team_member','budget_team_project'] SINGLE_TEAMLEAD: ['view_rate_own_timesheet','view_rate_other_timesheet','hourly-rate_own_profile','view_team_member'] - SINGLE_ADMIN: ['hourly-rate_own_profile','edit_exported_timesheet','teams_own_profile','view_team_member'] - SINGLE_SUPER_ADMIN: ['hourly-rate_own_profile','hourly-rate_other_profile','roles_own_profile','system_information','system_configuration','plugins','edit_exported_timesheet','teams_own_profile','view_team_member','upload_invoice_template'] + SINGLE_ADMIN: ['hourly-rate_own_profile','edit_exported_timesheet','teams_own_profile','view_team_member','view_all_data'] + SINGLE_SUPER_ADMIN: ['hourly-rate_own_profile','hourly-rate_other_profile','roles_own_profile','system_information','system_configuration','plugins','edit_exported_timesheet','teams_own_profile','view_team_member','upload_invoice_template','view_all_data'] # link above sets to one complete set for each user role ROLE_USER: ['@TIMESHEET','@PROFILE','@SINGLE_USER'] ROLE_TEAMLEAD: ['@ACTIVITIES_TEAMLEAD','@PROJECTS_TEAMLEAD','@CUSTOMERS_TEAMLEAD','@TIMESHEET_OTHER','@INVOICE','@TIMESHEET','@PROFILE','@EXPORT','@TAGS','@SINGLE_TEAMLEAD'] diff --git a/src/Controller/PermissionController.php b/src/Controller/PermissionController.php index e0ff56f0..2ef7ebf3 100644 --- a/src/Controller/PermissionController.php +++ b/src/Controller/PermissionController.php @@ -11,6 +11,7 @@ namespace App\Controller; use App\Entity\Role; use App\Entity\RolePermission; +use App\Entity\User; use App\Event\PermissionSectionsEvent; use App\Event\PermissionsEvent; use App\Form\RoleType; @@ -157,6 +158,7 @@ final class PermissionController extends AbstractController 'sorted' => $event->getPermissions(), 'manager' => $this->manager, 'system_roles' => $this->roleService->getSystemRoles(), + 'always_apply_superadmin' => RolePermissionManager::SUPER_ADMIN_PERMISSIONS, ]); } @@ -219,12 +221,16 @@ final class PermissionController extends AbstractController * @Route(path="/roles/{id}/{name}/{value}", name="admin_user_permission_save", methods={"GET"}) * @Security("is_granted('role_permissions')") */ - public function savePermission(Role $role, string $name, string $value, RolePermissionRepository $rolePermissionRepository): Response + public function savePermission(Role $role, string $name, bool $value, RolePermissionRepository $rolePermissionRepository): Response { if (!$this->manager->isRegisteredPermission($name)) { throw $this->createNotFoundException('Unknown permission: ' . $name); } + if (false === $value && $role->getName() === User::ROLE_SUPER_ADMIN && \in_array($name, RolePermissionManager::SUPER_ADMIN_PERMISSIONS)) { + throw $this->createAccessDeniedException(sprintf('Permission "%s" cannot be deactivated for role "%s"', $name, $role->getName())); + } + try { $permission = $rolePermissionRepository->findRolePermission($role, $name); if (null === $permission) { diff --git a/src/Entity/User.php b/src/Entity/User.php index 43233daa..32002a47 100644 --- a/src/Entity/User.php +++ b/src/Entity/User.php @@ -126,6 +126,13 @@ class User extends BaseUser implements UserInterface */ private $auth = self::AUTH_INTERNAL; + /** + * This flag will be initialized in UserEnvironmentSubscriber. + * + * @var bool|null + */ + private $isAllowedToSeeAllData = null; + /** * User constructor. */ @@ -379,7 +386,27 @@ class User extends BaseUser implements UserInterface public function canSeeAllData(): bool { - return $this->isSuperAdmin() || $this->isAdmin(); + return $this->isSuperAdmin() || true === $this->isAllowedToSeeAllData; + } + + /** + * This method should not be called by plugins and returns true on success or false on a failure. + * + * @internal immutable property that cannot be set by plugins + * @param bool $canSeeAllData + * @return bool + * @throws \Exception + */ + public function initCanSeeAllData(bool $canSeeAllData): bool + { + // prevent manipulation from plugins + if (null !== $this->isAllowedToSeeAllData) { + return false; + } + + $this->isAllowedToSeeAllData = $canSeeAllData; + + return true; } public function isTeamlead(): bool diff --git a/src/EventSubscriber/UserEnvironmentSubscriber.php b/src/EventSubscriber/UserEnvironmentSubscriber.php index 687079c9..4e2dee4d 100644 --- a/src/EventSubscriber/UserEnvironmentSubscriber.php +++ b/src/EventSubscriber/UserEnvironmentSubscriber.php @@ -14,6 +14,7 @@ use Symfony\Component\EventDispatcher\EventSubscriberInterface; use Symfony\Component\HttpKernel\Event\RequestEvent; use Symfony\Component\HttpKernel\KernelEvents; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; +use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; class UserEnvironmentSubscriber implements EventSubscriberInterface { @@ -21,21 +22,30 @@ class UserEnvironmentSubscriber implements EventSubscriberInterface * @var TokenStorageInterface */ private $storage; + /** + * @var AuthorizationCheckerInterface + */ + private $auth; - public function __construct(TokenStorageInterface $tokenStorage) + public function __construct(TokenStorageInterface $tokenStorage, AuthorizationCheckerInterface $auth) { $this->storage = $tokenStorage; + $this->auth = $auth; } public static function getSubscribedEvents(): array { return [ - KernelEvents::REQUEST => ['prepareEnvironment', 100], + KernelEvents::REQUEST => ['prepareEnvironment', -100], ]; } public function prepareEnvironment(RequestEvent $event) { + if (!$event->isMasterRequest()) { + return; + } + if (null === $this->storage->getToken()) { return; } @@ -45,6 +55,7 @@ class UserEnvironmentSubscriber implements EventSubscriberInterface if ($user instanceof User) { date_default_timezone_set($user->getTimezone()); \Locale::setDefault($user->getLocale()); + $user->initCanSeeAllData($this->auth->isGranted('view_all_data')); } } } diff --git a/src/Repository/RoleRepository.php b/src/Repository/RoleRepository.php index c7dd9c10..5561cb29 100644 --- a/src/Repository/RoleRepository.php +++ b/src/Repository/RoleRepository.php @@ -13,16 +13,11 @@ use App\Entity\Role; use Doctrine\ORM\EntityRepository; use Doctrine\ORM\ORMException; +/** + * @method Role[] findAll() + */ class RoleRepository extends EntityRepository { - /** - * @return Role[] - */ - public function findAll() - { - return parent::findAll(); - } - public function saveRole(Role $role) { $entityManager = $this->getEntityManager(); diff --git a/src/Repository/UserRepository.php b/src/Repository/UserRepository.php index c448dab4..1149e726 100644 --- a/src/Repository/UserRepository.php +++ b/src/Repository/UserRepository.php @@ -163,11 +163,30 @@ class UserRepository extends EntityRepository implements UserLoaderInterface return; } + $or = $qb->expr()->orX(); + + // if no explicit team was requested and the user is part of some teams + // then find all members of teams where he is teamlead + if (null !== $user && $user->hasTeamAssignment()) { + $qb->leftJoin('u.teams', 't'); + $or->add($qb->expr()->eq('t.teamlead', ':teamlead')); + $qb->setParameter('teamlead', $user); + } + + // if teams where requested, then select all team members + if (\count($teams) > 0) { + $or->add($qb->expr()->isMemberOf(':teams', 'u.teams')); + $qb->setParameter('teams', $teams); + } + + // and make sure, that the user himself is always returned if (null !== $user) { - $qb->leftJoin('u.teams', 'teams') - ->leftJoin('teams.users', 'users') - ->andWhere('teams.teamlead = :id') - ->setParameter('id', $user); + $or->add($qb->expr()->eq('u.id', ':user')); + $qb->setParameter('user', $user); + } + + if ($or->count() > 0) { + $qb->andWhere($or); } } @@ -203,10 +222,12 @@ class UserRepository extends EntityRepository implements UserLoaderInterface ->orderBy('u.' . $query->getOrderBy(), $query->getOrder()) ; - if (UserQuery::SHOW_VISIBLE == $query->getVisibility()) { + $this->addPermissionCriteria($qb, $query->getCurrentUser(), $query->getTeams()); + + if ($query->isShowVisible()) { $qb->andWhere($qb->expr()->eq('u.enabled', ':enabled')); $qb->setParameter('enabled', true, \PDO::PARAM_BOOL); - } elseif (UserQuery::SHOW_HIDDEN == $query->getVisibility()) { + } elseif ($query->isShowHidden()) { $qb->andWhere($qb->expr()->eq('u.enabled', ':enabled')); $qb->setParameter('enabled', false, \PDO::PARAM_BOOL); } diff --git a/src/Security/RolePermissionManager.php b/src/Security/RolePermissionManager.php index 72b73a82..565a984b 100644 --- a/src/Security/RolePermissionManager.php +++ b/src/Security/RolePermissionManager.php @@ -14,6 +14,17 @@ use App\Repository\RolePermissionRepository; final class RolePermissionManager { + /** + * Permissions that are always true for ROLE_SUPER_ADMIN, no matter what is inside the database. + * + * @var string[] + */ + public const SUPER_ADMIN_PERMISSIONS = [ + 'view_all_data', + 'role_permissions', + 'view_user' + ]; + /** * @var array */ @@ -36,23 +47,22 @@ final class RolePermissionManager foreach ($all as $item) { $perm = $item['permission']; $role = strtoupper($item['role']); - $isAllowed = $item['allowed']; + $isAllowed = (bool) $item['allowed']; - // see permissions.html.twig for this special case - if ($role === User::ROLE_SUPER_ADMIN && \in_array($perm, ['role_permissions', 'view_user'])) { + // these permissions may not be revoked at any time, because super admin would loose the ability to reactivate any permission + if ($role === User::ROLE_SUPER_ADMIN && \in_array($perm, self::SUPER_ADMIN_PERMISSIONS)) { continue; } - if (!$isAllowed) { - if (\array_key_exists($role, $this->permissions)) { - if (($key = array_search($perm, $this->permissions[$role])) !== false) { - unset($this->permissions[$role][$key]); - } + if (!\array_key_exists($role, $this->permissions)) { + $this->permissions[$role] = []; + } + + if (false === $isAllowed) { + if (($key = array_search($perm, $this->permissions[$role])) !== false) { + unset($this->permissions[$role][$key]); } } else { - if (!\array_key_exists($role, $this->permissions)) { - $this->permissions[$role] = []; - } $this->permissions[$role][] = $perm; } } diff --git a/templates/user/permissions.html.twig b/templates/user/permissions.html.twig index 18014c55..1be5d843 100644 --- a/templates/user/permissions.html.twig +++ b/templates/user/permissions.html.twig @@ -43,10 +43,14 @@ {% set value = manager.permission(role.name, permission) %} {# see RolePermissionManager for this special case #} - {% if (permission != 'role_permissions' and permission != 'view_user') or role.name != 'ROLE_SUPER_ADMIN' %} - {{ widgets.label_boolean(value) }} + {% if role.name == 'ROLE_SUPER_ADMIN' and permission in always_apply_superadmin %} + {% if value %} + {{ widgets.label('yes'|trans, 'warning') }} + {% else %} + {{ widgets.label('no'|trans, 'danger') }} + {% endif %} {% else %} - {{ widgets.label_boolean(value) }} + {{ widgets.label_boolean(value) }} {% endif %} {% endfor %} diff --git a/tests/Controller/PermissionControllerTest.php b/tests/Controller/PermissionControllerTest.php index 71407efe..18f89f89 100644 --- a/tests/Controller/PermissionControllerTest.php +++ b/tests/Controller/PermissionControllerTest.php @@ -33,7 +33,7 @@ class PermissionControllerTest extends ControllerBaseTest $client = $this->getClientForAuthenticatedUser(User::ROLE_SUPER_ADMIN); $this->assertAccessIsGranted($client, '/admin/permissions'); $this->assertHasDataTable($client); - $this->assertDataTableRowCount($client, 'datatable_user_admin_permissions', 111); + $this->assertDataTableRowCount($client, 'datatable_user_admin_permissions', 112); $this->assertPageActions($client, [ 'back' => $this->createUrl('/admin/user/'), 'roles modal-ajax-form' => $this->createUrl('/admin/permissions/roles/create'), diff --git a/tests/Entity/UserTest.php b/tests/Entity/UserTest.php index 895dc57c..340c72e3 100644 --- a/tests/Entity/UserTest.php +++ b/tests/Entity/UserTest.php @@ -177,12 +177,13 @@ class UserTest extends TestCase self::assertFalse($sut->isTeamlead()); $sut->addRole(User::ROLE_ADMIN); - self::assertTrue($sut->canSeeAllData()); + self::assertFalse($sut->canSeeAllData()); self::assertTrue($sut->isAdmin()); self::assertFalse($sut->isTeamlead()); $sut->addRole(User::ROLE_TEAMLEAD); self::assertTrue($sut->isTeamlead()); + self::assertFalse($sut->canSeeAllData()); $sut->removeRole(User::ROLE_ADMIN); self::assertFalse($sut->canSeeAllData()); @@ -192,6 +193,11 @@ class UserTest extends TestCase self::assertTrue($sut->canSeeAllData()); self::assertFalse($sut->isAdmin()); self::assertTrue($sut->isSuperAdmin()); + + $sut->removeRole(User::ROLE_SUPER_ADMIN); + self::assertFalse($sut->canSeeAllData()); + self::assertFalse($sut->isSuperAdmin()); + self::assertTrue($sut->isTeamlead()); } /** @@ -235,4 +241,14 @@ class UserTest extends TestCase self::assertEquals('foobar', $sut->getPreferenceValue('test')); } + + public function testCanSeeAllData() + { + $sut = new User(); + $sut->addRole(User::ROLE_USER); + self::assertFalse($sut->canSeeAllData()); + self::assertTrue($sut->initCanSeeAllData(true)); + self::assertTrue($sut->canSeeAllData()); + self::assertFalse($sut->initCanSeeAllData(true)); + } }