fix(sharing): restore STATUS_ACCEPTED for OC-migrated group share subshares

When an ownCloud-migrated group share (which has no per-user USERGROUP
subshare) is renamed for the first time, DefaultShareProvider::move()
inserted a new USERGROUP row without setting `accepted`. The column
defaulted to 0 (STATUS_PENDING), causing MountProvider to skip the
share on the next login — the shared file disappeared for the recipient.

Fix: set accepted = STATUS_ACCEPTED explicitly on the INSERT in
DefaultShareProvider::move() for the TYPE_GROUP branch.

Secondary fix: SharedMount::moveMount() silently returned true when
updateFileTarget() threw (e.g. group no longer exists on an OC-migrated
instance). Set $result = false in the catch block so View::rename()
propagates the failure instead of silently corrupting VFS state.

An opt-in occ command (sharing:fix-owncloud-group-shares) with --dry-run
support is included to repair existing broken instances. It targets only
TYPE_USERGROUP subshares with accepted=STATUS_PENDING and permissions!=0
(shares that were accepted but broken by the missing column default),
leaving explicitly declined shares (permissions=0) untouched.

AI-Assisted-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Anna Larch <anna@nextcloud.com>
This commit is contained in:
Anna Larch 2026-05-19 15:44:40 +02:00 committed by Anna
parent 0a4d4bc8ab
commit 6220ae9175
9 changed files with 335 additions and 1 deletions

View file

@ -51,6 +51,7 @@ Turning the feature off removes shared files and folders on the server for all s
<command>OCA\Files_Sharing\Command\CleanupRemoteStorages</command>
<command>OCA\Files_Sharing\Command\ExiprationNotification</command>
<command>OCA\Files_Sharing\Command\DeleteOrphanShares</command>
<command>OCA\Files_Sharing\Command\FixOwncloudGroupSubshareStatus</command>
<command>OCA\Files_Sharing\Command\FixShareOwners</command>
<command>OCA\Files_Sharing\Command\ListShares</command>
</commands>

View file

