Skip to content

Commit 8041189

Browse files
provokateurinprovokateurin
committed
fix(federatedfilesharing): Do not set the share id for an existing share
Signed-off-by: provokateurin <kate@provokateurin.de>
1 parent a2458ff commit 8041189

3 files changed

Lines changed: 60 additions & 82 deletions

File tree

‎apps/federatedfilesharing/lib/FederatedShareProvider.php‎

Lines changed: 23 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -132,30 +132,30 @@ public function create(IShare $share) {
132132
$share->setSharedWith($cloudId->getId());
133133

134134
try {
135-
$remoteShare = $this->getShareFromExternalShareTable($share);
135+
$remoteShare = $this->getShareFromExternalShareTable($share->getShareOwner(), $share->getTarget());
136136
} catch (ShareNotFound $e) {
137137
$remoteShare = null;
138138
}
139139

140140
if ($remoteShare) {
141-
try {
142-
$ownerCloudId = $this->cloudIdManager->getCloudId($remoteShare['owner'], $remoteShare['remote']);
143-
$shareId = $this->addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $ownerCloudId->getId(), $permissions, 'tmp_token_' . time(), $shareType, $expirationDate);
144-
$share->setId($shareId);
145-
[$token, $remoteId] = $this->askOwnerToReShare($shareWith, $share, $shareId);
146-
// remote share was create successfully if we get a valid token as return
147-
$send = is_string($token) && $token !== '';
148-
} catch (\Exception $e) {
149-
// fall back to old re-share behavior if the remote server
150-
// doesn't support flat re-shares (was introduced with Nextcloud 9.1)
151-
$this->removeShareFromTable($share);
152-
$shareId = $this->createFederatedShare($share);
153-
}
154-
if ($send) {
141+
$ownerCloudId = $this->cloudIdManager->getCloudId($remoteShare['owner'], $remoteShare['remote']);
142+
$shareId = $this->addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $ownerCloudId->getId(), $permissions, 'tmp_token_' . time(), $shareType, $expirationDate);
143+
[$token, $remoteId] = $this->notifications->requestReShare(
144+
$remoteShare['share_token'],
145+
$remoteShare['remote_id'],
146+
$shareId,
147+
$remoteShare['remote'],
148+
$shareWith,
149+
$permissions,
150+
$share->getNode()->getName(),
151+
$shareType,
152+
);
153+
// remote share was create successfully if we get a valid token as return
154+
if (is_string($token) && $token !== '') {
155155
$this->updateSuccessfulReshare($shareId, $token);
156156
$this->storeRemoteId($shareId, $remoteId);
157157
} else {
158-
$this->removeShareFromTable($share);
158+
$this->removeShareFromTable($shareId);
159159
$message_t = $this->l->t('File is already shared with %s', [$shareWith]);
160160
throw new \Exception($message_t);
161161
}
@@ -222,7 +222,7 @@ protected function createFederatedShare(IShare $share) {
222222
}
223223

224224
if ($failure) {
225-
$this->removeShareFromTableById($shareId);
225+
$this->removeShareFromTable($shareId);
226226
$message_t = $this->l->t('Sharing %1$s failed, could not find %2$s, maybe the server is currently unreachable or uses a self-signed certificate.',
227227
[$share->getNode()->getName(), $share->getSharedWith()]);
228228
throw new \Exception($message_t);
@@ -231,45 +231,17 @@ protected function createFederatedShare(IShare $share) {
231231
return $shareId;
232232
}
233233

234-
/**
235-
* @param string $shareWith
236-
* @param IShare $share
237-
* @param string $shareId internal share Id
238-
* @return array
239-
* @throws \Exception
240-
*/
241-
protected function askOwnerToReShare($shareWith, IShare $share, $shareId) {
242-
$remoteShare = $this->getShareFromExternalShareTable($share);
243-
$token = $remoteShare['share_token'];
244-
$remoteId = $remoteShare['remote_id'];
245-
$remote = $remoteShare['remote'];
246-
247-
[$token, $remoteId] = $this->notifications->requestReShare(
248-
$token,
249-
$remoteId,
250-
$shareId,
251-
$remote,
252-
$shareWith,
253-
$share->getPermissions(),
254-
$share->getNode()->getName(),
255-
$share->getShareType(),
256-
);
257-
258-
return [$token, $remoteId];
259-
}
260-
261234
/**
262235
* get federated share from the share_external table but exclude mounted link shares
263236
*
264-
* @param IShare $share
265237
* @return array
266238
* @throws ShareNotFound
267239
*/
268-
protected function getShareFromExternalShareTable(IShare $share) {
240+
protected function getShareFromExternalShareTable(string $owner, string $target) {
269241
$query = $this->dbConnection->getQueryBuilder();
270242
$query->select('*')->from($this->externalShareTable)
271-
->where($query->expr()->eq('user', $query->createNamedParameter($share->getShareOwner())))
272-
->andWhere($query->expr()->eq('mountpoint', $query->createNamedParameter($share->getTarget())));
243+
->where($query->expr()->eq('user', $query->createNamedParameter($owner)))
244+
->andWhere($query->expr()->eq('mountpoint', $query->createNamedParameter($target)));
273245
$qResult = $query->executeQuery();
274246
$result = $qResult->fetchAll();
275247
$qResult->closeCursor();
@@ -472,7 +444,7 @@ public function delete(IShare $share) {
472444

473445
// only remove the share when all messages are send to not lose information
474446
// about the share to early
475-
$this->removeShareFromTable($share);
447+
$this->removeShareFromTable($share->getId());
476448
}
477449

478450
/**
@@ -503,20 +475,9 @@ protected function revokeShare($share, $isOwner) {
503475
}
504476

505477
/**
506-
* remove share from table
507-
*
508-
* @param IShare $share
509-
*/
510-
public function removeShareFromTable(IShare $share) {
511-
$this->removeShareFromTableById($share->getId());
512-
}
513-
514-
/**
515-
* remove share from table
516-
*
517-
* @param string $shareId
478+
* Remove share from table.
518479
*/
519-
private function removeShareFromTableById($shareId) {
480+
public function removeShareFromTable(int $shareId): void {
520481
$qb = $this->dbConnection->getQueryBuilder();
521482
$qb->delete('share')
522483
->where($qb->expr()->eq('id', $qb->createNamedParameter($shareId)))

‎apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -410,8 +410,8 @@ protected function shareDeclined($id, array $notification) {
410410
* @param IShare $share
411411
* @throws ShareNotFound
412412
*/
413-
protected function executeDeclineShare(IShare $share) {
414-
$this->federatedShareProvider->removeShareFromTable($share);
413+
protected function executeDeclineShare(IShare $share): void {
414+
$this->federatedShareProvider->removeShareFromTable($share->getId());
415415

416416
try {
417417
$fileId = (int)$share->getNode()->getId();
@@ -448,7 +448,7 @@ private function undoReshare($id, array $notification) {
448448
$share = $this->federatedShareProvider->getShareById($id);
449449

450450
$this->verifyShare($share, $token);
451-
$this->federatedShareProvider->removeShareFromTable($share);
451+
$this->federatedShareProvider->removeShareFromTable($share->getId());
452452
return [];
453453
}
454454

‎apps/federatedfilesharing/tests/FederatedShareProviderTest.php‎

Lines changed: 34 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,8 @@ public function testCreate(?\DateTime $expirationDate, ?string $expectedDataDate
132132
->setPermissions(19)
133133
->setShareType(IShare::TYPE_REMOTE)
134134
->setExpirationDate($expirationDate)
135-
->setNode($node);
135+
->setNode($node)
136+
->setTarget('');
136137

137138
$this->tokenHandler->method('generateToken')->willReturn('token');
138139

@@ -213,7 +214,8 @@ public function testCreateCouldNotFindServer(): void {
213214
->setShareOwner('shareOwner')
214215
->setPermissions(19)
215216
->setShareType(IShare::TYPE_REMOTE)
216-
->setNode($node);
217+
->setNode($node)
218+
->setTarget('');
217219

218220
$this->tokenHandler->method('generateToken')->willReturn('token');
219221

@@ -274,7 +276,8 @@ public function testCreateException(): void {
274276
->setShareOwner('shareOwner')
275277
->setPermissions(19)
276278
->setShareType(IShare::TYPE_REMOTE)
277-
->setNode($node);
279+
->setNode($node)
280+
->setTarget('');
278281

279282
$this->tokenHandler->method('generateToken')->willReturn('token');
280283

@@ -382,7 +385,8 @@ public function testCreateAlreadyShared(): void {
382385
->setShareOwner('shareOwner')
383386
->setPermissions(19)
384387
->setShareType(IShare::TYPE_REMOTE)
385-
->setNode($node);
388+
->setNode($node)
389+
->setTarget('');
386390

387391
$this->tokenHandler->method('generateToken')->willReturn('token');
388392

@@ -454,7 +458,8 @@ public function testUpdate(string $owner, string $sharedBy, ?\DateTime $expirati
454458
->setPermissions(19)
455459
->setShareType(IShare::TYPE_REMOTE)
456460
->setExpirationDate(new \DateTime('2019-02-01T01:02:03'))
457-
->setNode($node);
461+
->setNode($node)
462+
->setTarget('');
458463

459464
$this->tokenHandler->method('generateToken')->willReturn('token');
460465
$this->addressHandler->expects($this->any())->method('generateRemoteURL')
@@ -531,7 +536,8 @@ public function testGetSharedBy(): void {
531536
->setShareOwner('shareOwner')
532537
->setPermissions(19)
533538
->setShareType(IShare::TYPE_REMOTE)
534-
->setNode($node);
539+
->setNode($node)
540+
->setTarget('');
535541
$this->provider->create($share);
536542

537543
$share2 = $this->shareManager->newShare();
@@ -540,7 +546,8 @@ public function testGetSharedBy(): void {
540546
->setShareOwner('shareOwner')
541547
->setPermissions(19)
542548
->setShareType(IShare::TYPE_REMOTE)
543-
->setNode($node);
549+
->setNode($node)
550+
->setTarget('');
544551
$this->provider->create($share2);
545552

546553
$shares = $this->provider->getSharesBy('sharedBy', IShare::TYPE_REMOTE, null, false, -1, 0);
@@ -575,7 +582,8 @@ public function testGetSharedByWithNode(): void {
575582
->setShareOwner('shareOwner')
576583
->setPermissions(19)
577584
->setShareType(IShare::TYPE_REMOTE)
578-
->setNode($node);
585+
->setNode($node)
586+
->setTarget('');
579587
$this->provider->create($share);
580588

581589
$node2 = $this->getMockBuilder(File::class)->getMock();
@@ -588,7 +596,8 @@ public function testGetSharedByWithNode(): void {
588596
->setShareOwner('shareOwner')
589597
->setPermissions(19)
590598
->setShareType(IShare::TYPE_REMOTE)
591-
->setNode($node2);
599+
->setNode($node2)
600+
->setTarget('');
592601
$this->provider->create($share2);
593602

594603
$shares = $this->provider->getSharesBy('sharedBy', IShare::TYPE_REMOTE, $node2, false, -1, 0);
@@ -622,7 +631,8 @@ public function testGetSharedByWithReshares(): void {
622631
->setShareOwner('shareOwner')
623632
->setPermissions(19)
624633
->setShareType(IShare::TYPE_REMOTE)
625-
->setNode($node);
634+
->setNode($node)
635+
->setTarget('');
626636
$this->provider->create($share);
627637

628638
$share2 = $this->shareManager->newShare();
@@ -631,7 +641,8 @@ public function testGetSharedByWithReshares(): void {
631641
->setShareOwner('shareOwner')
632642
->setPermissions(19)
633643
->setShareType(IShare::TYPE_REMOTE)
634-
->setNode($node);
644+
->setNode($node)
645+
->setTarget('');
635646
$this->provider->create($share2);
636647

637648
$shares = $this->provider->getSharesBy('shareOwner', IShare::TYPE_REMOTE, null, true, -1, 0);
@@ -672,7 +683,8 @@ public function testGetSharedByWithLimit(): void {
672683
->setShareOwner('shareOwner')
673684
->setPermissions(19)
674685
->setShareType(IShare::TYPE_REMOTE)
675-
->setNode($node);
686+
->setNode($node)
687+
->setTarget('');
676688
$this->provider->create($share);
677689

678690
$share2 = $this->shareManager->newShare();
@@ -681,7 +693,8 @@ public function testGetSharedByWithLimit(): void {
681693
->setShareOwner('shareOwner')
682694
->setPermissions(19)
683695
->setShareType(IShare::TYPE_REMOTE)
684-
->setNode($node);
696+
->setNode($node)
697+
->setTarget('');
685698
$this->provider->create($share2);
686699

687700
$shares = $this->provider->getSharesBy('shareOwner', IShare::TYPE_REMOTE, null, true, 1, 1);
@@ -862,7 +875,8 @@ public function testGetSharesInFolder(): void {
862875
->setShareOwner($u1->getUID())
863876
->setPermissions(Constants::PERMISSION_READ)
864877
->setShareType(IShare::TYPE_REMOTE)
865-
->setNode($file1);
878+
->setNode($file1)
879+
->setTarget('');
866880
$this->provider->create($share1);
867881

868882
$share2 = $this->shareManager->newShare();
@@ -871,7 +885,8 @@ public function testGetSharesInFolder(): void {
871885
->setShareOwner($u1->getUID())
872886
->setPermissions(Constants::PERMISSION_READ)
873887
->setShareType(IShare::TYPE_REMOTE)
874-
->setNode($file2);
888+
->setNode($file2)
889+
->setTarget('');
875890
$this->provider->create($share2);
876891

877892
$result = $this->provider->getSharesInFolder($u1->getUID(), $folder1, false);
@@ -922,7 +937,8 @@ public function testGetAccessList(): void {
922937
->setShareOwner($u1->getUID())
923938
->setPermissions(Constants::PERMISSION_READ)
924939
->setShareType(IShare::TYPE_REMOTE)
925-
->setNode($file1);
940+
->setNode($file1)
941+
->setTarget('');
926942
$this->provider->create($share1);
927943

928944
$share2 = $this->shareManager->newShare();
@@ -931,7 +947,8 @@ public function testGetAccessList(): void {
931947
->setShareOwner($u1->getUID())
932948
->setPermissions(Constants::PERMISSION_READ)
933949
->setShareType(IShare::TYPE_REMOTE)
934-
->setNode($file1);
950+
->setNode($file1)
951+
->setTarget('');
935952
$this->provider->create($share2);
936953

937954
$result = $this->provider->getAccessList([$file1], true);

0 commit comments

Comments
 (0)