added team permissions on user queries (#1815)

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
 

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_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']
+            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']

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) {

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

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'));
         }
     }
 }

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();

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')
+            $or->add($qb->expr()->eq('u.id', ':user'));
-                ->leftJoin('teams.users', 'users')
+            $qb->setParameter('user', $user);
-                ->andWhere('teams.teamlead = :id')
+        }
-                ->setParameter('id', $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);
         }

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
+            // 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, ['role_permissions', 'view_user'])) {
+            if ($role === User::ROLE_SUPER_ADMIN && \in_array($perm, self::SUPER_ADMIN_PERMISSIONS)) {
                 continue;
             }
 
-            if (!$isAllowed) {
+            if (!\array_key_exists($role, $this->permissions)) {
-                if (\array_key_exists($role, $this->permissions)) {
+                $this->permissions[$role] = [];
-                    if (($key = array_search($perm, $this->permissions[$role])) !== false) {
+            }
-                        unset($this->permissions[$role][$key]);
+
-                    }
+            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;
             }
         }

templates/user/permissions.html.twig

@@ -43,10 +43,14 @@
                         {% set value = manager.permission(role.name, permission) %}
                         <td class="text-center">
                             {# see RolePermissionManager for this special case #}
-                            {% if (permission != 'role_permissions' and permission != 'view_user') or role.name != 'ROLE_SUPER_ADMIN' %}
+                            {% if role.name == 'ROLE_SUPER_ADMIN' and permission in always_apply_superadmin %}
-                                <a href="{{ path('admin_user_permission_save', {'id': role.id, 'name': permission, 'value': (value ? '0' : '1')}) }}">{{ widgets.label_boolean(value) }}</a>
+                                {% if value %}
+                                    {{ widgets.label('yes'|trans, 'warning') }}
+                                {% else %}
+                                    <a href="{{ path('admin_user_permission_save', {'id': role.id, 'name': permission, 'value': '1'}) }}">{{ widgets.label('no'|trans, 'danger') }}</a>
+                                {% endif %}
                             {% else %}
-                                {{ widgets.label_boolean(value) }}
+                                <a href="{{ path('admin_user_permission_save', {'id': role.id, 'name': permission, 'value': (value ? '0' : '1')}) }}">{{ widgets.label_boolean(value) }}</a>
                             {% endif %}
                         </td>
                     {% endfor %}

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'),

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));
+    }
 }