@ -27,6 +27,7 @@ return array(
'OCA\\Files_Sharing\\Command\\CleanupRemoteStorages' => $baseDir . '/../lib/Command/CleanupRemoteStorages.php',
'OCA\\Files_Sharing\\Command\\DeleteOrphanShares' => $baseDir . '/../lib/Command/DeleteOrphanShares.php',
'OCA\\Files_Sharing\\Command\\ExiprationNotification' => $baseDir . '/../lib/Command/ExiprationNotification.php',
'OCA\\Files_Sharing\\Command\\FixOwncloudGroupSubshareStatus' => $baseDir . '/../lib/Command/FixOwncloudGroupSubshareStatus.php',
'OCA\\Files_Sharing\\Command\\FixShareOwners' => $baseDir . '/../lib/Command/FixShareOwners.php',
'OCA\\Files_Sharing\\Command\\ListShares' => $baseDir . '/../lib/Command/ListShares.php',
'OCA\\Files_Sharing\\Config\\ConfigLexicon' => $baseDir . '/../lib/Config/ConfigLexicon.php',

View file

@ -42,6 +42,7 @@ class ComposerStaticInitFiles_Sharing
'OCA\\Files_Sharing\\Command\\CleanupRemoteStorages' => __DIR__ . '/..' . '/../lib/Command/CleanupRemoteStorages.php',
'OCA\\Files_Sharing\\Command\\DeleteOrphanShares' => __DIR__ . '/..' . '/../lib/Command/DeleteOrphanShares.php',
'OCA\\Files_Sharing\\Command\\ExiprationNotification' => __DIR__ . '/..' . '/../lib/Command/ExiprationNotification.php',
'OCA\\Files_Sharing\\Command\\FixOwncloudGroupSubshareStatus' => __DIR__ . '/..' . '/../lib/Command/FixOwncloudGroupSubshareStatus.php',
'OCA\\Files_Sharing\\Command\\FixShareOwners' => __DIR__ . '/..' . '/../lib/Command/FixShareOwners.php',
'OCA\\Files_Sharing\\Command\\ListShares' => __DIR__ . '/..' . '/../lib/Command/ListShares.php',
'OCA\\Files_Sharing\\Config\\ConfigLexicon' => __DIR__ . '/..' . '/../lib/Config/ConfigLexicon.php',

View file

@ -0,0 +1,88 @@
<?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\Command;
use OC\Core\Command\Base;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IDBConnection;
use OCP\Share\IShare;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Output\OutputInterface;
/**
* Fixes USERGROUP subshares that were created without `accepted = STATUS_ACCEPTED`
* by a rename on an ownCloud-migrated instance.
*
* When an OC-migrated group share (which has no per-user USERGROUP subshare) is
* renamed for the first time, DefaultShareProvider::move() inserted a new USERGROUP
* row without setting `accepted`. The column defaulted to 0 (STATUS_PENDING), causing
* MountProvider to skip the share on the next login the file disappeared for the
* recipient.
*
* USERGROUP subshares with permissions = 0 were explicitly declined by the user
* and are left untouched.
*/
class FixOwncloudGroupSubshareStatus extends Base {
public function __construct(
private IDBConnection $connection,
) {
parent::__construct();
}
#[\Override]
protected function configure(): void {
$this
->setName('sharing:fix-owncloud-group-shares')
->setDescription('Fix group share subshares left pending after renaming on an ownCloud-migrated instance')
->addOption(
'dry-run',
null,
InputOption::VALUE_NONE,
'Show how many shares would be fixed without making any changes',
);
}
#[\Override]
public function execute(InputInterface $input, OutputInterface $output): int {
$dryRun = $input->getOption('dry-run');
$qb = $this->connection->getQueryBuilder();
$count = (int)$qb->select($qb->func()->count('id'))
->from('share')
->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_USERGROUP, IQueryBuilder::PARAM_INT)))
->andWhere($qb->expr()->eq('accepted', $qb->createNamedParameter(IShare::STATUS_PENDING, IQueryBuilder::PARAM_INT)))
->andWhere($qb->expr()->neq('permissions', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)))
->executeQuery()
->fetchOne();
if ($count === 0) {
$output->writeln('No affected group share subshares found.');
return self::SUCCESS;
}
if ($dryRun) {
$output->writeln("Would fix <info>$count</info> group share subshare(s) (dry-run, no changes made).");
return self::SUCCESS;
}
$qb = $this->connection->getQueryBuilder();
$qb->update('share')
->set('accepted', $qb->createNamedParameter(IShare::STATUS_ACCEPTED, IQueryBuilder::PARAM_INT))
->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_USERGROUP, IQueryBuilder::PARAM_INT)))
->andWhere($qb->expr()->eq('accepted', $qb->createNamedParameter(IShare::STATUS_PENDING, IQueryBuilder::PARAM_INT)))
->andWhere($qb->expr()->neq('permissions', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)))
->executeStatement();
$output->writeln("Fixed <info>$count</info> group share subshare(s).");
return self::SUCCESS;
}
}

View file

@ -59,7 +59,7 @@ class SharedMount extends MountPoint implements IMovableMount, ISharedMountPoint
* @param IShare $share
* @return bool
*/
private function updateFileTarget($newPath, &$share) {
protected function updateFileTarget($newPath, &$share) {
$share->setTarget($newPath);
foreach ($this->groupedShares as $tmpShare) {
@ -113,6 +113,7 @@ class SharedMount extends MountPoint implements IMovableMount, ISharedMountPoint
'exception' => $e,
]
);
$result = false;
}
return $result;

View file

