From dd47daa4a8bc21dd478b034ca1ffe9a3626b57b4 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Tue, 18 Feb 2025 16:36:28 +0100 Subject: [PATCH 1/3] fix(Share20): Convert broken hooks to IEventListener Signed-off-by: provokateurin --- .../sharing_features/sharing-v1-part2.feature | 23 +++++++++++++ lib/base.php | 12 ++++--- lib/composer/composer/autoload_classmap.php | 3 +- lib/composer/composer/autoload_static.php | 3 +- lib/private/Share20/GroupDeletedListener.php | 32 +++++++++++++++++++ lib/private/Share20/Hooks.php | 20 ------------ lib/private/Share20/UserDeletedListener.php | 32 +++++++++++++++++++ 7 files changed, 99 insertions(+), 26 deletions(-) create mode 100644 lib/private/Share20/GroupDeletedListener.php delete mode 100644 lib/private/Share20/Hooks.php create mode 100644 lib/private/Share20/UserDeletedListener.php diff --git a/build/integration/sharing_features/sharing-v1-part2.feature b/build/integration/sharing_features/sharing-v1-part2.feature index e4da84365085c..46bcdf486d28e 100644 --- a/build/integration/sharing_features/sharing-v1-part2.feature +++ b/build/integration/sharing_features/sharing-v1-part2.feature @@ -543,6 +543,29 @@ Feature: sharing And the HTTP status code should be "200" And last share_id is included in the answer + Scenario: Group shares are deleted when the group is deleted + Given As an "admin" + And user "user0" exists + And user "user1" exists + And group "group0" exists + And user "user0" belongs to group "group0" + And file "textfile0.txt" of user "user1" is shared with group "group0" + And As an "user0" + When sending "GET" to "/apps/files_sharing/api/v1/shares?shared_with_me=true" + Then the OCS status code should be "100" + And the HTTP status code should be "200" + And last share_id is included in the answer + When group "group0" does not exist + Then sending "GET" to "/apps/files_sharing/api/v1/shares?shared_with_me=true" + And the OCS status code should be "100" + And the HTTP status code should be "200" + And last share_id is not included in the answer + When group "group0" exists + Then sending "GET" to "/apps/files_sharing/api/v1/shares?shared_with_me=true" + And the OCS status code should be "100" + And the HTTP status code should be "200" + And last share_id is not included in the answer + Scenario: User is not allowed to reshare file As an "admin" Given user "user0" exists diff --git a/lib/base.php b/lib/base.php index 546a119479bb1..d46bcf3f4b91b 100644 --- a/lib/base.php +++ b/lib/base.php @@ -7,8 +7,12 @@ * SPDX-License-Identifier: AGPL-3.0-only */ use OC\Encryption\HookManager; +use OC\Share20\GroupDeletedListener; use OC\Share20\Hooks; +use OC\Share20\UserDeletedListener; +use OC\Share20\UserRemovedListener; use OCP\EventDispatcher\IEventDispatcher; +use OCP\Group\Events\GroupDeletedEvent; use OCP\Group\Events\UserRemovedEvent; use OCP\ILogger; use OCP\IRequest; @@ -18,6 +22,7 @@ use OCP\Server; use OCP\Share; use OCP\User\Events\UserChangedEvent; +use OCP\User\Events\UserDeletedEvent; use OCP\Util; use Psr\Log\LoggerInterface; use Symfony\Component\Routing\Exception\MethodNotAllowedException; @@ -942,12 +947,11 @@ private static function registerRenderReferenceEventListener() { */ public static function registerShareHooks(\OC\SystemConfig $systemConfig): void { if ($systemConfig->getValue('installed')) { - OC_Hook::connect('OC_User', 'post_deleteUser', Hooks::class, 'post_deleteUser'); - OC_Hook::connect('OC_User', 'post_deleteGroup', Hooks::class, 'post_deleteGroup'); - /** @var IEventDispatcher $dispatcher */ $dispatcher = Server::get(IEventDispatcher::class); - $dispatcher->addServiceListener(UserRemovedEvent::class, \OC\Share20\UserRemovedListener::class); + $dispatcher->addServiceListener(UserRemovedEvent::class, UserRemovedListener::class); + $dispatcher->addServiceListener(GroupDeletedEvent::class, GroupDeletedListener::class); + $dispatcher->addServiceListener(UserDeletedEvent::class, UserDeletedListener::class); } } diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 5c36d30554b67..5238decfdf340 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1989,7 +1989,7 @@ 'OC\\Share20\\Exception\\BackendError' => $baseDir . '/lib/private/Share20/Exception/BackendError.php', 'OC\\Share20\\Exception\\InvalidShare' => $baseDir . '/lib/private/Share20/Exception/InvalidShare.php', 'OC\\Share20\\Exception\\ProviderException' => $baseDir . '/lib/private/Share20/Exception/ProviderException.php', - 'OC\\Share20\\Hooks' => $baseDir . '/lib/private/Share20/Hooks.php', + 'OC\\Share20\\GroupDeletedListener' => $baseDir . '/lib/private/Share20/GroupDeletedListener.php', 'OC\\Share20\\LegacyHooks' => $baseDir . '/lib/private/Share20/LegacyHooks.php', 'OC\\Share20\\Manager' => $baseDir . '/lib/private/Share20/Manager.php', 'OC\\Share20\\ProviderFactory' => $baseDir . '/lib/private/Share20/ProviderFactory.php', @@ -1998,6 +1998,7 @@ 'OC\\Share20\\ShareAttributes' => $baseDir . '/lib/private/Share20/ShareAttributes.php', 'OC\\Share20\\ShareDisableChecker' => $baseDir . '/lib/private/Share20/ShareDisableChecker.php', 'OC\\Share20\\ShareHelper' => $baseDir . '/lib/private/Share20/ShareHelper.php', + 'OC\\Share20\\UserDeletedListener' => $baseDir . '/lib/private/Share20/UserDeletedListener.php', 'OC\\Share20\\UserRemovedListener' => $baseDir . '/lib/private/Share20/UserRemovedListener.php', 'OC\\Share\\Constants' => $baseDir . '/lib/private/Share/Constants.php', 'OC\\Share\\Helper' => $baseDir . '/lib/private/Share/Helper.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index d366d58472f94..84ffeac0a49df 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -2038,7 +2038,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Share20\\Exception\\BackendError' => __DIR__ . '/../../..' . '/lib/private/Share20/Exception/BackendError.php', 'OC\\Share20\\Exception\\InvalidShare' => __DIR__ . '/../../..' . '/lib/private/Share20/Exception/InvalidShare.php', 'OC\\Share20\\Exception\\ProviderException' => __DIR__ . '/../../..' . '/lib/private/Share20/Exception/ProviderException.php', - 'OC\\Share20\\Hooks' => __DIR__ . '/../../..' . '/lib/private/Share20/Hooks.php', + 'OC\\Share20\\GroupDeletedListener' => __DIR__ . '/../../..' . '/lib/private/Share20/GroupDeletedListener.php', 'OC\\Share20\\LegacyHooks' => __DIR__ . '/../../..' . '/lib/private/Share20/LegacyHooks.php', 'OC\\Share20\\Manager' => __DIR__ . '/../../..' . '/lib/private/Share20/Manager.php', 'OC\\Share20\\ProviderFactory' => __DIR__ . '/../../..' . '/lib/private/Share20/ProviderFactory.php', @@ -2047,6 +2047,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Share20\\ShareAttributes' => __DIR__ . '/../../..' . '/lib/private/Share20/ShareAttributes.php', 'OC\\Share20\\ShareDisableChecker' => __DIR__ . '/../../..' . '/lib/private/Share20/ShareDisableChecker.php', 'OC\\Share20\\ShareHelper' => __DIR__ . '/../../..' . '/lib/private/Share20/ShareHelper.php', + 'OC\\Share20\\UserDeletedListener' => __DIR__ . '/../../..' . '/lib/private/Share20/UserDeletedListener.php', 'OC\\Share20\\UserRemovedListener' => __DIR__ . '/../../..' . '/lib/private/Share20/UserRemovedListener.php', 'OC\\Share\\Constants' => __DIR__ . '/../../..' . '/lib/private/Share/Constants.php', 'OC\\Share\\Helper' => __DIR__ . '/../../..' . '/lib/private/Share/Helper.php', diff --git a/lib/private/Share20/GroupDeletedListener.php b/lib/private/Share20/GroupDeletedListener.php new file mode 100644 index 0000000000000..7e1ad71c46596 --- /dev/null +++ b/lib/private/Share20/GroupDeletedListener.php @@ -0,0 +1,32 @@ + + */ +class GroupDeletedListener implements IEventListener { + public function __construct( + protected IManager $shareManager, + ) { + } + + public function handle(Event $event): void { + if (!$event instanceof GroupDeletedEvent) { + return; + } + + $this->shareManager->groupDeleted($event->getGroup()->getGID()); + } +} diff --git a/lib/private/Share20/Hooks.php b/lib/private/Share20/Hooks.php deleted file mode 100644 index 809b50791e50f..0000000000000 --- a/lib/private/Share20/Hooks.php +++ /dev/null @@ -1,20 +0,0 @@ -get(IShareManager::class)->userDeleted($arguments['uid']); - } - - public static function post_deleteGroup($arguments) { - \OC::$server->get(IShareManager::class)->groupDeleted($arguments['gid']); - } -} diff --git a/lib/private/Share20/UserDeletedListener.php b/lib/private/Share20/UserDeletedListener.php new file mode 100644 index 0000000000000..e0e091454b04b --- /dev/null +++ b/lib/private/Share20/UserDeletedListener.php @@ -0,0 +1,32 @@ + + */ +class UserDeletedListener implements IEventListener { + public function __construct( + protected IManager $shareManager, + ) { + } + + public function handle(Event $event): void { + if (!$event instanceof UserDeletedEvent) { + return; + } + + $this->shareManager->userDeleted($event->getUser()->getUID()); + } +} From 0df4817be1eb04c78fd83380fb97f4c2cea65380 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Mon, 24 Feb 2025 11:28:28 +0100 Subject: [PATCH 2/3] fix(Share20\Manager): Propagate user and group deletion to remote share providers Signed-off-by: provokateurin --- lib/private/Share20/Manager.php | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index a84481730c4b9..1749783bc51a5 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1540,8 +1540,14 @@ public function userDeleted($uid) { * @inheritdoc */ public function groupDeleted($gid) { - $provider = $this->factory->getProviderForType(IShare::TYPE_GROUP); - $provider->groupDeleted($gid); + foreach ([IShare::TYPE_GROUP, IShare::TYPE_REMOTE_GROUP] as $type) { + try { + $provider = $this->factory->getProviderForType($type); + } catch (ProviderException $e) { + continue; + } + $provider->groupDeleted($gid); + } $excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups_list', ''); if ($excludedGroups === '') { @@ -1561,8 +1567,14 @@ public function groupDeleted($gid) { * @inheritdoc */ public function userDeletedFromGroup($uid, $gid) { - $provider = $this->factory->getProviderForType(IShare::TYPE_GROUP); - $provider->userDeletedFromGroup($uid, $gid); + foreach ([IShare::TYPE_GROUP, IShare::TYPE_REMOTE_GROUP] as $type) { + try { + $provider = $this->factory->getProviderForType($type); + } catch (ProviderException $e) { + continue; + } + $provider->userDeletedFromGroup($uid, $gid); + } } /** From f5b5aec296800e32c426bb9446494e2c0454ef19 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Mon, 24 Feb 2025 12:07:08 +0100 Subject: [PATCH 3/3] fix(FederatedShareProvider): Delete external shares when groups are deleted or users removed from a group Signed-off-by: provokateurin --- .../lib/FederatedShareProvider.php | 58 ++++++++++++++----- 1 file changed, 44 insertions(+), 14 deletions(-) diff --git a/apps/federatedfilesharing/lib/FederatedShareProvider.php b/apps/federatedfilesharing/lib/FederatedShareProvider.php index 2c836dfc090ab..c7ee40a0cbebd 100644 --- a/apps/federatedfilesharing/lib/FederatedShareProvider.php +++ b/apps/federatedfilesharing/lib/FederatedShareProvider.php @@ -882,30 +882,60 @@ public function userDeleted($uid, $shareType) { //TODO: probably a good idea to send unshare info to remote servers $qb = $this->dbConnection->getQueryBuilder(); - $qb->delete('share') ->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_REMOTE))) ->andWhere($qb->expr()->eq('uid_owner', $qb->createNamedParameter($uid))) ->executeStatement(); + + $qb = $this->dbConnection->getQueryBuilder(); + $qb->delete('share_external') + ->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_GROUP))) + ->andWhere($qb->expr()->eq('user', $qb->createNamedParameter($uid))) + ->executeStatement(); } - /** - * This provider does not handle groups - * - * @param string $gid - */ public function groupDeleted($gid) { - // We don't handle groups here + $qb = $this->dbConnection->getQueryBuilder(); + $qb->select('id') + ->from('share_external') + ->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_GROUP))) + // This is not a typo, the group ID is really stored in the 'user' column + ->andWhere($qb->expr()->eq('user', $qb->createNamedParameter($gid))); + $cursor = $qb->executeQuery(); + $parentShareIds = $cursor->fetchAll(\PDO::FETCH_COLUMN); + $cursor->closeCursor(); + if ($parentShareIds === []) { + return; + } + + $qb = $this->dbConnection->getQueryBuilder(); + $parentShareIdsParam = $qb->createNamedParameter($parentShareIds, IQueryBuilder::PARAM_INT_ARRAY); + $qb->delete('share_external') + ->where($qb->expr()->in('id', $parentShareIdsParam)) + ->orWhere($qb->expr()->in('parent', $parentShareIdsParam)) + ->executeStatement(); } - /** - * This provider does not handle groups - * - * @param string $uid - * @param string $gid - */ public function userDeletedFromGroup($uid, $gid) { - // We don't handle groups here + $qb = $this->dbConnection->getQueryBuilder(); + $qb->select('id') + ->from('share_external') + ->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_GROUP))) + // This is not a typo, the group ID is really stored in the 'user' column + ->andWhere($qb->expr()->eq('user', $qb->createNamedParameter($gid))); + $cursor = $qb->executeQuery(); + $parentShareIds = $cursor->fetchAll(\PDO::FETCH_COLUMN); + $cursor->closeCursor(); + if ($parentShareIds === []) { + return; + } + + $qb = $this->dbConnection->getQueryBuilder(); + $parentShareIdsParam = $qb->createNamedParameter($parentShareIds, IQueryBuilder::PARAM_INT_ARRAY); + $qb->delete('share_external') + ->where($qb->expr()->in('parent', $parentShareIdsParam)) + ->andWhere($qb->expr()->eq('user', $qb->createNamedParameter($uid))) + ->executeStatement(); } /**