diff --git a/apps/files/lib/Sharing/Source/NodeShareSourceType.php b/apps/files/lib/Sharing/Source/NodeShareSourceType.php index ea1a435e0223f..52bce5542639b 100644 --- a/apps/files/lib/Sharing/Source/NodeShareSourceType.php +++ b/apps/files/lib/Sharing/Source/NodeShareSourceType.php @@ -23,10 +23,12 @@ use OCP\Files\Events\Node\NodeDeletedEvent; use OCP\Files\IRootFolder; use OCP\Files\Node; +use OCP\Files\Storage\ISharedStorage; use OCP\IDBConnection; use OCP\Interaction\InteractionResource; use OCP\Interaction\Resources\NodeResource; use OCP\IURLGenerator; +use OCP\IUser; use OCP\L10N\IFactory; /** @@ -87,4 +89,18 @@ public function handle(Event $event): void { throw $exception; } } + + #[\Override] + public function userHasDirectSharingAccessToSource(IUser $user, string $source): bool { + // TODO: cache nodes by id? + $userFolder = $this->rootFolder->getUserFolder($user->getUID()); + $nodes = $userFolder->getById((int)$source); + foreach ($nodes as $node) { + if (!$node->getStorage() instanceof ISharedStorage && $node->isShareable()) { + return true; + } + } + + return false; + } } diff --git a/lib/private/Sharing/SharingBackend.php b/lib/private/Sharing/SharingBackend.php index 5d6cb5d4017d2..86c6db25d9266 100644 --- a/lib/private/Sharing/SharingBackend.php +++ b/lib/private/Sharing/SharingBackend.php @@ -559,6 +559,15 @@ private function hideDisabledUserShares(): bool { * @return list */ private function list(ShareAccessContext $accessContext, ?string $filterShareID, ?string $filterSourceTypeClass, ?string $filterSourceTypeValue, ?string $lastShareID, ?int $limit): array { + if ($filterSourceTypeClass) { + $filterSourceType = $this->registry->getSourceTypes()[$filterSourceTypeClass] ?? null; + if ($filterSourceType === null) { + throw new RuntimeException('The source type is not registered: ' . $filterSourceTypeClass); + } + } else { + $filterSourceType = null; + } + /** @var array, list> $recipientTypeValues */ $recipientTypeValues = []; @@ -566,10 +575,21 @@ private function list(ShareAccessContext $accessContext, ?string $filterShareID, $queries = []; if ($accessContext->overrideChecks) { $queries[] = $this->connection->getQueryBuilder(); + $userHasDirectAccess = false; } else { + if ($filterSourceType && $filterSourceTypeValue !== null && $accessContext->currentUser instanceof IUser) { + $userHasDirectAccess = $filterSourceType->userHasDirectSharingAccessToSource($accessContext->currentUser, $filterSourceTypeValue); + } else { + $userHasDirectAccess = false; + } + if ($accessContext->currentUser instanceof IUser) { $qb = $this->connection->getQueryBuilder(); - $qb->where($qb->expr()->eq('s.owner_user_id', $qb->createNamedParameter($accessContext->currentUser->getUID()))); + // if the access user has "direct share access" we don't filter by owner, but instead validate that all share sources are accessible + if (!$userHasDirectAccess) { + $qb->where($qb->expr()->eq('s.owner_user_id', $qb->createNamedParameter($accessContext->currentUser->getUID()))); + } + $queries[] = $qb; } @@ -581,7 +601,8 @@ private function list(ShareAccessContext $accessContext, ?string $filterShareID, } // Do not add a query if no recipients matched, otherwise all shares will be returned. - if ($recipientTypeValues !== []) { + // If the user has "direct" access, we already get all the shares, so no need to run an extra query for recipients + if ($recipientTypeValues !== [] && !$userHasDirectAccess) { $qb = $this->connection->getQueryBuilder(); $qb->innerJoin('s', 'sharing_share_recipients', 'sr', $qb->expr()->andX( $qb->expr()->eq('s.state', $qb->createNamedParameter(ShareState::Active->value)), @@ -631,7 +652,7 @@ private function list(ShareAccessContext $accessContext, ?string $filterShareID, $qb->andWhere($qb->expr()->eq('s.id', $qb->createNamedParameter($filterShareID))); } - if ($filterSourceTypeClass !== null) { + if ($filterSourceType !== null) { $sourceTypeFilters = [ $qb->expr()->eq('s.id', 'ss.share_id'), $qb->expr()->eq('ss.source_class', $qb->createNamedParameter($filterSourceTypeClass)), @@ -802,7 +823,9 @@ private function list(ShareAccessContext $accessContext, ?string $filterShareID, if ($share['owner']->isCurrentUser($accessContext)) { continue; } - + if ($userHasDirectAccess) { + continue; + } $isAnyMatchingRecipient = false; foreach ($share['recipients'] as &$recipient) { $isMatchingRecipient = false; @@ -962,6 +985,23 @@ private function list(ShareAccessContext $accessContext, ?string $filterShareID, $share['permissions'], ), $shares); + // when listing shares for a source, we also return any non-owned share if the user has "direct" access to the source + // but we do need to validate that the user has "direct" access to *all* of the sources in the share, not just one + if (!$accessContext->overrideChecks && $filterSourceType && $filterSourceTypeValue !== null && $accessContext->currentUser instanceof IUser) { + $shares = array_filter($shares, function (Share $share) use ($accessContext): bool { + if (!$share->owner->isCurrentUser($accessContext) && count($share->sources) > 1) { + foreach ($share->sources as $source) { + $sourceType = $this->registry->getSourceTypes()[$source->class]; + if (!$sourceType->userHasDirectSharingAccessToSource($accessContext->currentUser, $source->value)) { + return false; + } + } + } + + return true; + }); + } + if (!$accessContext->overrideChecks) { $filterPropertyTypes = array_filter($registryPropertyTypes, static fn (ISharePropertyType $propertyType): bool => $propertyType instanceof ISharePropertyTypeFilter); if ($filterPropertyTypes !== []) { diff --git a/lib/private/Sharing/SharingManager.php b/lib/private/Sharing/SharingManager.php index 76593a8f4bc0b..dc96ad31a85ff 100644 --- a/lib/private/Sharing/SharingManager.php +++ b/lib/private/Sharing/SharingManager.php @@ -698,7 +698,7 @@ private function validateInteraction(ShareAccessContext $accessContext, Share $s $action = new ShareAction(null, array_values(array_map(static fn (SharePermission $permission): string => $permission->class, $share->getEnabledPermissions()))); $usersToCheck = []; - if ($share->owner->instance === null && ($ownerUser = $this->userManager->get($share->owner->userId)) !== null) { + if ($share->owner->instance === null && ($ownerUser = $this->userManager->get($share->owner->userId)) instanceof IUser) { $usersToCheck[] = $ownerUser; } diff --git a/lib/unstable/Sharing/Source/IShareSourceType.php b/lib/unstable/Sharing/Source/IShareSourceType.php index 0cce43c89731d..0aa7a2ce95331 100644 --- a/lib/unstable/Sharing/Source/IShareSourceType.php +++ b/lib/unstable/Sharing/Source/IShareSourceType.php @@ -13,6 +13,7 @@ use NCU\Sharing\Icon\ShareIconURL; use OCP\AppFramework\Attribute\Implementable; use OCP\Interaction\InteractionResource; +use OCP\IUser; use OCP\L10N\IFactory; /** @@ -57,4 +58,13 @@ public function getSourceIcon(string $source): null|ShareIconSVG|ShareIconURL; * @experimental 35.0.0 */ public function getSourceInteractionResource(string $userId, string $source): InteractionResource; + + /** + * Check if a user has access to the specified source without taking sharing into account, and has sufficient permissions to create shares. + * + * All users with "direct" access to the source will be able to see and manage shares made by other users for the source. + * + * @param non-empty-string $source + */ + public function userHasDirectSharingAccessToSource(IUser $user, string $source): bool; } diff --git a/tests/lib/Sharing/AbstractSharingManagerTests.php b/tests/lib/Sharing/AbstractSharingManagerTests.php index 62260bbf2ae2f..c8fde3cf73086 100644 --- a/tests/lib/Sharing/AbstractSharingManagerTests.php +++ b/tests/lib/Sharing/AbstractSharingManagerTests.php @@ -85,6 +85,10 @@ abstract protected function getShares(ShareAccessContext $accessContext, ?string protected IUser $user2; + protected TestShareSourceType1 $shareSourceType1; + + protected TestShareSourceType2 $shareSourceType2; + #[\Override] public function setUp(): void { parent::setUp(); @@ -112,8 +116,11 @@ public function setUp(): void { $this->registry = Server::get(ISharingRegistry::class); $this->registry->clear(); - $this->registry->registerSourceType(new TestShareSourceType1(['source1' => 'Source 1'])); - $this->registry->registerSourceType(new TestShareSourceType2(['source2' => 'Source 2'])); + + $this->shareSourceType1 = new TestShareSourceType1(['source1' => 'Source 1']); + $this->registry->registerSourceType($this->shareSourceType1); + $this->shareSourceType2 = new TestShareSourceType2(['source2' => 'Source 2']); + $this->registry->registerSourceType($this->shareSourceType2); $this->registry->registerRecipientType(new TestShareRecipientType1( [ 'recipient1' => 'Recipient 1', @@ -4032,4 +4039,53 @@ public function testInitiatorDeleted(): void { ], ], $share['recipients']); } + + public function testGetWithDirectAccess(): void { + $accessContext = new ShareAccessContext($this->owner); + + $before = $this->manager->generateTimestamp(); + $this->dbConnection->beginTransaction(); + $id = $this->manager->createShare($accessContext); + $this->manager->addShareSource($accessContext, $id, new ShareSource(TestShareSourceType1::class, 'source1')); + $this->manager->addShareRecipient($accessContext, $id, new ShareRecipient(TestShareRecipientType1::class, 'recipient1', null)); + $this->manager->getShare($accessContext, $id); + $this->manager->updateSharePermission($accessContext, $id, new SharePermission(TestSharePermissionType1::class, true)); + $this->manager->updateShareState($accessContext, $id, ShareState::Active); + + $this->shareSourceType1->userAccess[$this->owner->getUID()] = ['source1']; + + $this->dbConnection->commit(); + + $after = $this->manager->generateTimestamp(); + + // user2 has no direct access, no shares + $shares = $this->getShares(new ShareAccessContext(currentUser: $this->user2), TestShareSourceType1::class, 'source1', null, null); + $this->assertCount(0, $shares); + + // give user2 direct access, can see shares + $this->shareSourceType1->userAccess[$this->user2->getUID()] = ['source1']; + $shares = $this->getShares(new ShareAccessContext(currentUser: $this->user2), TestShareSourceType1::class, 'source1', null, null); + + $this->assertCount(1, $shares); + $share = $shares[0]; + + $this->assertGreaterThanOrEqual($before, $share['last_updated']); + $this->assertLessThanOrEqual($after, $share['last_updated']); + $this->assertEquals([ + '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', + ], + ], $share['owner']); + + // add a source that user2 doesn't have access to, can't see share anymore + $this->dbConnection->beginTransaction(); + $this->manager->addShareSource($accessContext, $id, new ShareSource(TestShareSourceType2::class, 'source2')); + $this->dbConnection->commit(); + $shares = $this->getShares(new ShareAccessContext(currentUser: $this->user2), TestShareSourceType1::class, 'source1', null, null); + $this->assertCount(0, $shares); + } } diff --git a/tests/lib/Sharing/TestShareSourceType1.php b/tests/lib/Sharing/TestShareSourceType1.php index e61bd869db945..ca41def3d9a39 100644 --- a/tests/lib/Sharing/TestShareSourceType1.php +++ b/tests/lib/Sharing/TestShareSourceType1.php @@ -13,12 +13,15 @@ use NCU\Sharing\Icon\ShareIconURL; use NCU\Sharing\Source\IShareSourceType; use OCP\Interaction\InteractionResource; +use OCP\IUser; use OCP\L10N\IFactory; class TestShareSourceType1 implements IShareSourceType { public function __construct( /** @var array $validSources */ private readonly array $validSources, + /** @var array $validSources */ + public array $userAccess = [], ) { } @@ -48,4 +51,10 @@ public function getSourceIcon(string $source): null|ShareIconSVG|ShareIconURL { public function getSourceInteractionResource(string $userId, string $source): InteractionResource { return new TestInteractionResource($source); } + + #[\Override] + public function userHasDirectSharingAccessToSource(IUser $user, string $source): bool { + $userSources = $this->userAccess[$user->getUID()] ?? []; + return in_array($source, $userSources); + } }