@ -0,0 +1,131 @@
<?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\Command;
use OCA\Files_Sharing\Command\FixOwncloudGroupSubshareStatus;
use OCA\Files_Sharing\Tests\TestCase;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IDBConnection;
use OCP\Server;
use OCP\Share\IShare;
use Symfony\Component\Console\Tester\CommandTester;
#[\PHPUnit\Framework\Attributes\Group(name: 'DB')]
class FixOwncloudGroupSubshareStatusTest extends TestCase {
private IDBConnection $connection;
private CommandTester $commandTester;
protected function setUp(): void {
parent::setUp();
$this->connection = Server::get(IDBConnection::class);
$this->commandTester = new CommandTester(new FixOwncloudGroupSubshareStatus($this->connection));
$this->cleanDB();
}
protected function tearDown(): void {
$this->cleanDB();
parent::tearDown();
}
private function cleanDB(): void {
$this->connection->getQueryBuilder()->delete('share')->executeStatement();
}
private function insertShare(int $shareType, int $accepted, int $permissions): int {
$qb = $this->connection->getQueryBuilder();
$qb->insert('share')
->values([
'share_type' => $qb->createNamedParameter($shareType, IQueryBuilder::PARAM_INT),
'share_with' => $qb->createNamedParameter('user1'),
'uid_owner' => $qb->createNamedParameter('owner'),
'uid_initiator' => $qb->createNamedParameter('owner'),
'parent' => $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT),
'item_type' => $qb->createNamedParameter('file'),
'item_source' => $qb->createNamedParameter('42'),
'item_target' => $qb->createNamedParameter('/file'),
'file_source' => $qb->createNamedParameter(42, IQueryBuilder::PARAM_INT),
'file_target' => $qb->createNamedParameter('/file'),
'permissions' => $qb->createNamedParameter($permissions, IQueryBuilder::PARAM_INT),
'stime' => $qb->createNamedParameter(time(), IQueryBuilder::PARAM_INT),
'accepted' => $qb->createNamedParameter($accepted, IQueryBuilder::PARAM_INT),
])
->executeStatement();
return (int)$this->connection->lastInsertId('*PREFIX*share');
}
private function getAccepted(int $id): int {
$qb = $this->connection->getQueryBuilder();
return (int)$qb->select('accepted')
->from('share')
->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)))
->executeQuery()
->fetchOne();
}
public function testFixesPendingSubshareWithPermissions(): void {
$id = $this->insertShare(IShare::TYPE_USERGROUP, IShare::STATUS_PENDING, 31);
$this->commandTester->execute([]);
$this->assertSame(IShare::STATUS_ACCEPTED, $this->getAccepted($id));
$this->assertStringContainsString('Fixed', $this->commandTester->getDisplay());
}
public function testDryRunShowsCountWithoutChanging(): void {
$id = $this->insertShare(IShare::TYPE_USERGROUP, IShare::STATUS_PENDING, 31);
$this->commandTester->execute(['--dry-run' => true]);
$this->assertSame(IShare::STATUS_PENDING, $this->getAccepted($id));
$this->assertStringContainsString('dry-run', $this->commandTester->getDisplay());
}
public function testDoesNotTouchDeclinedSubshare(): void {
// permissions = 0 means the user explicitly declined the share
$id = $this->insertShare(IShare::TYPE_USERGROUP, IShare::STATUS_PENDING, 0);
$this->commandTester->execute([]);
$this->assertSame(IShare::STATUS_PENDING, $this->getAccepted($id));
$this->assertStringContainsString('No affected', $this->commandTester->getDisplay());
}
public function testDoesNotTouchAlreadyAcceptedSubshare(): void {
$id = $this->insertShare(IShare::TYPE_USERGROUP, IShare::STATUS_ACCEPTED, 31);
$this->commandTester->execute([]);
$this->assertSame(IShare::STATUS_ACCEPTED, $this->getAccepted($id));
$this->assertStringContainsString('No affected', $this->commandTester->getDisplay());
}
public function testDoesNotTouchNonUsergroupShares(): void {
$id = $this->insertShare(IShare::TYPE_GROUP, IShare::STATUS_PENDING, 31);
$this->commandTester->execute([]);
$this->assertSame(IShare::STATUS_PENDING, $this->getAccepted($id));
$this->assertStringContainsString('No affected', $this->commandTester->getDisplay());
}
public function testFixesMultipleAffectedRows(): void {
$id1 = $this->insertShare(IShare::TYPE_USERGROUP, IShare::STATUS_PENDING, 31);
$id2 = $this->insertShare(IShare::TYPE_USERGROUP, IShare::STATUS_PENDING, 17);
$idDeclined = $this->insertShare(IShare::TYPE_USERGROUP, IShare::STATUS_PENDING, 0);
$this->commandTester->execute([]);
$this->assertSame(IShare::STATUS_ACCEPTED, $this->getAccepted($id1));
$this->assertSame(IShare::STATUS_ACCEPTED, $this->getAccepted($id2));
$this->assertSame(IShare::STATUS_PENDING, $this->getAccepted($idDeclined));
$this->assertStringContainsString('2', $this->commandTester->getDisplay());
}
}

