Skip to content

[stable30] Fix: sharing attribute resolution #66390

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
backportbot wants to merge 3 commits into
base: stable30
Choose a base branch
from
Open

[stable30] Fix: sharing attribute resolution #66390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fad0f33
fix(sharing): keep reshare attributes consistent with received shares
GhassenKefi Nov 6, 2026
197989f
fix(files): consider all shares of the target when moving mount points
GhassenKefi Nov 6, 2026
f878594
fix(files_sharing): block downloading if needed
GhassenKefi Nov 9, 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
5 changes: 5 additions & 0 deletions apps/files_sharing/lib/Controller/ShareController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,11 @@
return new \OCP\AppFramework\Http\DataResponse('Share has no read permission');
}

$attributes = $share->getAttributes();
if ($attributes?->getAttribute('permissions', 'download') === false) {
return new \OCP\AppFramework\Http\DataResponse('Share has no download permission', \OCP\AppFramework\Http::STATUS_FORBIDDEN);
}

$files_list = null;
if (!is_null($files)) { // download selected files
$files_list = json_decode($files);
Expand Down Expand Up @@ -416,7 +421,7 @@
&& !isset($downloadStartSecret[32])
&& preg_match('!^[a-zA-Z0-9]+$!', $downloadStartSecret) === 1) {
// FIXME: set on the response once we use an actual app framework response
setcookie('ocDownloadStarted', $downloadStartSecret, time() + 20, '/');

Check failure on line 424 in apps/files_sharing/lib/Controller/ShareController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis-security

TaintedCookie 

apps/files_sharing/lib/Controller/ShareController.php:424:35: TaintedCookie: Detected tainted cookie (see https://psalm.dev/257)
}

$this->emitAccessShareHook($share);
Expand Down
5 changes: 5 additions & 0 deletions apps/files_sharing/lib/Updater.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

use OC\Files\Cache\FileAccess;
use OC\Files\Mount\MountPoint;
use OC\Share20\Manager;
use OCP\Constants;
use OCP\Files\Folder;
use OCP\Server;
Expand Down Expand Up @@ -50,6 +51,10 @@ private static function moveShareInOrOutOfShare($path): void {

$shareManager = \OC::$server->getShareManager();

if ($shareManager instanceof Manager) {
$shareManager->restrictDownloadOfReshares($src);
}

// FIXME: should CIRCLES be included here ??
$shares = $shareManager->getSharesBy($user->getUID(), IShare::TYPE_USER, $src, false, -1);
$shares = array_merge($shares, $shareManager->getSharesBy($user->getUID(), IShare::TYPE_GROUP, $src, false, -1));
Expand Down
29 changes: 29 additions & 0 deletions apps/files_sharing/tests/Controller/ShareControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use OCP\Accounts\IAccountManager;
use OCP\Accounts\IAccountProperty;
use OCP\Activity\IManager;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\Http\Template\ExternalShareMenuAction;
use OCP\AppFramework\Http\Template\LinkMenuAction;
Expand All @@ -42,6 +43,7 @@
use OCP\IUserManager;
use OCP\Security\ISecureRandom;
use OCP\Share\Exceptions\ShareNotFound;
use OCP\Share\IAttributes;
use OCP\Share\IPublicShareTemplateFactory;
use OCP\Share\IShare;
use PHPUnit\Framework\MockObject\MockObject;
Expand Down Expand Up @@ -875,6 +877,33 @@ public function testShowShareInvalid() {
$this->shareController->showShare();
}

public function testDownloadShareWithoutDownloadPermission(): void {
$attributes = $this->createMock(IAttributes::class);
$attributes->expects($this->once())
->method('getAttribute')
->with('permissions', 'download')
->willReturn(false);

$share = $this->createMock(IShare::class);
$share->method('getPassword')->willReturn('password');
$share->expects($this->once())
->method('getPermissions')
->willReturn(Constants::PERMISSION_READ);
$share->expects($this->once())
->method('getAttributes')
->willReturn($attributes);

$this->shareManager
->expects($this->once())
->method('getShareByToken')
->with('validtoken')
->willReturn($share);

$response = $this->shareController->downloadShare('validtoken');
$expectedResponse = new DataResponse('Share has no download permission', Http::STATUS_FORBIDDEN);
$this->assertEquals($expectedResponse, $response);
}

public function testDownloadShareWithCreateOnlyShare() {
$share = $this->getMockBuilder(IShare::class)->getMock();
$share->method('getPassword')->willReturn('password');
Expand Down
247 changes: 247 additions & 0 deletions apps/files_sharing/tests/ReshareAttributesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,247 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\Files_Sharing\Tests;

use OC\Files\View;
use OCA\Files_Sharing\Helper;
use OCP\Constants;
use OCP\Files\Config\IUserMountCache;
use OCP\Files\NotFoundException;
use OCP\IDBConnection;
use OCP\Server;
use OCP\Share\IShare;

/**
* @group DB
*/
class ReshareAttributesTest extends TestCase {
public static function setUpBeforeClass(): void {
parent::setUpBeforeClass();
Helper::registerHooks();
}

protected function setUp(): void {
parent::setUp();

$this->view->mkdir('folder');
$this->view->mkdir('folder/sub');
$this->view->file_put_contents('folder/sub/file.txt', $this->data);
}

private function createShare(int $type, string $path, string $initiator, ?string $recipient = null): IShare {
$this->loginHelper($initiator);
if ($recipient !== null) {
return $this->share($type, $path, $initiator, $recipient, Constants::PERMISSION_ALL);
}

$share = $this->shareManager->newShare()
->setShareType($type)
->setSharedBy($initiator)
->setNode($this->rootFolder->getUserFolder($initiator)->get($path))
->setPermissions(Constants::PERMISSION_READ);
return $this->shareManager->createShare($share);
}

private function setDownload(IShare $share, bool $canDownload): void {
$this->loginHelper($share->getSharedBy());
$share = $this->shareManager->getShareById($share->getFullId());
$attributes = $share->getAttributes() ?? $share->newAttributes();
$attributes->setAttribute('permissions', 'download', $canDownload);
$share->setAttributes($attributes);
$this->shareManager->updateShare($share);
}

private function canDownload(IShare $share): bool {
return $this->shareManager->getShareById($share->getFullId())->getAttributes()?->getAttribute('permissions', 'download') !== false;
}

/**
* Moving between storages on object store primary storage creates new
* file ids, so the shares of the moved node no longer resolve.
*/
private function assertCannotServeDownloads(IShare $share): void {
$share = $this->shareManager->getShareById($share->getFullId());
try {
$share->getNode();
$nodeExists = true;
} catch (NotFoundException) {
$nodeExists = false;
}
$this->assertFalse($nodeExists && $share->getAttributes()?->getAttribute('permissions', 'download') !== false);
}

public function testRestrictingDownloadRestrictsReshareChain(): void {
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
$reshare = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER3);
$link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2);
$fileLink = $this->createShare(IShare::TYPE_LINK, 'folder/sub/file.txt', self::TEST_FILES_SHARING_API_USER2);
$chainedLink = $this->createShare(IShare::TYPE_LINK, 'folder/sub', self::TEST_FILES_SHARING_API_USER3);

$this->setDownload($share, false);

$this->assertFalse($this->canDownload($reshare));
$this->assertFalse($this->canDownload($link));
$this->assertFalse($this->canDownload($fileLink));
$this->assertFalse($this->canDownload($chainedLink));
}

public function testRestrictingDownloadKeepsSharesOfTheOwner(): void {
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
$ownerShare = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER3);
$ownerLink = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER1);

$this->setDownload($share, false);

$this->assertTrue($this->canDownload($ownerShare));
$this->assertTrue($this->canDownload($ownerLink));
}

