From fd9166488b5924aba74d3f77bb6795be03501c81 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 3 Dec 2015 15:29:42 +0100 Subject: [PATCH 1/4] Check that the owner of a link share still has share permissions on access --- apps/dav/appinfo/v1/publicwebdav.php | 8 ++- .../dav/lib/connector/sabre/serverfactory.php | 2 +- .../files/sharing/publiclinkcheckplugin.php | 63 +++++++++++++++++++ 3 files changed, 71 insertions(+), 2 deletions(-) create mode 100644 apps/dav/lib/files/sharing/publiclinkcheckplugin.php diff --git a/apps/dav/appinfo/v1/publicwebdav.php b/apps/dav/appinfo/v1/publicwebdav.php index 6ddb570aca..b0ee264aac 100644 --- a/apps/dav/appinfo/v1/publicwebdav.php +++ b/apps/dav/appinfo/v1/publicwebdav.php @@ -46,7 +46,9 @@ $serverFactory = new OCA\DAV\Connector\Sabre\ServerFactory( $requestUri = \OC::$server->getRequest()->getRequestUri(); -$server = $serverFactory->createServer($baseuri, $requestUri, $authBackend, function () use ($authBackend) { +$linkCheckPlugin = new \OCA\DAV\Files\Sharing\PublicLinkCheckPlugin(); + +$server = $serverFactory->createServer($baseuri, $requestUri, $authBackend, function (\Sabre\DAV\Server $server) use ($authBackend, $linkCheckPlugin) { $isAjax = (isset($_SERVER['HTTP_X_REQUESTED_WITH']) && $_SERVER['HTTP_X_REQUESTED_WITH'] === 'XMLHttpRequest'); if (OCA\Files_Sharing\Helper::isOutgoingServer2serverShareEnabled() === false && !$isAjax) { // this is what is thrown when trying to access a non-existing share @@ -68,9 +70,13 @@ $server = $serverFactory->createServer($baseuri, $requestUri, $authBackend, func OC_Util::setupFS($owner); $ownerView = \OC\Files\Filesystem::getView(); $path = $ownerView->getPath($fileId); + $fileInfo = $ownerView->getFileInfo($path); + $linkCheckPlugin->setFileInfo($fileInfo); return new \OC\Files\View($ownerView->getAbsolutePath($path)); }); +$server->addPlugin($linkCheckPlugin); + // And off we go! $server->exec(); diff --git a/apps/dav/lib/connector/sabre/serverfactory.php b/apps/dav/lib/connector/sabre/serverfactory.php index 9a828787a0..8253948d96 100644 --- a/apps/dav/lib/connector/sabre/serverfactory.php +++ b/apps/dav/lib/connector/sabre/serverfactory.php @@ -118,7 +118,7 @@ class ServerFactory { $userFolder = \OC::$server->getUserFolder(); /** @var \OC\Files\View $view */ - $view = $viewCallBack(); + $view = $viewCallBack($server); $rootInfo = $view->getFileInfo(''); // Create ownCloud Dir diff --git a/apps/dav/lib/files/sharing/publiclinkcheckplugin.php b/apps/dav/lib/files/sharing/publiclinkcheckplugin.php new file mode 100644 index 0000000000..bbb5c61120 --- /dev/null +++ b/apps/dav/lib/files/sharing/publiclinkcheckplugin.php @@ -0,0 +1,63 @@ + + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @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 + * + */ + +namespace OCA\DAV\Files\Sharing; + +use OCP\Files\FileInfo; +use Sabre\DAV\Exception\NotFound; +use Sabre\DAV\ServerPlugin; +use Sabre\HTTP\RequestInterface; +use Sabre\HTTP\ResponseInterface; + +/** + * Verify that the public link share is valid + */ +class PublicLinkCheckPlugin extends ServerPlugin { + /** + * @var FileInfo + */ + private $fileInfo; + + /** + * @param FileInfo $fileInfo + */ + public function setFileInfo($fileInfo) { + $this->fileInfo = $fileInfo; + } + + /** + * This initializes the plugin. + * + * @param \Sabre\DAV\Server $server Sabre server + * + * @return void + */ + public function initialize(\Sabre\DAV\Server $server) { + $server->on('beforeMethod', [$this, 'beforeMethod']); + } + + public function beforeMethod(RequestInterface $request, ResponseInterface $response){ + // verify that the owner didn't have his share permissions revoked + if ($this->fileInfo && !$this->fileInfo->isShareable()) { + throw new NotFound(); + } + } +} From f9f28000166628315a0e212ff51ef104242ab96f Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 9 Feb 2016 13:00:08 +0100 Subject: [PATCH 2/4] check share permissions in share controller --- .../lib/controllers/sharecontroller.php | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/apps/files_sharing/lib/controllers/sharecontroller.php b/apps/files_sharing/lib/controllers/sharecontroller.php index dae61a3537..08679c88bb 100644 --- a/apps/files_sharing/lib/controllers/sharecontroller.php +++ b/apps/files_sharing/lib/controllers/sharecontroller.php @@ -227,6 +227,16 @@ class ShareController extends Controller { } } + /** + * Validate the permissions of the share + * + * @param Share\IShare $share + * @return bool + */ + private function validateShare(\OCP\Share\IShare $share) { + return $share->getNode()->isReadable() && $share->getNode()->isShareable(); + } + /** * @PublicPage * @NoCSRFRequired @@ -253,6 +263,9 @@ class ShareController extends Controller { array('token' => $token))); } + if (!$this->validateShare($share)) { + throw new NotFoundException(); + } // We can't get the path of a file share try { if ($share->getNode() instanceof \OCP\Files\File && $path !== '') { @@ -371,6 +384,10 @@ class ShareController extends Controller { $userFolder = $this->rootFolder->getUserFolder($share->getShareOwner()); $originalSharePath = $userFolder->getRelativePath($share->getNode()->getPath()); + if (!$this->validateShare($share)) { + throw new NotFoundException(); + } + // Single file share if ($share->getNode() instanceof \OCP\Files\File) { // Single file download From 359c62d90ec5729541a4b97d0e7c9116abb409e9 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Tue, 9 Feb 2016 14:56:47 +0100 Subject: [PATCH 3/4] Fix unit tests --- apps/files_sharing/tests/controller/sharecontroller.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/files_sharing/tests/controller/sharecontroller.php b/apps/files_sharing/tests/controller/sharecontroller.php index 11dc082390..8eb9cb8284 100644 --- a/apps/files_sharing/tests/controller/sharecontroller.php +++ b/apps/files_sharing/tests/controller/sharecontroller.php @@ -304,6 +304,8 @@ class ShareControllerTest extends \Test\TestCase { $file->method('getName')->willReturn('file1.txt'); $file->method('getMimetype')->willReturn('text/plain'); $file->method('getSize')->willReturn(33); + $file->method('isReadable')->willReturn(true); + $file->method('isShareable')->willReturn(true); $share = \OC::$server->getShareManager()->newShare(); $share->setId(42); From acd8c72d3dc25787950de8d1d7b8e735eff7b28f Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 9 Feb 2016 15:01:12 +0100 Subject: [PATCH 4/4] add tests --- .../tests/controller/sharecontroller.php | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/apps/files_sharing/tests/controller/sharecontroller.php b/apps/files_sharing/tests/controller/sharecontroller.php index 8eb9cb8284..6167b281fc 100644 --- a/apps/files_sharing/tests/controller/sharecontroller.php +++ b/apps/files_sharing/tests/controller/sharecontroller.php @@ -365,6 +365,55 @@ class ShareControllerTest extends \Test\TestCase { $this->assertEquals($expectedResponse, $response); } + /** + * @expectedException \OCP\Files\NotFoundException + */ + public function testShowShareInvalid() { + $owner = $this->getMock('OCP\IUser'); + $owner->method('getDisplayName')->willReturn('ownerDisplay'); + $owner->method('getUID')->willReturn('ownerUID'); + + $file = $this->getMock('OCP\Files\File'); + $file->method('getName')->willReturn('file1.txt'); + $file->method('getMimetype')->willReturn('text/plain'); + $file->method('getSize')->willReturn(33); + $file->method('isShareable')->willReturn(false); + $file->method('isReadable')->willReturn(true); + + $share = \OC::$server->getShareManager()->newShare(); + $share->setId(42); + $share->setPassword('password') + ->setShareOwner('ownerUID') + ->setNode($file) + ->setTarget('/file1.txt'); + + $this->session->method('exists')->with('public_link_authenticated')->willReturn(true); + $this->session->method('get')->with('public_link_authenticated')->willReturn('42'); + + $this->previewManager->method('isMimeSupported')->with('text/plain')->willReturn(true); + + $this->config->method('getSystemValue') + ->willReturnMap( + [ + ['max_filesize_animated_gifs_public_sharing', 10, 10], + ['enable_previews', true, true], + ] + ); + $shareTmpl['maxSizeAnimateGif'] = $this->config->getSystemValue('max_filesize_animated_gifs_public_sharing', 10); + $shareTmpl['previewEnabled'] = $this->config->getSystemValue('enable_previews', true); + + $this->shareManager + ->expects($this->once()) + ->method('getShareByToken') + ->with('token') + ->willReturn($share); + + $this->userManager->method('get')->with('ownerUID')->willReturn($owner); + + $this->shareController->showShare('token'); + } + + public function testDownloadShare() { $share = $this->getMock('\OCP\Share\IShare'); $share->method('getPassword')->willReturn('password');