Skip to content

Fix federated reshares #58689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
provokateurin wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix federated reshares #58689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6181c5
fix(federatedfilesharing): Do not set the share id for an existing share
provokateurin Mar 3, 2026
c8fd59c
fix(dav): Use share initiator to get the share node, because the owne…
provokateurin Mar 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions apps/dav/appinfo/v1/publicwebdav.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@ function (\Sabre\DAV\Server $server) use (
}

$share = $authBackend->getShare();
$owner = $share->getShareOwner();
$isReadable = $share->getPermissions() & Constants::PERMISSION_READ;
$fileId = $share->getNodeId();

Expand All @@ -107,7 +106,7 @@ function (\Sabre\DAV\Server $server) use (
Filesystem::logWarningWhenAddingStorageWrapper($previousLog);

$rootFolder = Server::get(IRootFolder::class);
$userFolder = $rootFolder->getUserFolder($owner);
$userFolder = $rootFolder->getUserFolder($share->getSharedBy());
$node = $userFolder->getFirstNodeById($fileId);
if (!$node) {
throw new \Sabre\DAV\Exception\NotFound();
Expand Down
3 changes: 1 addition & 2 deletions apps/dav/appinfo/v2/publicremote.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@
}

$share = $authBackend->getShare();
$owner = $share->getShareOwner();
$isReadable = $share->getPermissions() & Constants::PERMISSION_READ;
$fileId = $share->getNodeId();

Expand Down Expand Up @@ -135,7 +134,7 @@
Filesystem::logWarningWhenAddingStorageWrapper($previousLog);

$rootFolder = Server::get(IRootFolder::class);
$userFolder = $rootFolder->getUserFolder($owner);
$userFolder = $rootFolder->getUserFolder($share->getSharedBy());
$node = $userFolder->getFirstNodeById($fileId);
if (!$node) {
throw new NotFound();
Expand Down
2 changes: 1 addition & 1 deletion apps/dav/lib/Files/Sharing/PublicLinkCheckPlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public function initialize(\Sabre\DAV\Server $server) {
}

public function beforeMethod(RequestInterface $request, ResponseInterface $response) {
// verify that the owner didn't have their share permissions revoked
// verify that the initiator didn't have their share permissions revoked
if ($this->fileInfo && !$this->fileInfo->isShareable()) {
throw new NotFound();
}
Expand Down
81 changes: 22 additions & 59 deletions apps/federatedfilesharing/lib/FederatedShareProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,30 +128,30 @@ public function create(IShare $share): IShare {
$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);
Copy link
Contributor

@salmart-dev salmart-dev Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: I think that removing this will lead to a stale entry in oc_share if the remote call fails for any reason with an Exception

Copy link
Member Author

@provokateurin provokateurin Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this was always wrong. Basically when resharing fails, the original share was also deleted. This was a separate bug besides the fact that resharing didn't work in the first place.

Copy link
Contributor

@salmart-dev salmart-dev Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original share was deleted because the code causing the exception was setId, but the row in oc_share for Server B to Server C got created in the line above. If the request to the original server fails, we should remove that row, otherwise Server B displays that the file is shared with Server C, but that might not be true.

Copy link
Contributor

@salmart-dev salmart-dev Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could still be a case where the remote server managed to store the share information but failed afterwards, so removing the share here could lead to an inconsistency in that case, which in connection with my other comment could lead to still having a broken situation 🤔

$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);
Copy link
Contributor

@salmart-dev salmart-dev Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment: noticing that there is no handling for the case where the oc_share entry was missing but the remote had a matching share. This could happen if the current server's DB would be restored to a state without that specific row. I don't see any code that would handle a "share already exists on the remote, track it on this server" but I may have missed it.

$message_t = $this->l->t('File is already shared with %s', [$shareWith]);
throw new \Exception($message_t);
}
Expand Down Expand Up @@ -216,7 +216,7 @@ protected function createFederatedShare(IShare $share): string {
}

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);
Expand All @@ -225,45 +225,17 @@ protected function createFederatedShare(IShare $share): string {
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();
Expand Down Expand Up @@ -453,7 +425,7 @@ public function delete(IShare $share) {

// 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());
}

/**
Expand Down Expand Up @@ -483,19 +455,10 @@ protected function revokeShare($share, $isOwner) {
}
}

/**
* 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)))
Expand Down
4 changes: 2 additions & 2 deletions apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ protected function shareDeclined(string $id, array $notification): array {
* @throws ShareNotFound
*/
protected function executeDeclineShare(IShare $share): void {
$this->federatedShareProvider->removeShareFromTable($share);
$this->federatedShareProvider->removeShareFromTable($share->getId());

$user = $this->getCorrectUser($share);

Expand Down Expand Up @@ -420,7 +420,7 @@ private function undoReshare(string $id, array $notification): array {
$share = $this->federatedShareProvider->getShareById($id);

$this->verifyShare($share, $token);
$this->federatedShareProvider->removeShareFromTable($share);
$this->federatedShareProvider->removeShareFromTable($share->getId());
return [];
}

Expand Down
51 changes: 34 additions & 17 deletions apps/federatedfilesharing/tests/FederatedShareProviderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,8 @@ public function testCreate(?\DateTime $expirationDate, ?string $expectedDataDate
->setPermissions(19)
->setShareType(IShare::TYPE_REMOTE)
->setExpirationDate($expirationDate)
->setNode($node);
->setNode($node)
->setTarget('');

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

Expand Down Expand Up @@ -215,7 +216,8 @@ public function testCreateCouldNotFindServer(): void {
->setShareOwner('shareOwner')
->setPermissions(19)
->setShareType(IShare::TYPE_REMOTE)
->setNode($node);
->setNode($node)
->setTarget('');

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

Expand Down Expand Up @@ -268,7 +270,8 @@ public function testCreateException(): void {
->setShareOwner('shareOwner')
->setPermissions(19)
->setShareType(IShare::TYPE_REMOTE)
->setNode($node);
->setNode($node)
->setTarget('');

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

Expand Down Expand Up @@ -359,7 +362,8 @@ public function testCreateAlreadyShared(): void {
->setShareOwner('shareOwner')
->setPermissions(19)
->setShareType(IShare::TYPE_REMOTE)
->setNode($node);
->setNode($node)
->setTarget('');

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

Expand Down Expand Up @@ -431,7 +435,8 @@ public function testUpdate(string $owner, string $sharedBy, ?\DateTime $expirati
->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')
Expand Down Expand Up @@ -508,7 +513,8 @@ public function testGetSharedBy(): void {
->setShareOwner('shareOwner')
->setPermissions(19)
->setShareType(IShare::TYPE_REMOTE)
->setNode($node);
->setNode($node)
->setTarget('');
$this->provider->create($share);

$share2 = $this->shareManager->newShare();
Expand All @@ -517,7 +523,8 @@ public function testGetSharedBy(): void {
->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);
Expand Down Expand Up @@ -552,7 +559,8 @@ public function testGetSharedByWithNode(): void {
->setShareOwner('shareOwner')
->setPermissions(19)
->setShareType(IShare::TYPE_REMOTE)
->setNode($node);
->setNode($node)
->setTarget('');
$this->provider->create($share);

$node2 = $this->getMockBuilder(File::class)->getMock();
Expand All @@ -565,7 +573,8 @@ public function testGetSharedByWithNode(): void {
->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);
Expand Down Expand Up @@ -599,7 +608,8 @@ public function testGetSharedByWithReshares(): void {
->setShareOwner('shareOwner')
->setPermissions(19)
->setShareType(IShare::TYPE_REMOTE)
->setNode($node);
->setNode($node)
->setTarget('');
$this->provider->create($share);

$share2 = $this->shareManager->newShare();
Expand All @@ -608,7 +618,8 @@ public function testGetSharedByWithReshares(): void {
->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);
Expand Down Expand Up @@ -649,7 +660,8 @@ public function testGetSharedByWithLimit(): void {
->setShareOwner('shareOwner')
->setPermissions(19)
->setShareType(IShare::TYPE_REMOTE)
->setNode($node);
->setNode($node)
->setTarget('');
$this->provider->create($share);

$share2 = $this->shareManager->newShare();
Expand All @@ -658,7 +670,8 @@ public function testGetSharedByWithLimit(): void {
->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);
Expand Down Expand Up @@ -839,7 +852,8 @@ public function testGetSharesInFolder(): void {
->setShareOwner($u1->getUID())
->setPermissions(Constants::PERMISSION_READ)
->setShareType(IShare::TYPE_REMOTE)
->setNode($file1);
->setNode($file1)
->setTarget('');
$this->provider->create($share1);

$share2 = $this->shareManager->newShare();
Expand All @@ -848,7 +862,8 @@ public function testGetSharesInFolder(): void {
->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);
Expand Down Expand Up @@ -899,7 +914,8 @@ public function testGetAccessList(): void {
->setShareOwner($u1->getUID())
->setPermissions(Constants::PERMISSION_READ)
->setShareType(IShare::TYPE_REMOTE)
->setNode($file1);
->setNode($file1)
->setTarget('');
$this->provider->create($share1);

$share2 = $this->shareManager->newShare();
Expand All @@ -908,7 +924,8 @@ public function testGetAccessList(): void {
->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);
Expand Down
Loading