From d78c74d57c6d5177ca0853f495f50de5863d3179 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 14 Aug 2023 16:11:34 +0200 Subject: [PATCH] reuse l10n and request in dav folder listing instead of having to query those once for every node Signed-off-by: Robin Appelman --- apps/dav/lib/Connector/Sabre/Directory.php | 13 ++- apps/dav/lib/Connector/Sabre/File.php | 72 ++++++++++------- .../tests/unit/Connector/Sabre/FileTest.php | 80 ++++++++++++------- .../Sabre/RequestTest/RequestTestCase.php | 17 ++-- 4 files changed, 115 insertions(+), 67 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/Directory.php b/apps/dav/lib/Connector/Sabre/Directory.php index c29070fe921fb..7786ffe3f4eef 100644 --- a/apps/dav/lib/Connector/Sabre/Directory.php +++ b/apps/dav/lib/Connector/Sabre/Directory.php @@ -35,16 +35,19 @@ use OC\Files\Mount\MoveableMount; use OC\Files\View; use OC\Metadata\FileMetadata; +use OCA\DAV\AppInfo\Application; use OCA\DAV\Connector\Sabre\Exception\FileLocked; use OCA\DAV\Connector\Sabre\Exception\Forbidden; use OCA\DAV\Connector\Sabre\Exception\InvalidPath; -use OCA\DAV\Upload\FutureFile; use OCP\Files\FileInfo; use OCP\Files\Folder; use OCP\Files\ForbiddenException; use OCP\Files\InvalidPathException; use OCP\Files\NotPermittedException; use OCP\Files\StorageNotAvailableException; +use OCP\IL10N; +use OCP\IRequest; +use OCP\L10N\IFactory; use OCP\Lock\ILockingProvider; use OCP\Lock\LockedException; use Psr\Log\LoggerInterface; @@ -203,7 +206,7 @@ public function createDirectory($name) { * @throws \Sabre\DAV\Exception\NotFound * @throws \Sabre\DAV\Exception\ServiceUnavailable */ - public function getChild($name, $info = null) { + public function getChild($name, $info = null, IRequest $request = null, IL10N $l10n = null) { if (!$this->info->isReadable()) { // avoid detecting files through this way throw new NotFound(); @@ -230,7 +233,7 @@ public function getChild($name, $info = null) { if ($info->getMimeType() === FileInfo::MIMETYPE_FOLDER) { $node = new \OCA\DAV\Connector\Sabre\Directory($this->fileView, $info, $this->tree, $this->shareManager); } else { - $node = new \OCA\DAV\Connector\Sabre\File($this->fileView, $info, $this->shareManager); + $node = new \OCA\DAV\Connector\Sabre\File($this->fileView, $info, $this->shareManager, $request, $l10n); } if ($this->tree) { $this->tree->cacheNode($node); @@ -265,8 +268,10 @@ public function getChildren() { } $nodes = []; + $request = \OC::$server->get(IRequest::class); + $l10nFactory = \OC::$server->get(IFactory::class); foreach ($folderContent as $info) { - $node = $this->getChild($info->getName(), $info); + $node = $this->getChild($info->getName(), $info, $request, $l10nFactory->get(Application::APP_ID)); $nodes[] = $node; } $this->dirContent = $nodes; diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index a7cafeb4a5e1a..8ce8c843a6628 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -63,6 +63,7 @@ use OCP\Files\Storage; use OCP\Files\StorageNotAvailableException; use OCP\IL10N; +use OCP\IRequest; use OCP\L10N\IFactory as IL10NFactory; use OCP\Lock\ILockingProvider; use OCP\Lock\LockedException; @@ -77,8 +78,7 @@ use Sabre\DAV\IFile; class File extends Node implements IFile { - protected $request; - + protected IRequest $request; protected IL10N $l10n; /** @var array */ @@ -89,21 +89,26 @@ class File extends Node implements IFile { * * @param \OC\Files\View $view * @param \OCP\Files\FileInfo $info - * @param \OCP\Share\IManager $shareManager - * @param \OC\AppFramework\Http\Request $request + * @param ?\OCP\Share\IManager $shareManager + * @param ?IRequest $request + * @param ?IL10N $l10n */ - public function __construct(View $view, FileInfo $info, IManager $shareManager = null, Request $request = null) { + public function __construct(View $view, FileInfo $info, IManager $shareManager = null, IRequest $request = null, IL10N $l10n = null) { parent::__construct($view, $info, $shareManager); - // Querying IL10N directly results in a dependency loop - /** @var IL10NFactory $l10nFactory */ - $l10nFactory = \OC::$server->get(IL10NFactory::class); - $this->l10n = $l10nFactory->get(Application::APP_ID); + if ($l10n) { + $this->l10n = $l10n; + } else { + // Querying IL10N directly results in a dependency loop + /** @var IL10NFactory $l10nFactory */ + $l10nFactory = \OC::$server->get(IL10NFactory::class); + $this->l10n = $l10nFactory->get(Application::APP_ID); + } if (isset($request)) { $this->request = $request; } else { - $this->request = \OC::$server->getRequest(); + $this->request = \OC::$server->get(IRequest::class); } } @@ -149,7 +154,8 @@ public function put($data) { $this->verifyPath(); // chunked handling - if (isset($_SERVER['HTTP_OC_CHUNKED'])) { + $chunkedHeader = $this->request->getHeader('oc-chunked'); + if ($chunkedHeader) { try { return $this->createFileChunked($data); } catch (\Exception $e) { @@ -272,8 +278,9 @@ public function put($data) { if ($result === false) { $expected = -1; - if (isset($_SERVER['CONTENT_LENGTH'])) { - $expected = (int)$_SERVER['CONTENT_LENGTH']; + $lengthHeader = $this->request->getHeader('content-length'); + if ($lengthHeader) { + $expected = (int)$lengthHeader; } if ($expected !== 0) { throw new Exception( @@ -291,8 +298,9 @@ public function put($data) { // if content length is sent by client: // double check if the file was fully received // compare expected and actual size - if (isset($_SERVER['CONTENT_LENGTH']) && $_SERVER['REQUEST_METHOD'] === 'PUT') { - $expected = (int)$_SERVER['CONTENT_LENGTH']; + $lengthHeader = $this->request->getHeader('content-length'); + if ($lengthHeader && $this->request->getMethod() === 'PUT') { + $expected = (int)$lengthHeader; if ($count !== $expected) { throw new BadRequest( $this->l10n->t( @@ -374,8 +382,9 @@ public function put($data) { } // allow sync clients to send the mtime along in a header - if (isset($this->request->server['HTTP_X_OC_MTIME'])) { - $mtime = $this->sanitizeMtime($this->request->server['HTTP_X_OC_MTIME']); + $mtimeHeader = $this->request->getHeader('x-oc-mtime'); + if ($mtimeHeader !== '') { + $mtime = $this->sanitizeMtime($mtimeHeader); if ($this->fileView->touch($this->path, $mtime)) { $this->header('X-OC-MTime: accepted'); } @@ -386,8 +395,9 @@ public function put($data) { ]; // allow sync clients to send the creation time along in a header - if (isset($this->request->server['HTTP_X_OC_CTIME'])) { - $ctime = $this->sanitizeMtime($this->request->server['HTTP_X_OC_CTIME']); + $ctimeHeader = $this->request->getHeader('x-oc-ctime'); + if ($ctimeHeader) { + $ctime = $this->sanitizeMtime($ctimeHeader); $fileInfoUpdate['creation_time'] = $ctime; $this->header('X-OC-CTime: accepted'); } @@ -400,8 +410,9 @@ public function put($data) { $this->refreshInfo(); - if (isset($this->request->server['HTTP_OC_CHECKSUM'])) { - $checksum = trim($this->request->server['HTTP_OC_CHECKSUM']); + $checksumHeader = $this->request->getHeader('oc-checksum'); + if ($checksumHeader) { + $checksum = trim($checksumHeader); $this->setChecksum($checksum); } elseif ($this->getChecksum() !== null && $this->getChecksum() !== '') { $this->setChecksum(''); @@ -557,7 +568,7 @@ public function getContentType() { $mimeType = $this->info->getMimetype(); // PROPFIND needs to return the correct mime type, for consistency with the web UI - if (isset($_SERVER['REQUEST_METHOD']) && $_SERVER['REQUEST_METHOD'] === 'PROPFIND') { + if ($this->request->getMethod() === 'PROPFIND') { return $mimeType; } return \OC::$server->getMimeTypeDetector()->getSecureMimeType($mimeType); @@ -599,9 +610,10 @@ private function createFileChunked($data) { $bytesWritten = $chunk_handler->store($info['index'], $data); //detect aborted upload - if (isset($_SERVER['REQUEST_METHOD']) && $_SERVER['REQUEST_METHOD'] === 'PUT') { - if (isset($_SERVER['CONTENT_LENGTH'])) { - $expected = (int)$_SERVER['CONTENT_LENGTH']; + if ($this->request->getMethod() === 'PUT') { + $lengthHeader = $this->request->getHeader('content-length'); + if ($lengthHeader) { + $expected = (int)$lengthHeader; if ($bytesWritten !== $expected) { $chunk_handler->remove($info['index']); throw new BadRequest( @@ -667,8 +679,9 @@ private function createFileChunked($data) { } // allow sync clients to send the mtime along in a header - if (isset($this->request->server['HTTP_X_OC_MTIME'])) { - $mtime = $this->sanitizeMtime($this->request->server['HTTP_X_OC_MTIME']); + $mtimeHeader = $this->request->getHeader('x-oc-mtime'); + if ($mtimeHeader !== '') { + $mtime = $this->sanitizeMtime($mtimeHeader); if ($targetStorage->touch($targetInternalPath, $mtime)) { $this->header('X-OC-MTime: accepted'); } @@ -684,8 +697,9 @@ private function createFileChunked($data) { // FIXME: should call refreshInfo but can't because $this->path is not the of the final file $info = $this->fileView->getFileInfo($targetPath); - if (isset($this->request->server['HTTP_OC_CHECKSUM'])) { - $checksum = trim($this->request->server['HTTP_OC_CHECKSUM']); + $checksumHeader = $this->request->getHeader('oc-checksum'); + if ($checksumHeader) { + $checksum = trim($checksumHeader); $this->fileView->putFileInfo($targetPath, ['checksum' => $checksum]); } elseif ($info->getChecksum() !== null && $info->getChecksum() !== '') { $this->fileView->putFileInfo($this->path, ['checksum' => '']); diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 8d72fb13b7875..884245afa3bb2 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -73,9 +73,6 @@ class FileTest extends TestCase { protected function setUp(): void { parent::setUp(); - unset($_SERVER['HTTP_OC_CHUNKED']); - unset($_SERVER['CONTENT_LENGTH']); - unset($_SERVER['REQUEST_METHOD']); \OC_Hook::clear(); @@ -91,7 +88,6 @@ protected function setUp(): void { protected function tearDown(): void { $userManager = \OC::$server->getUserManager(); $userManager->get($this->user)->delete(); - unset($_SERVER['HTTP_OC_CHUNKED']); parent::tearDown(); } @@ -271,13 +267,17 @@ function ($path) use ($storage) { ->method('getRelativePath') ->willReturnArgument(0); - $_SERVER['HTTP_OC_CHUNKED'] = true; + $request = new Request([ + 'server' => [ + 'HTTP_OC_CHUNKED' => 'true' + ] + ], $this->requestId, $this->config, null); $info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-0', $this->getMockStorage(), null, [ 'permissions' => \OCP\Constants::PERMISSION_ALL, 'type' => FileInfo::TYPE_FOLDER, ], null); - $file = new \OCA\DAV\Connector\Sabre\File($view, $info); + $file = new \OCA\DAV\Connector\Sabre\File($view, $info, null, $request); // put first chunk $file->acquireLock(ILockingProvider::LOCK_SHARED); @@ -288,7 +288,7 @@ function ($path) use ($storage) { 'permissions' => \OCP\Constants::PERMISSION_ALL, 'type' => FileInfo::TYPE_FOLDER, ], null); - $file = new \OCA\DAV\Connector\Sabre\File($view, $info); + $file = new \OCA\DAV\Connector\Sabre\File($view, $info, null, $request); // action $caughtException = null; @@ -421,7 +421,7 @@ public function legalMtimeProvider() { public function testPutSingleFileLegalMtime($requestMtime, $resultMtime): void { $request = new Request([ 'server' => [ - 'HTTP_X_OC_MTIME' => $requestMtime, + 'HTTP_X_OC_MTIME' => (string)$requestMtime, ] ], $this->requestId, $this->config, null); $file = 'foo.txt'; @@ -444,11 +444,11 @@ public function testPutSingleFileLegalMtime($requestMtime, $resultMtime): void { public function testChunkedPutLegalMtime($requestMtime, $resultMtime): void { $request = new Request([ 'server' => [ - 'HTTP_X_OC_MTIME' => $requestMtime, + 'HTTP_X_OC_MTIME' => (string)$requestMtime, + 'HTTP_OC_CHUNKED' => 'true' ] ], $this->requestId, $this->config, null); - $_SERVER['HTTP_OC_CHUNKED'] = true; $file = 'foo.txt'; if ($resultMtime === null) { @@ -467,9 +467,13 @@ public function testChunkedPutLegalMtime($requestMtime, $resultMtime): void { * Test putting a file using chunking */ public function testChunkedPut(): void { - $_SERVER['HTTP_OC_CHUNKED'] = true; - $this->assertNull($this->doPut('/test.txt-chunking-12345-2-0')); - $this->assertNotEmpty($this->doPut('/test.txt-chunking-12345-2-1')); + $request = new Request([ + 'server' => [ + 'HTTP_OC_CHUNKED' => 'true' + ] + ], $this->requestId, $this->config, null); + $this->assertNull($this->doPut('/test.txt-chunking-12345-2-0', null, $request)); + $this->assertNotEmpty($this->doPut('/test.txt-chunking-12345-2-1', null, $request)); } /** @@ -580,9 +584,13 @@ public function testPutSingleFileTriggersHooksDifferentRoot(): void { public function testPutChunkedFileTriggersHooks(): void { HookHelper::setUpHooks(); - $_SERVER['HTTP_OC_CHUNKED'] = true; - $this->assertNull($this->doPut('/foo.txt-chunking-12345-2-0')); - $this->assertNotEmpty($this->doPut('/foo.txt-chunking-12345-2-1')); + $request = new Request([ + 'server' => [ + 'HTTP_OC_CHUNKED' => 'true' + ] + ], $this->requestId, $this->config, null); + $this->assertNull($this->doPut('/foo.txt-chunking-12345-2-0', null, $request)); + $this->assertNotEmpty($this->doPut('/foo.txt-chunking-12345-2-1', null, $request)); $this->assertCount(4, HookHelper::$hookCalls); $this->assertHookCall( @@ -616,9 +624,13 @@ public function testPutOverwriteChunkedFileTriggersHooks(): void { HookHelper::setUpHooks(); - $_SERVER['HTTP_OC_CHUNKED'] = true; - $this->assertNull($this->doPut('/foo.txt-chunking-12345-2-0')); - $this->assertNotEmpty($this->doPut('/foo.txt-chunking-12345-2-1')); + $request = new Request([ + 'server' => [ + 'HTTP_OC_CHUNKED' => 'true' + ] + ], $this->requestId, $this->config, null); + $this->assertNull($this->doPut('/foo.txt-chunking-12345-2-0', null, $request)); + $this->assertNotEmpty($this->doPut('/foo.txt-chunking-12345-2-1', null, $request)); $this->assertCount(4, HookHelper::$hookCalls); $this->assertHookCall( @@ -693,15 +705,19 @@ public function testSimplePutFailsSizeCheck(): void { ->method('filesize') ->willReturn(123456); - $_SERVER['CONTENT_LENGTH'] = 123456; - $_SERVER['REQUEST_METHOD'] = 'PUT'; + $request = new Request([ + 'server' => [ + 'CONTENT_LENGTH' => '123456', + ], + 'method' => 'PUT', + ], $this->requestId, $this->config, null); $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [ 'permissions' => \OCP\Constants::PERMISSION_ALL, 'type' => FileInfo::TYPE_FOLDER, ], null); - $file = new \OCA\DAV\Connector\Sabre\File($view, $info); + $file = new \OCA\DAV\Connector\Sabre\File($view, $info, null, $request); // action $thrown = false; @@ -764,13 +780,17 @@ public function testChunkedPutFailsFinalRename(): void { // simulate situation where the target file is locked $view->lockFile('/test.txt', ILockingProvider::LOCK_EXCLUSIVE); - $_SERVER['HTTP_OC_CHUNKED'] = true; + $request = new Request([ + 'server' => [ + 'HTTP_OC_CHUNKED' => 'true' + ] + ], $this->requestId, $this->config, null); $info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-0', $this->getMockStorage(), null, [ 'permissions' => \OCP\Constants::PERMISSION_ALL, 'type' => FileInfo::TYPE_FOLDER, ], null); - $file = new \OCA\DAV\Connector\Sabre\File($view, $info); + $file = new \OCA\DAV\Connector\Sabre\File($view, $info, null, $request); $file->acquireLock(ILockingProvider::LOCK_SHARED); $this->assertNull($file->put('test data one')); $file->releaseLock(ILockingProvider::LOCK_SHARED); @@ -779,7 +799,7 @@ public function testChunkedPutFailsFinalRename(): void { 'permissions' => \OCP\Constants::PERMISSION_ALL, 'type' => FileInfo::TYPE_FOLDER, ], null); - $file = new \OCA\DAV\Connector\Sabre\File($view, $info); + $file = new \OCA\DAV\Connector\Sabre\File($view, $info, null, $request); // action $thrown = false; @@ -872,15 +892,19 @@ public function testUploadAbort(): void { ->method('filesize') ->willReturn(123456); - $_SERVER['CONTENT_LENGTH'] = 12345; - $_SERVER['REQUEST_METHOD'] = 'PUT'; + $request = new Request([ + 'server' => [ + 'CONTENT_LENGTH' => '123456', + ], + 'method' => 'PUT', + ], $this->requestId, $this->config, null); $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [ 'permissions' => \OCP\Constants::PERMISSION_ALL, 'type' => FileInfo::TYPE_FOLDER, ], null); - $file = new \OCA\DAV\Connector\Sabre\File($view, $info); + $file = new \OCA\DAV\Connector\Sabre\File($view, $info, null, $request); // action $thrown = false; diff --git a/apps/dav/tests/unit/Connector/Sabre/RequestTest/RequestTestCase.php b/apps/dav/tests/unit/Connector/Sabre/RequestTest/RequestTestCase.php index c4f8c21eae186..a8b4049240802 100644 --- a/apps/dav/tests/unit/Connector/Sabre/RequestTest/RequestTestCase.php +++ b/apps/dav/tests/unit/Connector/Sabre/RequestTest/RequestTestCase.php @@ -32,7 +32,9 @@ use OCA\DAV\Connector\Sabre\Server; use OCA\DAV\Connector\Sabre\ServerFactory; use OCP\EventDispatcher\IEventDispatcher; +use OCP\IConfig; use OCP\IRequest; +use OCP\IRequestId; use Psr\Log\LoggerInterface; use Sabre\HTTP\Request; use Test\TestCase; @@ -58,8 +60,6 @@ protected function getStream($string) { protected function setUp(): void { parent::setUp(); - unset($_SERVER['HTTP_OC_CHUNKED']); - $this->serverFactory = new ServerFactory( \OC::$server->getConfig(), \OC::$server->get(LoggerInterface::class), @@ -106,20 +106,25 @@ protected function request($view, $user, $password, $method, $url, $body = null, // since sabre catches all exceptions we need to save them and throw them from outside the sabre server - $originalServer = $_SERVER; - + $serverParams = []; if (is_array($headers)) { foreach ($headers as $header => $value) { - $_SERVER['HTTP_' . strtoupper(str_replace('-', '_', $header))] = $value; + $serverParams['HTTP_' . strtoupper(str_replace('-', '_', $header))] = $value; } } + $ncRequest = new \OC\AppFramework\Http\Request([ + 'server' => $serverParams + ], $this->createMock(IRequestId::class), $this->createMock(IConfig::class), null); + + $this->overwriteService(IRequest::class, $ncRequest); $result = $this->makeRequest($server, $request); + $this->restoreService(IRequest::class); + foreach ($exceptionPlugin->getExceptions() as $exception) { throw $exception; } - $_SERVER = $originalServer; return $result; }