public function testRestrictingDownloadKeepsResharesWithAnotherAllowingShare(): void {
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
$groupShare = $this->createShare(IShare::TYPE_GROUP, 'folder', self::TEST_FILES_SHARING_API_USER1, 'group1');
$link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2);

$this->setDownload($share, false);
$this->assertTrue($this->canDownload($link));

$this->setDownload($groupShare, false);
$this->assertFalse($this->canDownload($link));
}

public function testRestrictingDownloadRestrictsCyclicReshares(): void {
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
$this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER3);
$cyclicShare = $this->createShare(IShare::TYPE_GROUP, 'folder', self::TEST_FILES_SHARING_API_USER3, 'group1');
$link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2);

$this->setDownload($share, false);

$this->assertFalse($this->canDownload($cyclicShare));
$this->assertFalse($this->canDownload($link));
}

public function testRestrictingDownloadKeepsCyclicResharesReachableFromTheOwner(): void {
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
$otherShare = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER3);
$this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER4);
$this->createShare(IShare::TYPE_GROUP, 'folder', self::TEST_FILES_SHARING_API_USER3, 'group1');
$link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2);

$this->setDownload($share, false);
$this->assertTrue($this->canDownload($link));

$this->setDownload($otherShare, false);
$this->assertFalse($this->canDownload($link));
}

public function testRestrictingDownloadOfReshareRestrictsItsReshares(): void {
$this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
$reshare = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER3);
$link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2);
$chainedLink = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER3);

