Skip to content

Commit edf5ba9

Browse files
icewind1991icewind1991
committed
feat: allow users with "native" access to see all shares of a file
Signed-off-by: Robin Appelman <robin@icewind.nl> # Conflicts: # lib/unstable/Sharing/Source/IShareSourceType.php # tests/lib/Sharing/AbstractSharingManagerTests.php
1 parent 1e92a39 commit edf5ba9

5 files changed

Lines changed: 134 additions & 6 deletions

File tree

‎apps/files/lib/Sharing/Source/NodeShareSourceType.php‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
use OCP\Files\Events\Node\NodeDeletedEvent;
2626
use OCP\Files\IRootFolder;
2727
use OCP\Files\Node;
28+
use OCP\Files\Storage\ISharedStorage;
2829
use OCP\IDBConnection;
2930
use OCP\Interaction\InteractionResource;
3031
use OCP\Interaction\Resources\NodeResource;
@@ -95,4 +96,18 @@ public function handle(Event $event): void {
9596
throw $exception;
9697
}
9798
}
99+
100+
#[\Override]
101+
public function userHasDirectSharingAccessToSource(IUser $user, string $source): bool {
102+
// TODO: cache nodes by id?
103+
$userFolder = $this->rootFolder->getUserFolder($user->getUID());
104+
$nodes = $userFolder->getById((int)$source);
105+
foreach ($nodes as $node) {
106+
if (!$node->getStorage() instanceof ISharedStorage && $node->isShareable()) {
107+
return true;
108+
}
109+
}
110+
111+
return false;
112+
}
98113
}

