diff --git a/apps/files_sharing/lib/SharedMount.php b/apps/files_sharing/lib/SharedMount.php index fddd81d5a7001..e72e09711fca8 100644 --- a/apps/files_sharing/lib/SharedMount.php +++ b/apps/files_sharing/lib/SharedMount.php @@ -73,6 +73,9 @@ protected function updateFileTarget($newPath, &$share) { /** * Format a path to be relative to the /user/files/ directory * + * The result is normalized, so it never carries a trailing slash. Callers may + * pass mount points, which always end in a slash. + * * @param string $path the absolute path * @return string e.g. turns '/admin/files/test.txt' into '/test.txt' * @throws BrokenPath @@ -91,7 +94,7 @@ protected function stripUserFilesPath(string $path): string { $sliced = array_slice($split, 2); $relPath = implode('/', $sliced); - return '/' . $relPath; + return Filesystem::normalizePath('/' . $relPath); } #[Override] diff --git a/apps/files_sharing/tests/SharedMountTest.php b/apps/files_sharing/tests/SharedMountTest.php index 22f42b050fa93..1e86969fe084c 100644 --- a/apps/files_sharing/tests/SharedMountTest.php +++ b/apps/files_sharing/tests/SharedMountTest.php @@ -166,6 +166,9 @@ public static function dataProviderTestStripUserFilesPath() { return [ ['/user/files/foo.txt', '/foo.txt', false], ['/user/files/folder/foo.txt', '/folder/foo.txt', false], + ['/user/files/foo.txt/', '/foo.txt', false], + ['/user/files/folder/foo.txt/', '/folder/foo.txt', false], + ['/user/files/folder//foo.txt', '/folder/foo.txt', false], ['/data/user/files/foo.txt', null, true], ['/data/user/files/', null, true], ['/files/foo.txt', null, true], diff --git a/build/integration/sharing_features/sharing-v1-part4.feature b/build/integration/sharing_features/sharing-v1-part4.feature index 08ab1a1daed3c..7ba1234a6500e 100644 --- a/build/integration/sharing_features/sharing-v1-part4.feature +++ b/build/integration/sharing_features/sharing-v1-part4.feature @@ -555,3 +555,25 @@ Scenario: User added/removed to group share with marking | permissions | 1 | Then the OCS status code should be "100" And the HTTP status code should be "200" + + # Renaming the parent folder moves the share mount through Updater::renameChildren, + # which passes a mount point instead of a path. Mount points carry a trailing slash, + # which used to end up in the share target. + Scenario: Renaming a folder that contains a received share keeps the share target normalized + Given user "user0" exists + And user "user1" exists + And User "user0" uploads file with content "content" to "/moved-share.txt" + And file "/moved-share.txt" of user "user0" is shared with user "user1" with permissions 19 + And user "user1" accepts last share + And user "user1" created a folder "/received" + And User "user1" moves file "/moved-share.txt" to "/received/moved-share.txt" + Then the HTTP status code should be "201" + When User "user1" moves file "/received" to "/archive" + Then the HTTP status code should be "201" + And As an "user1" + And Getting info of last share + And the OCS status code should be "100" + And Share fields of last share match with + | file_target | /archive/moved-share.txt | + And user "user1" should see following elements + | /archive/moved-share.txt | diff --git a/core/AppInfo/ConfigLexicon.php b/core/AppInfo/ConfigLexicon.php index f99c44e43a61f..cb49a42893ede 100644 --- a/core/AppInfo/ConfigLexicon.php +++ b/core/AppInfo/ConfigLexicon.php @@ -28,6 +28,7 @@ class ConfigLexicon implements ILexicon { public const SHARE_LINK_EXPIRE_DATE_ENFORCED = 'shareapi_enforce_expire_date'; public const USER_LANGUAGE = 'lang'; public const OCM_DISCOVERY_ENABLED = 'ocm_discovery_enabled'; + public const SHARE_REPAIR_REMOVED_TRAILING_SLASHES = 'share_repair_removed_trailing_slashes'; public const USER_LOCALE = 'locale'; public const USER_TIMEZONE = 'timezone'; @@ -113,6 +114,13 @@ public function getAppConfigs(): array { definition: 'Show the app store link in the app menu to accounts without admin rights', note: 'When this key is not set, the link is also hidden while a valid subscription is available or while "appstoreenabled" is disabled. Setting this key explicitly takes precedence over both.', ), + new Entry( + key: self::SHARE_REPAIR_REMOVED_TRAILING_SLASHES, + type: ValueType::BOOL, + defaultRaw: false, + definition: 'Whether the repair step stripping trailing slashes from share targets has already been run.', + lazy: true, + ), ]; } diff --git a/lib/private/Repair/RepairInvalidShares.php b/lib/private/Repair/RepairInvalidShares.php index f3a4cbef724e3..d2d1b04fe773d 100644 --- a/lib/private/Repair/RepairInvalidShares.php +++ b/lib/private/Repair/RepairInvalidShares.php @@ -8,7 +8,9 @@ namespace OC\Repair; +use OC\Core\AppInfo\ConfigLexicon; use OCP\Constants; +use OCP\IAppConfig; use OCP\IConfig; use OCP\IDBConnection; use OCP\Migration\IOutput; @@ -23,6 +25,7 @@ class RepairInvalidShares implements IRepairStep { public function __construct( protected IConfig $config, protected IDBConnection $connection, + protected IAppConfig $appConfig, ) { } @@ -87,6 +90,51 @@ private function removeSharesNonExistingParent(IOutput $output): void { } } + /** + * Strip trailing slashes that leaked into the share target when a parent folder + * of a moved incoming share was renamed + */ + private function removeTrailingSlashFromFileTarget(IOutput $output): void { + if ($this->appConfig->getValueBool('core', ConfigLexicon::SHARE_REPAIR_REMOVED_TRAILING_SLASHES, lazy: true)) { + return; + } + + $updatedEntries = 0; + + $query = $this->connection->getQueryBuilder(); + $query->select('id', 'file_target') + ->from('share') + ->where($query->expr()->like('file_target', $query->createNamedParameter('%/'))) + ->andWhere($query->expr()->neq('file_target', $query->createNamedParameter('/'))) + ->setMaxResults(self::CHUNK_SIZE); + + $updateQuery = $this->connection->getQueryBuilder(); + $updateQuery->update('share') + ->set('file_target', $updateQuery->createParameter('file_target')) + ->where($updateQuery->expr()->eq('id', $updateQuery->createParameter('id'))); + + $rowsInLastChunk = self::CHUNK_SIZE; + while ($rowsInLastChunk === self::CHUNK_SIZE) { + $result = $query->executeQuery(); + $rows = $result->fetchAllAssociative(); + $result->closeCursor(); + $rowsInLastChunk = count($rows); + + foreach ($rows as $row) { + $updatedEntries += $updateQuery + ->setParameter('file_target', rtrim($row['file_target'], '/')) + ->setParameter('id', (int)$row['id']) + ->executeStatement(); + } + } + + $this->appConfig->setValueBool('core', ConfigLexicon::SHARE_REPAIR_REMOVED_TRAILING_SLASHES, true, lazy: true); + + if ($updatedEntries > 0) { + $output->info('Removed trailing slashes from the target of ' . $updatedEntries . ' shares'); + } + } + #[\Override] public function run(IOutput $output) { $ocVersionFromBeforeUpdate = $this->config->getSystemValueString('version', '0.0.0'); @@ -95,5 +143,6 @@ public function run(IOutput $output) { } $this->removeSharesNonExistingParent($output); + $this->removeTrailingSlashFromFileTarget($output); } } diff --git a/tests/lib/Repair/RepairInvalidSharesTest.php b/tests/lib/Repair/RepairInvalidSharesTest.php index 615247ab01140..9b665c5f66e55 100644 --- a/tests/lib/Repair/RepairInvalidSharesTest.php +++ b/tests/lib/Repair/RepairInvalidSharesTest.php @@ -8,8 +8,10 @@ namespace Test\Repair; +use OC\Core\AppInfo\ConfigLexicon; use OC\Repair\RepairInvalidShares; use OCP\Constants; +use OCP\IAppConfig; use OCP\IConfig; use OCP\IDBConnection; use OCP\Migration\IOutput; @@ -28,6 +30,7 @@ class RepairInvalidSharesTest extends TestCase { private RepairInvalidShares $repair; private IDBConnection $connection; + private IAppConfig&\PHPUnit\Framework\MockObject\MockObject $appConfig; #[\Override] protected function setUp(): void { @@ -44,7 +47,9 @@ protected function setUp(): void { $this->connection = Server::get(IDBConnection::class); $this->deleteAllShares(); - $this->repair = new RepairInvalidShares($config, $this->connection); + $this->appConfig = $this->createMock(IAppConfig::class); + + $this->repair = new RepairInvalidShares($config, $this->connection, $this->appConfig); } #[\Override] @@ -127,6 +132,80 @@ public function testSharesNonExistingParent(): void { $result->closeCursor(); } + public static function trailingSlashProvider(): array { + return [ + // trailing slash left behind by renaming a parent folder of a moved share + ['/rename_folder/First_share_the_file.odt/', '/rename_folder/First_share_the_file.odt'], + ['/shared_folder/', '/shared_folder'], + // unchanged + ['/rename_folder/First_share_the_file.odt', '/rename_folder/First_share_the_file.odt'], + ['/', '/'], + ]; + } + + /** + * Test stripping trailing slashes from the share target + */ + private function addShareWithTarget(string $fileTarget): void { + $qb = $this->connection->getQueryBuilder(); + $qb->insert('share') + ->values([ + 'share_type' => $qb->expr()->literal(IShare::TYPE_USER), + 'share_with' => $qb->expr()->literal('recipientuser1'), + 'uid_owner' => $qb->expr()->literal('user1'), + 'item_type' => $qb->expr()->literal('folder'), + 'item_source' => $qb->expr()->literal(123), + 'item_target' => $qb->expr()->literal('/123'), + 'file_source' => $qb->expr()->literal(123), + 'file_target' => $qb->expr()->literal($fileTarget), + 'permissions' => $qb->expr()->literal(31), + 'stime' => $qb->expr()->literal(time()), + ]) + ->executeStatement(); + } + + private function getSingleFileTarget(): string { + $results = $this->connection->getQueryBuilder() + ->select('file_target') + ->from('share') + ->executeQuery() + ->fetchAllAssociative(); + + $this->assertCount(1, $results); + + return $results[0]['file_target']; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('trailingSlashProvider')] + public function testRemoveTrailingSlashFromFileTarget(string $fileTarget, string $expectedFileTarget): void { + $this->addShareWithTarget($fileTarget); + + $this->appConfig->method('getValueBool') + ->with('core', ConfigLexicon::SHARE_REPAIR_REMOVED_TRAILING_SLASHES, false, true) + ->willReturn(false); + $this->appConfig->expects($this->once()) + ->method('setValueBool') + ->with('core', ConfigLexicon::SHARE_REPAIR_REMOVED_TRAILING_SLASHES, true, true); + + $this->repair->run($this->createMock(IOutput::class)); + + $this->assertSame($expectedFileTarget, $this->getSingleFileTarget()); + } + + public function testRemoveTrailingSlashFromFileTargetSkippedWhenAlreadyRun(): void { + $this->addShareWithTarget('/rename_folder/First_share_the_file.odt/'); + + $this->appConfig->method('getValueBool') + ->with('core', ConfigLexicon::SHARE_REPAIR_REMOVED_TRAILING_SLASHES, false, true) + ->willReturn(true); + $this->appConfig->expects($this->never()) + ->method('setValueBool'); + + $this->repair->run($this->createMock(IOutput::class)); + + $this->assertSame('/rename_folder/First_share_the_file.odt/', $this->getSingleFileTarget()); + } + public static function fileSharePermissionsProvider(): array { return [ // unchanged for folder