diff --git a/README.md b/README.md index 08e2ea3c..3330268b 100644 --- a/README.md +++ b/README.md @@ -34,6 +34,8 @@ Workflows can be chained. For example, if the approved tag of a workflow A is us then once a file is approved by the workflow A, it becomes pending for the B one. For an example of a chain involving a leave request that needs to be approved by a manager and then the department head, there is a screenshot below. +**Notice**: If the approvers are a group the file may be shared with the individual users of the group instead of the group. + ![Workflow chain](https://github.com/nextcloud/approval/raw/main/img/screenshot_chained.jpg) ## Tag assignment diff --git a/lib/Service/ApprovalService.php b/lib/Service/ApprovalService.php index a7ed108a..33f4e423 100644 --- a/lib/Service/ApprovalService.php +++ b/lib/Service/ApprovalService.php @@ -13,7 +13,8 @@ use OCA\Approval\Exceptions\OutdatedEtagException; use OCA\DAV\Connector\Sabre\Node as SabreNode; use OCP\App\IAppManager; -use OCP\Files\File; +use OCP\Files\Config\ICachedMountFileInfo; +use OCP\Files\Config\IUserMountCache; use OCP\Files\FileInfo; use OCP\Files\IRootFolder; use OCP\Files\Node; @@ -50,6 +51,7 @@ public function __construct( private IL10N $l10n, private LoggerInterface $logger, private ?string $userId, + private IUserMountCache $userMountCache, ) { } @@ -527,17 +529,16 @@ public function requestViaTagAssignment(int $fileId, int $ruleId, string $reques * @param int $fileId * @param Rule $rule * @param string $userId - * @return array list of created shares + * @return void * @throws \OCP\Files\NotPermittedException * @throws \OC\User\NoUserException */ - private function shareWithApprovers(int $fileId, array $rule, string $userId): array { - $createdShares = []; + private function shareWithApprovers(int $fileId, array $rule, string $userId): void { // get node $userFolder = $this->root->getUserFolder($userId); $node = $userFolder->getFirstNodeById($fileId); if ($node === null) { - return []; + return; } // get the node again from the owner's storage to avoid sharing permission issues $ownerId = $node->getOwner()->getUID(); @@ -549,19 +550,36 @@ private function shareWithApprovers(int $fileId, array $rule, string $userId): a $label = $this->l10n->t('Please check my approval request'); $fileOwner = $node->getOwner()->getUID(); + // Gets all users that have access to the file + $mounts = $this->userMountCache->getMountsForFileId($fileId); + $userIdsWithAccess = array_unique(array_map(static fn (ICachedMountFileInfo $fileInfo) => $fileInfo->getUser()->getUID(), $mounts)); + foreach ($rule['approvers'] as $approver) { - if ($approver['type'] === 'user' && !$this->utilsService->userHasAccessTo($fileId, $approver['entityId'])) { + if ($approver['type'] === 'user' && !in_array($approver['entityId'], $userIdsWithAccess, true)) { // create user share - if ($this->utilsService->createShare($node, IShare::TYPE_USER, $approver['entityId'], $fileOwner, $label)) { - $createdShares[] = $approver; + if (!$this->utilsService->createShare($node, IShare::TYPE_USER, $approver['entityId'], $fileOwner, $label)) { + $this->logger->warning('Failed to create user share for file {fileId} with approver {approverId}', ['fileId' => $fileId, 'approverId' => $approver['entityId']]); } } } if ($this->shareManager->allowGroupSharing()) { foreach ($rule['approvers'] as $approver) { - if ($approver['type'] === 'group' && !$this->utilsService->groupHasAccessTo($fileOwner, $node, $approver['entityId'])) { - if ($this->utilsService->createShare($node, IShare::TYPE_GROUP, $approver['entityId'], $fileOwner, $label)) { - $createdShares[] = $approver; + if ($approver['type'] === 'group' && $this->groupManager->groupExists($approver['entityId'])) { + $groupMembers = $this->groupManager->get($approver['entityId'])->getUsers(); + $groupMemberIds = array_map(static fn (IUser $user) => $user->getUID(), $groupMembers); + $groupMembersThatNeedAccess = array_diff($groupMemberIds, $userIdsWithAccess); + // Create group share if everyone in the group needs access + if (count($groupMembersThatNeedAccess) === count($groupMemberIds)) { + if (!$this->utilsService->createShare($node, IShare::TYPE_GROUP, $approver['entityId'], $fileOwner, $label)) { + $this->logger->warning('Failed to create group share for file {fileId} with approver {approverId}', ['fileId' => $fileId, 'approverId' => $approver['entityId']]); + } + } elseif (count($groupMembersThatNeedAccess) > 0) { + // Create user shares for each member that needs access + foreach ($groupMembersThatNeedAccess as $groupMemberId) { + if (!$this->utilsService->createShare($node, IShare::TYPE_USER, $groupMemberId, $fileOwner, $label)) { + $this->logger->warning('Failed to create user share for file {fileId} with approver {approverId}', ['fileId' => $fileId, 'approverId' => $groupMemberId]); + } + } } } } @@ -571,14 +589,12 @@ private function shareWithApprovers(int $fileId, array $rule, string $userId): a if ($circlesEnabled) { foreach ($rule['approvers'] as $approver) { if ($approver['type'] === 'circle') { - if ($this->utilsService->createShare($node, IShare::TYPE_CIRCLE, $approver['entityId'], $fileOwner, $label)) { - $createdShares[] = $approver; + if (!$this->utilsService->createShare($node, IShare::TYPE_CIRCLE, $approver['entityId'], $fileOwner, $label)) { + $this->logger->warning('Failed to create circle share for file {fileId} with approver {approverId}', ['fileId' => $fileId, 'approverId' => $approver['entityId']]); } } } } - - return $createdShares; } /** diff --git a/lib/Service/UtilsService.php b/lib/Service/UtilsService.php index 776e1b34..4ce35e20 100644 --- a/lib/Service/UtilsService.php +++ b/lib/Service/UtilsService.php @@ -14,7 +14,6 @@ use OCP\Constants; use OCP\Files\IRootFolder; use OCP\Files\Node; - use OCP\IUser; use OCP\IUserManager; use OCP\Share\IManager as IShareManager; @@ -135,27 +134,6 @@ public function userHasAccessTo(int $fileId, ?string $userId): bool { return false; } - /** - * Return false if this folder and no parents are shared with that group - * - * @param string $userId - * @param Node $fileNode - * @param string|null $groupId - * @return bool - */ - public function groupHasAccessTo(string $userId, Node $fileNode, ?string $groupId): bool { - do { - $groupShares = $this->shareManager->getSharesBy($userId, ISHARE::TYPE_GROUP, $fileNode); - foreach ($groupShares as $groupShare) { - if ($groupShare->getSharedWith() === $groupId) { - return true; - } - } - $fileNode = $fileNode->getParent(); - } while ($fileNode->getParentId() !== -1); - return false; - } - /** * @param string $name of the new tag * @return array diff --git a/tests/unit/Service/ApprovalServiceTest.php b/tests/unit/Service/ApprovalServiceTest.php index 8c539c85..068f0642 100644 --- a/tests/unit/Service/ApprovalServiceTest.php +++ b/tests/unit/Service/ApprovalServiceTest.php @@ -12,6 +12,7 @@ use OCA\Approval\Activity\ActivityManager; use OCA\Approval\AppInfo\Application; use OCP\App\IAppManager; +use OCP\Files\Config\IUserMountCache; use OCP\Files\IRootFolder; use OCP\ICacheFactory; use OCP\IDBConnection; @@ -98,7 +99,8 @@ protected function setUp(): void { $c->get(IShareManager::class), $c->get(IL10N::class), $c->get(LoggerInterface::class), - 'user1' + 'user1', + $c->get(IUserMountCache::class) ); // add some tags