diff --git a/apps/files_sharing/appinfo/info.xml b/apps/files_sharing/appinfo/info.xml index ea77041b498ed..36d854766f3c0 100644 --- a/apps/files_sharing/appinfo/info.xml +++ b/apps/files_sharing/appinfo/info.xml @@ -51,6 +51,7 @@ Turning the feature off removes shared files and folders on the server for all s OCA\Files_Sharing\Command\CleanupRemoteStorages OCA\Files_Sharing\Command\ExiprationNotification OCA\Files_Sharing\Command\DeleteOrphanShares + OCA\Files_Sharing\Command\FixOwncloudGroupSubshareStatus OCA\Files_Sharing\Command\FixShareOwners OCA\Files_Sharing\Command\ListShares diff --git a/apps/files_sharing/composer/composer/autoload_classmap.php b/apps/files_sharing/composer/composer/autoload_classmap.php index 2d6b545a17a63..8140fc52f1628 100644 --- a/apps/files_sharing/composer/composer/autoload_classmap.php +++ b/apps/files_sharing/composer/composer/autoload_classmap.php @@ -27,6 +27,7 @@ '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', diff --git a/apps/files_sharing/composer/composer/autoload_static.php b/apps/files_sharing/composer/composer/autoload_static.php index 1fc2ddd2dfd8a..fd2189f481888 100644 --- a/apps/files_sharing/composer/composer/autoload_static.php +++ b/apps/files_sharing/composer/composer/autoload_static.php @@ -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', diff --git a/apps/files_sharing/lib/Command/FixOwncloudGroupSubshareStatus.php b/apps/files_sharing/lib/Command/FixOwncloudGroupSubshareStatus.php new file mode 100644 index 0000000000000..3140fa55a1900 --- /dev/null +++ b/apps/files_sharing/lib/Command/FixOwncloudGroupSubshareStatus.php @@ -0,0 +1,88 @@ +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 $count 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 $count group share subshare(s)."); + return self::SUCCESS; + } +} diff --git a/apps/files_sharing/lib/SharedMount.php b/apps/files_sharing/lib/SharedMount.php index 915a92e81f97a..fddd81d5a7001 100644 --- a/apps/files_sharing/lib/SharedMount.php +++ b/apps/files_sharing/lib/SharedMount.php @@ -59,7 +59,7 @@ public function __construct( * @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 @@ public function moveMount(string $target): bool { 'exception' => $e, ] ); + $result = false; } return $result; diff --git a/apps/files_sharing/tests/Command/FixOwncloudGroupSubshareStatusTest.php b/apps/files_sharing/tests/Command/FixOwncloudGroupSubshareStatusTest.php new file mode 100644 index 0000000000000..c58454ab9d042 --- /dev/null +++ b/apps/files_sharing/tests/Command/FixOwncloudGroupSubshareStatusTest.php @@ -0,0 +1,131 @@ +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()); + } +} diff --git a/apps/files_sharing/tests/SharedMountTest.php b/apps/files_sharing/tests/SharedMountTest.php index 020b6ec41c5c8..851dd7168e4a5 100644 --- a/apps/files_sharing/tests/SharedMountTest.php +++ b/apps/files_sharing/tests/SharedMountTest.php @@ -9,7 +9,9 @@ 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 @@ public static function dataProviderTestStripUserFilesPath() { ]; } + /** + * 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 @@ public function stripUserFilesPathDummy($path) { 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'); + } +} diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index 1e064ec821ec8..d349aa83491ea 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -609,6 +609,7 @@ public function move(IShare $share, $recipient) { '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. diff --git a/tests/lib/Share20/DefaultShareProviderTest.php b/tests/lib/Share20/DefaultShareProviderTest.php index 470a48adde2b9..97dfe1a409602 100644 --- a/tests/lib/Share20/DefaultShareProviderTest.php +++ b/tests/lib/Share20/DefaultShareProviderTest.php @@ -2272,12 +2272,18 @@ public function testMoveGroupShare(): void { $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 {