From 1740215ab81a77f302e79c5d5dbf7b93e7e97667 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 27 Aug 2026 14:46:02 +0200 Subject: [PATCH 1/9] test(Sharing): Ensure sharing_share_user_status table is empty after deleting all shares Signed-off-by: provokateurin --- tests/lib/Sharing/AbstractSharingManagerTests.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/lib/Sharing/AbstractSharingManagerTests.php b/tests/lib/Sharing/AbstractSharingManagerTests.php index 225f742bfe831..e36e8cbb5e5ed 100644 --- a/tests/lib/Sharing/AbstractSharingManagerTests.php +++ b/tests/lib/Sharing/AbstractSharingManagerTests.php @@ -257,6 +257,7 @@ protected function tearDown(): void { 'sharing_share_properties', 'sharing_share_recipients', 'sharing_share_sources', + 'sharing_share_user_status', ] as $table) { $qb = $this->dbConnection->getQueryBuilder(); $qb From b11957e323b60259569251bf9b2c623c4753acf4 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 27 Aug 2026 14:53:07 +0200 Subject: [PATCH 2/9] fix(Sharing): Update share timestamp setting share state to draft automatically Signed-off-by: provokateurin --- lib/private/Sharing/SharingManager.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/private/Sharing/SharingManager.php b/lib/private/Sharing/SharingManager.php index 7dca5330c7053..f0bdb59a76ab2 100644 --- a/lib/private/Sharing/SharingManager.php +++ b/lib/private/Sharing/SharingManager.php @@ -239,6 +239,8 @@ public function updateShareUserStatus(ShareAccessContext $accessContext, Share $ throw new ShareInvalidException('Cannot set user status for the owner of the share.', $this->l10n->t('Cannot set user status for the owner of the share.')); } + // We don't update the share last updated value, because the user status is not part of the share itself. + $this->backend->updateShareUserStatus($share->id, $currentUser->getUID(), $userStatus); $share = new Share( @@ -937,12 +939,15 @@ private function processShareUpdates(array $shares): array { try { $this->assertShareCanBeActive($share); } catch (ShareInvalidException) { + $time = $this->getTime(); + $this->backend->setLastUpdated([$share->id], $time); + $this->backend->updateShareState($share->id, ShareState::Draft); $share = new Share( $share->id, $share->owner, - $share->lastUpdated, + $time, ShareState::Draft, $share->userStatus, $share->sources, From baeda29f6461fe8f59c711f2a8af86c1300d40d1 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 27 Aug 2026 13:23:00 +0200 Subject: [PATCH 3/9] fix(Sharing): Always sort sources and recipients when formatting them Signed-off-by: provokateurin --- lib/unstable/Sharing/Recipient/ShareRecipient.php | 3 +++ lib/unstable/Sharing/Source/ShareSource.php | 3 +++ tests/lib/Sharing/AbstractSharingManagerTests.php | 6 ------ 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/unstable/Sharing/Recipient/ShareRecipient.php b/lib/unstable/Sharing/Recipient/ShareRecipient.php index 4825b96d5e777..ca615a83e774b 100644 --- a/lib/unstable/Sharing/Recipient/ShareRecipient.php +++ b/lib/unstable/Sharing/Recipient/ShareRecipient.php @@ -110,6 +110,9 @@ public static function formatMultiple(ISharingRegistry $registry, IFactory $l10n ++$recipientDisplayNames[$displayName]; } + // First sort by instance, then by class and finally by value to get a stable order regardless of the DB order + usort($recipients, static fn (ShareRecipient $a, ShareRecipient $b): int => 4 * ($a->instance === null ? -1 : ($a->instance <=> $b->instance)) + 2 * ($a->class <=> $b->class) + ($a->value <=> $b->value)); + return array_map(static fn (ShareRecipient $recipient): array => $recipient->format($registry, $l10nFactory, $urlGenerator, $userManager, $recipientDisplayNames[$recipientTypes[$recipient->class]?->getRecipientDisplayName($recipient->value) ?? $recipient->value] === 1), $recipients); } diff --git a/lib/unstable/Sharing/Source/ShareSource.php b/lib/unstable/Sharing/Source/ShareSource.php index 650f385723df5..174dbfae51ccc 100644 --- a/lib/unstable/Sharing/Source/ShareSource.php +++ b/lib/unstable/Sharing/Source/ShareSource.php @@ -81,6 +81,9 @@ public static function formatMultiple(ISharingRegistry $registry, IFactory $l10n ++$sourceDisplayNames[$displayName]; } + // First sort by class and then sort by value to get a stable order regardless of the DB order + usort($sources, static fn (ShareSource $a, ShareSource $b): int => 2 * ($a->class <=> $b->class) + ($a->value <=> $b->value)); + return array_map(static fn (ShareSource $source): array => $source->format($registry, $l10nFactory, $sourceDisplayNames[$source->getMetadata($registry)->getDisplayName()] === 1), $sources); } diff --git a/tests/lib/Sharing/AbstractSharingManagerTests.php b/tests/lib/Sharing/AbstractSharingManagerTests.php index e36e8cbb5e5ed..1e8ccd5361c38 100644 --- a/tests/lib/Sharing/AbstractSharingManagerTests.php +++ b/tests/lib/Sharing/AbstractSharingManagerTests.php @@ -3158,8 +3158,6 @@ public function testGetShareWithPublicSecret(bool $isSecretPublic): void { unset($formatted['last_updated']); $this->assertIsList($formatted['recipients']); $this->assertCount(2, $formatted['recipients']); - // Sort because database order is not guaranteed - usort($formatted['recipients'], fn (array $a, array $b): int => $a['value'] <=> $b['value']); $this->assertEquals([ 'class' => TestShareRecipientType1::class, 'value' => 'recipient1', @@ -3248,7 +3246,6 @@ public function testGetShareWithSecret(): void { $formatted = $this->getShare(new ShareAccessContext($this->user2), $share->id); $this->assertDateBetween($before, $after, $this->parseTime($formatted['last_updated'])); - usort($formatted['recipients'], fn (array $a, array $b): int => $a['value'] <=> $b['value']); $this->assertArrayHasKey('recipients', $formatted); $this->assertIsArray($formatted['recipients']); $this->assertCount(4, $formatted['recipients']); @@ -3308,9 +3305,6 @@ public function testGetShareUniqueDisplayNames(): void { $formatted = $this->getShare($accessContext, $share->id); - // Sort because database order is not guaranteed - usort($formatted['sources'], fn (array $a, array $b): int => $a['value'] <=> $b['value']); - usort($formatted['recipients'], fn (array $a, array $b): int => $a['value'] <=> $b['value']); $this->assertEquals([ [ 'class' => TestShareSourceType1::class, From 05257b41c25f49686e2a49a955be58911b9ff33c Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 27 Aug 2026 13:34:20 +0200 Subject: [PATCH 4/9] refactor(Sharing): Move formatting multiple permissions and properties to helper methods Signed-off-by: provokateurin --- .../Sharing/Permission/SharePermission.php | 14 ++++++++++++++ lib/unstable/Sharing/Property/ShareProperty.php | 14 ++++++++++++++ lib/unstable/Sharing/Share.php | 12 ++---------- 3 files changed, 30 insertions(+), 10 deletions(-) diff --git a/lib/unstable/Sharing/Permission/SharePermission.php b/lib/unstable/Sharing/Permission/SharePermission.php index 5d392a3126e62..f6cac7e6dea9b 100644 --- a/lib/unstable/Sharing/Permission/SharePermission.php +++ b/lib/unstable/Sharing/Permission/SharePermission.php @@ -50,4 +50,18 @@ public function format(ISharingRegistry $registry, IFactory $l10nFactory): array 'enabled' => $this->enabled, ]; } + + /** + * @param list $permissions + * @return list + * @experimental 35.0.0 + */ + public static function formatMultiple(ISharingRegistry $registry, IFactory $l10nFactory, array $permissions): array { + $permissions = array_map(static fn (SharePermission $permission): array => $permission->format($registry, $l10nFactory), $permissions); + + // First sort by priority and then sort by class name to get a stable order regardless of the DB order + usort($permissions, static fn (array $a, array $b): int => 2 * ($b['priority'] <=> $a['priority']) + ($a['class'] <=> $b['class'])); + + return $permissions; + } } diff --git a/lib/unstable/Sharing/Property/ShareProperty.php b/lib/unstable/Sharing/Property/ShareProperty.php index ae1bc23b71afb..388bb0758fe18 100644 --- a/lib/unstable/Sharing/Property/ShareProperty.php +++ b/lib/unstable/Sharing/Property/ShareProperty.php @@ -54,4 +54,18 @@ public function format(ISharingRegistry $registry, IFactory $l10nFactory, Share 'value' => $this->value, ]); } + + /** + * @param list $properties + * @return list + * @experimental 35.0.0 + */ + public static function formatMultiple(ISharingRegistry $registry, IFactory $l10nFactory, Share $share, array $properties): array { + $properties = array_map(fn (ShareProperty $property): array => $property->format($registry, $l10nFactory, $share), $properties); + + // First sort by priority and then sort by class name to get a stable order regardless of the DB order + usort($properties, static fn (array $a, array $b): int => 2 * ($b['priority'] <=> $a['priority']) + ($a['class'] <=> $b['class'])); + + return $properties; + } } diff --git a/lib/unstable/Sharing/Share.php b/lib/unstable/Sharing/Share.php index 4f30aacf51e13..7120033d4d5c0 100644 --- a/lib/unstable/Sharing/Share.php +++ b/lib/unstable/Sharing/Share.php @@ -185,10 +185,6 @@ public function getEnabledPermissions(): array { * @experimental 35.0.0 */ public function format(ISharingRegistry $registry, IFactory $l10nFactory, IURLGenerator $urlGenerator, IUserManager $userManager): array { - $properties = array_map(fn (ShareProperty $property): array => $property->format($registry, $l10nFactory, $this), array_values($this->properties)); - // First sort by priority and then sort by class name to get a stable order regardless of the DB order - usort($properties, static fn (array $a, array $b): int => 2 * ($b['priority'] <=> $a['priority']) + ($a['class'] <=> $b['class'])); - $registrySourceTypePermissionTypeClasses = $registry->getSourceTypePermissionTypeClasses(); $registryGenericPermissionTypeClasses = $registry->getGenericPermissionTypeClasses(); $registryPermissionTypeCompatiblePermissionPresetClasses = $registry->getPermissionTypeCompatiblePermissionPresetClasses(); @@ -237,10 +233,6 @@ public function format(ISharingRegistry $registry, IFactory $l10nFactory, IURLGe } } - $permissions = array_map(static fn (SharePermission $permission): array => $permission->format($registry, $l10nFactory), array_values($this->permissions)); - // First sort by priority and then sort by class name to get a stable order regardless of the DB order - usort($permissions, static fn (array $a, array $b): int => 2 * ($b['priority'] <=> $a['priority']) + ($a['class'] <=> $b['class'])); - return [ 'id' => $this->id, 'owner' => $this->owner->format($userManager), @@ -249,8 +241,8 @@ public function format(ISharingRegistry $registry, IFactory $l10nFactory, IURLGe 'user_status' => $this->userStatus?->value, 'sources' => ShareSource::formatMultiple($registry, $l10nFactory, $this->sources), 'recipients' => ShareRecipient::formatMultiple($registry, $l10nFactory, $urlGenerator, $userManager, $this->recipients), - 'properties' => $properties, - 'permissions' => $permissions, + 'properties' => ShareProperty::formatMultiple($registry, $l10nFactory, $this, array_values($this->properties)), + 'permissions' => SharePermission::formatMultiple($registry, $l10nFactory, array_values($this->permissions)), 'permission_preset' => $selectedPermissionPresetClass, ]; } From e803d23f5f5788096c8ef281f9c80c43b514f136 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Mon, 31 Aug 2026 08:51:55 +0200 Subject: [PATCH 5/9] fix(ISharingRegistry): Fix parameter nullablity for registerPermissionType Signed-off-by: provokateurin --- lib/unstable/Sharing/ISharingRegistry.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/unstable/Sharing/ISharingRegistry.php b/lib/unstable/Sharing/ISharingRegistry.php index 71ff44da3559b..7e56c194f00c1 100644 --- a/lib/unstable/Sharing/ISharingRegistry.php +++ b/lib/unstable/Sharing/ISharingRegistry.php @@ -97,7 +97,7 @@ public function markPropertyTypeCompatibleWithRecipientType(string $propertyType public function getPropertyTypes(): array; /** - * @param class-string $sourceTypeClass + * @param ?class-string $sourceTypeClass * @experimental 35.0.0 */ public function registerPermissionType(?string $sourceTypeClass, ISharePermissionType $permissionType): void; From 983d2c0bd272776732ffeb069cd0402af68b4911 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Mon, 31 Aug 2026 09:32:48 +0200 Subject: [PATCH 6/9] refactor(ISharingBackend): Remove createShareProperty and createSharePermission in favor of updateShareProperty and updateSharePermission Signed-off-by: provokateurin --- lib/private/Sharing/SharingBackend.php | 68 ++++++------------------ lib/unstable/Sharing/ISharingBackend.php | 16 ------ 2 files changed, 16 insertions(+), 68 deletions(-) diff --git a/lib/private/Sharing/SharingBackend.php b/lib/private/Sharing/SharingBackend.php index 558b4ad4c84c9..842e13c313cd4 100644 --- a/lib/private/Sharing/SharingBackend.php +++ b/lib/private/Sharing/SharingBackend.php @@ -380,37 +380,6 @@ public function updateShareRecipientSecret(string $id, ShareRecipient $recipient } } - #[\Override] - public function createShareProperty(string $id, ShareProperty $property): ?string { - $value = $property->value; - - $propertyType = $this->registry->getPropertyTypes()[$property->class]; - - if ($propertyType instanceof ISharePropertyTypeModifyValue) { - $value = $propertyType->modifyValueOnSave(null, $property->value); - } - - try { - $qb = $this->connection->getQueryBuilder(); - $qb - ->insert('sharing_share_properties') - ->values([ - 'share_id' => $qb->createNamedParameter($id), - 'property_class_id' => $qb->createNamedParameter($this->classMapper->getClassId($property->class), IQueryBuilder::PARAM_INT), - 'property_value' => $qb->createNamedParameter($value), - ]) - ->executeStatement(); - - return $value; - } catch (\OCP\DB\Exception $exception) { - if ($exception->getReason() === \OCP\DB\Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { - throw new RuntimeException('The property already exists: ' . $property->class, $exception->getCode(), $exception); - } - - throw $exception; - } - } - #[\Override] public function updateShareProperty(string $id, ShareProperty $property): ?string { $value = $property->value; @@ -448,31 +417,18 @@ public function updateShareProperty(string $id, ShareProperty $property): ?strin ) ->executeStatement(); if ($rowCount === 0) { - throw new ShareNotFoundException(); - } - - return $value; - } - - #[\Override] - public function createSharePermission(string $id, SharePermission $permission): void { - try { $qb = $this->connection->getQueryBuilder(); $qb - ->insert('sharing_share_permissions') + ->insert('sharing_share_properties') ->values([ 'share_id' => $qb->createNamedParameter($id), - 'permission_class_id' => $qb->createNamedParameter($this->classMapper->getClassId($permission->class), IQueryBuilder::PARAM_INT), - 'permission_enabled' => $qb->createNamedParameter($permission->enabled, IQueryBuilder::PARAM_BOOL), + 'property_class_id' => $qb->createNamedParameter($this->classMapper->getClassId($property->class), IQueryBuilder::PARAM_INT), + 'property_value' => $qb->createNamedParameter($value), ]) ->executeStatement(); - } catch (\OCP\DB\Exception $exception) { - if ($exception->getReason() === \OCP\DB\Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { - throw new RuntimeException('The permission already exists: ' . $permission->class, $exception->getCode(), $exception); - } - - throw $exception; } + + return $value; } #[\Override] @@ -487,7 +443,15 @@ public function updateSharePermission(string $id, SharePermission $permission): ) ->executeStatement(); if ($rowCount === 0) { - throw new ShareNotFoundException(); + $qb = $this->connection->getQueryBuilder(); + $qb + ->insert('sharing_share_permissions') + ->values([ + 'share_id' => $qb->createNamedParameter($id), + 'permission_class_id' => $qb->createNamedParameter($this->classMapper->getClassId($permission->class), IQueryBuilder::PARAM_INT), + 'permission_enabled' => $qb->createNamedParameter($permission->enabled, IQueryBuilder::PARAM_BOOL), + ]) + ->executeStatement(); } } @@ -1235,7 +1199,7 @@ public function createSharePropertyDefaultValue(Share $share, string $propertyTy $property = new ShareProperty($propertyTypeClass, $propertyType->getDefaultValue($share)); - $value = $this->createShareProperty($share->id, $property); + $value = $this->updateShareProperty($share->id, $property); if ($propertyType instanceof ISharePropertyTypeModifyValue) { $value = $propertyType->modifyValueOnLoad($value); } @@ -1274,7 +1238,7 @@ public function createSharePermissionDefaultValue(Share $share, string $permissi $permission = new SharePermission($permissionTypeClass, $permissionType->isEnabledByDefault()); - $this->createSharePermission($share->id, $permission); + $this->updateSharePermission($share->id, $permission); $permissions = $share->permissions; $permissions[$permissionTypeClass] = $permission; diff --git a/lib/unstable/Sharing/ISharingBackend.php b/lib/unstable/Sharing/ISharingBackend.php index f375178efe270..ce3ce7572428c 100644 --- a/lib/unstable/Sharing/ISharingBackend.php +++ b/lib/unstable/Sharing/ISharingBackend.php @@ -120,14 +120,6 @@ public function onInitiatorDeleted(ShareUser $initiator): array; */ public function updateShareRecipientSecret(string $id, ShareRecipient $recipient, string $secret): void; - /** - * Insert a property for a share. - * - * @throws ShareNotFoundException - * @experimental 35.0.0 - */ - public function createShareProperty(string $id, ShareProperty $property): ?string; - /** * Update a property of a share. * @@ -136,14 +128,6 @@ public function createShareProperty(string $id, ShareProperty $property): ?strin */ public function updateShareProperty(string $id, ShareProperty $property): ?string; - /** - * Insert a permission for a share. - * - * @throws ShareNotFoundException - * @experimental 35.0.0 - */ - public function createSharePermission(string $id, SharePermission $permission): void; - /** * Update a permission of a share. * From 369db4bf8372fde68b9699a4a18a00fc1c3c47af Mon Sep 17 00:00:00 2001 From: provokateurin Date: Wed, 26 Aug 2026 14:21:41 +0200 Subject: [PATCH 7/9] feat(Sharing): Add internal recipient IDs Signed-off-by: provokateurin --- apps/sharing/appinfo/info.xml | 2 +- .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + .../Version1000Date20250929161325.php | 4 +- .../Version1000Date20260826115938.php | 39 +++++++++++++ lib/private/Sharing/SharingBackend.php | 56 ++++++++++--------- 6 files changed, 76 insertions(+), 27 deletions(-) create mode 100644 apps/sharing/lib/Migration/Version1000Date20260826115938.php diff --git a/apps/sharing/appinfo/info.xml b/apps/sharing/appinfo/info.xml index 51272693cd6da..52b416fd0dbfb 100644 --- a/apps/sharing/appinfo/info.xml +++ b/apps/sharing/appinfo/info.xml @@ -9,7 +9,7 @@ Sharing TODO TODO - 1.0.2 + 1.0.3 AGPL-3.0-or-later Kate Döen Sharing diff --git a/apps/sharing/composer/composer/autoload_classmap.php b/apps/sharing/composer/composer/autoload_classmap.php index 68c95ab6cbe14..1675f8ab1834e 100644 --- a/apps/sharing/composer/composer/autoload_classmap.php +++ b/apps/sharing/composer/composer/autoload_classmap.php @@ -29,5 +29,6 @@ 'OCA\\Sharing\\Migration\\Version1000Date20250929161325' => $baseDir . '/../lib/Migration/Version1000Date20250929161325.php', 'OCA\\Sharing\\Migration\\Version1000Date20260731171922' => $baseDir . '/../lib/Migration/Version1000Date20260731171922.php', 'OCA\\Sharing\\Migration\\Version1000Date20260826073021' => $baseDir . '/../lib/Migration/Version1000Date20260826073021.php', + 'OCA\\Sharing\\Migration\\Version1000Date20260826115938' => $baseDir . '/../lib/Migration/Version1000Date20260826115938.php', 'OCA\\Sharing\\ResponseDefinitions' => $baseDir . '/../lib/ResponseDefinitions.php', ); diff --git a/apps/sharing/composer/composer/autoload_static.php b/apps/sharing/composer/composer/autoload_static.php index 7a84f99fd4ef7..f8a3a23d99efa 100644 --- a/apps/sharing/composer/composer/autoload_static.php +++ b/apps/sharing/composer/composer/autoload_static.php @@ -44,6 +44,7 @@ class ComposerStaticInitSharing 'OCA\\Sharing\\Migration\\Version1000Date20250929161325' => __DIR__ . '/..' . '/../lib/Migration/Version1000Date20250929161325.php', 'OCA\\Sharing\\Migration\\Version1000Date20260731171922' => __DIR__ . '/..' . '/../lib/Migration/Version1000Date20260731171922.php', 'OCA\\Sharing\\Migration\\Version1000Date20260826073021' => __DIR__ . '/..' . '/../lib/Migration/Version1000Date20260826073021.php', + 'OCA\\Sharing\\Migration\\Version1000Date20260826115938' => __DIR__ . '/..' . '/../lib/Migration/Version1000Date20260826115938.php', 'OCA\\Sharing\\ResponseDefinitions' => __DIR__ . '/..' . '/../lib/ResponseDefinitions.php', ); diff --git a/apps/sharing/lib/Migration/Version1000Date20250929161325.php b/apps/sharing/lib/Migration/Version1000Date20250929161325.php index 9f59295a82db7..0a73a994a0abe 100644 --- a/apps/sharing/lib/Migration/Version1000Date20250929161325.php +++ b/apps/sharing/lib/Migration/Version1000Date20250929161325.php @@ -55,6 +55,7 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt // TODO: Add possibility to mask permissions for recipients. For reshares the user may only mask permissions for their child recipients, not their self recipients $recipientsTable = $schema->createTable('sharing_share_recipients'); + $recipientsTable->addColumn('id', Types::BIGINT); $recipientsTable->addColumn('share_id', Types::BIGINT); $recipientsTable->addColumn('recipient_class_id', Types::INTEGER); $recipientsTable->addColumn('recipient_value', Types::STRING, ['length' => 255]); @@ -62,7 +63,8 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt $recipientsTable->addColumn('recipient_secret', Types::STRING, ['length' => 32]); $recipientsTable->addColumn('initiator_user_id', Types::STRING, ['length' => 64]); $recipientsTable->addColumn('initiator_instance', Types::STRING, ['length' => 128, 'notnull' => false]); - $recipientsTable->setPrimaryKey(['share_id', 'recipient_class_id', 'recipient_value']); + $recipientsTable->setPrimaryKey(['id']); + $recipientsTable->addUniqueIndex(['share_id', 'recipient_class_id', 'recipient_value']); $recipientsTable->addForeignKeyConstraint($shareTable->getName(), ['share_id'], ['id'], ['onDelete' => 'CASCADE']); // TODO: Maybe needs composite index with share_id $recipientsTable->addUniqueIndex(['recipient_secret']); diff --git a/apps/sharing/lib/Migration/Version1000Date20260826115938.php b/apps/sharing/lib/Migration/Version1000Date20260826115938.php new file mode 100644 index 0000000000000..471298f90f585 --- /dev/null +++ b/apps/sharing/lib/Migration/Version1000Date20260826115938.php @@ -0,0 +1,39 @@ +getTable('sharing_share_recipients'); + if (!$recipientsTable->hasColumn('id')) { + $recipientsTable->addColumn('id', Types::BIGINT); + $recipientsTable->dropPrimaryKey(); + $recipientsTable->setPrimaryKey(['id']); + $recipientsTable->addUniqueIndex(['share_id', 'recipient_class_id', 'recipient_value']); + } + + return $schema; + } +} diff --git a/lib/private/Sharing/SharingBackend.php b/lib/private/Sharing/SharingBackend.php index 842e13c313cd4..71a59b93c2db9 100644 --- a/lib/private/Sharing/SharingBackend.php +++ b/lib/private/Sharing/SharingBackend.php @@ -39,6 +39,7 @@ use OCP\IUser; use OCP\IUserManager; use OCP\L10N\IFactory; +use OCP\Snowflake\ISnowflakeGenerator; use Psr\Clock\ClockInterface; use RuntimeException; @@ -57,6 +58,7 @@ public function __construct( private IEventDispatcher $eventDispatcher, private ClassMapper $classMapper, private ClockInterface $clock, + private ISnowflakeGenerator $snowflakeGenerator, ) { $this->l10n = $factory->get('sharing'); } @@ -225,6 +227,7 @@ public function addShareRecipient(string $id, ShareRecipient $recipient): void { $qb = $this->connection->getQueryBuilder(); $values = [ + 'id' => $qb->createNamedParameter($this->snowflakeGenerator->nextId()), 'share_id' => $qb->createNamedParameter($id), 'recipient_class_id' => $qb->createNamedParameter($this->classMapper->getClassId($recipient->class), IQueryBuilder::PARAM_INT), 'recipient_value' => $qb->createNamedParameter($recipient->value), @@ -671,7 +674,7 @@ private function list( } // The key type is array-key, because PHP will automatically cast the value. We can't type it as integer though, because we need to also support 32 bit systems and there the autocasting doesn't happen, if the value is too large. - /** @var array, recipients: list, properties: array, ShareProperty>, permissions: array, SharePermission>}> $shares */ + /** @var array, recipients: array, properties: array, ShareProperty>, permissions: array, SharePermission>}> $shares */ $shares = []; foreach ($queries as $qb) { $qb @@ -731,14 +734,14 @@ private function list( continue; } - /** @var non-empty-string $id */ - $id = (string)$row['id']; + /** @var non-empty-string $shareId */ + $shareId = (string)$row['id']; /** @var numeric-string $lastUpdated */ $lastUpdated = (string)$row['last_updated']; /** @var string $state */ $state = $row['state']; - $shares[$id] ??= [ - 'id' => $id, + $shares[$shareId] ??= [ + 'id' => $shareId, 'owner' => new ShareUser($ownerUserId, $ownerInstance), 'last_updated' => $lastUpdated, 'state' => ShareState::from($state), @@ -850,15 +853,15 @@ private function list( } $value = $row['source_value']; - $id = $row['share_id']; - $shares[$id]['sources'][] = new ShareSource( + $shareId = $row['share_id']; + $shares[$shareId]['sources'][] = new ShareSource( $typeClass, $value, $shareSourceMetas[$typeClass][$value] ?? null, ); - $shareSourceTypeClasses[$id] ??= []; - $shareSourceTypeClasses[$id][$typeClass] = true; + $shareSourceTypeClasses[$shareId] ??= []; + $shareSourceTypeClasses[$shareId][$typeClass] = true; } } @@ -869,6 +872,7 @@ private function list( $qb = $this->connection->getQueryBuilder(); $qb ->select( + 'sr.id', 'sr.share_id', 'sr.recipient_class_id', 'sr.recipient_value', @@ -888,15 +892,17 @@ private function list( continue; } - /** @var non-empty-string $id */ - $id = (string)$row['share_id']; + /** @var non-empty-string $recipientId */ + $recipientId = (string)$row['id']; + /** @var non-empty-string $shareId */ + $shareId = (string)$row['share_id']; /** @var non-empty-string $initiatorUserId */ $initiatorUserId = $row['initiator_user_id']; /** @var ?non-empty-string $initiatorInstance */ $initiatorInstance = $row['initiator_instance']; /** @psalm-suppress PossiblyNullReference The initiator is automatically promoted to the owner, when the initiator is deleted. */ - if ($initiatorInstance === null && !$accessContext->overrideChecks && !$shares[$id]['owner']->isCurrentUser( + if ($initiatorInstance === null && !$accessContext->overrideChecks && !$shares[$shareId]['owner']->isCurrentUser( $accessContext ) && $this->hideDisabledUserShares() && !$this->userManager->get($initiatorUserId)->isEnabled()) { continue; @@ -910,7 +916,7 @@ private function list( /** @var non-empty-string $secret */ $secret = $row['recipient_secret']; - $shares[$id]['recipients'][] = new ShareRecipient( + $shares[$shareId]['recipients'][$recipientId] = new ShareRecipient( $typeClass, $value, $instance, @@ -921,8 +927,8 @@ private function list( ), ); - $shareRecipientTypeClasses[$id] ??= []; - $shareRecipientTypeClasses[$id][$typeClass] = true; + $shareRecipientTypeClasses[$shareId] ??= []; + $shareRecipientTypeClasses[$shareId][$typeClass] = true; } } @@ -997,9 +1003,9 @@ private function list( $result = $qb->executeQuery(); foreach ($result->fetchAll() as $row) { - /** @var non-empty-string $id */ - $id = (string)$row['share_id']; - if (!isset($shareSourceTypeClasses[$id], $shareRecipientTypeClasses[$id])) { + /** @var non-empty-string $shareId */ + $shareId = (string)$row['share_id']; + if (!isset($shareSourceTypeClasses[$shareId], $shareRecipientTypeClasses[$shareId])) { continue; } @@ -1010,13 +1016,13 @@ private function list( continue; } - if (array_intersect($registryPropertyTypeCompatibleSourceTypeClasses[$propertyTypeClass], array_keys($shareSourceTypeClasses[$id])) === []) { + if (array_intersect($registryPropertyTypeCompatibleSourceTypeClasses[$propertyTypeClass], array_keys($shareSourceTypeClasses[$shareId])) === []) { // Skip properties that are currently not compatible, but don't remove them. continue; } if (array_intersect( - $registryPropertyTypeCompatibleRecipientTypeClasses[$propertyTypeClass], array_keys($shareRecipientTypeClasses[$id]) + $registryPropertyTypeCompatibleRecipientTypeClasses[$propertyTypeClass], array_keys($shareRecipientTypeClasses[$shareId]) ) === []) { // Skip properties that are currently not compatible, but don't remove them. continue; @@ -1030,7 +1036,7 @@ private function list( $value = $propertyType->modifyValueOnLoad($value); } - $shares[$id]['properties'][$propertyTypeClass] = new ShareProperty($propertyTypeClass, $value); + $shares[$shareId]['properties'][$propertyTypeClass] = new ShareProperty($propertyTypeClass, $value); } } @@ -1052,17 +1058,17 @@ private function list( $result = $qb->executeQuery(); foreach ($result->fetchAll() as $row) { - $id = (string)$row['share_id']; + $shareId = (string)$row['share_id']; /** @var class-string $permissionTypeClass */ $permissionTypeClass = $this->classMapper->getClassName((int)$row['permission_class_id']); - if (!isset($shareCompatiblePermissionTypeClasses[$id][$permissionTypeClass])) { + if (!isset($shareCompatiblePermissionTypeClasses[$shareId][$permissionTypeClass])) { // Skip permissions that are currently not compatible, but don't remove them. continue; } $enabled = (bool)$row['permission_enabled']; - $shares[$id]['permissions'][$permissionTypeClass] = new SharePermission($permissionTypeClass, $enabled); + $shares[$shareId]['permissions'][$permissionTypeClass] = new SharePermission($permissionTypeClass, $enabled); } } @@ -1073,7 +1079,7 @@ private function list( $share['state'], $share['user_status'], $share['sources'], - $share['recipients'], + array_values($share['recipients']), $share['properties'], $share['permissions'], ), $shares); From feff479e2d0197c8cadcf13d739231453c3b6b34 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Mon, 31 Aug 2026 09:24:17 +0200 Subject: [PATCH 8/9] feat(Sharing): Add per-recipient permissions Signed-off-by: provokateurin --- apps/sharing/appinfo/info.xml | 3 +- .../composer/composer/autoload_classmap.php | 2 + .../composer/composer/autoload_static.php | 2 + apps/sharing/lib/Command/GetShares.php | 2 +- apps/sharing/lib/Command/SharingBase.php | 2 +- .../UpdateShareRecipientPermission.php | 60 +++++ .../lib/Controller/ApiV1Controller.php | 63 ++++-- .../Version1000Date20250929161325.php | 1 - .../Version1000Date20260826122228.php | 37 +++ apps/sharing/lib/ResponseDefinitions.php | 23 +- apps/sharing/openapi.json | 210 ++++++++++++++++- apps/sharing/tests/Command/CommandTest.php | 24 ++ .../tests/Controller/ApiV1ControllerTest.php | 6 + lib/private/Sharing/SharingBackend.php | 110 ++++++++- lib/private/Sharing/SharingManager.php | 74 +++++- lib/unstable/Sharing/ISharingBackend.php | 8 + lib/unstable/Sharing/ISharingManager.php | 9 + .../Sharing/Recipient/ShareRecipient.php | 32 ++- lib/unstable/Sharing/Share.php | 108 +++++++-- lib/unstable/Sharing/ShareAccessContext.php | 7 + openapi.json | 210 ++++++++++++++++- .../Sharing/AbstractSharingManagerTests.php | 212 ++++++++++++++++++ tests/lib/Sharing/ShareTest.php | 211 +++++++++++++++++ tests/lib/Sharing/SharingManagerTest.php | 52 +++-- 24 files changed, 1381 insertions(+), 87 deletions(-) create mode 100644 apps/sharing/lib/Command/UpdateShareRecipientPermission.php create mode 100644 apps/sharing/lib/Migration/Version1000Date20260826122228.php create mode 100644 tests/lib/Sharing/ShareTest.php diff --git a/apps/sharing/appinfo/info.xml b/apps/sharing/appinfo/info.xml index 52b416fd0dbfb..405780fba970c 100644 --- a/apps/sharing/appinfo/info.xml +++ b/apps/sharing/appinfo/info.xml @@ -9,7 +9,7 @@ Sharing TODO TODO - 1.0.3 + 1.0.4 AGPL-3.0-or-later Kate Döen Sharing @@ -30,6 +30,7 @@ \OCA\Sharing\Command\SelectSharePermissionPreset \OCA\Sharing\Command\UpdateSharePermission \OCA\Sharing\Command\UpdateShareProperty + \OCA\Sharing\Command\UpdateShareRecipientPermission \OCA\Sharing\Command\UpdateShareRecipientSecret \OCA\Sharing\Command\UpdateShareState \OCA\Sharing\Command\UpdateShareUserStatus diff --git a/apps/sharing/composer/composer/autoload_classmap.php b/apps/sharing/composer/composer/autoload_classmap.php index 1675f8ab1834e..cd25fd6f28b58 100644 --- a/apps/sharing/composer/composer/autoload_classmap.php +++ b/apps/sharing/composer/composer/autoload_classmap.php @@ -21,6 +21,7 @@ 'OCA\\Sharing\\Command\\SharingBase' => $baseDir . '/../lib/Command/SharingBase.php', 'OCA\\Sharing\\Command\\UpdateSharePermission' => $baseDir . '/../lib/Command/UpdateSharePermission.php', 'OCA\\Sharing\\Command\\UpdateShareProperty' => $baseDir . '/../lib/Command/UpdateShareProperty.php', + 'OCA\\Sharing\\Command\\UpdateShareRecipientPermission' => $baseDir . '/../lib/Command/UpdateShareRecipientPermission.php', 'OCA\\Sharing\\Command\\UpdateShareRecipientSecret' => $baseDir . '/../lib/Command/UpdateShareRecipientSecret.php', 'OCA\\Sharing\\Command\\UpdateShareState' => $baseDir . '/../lib/Command/UpdateShareState.php', 'OCA\\Sharing\\Command\\UpdateShareUserStatus' => $baseDir . '/../lib/Command/UpdateShareUserStatus.php', @@ -30,5 +31,6 @@ 'OCA\\Sharing\\Migration\\Version1000Date20260731171922' => $baseDir . '/../lib/Migration/Version1000Date20260731171922.php', 'OCA\\Sharing\\Migration\\Version1000Date20260826073021' => $baseDir . '/../lib/Migration/Version1000Date20260826073021.php', 'OCA\\Sharing\\Migration\\Version1000Date20260826115938' => $baseDir . '/../lib/Migration/Version1000Date20260826115938.php', + 'OCA\\Sharing\\Migration\\Version1000Date20260826122228' => $baseDir . '/../lib/Migration/Version1000Date20260826122228.php', 'OCA\\Sharing\\ResponseDefinitions' => $baseDir . '/../lib/ResponseDefinitions.php', ); diff --git a/apps/sharing/composer/composer/autoload_static.php b/apps/sharing/composer/composer/autoload_static.php index f8a3a23d99efa..35b44813420c3 100644 --- a/apps/sharing/composer/composer/autoload_static.php +++ b/apps/sharing/composer/composer/autoload_static.php @@ -36,6 +36,7 @@ class ComposerStaticInitSharing 'OCA\\Sharing\\Command\\SharingBase' => __DIR__ . '/..' . '/../lib/Command/SharingBase.php', 'OCA\\Sharing\\Command\\UpdateSharePermission' => __DIR__ . '/..' . '/../lib/Command/UpdateSharePermission.php', 'OCA\\Sharing\\Command\\UpdateShareProperty' => __DIR__ . '/..' . '/../lib/Command/UpdateShareProperty.php', + 'OCA\\Sharing\\Command\\UpdateShareRecipientPermission' => __DIR__ . '/..' . '/../lib/Command/UpdateShareRecipientPermission.php', 'OCA\\Sharing\\Command\\UpdateShareRecipientSecret' => __DIR__ . '/..' . '/../lib/Command/UpdateShareRecipientSecret.php', 'OCA\\Sharing\\Command\\UpdateShareState' => __DIR__ . '/..' . '/../lib/Command/UpdateShareState.php', 'OCA\\Sharing\\Command\\UpdateShareUserStatus' => __DIR__ . '/..' . '/../lib/Command/UpdateShareUserStatus.php', @@ -45,6 +46,7 @@ class ComposerStaticInitSharing 'OCA\\Sharing\\Migration\\Version1000Date20260731171922' => __DIR__ . '/..' . '/../lib/Migration/Version1000Date20260731171922.php', 'OCA\\Sharing\\Migration\\Version1000Date20260826073021' => __DIR__ . '/..' . '/../lib/Migration/Version1000Date20260826073021.php', 'OCA\\Sharing\\Migration\\Version1000Date20260826115938' => __DIR__ . '/..' . '/../lib/Migration/Version1000Date20260826115938.php', + 'OCA\\Sharing\\Migration\\Version1000Date20260826122228' => __DIR__ . '/..' . '/../lib/Migration/Version1000Date20260826122228.php', 'OCA\\Sharing\\ResponseDefinitions' => __DIR__ . '/..' . '/../lib/ResponseDefinitions.php', ); diff --git a/apps/sharing/lib/Command/GetShares.php b/apps/sharing/lib/Command/GetShares.php index 4f8240f285eca..01fafd9ef741b 100644 --- a/apps/sharing/lib/Command/GetShares.php +++ b/apps/sharing/lib/Command/GetShares.php @@ -84,7 +84,7 @@ public function execute(InputInterface $input, OutputInterface $output): int { $shares = $this->manager->getShares($this->accessContext, $filterSourceTypeClass, $filterSourceTypeValue, $filterState, $filterUserStatus, $lastShareID, $limit); $this->dbConnection->commit(); - $data = Share::formatMultiple($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $shares); + $data = Share::formatMultiple($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $this->accessContext, $shares); $this->writeArrayInOutputFormat($input, $output, $data); return Base::SUCCESS; } catch (Exception $exception) { diff --git a/apps/sharing/lib/Command/SharingBase.php b/apps/sharing/lib/Command/SharingBase.php index a0a7829d255d9..67aec4556f182 100644 --- a/apps/sharing/lib/Command/SharingBase.php +++ b/apps/sharing/lib/Command/SharingBase.php @@ -71,7 +71,7 @@ protected function wrapExecution(InputInterface $input, OutputInterface $output, $share = $closure(); $this->dbConnection->commit(); - $data = $share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager); + $data = $share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $this->accessContext); $this->writeArrayInOutputFormat($input, $output, $data); return Base::SUCCESS; diff --git a/apps/sharing/lib/Command/UpdateShareRecipientPermission.php b/apps/sharing/lib/Command/UpdateShareRecipientPermission.php new file mode 100644 index 0000000000000..09e7c7849c580 --- /dev/null +++ b/apps/sharing/lib/Command/UpdateShareRecipientPermission.php @@ -0,0 +1,60 @@ +setName('sharing:update-share-recipient-permission') + ->setDescription('Update a permission for a recipient of a share.') + ->addArgument('id', InputArgument::REQUIRED, 'Share ID') + ->addArgument('permission-class', InputArgument::REQUIRED, 'Permission class') + ->addArgument('permission-enabled', InputArgument::REQUIRED, 'Permission enabled. Only takes "true" or "false".') + ->addArgument('recipient-class', InputArgument::REQUIRED, 'Recipient class') + ->addArgument('recipient-value', InputArgument::REQUIRED, 'Recipient value') + ->addArgument('recipient-instance', InputArgument::OPTIONAL, 'Recipient instance'); + parent::configure(); + } + + #[\Override] + public function execute(InputInterface $input, OutputInterface $output): int { + /** @var string $id */ + $id = $input->getArgument('id'); + /** @var class-string $permissionClass */ + $permissionClass = $input->getArgument('permission-class'); + /** @var string $permissionEnabled */ + $permissionEnabled = $input->getArgument('permission-enabled'); + $permissionEnabled = $permissionEnabled === 'true'; + /** @var class-string $recipientClass */ + $recipientClass = $input->getArgument('recipient-class'); + /** @var non-empty-string $recipientValue */ + $recipientValue = $input->getArgument('recipient-value'); + /** @var ?non-empty-string $recipientInstance */ + $recipientInstance = $input->getArgument('recipient-instance'); + + $recipient = new ShareRecipient($recipientClass, $recipientValue, $recipientInstance); + $permission = new SharePermission($permissionClass, $permissionEnabled); + + return $this->wrapExecution($input, $output, function () use ($id, $recipient, $permission): Share { + $share = $this->manager->getShare($this->accessContext, $id); + return $this->manager->updateShareRecipientPermission($this->accessContext, $share, $recipient, $permission); + }); + } +} diff --git a/apps/sharing/lib/Controller/ApiV1Controller.php b/apps/sharing/lib/Controller/ApiV1Controller.php index fa632f2c67282..4af3e0b208171 100644 --- a/apps/sharing/lib/Controller/ApiV1Controller.php +++ b/apps/sharing/lib/Controller/ApiV1Controller.php @@ -156,7 +156,7 @@ public function createShare(): DataResponse { $share = $this->manager->createShare($this->accessContext); $this->dbConnection->commit(); - return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager), Http::STATUS_CREATED); + return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $this->accessContext), Http::STATUS_CREATED); } catch (Exception $exception) { $this->dbConnection->rollBack(); throw $exception; @@ -195,7 +195,7 @@ public function updateShareState(string $id, string $state): DataResponse { $share = $this->manager->getShare($this->accessContext, $id); $share = $this->manager->updateShareState($this->accessContext, $share, $shareState); $this->dbConnection->commit(); - return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager)); + return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $this->accessContext)); } catch (Exception $exception) { $this->dbConnection->rollBack(); throw $exception; @@ -235,7 +235,7 @@ public function updateShareUserStatus(string $id, string $userStatus): DataRespo $share = $this->manager->getShare($this->accessContext, $id); $share = $this->manager->updateShareUserStatus($this->accessContext, $share, $shareUserStatus); $this->dbConnection->commit(); - return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager)); + return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $this->accessContext)); } catch (Exception $exception) { $this->dbConnection->rollBack(); throw $exception; @@ -269,7 +269,7 @@ public function addShareSource(string $id, string $class, string $value): DataRe $share = $this->manager->getShare($this->accessContext, $id); $share = $this->manager->addShareSource($this->accessContext, $share, new ShareSource($class, $value)); $this->dbConnection->commit(); - return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager)); + return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $this->accessContext)); } catch (Exception $exception) { $this->dbConnection->rollBack(); throw $exception; @@ -306,7 +306,7 @@ public function removeShareSource(string $id, string $class, string $value): Dat $share = $this->manager->getShare($this->accessContext, $id); $share = $this->manager->removeShareSource($this->accessContext, $share, new ShareSource($class, $value)); $this->dbConnection->commit(); - return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager)); + return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $this->accessContext)); } catch (Exception $exception) { $this->dbConnection->rollBack(); throw $exception; @@ -343,7 +343,7 @@ public function addShareRecipient(string $id, string $class, string $value, ?str $share = $this->manager->getShare($this->accessContext, $id); $share = $this->manager->addShareRecipient($this->accessContext, $share, new ShareRecipient($class, $value, $instance)); $this->dbConnection->commit(); - return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager)); + return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $this->accessContext)); } catch (Exception $exception) { $this->dbConnection->rollBack(); throw $exception; @@ -381,7 +381,7 @@ public function removeShareRecipient(string $id, string $class, string $value, ? $share = $this->manager->getShare($this->accessContext, $id); $share = $this->manager->removeShareRecipient($this->accessContext, $share, new ShareRecipient($class, $value, $instance)); $this->dbConnection->commit(); - return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager)); + return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $this->accessContext)); } catch (Exception $exception) { $this->dbConnection->rollBack(); throw $exception; @@ -419,7 +419,7 @@ public function updateShareRecipientSecret(string $id, string $class, string $va $share = $this->manager->getShare($this->accessContext, $id); $share = $this->manager->updateShareRecipientSecret($this->accessContext, $share, new ShareRecipient($class, $value, $instance), $secret); $this->dbConnection->commit(); - return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager)); + return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $this->accessContext)); } catch (Exception $exception) { $this->dbConnection->rollBack(); throw $exception; @@ -457,7 +457,7 @@ public function updateShareProperty(string $id, string $class, ?string $value): $share = $this->manager->getShare($this->accessContext, $id); $share = $this->manager->updateShareProperty($this->accessContext, $share, new ShareProperty($class, $value)); $this->dbConnection->commit(); - return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager)); + return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $this->accessContext)); } catch (Exception $exception) { $this->dbConnection->rollBack(); throw $exception; @@ -495,7 +495,7 @@ public function updateSharePermission(string $id, string $class, bool $enabled): $share = $this->manager->getShare($this->accessContext, $id); $share = $this->manager->updateSharePermission($this->accessContext, $share, new SharePermission($class, $enabled)); $this->dbConnection->commit(); - return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager)); + return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $this->accessContext)); } catch (Exception $exception) { $this->dbConnection->rollBack(); throw $exception; @@ -509,6 +509,43 @@ public function updateSharePermission(string $id, string $class, bool $enabled): } } + /** + * Update a permission for a recipient of a share. + * + * @param string $id ID of the share + * @param class-string $recipientClass Type class of the recipient + * @param non-empty-string $recipientValue Value of the recipient + * @param ?non-empty-string $recipientInstance Instance of the recipient + * @param class-string $permissionClass Type class of the permission + * @param bool $enabled Enabled state of the permission + * @return DataResponse|DataResponse + * + * 200: Share recipient permission updated successfully + * 403: Updating the share recipient permission is not allowed + * 404: Share not found + */ + #[NoAdminRequired] + #[ApiRoute(verb: 'PUT', url: '/api/v1/share/{id}/recipient/permission')] + public function updateShareRecipientPermission(string $id, string $recipientClass, string $recipientValue, ?string $recipientInstance, string $permissionClass, bool $enabled): DataResponse { + try { + try { + $this->dbConnection->beginTransaction(); + + $share = $this->manager->getShare($this->accessContext, $id); + $share = $this->manager->updateShareRecipientPermission($this->accessContext, $share, new ShareRecipient($recipientClass, $recipientValue, $recipientInstance), new SharePermission($permissionClass, $enabled)); + $this->dbConnection->commit(); + return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $this->accessContext)); + } catch (Exception $exception) { + $this->dbConnection->rollBack(); + throw $exception; + } + } catch (ShareOperationForbiddenException $shareOperationForbiddenException) { + return new DataResponse($shareOperationForbiddenException->getHint(), Http::STATUS_FORBIDDEN); + } catch (ShareNotFoundException $shareNotFoundException) { + return new DataResponse($shareNotFoundException->getHint(), Http::STATUS_NOT_FOUND); + } + } + /** * Select a permission preset for a share. * @@ -532,7 +569,7 @@ public function selectSharePermissionPreset(string $id, string $permissionPreset $share = $this->manager->getShare($this->accessContext, $id); $share = $this->manager->selectSharePermissionPreset($this->accessContext, $share, $permissionPresetClass); $this->dbConnection->commit(); - return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager)); + return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $this->accessContext)); } catch (Exception $exception) { $this->dbConnection->rollBack(); throw $exception; @@ -601,7 +638,7 @@ public function getShare(string $id, ?string $secret = null, array $arguments = $share = $this->manager->getShare(new ShareAccessContext($this->accessContext->currentUser, $secret, $arguments, $this->accessContext->overrideChecks), $id); $this->dbConnection->commit(); - return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager)); + return new DataResponse($share->format($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $this->accessContext)); } catch (Exception $exception) { $this->dbConnection->rollBack(); throw $exception; @@ -672,7 +709,7 @@ public function getShares(?string $filterSourceTypeClass, ?string $filterSourceT $shares = $this->manager->getShares($this->accessContext, $filterSourceTypeClass, $filterSourceTypeValue, $filterState, $filterUserStatus, $lastShareID, $limit); $this->dbConnection->commit(); - return new DataResponse(Share::formatMultiple($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $shares)); + return new DataResponse(Share::formatMultiple($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $this->accessContext, $shares)); } catch (Exception $exception) { $this->dbConnection->rollBack(); throw $exception; diff --git a/apps/sharing/lib/Migration/Version1000Date20250929161325.php b/apps/sharing/lib/Migration/Version1000Date20250929161325.php index 0a73a994a0abe..164e7955a4277 100644 --- a/apps/sharing/lib/Migration/Version1000Date20250929161325.php +++ b/apps/sharing/lib/Migration/Version1000Date20250929161325.php @@ -53,7 +53,6 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt $sourcesTable->addForeignKeyConstraint($shareTable->getName(), ['share_id'], ['id'], ['onDelete' => 'CASCADE']); $sourcesTable->addForeignKeyConstraint($mappingTable->getName(), ['source_class_id'], ['class_id']); - // TODO: Add possibility to mask permissions for recipients. For reshares the user may only mask permissions for their child recipients, not their self recipients $recipientsTable = $schema->createTable('sharing_share_recipients'); $recipientsTable->addColumn('id', Types::BIGINT); $recipientsTable->addColumn('share_id', Types::BIGINT); diff --git a/apps/sharing/lib/Migration/Version1000Date20260826122228.php b/apps/sharing/lib/Migration/Version1000Date20260826122228.php new file mode 100644 index 0000000000000..c0dbee2ced5bc --- /dev/null +++ b/apps/sharing/lib/Migration/Version1000Date20260826122228.php @@ -0,0 +1,37 @@ +getTable('sharing_share_recipients'); + $mappingTable = $schema->getTable('sharing_classmap'); + + $recipientPermissionsTable = $schema->createTable('sharing_share_recipient_permissions'); + $recipientPermissionsTable->addColumn('recipient_id', Types::BIGINT); + $recipientPermissionsTable->addColumn('permission_class_id', Types::INTEGER); + $recipientPermissionsTable->addColumn('permission_enabled', Types::BOOLEAN); + $recipientPermissionsTable->setPrimaryKey(['recipient_id', 'permission_class_id']); + $recipientPermissionsTable->addForeignKeyConstraint($recipientsTable->getName(), ['recipient_id'], ['id'], ['onDelete' => 'CASCADE']); + $recipientPermissionsTable->addForeignKeyConstraint($mappingTable->getName(), ['permission_class_id'], ['class_id']); + + return $schema; + } +} diff --git a/apps/sharing/lib/ResponseDefinitions.php b/apps/sharing/lib/ResponseDefinitions.php index 5869f2ffdd0f5..116f26cb24ad8 100644 --- a/apps/sharing/lib/ResponseDefinitions.php +++ b/apps/sharing/lib/ResponseDefinitions.php @@ -16,7 +16,7 @@ use NCU\Sharing\Source\IShareSourceType; /** - * Keep the following types in sync with lib/unstable/Sharing/Share.php: + * Keep the following types in sync with apps/sharing/lib/ResponseDefinitions.php: * * @psalm-type SharingIconSVG = array{ * // An SVG using the currentColor value for dynamic theming. @@ -46,6 +46,16 @@ * icon: SharingIcon, * } * + * @psalm-type SharingPermission = array{ + * class: class-string, + * source_class: ?class-string, + * display_name: non-empty-string, + * hint: ?non-empty-string, + * priority: int<1, 100>, + * presets: list>, + * enabled: bool, + * } + * * @psalm-type SharingRecipient = array{ * class: class-string, * value: non-empty-string, @@ -58,6 +68,7 @@ * url?: non-empty-string, * }, * initiator: ?SharingUser, + * permissions: list * } * * @psalm-type SharingState = 'active'|'draft'|'deleted' @@ -107,16 +118,6 @@ * hint: ?non-empty-string, * } * - * @psalm-type SharingPermission = array{ - * class: class-string, - * source_class: ?class-string, - * display_name: non-empty-string, - * hint: ?non-empty-string, - * priority: int<1, 100>, - * presets: list>, - * enabled: bool, - * } - * * @psalm-type SharingSourceType = array{ * class: class-string, * } diff --git a/apps/sharing/openapi.json b/apps/sharing/openapi.json index 25ba5142af114..00b87fa2ebecb 100644 --- a/apps/sharing/openapi.json +++ b/apps/sharing/openapi.json @@ -386,7 +386,8 @@ "display_name", "icon", "secret", - "initiator" + "initiator", + "permissions" ], "properties": { "class": { @@ -440,6 +441,12 @@ "$ref": "#/components/schemas/User" } ] + }, + "permissions": { + "type": "array", + "items": { + "$ref": "#/components/schemas/Permission" + } } } }, @@ -2806,6 +2813,207 @@ } } }, + "/ocs/v2.php/apps/sharing/api/v1/share/{id}/recipient/permission": { + "put": { + "operationId": "api_v1-update-share-recipient-permission", + "summary": "Update a permission for a recipient of a share.", + "tags": [ + "api_v1" + ], + "security": [ + { + "bearer_auth": [] + }, + { + "basic_auth": [] + } + ], + "requestBody": { + "required": true, + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "recipientClass", + "recipientValue", + "permissionClass", + "enabled" + ], + "properties": { + "recipientClass": { + "type": "string", + "description": "Type class of the recipient", + "minLength": 1 + }, + "recipientValue": { + "type": "string", + "description": "Value of the recipient", + "minLength": 1 + }, + "recipientInstance": { + "type": "string", + "nullable": true, + "description": "Instance of the recipient", + "minLength": 1 + }, + "permissionClass": { + "type": "string", + "description": "Type class of the permission", + "minLength": 1 + }, + "enabled": { + "type": "boolean", + "description": "Enabled state of the permission" + } + } + } + } + } + }, + "parameters": [ + { + "name": "id", + "in": "path", + "description": "ID of the share", + "required": true, + "schema": { + "type": "string" + } + }, + { + "name": "OCS-APIRequest", + "in": "header", + "description": "Required to be true for the API request to pass", + "required": true, + "schema": { + "type": "boolean", + "default": true + } + } + ], + "responses": { + "200": { + "description": "Share recipient permission updated successfully", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "ocs" + ], + "properties": { + "ocs": { + "type": "object", + "required": [ + "meta", + "data" + ], + "properties": { + "meta": { + "$ref": "#/components/schemas/OCSMeta" + }, + "data": { + "$ref": "#/components/schemas/Share" + } + } + } + } + } + } + } + }, + "403": { + "description": "Updating the share recipient permission is not allowed", + "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": { + "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": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "ocs" + ], + "properties": { + "ocs": { + "type": "object", + "required": [ + "meta", + "data" + ], + "properties": { + "meta": { + "$ref": "#/components/schemas/OCSMeta" + }, + "data": {} + } + } + } + } + } + } + } + } + } + }, "/ocs/v2.php/apps/sharing/api/v1/share/{id}/permission/preset": { "put": { "operationId": "api_v1-select-share-permission-preset", diff --git a/apps/sharing/tests/Command/CommandTest.php b/apps/sharing/tests/Command/CommandTest.php index 6a553e5b337c4..0abfac47dfb1f 100644 --- a/apps/sharing/tests/Command/CommandTest.php +++ b/apps/sharing/tests/Command/CommandTest.php @@ -33,6 +33,7 @@ use OCA\Sharing\Command\SharingBase; use OCA\Sharing\Command\UpdateSharePermission; use OCA\Sharing\Command\UpdateShareProperty; +use OCA\Sharing\Command\UpdateShareRecipientPermission; use OCA\Sharing\Command\UpdateShareRecipientSecret; use OCA\Sharing\Command\UpdateShareState; use OCA\Sharing\Command\UpdateShareUserStatus; @@ -73,6 +74,7 @@ public function setUp(): void { RemoveShareSource::class, SelectSharePermissionPreset::class, UpdateSharePermission::class, + UpdateShareRecipientPermission::class, UpdateShareProperty::class, UpdateShareRecipientSecret::class, UpdateShareState::class, @@ -363,6 +365,28 @@ protected function updateSharePermission(ShareAccessContext $accessContext, Shar return json_decode($stdout, true, 512, JSON_THROW_ON_ERROR); } + /** + * @return SharingShare + */ + #[Override] + protected function updateShareRecipientPermission(ShareAccessContext $accessContext, Share $share, ShareRecipient $recipient, SharePermission $permission): array { + $stdout = $this->runCommand( + $accessContext, + UpdateShareRecipientPermission::class, + [ + ['id', $share->id], + ['permission-class', $permission->class], + ['permission-enabled', $permission->enabled ? 'true' : 'false'], + ['recipient-class', $recipient->class], + ['recipient-value', $recipient->value], + ['recipient-instance', $recipient->instance], + ], + [], + ); + /** @var SharingShare */ + return json_decode($stdout, true, 512, JSON_THROW_ON_ERROR); + } + /** * @return SharingShare */ diff --git a/apps/sharing/tests/Controller/ApiV1ControllerTest.php b/apps/sharing/tests/Controller/ApiV1ControllerTest.php index f005c758e2988..ef15c6c01d350 100644 --- a/apps/sharing/tests/Controller/ApiV1ControllerTest.php +++ b/apps/sharing/tests/Controller/ApiV1ControllerTest.php @@ -162,6 +162,12 @@ protected function updateSharePermission(ShareAccessContext $accessContext, Shar return $this->executeRequest($accessContext, fn (ApiV1Controller $controller): DataResponse => $controller->updateSharePermission($share->id, $permission->class, $permission->enabled)); } + #[Override] + protected function updateShareRecipientPermission(ShareAccessContext $accessContext, Share $share, ShareRecipient $recipient, SharePermission $permission): array { + /** @var SharingShare */ + return $this->executeRequest($accessContext, fn (ApiV1Controller $controller): DataResponse => $controller->updateShareRecipientPermission($share->id, $recipient->class, $recipient->value, $recipient->instance, $permission->class, $permission->enabled)); + } + #[Override] protected function selectSharePermissionPreset(ShareAccessContext $accessContext, Share $share, string $permissionPresetClass): array { /** @psalm-suppress ArgumentTypeCoercion */ diff --git a/lib/private/Sharing/SharingBackend.php b/lib/private/Sharing/SharingBackend.php index 71a59b93c2db9..eed53432aa6a7 100644 --- a/lib/private/Sharing/SharingBackend.php +++ b/lib/private/Sharing/SharingBackend.php @@ -458,6 +458,52 @@ public function updateSharePermission(string $id, SharePermission $permission): } } + #[\Override] + public function updateShareRecipientPermission(string $id, ShareRecipient $recipient, SharePermission $permission): void { + $qb = $this->connection->getQueryBuilder(); + $result = $qb + ->select('id') + ->from('sharing_share_recipients') + ->where($qb->expr()->eq('share_id', $qb->createNamedParameter($id))) + ->andWhere( + $qb->expr()->eq('recipient_class_id', $qb->createNamedParameter($this->classMapper->getClassId($recipient->class), IQueryBuilder::PARAM_INT)) + ) + ->andWhere($qb->expr()->eq('recipient_value', $qb->createNamedParameter($recipient->value))) + ->andWhere( + $recipient->instance === null + ? $qb->expr()->isNull('recipient_instance') + : $qb->expr()->eq('recipient_instance', $qb->createNamedParameter($recipient->instance)) + ) + ->executeQuery(); + + /** @var int|false $recipientId */ + $recipientId = $result->fetchOne(); + if ($recipientId === false) { + throw new ShareNotFoundException(); + } + + $qb = $this->connection->getQueryBuilder(); + $rowCount = $qb + ->update('sharing_share_recipient_permissions') + ->set('permission_enabled', $qb->createNamedParameter($permission->enabled, IQueryBuilder::PARAM_BOOL)) + ->where($qb->expr()->eq('recipient_id', $qb->createNamedParameter($recipientId))) + ->andWhere( + $qb->expr()->eq('permission_class_id', $qb->createNamedParameter($this->classMapper->getClassId($permission->class), IQueryBuilder::PARAM_INT)) + ) + ->executeStatement(); + if ($rowCount === 0) { + $qb = $this->connection->getQueryBuilder(); + $qb + ->insert('sharing_share_recipient_permissions') + ->values([ + 'recipient_id' => $qb->createNamedParameter($recipientId), + 'permission_class_id' => $qb->createNamedParameter($this->classMapper->getClassId($permission->class), IQueryBuilder::PARAM_INT), + 'permission_enabled' => $qb->createNamedParameter($permission->enabled, IQueryBuilder::PARAM_BOOL), + ]) + ->executeStatement(); + } + } + #[\Override] public function selectSharePermissionPreset(string $id, string $permissionPresetClass): void { $qb = $this->connection->getQueryBuilder(); @@ -865,9 +911,15 @@ private function list( } } + $shareCompatiblePermissionTypeClasses = array_map( + fn (array $shareData): array => array_flip($this->getShareCompatiblePermissionTypeClasses($shareData['sources'])), + $shares + ); + $registryRecipientTypes = $this->registry->getRecipientTypes(); /** @var array, bool>> $shareRecipientTypeClasses */ $shareRecipientTypeClasses = []; + $recipientIds = []; foreach ($chunks as $chunk) { $qb = $this->connection->getQueryBuilder(); $qb @@ -925,10 +977,63 @@ private function list( $initiatorUserId, $initiatorInstance, ), + [], ); $shareRecipientTypeClasses[$shareId] ??= []; $shareRecipientTypeClasses[$shareId][$typeClass] = true; + + $recipientIds[] = $recipientId; + } + } + + /** @var array, SharePermission>>> $shareRecipientPermissions */ + $shareRecipientPermissions = []; + foreach (array_chunk($recipientIds, 1000) as $chunk) { + $qb = $this->connection->getQueryBuilder(); + $result = $qb + ->select( + 'sr.share_id', + 'srp.recipient_id', + 'srp.permission_class_id', + 'srp.permission_enabled', + ) + ->from('sharing_share_recipient_permissions', 'srp') + ->innerJoin('srp', 'sharing_share_recipients', 'sr', $qb->expr()->eq('sr.id', 'srp.recipient_id')) + ->where($qb->expr()->in('srp.recipient_id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY))) + ->executeQuery(); + + foreach ($result->fetchAll() as $row) { + /** @var int $shareId */ + $shareId = $row['share_id']; + + /** @var class-string $permissionTypeClass */ + $permissionTypeClass = $this->classMapper->getClassName((int)$row['permission_class_id']); + if (!isset($shareCompatiblePermissionTypeClasses[$shareId][$permissionTypeClass])) { + // Skip permissions that are currently not compatible, but don't remove them. + continue; + } + + /** @var int $recipientId */ + $recipientId = $row['recipient_id']; + + $shareRecipientPermissions[$shareId] ??= []; + $shareRecipientPermissions[$shareId][$recipientId] ??= []; + $shareRecipientPermissions[$shareId][$recipientId][$permissionTypeClass] = new SharePermission($permissionTypeClass, (bool)$row['permission_enabled']); + } + } + + foreach ($shareRecipientPermissions as $shareId => $recipientPermissions) { + foreach ($recipientPermissions as $recipientId => $permissions) { + $recipient = $shares[$shareId]['recipients'][$recipientId]; + $shares[$shareId]['recipients'][$recipientId] = new ShareRecipient( + $recipient->class, + $recipient->value, + $recipient->instance, + $recipient->secret, + $recipient->initiator, + $permissions, + ); } } @@ -1040,11 +1145,6 @@ private function list( } } - $shareCompatiblePermissionTypeClasses = array_map( - fn (array $shareData): array => array_flip($this->getShareCompatiblePermissionTypeClasses($shareData['sources'])), - $shares - ); - foreach ($chunks as $chunk) { $qb = $this->connection->getQueryBuilder(); $qb diff --git a/lib/private/Sharing/SharingManager.php b/lib/private/Sharing/SharingManager.php index f0bdb59a76ab2..25d59e58a2378 100644 --- a/lib/private/Sharing/SharingManager.php +++ b/lib/private/Sharing/SharingManager.php @@ -373,7 +373,8 @@ public function addShareRecipient(ShareAccessContext $accessContext, Share $shar try { $this->validateShareEditPermissions($accessContext, $share); } catch (ShareOperationForbiddenException) { - $this->validatePermission($share, ReshareSharePermissionType::class); + // Only check that we have reshare permission, because we can't check if we're the initiator for a non-existent recipient that we're about to add. + $this->validatePermission($accessContext, $share, ReshareSharePermissionType::class); } if (($recipientType = $this->registry->getRecipientTypes()[$recipient->class] ?? null) === null) { @@ -674,6 +675,63 @@ public function updateSharePermission(ShareAccessContext $accessContext, Share $ return $share; } + #[\Override] + public function updateShareRecipientPermission(ShareAccessContext $accessContext, Share $share, ShareRecipient $recipient, SharePermission $permission): Share { + $this->assertInTransaction(); + + $time = $this->getTime(); + $this->backend->setLastUpdated([$share->id], $time); + + try { + $this->validateShareEditPermissions($accessContext, $share); + } catch (ShareOperationForbiddenException) { + $this->validateReshareOperation($accessContext, $share, $recipient); + } + + if ($permission->enabled) { + $enabledPermissions = $share->getEffectiveEnabledPermissions($accessContext); + if (!isset($enabledPermissions[$permission->class])) { + throw new ShareOperationForbiddenException(); + } + } + + $this->backend->updateShareRecipientPermission($share->id, $recipient, $permission); + + $recipients = $share->recipients; + foreach ($recipients as &$shareRecipient) { + if ($shareRecipient->class === $recipient->class && $shareRecipient->value === $recipient->value && $shareRecipient->instance === $recipient->instance) { + $permissions = $shareRecipient->permissions; + $permissions[$permission->class] = $permission; + + $shareRecipient = new ShareRecipient( + $shareRecipient->class, + $shareRecipient->value, + $shareRecipient->instance, + $shareRecipient->secret, + $shareRecipient->initiator, + $permissions, + ); + + break; + } + } + + $share = new Share( + $share->id, + $share->owner, + $time, + $share->state, + $share->userStatus, + $share->sources, + $recipients, + $share->properties, + $share->permissions, + ); + + [$share] = $this->processShareUpdates([$share]); + return $share; + } + #[\Override] public function selectSharePermissionPreset(ShareAccessContext $accessContext, Share $share, string $permissionPresetClass): Share { $this->assertInTransaction(); @@ -817,19 +875,17 @@ private function validateShareEditPermissions(ShareAccessContext $accessContext, * @param class-string $permissionTypeClass * @throws ShareOperationForbiddenException */ - private function validatePermission(Share $share, string $permissionTypeClass): void { - if ((($permission = $share->permissions[$permissionTypeClass] ?? null) !== null) && $permission->enabled) { - return; + private function validatePermission(ShareAccessContext $accessContext, Share $share, string $permissionTypeClass): void { + if (!isset($share->getEffectiveEnabledPermissions($accessContext)[$permissionTypeClass])) { + throw new ShareOperationForbiddenException(); } - - throw new ShareOperationForbiddenException(); } /** * @throws ShareOperationForbiddenException */ private function validateReshareOperation(ShareAccessContext $accessContext, Share $share, ShareRecipient $recipient): void { - $this->validatePermission($share, ReshareSharePermissionType::class); + $this->validatePermission($accessContext, $share, ReshareSharePermissionType::class); foreach ($share->recipients as $shareRecipient) { if ( @@ -851,7 +907,7 @@ private function validateReshareOperation(ShareAccessContext $accessContext, Sha */ private function validateInteraction(ShareAccessContext $accessContext, Share $share): void { $action = new ShareAction( - null, array_values(array_map(static fn (SharePermission $permission): string => $permission->class, $share->getEnabledPermissions())) + null, array_values(array_map(static fn (SharePermission $permission): string => $permission->class, $share->getEffectiveEnabledPermissions($accessContext))) ); $usersToCheck = []; @@ -913,7 +969,7 @@ private function assertShareCanBeActive(Share $share): void { throw new ShareInvalidException('No recipient set.', $this->l10n->t('You need to add at least one recipient to make the share available.')); } - if ($share->getEnabledPermissions() === []) { + if ($share->getEffectiveEnabledPermissions(new ShareAccessContext(overrideChecks: true)) === []) { throw new ShareInvalidException('No permission given.', $this->l10n->t('You need to allow at least one permission to make the share available.')); } diff --git a/lib/unstable/Sharing/ISharingBackend.php b/lib/unstable/Sharing/ISharingBackend.php index ce3ce7572428c..1d1f820886969 100644 --- a/lib/unstable/Sharing/ISharingBackend.php +++ b/lib/unstable/Sharing/ISharingBackend.php @@ -136,6 +136,14 @@ public function updateShareProperty(string $id, ShareProperty $property): ?strin */ public function updateSharePermission(string $id, SharePermission $permission): void; + /** + * Update a permission for a recipient of a share. + * + * @throws ShareNotFoundException + * @experimental 35.0.0 + */ + public function updateShareRecipientPermission(string $id, ShareRecipient $recipient, SharePermission $permission): void; + /** * Select a permission preset for a share. * diff --git a/lib/unstable/Sharing/ISharingManager.php b/lib/unstable/Sharing/ISharingManager.php index e6dbe3a749bbf..044cf78f0202b 100644 --- a/lib/unstable/Sharing/ISharingManager.php +++ b/lib/unstable/Sharing/ISharingManager.php @@ -179,6 +179,15 @@ public function updateShareProperty(ShareAccessContext $accessContext, Share $sh */ public function updateSharePermission(ShareAccessContext $accessContext, Share $share, SharePermission $permission): Share; + /** + * Update a permission for a recipient of a share. + * + * @throws ShareNotFoundException + * @throws ShareOperationForbiddenException + * @experimental 35.0.0 + */ + public function updateShareRecipientPermission(ShareAccessContext $accessContext, Share $share, ShareRecipient $recipient, SharePermission $permission): Share; + /** * Select a permission preset for a share. * diff --git a/lib/unstable/Sharing/Recipient/ShareRecipient.php b/lib/unstable/Sharing/Recipient/ShareRecipient.php index ca615a83e774b..7f3aa46c1b69c 100644 --- a/lib/unstable/Sharing/Recipient/ShareRecipient.php +++ b/lib/unstable/Sharing/Recipient/ShareRecipient.php @@ -11,6 +11,8 @@ use NCU\Sharing\Icon\ShareIconURL; use NCU\Sharing\ISharingRegistry; +use NCU\Sharing\Permission\ISharePermissionType; +use NCU\Sharing\Permission\SharePermission; use NCU\Sharing\Share; use NCU\Sharing\ShareUser; use OCP\AppFramework\Attribute\Consumable; @@ -24,26 +26,39 @@ * @experimental 35.0.0 */ #[Consumable(since: '35.0.0')] -final readonly class ShareRecipient { +final class ShareRecipient { + /** @var array, SharePermission> $disabledPermissions */ + private ?array $disabledPermissions = null; + /** * @experimental 35.0.0 */ public function __construct( /** @var class-string $class */ - public string $class, + public readonly string $class, /** @var non-empty-string $value */ - public string $value, + public readonly string $value, /** @var ?non-empty-string $instance */ - public ?string $instance, + public readonly ?string $instance, /** @var ?non-empty-string $secret */ - public ?string $secret = null, - public ?ShareUser $initiator = null, + public readonly ?string $secret = null, + public readonly ?ShareUser $initiator = null, + /** @var array, SharePermission> $permissions */ + public readonly array $permissions = [], ) { if ($instance !== null && !preg_match('/^https?:\/\/.+/', $instance)) { throw new RuntimeException('The instance is not a valid absolute URL: ' . $instance); } } + /** + * @return array, SharePermission> + * @experimental 35.0.0 + */ + public function getDisabledPermissions(): array { + return $this->disabledPermissions ??= array_filter($this->permissions, static fn (SharePermission $permission): bool => !$permission->enabled); + } + /** * @return SharingRecipient * @experimental 35.0.0 @@ -92,6 +107,7 @@ public function format(ISharingRegistry $registry, IFactory $l10nFactory, IURLGe 'icon' => $icon->format(), 'secret' => $secret, 'initiator' => $this->initiator?->format($userManager), + 'permissions' => SharePermission::formatMultiple($registry, $l10nFactory, array_values($this->permissions)), ]; } @@ -110,8 +126,8 @@ public static function formatMultiple(ISharingRegistry $registry, IFactory $l10n ++$recipientDisplayNames[$displayName]; } - // First sort by instance, then by class and finally by value to get a stable order regardless of the DB order - usort($recipients, static fn (ShareRecipient $a, ShareRecipient $b): int => 4 * ($a->instance === null ? -1 : ($a->instance <=> $b->instance)) + 2 * ($a->class <=> $b->class) + ($a->value <=> $b->value)); + // First sort by least amount of disabled permissions, then by instance, then by class and finally by value to get a stable order regardless of the DB order + usort($recipients, static fn (ShareRecipient $a, ShareRecipient $b): int => 8 * (count($a->getDisabledPermissions()) <=> count($b->getDisabledPermissions())) + 4 * ($a->instance === null ? -1 : ($a->instance <=> $b->instance)) + 2 * ($a->class <=> $b->class) + ($a->value <=> $b->value)); return array_map(static fn (ShareRecipient $recipient): array => $recipient->format($registry, $l10nFactory, $urlGenerator, $userManager, $recipientDisplayNames[$recipientTypes[$recipient->class]?->getRecipientDisplayName($recipient->value) ?? $recipient->value] === 1), $recipients); } diff --git a/lib/unstable/Sharing/Share.php b/lib/unstable/Sharing/Share.php index 7120033d4d5c0..bf446e321a741 100644 --- a/lib/unstable/Sharing/Share.php +++ b/lib/unstable/Sharing/Share.php @@ -23,6 +23,7 @@ use OCP\IURLGenerator; use OCP\IUserManager; use OCP\L10N\IFactory; +use OCP\Server; /** * Keep the following types in sync with apps/sharing/lib/ResponseDefinitions.php: @@ -55,6 +56,16 @@ * icon: SharingIcon, * } * + * @psalm-type SharingPermission = array{ + * class: class-string, + * source_class: ?class-string, + * display_name: non-empty-string, + * hint: ?non-empty-string, + * priority: int<1, 100>, + * presets: list>, + * enabled: bool, + * } + * * @psalm-type SharingRecipient = array{ * class: class-string, * value: non-empty-string, @@ -67,6 +78,7 @@ * url?: non-empty-string, * }, * initiator: ?SharingUser, + * permissions: list * } * * @psalm-type SharingState = 'active'|'draft'|'deleted' @@ -116,16 +128,6 @@ * hint: ?non-empty-string, * } * - * @psalm-type SharingPermission = array{ - * class: class-string, - * source_class: ?class-string, - * display_name: non-empty-string, - * hint: ?non-empty-string, - * priority: int<1, 100>, - * presets: list>, - * enabled: bool, - * } - * * @psalm-type SharingSourceType = array{ * class: class-string, * } @@ -148,8 +150,11 @@ */ #[Consumable(since: '35.0.0')] final class Share { - /** @var array, SharePermission> $enabledPermissions */ - private ?array $enabledPermissions = null; + /** @var array> $recipientsCache */ + private array $recipientsCache = []; + + /** @var array, SharePermission>> $enabledPermissionsCache */ + private array $enabledPermissionsCache = []; /** * @experimental 35.0.0 @@ -172,19 +177,82 @@ public function __construct( ) { } + /** + * @return list + * @experimental 35.0.0 + */ + public function getEffectiveRecipients(ShareAccessContext $accessContext): array { + $hash = $accessContext->getHash(); + if (($recipients = $this->recipientsCache[$hash] ?? null) !== null) { + return $recipients; + } + + $registry = Server::get(ISharingRegistry::class); + + /** @var array, list> $recipientTypeValues */ + $recipientTypeValues = []; + foreach ($registry->getRecipientTypes() as $recipientType) { + $recipientValues = $recipientType->getRecipients($accessContext->currentUser, $accessContext->arguments[$recipientType::class] ?? null); + if ($recipientValues !== []) { + $recipientTypeValues[$recipientType::class] = $recipientValues; + } + } + + /** @var list $recipients */ + $recipients = []; + foreach ($this->recipients as $recipient) { + if ( + // Remote recipient can only be accessed through their secret + ($accessContext->secret !== null && $accessContext->secret === $recipient->secret) + // Recipient values are only valid for local recipients + || ($recipient->instance === null && in_array($recipient->value, $recipientTypeValues[$recipient->class] ?? [], true))) { + $recipients[] = $recipient; + } + } + + return $this->recipientsCache[$hash] = $recipients; + } + /** * @return array, SharePermission> * @experimental 35.0.0 */ - public function getEnabledPermissions(): array { - return $this->enabledPermissions ??= array_filter($this->permissions, static fn (SharePermission $permission): bool => $permission->enabled); + public function getEffectiveEnabledPermissions(ShareAccessContext $accessContext): array { + $hash = $accessContext->getHash(); + if (($enabledPermissions = $this->enabledPermissionsCache[$hash] ?? null) !== null) { + return $enabledPermissions; + } + + $enabledPermissions = array_filter($this->permissions, static fn (SharePermission $permission): bool => $permission->enabled); + + if (!$accessContext->overrideChecks && !$this->owner->isCurrentUser($accessContext)) { + /** @var array, SharePermission> $recipientsEnabledPermissions */ + $recipientsEnabledPermissions = []; + foreach ($this->getEffectiveRecipients($accessContext) as $recipient) { + foreach ($recipient->permissions as $permission) { + // If any recipient has the permission enabled, we grant it. + if (!isset($recipientsEnabledPermissions[$permission->class]) || ($permission->enabled && !$recipientsEnabledPermissions[$permission->class]->enabled)) { + $recipientsEnabledPermissions[$permission->class] = $permission; + } + } + } + + foreach ($recipientsEnabledPermissions as $permission) { + // If the permission was disabled by any recipient and not enabled by another, we deny it. + if (!$permission->enabled) { + unset($enabledPermissions[$permission->class]); + } + } + } + + return $this->enabledPermissionsCache[$hash] = $enabledPermissions; } /** * @return SharingShare * @experimental 35.0.0 */ - public function format(ISharingRegistry $registry, IFactory $l10nFactory, IURLGenerator $urlGenerator, IUserManager $userManager): array { + public function format(ISharingRegistry $registry, IFactory $l10nFactory, IURLGenerator $urlGenerator, IUserManager $userManager, ShareAccessContext $accessContext): array { $registrySourceTypePermissionTypeClasses = $registry->getSourceTypePermissionTypeClasses(); $registryGenericPermissionTypeClasses = $registry->getGenericPermissionTypeClasses(); $registryPermissionTypeCompatiblePermissionPresetClasses = $registry->getPermissionTypeCompatiblePermissionPresetClasses(); @@ -205,7 +273,7 @@ public function format(ISharingRegistry $registry, IFactory $l10nFactory, IURLGe $selectedPermissionPresetClass = null; - $enabledPermissionTypeClasses = array_values(array_map(static fn (SharePermission $permission): string => $permission->class, $this->getEnabledPermissions())); + $enabledPermissionTypeClasses = array_values(array_map(static fn (SharePermission $permission): string => $permission->class, $this->getEffectiveEnabledPermissions($accessContext))); sort($enabledPermissionTypeClasses); $requiredPermissionTypeClasses = []; @@ -252,9 +320,9 @@ public function format(ISharingRegistry $registry, IFactory $l10nFactory, IURLGe * @return list * @experimental 35.0.0 */ - public static function formatMultiple(ISharingRegistry $registry, IFactory $l10nFactory, IURLGenerator $urlGenerator, IUserManager $userManager, array $shares): array { - // First sort by priority and then sort by share id to get a stable order regardless of the DB order - usort($shares, static fn (Share $a, Share $b): int => 2 * (count($b->getEnabledPermissions()) <=> count($a->getEnabledPermissions())) + ($a->id <=> $b->id)); - return array_map(static fn (Share $share): array => $share->format($registry, $l10nFactory, $urlGenerator, $userManager), $shares); + public static function formatMultiple(ISharingRegistry $registry, IFactory $l10nFactory, IURLGenerator $urlGenerator, IUserManager $userManager, ShareAccessContext $accessContext, array $shares): array { + // First sort by number of enabled permissions and then sort by share id to get a stable order regardless of the DB order + usort($shares, static fn (Share $a, Share $b): int => 2 * (count($b->getEffectiveEnabledPermissions($accessContext)) <=> count($a->getEffectiveEnabledPermissions($accessContext))) + ($a->id <=> $b->id)); + return array_map(static fn (Share $share): array => $share->format($registry, $l10nFactory, $urlGenerator, $userManager, $accessContext), $shares); } } diff --git a/lib/unstable/Sharing/ShareAccessContext.php b/lib/unstable/Sharing/ShareAccessContext.php index 92ea745359e5a..e237046e4962f 100644 --- a/lib/unstable/Sharing/ShareAccessContext.php +++ b/lib/unstable/Sharing/ShareAccessContext.php @@ -33,4 +33,11 @@ public function __construct( public bool $overrideChecks = false, ) { } + + /** + * @experimental 35.0.0 + */ + public function getHash(): string { + return hash('xxh128', serialize([$this->currentUser?->getUID(), $this->secret, $this->arguments, $this->overrideChecks])); + } } diff --git a/openapi.json b/openapi.json index 9d2384c6b63d4..f1772d43d0bee 100644 --- a/openapi.json +++ b/openapi.json @@ -4653,7 +4653,8 @@ "display_name", "icon", "secret", - "initiator" + "initiator", + "permissions" ], "properties": { "class": { @@ -4707,6 +4708,12 @@ "$ref": "#/components/schemas/SharingUser" } ] + }, + "permissions": { + "type": "array", + "items": { + "$ref": "#/components/schemas/SharingPermission" + } } } }, @@ -40176,6 +40183,207 @@ } } }, + "/ocs/v2.php/apps/sharing/api/v1/share/{id}/recipient/permission": { + "put": { + "operationId": "sharing-api_v1-update-share-recipient-permission", + "summary": "Update a permission for a recipient of a share.", + "tags": [ + "sharing/api_v1" + ], + "security": [ + { + "bearer_auth": [] + }, + { + "basic_auth": [] + } + ], + "requestBody": { + "required": true, + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "recipientClass", + "recipientValue", + "permissionClass", + "enabled" + ], + "properties": { + "recipientClass": { + "type": "string", + "description": "Type class of the recipient", + "minLength": 1 + }, + "recipientValue": { + "type": "string", + "description": "Value of the recipient", + "minLength": 1 + }, + "recipientInstance": { + "type": "string", + "nullable": true, + "description": "Instance of the recipient", + "minLength": 1 + }, + "permissionClass": { + "type": "string", + "description": "Type class of the permission", + "minLength": 1 + }, + "enabled": { + "type": "boolean", + "description": "Enabled state of the permission" + } + } + } + } + } + }, + "parameters": [ + { + "name": "id", + "in": "path", + "description": "ID of the share", + "required": true, + "schema": { + "type": "string" + } + }, + { + "name": "OCS-APIRequest", + "in": "header", + "description": "Required to be true for the API request to pass", + "required": true, + "schema": { + "type": "boolean", + "default": true + } + } + ], + "responses": { + "200": { + "description": "Share recipient permission updated successfully", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "ocs" + ], + "properties": { + "ocs": { + "type": "object", + "required": [ + "meta", + "data" + ], + "properties": { + "meta": { + "$ref": "#/components/schemas/OCSMeta" + }, + "data": { + "$ref": "#/components/schemas/SharingShare" + } + } + } + } + } + } + } + }, + "403": { + "description": "Updating the share recipient permission is not allowed", + "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": { + "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": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "ocs" + ], + "properties": { + "ocs": { + "type": "object", + "required": [ + "meta", + "data" + ], + "properties": { + "meta": { + "$ref": "#/components/schemas/OCSMeta" + }, + "data": {} + } + } + } + } + } + } + } + } + } + }, "/ocs/v2.php/apps/sharing/api/v1/share/{id}/permission/preset": { "put": { "operationId": "sharing-api_v1-select-share-permission-preset", diff --git a/tests/lib/Sharing/AbstractSharingManagerTests.php b/tests/lib/Sharing/AbstractSharingManagerTests.php index 1e8ccd5361c38..83c0d654b7011 100644 --- a/tests/lib/Sharing/AbstractSharingManagerTests.php +++ b/tests/lib/Sharing/AbstractSharingManagerTests.php @@ -96,6 +96,11 @@ abstract protected function updateShareProperty(ShareAccessContext $accessContex */ abstract protected function updateSharePermission(ShareAccessContext $accessContext, Share $share, SharePermission $permission): array; + /** + * @return SharingShare + */ + abstract protected function updateShareRecipientPermission(ShareAccessContext $accessContext, Share $share, ShareRecipient $recipient, SharePermission $permission): array; + /** * @return SharingShare */ @@ -205,6 +210,7 @@ public function setUp(): void { new TestShareRecipientType2( [ 'recipient2' => 'Recipient 2', + 'recipient3' => 'Recipient 3', ], [ $this->user2->getUID() => ['recipient2'], @@ -256,6 +262,7 @@ protected function tearDown(): void { 'sharing_share_permissions', 'sharing_share_properties', 'sharing_share_recipients', + 'sharing_share_recipient_permissions', 'sharing_share_sources', 'sharing_share_user_status', ] as $table) { @@ -326,6 +333,7 @@ public function testSearchRecipients(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], [ 'class' => TestShareRecipientType1::class, @@ -339,6 +347,7 @@ public function testSearchRecipients(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], [ 'class' => TestShareRecipientType1::class, @@ -352,6 +361,7 @@ public function testSearchRecipients(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], [ 'class' => TestShareRecipientType2::class, @@ -365,6 +375,7 @@ public function testSearchRecipients(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], [ 'class' => TestShareRecipientType2::class, @@ -378,6 +389,7 @@ public function testSearchRecipients(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], [ 'class' => TestShareRecipientType2::class, @@ -391,6 +403,7 @@ public function testSearchRecipients(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], ], $this->searchRecipients($accessContext, null, 'recipient', 10, 0)); @@ -407,6 +420,7 @@ public function testSearchRecipients(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], [ 'class' => TestShareRecipientType1::class, @@ -420,6 +434,7 @@ public function testSearchRecipients(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], [ 'class' => TestShareRecipientType1::class, @@ -433,6 +448,7 @@ public function testSearchRecipients(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], ], $this->searchRecipients($accessContext, [TestShareRecipientType1::class], 'recipient', 10, 0)); @@ -449,6 +465,7 @@ public function testSearchRecipients(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], ], $this->searchRecipients($accessContext, [TestShareRecipientType1::class], 'recipient', 1, 0)); @@ -465,6 +482,7 @@ public function testSearchRecipients(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], [ 'class' => TestShareRecipientType1::class, @@ -478,6 +496,7 @@ public function testSearchRecipients(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], ], $this->searchRecipients($accessContext, [TestShareRecipientType1::class], 'recipient', 10, 1)); } @@ -524,6 +543,7 @@ public function testSearchRecipientsUniqueDisplayNames(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], [ 'class' => TestShareRecipientType2::class, @@ -537,6 +557,7 @@ public function testSearchRecipientsUniqueDisplayNames(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], [ 'class' => TestShareRecipientType2::class, @@ -550,6 +571,7 @@ public function testSearchRecipientsUniqueDisplayNames(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], ], $this->searchRecipients($accessContext, null, 'recipient', 10, 0)); } @@ -585,6 +607,7 @@ public function testSearchRecipientsIcons(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], [ 'class' => TestShareRecipientType1::class, @@ -599,6 +622,7 @@ public function testSearchRecipientsIcons(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], ], $this->searchRecipients($accessContext, null, 'icon', 10, 0)); } @@ -623,6 +647,7 @@ public function testSearchRecipientsOmitExisting(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], [ 'class' => TestShareRecipientType2::class, @@ -636,6 +661,7 @@ public function testSearchRecipientsOmitExisting(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], ], $this->searchRecipients($accessContext, null, 'recipient', 3, 0, $share)); @@ -656,6 +682,7 @@ public function testSearchRecipientsOmitExisting(): void { 'updatable' => false, ], 'initiator' => null, + 'permissions' => [], ], ], $this->searchRecipients($accessContext, null, 'recipient', 3, 0, $share)); } @@ -955,6 +982,7 @@ public function testAddShareRecipient(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], $formatted['recipients']); } @@ -1057,6 +1085,7 @@ public function testAddChildShareRecipientWithResharePermission(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], [ 'class' => TestShareRecipientType2::class, @@ -1078,6 +1107,7 @@ public function testAddChildShareRecipientWithResharePermission(): void { 'dark' => 'http://localhost/index.php/avatar/user1/64/dark', ], ], + 'permissions' => [], ], ], $formatted['recipients']); } @@ -1124,6 +1154,7 @@ public function testRemoveShareRecipient(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], $formatted['recipients']); @@ -1256,6 +1287,7 @@ public function testRemoveChildShareRecipientWithResharePermission(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], $formatted['recipients']); } @@ -1422,6 +1454,7 @@ public function testUpdateShareRecipientSecret(bool $isSecretUpdatable): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], $formatted['recipients']); } @@ -1853,6 +1886,159 @@ public function testUpdateSharePermissionInteractionRestricted(): void { $eventDispatcher->removeListener(RestrictInteractionEvent::class, $listener); } + public function testUpdateShareRecipientPermission(): void { + $accessContext = new ShareAccessContext($this->owner); + + $this->dbConnection->beginTransaction(); + $share = $this->manager->createShare($accessContext); + $share = $this->manager->addShareSource($accessContext, $share, new ShareSource(TestShareSourceType1::class, 'source1')); + $share = $this->manager->addShareRecipient($accessContext, $share, new ShareRecipient(TestShareRecipientType1::class, 'recipient1', null)); + $share = $this->manager->updateSharePermission($accessContext, $share, new SharePermission(ReshareSharePermissionType::class, true)); + $share = $this->manager->updateShareState($accessContext, $share, ShareState::Active); + + $this->dbConnection->commit(); + + $accessContext1 = new ShareAccessContext($this->user1); + $recipient2 = new ShareRecipient(TestShareRecipientType2::class, 'recipient2', null); + $this->dbConnection->beginTransaction(); + $share1 = $this->manager->getShare($accessContext1, $share->id); + $share1 = $this->manager->addShareRecipient($accessContext1, $share1, $recipient2); + + $this->dbConnection->commit(); + $formatted = $this->updateShareRecipientPermission($accessContext1, $share1, $recipient2, new SharePermission(ReshareSharePermissionType::class, false)); + $this->assertEquals([ + [ + 'class' => TestShareRecipientType1::class, + 'value' => 'recipient1', + 'instance' => null, + 'display_name' => 'Recipient 1', + 'icon' => [ + 'svg' => '', + ], + 'secret' => [ + 'updatable' => false, + ], + 'initiator' => [ + 'user_id' => 'owner', + 'instance' => null, + 'display_name' => 'Owner', + 'icon' => [ + 'light' => 'http://localhost/index.php/avatar/owner/64', + 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', + ], + ], + 'permissions' => [], + ], + [ + 'class' => TestShareRecipientType2::class, + 'value' => 'recipient2', + 'instance' => null, + 'display_name' => 'Recipient 2', + 'icon' => [ + 'svg' => '', + ], + 'secret' => [ + 'updatable' => false, + ], + 'initiator' => [ + 'user_id' => 'user1', + 'instance' => null, + 'display_name' => 'User 1', + 'icon' => [ + 'light' => 'http://localhost/index.php/avatar/user1/64', + 'dark' => 'http://localhost/index.php/avatar/user1/64/dark', + ], + ], + 'permissions' => [ + [ + 'class' => ReshareSharePermissionType::class, + 'source_class' => null, + 'display_name' => 'Share with others', + 'hint' => null, + 'presets' => [], + 'enabled' => false, + 'priority' => 90, + ], + ], + ], + ], $formatted['recipients']); + + $accessContext2 = new ShareAccessContext($this->user2); + $this->dbConnection->beginTransaction(); + $share2 = $this->manager->getShare($accessContext2, $share->id); + try { + $this->manager->addShareRecipient($accessContext2, $share2, new ShareRecipient(TestShareRecipientType2::class, 'recipient3', null)); + $this->fail('Adding share recipient without reshare permission should fail.'); + } catch (HintException $hintException) { + $this->assertEquals('You are not allowed to edit this share.', $hintException->getHint()); + } finally { + $this->dbConnection->commit(); + } + + $formatted = $this->updateShareRecipientPermission($accessContext1, $share1, $recipient2, new SharePermission(ReshareSharePermissionType::class, true)); + $this->assertEquals([ + [ + 'class' => TestShareRecipientType1::class, + 'value' => 'recipient1', + 'instance' => null, + 'display_name' => 'Recipient 1', + 'icon' => [ + 'svg' => '', + ], + 'secret' => [ + 'updatable' => false, + ], + 'initiator' => [ + 'user_id' => 'owner', + 'instance' => null, + 'display_name' => 'Owner', + 'icon' => [ + 'light' => 'http://localhost/index.php/avatar/owner/64', + 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', + ], + ], + 'permissions' => [], + ], + [ + 'class' => TestShareRecipientType2::class, + 'value' => 'recipient2', + 'instance' => null, + 'display_name' => 'Recipient 2', + 'icon' => [ + 'svg' => '', + ], + 'secret' => [ + 'updatable' => false, + ], + 'initiator' => [ + 'user_id' => 'user1', + 'instance' => null, + 'display_name' => 'User 1', + 'icon' => [ + 'light' => 'http://localhost/index.php/avatar/user1/64', + 'dark' => 'http://localhost/index.php/avatar/user1/64/dark', + ], + ], + 'permissions' => [ + [ + 'class' => ReshareSharePermissionType::class, + 'source_class' => null, + 'display_name' => 'Share with others', + 'hint' => null, + 'presets' => [], + 'enabled' => true, + 'priority' => 90, + ], + ], + ], + ], $formatted['recipients']); + + $this->dbConnection->beginTransaction(); + $share2 = $this->manager->getShare($accessContext2, $share->id); + $this->manager->addShareRecipient($accessContext2, $share2, new ShareRecipient(TestShareRecipientType2::class, 'recipient3', null)); + $this->dbConnection->commit(); + } + public function testSelectSharePermissionPreset(): void { $this->registry->clear(); $this->registry->registerPermissionPreset(new TestSharePermissionPreset1()); @@ -2235,6 +2421,7 @@ public function testGetShare(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], 'properties' => [ @@ -2355,6 +2542,7 @@ public function testGetShareAsRecipientActive(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], [ 'class' => TestShareRecipientType2::class, @@ -2376,6 +2564,7 @@ public function testGetShareAsRecipientActive(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], 'properties' => [ @@ -2481,6 +2670,7 @@ public function testGetShareAsRecipientWithArguments(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], 'properties' => [], @@ -2607,6 +2797,7 @@ public function testGetShareWithSecretActive(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], 'properties' => [], @@ -2729,6 +2920,7 @@ public function testGetShareAsRecipientFilteredProperties(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], 'properties' => [ @@ -2832,6 +3024,7 @@ public function testGetShareAsRecipientFilteredProperties(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], 'properties' => [ @@ -2955,6 +3148,7 @@ public function testGetShareAsRecipientFilteredArguments(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], 'properties' => [ @@ -3052,6 +3246,7 @@ public function testGetShareAsRecipientFilteredArguments(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], 'properties' => [ @@ -3178,6 +3373,7 @@ public function testGetShareWithPublicSecret(bool $isSecretPublic): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], $formatted['recipients'][0]); $this->assertIsArray($formatted['recipients'][1]); if ($isSecretPublic) { @@ -3352,6 +3548,7 @@ public function testGetShareUniqueDisplayNames(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], [ 'class' => TestShareRecipientType2::class, @@ -3373,6 +3570,7 @@ public function testGetShareUniqueDisplayNames(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], [ 'class' => TestShareRecipientType2::class, @@ -3394,6 +3592,7 @@ public function testGetShareUniqueDisplayNames(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], $formatted['recipients']); } @@ -3485,6 +3684,7 @@ public function testGetShareDisabledInitiator(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], [ 'class' => TestShareRecipientType2::class, @@ -3506,6 +3706,7 @@ public function testGetShareDisabledInitiator(): void { 'dark' => 'http://localhost/index.php/avatar/user1/64/dark', ], ], + 'permissions' => [], ], ], $formatted['recipients']); } @@ -3591,6 +3792,7 @@ public function testGetShares(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], 'properties' => [ @@ -3672,6 +3874,7 @@ public function testGetShares(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], 'properties' => [ @@ -3761,6 +3964,7 @@ public function testGetShares(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], 'properties' => [ @@ -3850,6 +4054,7 @@ public function testGetShares(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], 'properties' => [ @@ -3942,6 +4147,7 @@ public function testGetShares(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], 'properties' => [ @@ -4031,6 +4237,7 @@ public function testGetShares(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], 'properties' => [ @@ -4123,6 +4330,7 @@ public function testGetShares(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], 'properties' => [ @@ -4215,6 +4423,7 @@ public function testGetShares(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], 'properties' => [ @@ -4304,6 +4513,7 @@ public function testGetShares(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], 'properties' => [ @@ -4433,6 +4643,7 @@ public function testInitiatorDeleted(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], [ 'class' => TestShareRecipientType2::class, @@ -4454,6 +4665,7 @@ public function testInitiatorDeleted(): void { 'dark' => 'http://localhost/index.php/avatar/owner/64/dark', ], ], + 'permissions' => [], ], ], $formatted['recipients']); } diff --git a/tests/lib/Sharing/ShareTest.php b/tests/lib/Sharing/ShareTest.php new file mode 100644 index 0000000000000..817a525a719f2 --- /dev/null +++ b/tests/lib/Sharing/ShareTest.php @@ -0,0 +1,211 @@ +registry = Server::get(ISharingRegistry::class); + $this->registry->clear(); + + $this->owner = $this->createUser('owner', 'password'); + $this->user1 = $this->createUser('user1', 'password'); + $this->user2 = $this->createUser('user2', 'password'); + } + + #[\Override] + public function tearDown(): void { + $this->registry->clear(); + + parent::tearDown(); + } + + public function testGetEffectiveRecipients(): void { + $recipient1 = new ShareRecipient(TestShareRecipientType1::class, 'recipient1', null); + $recipient2 = new ShareRecipient(TestShareRecipientType1::class, 'recipient2', null); + $recipient3 = new ShareRecipient(TestShareRecipientType1::class, 'recipient3', null, 'secret'); + + $this->registry->registerRecipientType( + new TestShareRecipientType1( + [ + 'recipient1' => 'Recipient 1', + 'recipient2' => 'Recipient 2', + 'recipient3' => 'Recipient 3', + ], + [ + $this->user1->getUID() => ['recipient1'], + $this->user2->getUID() => ['recipient2'], + ], + [], + ) + ); + + $share = new Share( + '123', + new ShareUser($this->owner->getUID(), null), + new DateTimeImmutable(), + ShareState::Active, + null, + [], + [ + $recipient1, + $recipient2, + $recipient3, + ], + [], + [], + ); + + $this->assertEquals([$recipient1], $share->getEffectiveRecipients(new ShareAccessContext($this->user1))); + $this->assertEquals([$recipient2], $share->getEffectiveRecipients(new ShareAccessContext($this->user2))); + $this->assertEquals([$recipient3], $share->getEffectiveRecipients(new ShareAccessContext(secret: 'secret'))); + $this->assertEquals([$recipient1, $recipient3], $share->getEffectiveRecipients(new ShareAccessContext($this->user1, 'secret'))); + } + + /** + * @return list, array, SharePermission>, bool}> + */ + public static function dataGetEffectiveEnabledPermissions(): array { + $recipient1 = new ShareRecipient(TestShareRecipientType1::class, 'recipient1', null, null, null, [TestSharePermissionType1::class => new SharePermission(TestSharePermissionType1::class, true)]); + $recipient2 = new ShareRecipient(TestShareRecipientType1::class, 'recipient2', null, null, null, [TestSharePermissionType1::class => new SharePermission(TestSharePermissionType1::class, false)]); + + return [ + [ + [$recipient1], + [TestSharePermissionType1::class => new SharePermission(TestSharePermissionType1::class, true)], + true, + ], + [ + [$recipient1], + [TestSharePermissionType1::class => new SharePermission(TestSharePermissionType1::class, false)], + false, + ], + [ + [$recipient1], + [], + false, + ], + // + [ + [$recipient2], + [TestSharePermissionType1::class => new SharePermission(TestSharePermissionType1::class, true)], + false, + ], + [ + [$recipient2], + [TestSharePermissionType1::class => new SharePermission(TestSharePermissionType1::class, false)], + false, + ], + [ + [$recipient2], + [], + false, + ], + // + [ + [$recipient1, $recipient2], + [TestSharePermissionType1::class => new SharePermission(TestSharePermissionType1::class, true)], + true, + ], + [ + [$recipient1, $recipient2], + [TestSharePermissionType1::class => new SharePermission(TestSharePermissionType1::class, false)], + false, + ], + [ + [$recipient1, $recipient2], + [], + false, + ], + // + [ + [$recipient2, $recipient1], + [TestSharePermissionType1::class => new SharePermission(TestSharePermissionType1::class, true)], + true, + ], + [ + [$recipient2, $recipient1], + [TestSharePermissionType1::class => new SharePermission(TestSharePermissionType1::class, false)], + false, + ], + [ + [$recipient2, $recipient1], + [], + false, + ], + ]; + } + + /** + * @param list $recipients + * @param array, SharePermission> $permissions + */ + #[DataProvider('dataGetEffectiveEnabledPermissions')] + public function testGetEffectiveEnabledPermissions(array $recipients, array $permissions, bool $enabled): void { + $this->registry->registerRecipientType( + new TestShareRecipientType1( + [ + 'recipient1' => 'Recipient 1', + 'recipient2' => 'Recipient 2', + ], + [ + $this->user1->getUID() => ['recipient1', 'recipient2'], + ], + [], + ) + ); + + $share = new Share( + '123', + new ShareUser($this->owner->getUID(), null), + new DateTimeImmutable(), + ShareState::Active, + null, + [], + $recipients, + [], + $permissions, + ); + + $this->assertEquals( + $enabled + ? [TestSharePermissionType1::class => new SharePermission(TestSharePermissionType1::class, true)] + : [], + $share->getEffectiveEnabledPermissions(new ShareAccessContext($this->user1)), + ); + } +} diff --git a/tests/lib/Sharing/SharingManagerTest.php b/tests/lib/Sharing/SharingManagerTest.php index 52637b9aaf78b..b25a4f7610909 100644 --- a/tests/lib/Sharing/SharingManagerTest.php +++ b/tests/lib/Sharing/SharingManagerTest.php @@ -46,9 +46,16 @@ private function assertShareSyncedWithDb(ShareAccessContext $accessContext, Shar ); // cache the enabled permissions for both - $retrieved->getEnabledPermissions(); + foreach ([$retrieved, $share] as $item) { + $item->getEffectiveEnabledPermissions(new ShareAccessContext(overrideChecks: true)); + $item->getEffectiveEnabledPermissions(new ShareAccessContext($this->owner)); + $item->getEffectiveEnabledPermissions(new ShareAccessContext($this->user1)); + $item->getEffectiveEnabledPermissions(new ShareAccessContext($this->user2)); + foreach ($item->recipients as $recipient) { + $recipient->getDisabledPermissions(); + } + } - $share->getEnabledPermissions(); // ensure source metadata is loaded foreach ($share->sources as $source) { $source->format($this->registry, $this->l10nFactory, false); @@ -81,7 +88,7 @@ protected function createShare(ShareAccessContext $accessContext): array { $this->dbConnection->beginTransaction(); $share = $this->manager->createShare($accessContext); $this->assertShareSyncedWithDb($accessContext, $share); - $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class)); + $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $accessContext); $this->dbConnection->commit(); return $share; } catch (Exception $exception) { @@ -96,7 +103,7 @@ protected function updateShareState(ShareAccessContext $accessContext, Share $sh $this->dbConnection->beginTransaction(); $share = $this->manager->updateShareState($accessContext, $share, $state); $this->assertShareSyncedWithDb($accessContext, $share); - $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class)); + $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $accessContext); $this->dbConnection->commit(); return $share; } catch (Exception $exception) { @@ -111,7 +118,7 @@ protected function updateShareUserStatus(ShareAccessContext $accessContext, Shar $this->dbConnection->beginTransaction(); $share = $this->manager->updateShareUserStatus($accessContext, $share, $userStatus); $this->assertShareSyncedWithDb($accessContext, $share); - $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class)); + $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $accessContext); $this->dbConnection->commit(); return $share; } catch (Exception $exception) { @@ -126,7 +133,7 @@ protected function addShareSource(ShareAccessContext $accessContext, Share $shar $this->dbConnection->beginTransaction(); $share = $this->manager->addShareSource($accessContext, $share, $source); $this->assertShareSyncedWithDb($accessContext, $share); - $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class)); + $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $accessContext); $this->dbConnection->commit(); return $share; } catch (Exception $exception) { @@ -142,7 +149,7 @@ protected function removeShareSource(ShareAccessContext $accessContext, Share $s $this->assertShareSyncedWithDb($accessContext, $share); $share = $this->manager->removeShareSource($accessContext, $share, $source); $this->assertShareSyncedWithDb($accessContext, $share); - $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class)); + $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $accessContext); $this->dbConnection->commit(); return $share; } catch (Exception $exception) { @@ -157,7 +164,7 @@ protected function addShareRecipient(ShareAccessContext $accessContext, Share $s $this->dbConnection->beginTransaction(); $share = $this->manager->addShareRecipient($accessContext, $share, $recipient); $this->assertShareSyncedWithDb($accessContext, $share); - $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class)); + $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $accessContext); $this->dbConnection->commit(); return $share; } catch (Exception $exception) { @@ -172,7 +179,7 @@ protected function removeShareRecipient(ShareAccessContext $accessContext, Share $this->dbConnection->beginTransaction(); $share = $this->manager->removeShareRecipient($accessContext, $share, $recipient); $this->assertShareSyncedWithDb($accessContext, $share); - $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class)); + $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $accessContext); $this->dbConnection->commit(); return $share; } catch (Exception $exception) { @@ -187,7 +194,7 @@ protected function updateShareRecipientSecret(ShareAccessContext $accessContext, $this->dbConnection->beginTransaction(); $share = $this->manager->updateShareRecipientSecret($accessContext, $share, $recipient, $secret); $this->assertShareSyncedWithDb($accessContext, $share); - $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class)); + $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $accessContext); $this->dbConnection->commit(); return $share; } catch (Exception $exception) { @@ -202,7 +209,7 @@ protected function updateShareProperty(ShareAccessContext $accessContext, Share $this->dbConnection->beginTransaction(); $share = $this->manager->updateShareProperty($accessContext, $share, $property); $this->assertShareSyncedWithDb($accessContext, $share); - $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class)); + $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $accessContext); $this->dbConnection->commit(); return $share; } catch (Exception $exception) { @@ -217,7 +224,22 @@ protected function updateSharePermission(ShareAccessContext $accessContext, Shar $this->dbConnection->beginTransaction(); $share = $this->manager->updateSharePermission($accessContext, $share, $permission); $this->assertShareSyncedWithDb($accessContext, $share); - $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class)); + $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $accessContext); + $this->dbConnection->commit(); + return $share; + } catch (Exception $exception) { + $this->dbConnection->rollBack(); + throw $exception; + } + } + + #[\Override] + protected function updateShareRecipientPermission(ShareAccessContext $accessContext, Share $share, ShareRecipient $recipient, SharePermission $permission): array { + try { + $this->dbConnection->beginTransaction(); + $share = $this->manager->updateShareRecipientPermission($accessContext, $share, $recipient, $permission); + $this->assertShareSyncedWithDb($accessContext, $share); + $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $accessContext); $this->dbConnection->commit(); return $share; } catch (Exception $exception) { @@ -233,7 +255,7 @@ protected function selectSharePermissionPreset(ShareAccessContext $accessContext /** @psalm-suppress ArgumentTypeCoercion */ $share = $this->manager->selectSharePermissionPreset($accessContext, $share, $permissionPresetClass); $this->assertShareSyncedWithDb($accessContext, $share); - $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class)); + $share = $share->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $accessContext); $this->dbConnection->commit(); return $share; } catch (Exception $exception) { @@ -259,7 +281,7 @@ protected function deleteShare(ShareAccessContext $accessContext, Share $share): protected function getShare(ShareAccessContext $accessContext, string $id): array { try { $this->dbConnection->beginTransaction(); - $share = $this->manager->getShare($accessContext, $id)->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class)); + $share = $this->manager->getShare($accessContext, $id)->format($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $accessContext); $this->dbConnection->commit(); return $share; } catch (Exception $exception) { @@ -275,7 +297,7 @@ protected function getShares(ShareAccessContext $accessContext, ?string $filterS /** @psalm-suppress ArgumentTypeCoercion */ $shares = $this->manager->getShares($accessContext, $filterSourceTypeClass, $filterSourceTypeValue, $filterState, $filterUserStatus, $lastShareID, $limit); $this->dbConnection->commit(); - return Share::formatMultiple($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $shares); + return Share::formatMultiple($this->registry, Server::get(IFactory::class), Server::get(IURLGenerator::class), Server::get(IUserManager::class), $accessContext, $shares); } catch (Exception $exception) { $this->dbConnection->rollBack(); throw $exception; From 69696503e1e50195ac6d811df84947dd60dc7f61 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Tue, 1 Sep 2026 08:16:22 +0200 Subject: [PATCH 9/9] refactor(Sharing): Move transaction check from SharingManager to SharingBackend Signed-off-by: provokateurin --- lib/private/Sharing/SharingBackend.php | 52 ++++++++++++++++++++++++++ lib/private/Sharing/SharingManager.php | 46 ----------------------- 2 files changed, 52 insertions(+), 46 deletions(-) diff --git a/lib/private/Sharing/SharingBackend.php b/lib/private/Sharing/SharingBackend.php index eed53432aa6a7..bc62456b12a2d 100644 --- a/lib/private/Sharing/SharingBackend.php +++ b/lib/private/Sharing/SharingBackend.php @@ -65,6 +65,8 @@ public function __construct( #[\Override] public function createShare(string $id, ShareUser $owner, \DateTimeImmutable $lastUpdated): void { + $this->assertInTransaction(); + $qb = $this->connection->getQueryBuilder(); $qb ->insert('sharing_share') @@ -80,6 +82,8 @@ public function createShare(string $id, ShareUser $owner, \DateTimeImmutable $la #[\Override] public function onOwnerDeleted(ShareUser $owner): array { + $this->assertInTransaction(); + $qb = $this->connection->getQueryBuilder(); $qb ->selectDistinct('id') @@ -114,6 +118,8 @@ public function onOwnerDeleted(ShareUser $owner): array { #[\Override] public function updateShareState(string $id, ShareState $state): void { + $this->assertInTransaction(); + $qb = $this->connection->getQueryBuilder(); $rowCount = $qb ->update('sharing_share') @@ -127,6 +133,8 @@ public function updateShareState(string $id, ShareState $state): void { #[\Override] public function updateShareUserStatus(string $id, string $userId, ShareUserStatus $userStatus): void { + $this->assertInTransaction(); + $qb = $this->connection->getQueryBuilder(); $rowCount = $qb ->update('sharing_share_user_status') @@ -149,6 +157,8 @@ public function updateShareUserStatus(string $id, string $userId, ShareUserStatu #[\Override] public function addShareSource(string $id, ShareSource $source): void { + $this->assertInTransaction(); + try { $qb = $this->connection->getQueryBuilder(); $qb @@ -173,6 +183,8 @@ public function addShareSource(string $id, ShareSource $source): void { #[\Override] public function removeShareSource(string $id, ShareSource $source): void { + $this->assertInTransaction(); + $qb = $this->connection->getQueryBuilder(); $rowCount = $qb ->delete('sharing_share_sources') @@ -187,6 +199,8 @@ public function removeShareSource(string $id, ShareSource $source): void { #[\Override] public function onSourceDeleted(ShareSource $source): array { + $this->assertInTransaction(); + $qb = $this->connection->getQueryBuilder(); $result = $qb ->selectDistinct('share_id') @@ -215,6 +229,8 @@ public function onSourceDeleted(ShareSource $source): array { #[\Override] public function addShareRecipient(string $id, ShareRecipient $recipient): void { + $this->assertInTransaction(); + if ($recipient->secret === null) { throw new RuntimeException('The secret must not be null.'); } @@ -255,6 +271,8 @@ public function addShareRecipient(string $id, ShareRecipient $recipient): void { #[\Override] public function removeShareRecipient(string $id, ShareRecipient $recipient): void { + $this->assertInTransaction(); + $qb = $this->connection->getQueryBuilder(); $rowCount = $qb ->delete('sharing_share_recipients') @@ -276,6 +294,8 @@ public function removeShareRecipient(string $id, ShareRecipient $recipient): voi #[\Override] public function onRecipientDeleted(ShareRecipient $recipient): array { + $this->assertInTransaction(); + $qb = $this->connection->getQueryBuilder(); $result = $qb ->selectDistinct('share_id') @@ -318,6 +338,8 @@ public function onRecipientDeleted(ShareRecipient $recipient): array { #[\Override] public function onInitiatorDeleted(ShareUser $initiator): array { + $this->assertInTransaction(); + $qb = $this->connection->getQueryBuilder(); $qb ->selectDistinct('share_id') @@ -363,6 +385,8 @@ public function onInitiatorDeleted(ShareUser $initiator): array { #[\Override] public function updateShareRecipientSecret(string $id, ShareRecipient $recipient, string $secret): void { + $this->assertInTransaction(); + $qb = $this->connection->getQueryBuilder(); $rowCount = $qb ->update('sharing_share_recipients') @@ -385,6 +409,8 @@ public function updateShareRecipientSecret(string $id, ShareRecipient $recipient #[\Override] public function updateShareProperty(string $id, ShareProperty $property): ?string { + $this->assertInTransaction(); + $value = $property->value; $propertyType = $this->registry->getPropertyTypes()[$property->class]; @@ -436,6 +462,8 @@ public function updateShareProperty(string $id, ShareProperty $property): ?strin #[\Override] public function updateSharePermission(string $id, SharePermission $permission): void { + $this->assertInTransaction(); + $qb = $this->connection->getQueryBuilder(); $rowCount = $qb ->update('sharing_share_permissions') @@ -460,6 +488,8 @@ public function updateSharePermission(string $id, SharePermission $permission): #[\Override] public function updateShareRecipientPermission(string $id, ShareRecipient $recipient, SharePermission $permission): void { + $this->assertInTransaction(); + $qb = $this->connection->getQueryBuilder(); $result = $qb ->select('id') @@ -506,6 +536,8 @@ public function updateShareRecipientPermission(string $id, ShareRecipient $recip #[\Override] public function selectSharePermissionPreset(string $id, string $permissionPresetClass): void { + $this->assertInTransaction(); + $qb = $this->connection->getQueryBuilder(); $qb ->update('sharing_share_permissions') @@ -531,6 +563,8 @@ public function selectSharePermissionPreset(string $id, string $permissionPreset #[\Override] public function deleteShare(string $id): void { + $this->assertInTransaction(); + $qb = $this->connection->getQueryBuilder(); $rowCount = $qb ->delete('sharing_share') @@ -545,6 +579,8 @@ public function deleteShare(string $id): void { #[\Override] public function getShare(ShareAccessContext $accessContext, string $id): Share { + $this->assertInTransaction(); + $shares = $this->list($accessContext, $id, null, null, null, null, null, null); if (count($shares) !== 1) { throw new ShareNotFoundException(); @@ -563,11 +599,15 @@ public function getShares( ?string $lastShareID, ?int $limit, ): array { + $this->assertInTransaction(); + return $this->list($accessContext, null, $filterSourceTypeClass, $filterSourceTypeValue, $filterState, $filterUserStatus, $lastShareID, $limit); } #[\Override] public function hasShare(string $id): bool { + $this->assertInTransaction(); + $qb = $this->connection->getQueryBuilder(); $result = $qb @@ -581,6 +621,8 @@ public function hasShare(string $id): bool { #[\Override] public function getShareOwner(string $id): ShareUser { + $this->assertInTransaction(); + $qb = $this->connection->getQueryBuilder(); $qb ->select('owner_user_id', 'owner_instance') @@ -608,6 +650,8 @@ public function getShareOwner(string $id): ShareUser { */ #[\Override] public function setLastUpdated(array $ids, \DateTimeImmutable $lastUpdated): void { + $this->assertInTransaction(); + foreach (array_chunk($ids, 1000) as $chunk) { $qb = $this->connection->getQueryBuilder(); @@ -1258,6 +1302,8 @@ private function getShareCompatiblePermissionTypeClasses(array $sources): array #[\Override] public function ensureDefaults(array $shares): array { + $this->assertInTransaction(); + $defaultSet = false; foreach ($shares as &$share) { $shareSourceTypeClasses = array_map(fn (ShareSource $source): string => $source->class, $share->sources); @@ -1376,4 +1422,10 @@ private static function parseTimestamp(string $timestampMs): \DateTimeImmutable return $time; } + + private function assertInTransaction(): void { + if (!$this->connection->inTransaction()) { + throw new RuntimeException('The SharingBackend can only be used inside a transaction.'); + } + } } diff --git a/lib/private/Sharing/SharingManager.php b/lib/private/Sharing/SharingManager.php index 25d59e58a2378..add248bcb934e 100644 --- a/lib/private/Sharing/SharingManager.php +++ b/lib/private/Sharing/SharingManager.php @@ -164,8 +164,6 @@ public function createShare(ShareAccessContext $accessContext): Share { throw new RuntimeException('No user present to create a share'); } - $this->assertInTransaction(); - $id = $this->snowflakeGenerator->nextId(); $lastUpdated = $this->getTime(); $this->backend->createShare($id, new ShareUser($currentUser->getUID(), null), $lastUpdated); @@ -182,8 +180,6 @@ public function onOwnerDeleted(ShareAccessContext $accessContext, ShareUser $own throw new RuntimeException('Only possible if checks are overridden.'); } - $this->assertInTransaction(); - // No need to update the last updated timestamp, because the share will be deleted anyway. $ids = $this->backend->onOwnerDeleted($owner); @@ -198,8 +194,6 @@ public function onOwnerDeleted(ShareAccessContext $accessContext, ShareUser $own #[\Override] public function updateShareState(ShareAccessContext $accessContext, Share $share, ShareState $state): Share { - $this->assertInTransaction(); - $time = $this->getTime(); $this->backend->setLastUpdated([$share->id], $time); @@ -233,8 +227,6 @@ public function updateShareUserStatus(ShareAccessContext $accessContext, Share $ throw new RuntimeException('No user present to update share status for.'); } - $this->assertInTransaction(); - if ($share->owner->isCurrentUser($accessContext)) { throw new ShareInvalidException('Cannot set user status for the owner of the share.', $this->l10n->t('Cannot set user status for the owner of the share.')); } @@ -261,8 +253,6 @@ public function updateShareUserStatus(ShareAccessContext $accessContext, Share $ #[\Override] public function addShareSource(ShareAccessContext $accessContext, Share $share, ShareSource $source): Share { - $this->assertInTransaction(); - // only the owner can add sources, otherwise a user could add sources others don't have access to, which would remove their access $this->validateShareEditPermissions($accessContext, $share, true); @@ -306,8 +296,6 @@ public function addShareSource(ShareAccessContext $accessContext, Share $share, #[\Override] public function removeShareSource(ShareAccessContext $accessContext, Share $share, ShareSource $source): Share { - $this->assertInTransaction(); - // only the owner can remove sources, to mirror the "add source" permissions $this->validateShareEditPermissions($accessContext, $share, true); @@ -348,8 +336,6 @@ public function onSourceDeleted(ShareAccessContext $accessContext, ShareSource $ throw new RuntimeException('Only possible if checks are overridden.'); } - $this->assertInTransaction(); - $timestamp = $this->getTime(); $updatedIds = $this->backend->onSourceDeleted($source); @@ -368,8 +354,6 @@ public function addShareRecipient(ShareAccessContext $accessContext, Share $shar throw new RuntimeException('No current user provided in access context.'); } - $this->assertInTransaction(); - try { $this->validateShareEditPermissions($accessContext, $share); } catch (ShareOperationForbiddenException) { @@ -446,10 +430,6 @@ public function addShareRecipient(ShareAccessContext $accessContext, Share $shar #[\Override] public function removeShareRecipient(ShareAccessContext $accessContext, Share $share, ShareRecipient $recipient): Share { - $this->assertInTransaction(); - - $this->assertInTransaction(); - try { $this->validateShareEditPermissions($accessContext, $share); } catch (ShareOperationForbiddenException) { @@ -492,8 +472,6 @@ public function onRecipientDeleted(ShareAccessContext $accessContext, ShareRecip throw new RuntimeException('Only possible if checks are overridden.'); } - $this->assertInTransaction(); - $timestamp = $this->getTime(); $updatedIds = $this->backend->onRecipientDeleted($recipient); @@ -512,8 +490,6 @@ public function onInitiatorDeleted(ShareAccessContext $accessContext, ShareUser throw new RuntimeException('Only possible if checks are overridden.'); } - $this->assertInTransaction(); - $timestamp = $this->getTime(); $updatedIds = $this->backend->onInitiatorDeleted($initiator); @@ -535,8 +511,6 @@ private function validateShareSecret(string $secret): bool { #[\Override] public function updateShareRecipientSecret(ShareAccessContext $accessContext, Share $share, ShareRecipient $recipient, string $secret): Share { - $this->assertInTransaction(); - try { $this->validateShareEditPermissions($accessContext, $share); } catch (ShareOperationForbiddenException) { @@ -593,8 +567,6 @@ public function updateShareRecipientSecret(ShareAccessContext $accessContext, Sh #[\Override] public function updateShareProperty(ShareAccessContext $accessContext, Share $share, ShareProperty $property): Share { - $this->assertInTransaction(); - $this->validateShareEditPermissions($accessContext, $share); if (($propertyType = $this->registry->getPropertyTypes()[$property->class] ?? null) === null) { @@ -639,8 +611,6 @@ public function updateShareProperty(ShareAccessContext $accessContext, Share $sh #[\Override] public function updateSharePermission(ShareAccessContext $accessContext, Share $share, SharePermission $permission): Share { - $this->assertInTransaction(); - $this->validateShareEditPermissions($accessContext, $share); if (!isset($this->registry->getPermissionTypes()[$permission->class])) { @@ -677,8 +647,6 @@ public function updateSharePermission(ShareAccessContext $accessContext, Share $ #[\Override] public function updateShareRecipientPermission(ShareAccessContext $accessContext, Share $share, ShareRecipient $recipient, SharePermission $permission): Share { - $this->assertInTransaction(); - $time = $this->getTime(); $this->backend->setLastUpdated([$share->id], $time); @@ -734,8 +702,6 @@ public function updateShareRecipientPermission(ShareAccessContext $accessContext #[\Override] public function selectSharePermissionPreset(ShareAccessContext $accessContext, Share $share, string $permissionPresetClass): Share { - $this->assertInTransaction(); - $this->validateShareEditPermissions($accessContext, $share); if (($this->registry->getPermissionPresetCompatiblePermissionTypeClasses()[$permissionPresetClass] ?? null) === null) { @@ -773,8 +739,6 @@ public function selectSharePermissionPreset(ShareAccessContext $accessContext, S #[\Override] public function deleteShare(ShareAccessContext $accessContext, Share $share): void { - $this->assertInTransaction(); - // No need to update the last updated timestamp, because the share will be deleted anyway. $this->validateShareEditPermissions($accessContext, $share); @@ -789,8 +753,6 @@ public function deleteShare(ShareAccessContext $accessContext, Share $share): vo #[\Override] public function getShare(ShareAccessContext $accessContext, string $id): Share { - $this->assertInTransaction(); - return $this->backend->getShare($accessContext, $id); } @@ -804,8 +766,6 @@ public function getShares( ?string $lastShareID, ?int $limit, ): array { - $this->assertInTransaction(); - return $this->backend->getShares($accessContext, $filterSourceTypeClass, $filterSourceTypeValue, $filterState, $filterUserStatus, $lastShareID, $limit); } @@ -833,12 +793,6 @@ public function handle(Event $event): void { } } - private function assertInTransaction(): void { - if (!$this->dbConnection->inTransaction()) { - throw new RuntimeException('The SharingManager can only be used inside a transaction.'); - } - } - /** * @throws ShareOperationForbiddenException */