diff --git a/apps/dav/appinfo/v1/webdav.php b/apps/dav/appinfo/v1/webdav.php index 2d89a03c8bd6..f6ea0739ffee 100644 --- a/apps/dav/appinfo/v1/webdav.php +++ b/apps/dav/appinfo/v1/webdav.php @@ -62,5 +62,6 @@ $event = new \OCP\SabrePluginEvent($server); \OC::$server->getEventDispatcher()->dispatch('OCA\DAV\Connector\Sabre::authInit', $event); +\Sabre\DAV\Server::$streamMultiStatus = true; // And off we go! $server->exec(); diff --git a/apps/dav/lib/Capabilities.php b/apps/dav/lib/Capabilities.php index dd0fd6e7943d..08fcae3fd060 100644 --- a/apps/dav/lib/Capabilities.php +++ b/apps/dav/lib/Capabilities.php @@ -38,11 +38,14 @@ public function __construct(IConfig $config) { } public function getCapabilities() { - $cap = [ + $cap = [ 'dav' => [ 'chunking' => '1.0', 'reports' => [ 'search-files', + ], + 'propfind' => [ + 'depth_infinity' => $this->config->getSystemValue('dav.propfind.depth_infinity', true), ] ] ]; diff --git a/apps/dav/lib/Connector/Sabre/FilesPlugin.php b/apps/dav/lib/Connector/Sabre/FilesPlugin.php index 69635f30592f..b34af6b620e2 100644 --- a/apps/dav/lib/Connector/Sabre/FilesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesPlugin.php @@ -31,8 +31,8 @@ namespace OCA\DAV\Connector\Sabre; use OC\AppFramework\Http\Request; +use OC\Files\Filesystem; use OCA\DAV\Files\IProvidesAdditionalHeaders; -use OCA\DAV\Meta\MetaFile; use OCP\Files\ForbiddenException; use OCP\Files\StorageNotAvailableException; use OCP\IConfig; @@ -47,6 +47,7 @@ use Sabre\DAV\Tree; use Sabre\HTTP\RequestInterface; use Sabre\HTTP\ResponseInterface; +use OCA\DAV\Connector\Sabre\Node; class FilesPlugin extends ServerPlugin { @@ -167,6 +168,36 @@ public function initialize(\Sabre\DAV\Server $server) { }); $this->server->on('beforeMove', [$this, 'checkMove']); $this->server->on('validateTokens', [$this, 'validateTokens'], 0); + $this->server->on('method:PROPFIND', [$this, 'checkPropFind']); + } + + /** + * Check if the file tree can be found and if it's traversable. + * Also check for read permissions if the requested resource is + * a share, since a share can be create-only. + * + * @param RequestInterface $request + */ + public function checkPropFind($request) { + $path = $request->getPath(); + $node = $this->server->tree->getNodeForPath($path); + + if ($node instanceof Node) { + /** @var Node $node */ + '@phan-var Node $node'; + $fileInfo = $node->getFileInfo(); + list($storage, ) = Filesystem::resolvePath($fileInfo->getPath()); + + if ($storage->instanceOfStorage('\OCA\Files_Sharing\SharedStorage')) { + /** @var \OCA\Files_Sharing\SharedStorage $storage */ + '@phan-var \OCA\Files_Sharing\SharedStorage $storage'; + $hasReadPermission = ($storage->getShare()->getPermissions() & \OCP\Constants::PERMISSION_READ) > 0; + + if (!$hasReadPermission) { + throw new NotFound(); + } + } + } } /** diff --git a/apps/dav/lib/Connector/Sabre/ServerFactory.php b/apps/dav/lib/Connector/Sabre/ServerFactory.php index ad4af4d589e9..d0229205f98e 100644 --- a/apps/dav/lib/Connector/Sabre/ServerFactory.php +++ b/apps/dav/lib/Connector/Sabre/ServerFactory.php @@ -42,6 +42,8 @@ use OCP\IUserSession; use Sabre\DAV\Auth\Backend\BackendInterface; use OCP\AppFramework\Utility\ITimeFactory; +use Sabre\DAV\Exception\PreconditionFailed; +use Sabre\HTTP\Request; class ServerFactory { /** @var IConfig */ @@ -133,6 +135,16 @@ public function createServer( $server->addPlugin(new BrowserErrorPagePlugin()); } + $config = $this->config; + + $server->on('beforeMethod:PROPFIND', function (Request $request) use ($config) { + $depthHeader = strtolower($request->getHeader('depth')); + + if ($depthHeader === 'infinity' && !$config->getSystemValue('dav.propfind.depth_infinity', true)) { + throw new PreconditionFailed('Depth infinity not supported'); + } + }, 0); + // wait with registering these until auth is handled and the filesystem is setup $server->on('beforeMethod:*', function () use ($server, $objectTree, $viewCallBack) { // ensure the skeleton is copied diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index 186f56cf055e..0921b13b8821 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -74,6 +74,7 @@ use Sabre\CardDAV\VCFExportPlugin; use Sabre\DAV\Auth\Plugin; use Sabre\DAV\Exception; +use Sabre\HTTP\Request; class Server { @@ -226,6 +227,7 @@ public function __construct(IRequest $request, $baseUri) { )); if ($this->isRequestForSubtree(['files', 'trash-bin', 'public-files'])) { + \Sabre\DAV\Server::$streamMultiStatus = true; $this->server->addPlugin(new ViewOnlyPlugin( OC::$server->getLogger() )); @@ -236,6 +238,15 @@ public function __construct(IRequest $request, $baseUri) { } $this->server->addPlugin(new PreviewPlugin(OC::$server->getTimeFactory(), OC::$server->getPreviewManager())); + + $this->server->on('beforeMethod:PROPFIND', function (Request $request) use ($config) { + $depthHeader = strtolower($request->getHeader('depth')); + + if ($depthHeader === 'infinity' && !$config->getSystemValue('dav.propfind.depth_infinity', true)) { + throw new Exception\PreconditionFailed('Depth infinity not supported'); + } + }, 0); + // wait with registering these until auth is handled and the filesystem is setup $this->server->on('beforeMethod:*', function () use ($root) { // custom properties plugin must be the last one diff --git a/apps/dav/tests/unit/CapabilitiesTest.php b/apps/dav/tests/unit/CapabilitiesTest.php new file mode 100644 index 000000000000..0a059003ba80 --- /dev/null +++ b/apps/dav/tests/unit/CapabilitiesTest.php @@ -0,0 +1,59 @@ + + * @author Jannik Stehle + * + * @copyright Copyright (c) 2021, ownCloud GmbH + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +class CapabilitiesTest extends TestCase { + + /** @var IConfig| MockObject */ + protected $config; + + /** @var Capabilities */ + protected $capabilities; + + protected function setUp(): void { + parent::setUp(); + + $this->config = $this->createMock(IConfig::class); + $this->capabilities = new Capabilities($this->config); + } + + public function testGetCapabilities() { + $this->config->expects($this->exactly(2)) + ->method('getSystemValue') + ->withConsecutive(['dav.propfind.depth_infinity'], ['dav.enable.async']) + ->willReturn(true, true); + + $result = $this->capabilities->getCapabilities(); + $this->assertEquals($result['dav']['chunking'], '1.0'); + $this->assertEquals($result['dav']['reports'][0], 'search-files'); + $this->assertEquals($result['dav']['trashbin'], '1.0'); + $this->assertEquals($result['dav']['propfind']['depth_infinity'], true); + $this->assertEquals($result['async'], '1.0'); + } +} diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php index 659086062e5a..61b35a748307 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php @@ -25,6 +25,8 @@ */ namespace OCA\DAV\Tests\unit\Connector\Sabre; +use OC\Files\Filesystem; +use OC\Files\Mount\MountPoint; use OC\User\User; use OCA\DAV\Connector\Sabre\Directory; use OCA\DAV\Connector\Sabre\File; @@ -32,9 +34,13 @@ use OCP\Files\FileInfo; use OCA\DAV\Connector\Sabre\Node; use OCA\DAV\Meta\MetaFile; +use OCA\Files_Sharing\SharedStorage; use OCP\Files\StorageNotAvailableException; use OCP\IConfig; use OCP\IRequest; +use OCP\Share\IShare; +use Sabre\DAV\Exception\NotFound; +use Sabre\DAV\ICollection; use Sabre\DAV\PropFind; use Sabre\DAV\PropPatch; use Sabre\DAV\Server; @@ -49,6 +55,8 @@ * This file is licensed under the Affero General Public License version 3 or * later. * See the COPYING-README file. + * + * @group DB */ class FilesPluginTest extends TestCase { public const GETETAG_PROPERTYNAME = FilesPlugin::GETETAG_PROPERTYNAME; @@ -627,4 +635,58 @@ public function testAdditionalHeaders() { $this->plugin->httpGet($request, $response); } + + public function testCheckPropfindInvalidPath() { + $this->expectException(NotFound::class); + + $this->plugin = new FilesPlugin( + $this->tree, + $this->config, + $this->request + ); + + $this->server->tree = new Tree($this->getMockBuilder(ICollection::class)->getMock()); + $this->plugin->initialize($this->server); + + $sabreRequest = $this->getMockBuilder(RequestInterface::class)->disableOriginalConstructor()->getMock(); + $sabreRequest->method('getPath')->willReturn('/invalid/path'); + + $this->plugin->checkPropFind($sabreRequest); + } + + public function testCheckPropfindInsufficientPermissions() { + $this->expectException(NotFound::class); + + $this->plugin = new FilesPlugin( + $this->tree, + $this->config, + $this->request + ); + + $node = $this->createMock(Node::class); + + $shareMock = $this->createMock(IShare::class); + $shareMock->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_CREATE); + + $storage = $this->createMock(SharedStorage::class); + $storage->method('instanceOfStorage')->willReturn(true); + $storage->method('getShare')->willReturn($shareMock); + $mount = new MountPoint($storage, '/user2/files/sharedfolder', [[]], Filesystem::getLoader()); + Filesystem::getMountManager()->addMount($mount); + + $fileInfo = $this->createMock(FileInfo::class); + $fileInfo->method('getPath')->willReturn('user2/files/sharedfolder'); + + $node->method('getFileInfo')->willReturn($fileInfo); + + $this->tree->method('getNodeForPath')->willReturn($node); + $this->server->tree = $this->tree; + $this->plugin->initialize($this->server); + + $sabreRequest = $this->getMockBuilder(RequestInterface::class)->disableOriginalConstructor()->getMock(); + $sabreRequest->method('getPath')->willReturn('user2/files/sharedfolder'); + + $this->plugin->checkPropFind($sabreRequest); + Filesystem::getMountManager()->removeMount('/user2/files/sharedfolder'); + } } diff --git a/changelog/unreleased/38583 b/changelog/unreleased/38583 new file mode 100644 index 000000000000..3c88d4f1127e --- /dev/null +++ b/changelog/unreleased/38583 @@ -0,0 +1,9 @@ +Change: Enable streaming for propfind requests + +Propfind requests will now be streamed to reduce memory usage +with large responses. +Additionally, the new config `dav.propfind.depth_infinity` has been +added to tell clients whether `depth=infinity` is allowed for +propfind requests. It defaults to true. + +https://github.com/owncloud/core/pull/38583 diff --git a/config/config.sample.php b/config/config.sample.php index 213a5b3ee122..33405fd310e8 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -1657,6 +1657,15 @@ */ 'dav.enable.async' => false, +/** + * Allow propfind depth infinity + * + * With this setting that defaults to true, propfind requests will now be streamed to reduce memory usage + * with large responses. It tells the clients whether `depth=infinity` is allowed for propfind requests. + * For details see: https://datatracker.ietf.org/doc/html/rfc4918#section-10.2 + */ +'dav.propfind.depth_infinity' => true, + /** * Show the grace period popup * Decide whether show or not the grace period popup. There is no change in the diff --git a/tests/acceptance/features/apiWebdavOperations/propfind.feature b/tests/acceptance/features/apiWebdavOperations/propfind.feature index 4c88ab7abb9c..1782054bff1b 100644 --- a/tests/acceptance/features/apiWebdavOperations/propfind.feature +++ b/tests/acceptance/features/apiWebdavOperations/propfind.feature @@ -7,9 +7,17 @@ Feature: PROPFIND When user "Alice" requests "/remote.php/dav/files" with "PROPFIND" using basic auth Then the HTTP status code should be "405" - Scenario: PROPFIND to "/remote.php/dav/files" with depth header + @skipOnOcV10.6 @skipOnOcV10.7 @skipOnOcV10.8.0 + Scenario Outline: PROPFIND to "/remote.php/dav/files" with depth header Given user "Alice" has been created with default attributes and without skeleton files - When user "Alice" requests "/remote.php/dav/files" with "PROPFIND" using basic auth and with headers - | header | value | - | depth | 0 | - Then the HTTP status code should be "207" + And the administrator has set depth_infinity_allowed to + When user "Alice" requests "/remote.php/dav/files/alice" with "PROPFIND" using basic auth and with headers + | header | value | + | depth | | + Then the HTTP status code should be "" + Examples: + | depth_infinity_allowed | depth | http_status | + | 1 | 0 | 207 | + | 1 | infinity | 207 | + | 0 | 0 | 207 | + | 0 | infinity | 412 | diff --git a/tests/acceptance/features/bootstrap/OccContext.php b/tests/acceptance/features/bootstrap/OccContext.php index 666de4961ad7..271d75700785 100644 --- a/tests/acceptance/features/bootstrap/OccContext.php +++ b/tests/acceptance/features/bootstrap/OccContext.php @@ -938,6 +938,20 @@ public function theAdministratorHasSetTheDefaultFolderForReceivedSharesTo(string ); } + /** + * @When the administrator has set depth_infinity_allowed to :depth_infinity_allowed + * + * @param int $depth_infinity_allowed + * + * @return void + */ + public function theAdministratorHasSetDepthInfinityAllowedTo($depth_infinity_allowed) { + $this->addSystemConfigKeyUsingTheOccCommand( + "dav.propfind.depth_infinity", + $depth_infinity_allowed + ); + } + /** * @Given the administrator has set the mail smtpmode to :smtpmode *