From 045ad43237c8ee6b349aa0df0a4e59cf2907d9f3 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Tue, 3 Mar 2026 15:25:03 +0100 Subject: [PATCH] fix(federatedfilesharing): Do not set the share id for an existing share Signed-off-by: provokateurin --- .../lib/FederatedShareProvider.php | 81 +++++-------------- .../lib/OCM/CloudFederationProviderFiles.php | 4 +- ...ederatedShareProviderReshareRemoteTest.php | 10 +-- .../tests/FederatedShareProviderTest.php | 51 ++++++++---- 4 files changed, 63 insertions(+), 83 deletions(-) diff --git a/apps/federatedfilesharing/lib/FederatedShareProvider.php b/apps/federatedfilesharing/lib/FederatedShareProvider.php index 59087efaca5..1302f2ca980 100644 --- a/apps/federatedfilesharing/lib/FederatedShareProvider.php +++ b/apps/federatedfilesharing/lib/FederatedShareProvider.php @@ -128,30 +128,30 @@ class FederatedShareProvider implements IShareProvider, IShareProviderSupportsAl $share->setSharedWith($cloudId->getId()); try { - $remoteShare = $this->getShareFromExternalShareTable($share); + $remoteShare = $this->getShareFromExternalShareTable($share->getShareOwner(), $share->getTarget()); } catch (ShareNotFound $e) { $remoteShare = null; } if ($remoteShare) { - try { - $ownerCloudId = $this->cloudIdManager->getCloudId($remoteShare['owner'], $remoteShare['remote']); - $shareId = $this->addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $ownerCloudId->getId(), $permissions, 'tmp_token_' . time(), $shareType, $expirationDate); - $share->setId($shareId); - [$token, $remoteId] = $this->askOwnerToReShare($shareWith, $share, $shareId); - // remote share was create successfully if we get a valid token as return - $send = is_string($token) && $token !== ''; - } catch (\Exception $e) { - // fall back to old re-share behavior if the remote server - // doesn't support flat re-shares (was introduced with Nextcloud 9.1) - $this->removeShareFromTable($share); - $shareId = $this->createFederatedShare($share); - } - if ($send) { + $ownerCloudId = $this->cloudIdManager->getCloudId($remoteShare['owner'], $remoteShare['remote']); + $shareId = $this->addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $ownerCloudId->getId(), $permissions, 'tmp_token_' . time(), $shareType, $expirationDate); + [$token, $remoteId] = $this->notifications->requestReShare( + $remoteShare['share_token'], + $remoteShare['remote_id'], + $shareId, + $remoteShare['remote'], + $shareWith, + $permissions, + $share->getNode()->getName(), + $shareType, + ); + // remote share was create successfully if we get a valid token as return + if (is_string($token) && $token !== '') { $this->updateSuccessfulReshare($shareId, $token); $this->storeRemoteId($shareId, $remoteId); } else { - $this->removeShareFromTable($share); + $this->removeShareFromTable($shareId); $message_t = $this->l->t('File is already shared with %s', [$shareWith]); throw new \Exception($message_t); } @@ -216,7 +216,7 @@ class FederatedShareProvider implements IShareProvider, IShareProviderSupportsAl } if ($failure) { - $this->removeShareFromTableById($shareId); + $this->removeShareFromTable($shareId); $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.', [$share->getNode()->getName(), $share->getSharedWith()]); throw new \Exception($message_t); @@ -225,45 +225,17 @@ class FederatedShareProvider implements IShareProvider, IShareProviderSupportsAl return $shareId; } - /** - * @param string $shareWith - * @param IShare $share - * @param string $shareId internal share Id - * @return array - * @throws \Exception - */ - protected function askOwnerToReShare($shareWith, IShare $share, $shareId) { - $remoteShare = $this->getShareFromExternalShareTable($share); - $token = $remoteShare['share_token']; - $remoteId = $remoteShare['remote_id']; - $remote = $remoteShare['remote']; - - [$token, $remoteId] = $this->notifications->requestReShare( - $token, - $remoteId, - $shareId, - $remote, - $shareWith, - $share->getPermissions(), - $share->getNode()->getName(), - $share->getShareType(), - ); - - return [$token, $remoteId]; - } - /** * get federated share from the share_external table but exclude mounted link shares * - * @param IShare $share * @return array * @throws ShareNotFound */ - protected function getShareFromExternalShareTable(IShare $share) { + protected function getShareFromExternalShareTable(string $owner, string $target) { $query = $this->dbConnection->getQueryBuilder(); $query->select('*')->from($this->externalShareTable) - ->where($query->expr()->eq('user', $query->createNamedParameter($share->getShareOwner()))) - ->andWhere($query->expr()->eq('mountpoint', $query->createNamedParameter($share->getTarget()))); + ->where($query->expr()->eq('user', $query->createNamedParameter($owner))) + ->andWhere($query->expr()->eq('mountpoint', $query->createNamedParameter($target))); $qResult = $query->executeQuery(); $result = $qResult->fetchAllAssociative(); $qResult->closeCursor(); @@ -468,7 +440,7 @@ class FederatedShareProvider implements IShareProvider, IShareProviderSupportsAl // only remove the share when all messages are send to not lose information // about the share to early - $this->removeShareFromTable($share); + $this->removeShareFromTable($share->getId()); } /** @@ -493,19 +465,10 @@ class FederatedShareProvider implements IShareProvider, IShareProviderSupportsAl } } - /** - * remove share from table - * - * @param IShare $share - */ - public function removeShareFromTable(IShare $share) { - $this->removeShareFromTableById($share->getId()); - } - /** * Remove share from table. */ - private function removeShareFromTableById(string $shareId): void { + public function removeShareFromTable(string $shareId): void { $qb = $this->dbConnection->getQueryBuilder(); $qb->delete('share') ->where($qb->expr()->eq('id', $qb->createNamedParameter($shareId))) diff --git a/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php b/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php index a5ae8c87abd..3ae6366fabe 100644 --- a/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php +++ b/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php @@ -382,7 +382,7 @@ class CloudFederationProviderFiles implements ISignedCloudFederationProvider { * @throws ShareNotFound */ protected function executeDeclineShare(IShare $share): void { - $this->federatedShareProvider->removeShareFromTable($share); + $this->federatedShareProvider->removeShareFromTable($share->getId()); $user = $this->getCorrectUser($share); @@ -420,7 +420,7 @@ class CloudFederationProviderFiles implements ISignedCloudFederationProvider { $share = $this->federatedShareProvider->getShareById($id); $this->verifyShare($share, $token); - $this->federatedShareProvider->removeShareFromTable($share); + $this->federatedShareProvider->removeShareFromTable($share->getId()); return []; } diff --git a/apps/federatedfilesharing/tests/FederatedShareProviderReshareRemoteTest.php b/apps/federatedfilesharing/tests/FederatedShareProviderReshareRemoteTest.php index 65cecaf1580..deb0938578a 100644 --- a/apps/federatedfilesharing/tests/FederatedShareProviderReshareRemoteTest.php +++ b/apps/federatedfilesharing/tests/FederatedShareProviderReshareRemoteTest.php @@ -249,18 +249,18 @@ class FederatedShareProviderReshareRemoteTest extends \Test\TestCase { $qb6->method('executeQuery')->willReturn($result6); - $queryBuilderMatcher = $this->exactly(8); + $queryBuilderMatcher = $this->exactly(7); $this->connection ->expects($queryBuilderMatcher) ->method('getQueryBuilder') ->willReturnCallback(function () use ($queryBuilderMatcher, $qb1, $qb2, $qb3, $qb4, $qb5, $qb6) { return match ($queryBuilderMatcher->numberOfInvocations()) { 1, 2 => $qb1, - 3, 5 => $qb2, + 3 => $qb2, 4 => $qb3, - 6 => $qb4, - 7 => $qb5, - 8 => $qb6, + 5 => $qb4, + 6 => $qb5, + 7 => $qb6, default => throw new LogicException('Unexpected number of invocations for getQueryBuilder') }; }); diff --git a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php index b5cb8a85a53..da37617c121 100644 --- a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php +++ b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php @@ -133,7 +133,8 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setPermissions(19) ->setShareType(IShare::TYPE_REMOTE) ->setExpirationDate($expirationDate) - ->setNode($node); + ->setNode($node) + ->setTarget(''); $this->tokenHandler->method('generateToken')->willReturn('token'); @@ -215,7 +216,8 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setShareOwner('shareOwner') ->setPermissions(19) ->setShareType(IShare::TYPE_REMOTE) - ->setNode($node); + ->setNode($node) + ->setTarget(''); $this->tokenHandler->method('generateToken')->willReturn('token'); @@ -268,7 +270,8 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setShareOwner('shareOwner') ->setPermissions(19) ->setShareType(IShare::TYPE_REMOTE) - ->setNode($node); + ->setNode($node) + ->setTarget(''); $this->tokenHandler->method('generateToken')->willReturn('token'); @@ -359,7 +362,8 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setShareOwner('shareOwner') ->setPermissions(19) ->setShareType(IShare::TYPE_REMOTE) - ->setNode($node); + ->setNode($node) + ->setTarget(''); $this->tokenHandler->method('generateToken')->willReturn('token'); @@ -431,7 +435,8 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setPermissions(19) ->setShareType(IShare::TYPE_REMOTE) ->setExpirationDate(new \DateTime('2019-02-01T01:02:03')) - ->setNode($node); + ->setNode($node) + ->setTarget(''); $this->tokenHandler->method('generateToken')->willReturn('token'); $this->addressHandler->expects($this->any())->method('generateRemoteURL') @@ -504,7 +509,8 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setShareOwner('shareOwner') ->setPermissions(19) ->setShareType(IShare::TYPE_REMOTE) - ->setNode($node); + ->setNode($node) + ->setTarget(''); $this->provider->create($share); $share2 = $this->shareManager->newShare(); @@ -513,7 +519,8 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setShareOwner('shareOwner') ->setPermissions(19) ->setShareType(IShare::TYPE_REMOTE) - ->setNode($node); + ->setNode($node) + ->setTarget(''); $this->provider->create($share2); $shares = $this->provider->getSharesBy('sharedBy', IShare::TYPE_REMOTE, null, false, -1, 0); @@ -548,7 +555,8 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setShareOwner('shareOwner') ->setPermissions(19) ->setShareType(IShare::TYPE_REMOTE) - ->setNode($node); + ->setNode($node) + ->setTarget(''); $this->provider->create($share); $node2 = $this->getMockBuilder(File::class)->getMock(); @@ -561,7 +569,8 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setShareOwner('shareOwner') ->setPermissions(19) ->setShareType(IShare::TYPE_REMOTE) - ->setNode($node2); + ->setNode($node2) + ->setTarget(''); $this->provider->create($share2); $shares = $this->provider->getSharesBy('sharedBy', IShare::TYPE_REMOTE, $node2, false, -1, 0); @@ -595,7 +604,8 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setShareOwner('shareOwner') ->setPermissions(19) ->setShareType(IShare::TYPE_REMOTE) - ->setNode($node); + ->setNode($node) + ->setTarget(''); $this->provider->create($share); $share2 = $this->shareManager->newShare(); @@ -604,7 +614,8 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setShareOwner('shareOwner') ->setPermissions(19) ->setShareType(IShare::TYPE_REMOTE) - ->setNode($node); + ->setNode($node) + ->setTarget(''); $this->provider->create($share2); $shares = $this->provider->getSharesBy('shareOwner', IShare::TYPE_REMOTE, null, true, -1, 0); @@ -645,7 +656,8 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setShareOwner('shareOwner') ->setPermissions(19) ->setShareType(IShare::TYPE_REMOTE) - ->setNode($node); + ->setNode($node) + ->setTarget(''); $this->provider->create($share); $share2 = $this->shareManager->newShare(); @@ -654,7 +666,8 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setShareOwner('shareOwner') ->setPermissions(19) ->setShareType(IShare::TYPE_REMOTE) - ->setNode($node); + ->setNode($node) + ->setTarget(''); $this->provider->create($share2); $shares = $this->provider->getSharesBy('shareOwner', IShare::TYPE_REMOTE, null, true, 1, 1); @@ -835,7 +848,8 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setShareOwner($u1->getUID()) ->setPermissions(Constants::PERMISSION_READ) ->setShareType(IShare::TYPE_REMOTE) - ->setNode($file1); + ->setNode($file1) + ->setTarget(''); $this->provider->create($share1); $share2 = $this->shareManager->newShare(); @@ -844,7 +858,8 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setShareOwner($u1->getUID()) ->setPermissions(Constants::PERMISSION_READ) ->setShareType(IShare::TYPE_REMOTE) - ->setNode($file2); + ->setNode($file2) + ->setTarget(''); $this->provider->create($share2); $result = $this->provider->getSharesInFolder($u1->getUID(), $folder1, false); @@ -895,7 +910,8 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setShareOwner($u1->getUID()) ->setPermissions(Constants::PERMISSION_READ) ->setShareType(IShare::TYPE_REMOTE) - ->setNode($file1); + ->setNode($file1) + ->setTarget(''); $this->provider->create($share1); $share2 = $this->shareManager->newShare(); @@ -904,7 +920,8 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setShareOwner($u1->getUID()) ->setPermissions(Constants::PERMISSION_READ) ->setShareType(IShare::TYPE_REMOTE) - ->setNode($file1); + ->setNode($file1) + ->setTarget(''); $this->provider->create($share2); $result = $this->provider->getAccessList([$file1], true);