View file

@ -9,7 +9,9 @@ namespace OCA\Files_Sharing\Tests;
use OC\Files\Filesystem;
use OCA\Files_Sharing\SharedMount;
use OCA\Files_Sharing\SharedStorage;
use OCP\Constants;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IDBConnection;
use OCP\IGroupManager;
use OCP\IUserManager;
@ -171,6 +173,93 @@ class SharedMountTest extends TestCase {
];
}
/**
* Simulate the ownCloud migration scenario: a TYPE_GROUP share whose `accepted`
* column was set to STATUS_ACCEPTED by the SetAcceptedStatus repair step, but
* which has no per-user USERGROUP subshare (OC never created them).
*
* On first rename, DefaultShareProvider::move() must create the USERGROUP subshare
* with accepted = STATUS_ACCEPTED so MountProvider does not skip it on the next
* login, causing the file to appear to vanish.
*/
public function testMoveOwncloudMigratedGroupShareRemainsVisibleAfterRename(): void {
$testGroup = $this->groupManager->createGroup('testGroupOC');
$user1 = $this->userManager->get(self::TEST_FILES_SHARING_API_USER1);
$user2 = $this->userManager->get(self::TEST_FILES_SHARING_API_USER2);
$testGroup->addUser($user1);
$testGroup->addUser($user2);
// Create a group share without going through the NC acceptance flow,
// then directly set accepted = STATUS_ACCEPTED on the GROUP row to mimic
// the state left by SetAcceptedStatus after an OC→NC migration.
$share = $this->share(
IShare::TYPE_GROUP,
$this->filename,
self::TEST_FILES_SHARING_API_USER1,
'testGroupOC',
Constants::PERMISSION_READ | Constants::PERMISSION_UPDATE | Constants::PERMISSION_SHARE
);
$db = Server::get(IDBConnection::class);
// Remove any USERGROUP subshares the acceptance flow may have created,
// then stamp the GROUP row as STATUS_ACCEPTED — this is the OC migration state.
$qb = $db->getQueryBuilder();
$qb->delete('share')
->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_USERGROUP, IQueryBuilder::PARAM_INT)))
->andWhere($qb->expr()->eq('parent', $qb->createNamedParameter((int)$share->getId(), IQueryBuilder::PARAM_INT)))
->executeStatement();
$qb = $db->getQueryBuilder();
$qb->update('share')
->set('accepted', $qb->createNamedParameter(IShare::STATUS_ACCEPTED, IQueryBuilder::PARAM_INT))
->where($qb->expr()->eq('id', $qb->createNamedParameter((int)$share->getId(), IQueryBuilder::PARAM_INT)))
->executeStatement();
// Log in as the recipient; the share should be visible via the GROUP row.
self::loginHelper(self::TEST_FILES_SHARING_API_USER2);
$this->assertTrue(Filesystem::file_exists($this->filename), 'Share should be visible before rename');
// Rename — this triggers move() which must create the USERGROUP subshare
// with accepted = STATUS_ACCEPTED (not the default STATUS_PENDING).
$renamed = Filesystem::rename($this->filename, $this->filename . '_oc_renamed');
$this->assertTrue($renamed, 'Rename should succeed');
$this->assertTrue(Filesystem::file_exists($this->filename . '_oc_renamed'));
// Re-login to flush the mount cache and force MountProvider to rebuild from DB.
// If the USERGROUP subshare was created with accepted = STATUS_PENDING, the
// share would be skipped here and the file would appear to vanish.
self::loginHelper(self::TEST_FILES_SHARING_API_USER2);
$this->assertTrue(
Filesystem::file_exists($this->filename . '_oc_renamed'),
'Share must still be visible after re-login — USERGROUP subshare must have accepted = STATUS_ACCEPTED'
);
// Cleanup
self::loginHelper(self::TEST_FILES_SHARING_API_USER1);
$this->shareManager->deleteShare($share);
$testGroup->removeUser($user1);
$testGroup->removeUser($user2);
$this->groupManager->get('testGroupOC')?->delete();
}
/**
* When updateFileTarget() throws e.g. Manager::moveShare() rejects an
* OC-migrated group share because the original group no longer exists
* moveMount() must return false so View::rename() propagates the failure
* cleanly rather than silently corrupting the virtual filesystem state.
*/
public function testMoveMountReturnsFalseWhenUpdateFileTargetThrows(): void {
$storage = $this->createMock(SharedStorage::class);
$storage->method('getShare')->willReturn($this->createMock(IShare::class));
$mount = new SharedMountWithFailingUpdate($storage);
$result = $mount->moveMount('/' . self::TEST_FILES_SHARING_API_USER2 . '/files/newname');
$this->assertFalse($result);
}
/**
* If the permissions on a group share are upgraded be sure to still respect
* removed shares by a member of that group
@ -244,3 +333,18 @@ class DummyTestClassSharedMount extends SharedMount {
return $this->stripUserFilesPath($path);
}
}
/**
* SharedMount subclass whose updateFileTarget() always throws, used to verify
* that moveMount() returns false and does not silently corrupt VFS state.
*/
class SharedMountWithFailingUpdate extends SharedMount {
public function __construct(SharedStorage $storage) {
$this->storage = $storage;
$this->mountPoint = '/testuser/files/testfile';
}
protected function updateFileTarget($newPath, &$share): void {
throw new \Exception('Simulated failure: group no longer exists');
}
}