$this->setDownload($reshare, false);

$this->assertTrue($this->canDownload($link));
$this->assertFalse($this->canDownload($chainedLink));
}

public function testAllowingDownloadAgainKeepsResharesRestricted(): void {
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
$link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2);

$this->setDownload($share, false);
$this->setDownload($share, true);

$this->assertFalse($this->canDownload($link));
}

public function testUpdatingPermissionsKeepsResharesUnchanged(): void {
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
$link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2);

$this->loginHelper(self::TEST_FILES_SHARING_API_USER1);
$share = $this->shareManager->getShareById($share->getFullId());
$share->setPermissions(Constants::PERMISSION_READ | Constants::PERMISSION_SHARE);
$this->shareManager->updateShare($share);

$this->assertNull($this->shareManager->getShareById($link->getFullId())->getAttributes());
}

public function testRestrictingDownloadRestrictsResharesOfUnresolvableSharers(): void {
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
$link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2);

$qb = Server::get(IDBConnection::class)->getQueryBuilder();
$qb->update('share')
->set('uid_initiator', $qb->createNamedParameter('non-existing-user'))
->where($qb->expr()->eq('id', $qb->createNamedParameter($link->getId())))
->executeStatement();

$this->setDownload($share, false);

$this->assertFalse($this->canDownload($link));
}

public function testMovingIntoRestrictedShareRestrictsReshares(): void {
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
$this->setDownload($share, false);

$this->loginHelper(self::TEST_FILES_SHARING_API_USER2);
$view = new View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files');
$view->mkdir('own');
$view->file_put_contents('own/file.txt', $this->data);
$link = $this->createShare(IShare::TYPE_LINK, 'own', self::TEST_FILES_SHARING_API_USER2);
$reshare = $this->createShare(IShare::TYPE_USER, 'own', self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER3);
$chainedLink = $this->createShare(IShare::TYPE_LINK, 'own', self::TEST_FILES_SHARING_API_USER3);

$this->loginHelper(self::TEST_FILES_SHARING_API_USER2);
// Start from fresh mounts, as a separate request would
Server::get(IUserMountCache::class)->clear();
$view->rename('own', 'folder/own');

$this->assertCannotServeDownloads($link);
$this->assertCannotServeDownloads($reshare);
$this->assertCannotServeDownloads($chainedLink);
}

public function testMovingByOwnerIntoRestrictedShareRestrictsReshares(): void {
$this->view->mkdir('open');
$this->view->mkdir('open/moved');
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
$this->createShare(IShare::TYPE_USER, 'open', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
$this->setDownload($share, false);
$link = $this->createShare(IShare::TYPE_LINK, 'open/moved', self::TEST_FILES_SHARING_API_USER2);
$this->assertTrue($this->canDownload($link));

$this->loginHelper(self::TEST_FILES_SHARING_API_USER1);
// Start from fresh mounts, as a separate request would
Server::get(IUserMountCache::class)->clear();
$this->view->rename('open/moved', 'folder/moved');

$this->assertFalse($this->canDownload($link));
}

public function testMovingIntoAllowingShareKeepsReshares(): void {
$this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);

$this->loginHelper(self::TEST_FILES_SHARING_API_USER2);
$view = new View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files');
$view->mkdir('own');
$link = $this->createShare(IShare::TYPE_LINK, 'own', self::TEST_FILES_SHARING_API_USER2);

$this->loginHelper(self::TEST_FILES_SHARING_API_USER2);
// Start from fresh mounts, as a separate request would
Server::get(IUserMountCache::class)->clear();
$view->rename('own', 'folder/own');

$this->assertTrue($this->canDownload($link));
}
}
3 changes: 2 additions & 1 deletion lib/private/Files/View.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1861,6 +1861,7 @@ private function targetIsNotShared(string $user, string $targetPath): bool {
$providers = [
IShare::TYPE_USER,
IShare::TYPE_GROUP,
IShare::TYPE_LINK,
IShare::TYPE_EMAIL,
IShare::TYPE_CIRCLE,
IShare::TYPE_ROOM,
Expand All @@ -1870,7 +1871,7 @@ private function targetIsNotShared(string $user, string $targetPath): bool {
$shareManager = Server::get(IManager::class);
/** @var IShare[] $shares */
$shares = array_merge(...array_map(function (int $type) use ($shareManager, $user) {
return $shareManager->getSharesBy($user, $type);
return $shareManager->getSharesBy($user, $type, null, false, -1);
}, $providers));

foreach ($shares as $share) {
Expand Down
Loading
Loading