fix(federatedfilesharing): Do not set the share id for an existing share

Signed-off-by: provokateurin <kate@provokateurin.de>

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)))

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 [];
 	}
 

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')
 				};
 			});

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);