View file

@ -609,6 +609,7 @@ class DefaultShareProvider implements
'permissions' => $qb->createNamedParameter($share->getPermissions()),
'attributes' => $qb->createNamedParameter($shareAttributes),
'stime' => $qb->createNamedParameter($share->getShareTime()->getTimestamp()),
'accepted' => $qb->createNamedParameter(IShare::STATUS_ACCEPTED),
])->executeStatement();
} else {
// Already a usergroup share. Update it.

View file

@ -2272,12 +2272,18 @@ class DefaultShareProviderTest extends \Test\TestCase {
$share = $this->provider->getShareById($id, 'user0');
$this->assertSame('/newTarget', $share->getTarget());
// The USERGROUP subshare created on first move must be STATUS_ACCEPTED so
// MountProvider does not skip it (default DB value is STATUS_PENDING=0).
$this->assertSame(IShare::STATUS_ACCEPTED, $share->getStatus());
$share->setTarget('/ultraNewTarget');
$this->provider->move($share, 'user0');
$share = $this->provider->getShareById($id, 'user0');
$this->assertSame('/ultraNewTarget', $share->getTarget());
// Second move hits the UPDATE branch (USERGROUP subshare already exists).
// STATUS_ACCEPTED must be preserved — the UPDATE only touches file_target.
$this->assertSame(IShare::STATUS_ACCEPTED, $share->getStatus());
}
public static function dataDeleteUser(): array {