From 1209bee5c5674997b867c92b4b74e3793809b9da Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Fri, 6 Oct 2023 02:26:05 +0300 Subject: [PATCH 01/41] chore: use a single call to change all permissions --- manifest.php | 4 +- model/ChangePermissionsService.php | 217 ++++++++++++++++++++++++++ model/DataBaseAccess.php | 126 +++++++++++++++ model/PermissionServiceProvider.php | 26 +++ model/tasks/ChangePermissionsTask.php | 12 +- 5 files changed, 383 insertions(+), 2 deletions(-) create mode 100644 model/ChangePermissionsService.php create mode 100644 model/PermissionServiceProvider.php diff --git a/manifest.php b/manifest.php index e60e143..9244ed7 100644 --- a/manifest.php +++ b/manifest.php @@ -19,6 +19,7 @@ */ use oat\taoDacSimple\model\Copy\ServiceProvider\CopyServiceProvider; +use oat\taoDacSimple\model\PermissionServiceProvider; use oat\taoDacSimple\scripts\install\AttachEventHandler; use oat\taoDacSimple\scripts\update\Updater; use oat\taoDacSimple\scripts\install\SetupDataAccess; @@ -65,6 +66,7 @@ 'BASE_URL' => ROOT_URL . 'taoDacSimple/', ], 'containerServiceProviders' => [ - CopyServiceProvider::class + CopyServiceProvider::class, + PermissionServiceProvider::class, ] ]; diff --git a/model/ChangePermissionsService.php b/model/ChangePermissionsService.php new file mode 100644 index 0000000..cef2eff --- /dev/null +++ b/model/ChangePermissionsService.php @@ -0,0 +1,217 @@ +dataBaseAccess = $dataBaseAccess; + $this->strategy = $strategy; + } + + public function __invoke(core_kernel_classes_Resource $resource, array $permissionsToSet, bool $isRecursive): void + { + $resourceIds = $this->getResourceIdsToUpdate($resource, $isRecursive); + $currentPermissions = $this->getResourcesPermissions($resourceIds); + $permissionsDelta = $this->strategy->getDeltaPermissions( + $currentPermissions[$resource->getUri()] ?? [], + $permissionsToSet + ); + + if (empty($permissionsDelta)) { + return; + } + + $actions = $this->getActions($resourceIds, $currentPermissions, $permissionsDelta); + $this->dryRun($actions, $currentPermissions); + $this->wetRun($actions); + + + + + // >>> UDIR + + // <<< UDIR + } + + /* + private function triggerEvents( + string $resourceId, + string $rootResourceId, + array $addRemove, + bool $isRecursive, + bool $applyToNestedResources + ): void { + if (!empty($addRemove['add'])) { + foreach ($addRemove['add'] as $userId => $rights) { + $this->eventManager->trigger(new DacRootAddedEvent($userId, $resourceId, (array)$rights)); + } + } + if (!empty($addRemove['remove'])) { + foreach ($addRemove['remove'] as $userId => $rights) { + $this->eventManager->trigger(new DacRootRemovedEvent($userId, $resourceId, (array)$rights)); + } + } + $this->eventManager->trigger( + new DacAffectedUsersEvent( + array_keys($addRemove['add'] ?? []), + array_keys($addRemove['remove'] ?? []) + ) + ); + + $this->eventManager->trigger( + new DataAccessControlChangedEvent( + $resourceId, + $addRemove, + $isRecursive, + $applyToNestedResources, + $rootResourceId + ) + ); + }*/ + + private function getResourceIdsToUpdate(core_kernel_classes_Resource $resource, bool $isRecursive): array + { + if ($isRecursive) { + return $this->dataBaseAccess->getTreeIds($resource); + } + + return [$resource->getUri()]; + } + + private function getResourcesPermissions(array $resourceIds): array + { + if (empty($resourceIds)) { + return []; + } + + return $this->dataBaseAccess->getResourcesPermissions($resourceIds); + } + + private function getActions( + array $resourceIdsToUpdate, + array $currentResourcePermissions, + array $permissionsDelta + ): array { + $addActions = []; + $removeActions = []; + + foreach ($resourceIdsToUpdate as $resourceId) { + $resourcePermissions = $currentResourcePermissions[$resourceId]; + + $remove = $this->strategy->getPermissionsToRemove($resourcePermissions, $permissionsDelta); + + if (!empty($remove)) { + $removeActions[] = ['permissions' => $remove, 'resourceId' => $resourceId]; + } + + $add = $this->strategy->getPermissionsToAdd($resourcePermissions, $permissionsDelta); + + if (!empty($add)) { + $addActions[] = ['permissions' => $add, 'resourceId' => $resourceId]; + } + } + + return $this->deduplicateActions(['add' => $addActions, 'remove' => $removeActions]); + } + + private function deduplicateActions(array $actions): array + { + foreach ($actions['add'] as &$entry) { + foreach ($entry['permissions'] as &$grants) { + $grants = array_unique($grants); + } + } + + foreach ($actions['remove'] as &$entry) { + foreach ($entry['permissions'] as &$grants) { + $grants = array_unique($grants); + } + } + + return $actions; + } + + private function dryRun(array $actions, array $permissionsList): void + { + $resultPermissions = $permissionsList; + + foreach ($actions['remove'] as $item) { + $this->dryRemove($item['permissions'], $item['resourceId'], $resultPermissions); + } + foreach ($actions['add'] as $item) { + $this->dryAdd($item['permissions'], $item['resourceId'], $resultPermissions); + } + + $this->assertHasUserWithGrantPermission($resultPermissions); + } + + private function wetRun(array $actions): void + { + if (!empty($actions['remove'])) { + $this->dataBaseAccess->removeMultiplePermissionsNew($actions['remove']); + } + if (!empty($actions['add'])) { + $this->dataBaseAccess->addMultiplePermissionsNew($actions['add']); + } + } + + private function dryRemove(array $remove, string $resourceId, array &$resultPermissions): void + { + foreach ($remove as $userToRemove => $permissionToRemove) { + if (!empty($resultPermissions[$resourceId][$userToRemove])) { + $resultPermissions[$resourceId][$userToRemove] = array_diff( + $resultPermissions[$resourceId][$userToRemove], + $permissionToRemove + ); + } + } + } + + private function dryAdd(array $add, string $resourceId, array &$resultPermissions): void + { + foreach ($add as $userToAdd => $permissionToAdd) { + if (empty($resultPermissions[$resourceId][$userToAdd])) { + $resultPermissions[$resourceId][$userToAdd] = $permissionToAdd; + } else { + $resultPermissions[$resourceId][$userToAdd] = array_merge( + $resultPermissions[$resourceId][$userToAdd], + $permissionToAdd + ); + } + } + } + + /** + * Checks if all resources after all actions are applied will have at least + * one user with GRANT permission. + */ + private function assertHasUserWithGrantPermission(array $resultPermissions): void + { + foreach ($resultPermissions as $resultResources => $resultUsers) { + $granted = false; + foreach ($resultUsers as $permissions) { + $granted = in_array(PermissionProvider::PERMISSION_GRANT, $permissions, true); + + if ($granted) { + break; + } + } + + if (!$granted) { + throw new PermissionsServiceException( + sprintf( + 'Resource %s should have at least one user with GRANT access', + $resultResources + ) + ); + } + } + } +} \ No newline at end of file diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index 510d9b2..ddc88f9 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -21,6 +21,7 @@ namespace oat\taoDacSimple\model; use common_persistence_SqlPersistence; +use core_kernel_classes_Resource; use oat\oatbox\event\EventManager; use oat\oatbox\service\ConfigurableService; use oat\taoDacSimple\model\event\DacAddedEvent; @@ -65,6 +66,29 @@ protected function getEventManager() return $this->getServiceLocator()->get(EventManager::SERVICE_ID); } + public function getTreeIds(core_kernel_classes_Resource $resource): array + { + $query = <<<'SQL' +WITH RECURSIVE statements_tree AS ( + SELECT r.subject FROM statements r WHERE r.subject = ? GROUP BY r.subject + UNION ALL + SELECT s.subject FROM statements s JOIN statements_tree st ON s.predicate IN (?, ?) AND s.object = st.subject +) +SELECT subject FROM statements_tree; +SQL; + + $results = $this->fetchQuery( + $query, + [ + $resource->getUri(), + 'http://www.w3.org/2000/01/rdf-schema#subClassOf', + 'http://www.w3.org/1999/02/22-rdf-syntax-ns#type', + ] + ); + + return array_map(static fn (array $result): string => $result['subject'], $results); + } + /** * Retrieve info on users having privileges on a set of resources * @@ -209,6 +233,43 @@ public function addMultiplePermissions(array $permissionData) } } + /** + * Add batch permissions + * + * @access public + * @param array $permissionData + * @return void + * @throws Throwable + */ + public function addMultiplePermissionsNew(array $permissionData) + { + $insert = []; + + foreach ($permissionData as $permissionItem) { + foreach ($permissionItem['permissions'] as $userId => $privilegeIds) { + if (!empty($privilegeIds)) { + foreach ($privilegeIds as $privilegeId) { + $insert [] = [ + self::COLUMN_USER_ID => $userId, + self::COLUMN_RESOURCE_ID => $permissionItem['resourceId'], + self::COLUMN_PRIVILEGE => $privilegeId + ]; + } + } + } + } + + $this->insertPermissions($insert); + + foreach ($insert as $inserted) { + $this->getEventManager()->trigger(new DacAddedEvent( + $inserted[self::COLUMN_USER_ID], + $inserted[self::COLUMN_RESOURCE_ID], + (array)$inserted[self::COLUMN_PRIVILEGE] + )); + } + } + /** * Get the permissions to resource * @@ -279,8 +340,10 @@ public function removeMultiplePermissions(array $data) { $groupedRemove = []; $eventsData = []; + foreach ($data as $permissionItem) { $resource = &$permissionItem['resource']; + foreach ($permissionItem['permissions'] as $userId => $privilegeIds) { if (!empty($privilegeIds)) { $idString = implode($privilegeIds); @@ -329,6 +392,69 @@ public function removeMultiplePermissions(array $data) } } + /** + * Remove batch permissions + * + * @access public + * @param array $data + * @return void + */ + public function removeMultiplePermissionsNew(array $data) + { + $groupedRemove = []; + $eventsData = []; + + foreach ($data as $permissionItem) { + $resourceId = $permissionItem['resourceId']; + + foreach ($permissionItem['permissions'] as $userId => $privilegeIds) { + if (!empty($privilegeIds)) { + $idString = implode($privilegeIds); + + $groupedRemove[$userId][$idString]['resources'][] = $resourceId; + $groupedRemove[$userId][$idString]['privileges'] = $privilegeIds; + + $eventsData[] = [ + 'userId' => $userId, + 'resourceId' => $resourceId, + 'privileges' => $privilegeIds + ]; + } + } + } + foreach ($groupedRemove as $userId => $resources) { + foreach ($resources as $permissions) { + $inQueryPrivilege = implode(',', array_fill(0, count($permissions['privileges']), ' ? ')); + $inQueryResources = implode(',', array_fill(0, count($permissions['resources']), ' ? ')); + $query = sprintf( + 'DELETE FROM %s WHERE %s IN (%s) AND %s IN (%s) AND %s = ?', + self::TABLE_PRIVILEGES_NAME, + self::COLUMN_RESOURCE_ID, + $inQueryResources, + self::COLUMN_PRIVILEGE, + $inQueryPrivilege, + self::COLUMN_USER_ID + ); + + $params = array_merge( + array_values($permissions['resources']), + array_values($permissions['privileges']), + [$userId] + ); + + $this->getPersistence()->exec($query, $params); + } + } + + foreach ($eventsData as $eventData) { + $this->getEventManager()->trigger(new DacRemovedEvent( + $eventData['userId'], + $eventData['resourceId'], + $eventData['privileges'] + )); + } + } + /** * Remove all permissions from a resource * diff --git a/model/PermissionServiceProvider.php b/model/PermissionServiceProvider.php new file mode 100644 index 0000000..7c26b3b --- /dev/null +++ b/model/PermissionServiceProvider.php @@ -0,0 +1,26 @@ +services(); + + $services->set(SavePermissionsStrategy::class, SavePermissionsStrategy::class); + + $services + ->set(ChangePermissionsService::class, ChangePermissionsService::class) + ->public() + ->args([ + service(DataBaseAccess::SERVICE_ID), + service(SavePermissionsStrategy::class), + ]); + } +} \ No newline at end of file diff --git a/model/tasks/ChangePermissionsTask.php b/model/tasks/ChangePermissionsTask.php index c22e7bc..0368989 100644 --- a/model/tasks/ChangePermissionsTask.php +++ b/model/tasks/ChangePermissionsTask.php @@ -32,6 +32,7 @@ use oat\tao\model\taskQueue\QueueDispatcherInterface; use oat\tao\model\taskQueue\Task\TaskAwareInterface; use oat\tao\model\taskQueue\Task\TaskAwareTrait; +use oat\taoDacSimple\model\ChangePermissionsService; use oat\taoDacSimple\model\Command\ChangePermissionsCommand; use oat\taoDacSimple\model\PermissionsService; use oat\taoDacSimple\model\PermissionsServiceFactory; @@ -65,7 +66,14 @@ public function __invoke($params = []): Report $this->validateParams($params); try { - return $this->doHandle( + /** @var ChangePermissionsService $changePermissionsService */ + $changePermissionsService = $this->getServiceManager()->getContainer()->get(ChangePermissionsService::class); + $changePermissionsService( + $this->getResource($params[self::PARAM_RESOURCE]), + (array) $params[self::PARAM_PRIVILEGES], + filter_var($params[self::PARAM_RECURSIVE] ?? false, FILTER_VALIDATE_BOOL) + ); + $this->doHandle( $this->getClass($params[self::PARAM_RESOURCE]), ($params[self::PARAM_REQUEST_ROOT] ?? $params[self::PARAM_RESOURCE]), (array) $params[self::PARAM_PRIVILEGES], @@ -73,6 +81,8 @@ public function __invoke($params = []): Report (bool) ($params[self::PARAM_NESTED_RESOURCES] ?? false), $params[self::PARAM_PERMISSIONS_DELTA] ?? null ); + + return Report::createSuccess('Permissions saved'); } catch (Exception $e) { $errMessage = sprintf('Saving permissions failed: %s', $e->getMessage()); From be9bb44d1d6a450cdeb09bb6fef3d2ee19eebfb0 Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Mon, 9 Oct 2023 16:10:05 +0300 Subject: [PATCH 02/41] chore: update udir calls to rely on a single resource instead of nested --- model/ChangePermissionsService.php | 260 ++++++++++++-------------- model/DataBaseAccess.php | 103 +++++----- model/PermissionServiceProvider.php | 2 + model/tasks/ChangePermissionsTask.php | 16 +- 4 files changed, 186 insertions(+), 195 deletions(-) diff --git a/model/ChangePermissionsService.php b/model/ChangePermissionsService.php index cef2eff..6980eda 100644 --- a/model/ChangePermissionsService.php +++ b/model/ChangePermissionsService.php @@ -3,188 +3,139 @@ namespace oat\taoDacSimple\model; use core_kernel_classes_Resource; +use oat\oatbox\event\EventManager; +use oat\tao\model\event\DataAccessControlChangedEvent; +use oat\taoDacSimple\model\event\DacAffectedUsersEvent; +use oat\taoDacSimple\model\event\DacRootAddedEvent; +use oat\taoDacSimple\model\event\DacRootRemovedEvent; class ChangePermissionsService { private DataBaseAccess $dataBaseAccess; private PermissionsStrategyInterface $strategy; + private EventManager $eventManager; - public function __construct(DataBaseAccess $dataBaseAccess, PermissionsStrategyInterface $strategy) - { + public function __construct( + DataBaseAccess $dataBaseAccess, + PermissionsStrategyInterface $strategy, + EventManager $eventManager + ) { $this->dataBaseAccess = $dataBaseAccess; $this->strategy = $strategy; + $this->eventManager = $eventManager; } public function __invoke(core_kernel_classes_Resource $resource, array $permissionsToSet, bool $isRecursive): void { - $resourceIds = $this->getResourceIdsToUpdate($resource, $isRecursive); - $currentPermissions = $this->getResourcesPermissions($resourceIds); + $resources = $this->getResourceToUpdate($resource, $isRecursive); + $this->enrichWithPermissions($resources); + $rootResourcePermissions = $resources[$resource->getUri()]['permissions']; + $permissionsDelta = $this->strategy->getDeltaPermissions( - $currentPermissions[$resource->getUri()] ?? [], + $rootResourcePermissions['current'] ?? [], $permissionsToSet ); - if (empty($permissionsDelta)) { + if (empty($permissionsDelta['remove']) && empty($permissionsDelta['add'])) { return; } - $actions = $this->getActions($resourceIds, $currentPermissions, $permissionsDelta); - $this->dryRun($actions, $currentPermissions); - $this->wetRun($actions); - - + $resources = $this->enrichWithAddRemoveActions($resources, $permissionsDelta); + $this->dryRun($resources); + $this->wetRun($resources); - - // >>> UDIR - - // <<< UDIR + $this->triggerEvents($resource->getUri(), $permissionsDelta, $isRecursive); } - /* - private function triggerEvents( - string $resourceId, - string $rootResourceId, - array $addRemove, - bool $isRecursive, - bool $applyToNestedResources - ): void { - if (!empty($addRemove['add'])) { - foreach ($addRemove['add'] as $userId => $rights) { - $this->eventManager->trigger(new DacRootAddedEvent($userId, $resourceId, (array)$rights)); - } - } - if (!empty($addRemove['remove'])) { - foreach ($addRemove['remove'] as $userId => $rights) { - $this->eventManager->trigger(new DacRootRemovedEvent($userId, $resourceId, (array)$rights)); - } - } - $this->eventManager->trigger( - new DacAffectedUsersEvent( - array_keys($addRemove['add'] ?? []), - array_keys($addRemove['remove'] ?? []) - ) - ); - - $this->eventManager->trigger( - new DataAccessControlChangedEvent( - $resourceId, - $addRemove, - $isRecursive, - $applyToNestedResources, - $rootResourceId - ) - ); - }*/ - - private function getResourceIdsToUpdate(core_kernel_classes_Resource $resource, bool $isRecursive): array + private function getResourceToUpdate(core_kernel_classes_Resource $resource, bool $isRecursive): array { if ($isRecursive) { - return $this->dataBaseAccess->getTreeIds($resource); + return $this->dataBaseAccess->getResourceTree($resource); } - return [$resource->getUri()]; + return [ + $resource->getUri() => [ + 'id' => $resource->getUri(), + 'isClass' => $resource->isClass(), + 'level' => 1, + ], + ]; } - private function getResourcesPermissions(array $resourceIds): array + private function enrichWithPermissions(array &$resources): void { - if (empty($resourceIds)) { - return []; + if (empty($resources)) { + return; } - return $this->dataBaseAccess->getResourcesPermissions($resourceIds); - } - - private function getActions( - array $resourceIdsToUpdate, - array $currentResourcePermissions, - array $permissionsDelta - ): array { - $addActions = []; - $removeActions = []; + $permissions = $this->dataBaseAccess->getResourcesPermissions(array_column($resources, 'id')); - foreach ($resourceIdsToUpdate as $resourceId) { - $resourcePermissions = $currentResourcePermissions[$resourceId]; - - $remove = $this->strategy->getPermissionsToRemove($resourcePermissions, $permissionsDelta); - - if (!empty($remove)) { - $removeActions[] = ['permissions' => $remove, 'resourceId' => $resourceId]; - } - - $add = $this->strategy->getPermissionsToAdd($resourcePermissions, $permissionsDelta); + foreach ($resources as &$resource) { + $resource['permissions']['current'] = $permissions[$resource['id']]; + } + } - if (!empty($add)) { - $addActions[] = ['permissions' => $add, 'resourceId' => $resourceId]; - } + private function enrichWithAddRemoveActions(array $resources, array $permissionsDelta): array + { + foreach ($resources as &$resource) { + $resource['permissions']['remove'] = $this->strategy->getPermissionsToRemove( + $resource['permissions']['current'], + $permissionsDelta + ); + + $resource['permissions']['add'] = $this->strategy->getPermissionsToAdd( + $resource['permissions']['current'], + $permissionsDelta + ); } - return $this->deduplicateActions(['add' => $addActions, 'remove' => $removeActions]); + return $this->deduplicateChanges($resources); } - private function deduplicateActions(array $actions): array + private function deduplicateChanges(array $resources): array { - foreach ($actions['add'] as &$entry) { - foreach ($entry['permissions'] as &$grants) { - $grants = array_unique($grants); + foreach ($resources as &$resource) { + foreach ($resource['permissions']['remove'] as &$removePermissions) { + $removePermissions = array_unique($removePermissions); } - } - foreach ($actions['remove'] as &$entry) { - foreach ($entry['permissions'] as &$grants) { - $grants = array_unique($grants); + foreach ($resource['permissions']['add'] as &$addPermissions) { + $addPermissions = array_unique($addPermissions); } } - return $actions; + return $resources; } - private function dryRun(array $actions, array $permissionsList): void + private function dryRun(array $resources): void { - $resultPermissions = $permissionsList; - - foreach ($actions['remove'] as $item) { - $this->dryRemove($item['permissions'], $item['resourceId'], $resultPermissions); - } - foreach ($actions['add'] as $item) { - $this->dryAdd($item['permissions'], $item['resourceId'], $resultPermissions); - } + foreach ($resources as $resource) { + $newPermissions = $resource['permissions']['current']; - $this->assertHasUserWithGrantPermission($resultPermissions); - } + $this->dryRemove($resource, $newPermissions); + $this->dryAdd($resource, $newPermissions); - private function wetRun(array $actions): void - { - if (!empty($actions['remove'])) { - $this->dataBaseAccess->removeMultiplePermissionsNew($actions['remove']); - } - if (!empty($actions['add'])) { - $this->dataBaseAccess->addMultiplePermissionsNew($actions['add']); + $this->assertHasUserWithGrantPermission($resource['id'], $newPermissions); } } - private function dryRemove(array $remove, string $resourceId, array &$resultPermissions): void + private function dryRemove(array $resource, array &$newPermissions): void { - foreach ($remove as $userToRemove => $permissionToRemove) { - if (!empty($resultPermissions[$resourceId][$userToRemove])) { - $resultPermissions[$resourceId][$userToRemove] = array_diff( - $resultPermissions[$resourceId][$userToRemove], - $permissionToRemove - ); - } + foreach ($resource['permissions']['remove'] as $user => $removePermissions) { + $newPermissions[$user] = array_diff( + $newPermissions[$user] ?? [], + $removePermissions + ); } } - private function dryAdd(array $add, string $resourceId, array &$resultPermissions): void + private function dryAdd(array $resource, array &$newPermissions): void { - foreach ($add as $userToAdd => $permissionToAdd) { - if (empty($resultPermissions[$resourceId][$userToAdd])) { - $resultPermissions[$resourceId][$userToAdd] = $permissionToAdd; - } else { - $resultPermissions[$resourceId][$userToAdd] = array_merge( - $resultPermissions[$resourceId][$userToAdd], - $permissionToAdd - ); - } + foreach ($resource['permissions']['add'] as $user => $addPermissions) { + $newPermissions[$user] = array_merge( + $resource['permissions']['current'][$user] ?? [], + $addPermissions + ); } } @@ -192,26 +143,49 @@ private function dryAdd(array $add, string $resourceId, array &$resultPermission * Checks if all resources after all actions are applied will have at least * one user with GRANT permission. */ - private function assertHasUserWithGrantPermission(array $resultPermissions): void + private function assertHasUserWithGrantPermission(string $resourceId, array $newPermissions): void { - foreach ($resultPermissions as $resultResources => $resultUsers) { - $granted = false; - foreach ($resultUsers as $permissions) { - $granted = in_array(PermissionProvider::PERMISSION_GRANT, $permissions, true); - - if ($granted) { - break; - } + foreach ($newPermissions as $permissions) { + if (in_array(PermissionProvider::PERMISSION_GRANT, $permissions, true)) { + return; } + } - if (!$granted) { - throw new PermissionsServiceException( - sprintf( - 'Resource %s should have at least one user with GRANT access', - $resultResources - ) - ); - } + throw new PermissionsServiceException( + sprintf( + 'Resource %s should have at least one user with GRANT access', + $resourceId + ) + ); + } + + private function wetRun(array $resources): void + { + $this->dataBaseAccess->removeMultiplePermissionsNew($resources); + $this->dataBaseAccess->addMultiplePermissionsNew($resources); + } + + private function triggerEvents(string $resourceId, array $permissionsDelta, bool $isRecursive): void + { + // >>> UDIR + // TODO Collect all users and do in a single event + foreach ($permissionsDelta['add'] as $userId => $permissions) { + $this->eventManager->trigger(new DacRootAddedEvent($userId, $resourceId, $permissions)); + } + + // TODO Collect all users and do in a single event + foreach ($permissionsDelta['remove'] as $userId => $permissions) { + $this->eventManager->trigger(new DacRootRemovedEvent($userId, $resourceId, $permissions)); } + + $this->eventManager->trigger( + new DacAffectedUsersEvent( + array_keys($permissionsDelta['add']), + array_keys($permissionsDelta['remove']) + ) + ); + // <<< UDIR + + $this->eventManager->trigger(new DataAccessControlChangedEvent($resourceId, $permissionsDelta, $isRecursive)); } } \ No newline at end of file diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index ddc88f9..7f3474e 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -22,6 +22,8 @@ use common_persistence_SqlPersistence; use core_kernel_classes_Resource; +use oat\generis\model\OntologyRdf; +use oat\generis\model\OntologyRdfs; use oat\oatbox\event\EventManager; use oat\oatbox\service\ConfigurableService; use oat\taoDacSimple\model\event\DacAddedEvent; @@ -66,27 +68,36 @@ protected function getEventManager() return $this->getServiceLocator()->get(EventManager::SERVICE_ID); } - public function getTreeIds(core_kernel_classes_Resource $resource): array + public function getResourceTree(core_kernel_classes_Resource $resource): array { $query = <<<'SQL' WITH RECURSIVE statements_tree AS ( - SELECT r.subject FROM statements r WHERE r.subject = ? GROUP BY r.subject + SELECT r.subject, r.predicate, 1 as level FROM statements r WHERE r.subject = ? AND r.predicate IN (?, ?) GROUP BY r.subject UNION ALL - SELECT s.subject FROM statements s JOIN statements_tree st ON s.predicate IN (?, ?) AND s.object = st.subject + SELECT s.subject, s.predicate, level + 1 FROM statements s JOIN statements_tree st ON s.object = st.subject ) -SELECT subject FROM statements_tree; +SELECT subject, predicate, level FROM statements_tree; SQL; $results = $this->fetchQuery( $query, [ $resource->getUri(), - 'http://www.w3.org/2000/01/rdf-schema#subClassOf', - 'http://www.w3.org/1999/02/22-rdf-syntax-ns#type', + OntologyRdfs::RDFS_SUBCLASSOF, + OntologyRdf::RDF_TYPE, ] ); - return array_map(static fn (array $result): string => $result['subject'], $results); + $resources = []; + + foreach ($results as $result) { + $resources[$result['subject']] = [ + 'id' => $result['subject'], + 'isClass' => $result['predicate'] === OntologyRdfs::RDFS_SUBCLASSOF, + ]; + } + + return $resources; } /** @@ -237,24 +248,25 @@ public function addMultiplePermissions(array $permissionData) * Add batch permissions * * @access public - * @param array $permissionData * @return void * @throws Throwable */ - public function addMultiplePermissionsNew(array $permissionData) + public function addMultiplePermissionsNew(array $resources) { $insert = []; - foreach ($permissionData as $permissionItem) { - foreach ($permissionItem['permissions'] as $userId => $privilegeIds) { - if (!empty($privilegeIds)) { - foreach ($privilegeIds as $privilegeId) { - $insert [] = [ - self::COLUMN_USER_ID => $userId, - self::COLUMN_RESOURCE_ID => $permissionItem['resourceId'], - self::COLUMN_PRIVILEGE => $privilegeId - ]; - } + foreach ($resources as $resource) { + foreach ($resource['permissions']['add'] as $userId => $addPermissions) { + if (empty($addPermissions)) { + continue; + } + + foreach ($addPermissions as $permission) { + $insert[] = [ + self::COLUMN_USER_ID => $userId, + self::COLUMN_RESOURCE_ID => $resource['id'], + self::COLUMN_PRIVILEGE => $permission, + ]; } } } @@ -262,11 +274,13 @@ public function addMultiplePermissionsNew(array $permissionData) $this->insertPermissions($insert); foreach ($insert as $inserted) { - $this->getEventManager()->trigger(new DacAddedEvent( - $inserted[self::COLUMN_USER_ID], - $inserted[self::COLUMN_RESOURCE_ID], - (array)$inserted[self::COLUMN_PRIVILEGE] - )); + $this->getEventManager()->trigger( + new DacAddedEvent( + $inserted[self::COLUMN_USER_ID], + $inserted[self::COLUMN_RESOURCE_ID], + (array) $inserted[self::COLUMN_PRIVILEGE] + ) + ); } } @@ -399,33 +413,34 @@ public function removeMultiplePermissions(array $data) * @param array $data * @return void */ - public function removeMultiplePermissionsNew(array $data) + public function removeMultiplePermissionsNew(array $resources) { $groupedRemove = []; $eventsData = []; - foreach ($data as $permissionItem) { - $resourceId = $permissionItem['resourceId']; + foreach ($resources as $resource) { + foreach ($resource['permissions']['remove'] as $userId => $removePermissions) { + if (empty($removePermissions)) { + continue; + } - foreach ($permissionItem['permissions'] as $userId => $privilegeIds) { - if (!empty($privilegeIds)) { - $idString = implode($privilegeIds); + $idString = implode($removePermissions); - $groupedRemove[$userId][$idString]['resources'][] = $resourceId; - $groupedRemove[$userId][$idString]['privileges'] = $privilegeIds; + $groupedRemove[$userId][$idString]['resources'][] = $resource['id']; + $groupedRemove[$userId][$idString]['privileges'] = $removePermissions; - $eventsData[] = [ - 'userId' => $userId, - 'resourceId' => $resourceId, - 'privileges' => $privilegeIds - ]; - } + $eventsData[] = [ + 'userId' => $userId, + 'resourceId' => $resource['id'], + 'privileges' => $removePermissions, + ]; } } - foreach ($groupedRemove as $userId => $resources) { - foreach ($resources as $permissions) { - $inQueryPrivilege = implode(',', array_fill(0, count($permissions['privileges']), ' ? ')); - $inQueryResources = implode(',', array_fill(0, count($permissions['resources']), ' ? ')); + + foreach ($groupedRemove as $userId => $priviligeGroups) { + foreach ($priviligeGroups as $privilegeGroup) { + $inQueryPrivilege = implode(',', array_fill(0, count($privilegeGroup['privileges']), ' ? ')); + $inQueryResources = implode(',', array_fill(0, count($privilegeGroup['resources']), ' ? ')); $query = sprintf( 'DELETE FROM %s WHERE %s IN (%s) AND %s IN (%s) AND %s = ?', self::TABLE_PRIVILEGES_NAME, @@ -437,8 +452,8 @@ public function removeMultiplePermissionsNew(array $data) ); $params = array_merge( - array_values($permissions['resources']), - array_values($permissions['privileges']), + array_values($privilegeGroup['resources']), + array_values($privilegeGroup['privileges']), [$userId] ); diff --git a/model/PermissionServiceProvider.php b/model/PermissionServiceProvider.php index 7c26b3b..a08f7c1 100644 --- a/model/PermissionServiceProvider.php +++ b/model/PermissionServiceProvider.php @@ -3,6 +3,7 @@ namespace oat\taoDacSimple\model; use oat\generis\model\DependencyInjection\ContainerServiceProviderInterface; +use oat\oatbox\event\EventManager; use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator; use function Symfony\Component\DependencyInjection\Loader\Configurator\service; @@ -21,6 +22,7 @@ public function __invoke(ContainerConfigurator $configurator): void ->args([ service(DataBaseAccess::SERVICE_ID), service(SavePermissionsStrategy::class), + service(EventManager::SERVICE_ID), ]); } } \ No newline at end of file diff --git a/model/tasks/ChangePermissionsTask.php b/model/tasks/ChangePermissionsTask.php index 0368989..482be4c 100644 --- a/model/tasks/ChangePermissionsTask.php +++ b/model/tasks/ChangePermissionsTask.php @@ -73,14 +73,14 @@ public function __invoke($params = []): Report (array) $params[self::PARAM_PRIVILEGES], filter_var($params[self::PARAM_RECURSIVE] ?? false, FILTER_VALIDATE_BOOL) ); - $this->doHandle( - $this->getClass($params[self::PARAM_RESOURCE]), - ($params[self::PARAM_REQUEST_ROOT] ?? $params[self::PARAM_RESOURCE]), - (array) $params[self::PARAM_PRIVILEGES], - (bool) ($params[self::PARAM_RECURSIVE] ?? false), - (bool) ($params[self::PARAM_NESTED_RESOURCES] ?? false), - $params[self::PARAM_PERMISSIONS_DELTA] ?? null - ); +// $this->doHandle( +// $this->getClass($params[self::PARAM_RESOURCE]), +// ($params[self::PARAM_REQUEST_ROOT] ?? $params[self::PARAM_RESOURCE]), +// (array) $params[self::PARAM_PRIVILEGES], +// (bool) ($params[self::PARAM_RECURSIVE] ?? false), +// (bool) ($params[self::PARAM_NESTED_RESOURCES] ?? false), +// $params[self::PARAM_PERMISSIONS_DELTA] ?? null +// ); return Report::createSuccess('Permissions saved'); } catch (Exception $e) { From cb063eba3ef20ccd6cc4c123e218bd5d80fb3fc9 Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Mon, 9 Oct 2023 16:10:22 +0300 Subject: [PATCH 03/41] chore: update tao-core version --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 742fc56..28a2a63 100644 --- a/composer.json +++ b/composer.json @@ -22,7 +22,7 @@ "require": { "oat-sa/oatbox-extension-installer": "~1.1||dev-master", "oat-sa/generis": ">=15.24", - "oat-sa/tao-core": ">=52.1", + "oat-sa/tao-core": "dev-feature/adf-1590/optimize-permissions-task-queue as 99.99", "oat-sa/extension-tao-backoffice": ">=6.0.0", "oat-sa/extension-tao-item": ">=11.3" }, From 6604b7a5f7726898d06f9fd1158d61453a174f95 Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Mon, 9 Oct 2023 17:56:29 +0300 Subject: [PATCH 04/41] chore: use separate task for events --- model/ChangePermissionsService.php | 64 ++++++------ model/DataBaseAccess.php | 13 +-- model/PermissionServiceProvider.php | 4 +- model/tasks/ChangePermissionsTask.php | 107 +------------------ model/tasks/PostChangePermissionsTask.php | 119 ++++++++++++++++++++++ 5 files changed, 158 insertions(+), 149 deletions(-) create mode 100644 model/tasks/PostChangePermissionsTask.php diff --git a/model/ChangePermissionsService.php b/model/ChangePermissionsService.php index 6980eda..6df9fb4 100644 --- a/model/ChangePermissionsService.php +++ b/model/ChangePermissionsService.php @@ -3,26 +3,24 @@ namespace oat\taoDacSimple\model; use core_kernel_classes_Resource; -use oat\oatbox\event\EventManager; -use oat\tao\model\event\DataAccessControlChangedEvent; -use oat\taoDacSimple\model\event\DacAffectedUsersEvent; -use oat\taoDacSimple\model\event\DacRootAddedEvent; -use oat\taoDacSimple\model\event\DacRootRemovedEvent; +use oat\generis\model\OntologyRdfs; +use oat\tao\model\taskQueue\QueueDispatcherInterface; +use oat\taoDacSimple\model\tasks\PostChangePermissionsTask; class ChangePermissionsService { private DataBaseAccess $dataBaseAccess; private PermissionsStrategyInterface $strategy; - private EventManager $eventManager; + private QueueDispatcherInterface $queueDispatcher; public function __construct( DataBaseAccess $dataBaseAccess, PermissionsStrategyInterface $strategy, - EventManager $eventManager + QueueDispatcherInterface $queueDispatcher ) { $this->dataBaseAccess = $dataBaseAccess; $this->strategy = $strategy; - $this->eventManager = $eventManager; + $this->queueDispatcher = $queueDispatcher; } public function __invoke(core_kernel_classes_Resource $resource, array $permissionsToSet, bool $isRecursive): void @@ -44,13 +42,23 @@ public function __invoke(core_kernel_classes_Resource $resource, array $permissi $this->dryRun($resources); $this->wetRun($resources); - $this->triggerEvents($resource->getUri(), $permissionsDelta, $isRecursive); + $this->triggerEvents($resource, $permissionsDelta, $isRecursive); } private function getResourceToUpdate(core_kernel_classes_Resource $resource, bool $isRecursive): array { if ($isRecursive) { - return $this->dataBaseAccess->getResourceTree($resource); + $resources = []; + + foreach ($this->dataBaseAccess->getResourceTree($resource) as $result) { + $resources[$result['subject']] = [ + 'id' => $result['subject'], + 'isClass' => $result['predicate'] === OntologyRdfs::RDFS_SUBCLASSOF, + 'level' => $result['level'] + ]; + } + + return $resources; } return [ @@ -165,27 +173,23 @@ private function wetRun(array $resources): void $this->dataBaseAccess->addMultiplePermissionsNew($resources); } - private function triggerEvents(string $resourceId, array $permissionsDelta, bool $isRecursive): void - { - // >>> UDIR - // TODO Collect all users and do in a single event - foreach ($permissionsDelta['add'] as $userId => $permissions) { - $this->eventManager->trigger(new DacRootAddedEvent($userId, $resourceId, $permissions)); - } - - // TODO Collect all users and do in a single event - foreach ($permissionsDelta['remove'] as $userId => $permissions) { - $this->eventManager->trigger(new DacRootRemovedEvent($userId, $resourceId, $permissions)); - } - - $this->eventManager->trigger( - new DacAffectedUsersEvent( - array_keys($permissionsDelta['add']), - array_keys($permissionsDelta['remove']) + private function triggerEvents( + core_kernel_classes_Resource $resource, + array $permissionsDelta, + bool $isRecursive + ): void { + $this->queueDispatcher->createTask( + new PostChangePermissionsTask(), + [ + PostChangePermissionsTask::PARAM_RESOURCE_ID => $resource->getUri(), + PostChangePermissionsTask::PARAM_PERMISSIONS_DELTA => $permissionsDelta, + PostChangePermissionsTask::PARAM_IS_RECURSIVE => $isRecursive, + ], + sprintf( + 'Triggering permissions changes events for resource %s [%s]', + $resource->getLabel(), + $resource->getUri() ) ); - // <<< UDIR - - $this->eventManager->trigger(new DataAccessControlChangedEvent($resourceId, $permissionsDelta, $isRecursive)); } } \ No newline at end of file diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index 7f3474e..306d2cc 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -79,7 +79,7 @@ public function getResourceTree(core_kernel_classes_Resource $resource): array SELECT subject, predicate, level FROM statements_tree; SQL; - $results = $this->fetchQuery( + return $this->fetchQuery( $query, [ $resource->getUri(), @@ -87,17 +87,6 @@ public function getResourceTree(core_kernel_classes_Resource $resource): array OntologyRdf::RDF_TYPE, ] ); - - $resources = []; - - foreach ($results as $result) { - $resources[$result['subject']] = [ - 'id' => $result['subject'], - 'isClass' => $result['predicate'] === OntologyRdfs::RDFS_SUBCLASSOF, - ]; - } - - return $resources; } /** diff --git a/model/PermissionServiceProvider.php b/model/PermissionServiceProvider.php index a08f7c1..c97a51e 100644 --- a/model/PermissionServiceProvider.php +++ b/model/PermissionServiceProvider.php @@ -3,7 +3,7 @@ namespace oat\taoDacSimple\model; use oat\generis\model\DependencyInjection\ContainerServiceProviderInterface; -use oat\oatbox\event\EventManager; +use oat\tao\model\taskQueue\QueueDispatcherInterface; use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator; use function Symfony\Component\DependencyInjection\Loader\Configurator\service; @@ -22,7 +22,7 @@ public function __invoke(ContainerConfigurator $configurator): void ->args([ service(DataBaseAccess::SERVICE_ID), service(SavePermissionsStrategy::class), - service(EventManager::SERVICE_ID), + service(QueueDispatcherInterface::SERVICE_ID), ]); } } \ No newline at end of file diff --git a/model/tasks/ChangePermissionsTask.php b/model/tasks/ChangePermissionsTask.php index 482be4c..a708659 100644 --- a/model/tasks/ChangePermissionsTask.php +++ b/model/tasks/ChangePermissionsTask.php @@ -23,19 +23,12 @@ namespace oat\taoDacSimple\model\tasks; use common_exception_MissingParameter; -use common_report_Report as Report; -use core_kernel_classes_Class; -use core_kernel_classes_Resource; use oat\generis\model\OntologyAwareTrait; use oat\oatbox\extension\AbstractAction; -use oat\tao\model\taskQueue\QueueDispatcher; -use oat\tao\model\taskQueue\QueueDispatcherInterface; +use oat\oatbox\reporting\Report; use oat\tao\model\taskQueue\Task\TaskAwareInterface; use oat\tao\model\taskQueue\Task\TaskAwareTrait; use oat\taoDacSimple\model\ChangePermissionsService; -use oat\taoDacSimple\model\Command\ChangePermissionsCommand; -use oat\taoDacSimple\model\PermissionsService; -use oat\taoDacSimple\model\PermissionsServiceFactory; use Exception; use JsonSerializable; @@ -52,9 +45,6 @@ class ChangePermissionsTask extends AbstractAction implements TaskAwareInterface public const PARAM_RESOURCE = 'resource'; public const PARAM_PRIVILEGES = 'privileges'; public const PARAM_RECURSIVE = 'recursive'; - public const PARAM_NESTED_RESOURCES = 'nested_resources'; - public const PARAM_REQUEST_ROOT = 'request_root'; - public const PARAM_PERMISSIONS_DELTA = 'permissions_delta'; private const MANDATORY_PARAMS = [ self::PARAM_RESOURCE, @@ -73,107 +63,14 @@ public function __invoke($params = []): Report (array) $params[self::PARAM_PRIVILEGES], filter_var($params[self::PARAM_RECURSIVE] ?? false, FILTER_VALIDATE_BOOL) ); -// $this->doHandle( -// $this->getClass($params[self::PARAM_RESOURCE]), -// ($params[self::PARAM_REQUEST_ROOT] ?? $params[self::PARAM_RESOURCE]), -// (array) $params[self::PARAM_PRIVILEGES], -// (bool) ($params[self::PARAM_RECURSIVE] ?? false), -// (bool) ($params[self::PARAM_NESTED_RESOURCES] ?? false), -// $params[self::PARAM_PERMISSIONS_DELTA] ?? null -// ); return Report::createSuccess('Permissions saved'); } catch (Exception $e) { $errMessage = sprintf('Saving permissions failed: %s', $e->getMessage()); - $this->getLogger()->error($errMessage); - return Report::createFailure($errMessage); - } - } - - private function doHandle( - core_kernel_classes_Class $root, - string $requestRoot, - array $privileges, - bool $isRecursive, - bool $withNestedResources, - ?array $permissionsDelta - ): Report { - $permissionService = $this->getPermissionService(); - $permissionsDelta ??= $permissionService->getPermissionsDelta($root, $privileges); - - if ($withNestedResources) { - $message = sprintf( - "Permissions saved for resources under subclass %s [%s]", - $this->formatLabel($root), - $root->getUri() - ); - - $command = new ChangePermissionsCommand($root, $requestRoot, $privileges, $permissionsDelta); - $command->withNestedResources(); - $this->getPermissionService()->applyPermissions($command); - } elseif ($isRecursive) { - $message = 'Starting recursive permissions update'; - - $this->createSubtasksForClasses( - array_merge([$root], $root->getSubClasses(true)), - $requestRoot, - $privileges, - $permissionsDelta - ); - } else { - $message = 'Permissions saved'; - - $this->getPermissionService()->applyPermissions( - new ChangePermissionsCommand($root, $requestRoot, $privileges, $permissionsDelta) - ); + return Report::createError($errMessage); } - - return Report::createSuccess($message); - } - - private function createSubtasksForClasses( - array $allClasses, - string $requestRoot, - array $privileges, - array $permissionsDelta - ): void { - foreach ($allClasses as $oneClass) { - $this->getDispatcher()->createTask( - new self(), - [ - self::PARAM_RESOURCE => $oneClass->getUri(), - self::PARAM_REQUEST_ROOT => $requestRoot, - self::PARAM_PRIVILEGES => $privileges, - self::PARAM_RECURSIVE => false, - self::PARAM_NESTED_RESOURCES => true, - self::PARAM_PERMISSIONS_DELTA => $permissionsDelta, - ], - sprintf( - 'Processing permissions for class %s [%s]', - $this->formatLabel($oneClass), - $oneClass->getUri() - ) - ); - } - } - - protected function formatLabel(core_kernel_classes_Resource $resource): string - { - return strlen($resource->getLabel()) > 128 - ? '...' . substr($resource->getLabel(), -128) - : $resource->getLabel(); - } - - private function getDispatcher(): QueueDispatcher - { - return $this->serviceLocator->get(QueueDispatcherInterface::SERVICE_ID); - } - - private function getPermissionService(): PermissionsService - { - return $this->serviceLocator->get(PermissionsServiceFactory::SERVICE_ID)->create(); } private function validateParams(array $params): void diff --git a/model/tasks/PostChangePermissionsTask.php b/model/tasks/PostChangePermissionsTask.php new file mode 100644 index 0000000..8c4dcd2 --- /dev/null +++ b/model/tasks/PostChangePermissionsTask.php @@ -0,0 +1,119 @@ +validateParams($params); + + try { + $reports = []; + $eventManager = $this->getEventManager(); + $permissionsDelta = $params[self::PARAM_PERMISSIONS_DELTA]; + + // TODO Collect all users and do in a single event + foreach ($permissionsDelta['add'] as $userId => $permissions) { + $eventManager->trigger(new DacRootAddedEvent($userId, $params[self::PARAM_RESOURCE_ID], $permissions)); + } + + $reports[] = Report::createInfo('Required permissions successfully added to parent classes'); + + // TODO Collect all users and do in a single event + foreach ($permissionsDelta['remove'] as $userId => $permissions) { + $eventManager->trigger(new DacRootRemovedEvent($userId, $params[self::PARAM_RESOURCE_ID], $permissions)); + } + + $reports[] = Report::createInfo('Not necessary permissions successfully removed from parent classes'); + + $eventManager->trigger( + new DacAffectedUsersEvent( + array_keys($permissionsDelta['add']), + array_keys($permissionsDelta['remove']) + ) + ); + + $reports[] = Report::createInfo('Affected users successfully updated'); + + $eventManager->trigger( + new DataAccessControlChangedEvent( + $params[self::PARAM_RESOURCE_ID], + $permissionsDelta, + $params[self::PARAM_IS_RECURSIVE] + ) + ); + + return Report::createSuccess('Success', null, $reports); + } catch (Exception $exception) { + $errMessage = sprintf('Error: %s', $exception->getMessage()); + $this->getLogger()->error($errMessage); + + return Report::createError($errMessage); + } + } + + public function jsonSerialize(): string + { + return __CLASS__; + } + + private function validateParams(array $params): void + { + foreach (self::MANDATORY_PARAMS as $param) { + if (!isset($params[$param])) { + throw new common_exception_MissingParameter( + sprintf( + 'Missing parameter `%s` in %s', + $param, + self::class + ) + ); + } + } + + $permissionsDelta = $params[self::PARAM_PERMISSIONS_DELTA]; + + if ( + !array_key_exists('add', $permissionsDelta) + || !array_key_exists('remove', $permissionsDelta) + ) { + throw new InvalidArgumentException( + sprintf( + 'Parameter "%s" must contain "add" and "remove" keys', + self::PARAM_PERMISSIONS_DELTA + ) + ); + } + } + + private function getEventManager(): EventManager + { + return $this->getServiceManager()->getContainer()->get(EventManager::SERVICE_ID); + } +} \ No newline at end of file From 85b3bb9180e28626fa6a0b5ccf9ff06afac92f34 Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Mon, 16 Oct 2023 12:19:10 +0300 Subject: [PATCH 05/41] refactor: reduce number of call to add/remove parent permissions, do everything in a single task instead of two tasks --- model/ChangePermissionsService.php | 47 +++--- model/DataBaseAccess.php | 197 +++++++++++++++++++--- model/PermissionServiceProvider.php | 28 --- model/PermissionsServiceFactory.php | 47 ++++-- model/event/DacRootChangedEvent.php | 61 +++++++ model/tasks/ChangePermissionsTask.php | 10 +- model/tasks/PostChangePermissionsTask.php | 119 ------------- 7 files changed, 302 insertions(+), 207 deletions(-) delete mode 100644 model/PermissionServiceProvider.php create mode 100644 model/event/DacRootChangedEvent.php delete mode 100644 model/tasks/PostChangePermissionsTask.php diff --git a/model/ChangePermissionsService.php b/model/ChangePermissionsService.php index 6df9fb4..d141f6f 100644 --- a/model/ChangePermissionsService.php +++ b/model/ChangePermissionsService.php @@ -1,29 +1,33 @@ dataBaseAccess = $dataBaseAccess; $this->strategy = $strategy; - $this->queueDispatcher = $queueDispatcher; + $this->eventManager = $eventManager; } - public function __invoke(core_kernel_classes_Resource $resource, array $permissionsToSet, bool $isRecursive): void + public function change(core_kernel_classes_Resource $resource, array $permissionsToSet, bool $isRecursive): void { $resources = $this->getResourceToUpdate($resource, $isRecursive); $this->enrichWithPermissions($resources); @@ -169,8 +173,8 @@ private function assertHasUserWithGrantPermission(string $resourceId, array $new private function wetRun(array $resources): void { - $this->dataBaseAccess->removeMultiplePermissionsNew($resources); - $this->dataBaseAccess->addMultiplePermissionsNew($resources); + $this->dataBaseAccess->removeResourcesPermissions($resources); + $this->dataBaseAccess->addResourcesPermissions($resources); } private function triggerEvents( @@ -178,17 +182,22 @@ private function triggerEvents( array $permissionsDelta, bool $isRecursive ): void { - $this->queueDispatcher->createTask( - new PostChangePermissionsTask(), - [ - PostChangePermissionsTask::PARAM_RESOURCE_ID => $resource->getUri(), - PostChangePermissionsTask::PARAM_PERMISSIONS_DELTA => $permissionsDelta, - PostChangePermissionsTask::PARAM_IS_RECURSIVE => $isRecursive, - ], - sprintf( - 'Triggering permissions changes events for resource %s [%s]', - $resource->getLabel(), - $resource->getUri() + if (!empty($permissionsDelta['add']) || !empty($permissionsDelta['remove'])) { + $this->eventManager->trigger(new DacRootChangedEvent($resource->getUri(), $permissionsDelta)); + } + + $this->eventManager->trigger( + new DacAffectedUsersEvent( + array_keys($permissionsDelta['add']), + array_keys($permissionsDelta['remove']) + ) + ); + + $this->eventManager->trigger( + new DataAccessControlChangedEvent( + $resource->getUri(), + $permissionsDelta, + $isRecursive ) ); } diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index 306d2cc..0a2b1f5 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -68,13 +68,88 @@ protected function getEventManager() return $this->getServiceLocator()->get(EventManager::SERVICE_ID); } + public function getParentClassesIds(string $resourceUri): array + { + $query = <<<'SQL' +WITH RECURSIVE statements_tree AS ( + SELECT + r.object + FROM statements r + WHERE r.subject = ? + AND r.predicate IN (?, ?) + UNION ALL + SELECT + s.object + FROM statements s + JOIN statements_tree st + ON s.subject = st.object + AND s.predicate IN (?, ?) + AND s.object NOT IN (?, ?, ?, ?) +) +SELECT object FROM statements_tree; +SQL; + + return array_column( + $this->fetchQuery( + $query, + [ + $resourceUri, + OntologyRdfs::RDFS_SUBCLASSOF, + OntologyRdf::RDF_TYPE, + OntologyRdfs::RDFS_SUBCLASSOF, + OntologyRdf::RDF_TYPE, + 'http://www.tao.lu/Ontologies/TAO.rdf#AssessmentContentObject', + 'http://www.tao.lu/Ontologies/TAO.rdf#TAOObject', + 'http://www.tao.lu/Ontologies/generis.rdf#generis_Ressource', + 'http://www.w3.org/2000/01/rdf-schema#Resource', + ] + ), + 'object' + ); + } + + public function getClassesResources(array $classesIds): array + { + $inQuery = implode(',', array_fill(0, count($classesIds), '?')); + $query = "SELECT subject, object FROM statements WHERE predicate IN (?, ?) AND object IN ($inQuery)"; + + $results = $this->fetchQuery( + $query, + [ + OntologyRdfs::RDFS_SUBCLASSOF, + OntologyRdf::RDF_TYPE, + ...$classesIds, + ] + ); + + $classesResources = []; + + foreach ($results as $result) { + $classesResources[$result['object']][] = $result['subject']; + } + + return $classesResources; + } + public function getResourceTree(core_kernel_classes_Resource $resource): array { $query = <<<'SQL' WITH RECURSIVE statements_tree AS ( - SELECT r.subject, r.predicate, 1 as level FROM statements r WHERE r.subject = ? AND r.predicate IN (?, ?) GROUP BY r.subject + SELECT + r.subject, + r.predicate, + 1 as level + FROM statements r + WHERE r.subject = ? + AND r.predicate IN (?, ?) UNION ALL - SELECT s.subject, s.predicate, level + 1 FROM statements s JOIN statements_tree st ON s.object = st.subject + SELECT + s.subject, + s.predicate, + level + 1 + FROM statements s + JOIN statements_tree st + ON s.object = st.subject ) SELECT subject, predicate, level FROM statements_tree; SQL; @@ -151,6 +226,38 @@ public function getPermissions(array $userIds, array $resourceIds): array return $returnValue; } + public function getPermissionsByUsersAndResources(array $userIds, array $resourceIds): array + { + // Permissions for an empty set of resources must be an empty array + if (empty($resourceIds)) { + return []; + } + + $inQueryResources = implode(',', array_fill(0, count($resourceIds), '?')); + $inQueryUsers = implode(',', array_fill(0, count($userIds), '?')); + + $query = <<fetchQuery($query, [...$resourceIds, ...$userIds]); + + // If resource doesn't have permission don't return null + $data = array_fill_keys($resourceIds, []); + + foreach ($results as $result) { + $data[$result[self::COLUMN_RESOURCE_ID]][$result[self::COLUMN_USER_ID]][] = $result[self::COLUMN_PRIVILEGE]; + } + + return $data; + } + public function getResourcesPermissions(array $resourceIds) { // Return an empty array for resources not having permissions data @@ -233,14 +340,7 @@ public function addMultiplePermissions(array $permissionData) } } - /** - * Add batch permissions - * - * @access public - * @return void - * @throws Throwable - */ - public function addMultiplePermissionsNew(array $resources) + public function addResourcesPermissions(array $resources): void { $insert = []; @@ -332,6 +432,57 @@ public function removePermissions($user, $resourceId, $rights) return true; } + public function addReadAccess(array $addAccessList): bool + { + if (empty($addAccessList)) { + return true; + } + + $values = []; + $valuesQuery = []; + + foreach ($addAccessList as $resourceId => $usersIds) { + foreach ($usersIds as $userId) { + $values = array_merge($values, [$userId, $resourceId, PermissionProvider::PERMISSION_READ]); + $valuesQuery[] = '(?, ?, ?)'; + } + } + + $statement = sprintf( + 'INSERT INTO data_privileges (user_id, resource_id, privilege) VALUES %s', + implode(', ', $valuesQuery) + ); + + return $this->getPersistence()->exec($statement, $values); + } + + public function revokeAccess(array $revokeAccessList): bool + { + if (empty($revokeAccessList)) { + return true; + } + + try { + foreach ($revokeAccessList as $resourceId => $usersIds) { + $inQueryUsers = implode(',', array_fill(0, count($usersIds), ' ? ')); + + $this->getPersistence()->exec( + "DELETE FROM data_privileges WHERE resource_id = ? AND user_id IN ($inQueryUsers)", + [ + $resourceId, + ...$usersIds, + ] + ); + } + + return true; + } catch (Throwable $exception) { + $this->logError('Error when revoking access: ' . $exception->getMessage()); + + return false; + } + } + /** * Remove batch permissions * @@ -395,14 +546,7 @@ public function removeMultiplePermissions(array $data) } } - /** - * Remove batch permissions - * - * @access public - * @param array $data - * @return void - */ - public function removeMultiplePermissionsNew(array $resources) + public function removeResourcesPermissions(array $resources): void { $groupedRemove = []; $eventsData = []; @@ -430,15 +574,14 @@ public function removeMultiplePermissionsNew(array $resources) foreach ($priviligeGroups as $privilegeGroup) { $inQueryPrivilege = implode(',', array_fill(0, count($privilegeGroup['privileges']), ' ? ')); $inQueryResources = implode(',', array_fill(0, count($privilegeGroup['resources']), ' ? ')); - $query = sprintf( - 'DELETE FROM %s WHERE %s IN (%s) AND %s IN (%s) AND %s = ?', - self::TABLE_PRIVILEGES_NAME, - self::COLUMN_RESOURCE_ID, - $inQueryResources, - self::COLUMN_PRIVILEGE, - $inQueryPrivilege, - self::COLUMN_USER_ID - ); + + $query = <<services(); - - $services->set(SavePermissionsStrategy::class, SavePermissionsStrategy::class); - - $services - ->set(ChangePermissionsService::class, ChangePermissionsService::class) - ->public() - ->args([ - service(DataBaseAccess::SERVICE_ID), - service(SavePermissionsStrategy::class), - service(QueueDispatcherInterface::SERVICE_ID), - ]); - } -} \ No newline at end of file diff --git a/model/PermissionsServiceFactory.php b/model/PermissionsServiceFactory.php index 00fc0ac..69fc68a 100644 --- a/model/PermissionsServiceFactory.php +++ b/model/PermissionsServiceFactory.php @@ -40,21 +40,46 @@ class PermissionsServiceFactory extends ConfigurableService */ public function create(): PermissionsService { - if (!$this->hasOption(self::OPTION_SAVE_STRATEGY)) { - throw new RuntimeException( - sprintf('Option %s is not configured. Please check %s', self::OPTION_SAVE_STRATEGY, self::SERVICE_ID) - ); - } + return new PermissionsService( + $this->getDataBaseAccess(), + $this->getPermissionsStrategy(), + $this->getEventManager() + ); + } - /** @var DataBaseAccess $dataBaseAccess */ - $dataBaseAccess = $this->serviceLocator->get(DataBaseAccess::SERVICE_ID); + public function createChangePermissionsService(): ChangePermissionsService + { + return new ChangePermissionsService( + $this->getDataBaseAccess(), + $this->getPermissionsStrategy(), + $this->getEventManager() + ); + } - /** @var EventManager $eventManager */ - $eventManager = $this->serviceLocator->get(EventManager::SERVICE_ID); + private function getDataBaseAccess(): DataBaseAccess + { + return $this->serviceLocator->get(DataBaseAccess::SERVICE_ID); + } + private function getPermissionsStrategy(): PermissionsStrategyInterface + { $strategyClass = $this->getOption(self::OPTION_SAVE_STRATEGY); - $permissionsStrategy = new $strategyClass(); - return new PermissionsService($dataBaseAccess, $permissionsStrategy, $eventManager); + if ($strategyClass === null) { + throw new RuntimeException( + sprintf( + 'Option %s is not configured. Please check %s', + self::OPTION_SAVE_STRATEGY, + self::SERVICE_ID + ) + ); + } + + return new $strategyClass(); + } + + private function getEventManager(): EventManager + { + return $this->serviceLocator->get(EventManager::SERVICE_ID); } } diff --git a/model/event/DacRootChangedEvent.php b/model/event/DacRootChangedEvent.php new file mode 100644 index 0000000..75908fe --- /dev/null +++ b/model/event/DacRootChangedEvent.php @@ -0,0 +1,61 @@ +resourceUri = $resourceUri; + $this->permissionsDelta = $permissionsDelta; + } + + public function getResourceUri(): string + { + return $this->resourceUri; + } + + public function getPermissionsDelta(): array + { + return $this->permissionsDelta; + } + + public function getName(): string + { + return __CLASS__; + } + + public function jsonSerialize(): array + { + return [ + 'resourceUri' => $this->resourceUri, + 'permissionsDelta' => $this->permissionsDelta, + ]; + } +} diff --git a/model/tasks/ChangePermissionsTask.php b/model/tasks/ChangePermissionsTask.php index a708659..306d710 100644 --- a/model/tasks/ChangePermissionsTask.php +++ b/model/tasks/ChangePermissionsTask.php @@ -31,6 +31,7 @@ use oat\taoDacSimple\model\ChangePermissionsService; use Exception; use JsonSerializable; +use oat\taoDacSimple\model\PermissionsServiceFactory; /** * Class ChangePermissionsTask @@ -56,9 +57,7 @@ public function __invoke($params = []): Report $this->validateParams($params); try { - /** @var ChangePermissionsService $changePermissionsService */ - $changePermissionsService = $this->getServiceManager()->getContainer()->get(ChangePermissionsService::class); - $changePermissionsService( + $this->getChangePermissionsService()->change( $this->getResource($params[self::PARAM_RESOURCE]), (array) $params[self::PARAM_PRIVILEGES], filter_var($params[self::PARAM_RECURSIVE] ?? false, FILTER_VALIDATE_BOOL) @@ -92,4 +91,9 @@ public function jsonSerialize(): string { return __CLASS__; } + + private function getChangePermissionsService(): ChangePermissionsService + { + return $this->serviceLocator->get(PermissionsServiceFactory::class)->createChangePermissionsService(); + } } diff --git a/model/tasks/PostChangePermissionsTask.php b/model/tasks/PostChangePermissionsTask.php deleted file mode 100644 index 8c4dcd2..0000000 --- a/model/tasks/PostChangePermissionsTask.php +++ /dev/null @@ -1,119 +0,0 @@ -validateParams($params); - - try { - $reports = []; - $eventManager = $this->getEventManager(); - $permissionsDelta = $params[self::PARAM_PERMISSIONS_DELTA]; - - // TODO Collect all users and do in a single event - foreach ($permissionsDelta['add'] as $userId => $permissions) { - $eventManager->trigger(new DacRootAddedEvent($userId, $params[self::PARAM_RESOURCE_ID], $permissions)); - } - - $reports[] = Report::createInfo('Required permissions successfully added to parent classes'); - - // TODO Collect all users and do in a single event - foreach ($permissionsDelta['remove'] as $userId => $permissions) { - $eventManager->trigger(new DacRootRemovedEvent($userId, $params[self::PARAM_RESOURCE_ID], $permissions)); - } - - $reports[] = Report::createInfo('Not necessary permissions successfully removed from parent classes'); - - $eventManager->trigger( - new DacAffectedUsersEvent( - array_keys($permissionsDelta['add']), - array_keys($permissionsDelta['remove']) - ) - ); - - $reports[] = Report::createInfo('Affected users successfully updated'); - - $eventManager->trigger( - new DataAccessControlChangedEvent( - $params[self::PARAM_RESOURCE_ID], - $permissionsDelta, - $params[self::PARAM_IS_RECURSIVE] - ) - ); - - return Report::createSuccess('Success', null, $reports); - } catch (Exception $exception) { - $errMessage = sprintf('Error: %s', $exception->getMessage()); - $this->getLogger()->error($errMessage); - - return Report::createError($errMessage); - } - } - - public function jsonSerialize(): string - { - return __CLASS__; - } - - private function validateParams(array $params): void - { - foreach (self::MANDATORY_PARAMS as $param) { - if (!isset($params[$param])) { - throw new common_exception_MissingParameter( - sprintf( - 'Missing parameter `%s` in %s', - $param, - self::class - ) - ); - } - } - - $permissionsDelta = $params[self::PARAM_PERMISSIONS_DELTA]; - - if ( - !array_key_exists('add', $permissionsDelta) - || !array_key_exists('remove', $permissionsDelta) - ) { - throw new InvalidArgumentException( - sprintf( - 'Parameter "%s" must contain "add" and "remove" keys', - self::PARAM_PERMISSIONS_DELTA - ) - ); - } - } - - private function getEventManager(): EventManager - { - return $this->getServiceManager()->getContainer()->get(EventManager::SERVICE_ID); - } -} \ No newline at end of file From f6470111b2612e3ec713274ea97abf4580a19f18 Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Tue, 17 Oct 2023 10:11:06 +0300 Subject: [PATCH 06/41] chore: remove unnecessary code --- manifest.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/manifest.php b/manifest.php index 9244ed7..e60e143 100644 --- a/manifest.php +++ b/manifest.php @@ -19,7 +19,6 @@ */ use oat\taoDacSimple\model\Copy\ServiceProvider\CopyServiceProvider; -use oat\taoDacSimple\model\PermissionServiceProvider; use oat\taoDacSimple\scripts\install\AttachEventHandler; use oat\taoDacSimple\scripts\update\Updater; use oat\taoDacSimple\scripts\install\SetupDataAccess; @@ -66,7 +65,6 @@ 'BASE_URL' => ROOT_URL . 'taoDacSimple/', ], 'containerServiceProviders' => [ - CopyServiceProvider::class, - PermissionServiceProvider::class, + CopyServiceProvider::class ] ]; From 6581cab4574ead055ed726b3ef4e376fed9bbe41 Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Tue, 17 Oct 2023 10:13:22 +0300 Subject: [PATCH 07/41] chore: change events order --- model/ChangePermissionsService.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/model/ChangePermissionsService.php b/model/ChangePermissionsService.php index d141f6f..28826c8 100644 --- a/model/ChangePermissionsService.php +++ b/model/ChangePermissionsService.php @@ -186,13 +186,6 @@ private function triggerEvents( $this->eventManager->trigger(new DacRootChangedEvent($resource->getUri(), $permissionsDelta)); } - $this->eventManager->trigger( - new DacAffectedUsersEvent( - array_keys($permissionsDelta['add']), - array_keys($permissionsDelta['remove']) - ) - ); - $this->eventManager->trigger( new DataAccessControlChangedEvent( $resource->getUri(), @@ -200,5 +193,12 @@ private function triggerEvents( $isRecursive ) ); + + $this->eventManager->trigger( + new DacAffectedUsersEvent( + array_keys($permissionsDelta['add']), + array_keys($permissionsDelta['remove']) + ) + ); } } \ No newline at end of file From c377da93beee0b6274c99b4a03f171e416810603 Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Tue, 17 Oct 2023 10:24:35 +0300 Subject: [PATCH 08/41] chore: add license block --- model/ChangePermissionsService.php | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/model/ChangePermissionsService.php b/model/ChangePermissionsService.php index 28826c8..39754f4 100644 --- a/model/ChangePermissionsService.php +++ b/model/ChangePermissionsService.php @@ -1,5 +1,23 @@ Date: Tue, 17 Oct 2023 10:59:48 +0300 Subject: [PATCH 09/41] refactor: keep new methods in a single place, wrap some calls into transaction --- model/DataBaseAccess.php | 353 ++++++++++++++++++++------------------- 1 file changed, 181 insertions(+), 172 deletions(-) diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index 0a2b1f5..ddb4967 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -164,6 +164,187 @@ public function getResourceTree(core_kernel_classes_Resource $resource): array ); } + public function getPermissionsByUsersAndResources(array $userIds, array $resourceIds): array + { + // Permissions for an empty set of resources must be an empty array + if (empty($resourceIds)) { + return []; + } + + $inQueryResources = implode(',', array_fill(0, count($resourceIds), '?')); + $inQueryUsers = implode(',', array_fill(0, count($userIds), '?')); + + $query = <<fetchQuery($query, [...$resourceIds, ...$userIds]); + + // If resource doesn't have permission don't return null + $data = array_fill_keys($resourceIds, []); + + foreach ($results as $result) { + $data[$result[self::COLUMN_RESOURCE_ID]][$result[self::COLUMN_USER_ID]][] = $result[self::COLUMN_PRIVILEGE]; + } + + return $data; + } + + public function addResourcesPermissions(array $resources): void + { + $insert = []; + + foreach ($resources as $resource) { + foreach ($resource['permissions']['add'] as $userId => $addPermissions) { + if (empty($addPermissions)) { + continue; + } + + foreach ($addPermissions as $permission) { + $insert[] = [ + self::COLUMN_USER_ID => $userId, + self::COLUMN_RESOURCE_ID => $resource['id'], + self::COLUMN_PRIVILEGE => $permission, + ]; + } + } + } + + $this->insertPermissions($insert); + + foreach ($insert as $inserted) { + $this->getEventManager()->trigger( + new DacAddedEvent( + $inserted[self::COLUMN_USER_ID], + $inserted[self::COLUMN_RESOURCE_ID], + (array) $inserted[self::COLUMN_PRIVILEGE] + ) + ); + } + } + + public function removeResourcesPermissions(array $resources): void + { + $groupedRemove = []; + $eventsData = []; + + foreach ($resources as $resource) { + foreach ($resource['permissions']['remove'] as $userId => $removePermissions) { + if (empty($removePermissions)) { + continue; + } + + $idString = implode($removePermissions); + + $groupedRemove[$userId][$idString]['resources'][] = $resource['id']; + $groupedRemove[$userId][$idString]['privileges'] = $removePermissions; + + $eventsData[] = [ + 'userId' => $userId, + 'resourceId' => $resource['id'], + 'privileges' => $removePermissions, + ]; + } + } + + foreach ($groupedRemove as $userId => $priviligeGroups) { + foreach ($priviligeGroups as $privilegeGroup) { + $inQueryPrivilege = implode(',', array_fill(0, count($privilegeGroup['privileges']), ' ? ')); + $inQueryResources = implode(',', array_fill(0, count($privilegeGroup['resources']), ' ? ')); + + $query = <<getPersistence()->exec($query, $params); + } + } + + foreach ($eventsData as $eventData) { + $this->getEventManager()->trigger(new DacRemovedEvent( + $eventData['userId'], + $eventData['resourceId'], + $eventData['privileges'] + )); + } + } + + public function addReadAccess(array $addAccessList): bool + { + if (empty($addAccessList)) { + return true; + } + + $values = []; + + try { + foreach ($addAccessList as $resourceId => $usersIds) { + foreach ($usersIds as $userId) { + $values[] = [ + 'user_id' => $userId, + 'resource_id' => $resourceId, + 'privilege' => PermissionProvider::PERMISSION_READ + ]; + } + } + + $this->insertPermissions($values); + + return true; + } catch (Throwable $exception) { + $this->logError('Error when adding READ access: ' . $exception->getMessage()); + + return false; + } + } + + public function revokeAccess(array $revokeAccessList): bool + { + if (empty($revokeAccessList)) { + return true; + } + + $persistence = $this->getPersistence(); + + try { + $persistence->transactional(static function () use ($revokeAccessList, $persistence): void { + foreach ($revokeAccessList as $resourceId => $usersIds) { + $inQueryUsers = implode(',', array_fill(0, count($usersIds), ' ? ')); + + $persistence->exec( + "DELETE FROM data_privileges WHERE resource_id = ? AND user_id IN ($inQueryUsers)", + [ + $resourceId, + ...$usersIds, + ] + ); + } + }); + + return true; + } catch (Throwable $exception) { + $this->logError('Error when revoking access: ' . $exception->getMessage()); + + return false; + } + } + /** * Retrieve info on users having privileges on a set of resources * @@ -226,38 +407,6 @@ public function getPermissions(array $userIds, array $resourceIds): array return $returnValue; } - public function getPermissionsByUsersAndResources(array $userIds, array $resourceIds): array - { - // Permissions for an empty set of resources must be an empty array - if (empty($resourceIds)) { - return []; - } - - $inQueryResources = implode(',', array_fill(0, count($resourceIds), '?')); - $inQueryUsers = implode(',', array_fill(0, count($userIds), '?')); - - $query = <<fetchQuery($query, [...$resourceIds, ...$userIds]); - - // If resource doesn't have permission don't return null - $data = array_fill_keys($resourceIds, []); - - foreach ($results as $result) { - $data[$result[self::COLUMN_RESOURCE_ID]][$result[self::COLUMN_USER_ID]][] = $result[self::COLUMN_PRIVILEGE]; - } - - return $data; - } - public function getResourcesPermissions(array $resourceIds) { // Return an empty array for resources not having permissions data @@ -340,39 +489,6 @@ public function addMultiplePermissions(array $permissionData) } } - public function addResourcesPermissions(array $resources): void - { - $insert = []; - - foreach ($resources as $resource) { - foreach ($resource['permissions']['add'] as $userId => $addPermissions) { - if (empty($addPermissions)) { - continue; - } - - foreach ($addPermissions as $permission) { - $insert[] = [ - self::COLUMN_USER_ID => $userId, - self::COLUMN_RESOURCE_ID => $resource['id'], - self::COLUMN_PRIVILEGE => $permission, - ]; - } - } - } - - $this->insertPermissions($insert); - - foreach ($insert as $inserted) { - $this->getEventManager()->trigger( - new DacAddedEvent( - $inserted[self::COLUMN_USER_ID], - $inserted[self::COLUMN_RESOURCE_ID], - (array) $inserted[self::COLUMN_PRIVILEGE] - ) - ); - } - } - /** * Get the permissions to resource * @@ -432,57 +548,6 @@ public function removePermissions($user, $resourceId, $rights) return true; } - public function addReadAccess(array $addAccessList): bool - { - if (empty($addAccessList)) { - return true; - } - - $values = []; - $valuesQuery = []; - - foreach ($addAccessList as $resourceId => $usersIds) { - foreach ($usersIds as $userId) { - $values = array_merge($values, [$userId, $resourceId, PermissionProvider::PERMISSION_READ]); - $valuesQuery[] = '(?, ?, ?)'; - } - } - - $statement = sprintf( - 'INSERT INTO data_privileges (user_id, resource_id, privilege) VALUES %s', - implode(', ', $valuesQuery) - ); - - return $this->getPersistence()->exec($statement, $values); - } - - public function revokeAccess(array $revokeAccessList): bool - { - if (empty($revokeAccessList)) { - return true; - } - - try { - foreach ($revokeAccessList as $resourceId => $usersIds) { - $inQueryUsers = implode(',', array_fill(0, count($usersIds), ' ? ')); - - $this->getPersistence()->exec( - "DELETE FROM data_privileges WHERE resource_id = ? AND user_id IN ($inQueryUsers)", - [ - $resourceId, - ...$usersIds, - ] - ); - } - - return true; - } catch (Throwable $exception) { - $this->logError('Error when revoking access: ' . $exception->getMessage()); - - return false; - } - } - /** * Remove batch permissions * @@ -546,62 +611,6 @@ public function removeMultiplePermissions(array $data) } } - public function removeResourcesPermissions(array $resources): void - { - $groupedRemove = []; - $eventsData = []; - - foreach ($resources as $resource) { - foreach ($resource['permissions']['remove'] as $userId => $removePermissions) { - if (empty($removePermissions)) { - continue; - } - - $idString = implode($removePermissions); - - $groupedRemove[$userId][$idString]['resources'][] = $resource['id']; - $groupedRemove[$userId][$idString]['privileges'] = $removePermissions; - - $eventsData[] = [ - 'userId' => $userId, - 'resourceId' => $resource['id'], - 'privileges' => $removePermissions, - ]; - } - } - - foreach ($groupedRemove as $userId => $priviligeGroups) { - foreach ($priviligeGroups as $privilegeGroup) { - $inQueryPrivilege = implode(',', array_fill(0, count($privilegeGroup['privileges']), ' ? ')); - $inQueryResources = implode(',', array_fill(0, count($privilegeGroup['resources']), ' ? ')); - - $query = <<getPersistence()->exec($query, $params); - } - } - - foreach ($eventsData as $eventData) { - $this->getEventManager()->trigger(new DacRemovedEvent( - $eventData['userId'], - $eventData['resourceId'], - $eventData['privileges'] - )); - } - } - /** * Remove all permissions from a resource * From abab2a675b255d7ae6beff3796421374780a6c9a Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Wed, 18 Oct 2023 10:12:22 +0300 Subject: [PATCH 10/41] chore: build tree based on provided parent classes to keep the right order --- model/DataBaseAccess.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index ddb4967..c290ce5 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -122,13 +122,14 @@ public function getClassesResources(array $classesIds): array ] ); - $classesResources = []; + $tree = []; - foreach ($results as $result) { - $classesResources[$result['object']][] = $result['subject']; + foreach ($classesIds as $classId) { + $resources = array_filter($results, static fn (array $result): bool => $result['object'] === $classId); + $tree[$classId] = array_column($resources, 'subject'); } - return $classesResources; + return $tree; } public function getResourceTree(core_kernel_classes_Resource $resource): array From 3606046442bf4baabfe716203e1b72711acebd81 Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Wed, 18 Oct 2023 11:21:05 +0300 Subject: [PATCH 11/41] chore: normalize request instead of taking delta directly --- model/ChangePermissionsService.php | 2 +- model/SyncPermissionsStrategy.php | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/model/ChangePermissionsService.php b/model/ChangePermissionsService.php index 39754f4..329aa7b 100644 --- a/model/ChangePermissionsService.php +++ b/model/ChangePermissionsService.php @@ -51,7 +51,7 @@ public function change(core_kernel_classes_Resource $resource, array $permission $this->enrichWithPermissions($resources); $rootResourcePermissions = $resources[$resource->getUri()]['permissions']; - $permissionsDelta = $this->strategy->getDeltaPermissions( + $permissionsDelta = $this->strategy->normalizeRequest( $rootResourcePermissions['current'] ?? [], $permissionsToSet ); diff --git a/model/SyncPermissionsStrategy.php b/model/SyncPermissionsStrategy.php index 70b9ad1..c67a880 100644 --- a/model/SyncPermissionsStrategy.php +++ b/model/SyncPermissionsStrategy.php @@ -29,7 +29,8 @@ public function normalizeRequest(array $currentPrivileges, array $privilegesToSe { // we are going to add everything what current item has and remove the rest return [ - 'add' => $privilegesToSet + 'add' => $privilegesToSet, + 'remove' => [], ]; } From 910d4dd7a0de0e6bce525d4fffd9fe568dbdb060 Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Wed, 18 Oct 2023 17:59:51 +0300 Subject: [PATCH 12/41] chore: clear code, fix php7.4 issue --- model/ChangePermissionsService.php | 6 +----- model/DataBaseAccess.php | 8 +++----- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/model/ChangePermissionsService.php b/model/ChangePermissionsService.php index 329aa7b..91f361e 100644 --- a/model/ChangePermissionsService.php +++ b/model/ChangePermissionsService.php @@ -73,11 +73,7 @@ private function getResourceToUpdate(core_kernel_classes_Resource $resource, boo $resources = []; foreach ($this->dataBaseAccess->getResourceTree($resource) as $result) { - $resources[$result['subject']] = [ - 'id' => $result['subject'], - 'isClass' => $result['predicate'] === OntologyRdfs::RDFS_SUBCLASSOF, - 'level' => $result['level'] - ]; + $resources[$result['id']] = $result; } return $resources; diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index c290ce5..f421510 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -152,7 +152,7 @@ public function getResourceTree(core_kernel_classes_Resource $resource): array JOIN statements_tree st ON s.object = st.subject ) -SELECT subject, predicate, level FROM statements_tree; +SELECT subject as id, IF(predicate = ?, 1, 0) as isClass, level FROM statements_tree; SQL; return $this->fetchQuery( @@ -161,6 +161,7 @@ public function getResourceTree(core_kernel_classes_Resource $resource): array $resource->getUri(), OntologyRdfs::RDFS_SUBCLASSOF, OntologyRdf::RDF_TYPE, + OntologyRdfs::RDFS_SUBCLASSOF, ] ); } @@ -330,10 +331,7 @@ public function revokeAccess(array $revokeAccessList): bool $persistence->exec( "DELETE FROM data_privileges WHERE resource_id = ? AND user_id IN ($inQueryUsers)", - [ - $resourceId, - ...$usersIds, - ] + array_merge([$resourceId], array_values($usersIds)) ); } }); From 35b9a118ff6d5846fec635e9e4fcb575adf7fdc4 Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Wed, 18 Oct 2023 17:32:42 +0200 Subject: [PATCH 13/41] chore: use command object --- model/ChangePermissionsService.php | 12 ++++++------ model/tasks/ChangePermissionsTask.php | 12 ++++++++++-- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/model/ChangePermissionsService.php b/model/ChangePermissionsService.php index 91f361e..5103277 100644 --- a/model/ChangePermissionsService.php +++ b/model/ChangePermissionsService.php @@ -23,9 +23,9 @@ namespace oat\taoDacSimple\model; use core_kernel_classes_Resource; -use oat\generis\model\OntologyRdfs; use oat\oatbox\event\EventManager; use oat\tao\model\event\DataAccessControlChangedEvent; +use oat\taoDacSimple\model\Command\ChangePermissionsCommand; use oat\taoDacSimple\model\event\DacAffectedUsersEvent; use oat\taoDacSimple\model\event\DacRootChangedEvent; @@ -45,15 +45,15 @@ public function __construct( $this->eventManager = $eventManager; } - public function change(core_kernel_classes_Resource $resource, array $permissionsToSet, bool $isRecursive): void + public function change(ChangePermissionsCommand $command): void { - $resources = $this->getResourceToUpdate($resource, $isRecursive); + $resources = $this->getResourceToUpdate($command->getRoot(), $command->isRecursive()); $this->enrichWithPermissions($resources); - $rootResourcePermissions = $resources[$resource->getUri()]['permissions']; + $rootResourcePermissions = $resources[$command->getRoot()->getUri()]['permissions']; $permissionsDelta = $this->strategy->normalizeRequest( $rootResourcePermissions['current'] ?? [], - $permissionsToSet + $command->getPrivilegesPerUser() ); if (empty($permissionsDelta['remove']) && empty($permissionsDelta['add'])) { @@ -64,7 +64,7 @@ public function change(core_kernel_classes_Resource $resource, array $permission $this->dryRun($resources); $this->wetRun($resources); - $this->triggerEvents($resource, $permissionsDelta, $isRecursive); + $this->triggerEvents($command->getRoot(), $permissionsDelta, $command->isRecursive()); } private function getResourceToUpdate(core_kernel_classes_Resource $resource, bool $isRecursive): array diff --git a/model/tasks/ChangePermissionsTask.php b/model/tasks/ChangePermissionsTask.php index 306d710..bb70cb7 100644 --- a/model/tasks/ChangePermissionsTask.php +++ b/model/tasks/ChangePermissionsTask.php @@ -31,6 +31,7 @@ use oat\taoDacSimple\model\ChangePermissionsService; use Exception; use JsonSerializable; +use oat\taoDacSimple\model\Command\ChangePermissionsCommand; use oat\taoDacSimple\model\PermissionsServiceFactory; /** @@ -57,12 +58,19 @@ public function __invoke($params = []): Report $this->validateParams($params); try { - $this->getChangePermissionsService()->change( + $command = new ChangePermissionsCommand( $this->getResource($params[self::PARAM_RESOURCE]), + $params[self::PARAM_RESOURCE], (array) $params[self::PARAM_PRIVILEGES], - filter_var($params[self::PARAM_RECURSIVE] ?? false, FILTER_VALIDATE_BOOL) + [] ); + if (filter_var($params[self::PARAM_RECURSIVE] ?? false, FILTER_VALIDATE_BOOL)) { + $command->withRecursion(); + } + + $this->getChangePermissionsService()->change($command); + return Report::createSuccess('Permissions saved'); } catch (Exception $e) { $errMessage = sprintf('Saving permissions failed: %s', $e->getMessage()); From dc476e6764284bb08fc2e66079e17e83e43b48d3 Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Thu, 19 Oct 2023 08:59:00 +0200 Subject: [PATCH 14/41] chore: merge methods to avoid double loops on all resources + mark TODO list --- model/ChangePermissionsService.php | 9 +-- model/DataBaseAccess.php | 42 ++++++------ model/PermissionsService.php | 11 ++++ .../model/ChangePermissionsServiceTest.php | 64 +++++++++++++++++++ test/unit/model/DataBaseAccessTest.php | 5 ++ 5 files changed, 103 insertions(+), 28 deletions(-) create mode 100644 test/unit/model/ChangePermissionsServiceTest.php diff --git a/model/ChangePermissionsService.php b/model/ChangePermissionsService.php index 5103277..eeaecad 100644 --- a/model/ChangePermissionsService.php +++ b/model/ChangePermissionsService.php @@ -62,7 +62,8 @@ public function change(ChangePermissionsCommand $command): void $resources = $this->enrichWithAddRemoveActions($resources, $permissionsDelta); $this->dryRun($resources); - $this->wetRun($resources); + + $this->dataBaseAccess->changeResourcePermissions($resources); $this->triggerEvents($command->getRoot(), $permissionsDelta, $command->isRecursive()); } @@ -185,12 +186,6 @@ private function assertHasUserWithGrantPermission(string $resourceId, array $new ); } - private function wetRun(array $resources): void - { - $this->dataBaseAccess->removeResourcesPermissions($resources); - $this->dataBaseAccess->addResourcesPermissions($resources); - } - private function triggerEvents( core_kernel_classes_Resource $resource, array $permissionsDelta, diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index f421510..0735472 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -70,6 +70,7 @@ protected function getEventManager() public function getParentClassesIds(string $resourceUri): array { + //@TODO @FIXME Migrate method to generis $query = <<<'SQL' WITH RECURSIVE statements_tree AS ( SELECT @@ -110,6 +111,7 @@ public function getParentClassesIds(string $resourceUri): array public function getClassesResources(array $classesIds): array { + //@TODO @FIXME Migrate method to generis $inQuery = implode(',', array_fill(0, count($classesIds), '?')); $query = "SELECT subject, object FROM statements WHERE predicate IN (?, ?) AND object IN ($inQuery)"; @@ -134,6 +136,7 @@ public function getClassesResources(array $classesIds): array public function getResourceTree(core_kernel_classes_Resource $resource): array { + //@TODO @FIXME Migrate method to generis $query = <<<'SQL' WITH RECURSIVE statements_tree AS ( SELECT @@ -198,9 +201,12 @@ public function getPermissionsByUsersAndResources(array $userIds, array $resourc return $data; } - public function addResourcesPermissions(array $resources): void + public function changeResourcePermissions(array $resources): void { + //@TODO Add a proper object as parameter rather than an array with unknown content $insert = []; + $groupedRemove = []; + $eventsData = []; foreach ($resources as $resource) { foreach ($resource['permissions']['add'] as $userId => $addPermissions) { @@ -216,27 +222,7 @@ public function addResourcesPermissions(array $resources): void ]; } } - } - - $this->insertPermissions($insert); - foreach ($insert as $inserted) { - $this->getEventManager()->trigger( - new DacAddedEvent( - $inserted[self::COLUMN_USER_ID], - $inserted[self::COLUMN_RESOURCE_ID], - (array) $inserted[self::COLUMN_PRIVILEGE] - ) - ); - } - } - - public function removeResourcesPermissions(array $resources): void - { - $groupedRemove = []; - $eventsData = []; - - foreach ($resources as $resource) { foreach ($resource['permissions']['remove'] as $userId => $removePermissions) { if (empty($removePermissions)) { continue; @@ -255,6 +241,19 @@ public function removeResourcesPermissions(array $resources): void } } + $this->insertPermissions($insert); + + foreach ($insert as $inserted) { + //@TODO @FIXME Do a single event to add multiple event logs entries + $this->getEventManager()->trigger( + new DacAddedEvent( + $inserted[self::COLUMN_USER_ID], + $inserted[self::COLUMN_RESOURCE_ID], + (array) $inserted[self::COLUMN_PRIVILEGE] + ) + ); + } + foreach ($groupedRemove as $userId => $priviligeGroups) { foreach ($priviligeGroups as $privilegeGroup) { $inQueryPrivilege = implode(',', array_fill(0, count($privilegeGroup['privileges']), ' ? ')); @@ -278,6 +277,7 @@ public function removeResourcesPermissions(array $resources): void } } + //@TODO @FIXME Do a single event to add multiple event logs entries foreach ($eventsData as $eventData) { $this->getEventManager()->trigger(new DacRemovedEvent( $eventData['userId'], diff --git a/model/PermissionsService.php b/model/PermissionsService.php index 43c7512..8c28388 100644 --- a/model/PermissionsService.php +++ b/model/PermissionsService.php @@ -32,6 +32,9 @@ use oat\taoDacSimple\model\event\DacRootAddedEvent; use oat\taoDacSimple\model\event\DacRootRemovedEvent; +/** + * @deprecated Use oat\taoDacSimple\model\ChangePermissionsService + */ class PermissionsService { use LoggerAwareTrait; @@ -52,6 +55,9 @@ public function __construct( $this->eventManager = $eventManager; } + /** + * @deprecated Use oat\taoDacSimple\model\ChangePermissionsService::change + */ public function applyPermissions(ChangePermissionsCommand $command): void { $resources = $this->getResourcesToUpdate($command); @@ -76,6 +82,7 @@ public function applyPermissions(ChangePermissionsCommand $command): void /** * @deprecated Use applyPermissions() instead + * @deprecated Use oat\taoDacSimple\model\ChangePermissionsService::change * @see applyPermissions */ public function saveResourcePermissionsRecursive( @@ -96,6 +103,7 @@ public function saveResourcePermissionsRecursive( /** * @deprecated Use applyPermissions() instead + * @deprecated Use oat\taoDacSimple\model\ChangePermissionsService::change * @see applyPermissions */ public function savePermissions( @@ -118,6 +126,9 @@ public function savePermissions( $this->applyPermissions($command); } + /** + * @deprecated This should be a private method, please use oat\taoDacSimple\model\ChangePermissionsService::change + */ public function getPermissionsDelta(core_kernel_classes_Resource $resource, array $permissionsToSet): array { $currentPermissions = $this->dataBaseAccess->getResourcePermissions($resource->getUri()); diff --git a/test/unit/model/ChangePermissionsServiceTest.php b/test/unit/model/ChangePermissionsServiceTest.php new file mode 100644 index 0000000..9526679 --- /dev/null +++ b/test/unit/model/ChangePermissionsServiceTest.php @@ -0,0 +1,64 @@ +eventManager = $this->createMock(EventManager::class); + $this->databaseAccess = $this->createMock(DataBaseAccess::class); + $this->strategy = $this->createMock(PermissionsStrategyInterface::class); + $this->service = new ChangePermissionsService( + $this->databaseAccess, + $this->strategy, + $this->eventManager + ); + + $this->service->setLogger($this->createMock(LoggerInterface::class)); + } + + public function testChange(): void + { + //@TODO Complete test before release this code + + $this->markTestIncomplete(); + } +} diff --git a/test/unit/model/DataBaseAccessTest.php b/test/unit/model/DataBaseAccessTest.php index 192e325..59aef58 100644 --- a/test/unit/model/DataBaseAccessTest.php +++ b/test/unit/model/DataBaseAccessTest.php @@ -296,6 +296,11 @@ private function getResourceMock(string $id): core_kernel_classes_Resource return $resourceMock; } + + public function sdsadsadsad() + { + + } } /** From 3f45f69e5688b941beb6b8205ce582cdfe03efe1 Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Thu, 19 Oct 2023 10:04:40 +0200 Subject: [PATCH 15/41] chore: only use real resources for data privileges --- model/DataBaseAccess.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index 0735472..c10a34e 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -154,6 +154,7 @@ public function getResourceTree(core_kernel_classes_Resource $resource): array FROM statements s JOIN statements_tree st ON s.object = st.subject + WHERE s.predicate IN (?, ?) ) SELECT subject as id, IF(predicate = ?, 1, 0) as isClass, level FROM statements_tree; SQL; @@ -165,6 +166,8 @@ public function getResourceTree(core_kernel_classes_Resource $resource): array OntologyRdfs::RDFS_SUBCLASSOF, OntologyRdf::RDF_TYPE, OntologyRdfs::RDFS_SUBCLASSOF, + OntologyRdf::RDF_TYPE, + OntologyRdfs::RDFS_SUBCLASSOF, ] ); } From 582af00a0fcbcfd920ccaac7fc6a22c669eaa9ca Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Thu, 19 Oct 2023 12:40:11 +0300 Subject: [PATCH 16/41] chore: do not add permissions for the AssessmentContentObject --- model/DataBaseAccess.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index c10a34e..a36e2fe 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -78,6 +78,7 @@ public function getParentClassesIds(string $resourceUri): array FROM statements r WHERE r.subject = ? AND r.predicate IN (?, ?) + AND r.object != ? UNION ALL SELECT s.object @@ -97,6 +98,7 @@ public function getParentClassesIds(string $resourceUri): array $resourceUri, OntologyRdfs::RDFS_SUBCLASSOF, OntologyRdf::RDF_TYPE, + 'http://www.tao.lu/Ontologies/TAO.rdf#AssessmentContentObject', OntologyRdfs::RDFS_SUBCLASSOF, OntologyRdf::RDF_TYPE, 'http://www.tao.lu/Ontologies/TAO.rdf#AssessmentContentObject', From 35f3c324430b728c8ab920af031d5c980a13b6fc Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Thu, 19 Oct 2023 13:55:38 +0200 Subject: [PATCH 17/41] chore: use methods from ontology to get resources info --- composer.json | 2 +- model/ChangePermissionsService.php | 4 +- model/DataBaseAccess.php | 108 ---------------------------- model/event/DacRootChangedEvent.php | 13 ++-- 4 files changed, 10 insertions(+), 117 deletions(-) diff --git a/composer.json b/composer.json index 28a2a63..5d1fe64 100644 --- a/composer.json +++ b/composer.json @@ -21,7 +21,7 @@ "minimum-stability": "dev", "require": { "oat-sa/oatbox-extension-installer": "~1.1||dev-master", - "oat-sa/generis": ">=15.24", + "oat-sa/generis": "dev-feature/adf-1590/optimize-permissions-task-queue as 99.99", "oat-sa/tao-core": "dev-feature/adf-1590/optimize-permissions-task-queue as 99.99", "oat-sa/extension-tao-backoffice": ">=6.0.0", "oat-sa/extension-tao-item": ">=11.3" diff --git a/model/ChangePermissionsService.php b/model/ChangePermissionsService.php index eeaecad..ee806f5 100644 --- a/model/ChangePermissionsService.php +++ b/model/ChangePermissionsService.php @@ -73,7 +73,7 @@ private function getResourceToUpdate(core_kernel_classes_Resource $resource, boo if ($isRecursive) { $resources = []; - foreach ($this->dataBaseAccess->getResourceTree($resource) as $result) { + foreach ($resource->getNestedResources() as $result) { $resources[$result['id']] = $result; } @@ -192,7 +192,7 @@ private function triggerEvents( bool $isRecursive ): void { if (!empty($permissionsDelta['add']) || !empty($permissionsDelta['remove'])) { - $this->eventManager->trigger(new DacRootChangedEvent($resource->getUri(), $permissionsDelta)); + $this->eventManager->trigger(new DacRootChangedEvent($resource, $permissionsDelta)); } $this->eventManager->trigger( diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index a36e2fe..98ba152 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -21,9 +21,6 @@ namespace oat\taoDacSimple\model; use common_persistence_SqlPersistence; -use core_kernel_classes_Resource; -use oat\generis\model\OntologyRdf; -use oat\generis\model\OntologyRdfs; use oat\oatbox\event\EventManager; use oat\oatbox\service\ConfigurableService; use oat\taoDacSimple\model\event\DacAddedEvent; @@ -68,111 +65,6 @@ protected function getEventManager() return $this->getServiceLocator()->get(EventManager::SERVICE_ID); } - public function getParentClassesIds(string $resourceUri): array - { - //@TODO @FIXME Migrate method to generis - $query = <<<'SQL' -WITH RECURSIVE statements_tree AS ( - SELECT - r.object - FROM statements r - WHERE r.subject = ? - AND r.predicate IN (?, ?) - AND r.object != ? - UNION ALL - SELECT - s.object - FROM statements s - JOIN statements_tree st - ON s.subject = st.object - AND s.predicate IN (?, ?) - AND s.object NOT IN (?, ?, ?, ?) -) -SELECT object FROM statements_tree; -SQL; - - return array_column( - $this->fetchQuery( - $query, - [ - $resourceUri, - OntologyRdfs::RDFS_SUBCLASSOF, - OntologyRdf::RDF_TYPE, - 'http://www.tao.lu/Ontologies/TAO.rdf#AssessmentContentObject', - OntologyRdfs::RDFS_SUBCLASSOF, - OntologyRdf::RDF_TYPE, - 'http://www.tao.lu/Ontologies/TAO.rdf#AssessmentContentObject', - 'http://www.tao.lu/Ontologies/TAO.rdf#TAOObject', - 'http://www.tao.lu/Ontologies/generis.rdf#generis_Ressource', - 'http://www.w3.org/2000/01/rdf-schema#Resource', - ] - ), - 'object' - ); - } - - public function getClassesResources(array $classesIds): array - { - //@TODO @FIXME Migrate method to generis - $inQuery = implode(',', array_fill(0, count($classesIds), '?')); - $query = "SELECT subject, object FROM statements WHERE predicate IN (?, ?) AND object IN ($inQuery)"; - - $results = $this->fetchQuery( - $query, - [ - OntologyRdfs::RDFS_SUBCLASSOF, - OntologyRdf::RDF_TYPE, - ...$classesIds, - ] - ); - - $tree = []; - - foreach ($classesIds as $classId) { - $resources = array_filter($results, static fn (array $result): bool => $result['object'] === $classId); - $tree[$classId] = array_column($resources, 'subject'); - } - - return $tree; - } - - public function getResourceTree(core_kernel_classes_Resource $resource): array - { - //@TODO @FIXME Migrate method to generis - $query = <<<'SQL' -WITH RECURSIVE statements_tree AS ( - SELECT - r.subject, - r.predicate, - 1 as level - FROM statements r - WHERE r.subject = ? - AND r.predicate IN (?, ?) - UNION ALL - SELECT - s.subject, - s.predicate, - level + 1 - FROM statements s - JOIN statements_tree st - ON s.object = st.subject - WHERE s.predicate IN (?, ?) -) -SELECT subject as id, IF(predicate = ?, 1, 0) as isClass, level FROM statements_tree; -SQL; - - return $this->fetchQuery( - $query, - [ - $resource->getUri(), - OntologyRdfs::RDFS_SUBCLASSOF, - OntologyRdf::RDF_TYPE, - OntologyRdfs::RDFS_SUBCLASSOF, - OntologyRdf::RDF_TYPE, - OntologyRdfs::RDFS_SUBCLASSOF, - ] - ); - } public function getPermissionsByUsersAndResources(array $userIds, array $resourceIds): array { diff --git a/model/event/DacRootChangedEvent.php b/model/event/DacRootChangedEvent.php index 75908fe..6fa86e7 100644 --- a/model/event/DacRootChangedEvent.php +++ b/model/event/DacRootChangedEvent.php @@ -22,23 +22,24 @@ namespace oat\taoDacSimple\model\event; +use core_kernel_classes_Resource; use JsonSerializable; use oat\oatbox\event\Event; class DacRootChangedEvent implements Event, JsonSerializable { - private string $resourceUri; + private core_kernel_classes_Resource $resource; private array $permissionsDelta; - public function __construct(string $resourceUri, array $permissionsDelta) + public function __construct(core_kernel_classes_Resource $resource, array $permissionsDelta) { - $this->resourceUri = $resourceUri; + $this->resource = $resource; $this->permissionsDelta = $permissionsDelta; } - public function getResourceUri(): string + public function getResource(): core_kernel_classes_Resource { - return $this->resourceUri; + return $this->resource; } public function getPermissionsDelta(): array @@ -54,7 +55,7 @@ public function getName(): string public function jsonSerialize(): array { return [ - 'resourceUri' => $this->resourceUri, + 'resourceUri' => $this->getResource()->getUri(), 'permissionsDelta' => $this->permissionsDelta, ]; } From 88ab9adf235b56257f6d06a0e1713874592784fb Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Thu, 19 Oct 2023 15:18:52 +0200 Subject: [PATCH 18/41] chore: deal with proper command object rather than unknown array --- model/Command/RevokeAccessCommand.php | 71 +++++++++++++++++++++++++++ model/DataBaseAccess.php | 19 ++++--- 2 files changed, 84 insertions(+), 6 deletions(-) create mode 100644 model/Command/RevokeAccessCommand.php diff --git a/model/Command/RevokeAccessCommand.php b/model/Command/RevokeAccessCommand.php new file mode 100644 index 0000000..5eadc8c --- /dev/null +++ b/model/Command/RevokeAccessCommand.php @@ -0,0 +1,71 @@ + ['userId1', 'userId2']] + * + * @var string[][] + */ + private array $resourceMap = []; + + public function __construct() { + } + + public function revokeResourceForUser(string $resourceId, string $userId): void + { + $this->resourceMap[$resourceId] = $this->resourceMap[$resourceId] ?? []; + $this->resourceMap[$resourceId] = array_unique(array_merge($this->resourceMap[$resourceId], [$userId])); + } + + public function cancelRevokeResourceForUser(string $resourceId, string $userId): void + { + $this->resourceMap[$resourceId] = $this->resourceMap[$resourceId] ?? []; + + $key = array_search($userId, $this->resourceMap[$resourceId]); + + if ($key === false) { + return; + } + + unset($this->resourceMap[$resourceId][$key]); + } + + public function getResourceIdsToRevoke(): array + { + return array_keys($this->resourceMap); + } + + public function getUserIdsToRevoke(string $resourceId): array + { + return $this->resourceMap[$resourceId] ?? []; + } +} diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index 98ba152..08a0cf7 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -23,6 +23,7 @@ use common_persistence_SqlPersistence; use oat\oatbox\event\EventManager; use oat\oatbox\service\ConfigurableService; +use oat\taoDacSimple\model\Command\RevokeAccessCommand; use oat\taoDacSimple\model\event\DacAddedEvent; use oat\taoDacSimple\model\event\DacRemovedEvent; use oat\generis\persistence\PersistenceManager; @@ -186,6 +187,7 @@ public function changeResourcePermissions(array $resources): void public function addReadAccess(array $addAccessList): bool { + //@TODO Use proper object in the command instead of an array if (empty($addAccessList)) { return true; } @@ -213,21 +215,26 @@ public function addReadAccess(array $addAccessList): bool } } - public function revokeAccess(array $revokeAccessList): bool + public function revokeAccess(RevokeAccessCommand $revokeAccess): bool { - if (empty($revokeAccessList)) { + $resourceIds = $revokeAccess->getResourceIdsToRevoke(); + + if (empty($resourceIds)) { return true; } $persistence = $this->getPersistence(); try { - $persistence->transactional(static function () use ($revokeAccessList, $persistence): void { - foreach ($revokeAccessList as $resourceId => $usersIds) { - $inQueryUsers = implode(',', array_fill(0, count($usersIds), ' ? ')); + $persistence->transactional(static function () use ($resourceIds, $revokeAccess, $persistence): void { + foreach ($resourceIds as $resourceId) { + $usersIds = $revokeAccess->getUserIdsToRevoke($resourceId); $persistence->exec( - "DELETE FROM data_privileges WHERE resource_id = ? AND user_id IN ($inQueryUsers)", + sprintf( + 'DELETE FROM data_privileges WHERE resource_id = ? AND user_id IN (%s)', + implode(',', array_fill(0, count($usersIds), ' ? ')) + ), array_merge([$resourceId], array_values($usersIds)) ); } From b31e61dbe74bcda6e852f6bae04fb70c79972c21 Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Thu, 19 Oct 2023 16:03:14 +0200 Subject: [PATCH 19/41] feat: create new command to change all permissions --- model/Command/ChangeAccessCommand.php | 94 +++++++++++++++++++++++++++ model/Command/RevokeAccessCommand.php | 71 -------------------- model/DataBaseAccess.php | 94 +++++++++++++-------------- model/PermissionProvider.php | 5 ++ 4 files changed, 144 insertions(+), 120 deletions(-) create mode 100644 model/Command/ChangeAccessCommand.php delete mode 100644 model/Command/RevokeAccessCommand.php diff --git a/model/Command/ChangeAccessCommand.php b/model/Command/ChangeAccessCommand.php new file mode 100644 index 0000000..ecf485c --- /dev/null +++ b/model/Command/ChangeAccessCommand.php @@ -0,0 +1,94 @@ + ['userId1', 'userId2']] + * + * @var string[][] + */ + private array $removeAccessMap = []; + + /** + * An array in the form ['resourceId' [ 'READ' => ['userId1', 'userId2']]] + * + * @var string[][][] + */ + private array $giveAccessMap = []; + + public function __construct() { + } + + public function revokeResourceForUser(string $resourceId, string $userId): void + { + $this->removeAccessMap[$resourceId] = $this->removeAccessMap[$resourceId] ?? []; + $this->removeAccessMap[$resourceId] = array_unique(array_merge($this->removeAccessMap[$resourceId], [$userId])); + } + + public function cancelRevokeResourceForUser(string $resourceId, string $userId): void + { + $this->removeAccessMap[$resourceId] = $this->removeAccessMap[$resourceId] ?? []; + + $key = array_search($userId, $this->removeAccessMap[$resourceId]); + + if ($key === false) { + return; + } + + unset($this->removeAccessMap[$resourceId][$key]); + } + + public function getResourceIdsToRevoke(): array + { + return array_keys($this->removeAccessMap); + } + + public function getUserIdsToRevoke(string $resourceId): array + { + return $this->removeAccessMap[$resourceId] ?? []; + } + + public function grantResourceForUser(string $resourceId, string $permission, string $userId): void + { + $this->giveAccessMap[$resourceId] = $this->giveAccessMap[$resourceId] ?? []; + $this->giveAccessMap[$resourceId][$permission] = $this->giveAccessMap[$resourceId][$permission] ?? []; + $this->giveAccessMap[$resourceId][$permission] = array_unique( + array_merge( + $this->giveAccessMap[$resourceId][$permission], + [$userId] + ) + ); + } + + public function getResourceIdsToGrant(): array + { + return array_keys($this->giveAccessMap); + } + + public function getUserIdsToGrant(string $resourceId, string $permission): array + { + return $this->giveAccessMap[$resourceId][$permission] ?? []; + } +} diff --git a/model/Command/RevokeAccessCommand.php b/model/Command/RevokeAccessCommand.php deleted file mode 100644 index 5eadc8c..0000000 --- a/model/Command/RevokeAccessCommand.php +++ /dev/null @@ -1,71 +0,0 @@ - ['userId1', 'userId2']] - * - * @var string[][] - */ - private array $resourceMap = []; - - public function __construct() { - } - - public function revokeResourceForUser(string $resourceId, string $userId): void - { - $this->resourceMap[$resourceId] = $this->resourceMap[$resourceId] ?? []; - $this->resourceMap[$resourceId] = array_unique(array_merge($this->resourceMap[$resourceId], [$userId])); - } - - public function cancelRevokeResourceForUser(string $resourceId, string $userId): void - { - $this->resourceMap[$resourceId] = $this->resourceMap[$resourceId] ?? []; - - $key = array_search($userId, $this->resourceMap[$resourceId]); - - if ($key === false) { - return; - } - - unset($this->resourceMap[$resourceId][$key]); - } - - public function getResourceIdsToRevoke(): array - { - return array_keys($this->resourceMap); - } - - public function getUserIdsToRevoke(string $resourceId): array - { - return $this->resourceMap[$resourceId] ?? []; - } -} diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index 08a0cf7..a4e5817 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -23,7 +23,7 @@ use common_persistence_SqlPersistence; use oat\oatbox\event\EventManager; use oat\oatbox\service\ConfigurableService; -use oat\taoDacSimple\model\Command\RevokeAccessCommand; +use oat\taoDacSimple\model\Command\ChangeAccessCommand; use oat\taoDacSimple\model\event\DacAddedEvent; use oat\taoDacSimple\model\event\DacRemovedEvent; use oat\generis\persistence\PersistenceManager; @@ -185,67 +185,63 @@ public function changeResourcePermissions(array $resources): void } } - public function addReadAccess(array $addAccessList): bool + public function changeAccess(ChangeAccessCommand $command): bool { - //@TODO Use proper object in the command instead of an array - if (empty($addAccessList)) { - return true; - } + $resourceIds = $command->getResourceIdsToGrant(); - $values = []; + if (!empty($resourceIds)) { + $values = []; - try { - foreach ($addAccessList as $resourceId => $usersIds) { - foreach ($usersIds as $userId) { - $values[] = [ - 'user_id' => $userId, - 'resource_id' => $resourceId, - 'privilege' => PermissionProvider::PERMISSION_READ - ]; + try { + foreach ($resourceIds as $resourceId) { + foreach (PermissionProvider::ALLOWED_PERMISSIONS as $permission) { + $usersIds = $command->getUserIdsToGrant($resourceId, $permission); + + foreach ($usersIds as $userId) { + $values[] = [ + 'user_id' => $userId, + 'resource_id' => $resourceId, + 'privilege' => $permission, + ]; + } + } } - } - $this->insertPermissions($values); + $this->insertPermissions($values); + } catch (Throwable $exception) { + $this->logError('Error when adding permission access: ' . $exception->getMessage()); - return true; - } catch (Throwable $exception) { - $this->logError('Error when adding READ access: ' . $exception->getMessage()); - - return false; + return false; + } } - } - public function revokeAccess(RevokeAccessCommand $revokeAccess): bool - { - $resourceIds = $revokeAccess->getResourceIdsToRevoke(); + $resourceIds = $command->getResourceIdsToRevoke(); - if (empty($resourceIds)) { - return true; - } - - $persistence = $this->getPersistence(); + if (!empty($resourceIds)) { + $persistence = $this->getPersistence(); - try { - $persistence->transactional(static function () use ($resourceIds, $revokeAccess, $persistence): void { - foreach ($resourceIds as $resourceId) { - $usersIds = $revokeAccess->getUserIdsToRevoke($resourceId); - - $persistence->exec( - sprintf( - 'DELETE FROM data_privileges WHERE resource_id = ? AND user_id IN (%s)', - implode(',', array_fill(0, count($usersIds), ' ? ')) - ), - array_merge([$resourceId], array_values($usersIds)) - ); - } - }); + try { + $persistence->transactional(static function () use ($resourceIds, $command, $persistence): void { + foreach ($resourceIds as $resourceId) { + $usersIds = $command->getUserIdsToRevoke($resourceId); - return true; - } catch (Throwable $exception) { - $this->logError('Error when revoking access: ' . $exception->getMessage()); + $persistence->exec( + sprintf( + 'DELETE FROM data_privileges WHERE resource_id = ? AND user_id IN (%s)', + implode(',', array_fill(0, count($usersIds), ' ? ')) + ), + array_merge([$resourceId], array_values($usersIds)) + ); + } + }); + } catch (Throwable $exception) { + $this->logError('Error when revoking access: ' . $exception->getMessage()); - return false; + return false; + } } + + return true; } /** diff --git a/model/PermissionProvider.php b/model/PermissionProvider.php index caab914..eb131c0 100644 --- a/model/PermissionProvider.php +++ b/model/PermissionProvider.php @@ -48,6 +48,11 @@ class PermissionProvider extends ConfigurableService implements PermissionInterf public const PERMISSION_GRANT = 'GRANT'; public const PERMISSION_READ = 'READ'; public const PERMISSION_WRITE = 'WRITE'; + public const ALLOWED_PERMISSIONS = [ + PermissionProvider::PERMISSION_READ, + PermissionProvider::PERMISSION_GRANT, + PermissionProvider::PERMISSION_WRITE, + ]; /** * (non-PHPdoc) From 2eebabc5c2acf6a6d874fb7d52dba1266c8217fe Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Thu, 19 Oct 2023 16:36:43 +0200 Subject: [PATCH 20/41] chore: add possibility to add/revoke access in commands --- model/Command/ChangeAccessCommand.php | 37 ++++++++++++++++++++++++--- model/DataBaseAccess.php | 4 +-- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/model/Command/ChangeAccessCommand.php b/model/Command/ChangeAccessCommand.php index ecf485c..8e4c79d 100644 --- a/model/Command/ChangeAccessCommand.php +++ b/model/Command/ChangeAccessCommand.php @@ -38,16 +38,23 @@ class ChangeAccessCommand */ private array $giveAccessMap = []; + /** + * An array in the form ['resourceId' [ 'READ' => ['userId1', 'userId2']]] + * + * @var string[][][] + */ + private array $revokeAccessMap = []; + public function __construct() { } - public function revokeResourceForUser(string $resourceId, string $userId): void + public function removeResourceForUser(string $resourceId, string $userId): void { $this->removeAccessMap[$resourceId] = $this->removeAccessMap[$resourceId] ?? []; $this->removeAccessMap[$resourceId] = array_unique(array_merge($this->removeAccessMap[$resourceId], [$userId])); } - public function cancelRevokeResourceForUser(string $resourceId, string $userId): void + public function cancelRemoveResourceForUser(string $resourceId, string $userId): void { $this->removeAccessMap[$resourceId] = $this->removeAccessMap[$resourceId] ?? []; @@ -60,12 +67,12 @@ public function cancelRevokeResourceForUser(string $resourceId, string $userId): unset($this->removeAccessMap[$resourceId][$key]); } - public function getResourceIdsToRevoke(): array + public function getResourceIdsToRemove(): array { return array_keys($this->removeAccessMap); } - public function getUserIdsToRevoke(string $resourceId): array + public function getUserIdsToRemove(string $resourceId): array { return $this->removeAccessMap[$resourceId] ?? []; } @@ -91,4 +98,26 @@ public function getUserIdsToGrant(string $resourceId, string $permission): array { return $this->giveAccessMap[$resourceId][$permission] ?? []; } + + public function revokeResourceForUser(string $resourceId, string $permission, string $userId): void + { + $this->revokeAccessMap[$resourceId] = $this->revokeAccessMap[$resourceId] ?? []; + $this->revokeAccessMap[$resourceId][$permission] = $this->revokeAccessMap[$resourceId][$permission] ?? []; + $this->revokeAccessMap[$resourceId][$permission] = array_unique( + array_merge( + $this->revokeAccessMap[$resourceId][$permission], + [$userId] + ) + ); + } + + public function getResourceIdsToRevoke(): array + { + return array_keys($this->revokeAccessMap); + } + + public function getUserIdsToRevoke(string $resourceId, string $permission): array + { + return $this->revokeAccessMap[$resourceId][$permission] ?? []; + } } diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index a4e5817..edf9e7b 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -215,7 +215,7 @@ public function changeAccess(ChangeAccessCommand $command): bool } } - $resourceIds = $command->getResourceIdsToRevoke(); + $resourceIds = $command->getResourceIdsToRemove(); if (!empty($resourceIds)) { $persistence = $this->getPersistence(); @@ -223,7 +223,7 @@ public function changeAccess(ChangeAccessCommand $command): bool try { $persistence->transactional(static function () use ($resourceIds, $command, $persistence): void { foreach ($resourceIds as $resourceId) { - $usersIds = $command->getUserIdsToRevoke($resourceId); + $usersIds = $command->getUserIdsToRemove($resourceId); $persistence->exec( sprintf( From 27e9e9c4a7ec9fbecfa2e934b29532846569cc77 Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Thu, 19 Oct 2023 17:35:42 +0300 Subject: [PATCH 21/41] chore: add new event for event log, wrap calls into transactions --- model/DataBaseAccess.php | 104 ++++++++++++++++---------------- model/event/DacChangedEvent.php | 67 ++++++++++++++++++++ 2 files changed, 119 insertions(+), 52 deletions(-) create mode 100644 model/event/DacChangedEvent.php diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index edf9e7b..3824703 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -25,6 +25,7 @@ use oat\oatbox\service\ConfigurableService; use oat\taoDacSimple\model\Command\ChangeAccessCommand; use oat\taoDacSimple\model\event\DacAddedEvent; +use oat\taoDacSimple\model\event\DacChangedEvent; use oat\taoDacSimple\model\event\DacRemovedEvent; use oat\generis\persistence\PersistenceManager; use PDO; @@ -102,9 +103,11 @@ public function getPermissionsByUsersAndResources(array $userIds, array $resourc public function changeResourcePermissions(array $resources): void { //@TODO Add a proper object as parameter rather than an array with unknown content - $insert = []; + $inserts = []; $groupedRemove = []; - $eventsData = []; + + $addEventsData = []; + $removeEventsData = []; foreach ($resources as $resource) { foreach ($resource['permissions']['add'] as $userId => $addPermissions) { @@ -113,12 +116,18 @@ public function changeResourcePermissions(array $resources): void } foreach ($addPermissions as $permission) { - $insert[] = [ + $inserts[] = [ self::COLUMN_USER_ID => $userId, self::COLUMN_RESOURCE_ID => $resource['id'], self::COLUMN_PRIVILEGE => $permission, ]; } + + $addEventsData[] = [ + 'userId' => $userId, + 'resourceId' => $resource['id'], + 'privileges' => $addPermissions, + ]; } foreach ($resource['permissions']['remove'] as $userId => $removePermissions) { @@ -131,7 +140,7 @@ public function changeResourcePermissions(array $resources): void $groupedRemove[$userId][$idString]['resources'][] = $resource['id']; $groupedRemove[$userId][$idString]['privileges'] = $removePermissions; - $eventsData[] = [ + $removeEventsData[] = [ 'userId' => $userId, 'resourceId' => $resource['id'], 'privileges' => $removePermissions, @@ -139,25 +148,18 @@ public function changeResourcePermissions(array $resources): void } } - $this->insertPermissions($insert); + $persistence = $this->getPersistence(); - foreach ($insert as $inserted) { - //@TODO @FIXME Do a single event to add multiple event logs entries - $this->getEventManager()->trigger( - new DacAddedEvent( - $inserted[self::COLUMN_USER_ID], - $inserted[self::COLUMN_RESOURCE_ID], - (array) $inserted[self::COLUMN_PRIVILEGE] - ) - ); - } + try { + $persistence->transactional(function () use ($inserts, $groupedRemove, $persistence) { + $this->insertPermissions($inserts, $persistence); - foreach ($groupedRemove as $userId => $priviligeGroups) { - foreach ($priviligeGroups as $privilegeGroup) { - $inQueryPrivilege = implode(',', array_fill(0, count($privilegeGroup['privileges']), ' ? ')); - $inQueryResources = implode(',', array_fill(0, count($privilegeGroup['resources']), ' ? ')); + foreach ($groupedRemove as $userId => $privilegeGroups) { + foreach ($privilegeGroups as $privilegeGroup) { + $inQueryPrivilege = implode(',', array_fill(0, count($privilegeGroup['privileges']), ' ? ')); + $inQueryResources = implode(',', array_fill(0, count($privilegeGroup['resources']), ' ? ')); - $query = <<getPersistence()->exec($query, $params); - } - } + $persistence->exec($query, $params); + } + } + }); - //@TODO @FIXME Do a single event to add multiple event logs entries - foreach ($eventsData as $eventData) { - $this->getEventManager()->trigger(new DacRemovedEvent( - $eventData['userId'], - $eventData['resourceId'], - $eventData['privileges'] - )); + if (!empty($addEventsData) || !empty($removeEventsData)) { + $this->getEventManager()->trigger(new DacChangedEvent($addEventsData, $removeEventsData)); + } + } catch (Throwable $exception) { + $this->logError('Error while changing resource permissions: ' . $exception->getMessage()); } } @@ -207,7 +208,8 @@ public function changeAccess(ChangeAccessCommand $command): bool } } - $this->insertPermissions($values); + $persistence = $this->getPersistence(); + $persistence->transactional(fn () => $this->insertPermissions($values, $persistence)); } catch (Throwable $exception) { $this->logError('Error when adding permission access: ' . $exception->getMessage()); @@ -377,7 +379,8 @@ public function addMultiplePermissions(array $permissionData) } } - $this->insertPermissions($insert); + $persistence = $this->getPersistence(); + $persistence->transactional(fn () => $this->insertPermissions($insert, $persistence)); foreach ($insert as $inserted) { $this->getEventManager()->trigger(new DacAddedEvent( @@ -624,7 +627,7 @@ public function removeTables() /** * @throws Throwable */ - private function insertPermissions(array $insert): void + private function insertPermissions(array $insert, common_persistence_SqlPersistence $persistence): void { if (empty($insert)) { return; @@ -632,21 +635,18 @@ private function insertPermissions(array $insert): void $logger = $this->getLogger(); $insertCount = count($insert); - $persistence = $this->getPersistence(); - $persistence->transactional(function () use ($insert, $logger, $insertCount, $persistence) { - foreach (array_chunk($insert, $this->insertChunkSize) as $index => $batch) { - $logger->debug( - 'Processing chunk {index}/{total} with {items} ACL entries', - [ - 'index' => $index + 1, - 'total' => ceil($insertCount / $this->insertChunkSize), - 'items' => count($batch) - ] - ); + foreach (array_chunk($insert, $this->insertChunkSize) as $index => $batch) { + $logger->debug( + sprintf( + 'Processing chunk %d/%d with %d ACL entries', + $index + 1, + ceil($insertCount / $this->insertChunkSize), + count($batch), + ) + ); - $persistence->insertMultiple(self::TABLE_PRIVILEGES_NAME, $batch); - } - }); + $persistence->insertMultiple(self::TABLE_PRIVILEGES_NAME, $batch); + } } } diff --git a/model/event/DacChangedEvent.php b/model/event/DacChangedEvent.php new file mode 100644 index 0000000..146ac53 --- /dev/null +++ b/model/event/DacChangedEvent.php @@ -0,0 +1,67 @@ +added = $added; + $this->removed = $removed; + } + + public function getValues(): array + { + return array_merge( + $this->enrichWithActions($this->added, 'add'), + $this->enrichWithActions($this->removed, 'remove'), + ); + } + + public function getName(): string + { + return __CLASS__; + } + + public function jsonSerialize(): array + { + return [ + 'added' => $this->added, + 'removed' => $this->removed, + ]; + } + + private function enrichWithActions(array $values, string $action): array + { + return array_map( + static fn (array $record): array => array_merge($record, ['action' => $action]), + $values + ); + } +} From 9484c3c5bb8a2a99a1da8d1f47688ee0af8e7b64 Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Thu, 19 Oct 2023 17:35:18 +0200 Subject: [PATCH 22/41] chore: use the new method for change access --- model/ChangePermissionsService.php | 52 +++++++++++++++++------------- model/DataBaseAccess.php | 48 ++++++++++++++++++++++++--- 2 files changed, 73 insertions(+), 27 deletions(-) diff --git a/model/ChangePermissionsService.php b/model/ChangePermissionsService.php index ee806f5..62b044a 100644 --- a/model/ChangePermissionsService.php +++ b/model/ChangePermissionsService.php @@ -25,6 +25,7 @@ use core_kernel_classes_Resource; use oat\oatbox\event\EventManager; use oat\tao\model\event\DataAccessControlChangedEvent; +use oat\taoDacSimple\model\Command\ChangeAccessCommand; use oat\taoDacSimple\model\Command\ChangePermissionsCommand; use oat\taoDacSimple\model\event\DacAffectedUsersEvent; use oat\taoDacSimple\model\event\DacRootChangedEvent; @@ -60,10 +61,11 @@ public function change(ChangePermissionsCommand $command): void return; } - $resources = $this->enrichWithAddRemoveActions($resources, $permissionsDelta); + $changeAccessCommand = new ChangeAccessCommand(); + $resources = $this->enrichWithAddRemoveActions($resources, $permissionsDelta, $changeAccessCommand); $this->dryRun($resources); - $this->dataBaseAccess->changeResourcePermissions($resources); + $this->dataBaseAccess->changeAccess($changeAccessCommand); $this->triggerEvents($command->getRoot(), $permissionsDelta, $command->isRecursive()); } @@ -102,32 +104,36 @@ private function enrichWithPermissions(array &$resources): void } } - private function enrichWithAddRemoveActions(array $resources, array $permissionsDelta): array - { + private function enrichWithAddRemoveActions( + array $resources, + array $permissionsDelta, + ChangeAccessCommand $command + ): array { foreach ($resources as &$resource) { - $resource['permissions']['remove'] = $this->strategy->getPermissionsToRemove( - $resource['permissions']['current'], - $permissionsDelta - ); - - $resource['permissions']['add'] = $this->strategy->getPermissionsToAdd( - $resource['permissions']['current'], - $permissionsDelta + $resource['permissions']['remove'] = array_unique( + $this->strategy->getPermissionsToRemove( + $resource['permissions']['current'], + $permissionsDelta + ) ); - } - return $this->deduplicateChanges($resources); - } - - private function deduplicateChanges(array $resources): array - { - foreach ($resources as &$resource) { - foreach ($resource['permissions']['remove'] as &$removePermissions) { - $removePermissions = array_unique($removePermissions); + foreach ($resource['permissions']['remove'] as $userId => $permissions) { + foreach ($permissions as $permission) { + $command->revokeResourceForUser($resource['id'], $permission, $userId); + } } - foreach ($resource['permissions']['add'] as &$addPermissions) { - $addPermissions = array_unique($addPermissions); + $resource['permissions']['add'] = array_unique( + $this->strategy->getPermissionsToAdd( + $resource['permissions']['current'], + $permissionsDelta + ) + ); + + foreach ($resource['permissions']['add'] as $userId => $permissions) { + foreach ($permissions as $permission) { + $command->grantResourceForUser($resource['id'], $permission, $userId); + } } } diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index 3824703..6f0f8fa 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -102,6 +102,7 @@ public function getPermissionsByUsersAndResources(array $userIds, array $resourc public function changeResourcePermissions(array $resources): void { + // @TODO Remove this method after it is not needed anymore //@TODO Add a proper object as parameter rather than an array with unknown content $inserts = []; $groupedRemove = []; @@ -217,6 +218,41 @@ public function changeAccess(ChangeAccessCommand $command): bool } } + //@TODO Group users per privilege per resource to reduce the amount of DELETE queries - Check this::changeResourcePermissions + //@TODO Call proper events + $resourceIds = $command->getResourceIdsToRevoke(); + + if (!empty($resourceIds)) { + $persistence = $this->getPersistence(); + + try { + $persistence->transactional(static function () use ($resourceIds, $command, $persistence): void { + foreach ($resourceIds as $resourceId) { + foreach (PermissionProvider::ALLOWED_PERMISSIONS as $permission) { + $userIds = $command->getUserIdsToRevoke($resourceId, $permission); + + if (count($userIds) === 0) { + continue; + } + + $persistence->exec( + sprintf( + 'DELETE FROM data_privileges WHERE resource_id = ? AND privilege = ? AND user_id IN (%s)', + implode(',', array_fill(0, count($userIds), ' ? ')) + ), + array_merge([$resourceId, $permission], array_values($userIds)) + ); + } + } + }); + } catch (Throwable $exception) { + $this->logError('Error when revoking access: ' . $exception->getMessage()); + + return false; + } + } + + //@TODO @FIXME Stop using this and use Revoke instead $resourceIds = $command->getResourceIdsToRemove(); if (!empty($resourceIds)) { @@ -225,19 +261,23 @@ public function changeAccess(ChangeAccessCommand $command): bool try { $persistence->transactional(static function () use ($resourceIds, $command, $persistence): void { foreach ($resourceIds as $resourceId) { - $usersIds = $command->getUserIdsToRemove($resourceId); + $userIds = $command->getUserIdsToRemove($resourceId); + + if (empty($userIds)) { + continue; + } $persistence->exec( sprintf( 'DELETE FROM data_privileges WHERE resource_id = ? AND user_id IN (%s)', - implode(',', array_fill(0, count($usersIds), ' ? ')) + implode(',', array_fill(0, count($userIds), ' ? ')) ), - array_merge([$resourceId], array_values($usersIds)) + array_merge([$resourceId], array_values($userIds)) ); } }); } catch (Throwable $exception) { - $this->logError('Error when revoking access: ' . $exception->getMessage()); + $this->logError('Error when removing access: ' . $exception->getMessage()); return false; } From 5d823097272eb1b6da078dd90cefea2d13e039f9 Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Thu, 19 Oct 2023 17:51:58 +0200 Subject: [PATCH 23/41] chore: use remove instead of remove --- model/Command/ChangeAccessCommand.php | 49 +++++++-------------------- model/DataBaseAccess.php | 31 ----------------- 2 files changed, 13 insertions(+), 67 deletions(-) diff --git a/model/Command/ChangeAccessCommand.php b/model/Command/ChangeAccessCommand.php index 8e4c79d..03d17a5 100644 --- a/model/Command/ChangeAccessCommand.php +++ b/model/Command/ChangeAccessCommand.php @@ -24,13 +24,6 @@ class ChangeAccessCommand { - /** - * An array in the form ['resourceId' => ['userId1', 'userId2']] - * - * @var string[][] - */ - private array $removeAccessMap = []; - /** * An array in the form ['resourceId' [ 'READ' => ['userId1', 'userId2']]] * @@ -48,35 +41,6 @@ class ChangeAccessCommand public function __construct() { } - public function removeResourceForUser(string $resourceId, string $userId): void - { - $this->removeAccessMap[$resourceId] = $this->removeAccessMap[$resourceId] ?? []; - $this->removeAccessMap[$resourceId] = array_unique(array_merge($this->removeAccessMap[$resourceId], [$userId])); - } - - public function cancelRemoveResourceForUser(string $resourceId, string $userId): void - { - $this->removeAccessMap[$resourceId] = $this->removeAccessMap[$resourceId] ?? []; - - $key = array_search($userId, $this->removeAccessMap[$resourceId]); - - if ($key === false) { - return; - } - - unset($this->removeAccessMap[$resourceId][$key]); - } - - public function getResourceIdsToRemove(): array - { - return array_keys($this->removeAccessMap); - } - - public function getUserIdsToRemove(string $resourceId): array - { - return $this->removeAccessMap[$resourceId] ?? []; - } - public function grantResourceForUser(string $resourceId, string $permission, string $userId): void { $this->giveAccessMap[$resourceId] = $this->giveAccessMap[$resourceId] ?? []; @@ -111,6 +75,19 @@ public function revokeResourceForUser(string $resourceId, string $permission, st ); } + public function removeRevokeResourceForUser(string $resourceId, string $permission, string $userId): void + { + $this->revokeAccessMap[$resourceId][$permission] = $this->revokeAccessMap[$resourceId][$permission] ?? []; + + $key = array_search($userId, $this->revokeAccessMap[$resourceId][$permission]); + + if ($key === false) { + return; + } + + unset($this->revokeAccessMap[$resourceId][$permission][$key]); + } + public function getResourceIdsToRevoke(): array { return array_keys($this->revokeAccessMap); diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index 6f0f8fa..2efa74a 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -252,37 +252,6 @@ public function changeAccess(ChangeAccessCommand $command): bool } } - //@TODO @FIXME Stop using this and use Revoke instead - $resourceIds = $command->getResourceIdsToRemove(); - - if (!empty($resourceIds)) { - $persistence = $this->getPersistence(); - - try { - $persistence->transactional(static function () use ($resourceIds, $command, $persistence): void { - foreach ($resourceIds as $resourceId) { - $userIds = $command->getUserIdsToRemove($resourceId); - - if (empty($userIds)) { - continue; - } - - $persistence->exec( - sprintf( - 'DELETE FROM data_privileges WHERE resource_id = ? AND user_id IN (%s)', - implode(',', array_fill(0, count($userIds), ' ? ')) - ), - array_merge([$resourceId], array_values($userIds)) - ); - } - }); - } catch (Throwable $exception) { - $this->logError('Error when removing access: ' . $exception->getMessage()); - - return false; - } - } - return true; } From 6ba4df9354c8ac1d9e68e15757be6d8ad7e78f33 Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Thu, 19 Oct 2023 19:20:40 +0200 Subject: [PATCH 24/41] chore: group information to better delete query + single transaction for all operations --- model/Command/ChangeAccessCommand.php | 34 +++++ model/DataBaseAccess.php | 176 +++++--------------------- 2 files changed, 69 insertions(+), 141 deletions(-) diff --git a/model/Command/ChangeAccessCommand.php b/model/Command/ChangeAccessCommand.php index 03d17a5..01038f4 100644 --- a/model/Command/ChangeAccessCommand.php +++ b/model/Command/ChangeAccessCommand.php @@ -24,6 +24,13 @@ class ChangeAccessCommand { + /** + * An array in the form ['userId' [ 'READ' => ['resource1', 'resource2']]] + * + * @var string[][][] + */ + private array $userRevokedPermissions = []; + /** * An array in the form ['resourceId' [ 'READ' => ['userId1', 'userId2']]] * @@ -73,10 +80,22 @@ public function revokeResourceForUser(string $resourceId, string $permission, st [$userId] ) ); + + $this->userRevokedPermissions[$userId] = $this->userRevokedPermissions[$userId] ?? []; + $this->userRevokedPermissions[$userId][$permission] = $this->userRevokedPermissions[$userId][$permission] ?? []; + $this->userRevokedPermissions[$userId][$permission][] = $resourceId; } public function removeRevokeResourceForUser(string $resourceId, string $permission, string $userId): void { + $this->userRevokedPermissions[$userId][$permission] = $this->userRevokedPermissions[$userId][$permission] ?? []; + + $key = array_search($resourceId, $this->userRevokedPermissions[$userId][$permission]); + + if ($key !== false) { + unset($this->userRevokedPermissions[$userId][$permission][$key]); + } + $this->revokeAccessMap[$resourceId][$permission] = $this->revokeAccessMap[$resourceId][$permission] ?? []; $key = array_search($userId, $this->revokeAccessMap[$resourceId][$permission]); @@ -97,4 +116,19 @@ public function getUserIdsToRevoke(string $resourceId, string $permission): arra { return $this->revokeAccessMap[$resourceId][$permission] ?? []; } + + public function getUserIdsToRevokedPermissions(): array + { + return array_keys($this->userRevokedPermissions); + } + + public function getUserPermissionsToRevoke(string $userId): array + { + return array_keys($this->userRevokedPermissions[$userId]); + } + + public function getResourceIdsByUserAndPermissionToRevoke(string $userId, string $permission): array + { + return $this->userRevokedPermissions[$userId][$permission] ?? []; + } } diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index 2efa74a..c72ced9 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -70,6 +70,9 @@ protected function getEventManager() public function getPermissionsByUsersAndResources(array $userIds, array $resourceIds): array { + /** + * @TODO FIXME Lets see if we can make this methods return better contract or if it is still necessary + */ // Permissions for an empty set of resources must be an empty array if (empty($resourceIds)) { return []; @@ -100,159 +103,50 @@ public function getPermissionsByUsersAndResources(array $userIds, array $resourc return $data; } - public function changeResourcePermissions(array $resources): void + public function changeAccess(ChangeAccessCommand $command): void { - // @TODO Remove this method after it is not needed anymore - //@TODO Add a proper object as parameter rather than an array with unknown content - $inserts = []; - $groupedRemove = []; - - $addEventsData = []; - $removeEventsData = []; - - foreach ($resources as $resource) { - foreach ($resource['permissions']['add'] as $userId => $addPermissions) { - if (empty($addPermissions)) { - continue; - } - - foreach ($addPermissions as $permission) { - $inserts[] = [ - self::COLUMN_USER_ID => $userId, - self::COLUMN_RESOURCE_ID => $resource['id'], - self::COLUMN_PRIVILEGE => $permission, - ]; - } - - $addEventsData[] = [ - 'userId' => $userId, - 'resourceId' => $resource['id'], - 'privileges' => $addPermissions, - ]; - } - - foreach ($resource['permissions']['remove'] as $userId => $removePermissions) { - if (empty($removePermissions)) { - continue; - } - - $idString = implode($removePermissions); - - $groupedRemove[$userId][$idString]['resources'][] = $resource['id']; - $groupedRemove[$userId][$idString]['privileges'] = $removePermissions; - - $removeEventsData[] = [ - 'userId' => $userId, - 'resourceId' => $resource['id'], - 'privileges' => $removePermissions, - ]; - } - } - $persistence = $this->getPersistence(); - - try { - $persistence->transactional(function () use ($inserts, $groupedRemove, $persistence) { - $this->insertPermissions($inserts, $persistence); - - foreach ($groupedRemove as $userId => $privilegeGroups) { - foreach ($privilegeGroups as $privilegeGroup) { - $inQueryPrivilege = implode(',', array_fill(0, count($privilegeGroup['privileges']), ' ? ')); - $inQueryResources = implode(',', array_fill(0, count($privilegeGroup['resources']), ' ? ')); - - $query = <<transactional(static function () use ($command, $persistence, $class): void { + foreach ($command->getUserIdsToRevokedPermissions() as $userId) { + foreach ($command->getUserPermissionsToRevoke($userId) as $permission) { + $resourceIds = $command->getResourceIdsByUserAndPermissionToRevoke($userId, $permission); + + if (count($resourceIds) > 0) { + $persistence->exec( + sprintf( + 'DELETE FROM data_privileges WHERE user_id = ? AND privilege = ? AND resource_id IN (%s)', + implode(',', array_fill(0, count($resourceIds), ' ? ')), + ), + array_merge([$userId, $permission], $resourceIds) ); - - $persistence->exec($query, $params); } } - }); - - if (!empty($addEventsData) || !empty($removeEventsData)) { - $this->getEventManager()->trigger(new DacChangedEvent($addEventsData, $removeEventsData)); } - } catch (Throwable $exception) { - $this->logError('Error while changing resource permissions: ' . $exception->getMessage()); - } - } - - public function changeAccess(ChangeAccessCommand $command): bool - { - $resourceIds = $command->getResourceIdsToGrant(); - - if (!empty($resourceIds)) { - $values = []; - - try { - foreach ($resourceIds as $resourceId) { - foreach (PermissionProvider::ALLOWED_PERMISSIONS as $permission) { - $usersIds = $command->getUserIdsToGrant($resourceId, $permission); - - foreach ($usersIds as $userId) { - $values[] = [ - 'user_id' => $userId, - 'resource_id' => $resourceId, - 'privilege' => $permission, - ]; - } - } - } - $persistence = $this->getPersistence(); - $persistence->transactional(fn () => $this->insertPermissions($values, $persistence)); - } catch (Throwable $exception) { - $this->logError('Error when adding permission access: ' . $exception->getMessage()); + $insert = []; - return false; - } - } + foreach ($command->getResourceIdsToGrant() as $resourceId) { + foreach (PermissionProvider::ALLOWED_PERMISSIONS as $permission) { + $usersIds = $command->getUserIdsToGrant($resourceId, $permission); - //@TODO Group users per privilege per resource to reduce the amount of DELETE queries - Check this::changeResourcePermissions - //@TODO Call proper events - $resourceIds = $command->getResourceIdsToRevoke(); - - if (!empty($resourceIds)) { - $persistence = $this->getPersistence(); - - try { - $persistence->transactional(static function () use ($resourceIds, $command, $persistence): void { - foreach ($resourceIds as $resourceId) { - foreach (PermissionProvider::ALLOWED_PERMISSIONS as $permission) { - $userIds = $command->getUserIdsToRevoke($resourceId, $permission); - - if (count($userIds) === 0) { - continue; - } - - $persistence->exec( - sprintf( - 'DELETE FROM data_privileges WHERE resource_id = ? AND privilege = ? AND user_id IN (%s)', - implode(',', array_fill(0, count($userIds), ' ? ')) - ), - array_merge([$resourceId, $permission], array_values($userIds)) - ); - } + foreach ($usersIds as $userId) { + $insert[] = [ + 'user_id' => $userId, + 'resource_id' => $resourceId, + 'privilege' => $permission, + ]; } - }); - } catch (Throwable $exception) { - $this->logError('Error when revoking access: ' . $exception->getMessage()); - - return false; + } } - } - return true; + $class->insertPermissions($insert, $persistence); + + /** + * @TODO FIXME Call proper events here + */ + }); } /** From c9a4e02ab55efe32ff5a302ab4c93230490df642 Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Thu, 19 Oct 2023 19:24:23 +0200 Subject: [PATCH 25/41] chore: typo --- model/Command/ChangeAccessCommand.php | 2 +- model/DataBaseAccess.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/model/Command/ChangeAccessCommand.php b/model/Command/ChangeAccessCommand.php index 01038f4..e10c417 100644 --- a/model/Command/ChangeAccessCommand.php +++ b/model/Command/ChangeAccessCommand.php @@ -117,7 +117,7 @@ public function getUserIdsToRevoke(string $resourceId, string $permission): arra return $this->revokeAccessMap[$resourceId][$permission] ?? []; } - public function getUserIdsToRevokedPermissions(): array + public function getUserIdsToRevokePermissions(): array { return array_keys($this->userRevokedPermissions); } diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index c72ced9..81ec830 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -109,7 +109,7 @@ public function changeAccess(ChangeAccessCommand $command): void $class = $this; $persistence->transactional(static function () use ($command, $persistence, $class): void { - foreach ($command->getUserIdsToRevokedPermissions() as $userId) { + foreach ($command->getUserIdsToRevokePermissions() as $userId) { foreach ($command->getUserPermissionsToRevoke($userId) as $permission) { $resourceIds = $command->getResourceIdsByUserAndPermissionToRevoke($userId, $permission); From 56b081d60774453010d27ce38fe0b3393ce69a99 Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Fri, 20 Oct 2023 08:47:17 +0200 Subject: [PATCH 26/41] chore: add unit test for ChangeAccessCommand --- .../model/Command/ChangeAccessCommandTest.php | 114 ++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 test/unit/model/Command/ChangeAccessCommandTest.php diff --git a/test/unit/model/Command/ChangeAccessCommandTest.php b/test/unit/model/Command/ChangeAccessCommandTest.php new file mode 100644 index 0000000..29f8c66 --- /dev/null +++ b/test/unit/model/Command/ChangeAccessCommandTest.php @@ -0,0 +1,114 @@ +sut = new ChangeAccessCommand(); + } + + public function testGrantPermissions(): void + { + $this->sut->grantResourceForUser('r1', PermissionProvider::PERMISSION_READ, 'u1'); + $this->sut->grantResourceForUser('r1', PermissionProvider::PERMISSION_WRITE, 'u1'); + $this->sut->grantResourceForUser('r1', PermissionProvider::PERMISSION_GRANT, 'u1'); + + $this->sut->grantResourceForUser('r2', PermissionProvider::PERMISSION_READ, 'u1'); + $this->sut->grantResourceForUser('r2', PermissionProvider::PERMISSION_WRITE, 'u1'); + $this->sut->grantResourceForUser('r2', PermissionProvider::PERMISSION_GRANT, 'u1'); + + $this->sut->grantResourceForUser('r2', PermissionProvider::PERMISSION_READ, 'u2'); + $this->sut->grantResourceForUser('r2', PermissionProvider::PERMISSION_WRITE, 'u2'); + $this->sut->grantResourceForUser('r2', PermissionProvider::PERMISSION_GRANT, 'u2'); + + $this->assertSame(['r1', 'r2'], $this->sut->getResourceIdsToGrant()); + + $this->assertSame(['u1'], $this->sut->getUserIdsToGrant('r1', PermissionProvider::PERMISSION_READ)); + $this->assertSame(['u1'], $this->sut->getUserIdsToGrant('r1', PermissionProvider::PERMISSION_WRITE)); + $this->assertSame(['u1'], $this->sut->getUserIdsToGrant('r1', PermissionProvider::PERMISSION_GRANT)); + + $this->assertSame(['u1', 'u2'], $this->sut->getUserIdsToGrant('r2', PermissionProvider::PERMISSION_READ)); + $this->assertSame(['u1', 'u2'], $this->sut->getUserIdsToGrant('r2', PermissionProvider::PERMISSION_WRITE)); + $this->assertSame(['u1', 'u2'], $this->sut->getUserIdsToGrant('r2', PermissionProvider::PERMISSION_GRANT)); + } + + public function testRevokePermissions(): void + { + $this->sut->revokeResourceForUser('r1', PermissionProvider::PERMISSION_READ, 'u1'); + $this->sut->revokeResourceForUser('r1', PermissionProvider::PERMISSION_WRITE, 'u1'); + $this->sut->revokeResourceForUser('r1', PermissionProvider::PERMISSION_GRANT, 'u1'); + + $this->sut->revokeResourceForUser('r2', PermissionProvider::PERMISSION_READ, 'u1'); + $this->sut->revokeResourceForUser('r2', PermissionProvider::PERMISSION_WRITE, 'u1'); + $this->sut->revokeResourceForUser('r2', PermissionProvider::PERMISSION_GRANT, 'u1'); + + $this->sut->revokeResourceForUser('r2', PermissionProvider::PERMISSION_READ, 'u2'); + $this->sut->revokeResourceForUser('r2', PermissionProvider::PERMISSION_WRITE, 'u2'); + $this->sut->revokeResourceForUser('r2', PermissionProvider::PERMISSION_GRANT, 'u2'); + + $this->sut->removeRevokeResourceForUser('r2', PermissionProvider::PERMISSION_GRANT, 'u2'); + + $this->assertSame(['r1', 'r2'], $this->sut->getResourceIdsToRevoke()); + $this->assertSame(['u1', 'u2'], $this->sut->getUserIdsToRevokePermissions()); + + $this->assertSame(['r1', 'r2'], $this->sut->getResourceIdsByUserAndPermissionToRevoke('u1', PermissionProvider::PERMISSION_READ)); + $this->assertSame(['r1', 'r2'], $this->sut->getResourceIdsByUserAndPermissionToRevoke('u1', PermissionProvider::PERMISSION_WRITE)); + $this->assertSame(['r1', 'r2'], $this->sut->getResourceIdsByUserAndPermissionToRevoke('u1', PermissionProvider::PERMISSION_GRANT)); + + $this->assertSame(['r2'], $this->sut->getResourceIdsByUserAndPermissionToRevoke('u2', PermissionProvider::PERMISSION_READ)); + $this->assertSame(['r2'], $this->sut->getResourceIdsByUserAndPermissionToRevoke('u2', PermissionProvider::PERMISSION_WRITE)); + $this->assertSame([], $this->sut->getResourceIdsByUserAndPermissionToRevoke('u2', PermissionProvider::PERMISSION_GRANT)); + + $this->assertSame( + [ + PermissionProvider::PERMISSION_READ, + PermissionProvider::PERMISSION_WRITE, + PermissionProvider::PERMISSION_GRANT + ], + $this->sut->getUserPermissionsToRevoke('u1') + ); + $this->assertSame( + [ + PermissionProvider::PERMISSION_READ, + PermissionProvider::PERMISSION_WRITE, + PermissionProvider::PERMISSION_GRANT + ], + $this->sut->getUserPermissionsToRevoke('u2') + ); + + $this->assertSame(['u1'], $this->sut->getUserIdsToRevoke('r1', PermissionProvider::PERMISSION_READ)); + $this->assertSame(['u1'], $this->sut->getUserIdsToRevoke('r1', PermissionProvider::PERMISSION_WRITE)); + $this->assertSame(['u1'], $this->sut->getUserIdsToRevoke('r1', PermissionProvider::PERMISSION_GRANT)); + $this->assertSame(['u1', 'u2'], $this->sut->getUserIdsToRevoke('r2', PermissionProvider::PERMISSION_READ)); + $this->assertSame(['u1', 'u2'], $this->sut->getUserIdsToRevoke('r2', PermissionProvider::PERMISSION_WRITE)); + $this->assertSame(['u1'], $this->sut->getUserIdsToRevoke('r2', PermissionProvider::PERMISSION_GRANT)); + } +} From 826300124fe8fd25490760471613ad8245568a29 Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Fri, 20 Oct 2023 09:59:37 +0200 Subject: [PATCH 27/41] chore: add TODO --- model/PermissionsService.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/model/PermissionsService.php b/model/PermissionsService.php index 8c28388..ef782d6 100644 --- a/model/PermissionsService.php +++ b/model/PermissionsService.php @@ -60,6 +60,7 @@ public function __construct( */ public function applyPermissions(ChangePermissionsCommand $command): void { + //@TODO FIXME Call ChangePermissionService::change($command); $resources = $this->getResourcesToUpdate($command); $currentPermissions = $this->getResourcesPermissions($resources); @@ -131,6 +132,7 @@ public function savePermissions( */ public function getPermissionsDelta(core_kernel_classes_Resource $resource, array $permissionsToSet): array { + //@TODO FIXME Remove this method as it is used only here and we will change to call ChangePermissionsService::chage $currentPermissions = $this->dataBaseAccess->getResourcePermissions($resource->getUri()); return $this->strategy->normalizeRequest($currentPermissions, $permissionsToSet); From 69a1755f4b085444c6c0e4cff5eab1785b028db6 Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Fri, 20 Oct 2023 11:35:16 +0200 Subject: [PATCH 28/41] refactor!: Removed old methods and PermissionsService which had already too much deprecations --- manifest.php | 6 +- model/ChangePermissionsService.php | 36 ++ model/Command/ChangePermissionsCommand.php | 55 +-- model/PermissionsService.php | 363 ------------------ model/PermissionsServiceFactory.php | 19 +- .../PermissionsServiceProvider.php | 49 +++ model/eventHandler/ResourceUpdateHandler.php | 19 +- model/tasks/ChangePermissionsTask.php | 10 +- .../Command/ChangePermissionsCommandTest.php | 27 +- test/unit/model/PermissionsServiceTest.php | 341 ---------------- .../ResourceUpdateHandlerTest.php | 17 +- 11 files changed, 111 insertions(+), 831 deletions(-) delete mode 100644 model/PermissionsService.php create mode 100644 model/ServiceProvider/PermissionsServiceProvider.php delete mode 100644 test/unit/model/PermissionsServiceTest.php diff --git a/manifest.php b/manifest.php index e60e143..1a76723 100644 --- a/manifest.php +++ b/manifest.php @@ -15,10 +15,11 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * - * Copyright (c) 2014-2022 (original work) Open Assessment Technologies SA; + * Copyright (c) 2014-2023 (original work) Open Assessment Technologies SA; */ use oat\taoDacSimple\model\Copy\ServiceProvider\CopyServiceProvider; +use oat\taoDacSimple\model\ServiceProvider\PermissionsServiceProvider; use oat\taoDacSimple\scripts\install\AttachEventHandler; use oat\taoDacSimple\scripts\update\Updater; use oat\taoDacSimple\scripts\install\SetupDataAccess; @@ -65,6 +66,7 @@ 'BASE_URL' => ROOT_URL . 'taoDacSimple/', ], 'containerServiceProviders' => [ - CopyServiceProvider::class + CopyServiceProvider::class, + PermissionsServiceProvider::class ] ]; diff --git a/model/ChangePermissionsService.php b/model/ChangePermissionsService.php index 62b044a..9a92951 100644 --- a/model/ChangePermissionsService.php +++ b/model/ChangePermissionsService.php @@ -216,4 +216,40 @@ private function triggerEvents( ) ); } + +//@TODO FIXME The events bellow were sent before and we need to make sure they are either not necessary anymore or still sent +// private function triggerEvents( +// string $resourceId, +// string $rootResourceId, +// array $addRemove, +// bool $isRecursive, +// bool $applyToNestedResources +// ): void { +// if (!empty($addRemove['add'])) { +// foreach ($addRemove['add'] as $userId => $rights) { +// $this->eventManager->trigger(new DacRootAddedEvent($userId, $resourceId, (array)$rights)); +// } +// } +// if (!empty($addRemove['remove'])) { +// foreach ($addRemove['remove'] as $userId => $rights) { +// $this->eventManager->trigger(new DacRootRemovedEvent($userId, $resourceId, (array)$rights)); +// } +// } +// $this->eventManager->trigger( +// new DacAffectedUsersEvent( +// array_keys($addRemove['add'] ?? []), +// array_keys($addRemove['remove'] ?? []) +// ) +// ); +// +// $this->eventManager->trigger( +// new DataAccessControlChangedEvent( +// $resourceId, +// $addRemove, +// $isRecursive, +// $applyToNestedResources, +// $rootResourceId +// ) +// ); +// } } diff --git a/model/Command/ChangePermissionsCommand.php b/model/Command/ChangePermissionsCommand.php index e96d5ed..95952ff 100644 --- a/model/Command/ChangePermissionsCommand.php +++ b/model/Command/ChangePermissionsCommand.php @@ -37,31 +37,23 @@ class ChangePermissionsCommand { private core_kernel_classes_Resource $root; - private string $masterRequest; - /** - * An array in the form ['userId' => ['READ', 'WRITE], ...] + * An array in the form ['userId' => ['READ', 'WRITE'], ...] * * @var string[] */ private array $privilegesPerUser; - private bool $isRecursive = false; - - private bool $applyToNestedResources = false; - - private array $permissionsDelta; + private bool $isRecursive; - public function __construct( - core_kernel_classes_Resource $root, - string $masterRequest, - array $privileges, - array $permissionsDelta - ) { + /** + * @param array $privilegesPerUser An array in the form ['userId' => ['READ', 'WRITE'], ...] + */ + public function __construct(core_kernel_classes_Resource $root, array $privilegesPerUser, bool $isRecursive = false) + { $this->root = $root; - $this->masterRequest = $masterRequest; - $this->privilegesPerUser = $privileges; - $this->permissionsDelta = $permissionsDelta; + $this->privilegesPerUser = $privilegesPerUser; + $this->isRecursive = $isRecursive; } /** @@ -80,47 +72,18 @@ public function withRecursion(): void $this->isRecursive = true; } - /** - * Sets the applyToNestedResources flag for the command. - * - * For commands having a class as the root AND not having the recursion - * flag set, setting the nested resources flag makes PermissionsService to - * update permissions for the class and all instances of that class, but - * skips all nested classes and instances of them (i.e. does not go down - * into nested levels of the resource tree). - */ - public function withNestedResources(): void - { - $this->applyToNestedResources = true; - } - public function getRoot(): core_kernel_classes_Resource { return $this->root; } - public function getMasterRequest(): string - { - return $this->masterRequest; - } - public function getPrivilegesPerUser(): array { return $this->privilegesPerUser; } - public function getPermissionsDelta(): array - { - return $this->permissionsDelta; - } - public function isRecursive(): bool { return $this->isRecursive; } - - public function applyToNestedResources(): bool - { - return $this->applyToNestedResources; - } } diff --git a/model/PermissionsService.php b/model/PermissionsService.php deleted file mode 100644 index ef782d6..0000000 --- a/model/PermissionsService.php +++ /dev/null @@ -1,363 +0,0 @@ -dataBaseAccess = $dataBaseAccess; - $this->strategy = $strategy; - $this->eventManager = $eventManager; - } - - /** - * @deprecated Use oat\taoDacSimple\model\ChangePermissionsService::change - */ - public function applyPermissions(ChangePermissionsCommand $command): void - { - //@TODO FIXME Call ChangePermissionService::change($command); - $resources = $this->getResourcesToUpdate($command); - $currentPermissions = $this->getResourcesPermissions($resources); - - if (empty($command->getPermissionsDelta())) { - return; - } - - $actions = $this->getActions($resources, $currentPermissions, $command->getPermissionsDelta()); - $this->dryRun($actions, $currentPermissions); - $this->wetRun($actions); - - $this->triggerEvents( - $command->getRoot()->getUri(), - $command->getMasterRequest(), - $command->getPermissionsDelta(), - $command->isRecursive(), - $command->applyToNestedResources() - ); - } - - /** - * @deprecated Use applyPermissions() instead - * @deprecated Use oat\taoDacSimple\model\ChangePermissionsService::change - * @see applyPermissions - */ - public function saveResourcePermissionsRecursive( - core_kernel_classes_Resource $resource, - array $privilegesToSet - ): void { - $command = new ChangePermissionsCommand( - $resource, - $resource->getUri(), - $privilegesToSet, - $this->getPermissionsDelta($resource, $privilegesToSet) - ); - $command->withRecursion(); - $command->withNestedResources(); - - $this->applyPermissions($command); - } - - /** - * @deprecated Use applyPermissions() instead - * @deprecated Use oat\taoDacSimple\model\ChangePermissionsService::change - * @see applyPermissions - */ - public function savePermissions( - bool $isRecursive, - core_kernel_classes_Class $class, - array $privilegesToSet - ): void { - $command = new ChangePermissionsCommand( - $class, - $class->getUri(), - $privilegesToSet, - $this->getPermissionsDelta($class, $privilegesToSet) - ); - - if ($isRecursive) { - $command->withRecursion(); - $command->withNestedResources(); - } - - $this->applyPermissions($command); - } - - /** - * @deprecated This should be a private method, please use oat\taoDacSimple\model\ChangePermissionsService::change - */ - public function getPermissionsDelta(core_kernel_classes_Resource $resource, array $permissionsToSet): array - { - //@TODO FIXME Remove this method as it is used only here and we will change to call ChangePermissionsService::chage - $currentPermissions = $this->dataBaseAccess->getResourcePermissions($resource->getUri()); - - return $this->strategy->normalizeRequest($currentPermissions, $permissionsToSet); - } - - /** - * Gets the list of resources to update for a given command. - * - * - For recursive requests, lists both instances and classes contained in - * the root class pointed out by $command. This is used to provide - * backwards-compatible behaviour for savePermissions() and - * saveResourcePermissionsRecursive(). - * - * - For non-recursive requests, returns only resource instances contained - * in the provided class, but skips child classes (as well as resources - * contained in child classes, etc). - * - * @return core_kernel_classes_Resource[] - */ - private function getResourcesToUpdate(ChangePermissionsCommand $command): array - { - $root = $command->getRoot(); - - if ($command->isRecursive()) { - return $this->getResourcesByClassRecursive($root); - } - - if ($command->applyToNestedResources() && $root->isClass()) { - return array_merge([$root], $root->getInstances()); // non-recursive - } - - return [$root]; - } - - private function getActions( - array $resourcesToUpdate, - array $currentResourcePermissions, - array $permissionsDelta - ): array { - $addActions = []; - $removeActions = []; - - foreach ($resourcesToUpdate as $resource) { - $thisResourcePermissions = $currentResourcePermissions[$resource->getUri()]; - - $remove = $this->strategy->getPermissionsToRemove($thisResourcePermissions, $permissionsDelta); - - if (!empty($remove)) { - $removeActions[] = ['permissions' => $remove, 'resource' => $resource]; - } - - $add = $this->strategy->getPermissionsToAdd($thisResourcePermissions, $permissionsDelta); - - if (!empty($add)) { - $addActions[] = ['permissions' => $add, 'resource' => $resource]; - } - } - - return $this->deduplicateActions(['add' => $addActions, 'remove' => $removeActions]); - } - - private function dryRun(array $actions, array $permissionsList): void - { - $resultPermissions = $permissionsList; - - foreach ($actions['remove'] as $item) { - $this->dryRemove($item['permissions'], $item['resource'], $resultPermissions); - } - foreach ($actions['add'] as $item) { - $this->dryAdd($item['permissions'], $item['resource'], $resultPermissions); - } - - $this->assertHasUserWithGrantPermission($resultPermissions); - } - - private function wetRun(array $actions): void - { - if (!empty($actions['remove'])) { - $this->dataBaseAccess->removeMultiplePermissions($actions['remove']); - } - if (!empty($actions['add'])) { - $this->dataBaseAccess->addMultiplePermissions($actions['add']); - } - } - - private function dryRemove(array $remove, core_kernel_classes_Resource $resource, array &$resultPermissions): void - { - foreach ($remove as $userToRemove => $permissionToRemove) { - if (!empty($resultPermissions[$resource->getUri()][$userToRemove])) { - $resultPermissions[$resource->getUri()][$userToRemove] = array_diff( - $resultPermissions[$resource->getUri()][$userToRemove], - $permissionToRemove - ); - } - } - } - - private function dryAdd(array $add, core_kernel_classes_Resource $resource, array &$resultPermissions): void - { - foreach ($add as $userToAdd => $permissionToAdd) { - if (empty($resultPermissions[$resource->getUri()][$userToAdd])) { - $resultPermissions[$resource->getUri()][$userToAdd] = $permissionToAdd; - } else { - $resultPermissions[$resource->getUri()][$userToAdd] = array_merge( - $resultPermissions[$resource->getUri()][$userToAdd], - $permissionToAdd - ); - } - } - } - - private function deduplicateActions(array $actions): array - { - foreach ($actions['add'] as &$entry) { - foreach ($entry['permissions'] as &$grants) { - $grants = array_unique($grants); - } - } - - foreach ($actions['remove'] as &$entry) { - foreach ($entry['permissions'] as &$grants) { - $grants = array_unique($grants); - } - } - - return $actions; - } - - /** - * Provides an array holding the provided resource and, if it is a class, all - * resources that are instances of the provided class or any of its descendants - * plus all descendant classes. - * - * @return core_kernel_classes_Resource[] - */ - private function getResourcesByClassRecursive(core_kernel_classes_Resource $resource): array - { - $resources = [$resource]; - - if ($resource->isClass()) { - $class = $resource->getClass($resource->getUri()); - - $resources = array_merge( - $resources, - $class->getSubClasses(true), - $class->getInstances(true) - ); - } - - return $resources; - } - - private function getResourcesPermissions(array $resources): array - { - if (empty($resources)) { - return []; - } - - $resourceIds = []; - foreach ($resources as $resource) { - $resourceIds[] = $resource->getUri(); - } - - return $this->dataBaseAccess->getResourcesPermissions($resourceIds); - } - - /** - * Checks if all resources after all actions are applied will have at least - * one user with GRANT permission. - */ - private function assertHasUserWithGrantPermission(array $resultPermissions): void - { - foreach ($resultPermissions as $resultResources => $resultUsers) { - $granted = false; - foreach ($resultUsers as $permissions) { - $granted = in_array(PermissionProvider::PERMISSION_GRANT, $permissions, true); - - if ($granted) { - break; - } - } - - if (!$granted) { - throw new PermissionsServiceException( - sprintf( - 'Resource %s should have at least one user with GRANT access', - $resultResources - ) - ); - } - } - } - - private function triggerEvents( - string $resourceId, - string $rootResourceId, - array $addRemove, - bool $isRecursive, - bool $applyToNestedResources - ): void { - if (!empty($addRemove['add'])) { - foreach ($addRemove['add'] as $userId => $rights) { - $this->eventManager->trigger(new DacRootAddedEvent($userId, $resourceId, (array)$rights)); - } - } - if (!empty($addRemove['remove'])) { - foreach ($addRemove['remove'] as $userId => $rights) { - $this->eventManager->trigger(new DacRootRemovedEvent($userId, $resourceId, (array)$rights)); - } - } - $this->eventManager->trigger( - new DacAffectedUsersEvent( - array_keys($addRemove['add'] ?? []), - array_keys($addRemove['remove'] ?? []) - ) - ); - - $this->eventManager->trigger( - new DataAccessControlChangedEvent( - $resourceId, - $addRemove, - $isRecursive, - $applyToNestedResources, - $rootResourceId - ) - ); - } -} diff --git a/model/PermissionsServiceFactory.php b/model/PermissionsServiceFactory.php index 69fc68a..aaca43c 100644 --- a/model/PermissionsServiceFactory.php +++ b/model/PermissionsServiceFactory.php @@ -15,8 +15,7 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * - * Copyright (c) 2020 (original work) Open Assessment Technologies SA; - * + * Copyright (c) 2020-2023 (original work) Open Assessment Technologies SA; */ declare(strict_types=1); @@ -30,24 +29,10 @@ class PermissionsServiceFactory extends ConfigurableService { public const SERVICE_ID = 'taoDacSimple/PermissionsService'; - public const OPTION_SAVE_STRATEGY = 'save_strategy'; - public const OPTION_RECURSIVE_BY_DEFAULT = 'recursive_by_default'; - /** - * @return PermissionsService - */ - public function create(): PermissionsService - { - return new PermissionsService( - $this->getDataBaseAccess(), - $this->getPermissionsStrategy(), - $this->getEventManager() - ); - } - - public function createChangePermissionsService(): ChangePermissionsService + public function create(): ChangePermissionsService { return new ChangePermissionsService( $this->getDataBaseAccess(), diff --git a/model/ServiceProvider/PermissionsServiceProvider.php b/model/ServiceProvider/PermissionsServiceProvider.php new file mode 100644 index 0000000..746b692 --- /dev/null +++ b/model/ServiceProvider/PermissionsServiceProvider.php @@ -0,0 +1,49 @@ + + */ + +declare(strict_types=1); + +namespace oat\taoDacSimple\model\ServiceProvider; + +use oat\taoDacSimple\model\ChangePermissionsService; +use oat\generis\model\DependencyInjection\ContainerServiceProviderInterface; +use oat\taoDacSimple\model\PermissionsServiceFactory; +use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator; + +use function Symfony\Component\DependencyInjection\Loader\Configurator\service; + +class PermissionsServiceProvider implements ContainerServiceProviderInterface +{ + public function __invoke(ContainerConfigurator $configurator): void + { + $services = $configurator->services(); + + $services->set(ChangePermissionsService::class) + ->public() + ->factory( + [ + service(PermissionsServiceFactory::SERVICE_ID), + 'create' + ] + ); + } +} diff --git a/model/eventHandler/ResourceUpdateHandler.php b/model/eventHandler/ResourceUpdateHandler.php index e27055e..4b7ef72 100644 --- a/model/eventHandler/ResourceUpdateHandler.php +++ b/model/eventHandler/ResourceUpdateHandler.php @@ -26,8 +26,8 @@ use oat\generis\model\OntologyAwareTrait; use oat\oatbox\service\ConfigurableService; use oat\tao\model\event\ResourceMovedEvent; -use oat\taoDacSimple\model\PermissionsService; -use oat\taoDacSimple\model\PermissionsServiceFactory; +use oat\taoDacSimple\model\ChangePermissionsService; +use oat\taoDacSimple\model\Command\ChangePermissionsCommand; use oat\taoDacSimple\model\RolePrivilegeRetriever; class ResourceUpdateHandler extends ConfigurableService @@ -53,18 +53,11 @@ public function catchResourceUpdated(ResourceMovedEvent $event): void } } - $permissionService->saveResourcePermissionsRecursive( - $movedResource, - $rolePrivilegeList - ); + $permissionService->change(new ChangePermissionsCommand($movedResource, $rolePrivilegeList, true)); if (isset($itemPrivilegesMap)) { foreach ($itemPrivilegesMap as $uri => $itemPrivilege) { - $permissionService - ->saveResourcePermissionsRecursive( - $this->getResource($uri), - $itemPrivilege - ); + $permissionService->change(new ChangePermissionsCommand($this->getResource($uri), $itemPrivilege, true)); } } } @@ -74,8 +67,8 @@ private function getRolePrivilegeRetriever(): RolePrivilegeRetriever return $this->getServiceLocator()->get(RolePrivilegeRetriever::class); } - private function getPermissionService(): PermissionsService + private function getPermissionService(): ChangePermissionsService { - return $this->getServiceLocator()->get(PermissionsServiceFactory::class)->create(); + return $this->getServiceManager()->getContainer()->get(ChangePermissionsService::class); } } diff --git a/model/tasks/ChangePermissionsTask.php b/model/tasks/ChangePermissionsTask.php index bb70cb7..1991dd8 100644 --- a/model/tasks/ChangePermissionsTask.php +++ b/model/tasks/ChangePermissionsTask.php @@ -32,7 +32,6 @@ use Exception; use JsonSerializable; use oat\taoDacSimple\model\Command\ChangePermissionsCommand; -use oat\taoDacSimple\model\PermissionsServiceFactory; /** * Class ChangePermissionsTask @@ -60,15 +59,10 @@ public function __invoke($params = []): Report try { $command = new ChangePermissionsCommand( $this->getResource($params[self::PARAM_RESOURCE]), - $params[self::PARAM_RESOURCE], (array) $params[self::PARAM_PRIVILEGES], - [] + filter_var($params[self::PARAM_RECURSIVE] ?? false, FILTER_VALIDATE_BOOL) ); - if (filter_var($params[self::PARAM_RECURSIVE] ?? false, FILTER_VALIDATE_BOOL)) { - $command->withRecursion(); - } - $this->getChangePermissionsService()->change($command); return Report::createSuccess('Permissions saved'); @@ -102,6 +96,6 @@ public function jsonSerialize(): string private function getChangePermissionsService(): ChangePermissionsService { - return $this->serviceLocator->get(PermissionsServiceFactory::class)->createChangePermissionsService(); + return $this->getServiceManager()->getContainer()->get(ChangePermissionsService::class); } } diff --git a/test/unit/model/Command/ChangePermissionsCommandTest.php b/test/unit/model/Command/ChangePermissionsCommandTest.php index d57b2f4..4985edb 100644 --- a/test/unit/model/Command/ChangePermissionsCommandTest.php +++ b/test/unit/model/Command/ChangePermissionsCommandTest.php @@ -44,13 +44,11 @@ public function setUp(): void */ public function testConstructor(array $privileges): void { - $sut = new ChangePermissionsCommand($this->root, $this->root->getUri(), $privileges, []); + $sut = new ChangePermissionsCommand($this->root, $privileges); $this->assertSame($this->root, $sut->getRoot()); $this->assertSame($privileges, $sut->getPrivilegesPerUser()); - $this->assertSame([], $sut->getPermissionsDelta()); $this->assertFalse($sut->isRecursive()); - $this->assertFalse($sut->applyToNestedResources()); } public function constructorDataProvider(): array @@ -73,29 +71,8 @@ public function constructorDataProvider(): array public function testWithRecursion(): void { - $sut = new ChangePermissionsCommand($this->root, $this->root->getUri(), [], []); - $sut->withRecursion(); + $sut = new ChangePermissionsCommand($this->root, [], true); $this->assertTrue($sut->isRecursive()); - $this->assertFalse($sut->applyToNestedResources()); - } - - public function testWithNestedResources(): void - { - $sut = new ChangePermissionsCommand($this->root, $this->root->getUri(), [], []); - $sut->withNestedResources(); - - $this->assertFalse($sut->isRecursive()); - $this->assertTrue($sut->applyToNestedResources()); - } - - public function testWithBoth(): void - { - $sut = new ChangePermissionsCommand($this->root, $this->root->getUri(), [], []); - $sut->withRecursion(); - $sut->withNestedResources(); - - $this->assertTrue($sut->isRecursive()); - $this->assertTrue($sut->applyToNestedResources()); } } diff --git a/test/unit/model/PermissionsServiceTest.php b/test/unit/model/PermissionsServiceTest.php deleted file mode 100644 index 2915673..0000000 --- a/test/unit/model/PermissionsServiceTest.php +++ /dev/null @@ -1,341 +0,0 @@ -eventManager = $this->createMock(EventManager::class); - - $this->databaseAccess = $this->createMock(DataBaseAccess::class); - $this->databaseAccess->method('getResourcePermissions')->willReturn([]); - - $this->strategy = $this->createMock(PermissionsStrategyInterface::class); - - $this->service = new PermissionsService( - $this->databaseAccess, - $this->strategy, - $this->eventManager - ); - - $this->service->setLogger($this->createMock(LoggerInterface::class)); - } - - public function testSaveAddPermissions(): void - { - $this->databaseAccess->expects($this->atLeast(1))->method('addMultiplePermissions'); - $this->databaseAccess->expects($this->never())->method('removeMultiplePermissions'); - - $this->databaseAccess->method('getResourcesPermissions')->willReturn( - [ - 'res1' => [] - ] - ); - - $this->strategy->method('normalizeRequest')->willReturn( - [ - 'add' => [ - 'uid1' => ['GRANT', 'READ', 'WRITE'] - ] - ] - ); - - $this->strategy->method('getPermissionsToAdd')->willReturn( - [ - 'uid1' => ['GRANT', 'READ', 'WRITE'] - ] - ); - - $resource = $this->createMock(core_kernel_classes_Class::class); - $resource->method('getUri')->willReturn('res1'); - - $this->mockTriggeredEvents('res1', 'res1', 'uid1', false); - - $this->service->savePermissions( - false, - $resource, - [ - 'uid1' => ['GRANT', 'READ', 'WRITE'] - ] - ); - } - - public function testSavePermissionsAddRecursively(): void - { - $this->databaseAccess->method('getResourcesPermissions')->willReturn( - [ - 'uid2uri' => [] - ] - ); - - $this->databaseAccess->expects($this->once())->method('addMultiplePermissions'); - $this->databaseAccess->expects($this->never())->method('removeMultiplePermissions'); - - $this->strategy->method('normalizeRequest')->willReturn( - [ - 'add' => [ - 'uid1' => ['GRANT', 'READ', 'WRITE'] - ] - ] - ); - - $this->strategy->method('getPermissionsToAdd')->willReturn( - [ - 'uid1' => ['GRANT', 'READ', 'WRITE'] - ] - ); - - /** @var MockObject|core_kernel_classes_Class $childResource */ - $childResource = $this->createMock(core_kernel_classes_Class::class); - $childResource->method('getUri')->willReturn('uid2uri'); - - /** @var MockObject|core_kernel_classes_Class $resource */ - $resource = $this->createMock(core_kernel_classes_Class::class); - $resource->method('getSubClasses')->willReturn( - [ - $childResource - ] - ); - - $resource->method('getInstances')->willReturn([]); - $resource->method('getUri')->willReturn('uid2uri'); - - $this->mockTriggeredEvents('uid2uri', 'uid2uri', 'uid1', true); - - $this->service->savePermissions( - true, - $resource, - [ - 'uid1' => ['GRANT', 'READ', 'WRITE'] - ] - ); - } - - public function testCantRemoveResourceWithNoGrantLeft(): void - { - $this->expectException(PermissionsServiceException::class); - - $this->databaseAccess->method('getResourcesPermissions')->willReturn( - [ - 'uid2uri' => [ - 'uid1' => ['GRANT', 'READ', 'WRITE'] - ] - ] - ); - - $this->strategy->method('normalizeRequest')->willReturn( - [ - 'remove' => [ - 'uid1' => ['GRANT', 'READ', 'WRITE'] - ] - ] - ); - - $this->strategy->method('getPermissionsToRemove')->willReturn( - [ - 'uid1' => ['GRANT', 'READ', 'WRITE'] - ] - ); - - $resource = $this->createMock(core_kernel_classes_Class::class); - $resource->method('getUri')->willReturn('uid2uri'); - - $this->service->savePermissions( - false, - $resource, - [ - 'uid1' => ['GRANT', 'READ', 'WRITE'] - ] - ); - } - - public function testSaveRemovePermissions(): void - { - $this->databaseAccess->method('getResourcesPermissions')->willReturn( - [ - 'uid2uri' => [ - 'uid1' => ['GRANT', 'READ', 'WRITE'] - ] - ] - ); - - $this->databaseAccess->expects($this->never())->method('addMultiplePermissions'); - $this->databaseAccess->expects($this->once())->method('removeMultiplePermissions'); - - $this->strategy->method('normalizeRequest')->willReturn( - [ - 'remove' => [ - 'uid1' => ['GRANT', 'READ', 'WRITE'] - ] - ] - ); - - $this->strategy->method('getPermissionsToRemove')->willReturn( - [ - 'uid1' => ['READ', 'WRITE'] - ] - ); - - $resource = $this->createMock(core_kernel_classes_Class::class); - $resource->method('getUri')->willReturn('uid2uri'); - - $this->service->savePermissions( - false, - $resource, - [ - 'uid1' => ['READ', 'WRITE'] - ] - ); - } - - public function testDoNotSaveAnythingWhenThereIsNothingToSave(): void - { - $this->databaseAccess->expects($this->never())->method('addPermissions'); - $this->databaseAccess->expects($this->never())->method('removePermissions'); - - $this->databaseAccess->method('getResourcesPermissions')->willReturn([]); - - $this->strategy->method('normalizeRequest')->willReturn([]); - - $resource = $this->createMock(core_kernel_classes_Class::class); - - $this->service->savePermissions(false, $resource, []); - } - - public function testDuplicatedAddPermissionsAreNotPersistedTwice(): void - { - $resource = $this->createMock(core_kernel_classes_Class::class); - $resource->method('getUri')->willReturn('res1'); - - $this->strategy - ->method('normalizeRequest') - ->willReturn([ - 'add' => [ - 'uid1' => ['GRANT', 'READ', 'WRITE', 'GRANT'] - ] - ]); - - $this->strategy->method('getPermissionsToAdd')->willReturn([ - 'uid1' => ['GRANT', 'READ', 'WRITE', 'GRANT'] - ]); - - $this->databaseAccess - ->expects($this->once()) - ->method('addMultiplePermissions') - ->with([ - [ - 'resource' => $resource, - 'permissions' => [ - 'uid1' => ['GRANT', 'READ', 'WRITE'] - ] - ] - ]); - - $this->databaseAccess - ->expects($this->never()) - ->method('removeMultiplePermissions'); - - $this->databaseAccess - ->method('getResourcesPermissions') - ->willReturn(['res1' => []]); - - $this->service->savePermissions( - false, - $resource, - [ - 'uid1' => ['GRANT', 'READ', 'WRITE'] - ] - ); - } - - private function mockTriggeredEvents( - string $resourceId, - ?string $rootResourceId, - string $userId, - bool $isRecursive - ): void { - $this->eventManager->expects($this->at(0)) - ->method('trigger') - ->with( - new DacRootAddedEvent( - $userId, - $resourceId, - ['GRANT', 'READ', 'WRITE'] - ) - ); - - $this->eventManager->expects($this->at(1)) - ->method('trigger') - ->with( - new DacAffectedUsersEvent( - [$userId], - [] - ) - ); - - $this->eventManager->expects($this->at(2)) - ->method('trigger') - ->with( - new DataAccessControlChangedEvent( - $resourceId, - [ - 'add' => ['uid1' => ['GRANT', 'READ', 'WRITE']] - ], - $isRecursive, - $isRecursive, - $rootResourceId - ) - ); - } -} diff --git a/test/unit/model/eventHandler/ResourceUpdateHandlerTest.php b/test/unit/model/eventHandler/ResourceUpdateHandlerTest.php index 42a55d8..2d00f83 100644 --- a/test/unit/model/eventHandler/ResourceUpdateHandlerTest.php +++ b/test/unit/model/eventHandler/ResourceUpdateHandlerTest.php @@ -30,8 +30,6 @@ use PHPUnit\Framework\TestCase; use oat\tao\model\event\ResourceMovedEvent; use oat\taoDacSimple\model\eventHandler\ResourceUpdateHandler; -use oat\taoDacSimple\model\PermissionsService; -use oat\taoDacSimple\model\PermissionsServiceFactory; use oat\taoDacSimple\model\RolePrivilegeRetriever; class ResourceUpdateHandlerTest extends TestCase @@ -41,12 +39,6 @@ class ResourceUpdateHandlerTest extends TestCase /** @var RolePrivilegeRetriever|MockObject */ private $rolePrivilegeRetriever; - /** @var PermissionsService|MockObject */ - private $permissionsServiceMock; - - /** @var PermissionsServiceFactory|MockObject */ - private $permissionsServiceFactory; - /** @var ResourceMovedEvent|MockObject */ private $eventMock; @@ -61,9 +53,7 @@ class ResourceUpdateHandlerTest extends TestCase public function setUp(): void { - $this->permissionsServiceMock = $this->createMock(PermissionsService::class); $this->rolePrivilegeRetriever = $this->createMock(RolePrivilegeRetriever::class); - $this->permissionsServiceFactory = $this->createMock(PermissionsServiceFactory::class); $this->eventMock = $this->createMock(ResourceMovedEvent::class); $this->resourceMock = $this->createMock(core_kernel_classes_Resource::class); $this->classMock = $this->createMock(core_kernel_classes_Class::class); @@ -77,10 +67,6 @@ public function setUp(): void ->method('getUri') ->willReturn('destinationClassUri'); - $this->permissionsServiceFactory - ->method('create') - ->willReturn($this->permissionsServiceMock); - $this->subject = new ResourceUpdateHandler(); $this->subject->setModel($this->ontoloigyModelMock); @@ -88,7 +74,6 @@ public function setUp(): void $this->getServiceManagerMock( [ RolePrivilegeRetriever::class => $this->rolePrivilegeRetriever, - PermissionsServiceFactory::class => $this->permissionsServiceFactory ] ) ); @@ -127,7 +112,7 @@ public function testCatchResourceUpdated() $this->permissionsServiceMock ->expects($this->once()) - ->method('saveResourcePermissionsRecursive') + ->method('change') ->with( $this->eventMock->getMovedResource(), [ From dfcdf28c95a357c23143e443220af1cbfddc1288 Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Fri, 20 Oct 2023 12:03:41 +0200 Subject: [PATCH 29/41] chore: remove TODO for events --- model/ChangePermissionsService.php | 36 ------------------------------ 1 file changed, 36 deletions(-) diff --git a/model/ChangePermissionsService.php b/model/ChangePermissionsService.php index 9a92951..62b044a 100644 --- a/model/ChangePermissionsService.php +++ b/model/ChangePermissionsService.php @@ -216,40 +216,4 @@ private function triggerEvents( ) ); } - -//@TODO FIXME The events bellow were sent before and we need to make sure they are either not necessary anymore or still sent -// private function triggerEvents( -// string $resourceId, -// string $rootResourceId, -// array $addRemove, -// bool $isRecursive, -// bool $applyToNestedResources -// ): void { -// if (!empty($addRemove['add'])) { -// foreach ($addRemove['add'] as $userId => $rights) { -// $this->eventManager->trigger(new DacRootAddedEvent($userId, $resourceId, (array)$rights)); -// } -// } -// if (!empty($addRemove['remove'])) { -// foreach ($addRemove['remove'] as $userId => $rights) { -// $this->eventManager->trigger(new DacRootRemovedEvent($userId, $resourceId, (array)$rights)); -// } -// } -// $this->eventManager->trigger( -// new DacAffectedUsersEvent( -// array_keys($addRemove['add'] ?? []), -// array_keys($addRemove['remove'] ?? []) -// ) -// ); -// -// $this->eventManager->trigger( -// new DataAccessControlChangedEvent( -// $resourceId, -// $addRemove, -// $isRecursive, -// $applyToNestedResources, -// $rootResourceId -// ) -// ); -// } } From 58f1c1fc2880f9afcc5aa21d5cccb2dedfeff3bd Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Fri, 20 Oct 2023 13:14:59 +0300 Subject: [PATCH 30/41] chore: fix issue with array unique, add event --- model/ChangePermissionsService.php | 16 ++--- model/DataBaseAccess.php | 103 +++++++++++++++++++---------- 2 files changed, 74 insertions(+), 45 deletions(-) diff --git a/model/ChangePermissionsService.php b/model/ChangePermissionsService.php index 62b044a..5362c1a 100644 --- a/model/ChangePermissionsService.php +++ b/model/ChangePermissionsService.php @@ -110,11 +110,9 @@ private function enrichWithAddRemoveActions( ChangeAccessCommand $command ): array { foreach ($resources as &$resource) { - $resource['permissions']['remove'] = array_unique( - $this->strategy->getPermissionsToRemove( - $resource['permissions']['current'], - $permissionsDelta - ) + $resource['permissions']['remove'] = $this->strategy->getPermissionsToRemove( + $resource['permissions']['current'], + $permissionsDelta ); foreach ($resource['permissions']['remove'] as $userId => $permissions) { @@ -123,11 +121,9 @@ private function enrichWithAddRemoveActions( } } - $resource['permissions']['add'] = array_unique( - $this->strategy->getPermissionsToAdd( - $resource['permissions']['current'], - $permissionsDelta - ) + $resource['permissions']['add'] = $this->strategy->getPermissionsToAdd( + $resource['permissions']['current'], + $permissionsDelta ); foreach ($resource['permissions']['add'] as $userId => $permissions) { diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index 81ec830..060ce6a 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -106,47 +106,63 @@ public function getPermissionsByUsersAndResources(array $userIds, array $resourc public function changeAccess(ChangeAccessCommand $command): void { $persistence = $this->getPersistence(); - $class = $this; - - $persistence->transactional(static function () use ($command, $persistence, $class): void { - foreach ($command->getUserIdsToRevokePermissions() as $userId) { - foreach ($command->getUserPermissionsToRevoke($userId) as $permission) { - $resourceIds = $command->getResourceIdsByUserAndPermissionToRevoke($userId, $permission); - - if (count($resourceIds) > 0) { - $persistence->exec( - sprintf( - 'DELETE FROM data_privileges WHERE user_id = ? AND privilege = ? AND resource_id IN (%s)', - implode(',', array_fill(0, count($resourceIds), ' ? ')), - ), - array_merge([$userId, $permission], $resourceIds) - ); + + try { + $persistence->transactional(function () use ($command, $persistence): void { + $removed = []; + + foreach ($command->getUserIdsToRevokePermissions() as $userId) { + $permissions = $command->getUserPermissionsToRevoke($userId); + + foreach ($permissions as $permission) { + $resourceIds = $command->getResourceIdsByUserAndPermissionToRevoke($userId, $permission); + + if (!empty($resourceIds)) { + // phpcs:disable Generic.Files.LineLength + $persistence->exec( + sprintf( + 'DELETE FROM data_privileges WHERE user_id = ? AND privilege = ? AND resource_id IN (%s)', + implode(',', array_fill(0, count($resourceIds), ' ? ')), + ), + array_merge([$userId, $permission], $resourceIds) + ); + // phpcs:enable Generic.Files.LineLength + + foreach ($resourceIds as $resourceId) { + $this->addEventValue($removed, $userId, $resourceId, $permission); + } + } } } - } - $insert = []; + $insert = []; + $added = []; - foreach ($command->getResourceIdsToGrant() as $resourceId) { - foreach (PermissionProvider::ALLOWED_PERMISSIONS as $permission) { - $usersIds = $command->getUserIdsToGrant($resourceId, $permission); + foreach ($command->getResourceIdsToGrant() as $resourceId) { + foreach (PermissionProvider::ALLOWED_PERMISSIONS as $permission) { + $usersIds = $command->getUserIdsToGrant($resourceId, $permission); - foreach ($usersIds as $userId) { - $insert[] = [ - 'user_id' => $userId, - 'resource_id' => $resourceId, - 'privilege' => $permission, - ]; + foreach ($usersIds as $userId) { + $insert[] = [ + 'user_id' => $userId, + 'resource_id' => $resourceId, + 'privilege' => $permission, + ]; + + $this->addEventValue($added, $userId, $resourceId, $permission); + } } } - } - $class->insertPermissions($insert, $persistence); + $this->insertPermissions($insert); - /** - * @TODO FIXME Call proper events here - */ - }); + if (!empty($added) || !empty($removed)) { + $this->getEventManager()->trigger(new DacChangedEvent($added, $removed)); + } + }); + } catch (Throwable $exception) { + $this->logError('Error while changing access: ' . $exception->getMessage()); + } } /** @@ -282,8 +298,7 @@ public function addMultiplePermissions(array $permissionData) } } - $persistence = $this->getPersistence(); - $persistence->transactional(fn () => $this->insertPermissions($insert, $persistence)); + $this->getPersistence()->transactional(fn () => $this->insertPermissions($insert)); foreach ($insert as $inserted) { $this->getEventManager()->trigger(new DacAddedEvent( @@ -530,7 +545,7 @@ public function removeTables() /** * @throws Throwable */ - private function insertPermissions(array $insert, common_persistence_SqlPersistence $persistence): void + private function insertPermissions(array $insert): void { if (empty($insert)) { return; @@ -538,6 +553,7 @@ private function insertPermissions(array $insert, common_persistence_SqlPersiste $logger = $this->getLogger(); $insertCount = count($insert); + $persistence = $this->getPersistence(); foreach (array_chunk($insert, $this->insertChunkSize) as $index => $batch) { $logger->debug( @@ -552,4 +568,21 @@ private function insertPermissions(array $insert, common_persistence_SqlPersiste $persistence->insertMultiple(self::TABLE_PRIVILEGES_NAME, $batch); } } + + private function addEventValue(array &$eventData, string $userId, string $resourceId, string $permission): void + { + $key = $userId . $resourceId; + + if (array_key_exists($key, $eventData)) { + $eventData[$key]['privileges'][] = $permission; + + return; + } + + $eventData[] = [ + 'userId' => $userId, + 'resourceId' => $resourceId, + 'privileges' => [$permission], + ]; + } } From 42d6eeae29633e5fe65a113eb653577d4899319e Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Fri, 20 Oct 2023 13:27:17 +0200 Subject: [PATCH 31/41] chore: remove old methods and add proper signature for new methods --- model/AdminService.php | 4 +- model/DataBaseAccess.php | 199 +++++++------------------ test/unit/model/DataBaseAccessTest.php | 74 --------- 3 files changed, 53 insertions(+), 224 deletions(-) diff --git a/model/AdminService.php b/model/AdminService.php index 204d512..7fe67d6 100644 --- a/model/AdminService.php +++ b/model/AdminService.php @@ -51,7 +51,9 @@ public static function setOwner($resourceUri, $userUri) } } - return $db->addPermissions($userUri, $resourceUri, ['OWNER']); + $db->addPermissions($userUri, $resourceUri, ['OWNER']); + + return true; } /** diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index 060ce6a..7d81bbd 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -59,15 +59,6 @@ public function setInsertChunkSize(int $size): void $this->insertChunkSize = $size; } - /** - * @return EventManager - */ - protected function getEventManager() - { - return $this->getServiceLocator()->get(EventManager::SERVICE_ID); - } - - public function getPermissionsByUsersAndResources(array $userIds, array $resourceIds): array { /** @@ -103,6 +94,12 @@ public function getPermissionsByUsersAndResources(array $userIds, array $resourc return $data; } + /** + * Allow to grant/revoke access for several users and resources + * + * @param ChangeAccessCommand $command + * @return void + */ public function changeAccess(ChangeAccessCommand $command): void { $persistence = $this->getPersistence(); @@ -168,10 +165,15 @@ public function changeAccess(ChangeAccessCommand $command): void /** * Retrieve info on users having privileges on a set of resources * - * @param array $resourceIds IDs of resources to fetch privileges for - * @return array A list of rows containing resource ID, user ID and privilege name + * @return array [ + * [ + * '{resourceId}', + * '{userId}', + * '{privilege}' + * ] + * ] */ - public function getUsersWithPermissions($resourceIds) + public function getUsersWithPermissions(array $resourceIds): array { $inQuery = implode(',', array_fill(0, count($resourceIds), '?')); $query = sprintf( @@ -190,10 +192,9 @@ public function getUsersWithPermissions($resourceIds) /** * Get the permissions for a list of resources and users * - * @access public - * @param array $userIds - * @param array $resourceIds - * @return array + * @return array [ + * '{resourceId}' => ['READ', 'WRITE'], + * ] */ public function getPermissions(array $userIds, array $resourceIds): array { @@ -227,9 +228,15 @@ public function getPermissions(array $userIds, array $resourceIds): array return $returnValue; } - public function getResourcesPermissions(array $resourceIds) + /** + * @return array [ + * '{resourceId}' => [ + * '{userId}' => ['READ', 'WRITE'], + * ] + * ] + */ + public function getResourcesPermissions(array $resourceIds): array { - // Return an empty array for resources not having permissions data $grants = array_fill_keys($resourceIds, []); foreach ($this->getUsersWithPermissions($resourceIds) as $entry) { @@ -243,16 +250,10 @@ public function getResourcesPermissions(array $resourceIds) /** * Add permissions of a user to a resource * - * @access public - * @param string $user - * @param string $resourceId - * @param array $rights - * - * @return bool + * @deprecated Please use $this::changeAccess() */ - public function addPermissions($user, $resourceId, $rights) + public function addPermissions(string $user, string $resourceId, array $rights): void { - // Add an ACL item for each user URI, resource ID and privilege combination foreach ($rights as $privilege) { $this->getPersistence()->insert( self::TABLE_PRIVILEGES_NAME, @@ -269,54 +270,19 @@ public function addPermissions($user, $resourceId, $rights) $resourceId, (array)$rights )); - - return true; - } - - /** - * Add batch permissions - * - * @access public - * @param array $permissionData - * @return void - * @throws Throwable - */ - public function addMultiplePermissions(array $permissionData) - { - $insert = []; - foreach ($permissionData as $permissionItem) { - foreach ($permissionItem['permissions'] as $userId => $privilegeIds) { - if (!empty($privilegeIds)) { - foreach ($privilegeIds as $privilegeId) { - $insert [] = [ - self::COLUMN_USER_ID => $userId, - self::COLUMN_RESOURCE_ID => $permissionItem['resource']->getUri(), - self::COLUMN_PRIVILEGE => $privilegeId - ]; - } - } - } - } - - $this->getPersistence()->transactional(fn () => $this->insertPermissions($insert)); - - foreach ($insert as $inserted) { - $this->getEventManager()->trigger(new DacAddedEvent( - $inserted[self::COLUMN_USER_ID], - $inserted[self::COLUMN_RESOURCE_ID], - (array)$inserted[self::COLUMN_PRIVILEGE] - )); - } } /** * Get the permissions to resource * - * @access public - * @param string $resourceId - * @return array + * @return array [ + * '{userId}' => [ + * 'READ', + * 'WRITE', + * ] + * ] */ - public function getResourcePermissions($resourceId) + public function getResourcePermissions(string $resourceId): array { $grants = []; $query = sprintf( @@ -337,13 +303,9 @@ public function getResourcePermissions($resourceId) /** * remove permissions to a resource for a user * - * @access public - * @param string $user - * @param string $resourceId - * @param array $rights - * @return boolean + * @deprecated Please use $this::changeAccess() */ - public function removePermissions($user, $resourceId, $rights) + public function removePermissions(string $user, string $resourceId, array $rights): void { //get all entries that match (user,resourceId) and remove them $inQueryPrivilege = implode(',', array_fill(0, count($rights), ' ? ')); @@ -364,81 +326,14 @@ public function removePermissions($user, $resourceId, $rights) $resourceId, $rights )); - - return true; - } - - /** - * Remove batch permissions - * - * @access public - * @param array $data - * @return void - */ - public function removeMultiplePermissions(array $data) - { - $groupedRemove = []; - $eventsData = []; - - foreach ($data as $permissionItem) { - $resource = &$permissionItem['resource']; - - foreach ($permissionItem['permissions'] as $userId => $privilegeIds) { - if (!empty($privilegeIds)) { - $idString = implode($privilegeIds); - - $groupedRemove[$userId][$idString]['resources'][] = $resource->getUri(); - $groupedRemove[$userId][$idString]['privileges'] = $privilegeIds; - - $eventsData[] = [ - 'userId' => $userId, - 'resourceId' => $resource->getUri(), - 'privileges' => $privilegeIds - ]; - } - } - } - foreach ($groupedRemove as $userId => $resources) { - foreach ($resources as $permissions) { - $inQueryPrivilege = implode(',', array_fill(0, count($permissions['privileges']), ' ? ')); - $inQueryResources = implode(',', array_fill(0, count($permissions['resources']), ' ? ')); - $query = sprintf( - 'DELETE FROM %s WHERE %s IN (%s) AND %s IN (%s) AND %s = ?', - self::TABLE_PRIVILEGES_NAME, - self::COLUMN_RESOURCE_ID, - $inQueryResources, - self::COLUMN_PRIVILEGE, - $inQueryPrivilege, - self::COLUMN_USER_ID - ); - - $params = array_merge( - array_values($permissions['resources']), - array_values($permissions['privileges']), - [$userId] - ); - - $this->getPersistence()->exec($query, $params); - } - } - - foreach ($eventsData as $eventData) { - $this->getEventManager()->trigger(new DacRemovedEvent( - $eventData['userId'], - $eventData['resourceId'], - $eventData['privileges'] - )); - } } /** - * Remove all permissions from a resource + * Completely remove all permissions to any user for the resourceIds * - * @access public - * @param array $resourceIds - * @return boolean + * @deprecated Please use $this::changeAccess() */ - public function removeAllPermissions($resourceIds) + public function removeAllPermissions(array $resourceIds): void { //get all entries that match (resourceId) and remove them $inQuery = implode(',', array_fill(0, count($resourceIds), ' ? ')); @@ -450,20 +345,21 @@ public function removeAllPermissions($resourceIds) ); $this->getPersistence()->exec($query, $resourceIds); + foreach ($resourceIds as $resourceId) { $this->getEventManager()->trigger(new DacRemovedEvent('-', $resourceId, ['-'])); } - return true; } /** * Filter users\roles that have no permissions * - * @access public - * @param array $userIds - * @return array + * @return array [ + * '{userId}' => '{userId}', + * '{userId}' => '{userId}', + * ] */ - public function checkPermissions($userIds) + public function checkPermissions(array $userIds): array { $chunks = array_chunk($userIds, $this->getOption(self::OPTION_FETCH_USER_PERMISSIONS_CHUNK_SIZE, 20)); $existingUsers = []; @@ -487,6 +383,11 @@ public function checkPermissions($userIds) return $existingUsers; } + private function getEventManager(): EventManager + { + return $this->getServiceLocator()->get(EventManager::SERVICE_ID); + } + /** * @return common_persistence_SqlPersistence */ diff --git a/test/unit/model/DataBaseAccessTest.php b/test/unit/model/DataBaseAccessTest.php index 59aef58..cc69646 100644 --- a/test/unit/model/DataBaseAccessTest.php +++ b/test/unit/model/DataBaseAccessTest.php @@ -213,80 +213,6 @@ public function testGetPermissions(array $userIds, array $resourceIds) $this->assertEquals([], $this->sut->getPermissions($userIds, [])); } - /** - * @dataProvider addMultiplePermissionsDataProvider - */ - public function testAddMultiplePermissions(int $numEvents, int $numInserts, array $permissionData): void - { - $this->persistenceMock - ->expects($this->exactly($numInserts)) - ->method('insertMultiple') - ->with(DataBaseAccess::TABLE_PRIVILEGES_NAME, self::anything()) - ->willReturnCallback(function ($tableName, array $data, array $types = []) { - return count($data); - }); - - $this->persistenceMock - ->expects($this->exactly($numInserts > 0 ? 1 : 0)) - ->method('transactional') - ->willReturnCallback(function (callable $closure) { - $closure(); - }); - - $this->eventManager - ->expects($this->exactly($numEvents)) - ->method('trigger') - ->with($this->isInstanceOf(DacAddedEvent::class)); - - $this->sut->addMultiplePermissions($permissionData); - } - - public function addMultiplePermissionsDataProvider(): array - { - return [ - 'Empty arrays don\'t result in events nor queries' => [ - 'numEvents' => 0, - 'numInserts' => 0, - 'permissionData' => [] - ], - 'Empty permissions don\'t result in events nor queries' => [ - 'numEvents' => 0, - 'numInserts' => 0, - 'permissionData' => [ - [ - 'resource' => $this->getResourceMock('123'), - 'permissions' => [] - ] - ] - ], - '3 permissions: One resource, single user' => [ - 'numEvents' => 3, - 'numInserts' => 3, - 'permissionData' => [ - [ - 'resource' => $this->getResourceMock('123'), - 'permissions' => [ - 123 => ['GRANT', 'READ', 'WRITE'] - ] - ] - ] - ], - '6 permissions: One resource, two users' => [ - 'numEvents' => 6, - 'numInserts' => 6, - 'permissionData' => [ - [ - 'resource' => $this->getResourceMock('456'), - 'permissions' => [ - 123 => ['GRANT', 'READ', 'WRITE'], - 456 => ['GRANT', 'WRITE', 'READ'] - ] - ] - ] - ] - ]; - } - private function getResourceMock(string $id): core_kernel_classes_Resource { $resourceMock = $this->createMock(core_kernel_classes_Resource::class); From 779a565840378db2bc1bac26432d7217426be0eb Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Fri, 20 Oct 2023 13:50:28 +0200 Subject: [PATCH 32/41] chore: add missing typehints and docs --- model/DataBaseAccess.php | 71 ++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 39 deletions(-) diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index 7d81bbd..7ef35e3 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -15,9 +15,11 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * - * Copyright (c) 2014-2021 (original work) Open Assessment Technologies SA (under the project TAO-PRODUCT); + * Copyright (c) 2014-2023 (original work) Open Assessment Technologies SA (under the project TAO-PRODUCT); */ +declare(strict_types=1); + namespace oat\taoDacSimple\model; use common_persistence_SqlPersistence; @@ -96,9 +98,6 @@ public function getPermissionsByUsersAndResources(array $userIds, array $resourc /** * Allow to grant/revoke access for several users and resources - * - * @param ChangeAccessCommand $command - * @return void */ public function changeAccess(ChangeAccessCommand $command): void { @@ -383,36 +382,19 @@ public function checkPermissions(array $userIds): array return $existingUsers; } - private function getEventManager(): EventManager - { - return $this->getServiceLocator()->get(EventManager::SERVICE_ID); - } - - /** - * @return common_persistence_SqlPersistence - */ - private function getPersistence() + public function removeTables(): void { - if (!$this->persistence) { - $this->persistence = $this->getServiceLocator()->get(PersistenceManager::SERVICE_ID) - ->getPersistenceById($this->getOption(self::OPTION_PERSISTENCE)); + $persistence = $this->getPersistence(); + $schema = $persistence->getDriver()->getSchemaManager()->createSchema(); + $fromSchema = clone $schema; + $schema->dropTable(self::TABLE_PRIVILEGES_NAME); + $queries = $persistence->getPlatform()->getMigrateSchemaSql($fromSchema, $schema); + foreach ($queries as $query) { + $persistence->exec($query); } - return $this->persistence; - } - - - /** - * @param string $query - * @param array $params - * @return array - */ - private function fetchQuery($query, $params) - { - $statement = $this->getPersistence()->query($query, $params); - return $statement->fetchAll(PDO::FETCH_ASSOC); } - public function createTables() + public function createTables(): void { $schemaManager = $this->getPersistence()->getDriver()->getSchemaManager(); $schema = $schemaManager->createSchema(); @@ -430,17 +412,28 @@ public function createTables() } } + private function getEventManager(): EventManager + { + return $this->getServiceLocator()->get(EventManager::SERVICE_ID); + } - public function removeTables() + /** + * @return common_persistence_SqlPersistence + */ + private function getPersistence() { - $persistence = $this->getPersistence(); - $schema = $persistence->getDriver()->getSchemaManager()->createSchema(); - $fromSchema = clone $schema; - $table = $schema->dropTable(self::TABLE_PRIVILEGES_NAME); - $queries = $persistence->getPlatform()->getMigrateSchemaSql($fromSchema, $schema); - foreach ($queries as $query) { - $persistence->exec($query); + if (!$this->persistence) { + $this->persistence = $this->getServiceLocator()->get(PersistenceManager::SERVICE_ID) + ->getPersistenceById($this->getOption(self::OPTION_PERSISTENCE)); } + return $this->persistence; + } + + private function fetchQuery(string $query, array $params): array + { + return $this->getPersistence() + ->query($query, $params) + ->fetchAll(PDO::FETCH_ASSOC); } /** @@ -480,7 +473,7 @@ private function addEventValue(array &$eventData, string $userId, string $resour return; } - $eventData[] = [ + $eventData[$key] = [ 'userId' => $userId, 'resourceId' => $resourceId, 'privileges' => [$permission], From ec2a06da5739d00fee611ac92e44dbf8180fbc8a Mon Sep 17 00:00:00 2001 From: Gabriel Felipe Soares Date: Fri, 20 Oct 2023 14:34:16 +0200 Subject: [PATCH 33/41] chore: delete in chunks + remove try/catch and delegate to upper level --- model/DataBaseAccess.php | 138 +++++++++++-------------- test/unit/model/DataBaseAccessTest.php | 2 +- 2 files changed, 63 insertions(+), 77 deletions(-) diff --git a/model/DataBaseAccess.php b/model/DataBaseAccess.php index 7ef35e3..8f6f887 100644 --- a/model/DataBaseAccess.php +++ b/model/DataBaseAccess.php @@ -52,41 +52,42 @@ class DataBaseAccess extends ConfigurableService public const TABLE_PRIVILEGES_NAME = 'data_privileges'; public const INDEX_RESOURCE_ID = 'data_privileges_resource_id_index'; - private $insertChunkSize = 20000; + private $writeChunkSize = 1000; private $persistence; - public function setInsertChunkSize(int $size): void + public function setWriteChunkSize(int $size): void { - $this->insertChunkSize = $size; + $this->writeChunkSize = $size; } + /** + * @return array [ + * '{resourceId}' => [ + * '{userId}' => ['GRANT'], + * ] + * ] + */ public function getPermissionsByUsersAndResources(array $userIds, array $resourceIds): array { - /** - * @TODO FIXME Lets see if we can make this methods return better contract or if it is still necessary - */ - // Permissions for an empty set of resources must be an empty array - if (empty($resourceIds)) { + if (empty($resourceIds) || empty($userIds)) { return []; } - $inQueryResources = implode(',', array_fill(0, count($resourceIds), '?')); - $inQueryUsers = implode(',', array_fill(0, count($userIds), '?')); - - $query = <<fetchQuery($query, [...$resourceIds, ...$userIds]); + // phpcs:disable Generic.Files.LineLength + $results = $this->fetchQuery( + sprintf( + 'SELECT resource_id, user_id, privilege FROM data_privileges WHERE resource_id IN (%s) AND user_id IN (%s)', + implode(',', array_fill(0, count($resourceIds), '?')), + implode(',', array_fill(0, count($userIds), '?')) + ), + [ + ...$resourceIds, + ...$userIds + ] + ); + // phpcs:disable Generic.Files.LineLength - // If resource doesn't have permission don't return null $data = array_fill_keys($resourceIds, []); foreach ($results as $result) { @@ -103,62 +104,60 @@ public function changeAccess(ChangeAccessCommand $command): void { $persistence = $this->getPersistence(); - try { - $persistence->transactional(function () use ($command, $persistence): void { - $removed = []; + $persistence->transactional(function () use ($command, $persistence): void { + $removed = []; - foreach ($command->getUserIdsToRevokePermissions() as $userId) { - $permissions = $command->getUserPermissionsToRevoke($userId); + foreach ($command->getUserIdsToRevokePermissions() as $userId) { + $permissions = $command->getUserPermissionsToRevoke($userId); - foreach ($permissions as $permission) { - $resourceIds = $command->getResourceIdsByUserAndPermissionToRevoke($userId, $permission); + foreach ($permissions as $permission) { + $resourceIds = $command->getResourceIdsByUserAndPermissionToRevoke($userId, $permission); - if (!empty($resourceIds)) { + if (!empty($resourceIds)) { + foreach (array_chunk($resourceIds, $this->writeChunkSize) as $batch) { // phpcs:disable Generic.Files.LineLength $persistence->exec( sprintf( 'DELETE FROM data_privileges WHERE user_id = ? AND privilege = ? AND resource_id IN (%s)', - implode(',', array_fill(0, count($resourceIds), ' ? ')), + implode(',', array_fill(0, count($batch), '?')), ), - array_merge([$userId, $permission], $resourceIds) + array_merge([$userId, $permission], $batch) ); // phpcs:enable Generic.Files.LineLength + } - foreach ($resourceIds as $resourceId) { - $this->addEventValue($removed, $userId, $resourceId, $permission); - } + foreach ($resourceIds as $resourceId) { + $this->addEventValue($removed, $userId, $resourceId, $permission); } } } + } - $insert = []; - $added = []; + $insert = []; + $added = []; - foreach ($command->getResourceIdsToGrant() as $resourceId) { - foreach (PermissionProvider::ALLOWED_PERMISSIONS as $permission) { - $usersIds = $command->getUserIdsToGrant($resourceId, $permission); + foreach ($command->getResourceIdsToGrant() as $resourceId) { + foreach (PermissionProvider::ALLOWED_PERMISSIONS as $permission) { + $usersIds = $command->getUserIdsToGrant($resourceId, $permission); - foreach ($usersIds as $userId) { - $insert[] = [ - 'user_id' => $userId, - 'resource_id' => $resourceId, - 'privilege' => $permission, - ]; + foreach ($usersIds as $userId) { + $insert[] = [ + 'user_id' => $userId, + 'resource_id' => $resourceId, + 'privilege' => $permission, + ]; - $this->addEventValue($added, $userId, $resourceId, $permission); - } + $this->addEventValue($added, $userId, $resourceId, $permission); } } + } - $this->insertPermissions($insert); + $this->insertPermissions($insert); - if (!empty($added) || !empty($removed)) { - $this->getEventManager()->trigger(new DacChangedEvent($added, $removed)); - } - }); - } catch (Throwable $exception) { - $this->logError('Error while changing access: ' . $exception->getMessage()); - } + if (!empty($added) || !empty($removed)) { + $this->getEventManager()->trigger(new DacChangedEvent($added, $removed)); + } + }); } /** @@ -351,7 +350,7 @@ public function removeAllPermissions(array $resourceIds): void } /** - * Filter users\roles that have no permissions + * Filter users\roles that have permissions * * @return array [ * '{userId}' => '{userId}', @@ -441,25 +440,12 @@ private function fetchQuery(string $query, array $params): array */ private function insertPermissions(array $insert): void { - if (empty($insert)) { - return; - } - - $logger = $this->getLogger(); - $insertCount = count($insert); - $persistence = $this->getPersistence(); + if (!empty($insert)) { + $persistence = $this->getPersistence(); - foreach (array_chunk($insert, $this->insertChunkSize) as $index => $batch) { - $logger->debug( - sprintf( - 'Processing chunk %d/%d with %d ACL entries', - $index + 1, - ceil($insertCount / $this->insertChunkSize), - count($batch), - ) - ); - - $persistence->insertMultiple(self::TABLE_PRIVILEGES_NAME, $batch); + foreach (array_chunk($insert, $this->writeChunkSize) as $batch) { + $persistence->insertMultiple(self::TABLE_PRIVILEGES_NAME, $batch); + } } } diff --git a/test/unit/model/DataBaseAccessTest.php b/test/unit/model/DataBaseAccessTest.php index cc69646..6358922 100644 --- a/test/unit/model/DataBaseAccessTest.php +++ b/test/unit/model/DataBaseAccessTest.php @@ -63,7 +63,7 @@ public function setUp(): void $this->sut = new DataBaseAccess(); $this->sut->setLogger(new NullLogger()); - $this->sut->setInsertChunkSize(self::INSERT_CHUNK_SIZE); + $this->sut->setWriteChunkSize(self::INSERT_CHUNK_SIZE); $this->sut->setServiceLocator( $this->getServiceManagerMock( [ From 243d4a66f0c7081f972dba9140aac24d6ab2512c Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Fri, 20 Oct 2023 16:05:02 +0300 Subject: [PATCH 34/41] chore: reduce number of iterations --- model/ChangePermissionsService.php | 94 +++++++-------------------- model/Command/ChangeAccessCommand.php | 3 - 2 files changed, 23 insertions(+), 74 deletions(-) diff --git a/model/ChangePermissionsService.php b/model/ChangePermissionsService.php index 5362c1a..d58183a 100644 --- a/model/ChangePermissionsService.php +++ b/model/ChangePermissionsService.php @@ -49,22 +49,16 @@ public function __construct( public function change(ChangePermissionsCommand $command): void { $resources = $this->getResourceToUpdate($command->getRoot(), $command->isRecursive()); - $this->enrichWithPermissions($resources); - $rootResourcePermissions = $resources[$command->getRoot()->getUri()]['permissions']; - $permissionsDelta = $this->strategy->normalizeRequest( - $rootResourcePermissions['current'] ?? [], - $command->getPrivilegesPerUser() - ); + $permissions = $this->dataBaseAccess->getResourcesPermissions(array_column($resources, 'id')); + $rootPermissions = $permissions[$command->getRoot()->getUri()]; + $permissionsDelta = $this->strategy->normalizeRequest($rootPermissions, $command->getPrivilegesPerUser()); if (empty($permissionsDelta['remove']) && empty($permissionsDelta['add'])) { return; } - $changeAccessCommand = new ChangeAccessCommand(); - $resources = $this->enrichWithAddRemoveActions($resources, $permissionsDelta, $changeAccessCommand); - $this->dryRun($resources); - + $changeAccessCommand = $this->calculateChanges($resources, $permissionsDelta, $permissions); $this->dataBaseAccess->changeAccess($changeAccessCommand); $this->triggerEvents($command->getRoot(), $permissionsDelta, $command->isRecursive()); @@ -91,90 +85,48 @@ private function getResourceToUpdate(core_kernel_classes_Resource $resource, boo ]; } - private function enrichWithPermissions(array &$resources): void - { - if (empty($resources)) { - return; - } + private function calculateChanges( + array $resources, + array $permissionsDelta, + array $currentPermissions + ): ChangeAccessCommand { + $command = new ChangeAccessCommand(); - $permissions = $this->dataBaseAccess->getResourcesPermissions(array_column($resources, 'id')); + foreach ($resources as $resource) { + $resourcePermissions = $currentPermissions[$resource['id']]; - foreach ($resources as &$resource) { - $resource['permissions']['current'] = $permissions[$resource['id']]; - } - } + $remove = $this->strategy->getPermissionsToRemove($resourcePermissions, $permissionsDelta); + $add = $this->strategy->getPermissionsToAdd($resourcePermissions, $permissionsDelta); + + foreach ($remove as $userId => $permissions) { + $resourcePermissions[$userId] = array_diff($resourcePermissions[$userId] ?? [], $permissions); - private function enrichWithAddRemoveActions( - array $resources, - array $permissionsDelta, - ChangeAccessCommand $command - ): array { - foreach ($resources as &$resource) { - $resource['permissions']['remove'] = $this->strategy->getPermissionsToRemove( - $resource['permissions']['current'], - $permissionsDelta - ); - - foreach ($resource['permissions']['remove'] as $userId => $permissions) { foreach ($permissions as $permission) { $command->revokeResourceForUser($resource['id'], $permission, $userId); } } - $resource['permissions']['add'] = $this->strategy->getPermissionsToAdd( - $resource['permissions']['current'], - $permissionsDelta - ); + foreach ($add as $userId => $permissions) { + $resourcePermissions[$userId] = array_merge($resourcePermissions[$userId] ?? [], $permissions); - foreach ($resource['permissions']['add'] as $userId => $permissions) { foreach ($permissions as $permission) { $command->grantResourceForUser($resource['id'], $permission, $userId); } } - } - - return $resources; - } - - private function dryRun(array $resources): void - { - foreach ($resources as $resource) { - $newPermissions = $resource['permissions']['current']; - - $this->dryRemove($resource, $newPermissions); - $this->dryAdd($resource, $newPermissions); - $this->assertHasUserWithGrantPermission($resource['id'], $newPermissions); + $this->assertHasUserWithGrantPermission($resource['id'], $resourcePermissions); } - } - private function dryRemove(array $resource, array &$newPermissions): void - { - foreach ($resource['permissions']['remove'] as $user => $removePermissions) { - $newPermissions[$user] = array_diff( - $newPermissions[$user] ?? [], - $removePermissions - ); - } - } - - private function dryAdd(array $resource, array &$newPermissions): void - { - foreach ($resource['permissions']['add'] as $user => $addPermissions) { - $newPermissions[$user] = array_merge( - $resource['permissions']['current'][$user] ?? [], - $addPermissions - ); - } + return $command; } /** * Checks if all resources after all actions are applied will have at least * one user with GRANT permission. */ - private function assertHasUserWithGrantPermission(string $resourceId, array $newPermissions): void + private function assertHasUserWithGrantPermission(string $resourceId, array $resourcePermissions): void { - foreach ($newPermissions as $permissions) { + foreach ($resourcePermissions as $permissions) { if (in_array(PermissionProvider::PERMISSION_GRANT, $permissions, true)) { return; } diff --git a/model/Command/ChangeAccessCommand.php b/model/Command/ChangeAccessCommand.php index e10c417..04c4931 100644 --- a/model/Command/ChangeAccessCommand.php +++ b/model/Command/ChangeAccessCommand.php @@ -45,9 +45,6 @@ class ChangeAccessCommand */ private array $revokeAccessMap = []; - public function __construct() { - } - public function grantResourceForUser(string $resourceId, string $permission, string $userId): void { $this->giveAccessMap[$resourceId] = $this->giveAccessMap[$resourceId] ?? []; From 3b2828c6f341de63e41abfc891101d609c475df9 Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Fri, 20 Oct 2023 17:00:01 +0300 Subject: [PATCH 35/41] chore: use chunks to save event logs for multiple changes at the same time --- model/event/DacChangedEvent.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/model/event/DacChangedEvent.php b/model/event/DacChangedEvent.php index 146ac53..b564f1d 100644 --- a/model/event/DacChangedEvent.php +++ b/model/event/DacChangedEvent.php @@ -38,9 +38,12 @@ public function __construct(array $added, array $removed) public function getValues(): array { + $added = array_chunk($this->added, 100); + $removed = array_chunk($this->removed, 100); + return array_merge( - $this->enrichWithActions($this->added, 'add'), - $this->enrichWithActions($this->removed, 'remove'), + $this->enrichWithActions($added, 'add'), + $this->enrichWithActions($removed, 'remove'), ); } From 2efa8b53529b78de8e9a3592d3606396634ed006 Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Thu, 26 Oct 2023 14:01:52 +0300 Subject: [PATCH 36/41] chore: add unit test for the new event --- test/unit/model/event/DacChangedEventTest.php | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100755 test/unit/model/event/DacChangedEventTest.php diff --git a/test/unit/model/event/DacChangedEventTest.php b/test/unit/model/event/DacChangedEventTest.php new file mode 100755 index 0000000..5f57dcc --- /dev/null +++ b/test/unit/model/event/DacChangedEventTest.php @@ -0,0 +1,66 @@ +assertInstanceOf(BulkEvent::class, new DacChangedEvent([], [])); + } + + public function testGetValues(): void + { + $added = [ + ['value' => 'a'], + ['value' => 'b'], + ['value' => 'c'], + ]; + $removed = [ + ['value' => 'd'], + ['value' => 'e'], + ['value' => 'f'], + ]; + $sut = new DacChangedEvent($added, $removed); + $expected = [ + [ + 'action' => 'add', + ['value' => 'a'], + ['value' => 'b'], + ['value' => 'c'] + ], + [ + 'action' => 'remove', + ['value' => 'd'], + ['value' => 'e'], + ['value' => 'f'], + ] + ]; + + $this->assertEquals($expected, $sut->getValues()); + } +} From 888d4f95bf8520fee47dd5be2ca5f24a40a294ab Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Thu, 26 Oct 2023 17:47:37 +0300 Subject: [PATCH 37/41] chore: check for uniques for userRevokedPermissions variable, change syntax to PHP 7.4 --- model/Command/ChangeAccessCommand.php | 43 +++++++++++++-------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/model/Command/ChangeAccessCommand.php b/model/Command/ChangeAccessCommand.php index 04c4931..c565017 100644 --- a/model/Command/ChangeAccessCommand.php +++ b/model/Command/ChangeAccessCommand.php @@ -47,14 +47,12 @@ class ChangeAccessCommand public function grantResourceForUser(string $resourceId, string $permission, string $userId): void { - $this->giveAccessMap[$resourceId] = $this->giveAccessMap[$resourceId] ?? []; - $this->giveAccessMap[$resourceId][$permission] = $this->giveAccessMap[$resourceId][$permission] ?? []; - $this->giveAccessMap[$resourceId][$permission] = array_unique( - array_merge( - $this->giveAccessMap[$resourceId][$permission], - [$userId] - ) - ); + $this->giveAccessMap[$resourceId] ??= []; + $this->giveAccessMap[$resourceId][$permission] ??= []; + + if (!in_array($userId, $this->giveAccessMap[$resourceId][$permission], true)) { + $this->giveAccessMap[$resourceId][$permission][] = $userId; + } } public function getResourceIdsToGrant(): array @@ -69,23 +67,24 @@ public function getUserIdsToGrant(string $resourceId, string $permission): array public function revokeResourceForUser(string $resourceId, string $permission, string $userId): void { - $this->revokeAccessMap[$resourceId] = $this->revokeAccessMap[$resourceId] ?? []; - $this->revokeAccessMap[$resourceId][$permission] = $this->revokeAccessMap[$resourceId][$permission] ?? []; - $this->revokeAccessMap[$resourceId][$permission] = array_unique( - array_merge( - $this->revokeAccessMap[$resourceId][$permission], - [$userId] - ) - ); - - $this->userRevokedPermissions[$userId] = $this->userRevokedPermissions[$userId] ?? []; - $this->userRevokedPermissions[$userId][$permission] = $this->userRevokedPermissions[$userId][$permission] ?? []; - $this->userRevokedPermissions[$userId][$permission][] = $resourceId; + $this->revokeAccessMap[$resourceId] ??= []; + $this->revokeAccessMap[$resourceId][$permission] ??= []; + + if (!in_array($userId, $this->revokeAccessMap[$resourceId][$permission], true)) { + $this->revokeAccessMap[$resourceId][$permission][] = $userId; + } + + $this->userRevokedPermissions[$userId] ??= []; + $this->userRevokedPermissions[$userId][$permission] ??= []; + + if (!in_array($resourceId, $this->userRevokedPermissions[$userId][$permission], true)) { + $this->userRevokedPermissions[$userId][$permission][] = $resourceId; + } } public function removeRevokeResourceForUser(string $resourceId, string $permission, string $userId): void { - $this->userRevokedPermissions[$userId][$permission] = $this->userRevokedPermissions[$userId][$permission] ?? []; + $this->userRevokedPermissions[$userId][$permission] ??= []; $key = array_search($resourceId, $this->userRevokedPermissions[$userId][$permission]); @@ -93,7 +92,7 @@ public function removeRevokeResourceForUser(string $resourceId, string $permissi unset($this->userRevokedPermissions[$userId][$permission][$key]); } - $this->revokeAccessMap[$resourceId][$permission] = $this->revokeAccessMap[$resourceId][$permission] ?? []; + $this->revokeAccessMap[$resourceId][$permission] ??= []; $key = array_search($userId, $this->revokeAccessMap[$resourceId][$permission]); From 822ebb3e81ee779131ef85ebe8362bc536494e6f Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Thu, 2 Nov 2023 14:14:18 +0300 Subject: [PATCH 38/41] test: add/update unit tests --- .../model/ChangePermissionsServiceTest.php | 98 +++++++++++++++++-- .../model/SyncPermissionsStrategyTest.php | 3 +- test/unit/model/event/DacChangedEventTest.php | 83 +++++++++++----- .../ResourceUpdateHandlerTest.php | 40 +++++--- 4 files changed, 174 insertions(+), 50 deletions(-) diff --git a/test/unit/model/ChangePermissionsServiceTest.php b/test/unit/model/ChangePermissionsServiceTest.php index 9526679..9ecfe94 100644 --- a/test/unit/model/ChangePermissionsServiceTest.php +++ b/test/unit/model/ChangePermissionsServiceTest.php @@ -23,8 +23,13 @@ namespace oat\taoDacSimple\test\unit\model; use oat\oatbox\event\EventManager; +use oat\tao\model\event\DataAccessControlChangedEvent; use oat\taoDacSimple\model\ChangePermissionsService; +use oat\taoDacSimple\model\Command\ChangeAccessCommand; +use oat\taoDacSimple\model\Command\ChangePermissionsCommand; use oat\taoDacSimple\model\DataBaseAccess; +use oat\taoDacSimple\model\event\DacAffectedUsersEvent; +use oat\taoDacSimple\model\event\DacRootChangedEvent; use oat\taoDacSimple\model\PermissionsStrategyInterface; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -39,26 +44,101 @@ class ChangePermissionsServiceTest extends TestCase private $strategy; /** @var MockObject|EventManager */ private $eventManager; - private ChangePermissionsService $service; + private ChangePermissionsService $sut; protected function setUp(): void { $this->eventManager = $this->createMock(EventManager::class); $this->databaseAccess = $this->createMock(DataBaseAccess::class); $this->strategy = $this->createMock(PermissionsStrategyInterface::class); - $this->service = new ChangePermissionsService( - $this->databaseAccess, - $this->strategy, - $this->eventManager - ); - $this->service->setLogger($this->createMock(LoggerInterface::class)); + $this->sut = new ChangePermissionsService($this->databaseAccess, $this->strategy, $this->eventManager); } public function testChange(): void { - //@TODO Complete test before release this code + $root = $this->createMock(\core_kernel_classes_Resource::class); + $root + ->expects($this->never()) + ->method('getNestedResources'); + $root + ->method('getUri') + ->willReturn('rootUri'); + $root + ->method('isClass') + ->willReturn(true); - $this->markTestIncomplete(); + $privilegesPerUser = [ + 'user1' => ['READ', 'WRITE', 'GRANT'], + 'user2' => ['READ', 'WRITE'], + ]; + + $changePermissionsCommand = $this->createMock(ChangePermissionsCommand::class); + $changePermissionsCommand + ->method('getRoot') + ->willReturn($root); + $changePermissionsCommand + ->method('isRecursive') + ->willReturn(false); + $changePermissionsCommand + ->method('getPrivilegesPerUser') + ->willReturn($privilegesPerUser); + + $rootPermissions = [ + 'user1' => ['READ'], + 'user2' => ['READ', 'WRITE', 'GRANT'], + ]; + + $changeAccessCommand = new ChangeAccessCommand(); + $changeAccessCommand->revokeResourceForUser('rootUri', 'GRANT', 'user2'); + $changeAccessCommand->grantResourceForUser('rootUri', 'WRITE', 'user1'); + $changeAccessCommand->grantResourceForUser('rootUri', 'GRANT', 'user1'); + + $this->databaseAccess + ->expects($this->once()) + ->method('getResourcesPermissions') + ->with(['rootUri']) + ->willReturn([ + 'rootUri' => $rootPermissions, + ]); + $this->databaseAccess + ->expects($this->once()) + ->method('changeAccess') + ->with($changeAccessCommand); + + $permissionsDelta = [ + 'remove' => [ + 'user2' => ['GRANT'], + ], + 'add' => [ + 'user1' => ['WRITE', 'GRANT'], + ], + ]; + + $this->strategy + ->expects($this->once()) + ->method('normalizeRequest') + ->with($rootPermissions, $privilegesPerUser) + ->willReturn($permissionsDelta); + $this->strategy + ->expects($this->once()) + ->method('getPermissionsToRemove') + ->with($rootPermissions, $permissionsDelta) + ->willReturn(['user2' => ['GRANT']]); + $this->strategy + ->expects($this->once()) + ->method('getPermissionsToAdd') + ->with($rootPermissions, $permissionsDelta) + ->willReturn(['user1' => ['WRITE', 'GRANT']]); + + $this->eventManager + ->method('trigger') + ->withConsecutive( + [new DacRootChangedEvent($root, $permissionsDelta)], + [new DataAccessControlChangedEvent('rootUri', $permissionsDelta, false)], + [new DacAffectedUsersEvent(['user1'], ['user2'])], + ); + + $this->sut->change($changePermissionsCommand); } } diff --git a/test/unit/model/SyncPermissionsStrategyTest.php b/test/unit/model/SyncPermissionsStrategyTest.php index 89d4404..96549f1 100644 --- a/test/unit/model/SyncPermissionsStrategyTest.php +++ b/test/unit/model/SyncPermissionsStrategyTest.php @@ -47,7 +47,8 @@ public function testNormalizeRequest(): void 'p1' => ['READ', 'WRITE'], 'p2' => ['READ'], 'p3' => ['READ', 'WRITE'], - ] + ], + 'remove' => [], ], $result ); diff --git a/test/unit/model/event/DacChangedEventTest.php b/test/unit/model/event/DacChangedEventTest.php index 5f57dcc..f776864 100755 --- a/test/unit/model/event/DacChangedEventTest.php +++ b/test/unit/model/event/DacChangedEventTest.php @@ -22,45 +22,76 @@ namespace oat\taoDacSimple\test\unit\model\event; +use JsonSerializable; use oat\oatbox\event\BulkEvent; +use oat\oatbox\event\Event; use oat\taoDacSimple\model\event\DacChangedEvent; use PHPUnit\Framework\TestCase; class DacChangedEventTest extends TestCase { - public function testIsBulkEvent(): void + private array $added; + private array $removed; + private DacChangedEvent $sut; + + protected function setUp(): void + { + $this->added = [ + ['resource' => 'resource1', 'user' => 'user1', 'permissions' => ['READ', 'WRITE', 'GRANT']], + ['resource' => 'resource2', 'user' => 'user2', 'permissions' => ['READ', 'WRITE']], + ['resource' => 'resource2', 'user' => 'user3', 'permissions' => ['READ']], + ]; + $this->removed = [ + ['resource' => 'resource1', 'user' => 'user4', 'permissions' => ['GRANT']], + ['resource' => 'resource1', 'user' => 'user5', 'permissions' => ['WRITE', 'GRANT']], + ['resource' => 'resource3', 'user' => 'user6', 'permissions' => ['READ']], + ]; + + $this->sut = new DacChangedEvent($this->added, $this->removed); + } + + public function testIsInstanceOfEvent(): void + { + $this->assertInstanceOf(Event::class, $this->sut); + } + + public function testGetName(): void + { + $this->assertEquals(DacChangedEvent::class, $this->sut->getName()); + } + + public function testIsInstanceOfBulkEvent(): void { - $this->assertInstanceOf(BulkEvent::class, new DacChangedEvent([], [])); + $this->assertInstanceOf(BulkEvent::class, $this->sut); } public function testGetValues(): void { - $added = [ - ['value' => 'a'], - ['value' => 'b'], - ['value' => 'c'], - ]; - $removed = [ - ['value' => 'd'], - ['value' => 'e'], - ['value' => 'f'], - ]; - $sut = new DacChangedEvent($added, $removed); - $expected = [ + $this->assertEquals( [ - 'action' => 'add', - ['value' => 'a'], - ['value' => 'b'], - ['value' => 'c'] + [ + 'action' => 'add', + ...$this->added, + ], + [ + 'action' => 'remove', + ...$this->removed, + ] ], - [ - 'action' => 'remove', - ['value' => 'd'], - ['value' => 'e'], - ['value' => 'f'], - ] - ]; + $this->sut->getValues() + ); + } - $this->assertEquals($expected, $sut->getValues()); + public function testInstanceOfJsonSerializable(): void + { + $this->assertInstanceOf(JsonSerializable::class, $this->sut); + } + + public function testJsonSerialize(): void + { + $this->assertEquals( + ['added' => $this->added, 'removed' => $this->removed], + $this->sut->jsonSerialize() + ); } } diff --git a/test/unit/model/eventHandler/ResourceUpdateHandlerTest.php b/test/unit/model/eventHandler/ResourceUpdateHandlerTest.php index 2d00f83..eac497a 100644 --- a/test/unit/model/eventHandler/ResourceUpdateHandlerTest.php +++ b/test/unit/model/eventHandler/ResourceUpdateHandlerTest.php @@ -27,6 +27,8 @@ use oat\generis\model\data\Ontology; use oat\generis\test\MockObject; use oat\generis\test\ServiceManagerMockTrait; +use oat\taoDacSimple\model\ChangePermissionsService; +use oat\taoDacSimple\model\Command\ChangePermissionsCommand; use PHPUnit\Framework\TestCase; use oat\tao\model\event\ResourceMovedEvent; use oat\taoDacSimple\model\eventHandler\ResourceUpdateHandler; @@ -51,29 +53,36 @@ class ResourceUpdateHandlerTest extends TestCase /** @var ResourceUpdateHandler */ private $subject; + /** + * @var ChangePermissionsService|MockObject + */ + private $changePermissionsServiceMock; + public function setUp(): void { $this->rolePrivilegeRetriever = $this->createMock(RolePrivilegeRetriever::class); - $this->eventMock = $this->createMock(ResourceMovedEvent::class); $this->resourceMock = $this->createMock(core_kernel_classes_Resource::class); - $this->classMock = $this->createMock(core_kernel_classes_Class::class); $this->ontoloigyModelMock = $this->createMock(Ontology::class); + $this->classMock = $this->createMock(core_kernel_classes_Class::class); + $this->classMock + ->method('getUri') + ->willReturn('destinationClassUri'); + + $this->eventMock = $this->createMock(ResourceMovedEvent::class); $this->eventMock ->method('getDestinationClass') ->willReturn($this->classMock); - $this->classMock - ->method('getUri') - ->willReturn('destinationClassUri'); + $this->changePermissionsServiceMock = $this->createMock(ChangePermissionsService::class); $this->subject = new ResourceUpdateHandler(); - $this->subject->setModel($this->ontoloigyModelMock); $this->subject->setServiceLocator( $this->getServiceManagerMock( [ RolePrivilegeRetriever::class => $this->rolePrivilegeRetriever, + ChangePermissionsService::class => $this->changePermissionsServiceMock, ] ) ); @@ -110,18 +119,21 @@ public function testCatchResourceUpdated() ] ); - $this->permissionsServiceMock + $this->changePermissionsServiceMock ->expects($this->once()) ->method('change') ->with( - $this->eventMock->getMovedResource(), - [ - 'http://www.tao.lu/Ontologies/TAO.rdf#BackOfficeRole' => [ - 'GRANT', - 'READ', - 'WRITE' + new ChangePermissionsCommand( + $this->eventMock->getMovedResource(), + [ + 'http://www.tao.lu/Ontologies/TAO.rdf#BackOfficeRole' => [ + 'GRANT', + 'READ', + 'WRITE' + ], ], - ] + true + ) ); $this->subject->catchResourceUpdated($this->eventMock); From f4208c9abfd8f475b3eb6939ab60f292f61203ac Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Thu, 2 Nov 2023 15:34:09 +0300 Subject: [PATCH 39/41] refactor!: remove unnecessary events --- model/event/DacRootAddedEvent.php | 33 ----------------------------- model/event/DacRootRemovedEvent.php | 33 ----------------------------- 2 files changed, 66 deletions(-) delete mode 100644 model/event/DacRootAddedEvent.php delete mode 100644 model/event/DacRootRemovedEvent.php diff --git a/model/event/DacRootAddedEvent.php b/model/event/DacRootAddedEvent.php deleted file mode 100644 index 4ce7f3e..0000000 --- a/model/event/DacRootAddedEvent.php +++ /dev/null @@ -1,33 +0,0 @@ - Date: Thu, 2 Nov 2023 16:56:20 +0300 Subject: [PATCH 40/41] chore: update generis and tao-core versions --- composer.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 5d1fe64..74bdef7 100644 --- a/composer.json +++ b/composer.json @@ -21,8 +21,8 @@ "minimum-stability": "dev", "require": { "oat-sa/oatbox-extension-installer": "~1.1||dev-master", - "oat-sa/generis": "dev-feature/adf-1590/optimize-permissions-task-queue as 99.99", - "oat-sa/tao-core": "dev-feature/adf-1590/optimize-permissions-task-queue as 99.99", + "oat-sa/generis": ">=15.32.0", + "oat-sa/tao-core": ">=53.11.4", "oat-sa/extension-tao-backoffice": ">=6.0.0", "oat-sa/extension-tao-item": ">=11.3" }, From 2ad9640bc8211b3ecf2e65eb26471324d65fc9f2 Mon Sep 17 00:00:00 2001 From: Andrei Shapiro Date: Thu, 2 Nov 2023 17:02:00 +0300 Subject: [PATCH 41/41] chore: fix CS --- model/ChangePermissionsService.php | 334 +++++++++--------- model/eventHandler/ResourceUpdateHandler.php | 8 +- .../model/Command/ChangeAccessCommandTest.php | 30 +- test/unit/model/DataBaseAccessTest.php | 5 - test/unit/model/event/DacChangedEventTest.php | 194 +++++----- 5 files changed, 295 insertions(+), 276 deletions(-) diff --git a/model/ChangePermissionsService.php b/model/ChangePermissionsService.php index d58183a..3bf7556 100644 --- a/model/ChangePermissionsService.php +++ b/model/ChangePermissionsService.php @@ -1,167 +1,167 @@ -dataBaseAccess = $dataBaseAccess; - $this->strategy = $strategy; - $this->eventManager = $eventManager; - } - - public function change(ChangePermissionsCommand $command): void - { - $resources = $this->getResourceToUpdate($command->getRoot(), $command->isRecursive()); - - $permissions = $this->dataBaseAccess->getResourcesPermissions(array_column($resources, 'id')); - $rootPermissions = $permissions[$command->getRoot()->getUri()]; - $permissionsDelta = $this->strategy->normalizeRequest($rootPermissions, $command->getPrivilegesPerUser()); - - if (empty($permissionsDelta['remove']) && empty($permissionsDelta['add'])) { - return; - } - - $changeAccessCommand = $this->calculateChanges($resources, $permissionsDelta, $permissions); - $this->dataBaseAccess->changeAccess($changeAccessCommand); - - $this->triggerEvents($command->getRoot(), $permissionsDelta, $command->isRecursive()); - } - - private function getResourceToUpdate(core_kernel_classes_Resource $resource, bool $isRecursive): array - { - if ($isRecursive) { - $resources = []; - - foreach ($resource->getNestedResources() as $result) { - $resources[$result['id']] = $result; - } - - return $resources; - } - - return [ - $resource->getUri() => [ - 'id' => $resource->getUri(), - 'isClass' => $resource->isClass(), - 'level' => 1, - ], - ]; - } - - private function calculateChanges( - array $resources, - array $permissionsDelta, - array $currentPermissions - ): ChangeAccessCommand { - $command = new ChangeAccessCommand(); - - foreach ($resources as $resource) { - $resourcePermissions = $currentPermissions[$resource['id']]; - - $remove = $this->strategy->getPermissionsToRemove($resourcePermissions, $permissionsDelta); - $add = $this->strategy->getPermissionsToAdd($resourcePermissions, $permissionsDelta); - - foreach ($remove as $userId => $permissions) { - $resourcePermissions[$userId] = array_diff($resourcePermissions[$userId] ?? [], $permissions); - - foreach ($permissions as $permission) { - $command->revokeResourceForUser($resource['id'], $permission, $userId); - } - } - - foreach ($add as $userId => $permissions) { - $resourcePermissions[$userId] = array_merge($resourcePermissions[$userId] ?? [], $permissions); - - foreach ($permissions as $permission) { - $command->grantResourceForUser($resource['id'], $permission, $userId); - } - } - - $this->assertHasUserWithGrantPermission($resource['id'], $resourcePermissions); - } - - return $command; - } - - /** - * Checks if all resources after all actions are applied will have at least - * one user with GRANT permission. - */ - private function assertHasUserWithGrantPermission(string $resourceId, array $resourcePermissions): void - { - foreach ($resourcePermissions as $permissions) { - if (in_array(PermissionProvider::PERMISSION_GRANT, $permissions, true)) { - return; - } - } - - throw new PermissionsServiceException( - sprintf( - 'Resource %s should have at least one user with GRANT access', - $resourceId - ) - ); - } - - private function triggerEvents( - core_kernel_classes_Resource $resource, - array $permissionsDelta, - bool $isRecursive - ): void { - if (!empty($permissionsDelta['add']) || !empty($permissionsDelta['remove'])) { - $this->eventManager->trigger(new DacRootChangedEvent($resource, $permissionsDelta)); - } - - $this->eventManager->trigger( - new DataAccessControlChangedEvent( - $resource->getUri(), - $permissionsDelta, - $isRecursive - ) - ); - - $this->eventManager->trigger( - new DacAffectedUsersEvent( - array_keys($permissionsDelta['add']), - array_keys($permissionsDelta['remove']) - ) - ); - } -} +dataBaseAccess = $dataBaseAccess; + $this->strategy = $strategy; + $this->eventManager = $eventManager; + } + + public function change(ChangePermissionsCommand $command): void + { + $resources = $this->getResourceToUpdate($command->getRoot(), $command->isRecursive()); + + $permissions = $this->dataBaseAccess->getResourcesPermissions(array_column($resources, 'id')); + $rootPermissions = $permissions[$command->getRoot()->getUri()]; + $permissionsDelta = $this->strategy->normalizeRequest($rootPermissions, $command->getPrivilegesPerUser()); + + if (empty($permissionsDelta['remove']) && empty($permissionsDelta['add'])) { + return; + } + + $changeAccessCommand = $this->calculateChanges($resources, $permissionsDelta, $permissions); + $this->dataBaseAccess->changeAccess($changeAccessCommand); + + $this->triggerEvents($command->getRoot(), $permissionsDelta, $command->isRecursive()); + } + + private function getResourceToUpdate(core_kernel_classes_Resource $resource, bool $isRecursive): array + { + if ($isRecursive) { + $resources = []; + + foreach ($resource->getNestedResources() as $result) { + $resources[$result['id']] = $result; + } + + return $resources; + } + + return [ + $resource->getUri() => [ + 'id' => $resource->getUri(), + 'isClass' => $resource->isClass(), + 'level' => 1, + ], + ]; + } + + private function calculateChanges( + array $resources, + array $permissionsDelta, + array $currentPermissions + ): ChangeAccessCommand { + $command = new ChangeAccessCommand(); + + foreach ($resources as $resource) { + $resourcePermissions = $currentPermissions[$resource['id']]; + + $remove = $this->strategy->getPermissionsToRemove($resourcePermissions, $permissionsDelta); + $add = $this->strategy->getPermissionsToAdd($resourcePermissions, $permissionsDelta); + + foreach ($remove as $userId => $permissions) { + $resourcePermissions[$userId] = array_diff($resourcePermissions[$userId] ?? [], $permissions); + + foreach ($permissions as $permission) { + $command->revokeResourceForUser($resource['id'], $permission, $userId); + } + } + + foreach ($add as $userId => $permissions) { + $resourcePermissions[$userId] = array_merge($resourcePermissions[$userId] ?? [], $permissions); + + foreach ($permissions as $permission) { + $command->grantResourceForUser($resource['id'], $permission, $userId); + } + } + + $this->assertHasUserWithGrantPermission($resource['id'], $resourcePermissions); + } + + return $command; + } + + /** + * Checks if all resources after all actions are applied will have at least + * one user with GRANT permission. + */ + private function assertHasUserWithGrantPermission(string $resourceId, array $resourcePermissions): void + { + foreach ($resourcePermissions as $permissions) { + if (in_array(PermissionProvider::PERMISSION_GRANT, $permissions, true)) { + return; + } + } + + throw new PermissionsServiceException( + sprintf( + 'Resource %s should have at least one user with GRANT access', + $resourceId + ) + ); + } + + private function triggerEvents( + core_kernel_classes_Resource $resource, + array $permissionsDelta, + bool $isRecursive + ): void { + if (!empty($permissionsDelta['add']) || !empty($permissionsDelta['remove'])) { + $this->eventManager->trigger(new DacRootChangedEvent($resource, $permissionsDelta)); + } + + $this->eventManager->trigger( + new DataAccessControlChangedEvent( + $resource->getUri(), + $permissionsDelta, + $isRecursive + ) + ); + + $this->eventManager->trigger( + new DacAffectedUsersEvent( + array_keys($permissionsDelta['add']), + array_keys($permissionsDelta['remove']) + ) + ); + } +} diff --git a/model/eventHandler/ResourceUpdateHandler.php b/model/eventHandler/ResourceUpdateHandler.php index 4b7ef72..3f8e373 100644 --- a/model/eventHandler/ResourceUpdateHandler.php +++ b/model/eventHandler/ResourceUpdateHandler.php @@ -57,7 +57,13 @@ public function catchResourceUpdated(ResourceMovedEvent $event): void if (isset($itemPrivilegesMap)) { foreach ($itemPrivilegesMap as $uri => $itemPrivilege) { - $permissionService->change(new ChangePermissionsCommand($this->getResource($uri), $itemPrivilege, true)); + $permissionService->change( + new ChangePermissionsCommand( + $this->getResource($uri), + $itemPrivilege, + true + ) + ); } } } diff --git a/test/unit/model/Command/ChangeAccessCommandTest.php b/test/unit/model/Command/ChangeAccessCommandTest.php index 29f8c66..305977f 100644 --- a/test/unit/model/Command/ChangeAccessCommandTest.php +++ b/test/unit/model/Command/ChangeAccessCommandTest.php @@ -79,13 +79,31 @@ public function testRevokePermissions(): void $this->assertSame(['r1', 'r2'], $this->sut->getResourceIdsToRevoke()); $this->assertSame(['u1', 'u2'], $this->sut->getUserIdsToRevokePermissions()); - $this->assertSame(['r1', 'r2'], $this->sut->getResourceIdsByUserAndPermissionToRevoke('u1', PermissionProvider::PERMISSION_READ)); - $this->assertSame(['r1', 'r2'], $this->sut->getResourceIdsByUserAndPermissionToRevoke('u1', PermissionProvider::PERMISSION_WRITE)); - $this->assertSame(['r1', 'r2'], $this->sut->getResourceIdsByUserAndPermissionToRevoke('u1', PermissionProvider::PERMISSION_GRANT)); + $this->assertSame( + ['r1', 'r2'], + $this->sut->getResourceIdsByUserAndPermissionToRevoke('u1', PermissionProvider::PERMISSION_READ) + ); + $this->assertSame( + ['r1', 'r2'], + $this->sut->getResourceIdsByUserAndPermissionToRevoke('u1', PermissionProvider::PERMISSION_WRITE) + ); + $this->assertSame( + ['r1', 'r2'], + $this->sut->getResourceIdsByUserAndPermissionToRevoke('u1', PermissionProvider::PERMISSION_GRANT) + ); - $this->assertSame(['r2'], $this->sut->getResourceIdsByUserAndPermissionToRevoke('u2', PermissionProvider::PERMISSION_READ)); - $this->assertSame(['r2'], $this->sut->getResourceIdsByUserAndPermissionToRevoke('u2', PermissionProvider::PERMISSION_WRITE)); - $this->assertSame([], $this->sut->getResourceIdsByUserAndPermissionToRevoke('u2', PermissionProvider::PERMISSION_GRANT)); + $this->assertSame( + ['r2'], + $this->sut->getResourceIdsByUserAndPermissionToRevoke('u2', PermissionProvider::PERMISSION_READ) + ); + $this->assertSame( + ['r2'], + $this->sut->getResourceIdsByUserAndPermissionToRevoke('u2', PermissionProvider::PERMISSION_WRITE) + ); + $this->assertSame( + [], + $this->sut->getResourceIdsByUserAndPermissionToRevoke('u2', PermissionProvider::PERMISSION_GRANT) + ); $this->assertSame( [ diff --git a/test/unit/model/DataBaseAccessTest.php b/test/unit/model/DataBaseAccessTest.php index 6358922..f7d223d 100644 --- a/test/unit/model/DataBaseAccessTest.php +++ b/test/unit/model/DataBaseAccessTest.php @@ -222,11 +222,6 @@ private function getResourceMock(string $id): core_kernel_classes_Resource return $resourceMock; } - - public function sdsadsadsad() - { - - } } /** diff --git a/test/unit/model/event/DacChangedEventTest.php b/test/unit/model/event/DacChangedEventTest.php index f776864..d701135 100755 --- a/test/unit/model/event/DacChangedEventTest.php +++ b/test/unit/model/event/DacChangedEventTest.php @@ -1,97 +1,97 @@ -added = [ - ['resource' => 'resource1', 'user' => 'user1', 'permissions' => ['READ', 'WRITE', 'GRANT']], - ['resource' => 'resource2', 'user' => 'user2', 'permissions' => ['READ', 'WRITE']], - ['resource' => 'resource2', 'user' => 'user3', 'permissions' => ['READ']], - ]; - $this->removed = [ - ['resource' => 'resource1', 'user' => 'user4', 'permissions' => ['GRANT']], - ['resource' => 'resource1', 'user' => 'user5', 'permissions' => ['WRITE', 'GRANT']], - ['resource' => 'resource3', 'user' => 'user6', 'permissions' => ['READ']], - ]; - - $this->sut = new DacChangedEvent($this->added, $this->removed); - } - - public function testIsInstanceOfEvent(): void - { - $this->assertInstanceOf(Event::class, $this->sut); - } - - public function testGetName(): void - { - $this->assertEquals(DacChangedEvent::class, $this->sut->getName()); - } - - public function testIsInstanceOfBulkEvent(): void - { - $this->assertInstanceOf(BulkEvent::class, $this->sut); - } - - public function testGetValues(): void - { - $this->assertEquals( - [ - [ - 'action' => 'add', - ...$this->added, - ], - [ - 'action' => 'remove', - ...$this->removed, - ] - ], - $this->sut->getValues() - ); - } - - public function testInstanceOfJsonSerializable(): void - { - $this->assertInstanceOf(JsonSerializable::class, $this->sut); - } - - public function testJsonSerialize(): void - { - $this->assertEquals( - ['added' => $this->added, 'removed' => $this->removed], - $this->sut->jsonSerialize() - ); - } -} +added = [ + ['resource' => 'resource1', 'user' => 'user1', 'permissions' => ['READ', 'WRITE', 'GRANT']], + ['resource' => 'resource2', 'user' => 'user2', 'permissions' => ['READ', 'WRITE']], + ['resource' => 'resource2', 'user' => 'user3', 'permissions' => ['READ']], + ]; + $this->removed = [ + ['resource' => 'resource1', 'user' => 'user4', 'permissions' => ['GRANT']], + ['resource' => 'resource1', 'user' => 'user5', 'permissions' => ['WRITE', 'GRANT']], + ['resource' => 'resource3', 'user' => 'user6', 'permissions' => ['READ']], + ]; + + $this->sut = new DacChangedEvent($this->added, $this->removed); + } + + public function testIsInstanceOfEvent(): void + { + $this->assertInstanceOf(Event::class, $this->sut); + } + + public function testGetName(): void + { + $this->assertEquals(DacChangedEvent::class, $this->sut->getName()); + } + + public function testIsInstanceOfBulkEvent(): void + { + $this->assertInstanceOf(BulkEvent::class, $this->sut); + } + + public function testGetValues(): void + { + $this->assertEquals( + [ + [ + 'action' => 'add', + ...$this->added, + ], + [ + 'action' => 'remove', + ...$this->removed, + ] + ], + $this->sut->getValues() + ); + } + + public function testInstanceOfJsonSerializable(): void + { + $this->assertInstanceOf(JsonSerializable::class, $this->sut); + } + + public function testJsonSerialize(): void + { + $this->assertEquals( + ['added' => $this->added, 'removed' => $this->removed], + $this->sut->jsonSerialize() + ); + } +}