From 9ea950032cb550aa3e71b39bf68f410ff4fb1f7f Mon Sep 17 00:00:00 2001 From: provokateurin Date: Mon, 27 Jul 2026 08:29:15 +0200 Subject: [PATCH] feat(Sharing): Allow omitting existing recipients when searching for recipients Signed-off-by: provokateurin --- .../lib/Controller/ApiV1Controller.php | 28 ++++--- apps/sharing/openapi.json | 74 +++++++++++-------- apps/sharing/tests/Command/CommandTest.php | 14 +++- .../tests/Controller/ApiV1ControllerTest.php | 4 +- lib/private/Sharing/SharingManager.php | 23 +++++- lib/public/Sharing/ISharingManager.php | 6 +- openapi.json | 74 +++++++++++-------- .../Sharing/AbstractSharingManagerTests.php | 67 ++++++++++++++++- tests/lib/Sharing/SharingManagerTest.php | 14 +++- 9 files changed, 212 insertions(+), 92 deletions(-) diff --git a/apps/sharing/lib/Controller/ApiV1Controller.php b/apps/sharing/lib/Controller/ApiV1Controller.php index 11334aa04caba..200ac8719e7cf 100644 --- a/apps/sharing/lib/Controller/ApiV1Controller.php +++ b/apps/sharing/lib/Controller/ApiV1Controller.php @@ -75,18 +75,20 @@ public function __construct( /** * Search for recpients that can be added to a share. * - * @param ?list> $recipientTypeClasses Type class of recipients to filter by + * @param ?list> $filterRecipientTypeClasses Type classes of recipients to filter by * @param string $query The query to search for * @param int<1, 100> $limit The maximum number of participants * @param non-negative-int $offset The offset of the participants - * @return DataResponse, array{}>|DataResponse + * @param ?string $id If provided, recipients that are already part of the share will not be returned. + * @return DataResponse, array{}>|DataResponse * * 200: Recipients returned * 400: Invalid recipient search parameters + * 404: Share used for filtering existing recipients does not exist */ #[NoAdminRequired] #[ApiRoute(verb: 'GET', url: '/api/v1/recipients')] - public function searchRecipients(?array $recipientTypeClasses, string $query, int $limit = 10, int $offset = 0): DataResponse { + public function searchRecipients(?array $filterRecipientTypeClasses, string $query, int $limit = 10, int $offset = 0, ?string $id = null): DataResponse { /** @psalm-suppress DocblockTypeContradiction */ if ($limit < 1) { return new DataResponse('The limit is too low.', Http::STATUS_BAD_REQUEST); @@ -103,10 +105,17 @@ public function searchRecipients(?array $recipientTypeClasses, string $query, in } try { - $recipients = $this->manager->searchRecipients($this->accessContext, $recipientTypeClasses, $query, $limit, $offset); - return new DataResponse(ShareRecipient::formatMultiple($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $recipients)); - } catch (ShareInvalidException $shareInvalidException) { - return new DataResponse($shareInvalidException->getHint(), Http::STATUS_BAD_REQUEST); + try { + $this->dbConnection->beginTransaction(); + $recipients = $this->manager->searchRecipients($this->accessContext, $filterRecipientTypeClasses, $query, $limit, $offset, $id); + $this->dbConnection->commit(); + return new DataResponse(ShareRecipient::formatMultiple($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $recipients)); + } catch (Exception $exception) { + $this->dbConnection->rollBack(); + throw $exception; + } + } catch (ShareNotFoundException $shareNotFoundException) { + return new DataResponse($shareNotFoundException->getHint(), Http::STATUS_NOT_FOUND); } } @@ -519,10 +528,9 @@ public function deleteShare(string $id): DataResponse { * @param string $id ID of the share * @param ?string $secret Secret of the share * @param array, mixed> $arguments Arguments for accessing the share - * @return DataResponse|DataResponse + * @return DataResponse|DataResponse * * 200: Share returned - * 400: Invalid arguments * 404: Share not found */ #[PublicPage] @@ -540,8 +548,6 @@ public function getShare(string $id, ?string $secret = null, array $arguments = $this->dbConnection->rollBack(); throw $exception; } - } catch (ShareInvalidException $shareInvalidException) { - return new DataResponse($shareInvalidException->getHint(), Http::STATUS_BAD_REQUEST); } catch (ShareNotFoundException $shareNotFoundException) { return new DataResponse($shareNotFoundException->getHint(), Http::STATUS_NOT_FOUND); } diff --git a/apps/sharing/openapi.json b/apps/sharing/openapi.json index 6856c3e478e86..c47a92717860f 100644 --- a/apps/sharing/openapi.json +++ b/apps/sharing/openapi.json @@ -618,9 +618,9 @@ ], "parameters": [ { - "name": "recipientTypeClasses[]", + "name": "filterRecipientTypeClasses[]", "in": "query", - "description": "Type class of recipients to filter by", + "description": "Type classes of recipients to filter by", "schema": { "type": "array", "nullable": true, @@ -662,6 +662,16 @@ "minimum": 0 } }, + { + "name": "id", + "in": "query", + "description": "If provided, recipients that are already part of the share will not be returned.", + "schema": { + "type": "string", + "nullable": true, + "default": null + } + }, { "name": "OCS-APIRequest", "in": "header", @@ -737,6 +747,36 @@ } } }, + "404": { + "description": "Share used for filtering existing recipients does not exist", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "ocs" + ], + "properties": { + "ocs": { + "type": "object", + "required": [ + "meta", + "data" + ], + "properties": { + "meta": { + "$ref": "#/components/schemas/OCSMeta" + }, + "data": { + "type": "string" + } + } + } + } + } + } + } + }, "401": { "description": "Current user is not logged in", "content": { @@ -3005,36 +3045,6 @@ } } }, - "400": { - "description": "Invalid arguments", - "content": { - "application/json": { - "schema": { - "type": "object", - "required": [ - "ocs" - ], - "properties": { - "ocs": { - "type": "object", - "required": [ - "meta", - "data" - ], - "properties": { - "meta": { - "$ref": "#/components/schemas/OCSMeta" - }, - "data": { - "type": "string" - } - } - } - } - } - } - } - }, "404": { "description": "Share not found", "content": { diff --git a/apps/sharing/tests/Command/CommandTest.php b/apps/sharing/tests/Command/CommandTest.php index 73434fc141c27..b10dfef40c31e 100644 --- a/apps/sharing/tests/Command/CommandTest.php +++ b/apps/sharing/tests/Command/CommandTest.php @@ -145,10 +145,18 @@ private function runCommand(ShareAccessContext $accessContext, string $class, ar } #[Override] - protected function searchRecipients(ShareAccessContext $accessContext, ?array $recipientTypeClasses, string $query, int $limit, int $offset): array { + protected function searchRecipients(ShareAccessContext $accessContext, ?array $filterRecipientTypeClasses, string $query, int $limit, int $offset, ?string $id = null): array { // We don't have a command for this, so we just call the real manager to make the test pass. - /** @psalm-suppress ArgumentTypeCoercion */ - return ShareRecipient::formatMultiple(Server::get(ISharingRegistry::class), Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $this->manager->searchRecipients($accessContext, $recipientTypeClasses, $query, $limit, $offset)); + try { + $this->dbConnection->beginTransaction(); + /** @psalm-suppress ArgumentTypeCoercion */ + $shares = ShareRecipient::formatMultiple($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $this->manager->searchRecipients($accessContext, $filterRecipientTypeClasses, $query, $limit, $offset, $id)); + $this->dbConnection->commit(); + return $shares; + } catch (Exception $exception) { + $this->dbConnection->rollBack(); + throw $exception; + } } /** diff --git a/apps/sharing/tests/Controller/ApiV1ControllerTest.php b/apps/sharing/tests/Controller/ApiV1ControllerTest.php index 536f863824311..bfee7b020155f 100644 --- a/apps/sharing/tests/Controller/ApiV1ControllerTest.php +++ b/apps/sharing/tests/Controller/ApiV1ControllerTest.php @@ -84,9 +84,9 @@ private function executeRequest(ShareAccessContext $accessContext, Closure $clos } #[Override] - protected function searchRecipients(ShareAccessContext $accessContext, ?array $recipientTypeClasses, string $query, int $limit, int $offset): array { + protected function searchRecipients(ShareAccessContext $accessContext, ?array $filterRecipientTypeClasses, string $query, int $limit, int $offset, ?string $id = null): array { /** @psalm-suppress ArgumentTypeCoercion */ - return $this->executeRequest($accessContext, fn (ApiV1Controller $controller): DataResponse => $controller->searchRecipients($recipientTypeClasses, $query, $limit, $offset)); + return $this->executeRequest($accessContext, fn (ApiV1Controller $controller): DataResponse => $controller->searchRecipients($filterRecipientTypeClasses, $query, $limit, $offset, $id)); } #[Override] diff --git a/lib/private/Sharing/SharingManager.php b/lib/private/Sharing/SharingManager.php index fddfe871e5155..563ad2be1c5d6 100644 --- a/lib/private/Sharing/SharingManager.php +++ b/lib/private/Sharing/SharingManager.php @@ -87,12 +87,12 @@ public function __construct( } #[\Override] - public function searchRecipients(ShareAccessContext $accessContext, ?array $recipientTypeClasses, string $query, int $limit, int $offset): array { + public function searchRecipients(ShareAccessContext $accessContext, ?array $filterRecipientTypeClasses, string $query, int $limit, int $offset, ?string $id = null): array { $recipientTypes = $this->registry->getRecipientTypes(); - if ($recipientTypeClasses !== null) { + if ($filterRecipientTypeClasses !== null) { $filteredRecipientTypes = []; - foreach (array_unique($recipientTypeClasses) as $recipientTypeClass) { + foreach (array_unique($filterRecipientTypeClasses) as $recipientTypeClass) { if (($recipientType = $recipientTypes[$recipientTypeClass] ?? null) === null) { throw new RuntimeException('The recipient type is not registered: ' . $recipientTypeClass); } @@ -112,10 +112,25 @@ public function searchRecipients(ShareAccessContext $accessContext, ?array $reci )); } - return array_merge(...array_map( + $results = array_merge(...array_map( static fn (IShareRecipientTypeSearch $recipientType): array => $recipientType->searchRecipients($accessContext, $query, $limit, $offset), $recipientTypes, )); + + if ($id !== null) { + // Do not create a new access context with overridden checks, because it could leak the existence of shares and share recipients. + $share = $this->getShare($accessContext, $id); + $recipients = []; + foreach ($share->recipients as $recipient) { + $recipients[$recipient->class] ??= []; + $recipients[$recipient->class][$recipient->instance ?? ''] ??= []; + $recipients[$recipient->class][$recipient->instance ?? ''][$recipient->value] = true; + } + + $results = array_values(array_filter($results, static fn (ShareRecipient $recipient): bool => !isset($recipients[$recipient->class][$recipient->instance ?? ''][$recipient->value]))); + } + + return $results; } #[\Override] diff --git a/lib/public/Sharing/ISharingManager.php b/lib/public/Sharing/ISharingManager.php index 74c35466a4899..9310ac3f1cc68 100644 --- a/lib/public/Sharing/ISharingManager.php +++ b/lib/public/Sharing/ISharingManager.php @@ -30,13 +30,15 @@ interface ISharingManager { /** * Search for recpients that can be added to a share. * - * @param ?list> $recipientTypeClasses + * @param ?list> $filterRecipientTypeClasses * @param positive-int $limit * @param non-negative-int $offset + * @param ?string $id If provided, recipients that are already part of the share will not be returned. * @return list + * @throws ShareNotFoundException * @since 35.0.0 */ - public function searchRecipients(ShareAccessContext $accessContext, ?array $recipientTypeClasses, string $query, int $limit, int $offset): array; + public function searchRecipients(ShareAccessContext $accessContext, ?array $filterRecipientTypeClasses, string $query, int $limit, int $offset, ?string $id = null): array; /** * Generate a new secret. diff --git a/openapi.json b/openapi.json index 895290b17b998..bafad738d01f1 100644 --- a/openapi.json +++ b/openapi.json @@ -36514,9 +36514,9 @@ ], "parameters": [ { - "name": "recipientTypeClasses[]", + "name": "filterRecipientTypeClasses[]", "in": "query", - "description": "Type class of recipients to filter by", + "description": "Type classes of recipients to filter by", "schema": { "type": "array", "nullable": true, @@ -36558,6 +36558,16 @@ "minimum": 0 } }, + { + "name": "id", + "in": "query", + "description": "If provided, recipients that are already part of the share will not be returned.", + "schema": { + "type": "string", + "nullable": true, + "default": null + } + }, { "name": "OCS-APIRequest", "in": "header", @@ -36633,6 +36643,36 @@ } } }, + "404": { + "description": "Share used for filtering existing recipients does not exist", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "ocs" + ], + "properties": { + "ocs": { + "type": "object", + "required": [ + "meta", + "data" + ], + "properties": { + "meta": { + "$ref": "#/components/schemas/OCSMeta" + }, + "data": { + "type": "string" + } + } + } + } + } + } + } + }, "401": { "description": "Current user is not logged in", "content": { @@ -38901,36 +38941,6 @@ } } }, - "400": { - "description": "Invalid arguments", - "content": { - "application/json": { - "schema": { - "type": "object", - "required": [ - "ocs" - ], - "properties": { - "ocs": { - "type": "object", - "required": [ - "meta", - "data" - ], - "properties": { - "meta": { - "$ref": "#/components/schemas/OCSMeta" - }, - "data": { - "type": "string" - } - } - } - } - } - } - } - }, "404": { "description": "Share not found", "content": { diff --git a/tests/lib/Sharing/AbstractSharingManagerTests.php b/tests/lib/Sharing/AbstractSharingManagerTests.php index 093ac0948a1eb..96238de0aecca 100644 --- a/tests/lib/Sharing/AbstractSharingManagerTests.php +++ b/tests/lib/Sharing/AbstractSharingManagerTests.php @@ -37,7 +37,7 @@ * @psalm-import-type SharingRecipient from Share */ abstract class AbstractSharingManagerTests extends TestCase { - abstract protected function searchRecipients(ShareAccessContext $accessContext, ?array $recipientTypeClasses, string $query, int $limit, int $offset): array; + abstract protected function searchRecipients(ShareAccessContext $accessContext, ?array $filterRecipientTypeClasses, string $query, int $limit, int $offset, ?string $id = null): array; abstract protected function createShare(ShareAccessContext $accessContext): array; @@ -114,7 +114,9 @@ public function setUp(): void { [ $this->user1->getUID() => ['recipient1'], ], - [], + [ + new ShareRecipient(TestShareRecipientType1::class, 'recipient1', null), + ], )); $this->registry->registerRecipientType(new TestShareRecipientType2( [ @@ -123,7 +125,9 @@ public function setUp(): void { [ $this->user2->getUID() => ['recipient2'], ], - [], + [ + new ShareRecipient(TestShareRecipientType2::class, 'recipient2', null), + ], )); $this->registry->registerPropertyType(new TestSharePropertyType1(['valid1'])); $this->registry->markPropertyTypeCompatibleWithSourceType(TestSharePropertyType1::class, TestShareSourceType1::class); @@ -496,6 +500,63 @@ public function testSearchRecipientsIcons(): void { ], $this->searchRecipients($accessContext, null, 'icon', 10, 0)); } + public function testSearchRecipientsOmitExisting(): void { + $accessContext = new ShareAccessContext($this->owner); + + $this->dbConnection->beginTransaction(); + $id = $this->manager->createShare($accessContext); + $this->dbConnection->commit(); + + $this->assertEquals([ + [ + 'class' => TestShareRecipientType1::class, + 'value' => 'recipient1', + 'instance' => null, + 'display_name' => 'Recipient 1', + 'icon' => [ + 'svg' => '', + ], + 'secret' => [ + 'updatable' => false, + ], + 'initiator' => null, + ], + [ + 'class' => TestShareRecipientType2::class, + 'value' => 'recipient2', + 'instance' => null, + 'display_name' => 'Recipient 2', + 'icon' => [ + 'svg' => '', + ], + 'secret' => [ + 'updatable' => false, + ], + 'initiator' => null, + ], + ], $this->searchRecipients($accessContext, null, 'recipient', 3, 0, $id)); + + $this->dbConnection->beginTransaction(); + $this->manager->addShareRecipient($accessContext, $id, new ShareRecipient(TestShareRecipientType1::class, 'recipient1', null)); + $this->dbConnection->commit(); + + $this->assertEquals([ + [ + 'class' => TestShareRecipientType2::class, + 'value' => 'recipient2', + 'instance' => null, + 'display_name' => 'Recipient 2', + 'icon' => [ + 'svg' => '', + ], + 'secret' => [ + 'updatable' => false, + ], + 'initiator' => null, + ], + ], $this->searchRecipients($accessContext, null, 'recipient', 3, 0, $id)); + } + public function testCreateShare(): void { $accessContext = new ShareAccessContext($this->owner); diff --git a/tests/lib/Sharing/SharingManagerTest.php b/tests/lib/Sharing/SharingManagerTest.php index fdf9abb0a6401..117cb18c54d7b 100644 --- a/tests/lib/Sharing/SharingManagerTest.php +++ b/tests/lib/Sharing/SharingManagerTest.php @@ -27,9 +27,17 @@ final class SharingManagerTest extends AbstractSharingManagerTests { #[\Override] - protected function searchRecipients(ShareAccessContext $accessContext, ?array $recipientTypeClasses, string $query, int $limit, int $offset): array { - /** @psalm-suppress ArgumentTypeCoercion */ - return ShareRecipient::formatMultiple($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $this->manager->searchRecipients($accessContext, $recipientTypeClasses, $query, $limit, $offset)); + protected function searchRecipients(ShareAccessContext $accessContext, ?array $filterRecipientTypeClasses, string $query, int $limit, int $offset, ?string $id = null): array { + try { + $this->dbConnection->beginTransaction(); + /** @psalm-suppress ArgumentTypeCoercion */ + $shares = ShareRecipient::formatMultiple($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $this->manager->searchRecipients($accessContext, $filterRecipientTypeClasses, $query, $limit, $offset, $id)); + $this->dbConnection->commit(); + return $shares; + } catch (Exception $exception) { + $this->dbConnection->rollBack(); + throw $exception; + } } #[\Override]