‎lib/private/Sharing/SharingBackend.php‎

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -564,17 +564,37 @@ private function hideDisabledUserShares(): bool {
564564
* @return list<Share>
565565
*/
566566
private function list(ShareAccessContext $accessContext, ?string $filterShareID, ?string $filterSourceTypeClass, ?string $filterSourceTypeValue, ?string $lastShareID, ?int $limit): array {
567+
if ($filterSourceTypeClass) {
568+
$filterSourceType = $this->registry->getSourceTypes()[$filterSourceTypeClass] ?? null;
569+
if ($filterSourceType === null) {
570+
throw new RuntimeException('The source type is not registered: ' . $filterSourceTypeClass);
571+
}
572+
} else {
573+
$filterSourceType = null;
574+
}
575+
567576
/** @var array<class-string<IShareRecipientType>, list<string>> $recipientTypeValues */
568577
$recipientTypeValues = [];
569578

570579
/** @var list<IQueryBuilder> $queries */
571580
$queries = [];
572581
if ($accessContext->overrideChecks) {
573582
$queries[] = $this->connection->getQueryBuilder();
583+
$userHasDirectAccess = false;
574584
} else {
585+
if ($filterSourceType && $filterSourceTypeValue !== null && $accessContext->currentUser instanceof IUser) {
586+
$userHasDirectAccess = $filterSourceType->userHasDirectSharingAccessToSource($accessContext->currentUser, $filterSourceTypeValue);
587+
} else {
588+
$userHasDirectAccess = false;
589+
}
590+
575591
if ($accessContext->currentUser instanceof IUser) {
576592
$qb = $this->connection->getQueryBuilder();
577-
$qb->where($qb->expr()->eq('s.owner_user_id', $qb->createNamedParameter($accessContext->currentUser->getUID())));
593+
// if the access user has "direct share access" we don't filter by owner, but instead validate that all share sources are accessible
594+
if (!$userHasDirectAccess) {
595+
$qb->where($qb->expr()->eq('s.owner_user_id', $qb->createNamedParameter($accessContext->currentUser->getUID())));
596+
}
597+
578598
$queries[] = $qb;
579599
}
580600

@@ -586,7 +606,8 @@ private function list(ShareAccessContext $accessContext, ?string $filterShareID,
586606
}
587607

588608
// Do not add a query if no recipients matched, otherwise all shares will be returned.
589-
if ($recipientTypeValues !== []) {
609+
// If the user has "direct" access, we already get all the shares, so no need to run an extra query for recipients
610+
if ($recipientTypeValues !== [] && !$userHasDirectAccess) {
590611
$qb = $this->connection->getQueryBuilder();
591612
$qb->innerJoin('s', 'sharing_share_recipients', 'sr', $qb->expr()->andX(
592613
$qb->expr()->eq('s.state', $qb->createNamedParameter(ShareState::Active->value)),
@@ -636,7 +657,7 @@ private function list(ShareAccessContext $accessContext, ?string $filterShareID,
636657
$qb->andWhere($qb->expr()->eq('s.id', $qb->createNamedParameter($filterShareID)));
637658
}
638659

639-
if ($filterSourceTypeClass !== null) {
660+
if ($filterSourceType !== null) {
640661
$sourceTypeFilters = [
641662
$qb->expr()->eq('s.id', 'ss.share_id'),
642663
$qb->expr()->eq('ss.source_class_id', $qb->createNamedParameter($this->classMapper->getClassId($filterSourceTypeClass), IQueryBuilder::PARAM_INT)),
@@ -830,7 +851,9 @@ private function list(ShareAccessContext $accessContext, ?string $filterShareID,
830851
if ($share['owner']->isCurrentUser($accessContext)) {
831852
continue;
832853
}
833-
854+
if ($userHasDirectAccess) {
855+
continue;
856+
}
834857
$isAnyMatchingRecipient = false;
835858
foreach ($share['recipients'] as &$recipient) {
836859
$isMatchingRecipient = false;
@@ -991,6 +1014,23 @@ private function list(ShareAccessContext $accessContext, ?string $filterShareID,
9911014
$share['permissions'],
9921015
), $shares);
9931016

1017+
// when listing shares for a source, we also return any non-owned share if the user has "direct" access to the source
1018+
// but we do need to validate that the user has "direct" access to *all* of the sources in the share, not just one
1019+
if (!$accessContext->overrideChecks && $filterSourceType && $filterSourceTypeValue !== null && $accessContext->currentUser instanceof IUser) {
1020+
$shares = array_filter($shares, function (Share $share) use ($accessContext): bool {
1021+
if (!$share->owner->isCurrentUser($accessContext) && count($share->sources) > 1) {
1022+
foreach ($share->sources as $source) {
1023+
$sourceType = $this->registry->getSourceTypes()[$source->class];
1024+
if (!$sourceType->userHasDirectSharingAccessToSource($accessContext->currentUser, $source->value)) {
1025+
return false;
1026+
}
1027+
}
1028+
}
1029+
1030+
return true;
1031+
});
1032+
}
1033+
9941034
if (!$accessContext->overrideChecks) {
9951035
$filterPropertyTypes = array_filter($registryPropertyTypes, static fn (ISharePropertyType $propertyType): bool => $propertyType instanceof ISharePropertyTypeFilter);
9961036
if ($filterPropertyTypes !== []) {

‎lib/unstable/Sharing/Source/IShareSourceType.php‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,4 +55,13 @@ public function getSourcesMetadata(array $sources): array;
5555
* @experimental 35.0.0
5656
*/
5757
public function getSourceInteractionResource(IUser $user, string $source): InteractionResource;
58+
59+
/**
60+
* Check if a user has access to the specified source without taking sharing into account, and has sufficient permissions to create shares.
61+
*
62+
* All users with "direct" access to the source will be able to see and manage shares made by other users for the source.
63+
*
64+
* @param non-empty-string $source
65+
*/
66+
public function userHasDirectSharingAccessToSource(IUser $user, string $source): bool;
5867
}

‎tests/lib/Sharing/AbstractSharingManagerTests.php‎

Lines changed: 58 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,10 @@ abstract protected function getShares(ShareAccessContext $accessContext, ?string
8686

8787
protected IUser $user2;
8888

89+
protected TestShareSourceType1 $shareSourceType1;
90+
91+
protected TestShareSourceType2 $shareSourceType2;
92+
8993
private function parseTime(mixed $timestampMs): \DateTimeImmutable {
9094
$timestampMs = (int)$timestampMs;
9195
$time = \DateTimeImmutable::createFromFormat('U.u', number_format((float)$timestampMs / 1000.0, 3, '.', ''));
@@ -128,8 +132,11 @@ public function setUp(): void {
128132

129133
$this->registry = Server::get(ISharingRegistry::class);
130134
$this->registry->clear();
131-
$this->registry->registerSourceType(new TestShareSourceType1(['source1' => 'Source 1']));
132-
$this->registry->registerSourceType(new TestShareSourceType2(['source2' => 'Source 2']));
135+
136+
$this->shareSourceType1 = new TestShareSourceType1(['source1' => 'Source 1']);
137+
$this->registry->registerSourceType($this->shareSourceType1);
138+
$this->shareSourceType2 = new TestShareSourceType2(['source2' => 'Source 2']);
139+
$this->registry->registerSourceType($this->shareSourceType2);
133140
$this->registry->registerRecipientType(new TestShareRecipientType1(
134141
[
135142
'recipient1' => 'Recipient 1',
@@ -3999,4 +4006,53 @@ public function testInitiatorDeleted(): void {
39994006
],
40004007
], $share['recipients']);
40014008
}
4009+
4010+
public function testGetWithDirectAccess(): void {
4011+
$accessContext = new ShareAccessContext($this->owner);
4012+
4013+
$before = $this->manager->generateTimestamp();
4014+
$this->dbConnection->beginTransaction();
4015+
$id = $this->manager->createShare($accessContext);
4016+
$this->manager->addShareSource($accessContext, $id, new ShareSource(TestShareSourceType1::class, 'source1'));
4017+
$this->manager->addShareRecipient($accessContext, $id, new ShareRecipient(TestShareRecipientType1::class, 'recipient1', null));
4018+
$this->manager->getShare($accessContext, $id);
4019+
$this->manager->updateSharePermission($accessContext, $id, new SharePermission(TestSharePermissionType1::class, true));
4020+
$this->manager->updateShareState($accessContext, $id, ShareState::Active);
4021+
4022+
$this->shareSourceType1->userAccess[$this->owner->getUID()] = ['source1'];
4023+
4024+
$this->dbConnection->commit();
4025+
4026+
$after = $this->manager->generateTimestamp();
4027+
4028+
// user2 has no direct access, no shares
4029+
$shares = $this->getShares(new ShareAccessContext(currentUser: $this->user2), TestShareSourceType1::class, 'source1', null, null);
4030+
$this->assertCount(0, $shares);
4031+
4032+
// give user2 direct access, can see shares
4033+
$this->shareSourceType1->userAccess[$this->user2->getUID()] = ['source1'];
4034+
$shares = $this->getShares(new ShareAccessContext(currentUser: $this->user2), TestShareSourceType1::class, 'source1', null, null);
4035+
4036+
$this->assertCount(1, $shares);
4037+
$share = $shares[0];
4038+
4039+
$this->assertGreaterThanOrEqual($before, $share['last_updated']);
4040+
$this->assertLessThanOrEqual($after, $share['last_updated']);
4041+
$this->assertEquals([
4042+
'user_id' => 'owner',
4043+
'instance' => null,
4044+
'display_name' => 'Owner',
4045+
'icon' => [
4046+
'light' => 'http://localhost/index.php/avatar/owner/64',
4047+
'dark' => 'http://localhost/index.php/avatar/owner/64/dark',
4048+
],
4049+
], $share['owner']);
4050+
4051+
// add a source that user2 doesn't have access to, can't see share anymore
4052+
$this->dbConnection->beginTransaction();
4053+
$this->manager->addShareSource($accessContext, $id, new ShareSource(TestShareSourceType2::class, 'source2'));
4054+
$this->dbConnection->commit();
4055+
$shares = $this->getShares(new ShareAccessContext(currentUser: $this->user2), TestShareSourceType1::class, 'source1', null, null);
4056+
$this->assertCount(0, $shares);
4057+
}
40024058
}

‎tests/lib/Sharing/TestShareSourceType1.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ class TestShareSourceType1 implements IShareSourceType {
2121
public function __construct(
2222
/** @var array<string, non-empty-string> $validSources */
2323
private readonly array $validSources,
24+
/** @var array<non-empty-string, non-empty-string[]> $validSources */
25+
public array $userAccess = [],
2426
) {
2527
}
2628

@@ -59,4 +61,10 @@ public function getSourcesMetadata(array $sources): array {
5961
public function getSourceInteractionResource(IUser $user, string $source): InteractionResource {
6062
return new TestInteractionResource($source);
6163
}
64+
65+
#[\Override]
66+
public function userHasDirectSharingAccessToSource(IUser $user, string $source): bool {
67+
$userSources = $this->userAccess[$user->getUID()] ?? [];
68+
return in_array($source, $userSources);
69+
}
6270
}

0 commit comments

Comments
 (0)