From 782dc0526162409ba8ad72d577959219073905f8 Mon Sep 17 00:00:00 2001 From: Julien Veyssier Date: Wed, 19 Apr 2023 12:05:57 +0200 Subject: [PATCH 1/5] when requesting with a rule having a group as approver, do not share the file with the group if it already has access Signed-off-by: Julien Veyssier # Conflicts: # lib/Service/UtilsService.php --- lib/Service/ApprovalService.php | 2 +- lib/Service/UtilsService.php | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lib/Service/ApprovalService.php b/lib/Service/ApprovalService.php index 9be7374b..16b57ebc 100644 --- a/lib/Service/ApprovalService.php +++ b/lib/Service/ApprovalService.php @@ -532,7 +532,7 @@ private function shareWithApprovers(int $fileId, array $rule, string $userId): a } if ($this->shareManager->allowGroupSharing()) { foreach ($rule['approvers'] as $approver) { - if ($approver['type'] === 'group') { + if ($approver['type'] === 'group' && !$this->utilsService->groupHasAccessTo($fileId, $approver['entityId'])) { if ($this->utilsService->createShare($node, IShare::TYPE_GROUP, $approver['entityId'], $fileOwner, $label)) { $createdShares[] = $approver; } diff --git a/lib/Service/UtilsService.php b/lib/Service/UtilsService.php index cc6092bb..aeb339e3 100644 --- a/lib/Service/UtilsService.php +++ b/lib/Service/UtilsService.php @@ -15,6 +15,8 @@ use OCP\IConfig; use OCP\IUser; +use OCP\IGroup; +use OCP\IGroupManager; use OCP\IUserManager; use OCP\Security\ICrypto; use OCP\Share\IManager as IShareManager; @@ -31,6 +33,7 @@ class UtilsService { public function __construct( string $appName, private IUserManager $userManager, + private IGroupManager $groupManager, private IShareManager $shareManager, private IRootFolder $root, private ISystemTagManager $tagManager, @@ -166,6 +169,26 @@ public function userHasAccessTo(int $fileId, ?string $userId): bool { return false; } + /** + * Return false if at least one member of the group does not have access to the file + * + * @param int $fileId + * @param string|null $groupId + * @return bool + */ + public function groupHasAccessTo(int $fileId, ?string $groupId): bool { + $group = $this->groupManager->get($groupId); + if ($group instanceof IGroup) { + foreach ($group->getUsers() as $groupUser) { + if (!$this->userHasAccessTo($fileId, $groupUser->getUID())) { + return false; + } + } + return true; + } + return false; + } + /** * @param string $name of the new tag * @return array From 3fdf2109c2e47bb7d2dddc78109927d6d7db0048 Mon Sep 17 00:00:00 2001 From: Lukas Schaefer Date: Thu, 12 Jun 2025 13:40:07 -0400 Subject: [PATCH 2/5] Checks for shares first and then checks what members have access Signed-off-by: Lukas Schaefer --- lib/Service/ApprovalService.php | 24 ++++++++-- lib/Service/UtilsService.php | 79 ++++++++++++++++++++++++++++----- 2 files changed, 88 insertions(+), 15 deletions(-) diff --git a/lib/Service/ApprovalService.php b/lib/Service/ApprovalService.php index 16b57ebc..4ce651c7 100644 --- a/lib/Service/ApprovalService.php +++ b/lib/Service/ApprovalService.php @@ -532,9 +532,27 @@ private function shareWithApprovers(int $fileId, array $rule, string $userId): a } if ($this->shareManager->allowGroupSharing()) { foreach ($rule['approvers'] as $approver) { - if ($approver['type'] === 'group' && !$this->utilsService->groupHasAccessTo($fileId, $approver['entityId'])) { - if ($this->utilsService->createShare($node, IShare::TYPE_GROUP, $approver['entityId'], $fileOwner, $label)) { - $createdShares[] = $approver; + if ($approver['type'] === 'group' && !$this->utilsService->groupHasAccessTo($fileOwner, $node, $approver['entityId'])) { + $sharesNeeded = ['groupShare' => true, 'users' => []]; + // Only when the file is shared do you need to find a list of users the document needs to be shared with + if ($this->utilsService->isShared($node)) { + $sharesNeeded = $this->utilsService->usersNeedShare($fileId, $approver['entityId']); + } + if ($sharesNeeded['groupShare'] === true) { + if ($this->utilsService->createShare($node, IShare::TYPE_GROUP, $approver['entityId'], $fileOwner, $label)) { + $createdShares[] = $approver; + } + } elseif ($sharesNeeded['groupShare'] === false) { + // Goes through every user that doesn't have access and creates a share (very expensive) + $success = true; + foreach ($sharesNeeded['users'] as $user) { + if (!$this->utilsService->createShare($node, IShare::TYPE_USER, $user, $fileOwner, $label)) { + $success = false; + } + } + if ($success === true) { + $createdShares[] = $approver; + } } } } diff --git a/lib/Service/UtilsService.php b/lib/Service/UtilsService.php index aeb339e3..7890c785 100644 --- a/lib/Service/UtilsService.php +++ b/lib/Service/UtilsService.php @@ -8,15 +8,18 @@ namespace OCA\Approval\Service; use Exception; +use OC; use OCA\Approval\AppInfo\Application; +use OCA\Circles\CirclesManager; +use OCA\Circles\Exceptions\CircleNotFoundException; use OCP\Constants; use OCP\Files\IRootFolder; use OCP\Files\Node; use OCP\IConfig; -use OCP\IUser; use OCP\IGroup; use OCP\IGroupManager; +use OCP\IUser; use OCP\IUserManager; use OCP\Security\ICrypto; use OCP\Share\IManager as IShareManager; @@ -124,11 +127,11 @@ public function createShare(Node $node, int $type, string $sharedWith, string $s * @return bool */ public function isUserInCircle(string $userId, string $circleId): bool { - $circlesManager = \OC::$server->get(\OCA\Circles\CirclesManager::class); + $circlesManager = OC::$server->get(CirclesManager::class); $circlesManager->startSuperSession(); try { $circle = $circlesManager->getCircle($circleId); - } catch (\OCA\Circles\Exceptions\CircleNotFoundException $e) { + } catch (CircleNotFoundException $e) { $circlesManager->stopSession(); return false; } @@ -170,25 +173,53 @@ public function userHasAccessTo(int $fileId, ?string $userId): bool { } /** - * Return false if at least one member of the group does not have access to the file + * Return false if this folder and no parents are shared with that group * - * @param int $fileId + * @param string $userId + * @param Node $fileNode * @param string|null $groupId * @return bool */ - public function groupHasAccessTo(int $fileId, ?string $groupId): bool { - $group = $this->groupManager->get($groupId); - if ($group instanceof IGroup) { - foreach ($group->getUsers() as $groupUser) { - if (!$this->userHasAccessTo($fileId, $groupUser->getUID())) { - return false; + public function groupHasAccessTo(string $userId, Node $fileNode, ?string $groupId): bool { + $groupShares = $this->shareManager->getSharesBy($userId, ISHARE::TYPE_GROUP, $fileNode); + foreach ($groupShares as $groupShare) { + if ($groupShare->getSharedWith() === $groupId) { + return true; + } + } + $folderNode = $fileNode->getParent(); + while ($folderNode->getParentId() !== -1) { + $groupShares = $this->shareManager->getSharesBy($userId, ISHARE::TYPE_GROUP, $folderNode); + foreach ($groupShares as $groupShare) { + if ($groupShare->getSharedWith() === $groupId) { + return true; } } - return true; + $folderNode = $folderNode->getParent(); } return false; } + /** + * Return true if this folder and no parents are shared with anyone + * + * @param Node $node + * @return bool + */ + public function isShared(Node $node): bool { + $userId = $node->getOwner()->getUID(); + do { + if (!empty($this->shareManager->getSharesBy($userId, ISHARE::TYPE_GROUP, $node))) { + return true; + } + if (!empty($this->shareManager->getSharesBy($userId, ISHARE::TYPE_USER, $node))) { + return true; + } + $node = $node->getParent(); + } while ($node->getParentId() !== -1); + return false; + } + /** * @param string $name of the new tag * @return array @@ -214,4 +245,28 @@ public function deleteTag(int $id): array { return ['error' => 'Tag not found']; } } + + /** + * Find users that need a file to be shared with, so all members of the group have it + * Also says if group share is the correct choice. + * + * @param int $fileId of the file to check what + * @param string|null $groupId the id of the group + * @return array + */ + public function usersNeedShare(int $fileId, ?string $groupId): array { + $groupShare = true; + $users = []; + $group = $this->groupManager->get($groupId); + if ($group instanceof IGroup) { + foreach ($group->getUsers() as $groupUser) { + if ($this->userHasAccessTo($fileId, $groupUser->getUID())) { + $groupShare = false; + } else { + $users[] = $groupUser->getUID(); + } + } + } + return ['groupShare' => $groupShare, 'users' => $users]; + } } From f688e38738be08477ef9e215578da06154b2d14b Mon Sep 17 00:00:00 2001 From: Lukas Schaefer Date: Mon, 16 Jun 2025 14:50:33 -0400 Subject: [PATCH 3/5] filters by shares instead of checking for access Signed-off-by: Lukas Schaefer --- lib/Service/ApprovalService.php | 2 +- lib/Service/UtilsService.php | 61 ++++++++++++++++++++++----------- 2 files changed, 42 insertions(+), 21 deletions(-) diff --git a/lib/Service/ApprovalService.php b/lib/Service/ApprovalService.php index 4ce651c7..f92a7faa 100644 --- a/lib/Service/ApprovalService.php +++ b/lib/Service/ApprovalService.php @@ -536,7 +536,7 @@ private function shareWithApprovers(int $fileId, array $rule, string $userId): a $sharesNeeded = ['groupShare' => true, 'users' => []]; // Only when the file is shared do you need to find a list of users the document needs to be shared with if ($this->utilsService->isShared($node)) { - $sharesNeeded = $this->utilsService->usersNeedShare($fileId, $approver['entityId']); + $sharesNeeded = $this->utilsService->usersNeedShare($node, $approver['entityId']); } if ($sharesNeeded['groupShare'] === true) { if ($this->utilsService->createShare($node, IShare::TYPE_GROUP, $approver['entityId'], $fileOwner, $label)) { diff --git a/lib/Service/UtilsService.php b/lib/Service/UtilsService.php index 7890c785..a617d618 100644 --- a/lib/Service/UtilsService.php +++ b/lib/Service/UtilsService.php @@ -181,22 +181,15 @@ public function userHasAccessTo(int $fileId, ?string $userId): bool { * @return bool */ public function groupHasAccessTo(string $userId, Node $fileNode, ?string $groupId): bool { - $groupShares = $this->shareManager->getSharesBy($userId, ISHARE::TYPE_GROUP, $fileNode); - foreach ($groupShares as $groupShare) { - if ($groupShare->getSharedWith() === $groupId) { - return true; - } - } - $folderNode = $fileNode->getParent(); - while ($folderNode->getParentId() !== -1) { - $groupShares = $this->shareManager->getSharesBy($userId, ISHARE::TYPE_GROUP, $folderNode); + do { + $groupShares = $this->shareManager->getSharesBy($userId, ISHARE::TYPE_GROUP, $fileNode); foreach ($groupShares as $groupShare) { if ($groupShare->getSharedWith() === $groupId) { return true; } } - $folderNode = $folderNode->getParent(); - } + $fileNode = $fileNode->getParent(); + } while ($fileNode->getParentId() !== -1); return false; } @@ -250,22 +243,50 @@ public function deleteTag(int $id): array { * Find users that need a file to be shared with, so all members of the group have it * Also says if group share is the correct choice. * - * @param int $fileId of the file to check what + * @param Node $node of the file to check * @param string|null $groupId the id of the group * @return array */ - public function usersNeedShare(int $fileId, ?string $groupId): array { + public function usersNeedShare(Node $node, ?string $groupId): array { + $group = $this->groupManager->get($groupId); + $groupUsers = $group->getUsers(); + $usersSet = []; + $groupsSet = []; // Stores the user if a share does not exist directly otherwise it is false + foreach ($groupUsers as $groupUser) { + $usersSet[$groupUser->getUID()] = $groupUser; + } + $ownerid = $node->getOwner()->getUID(); + do { + foreach ($this->shareManager->getSharesBy($ownerid, ISHARE::TYPE_GROUP, $node) as $share) { + $groupsSet[$share->getSharedWith()] = true; + } + foreach ($this->shareManager->getSharesBy($ownerid, ISHARE::TYPE_USER, $node) as $share) { + if (isset($usersSet[$share->getSharedWith()])) { + $usersSet[$share->getSharedWith()] = false; + } + } + $node = $node->getParent(); + } while ($node->getParentId() !== -1); + $groupShare = true; $users = []; - $group = $this->groupManager->get($groupId); - if ($group instanceof IGroup) { - foreach ($group->getUsers() as $groupUser) { - if ($this->userHasAccessTo($fileId, $groupUser->getUID())) { - $groupShare = false; - } else { - $users[] = $groupUser->getUID(); + foreach ($usersSet as $uid => $hasShare) { + if ($hasShare !== false) { // User has no share + // Checks if the user is in a group that is being shared with + $groupList = $this->groupManager->getUserGroupIds($hasShare); + $groupFound = false; + foreach ($groupList as $groupId) { + if (isset($groups[$groupId])) { + $groupFound = true; + break; + } + } + if (!$groupFound) { // User is not in a group that is being shared with + $users[] = $uid; + continue; } } + $groupShare = false; } return ['groupShare' => $groupShare, 'users' => $users]; } From 55349abfbd73094b5179398b8de798f49804eed9 Mon Sep 17 00:00:00 2001 From: Lukas Schaefer Date: Tue, 17 Jun 2025 11:23:24 -0400 Subject: [PATCH 4/5] add limit to usersNeedShare to prevent large groups Signed-off-by: Lukas Schaefer --- lib/Service/UtilsService.php | 5 ++++- tests/unit/Service/ApprovalServiceTest.php | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/Service/UtilsService.php b/lib/Service/UtilsService.php index a617d618..aef39d6d 100644 --- a/lib/Service/UtilsService.php +++ b/lib/Service/UtilsService.php @@ -250,6 +250,9 @@ public function deleteTag(int $id): array { public function usersNeedShare(Node $node, ?string $groupId): array { $group = $this->groupManager->get($groupId); $groupUsers = $group->getUsers(); + if (count($groupUsers) > 10) { // Ensure that this potentially slow operation is not done for big groups + return ['groupShare' => true, 'users' => []]; + } $usersSet = []; $groupsSet = []; // Stores the user if a share does not exist directly otherwise it is false foreach ($groupUsers as $groupUser) { @@ -268,7 +271,7 @@ public function usersNeedShare(Node $node, ?string $groupId): array { $node = $node->getParent(); } while ($node->getParentId() !== -1); - $groupShare = true; + $groupShare = true; // true if no user has a share, false otherwise $users = []; foreach ($usersSet as $uid => $hasShare) { if ($hasShare !== false) { // User has no share diff --git a/tests/unit/Service/ApprovalServiceTest.php b/tests/unit/Service/ApprovalServiceTest.php index 93ae911d..ad4de9e7 100644 --- a/tests/unit/Service/ApprovalServiceTest.php +++ b/tests/unit/Service/ApprovalServiceTest.php @@ -105,6 +105,7 @@ protected function setUp(): void { $this->utilsService = new UtilsService( 'approval', $c->get(IUserManager::class), + $c->get(IGroupManager::class), $c->get(IShareManager::class), $c->get(IRootFolder::class), $c->get(ISystemTagManager::class), From a006d8a9865c6cd6501f52a0ec2de83380e24226 Mon Sep 17 00:00:00 2001 From: Lukas Schaefer Date: Tue, 17 Jun 2025 11:43:33 -0400 Subject: [PATCH 5/5] fix psalm Signed-off-by: Lukas Schaefer --- lib/Service/UtilsService.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/Service/UtilsService.php b/lib/Service/UtilsService.php index aef39d6d..e90b13c2 100644 --- a/lib/Service/UtilsService.php +++ b/lib/Service/UtilsService.php @@ -17,7 +17,6 @@ use OCP\Files\Node; use OCP\IConfig; -use OCP\IGroup; use OCP\IGroupManager; use OCP\IUser; use OCP\IUserManager; @@ -279,7 +278,7 @@ public function usersNeedShare(Node $node, ?string $groupId): array { $groupList = $this->groupManager->getUserGroupIds($hasShare); $groupFound = false; foreach ($groupList as $groupId) { - if (isset($groups[$groupId])) { + if (isset($groupsSet[$groupId])) { $groupFound = true; break; }