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>pull/60925/head
Anna Larch
2 weeks ago
committed by
backportbot[bot]
parent
3f25ee3b6f
commit
85202d7f4e
@ -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; |
||||
} |
||||
} |
||||
@ -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()); |
||||
} |
||||
} |
||||
Loading…
Reference in new issue