[Pkg-owncloud-commits] [owncloud] 85/118: Properly catch whether a share is `null`
David Prévot
taffit at moszumanska.debian.org
Fri Mar 27 22:13:16 UTC 2015
This is an automated email from the git hooks/post-receive script.
taffit pushed a commit to branch stable8
in repository owncloud.
commit 78c348381d8e6244cc147b8a7a092e79c1550d8d
Author: Lukas Reschke <lukas at owncloud.com>
Date: Tue Mar 24 11:21:58 2015 +0100
Properly catch whether a share is `null`
Despite it's PHPDoc the function might return `null` which was not properly catched and thus in some situations the share was resolved to the sharing users root directory.
To test this perform the following steps:
* Share file in owncloud 7 (7.0.4.2)
* Delete the parent folder of the shared file
* The share stays is in the DB and the share via the sharelink is inaccessible. (which is good)
* Upgrade to owncloud 8 (8.0.2) (This step is crucial. The bug is not reproduceable without upgrading from 7 to 8. It seems like the old tokens are handled different than the newer ones)
* Optional Step: Logout, Reset Browser Session, etc.
* Access the share via the old share url: almost empty page, but there is a dowload button which adds a "/download" to the URL.
* Upon clicking, a download.zip is downloaded which contains EVERYTHING from the owncloud directory (of the user who shared the file)
* No exception is thrown and no error is logged.
This will add a check whether the share is a valid one and also adds unit tests to prevent further regressions in the future. Needs to be backported to ownCloud 8.
Adding a proper clean-up of the orphaned shares is out-of-scope and would probably require some kind of FK or so.
Fixes https://github.com/owncloud/core/issues/15097
---
apps/files_sharing/application.php | 5 +-
.../lib/controllers/sharecontroller.php | 38 ++++++-------
.../lib/middleware/sharingcheckmiddleware.php | 3 +-
.../tests/controller/sharecontroller.php | 62 +++++++++++++++++++++-
lib/public/appframework/http/notfoundresponse.php | 43 +++++++++++++++
5 files changed, 128 insertions(+), 23 deletions(-)
diff --git a/apps/files_sharing/application.php b/apps/files_sharing/application.php
index 3302848..e23960c 100644
--- a/apps/files_sharing/application.php
+++ b/apps/files_sharing/application.php
@@ -42,7 +42,7 @@ class Application extends App {
$server->getAppConfig(),
$server->getConfig(),
$c->query('URLGenerator'),
- $server->getUserManager(),
+ $c->query('UserManager'),
$server->getLogger(),
$server->getActivityManager()
);
@@ -65,6 +65,9 @@ class Application extends App {
$container->registerService('URLGenerator', function(SimpleContainer $c) use ($server){
return $server->getUrlGenerator();
});
+ $container->registerService('UserManager', function(SimpleContainer $c) use ($server){
+ return $server->getUserManager();
+ });
$container->registerService('IsIncomingShareEnabled', function(SimpleContainer $c) {
return Helper::isIncomingServer2serverShareEnabled();
});
diff --git a/apps/files_sharing/lib/controllers/sharecontroller.php b/apps/files_sharing/lib/controllers/sharecontroller.php
index cd013d4..1bb3666 100644
--- a/apps/files_sharing/lib/controllers/sharecontroller.php
+++ b/apps/files_sharing/lib/controllers/sharecontroller.php
@@ -17,12 +17,12 @@ use OC_Files;
use OC_Util;
use OCP;
use OCP\Template;
-use OCP\JSON;
use OCP\Share;
use OCP\AppFramework\Controller;
use OCP\IRequest;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\AppFramework\Http\RedirectResponse;
+use OCP\AppFramework\Http\NotFoundResponse;
use OC\URLGenerator;
use OC\AppConfig;
use OCP\ILogger;
@@ -60,7 +60,7 @@ class ShareController extends Controller {
* @param AppConfig $appConfig
* @param OCP\IConfig $config
* @param URLGenerator $urlGenerator
- * @param OC\User\Manager $userManager
+ * @param OCP\IUserManager $userManager
* @param ILogger $logger
* @param OCP\Activity\IManager $activityManager
*/
@@ -70,7 +70,7 @@ class ShareController extends Controller {
AppConfig $appConfig,
OCP\IConfig $config,
URLGenerator $urlGenerator,
- OC\User\Manager $userManager,
+ OCP\IUserManager $userManager,
ILogger $logger,
OCP\Activity\IManager $activityManager) {
parent::__construct($appName, $request);
@@ -113,7 +113,7 @@ class ShareController extends Controller {
public function authenticate($token, $password = '') {
$linkItem = Share::getShareByToken($token, false);
if($linkItem === false) {
- return new TemplateResponse('core', '404', array(), 'guest');
+ return new NotFoundResponse();
}
$authenticate = Helper::authenticate($linkItem, $password);
@@ -139,18 +139,11 @@ class ShareController extends Controller {
// Check whether share exists
$linkItem = Share::getShareByToken($token, false);
if($linkItem === false) {
- return new TemplateResponse('core', '404', array(), 'guest');
+ return new NotFoundResponse();
}
$shareOwner = $linkItem['uid_owner'];
- $originalSharePath = null;
- $rootLinkItem = OCP\Share::resolveReShare($linkItem);
- if (isset($rootLinkItem['uid_owner'])) {
- OCP\JSON::checkUserExists($rootLinkItem['uid_owner']);
- OC_Util::tearDownFS();
- OC_Util::setupFS($rootLinkItem['uid_owner']);
- $originalSharePath = Filesystem::getPath($linkItem['file_source']);
- }
+ $originalSharePath = $this->getPath($token);
// Share is password protected - check whether the user is permitted to access the share
if (isset($linkItem['share_with']) && !Helper::authenticate($linkItem)) {
@@ -165,7 +158,7 @@ class ShareController extends Controller {
$file = basename($originalSharePath);
- $shareTmpl = array();
+ $shareTmpl = [];
$shareTmpl['displayName'] = User::getDisplayName($shareOwner);
$shareTmpl['filename'] = $file;
$shareTmpl['directory_path'] = $linkItem['file_target'];
@@ -263,22 +256,29 @@ class ShareController extends Controller {
}
/**
- * @param $token
- * @return null|string
+ * @param string $token
+ * @return string Resolved file path of the token
+ * @throws \Exception In case share could not get properly resolved
*/
private function getPath($token) {
$linkItem = Share::getShareByToken($token, false);
- $path = null;
if (is_array($linkItem) && isset($linkItem['uid_owner'])) {
// seems to be a valid share
$rootLinkItem = Share::resolveReShare($linkItem);
if (isset($rootLinkItem['uid_owner'])) {
- JSON::checkUserExists($rootLinkItem['uid_owner']);
+ if(!$this->userManager->userExists($rootLinkItem['uid_owner'])) {
+ throw new \Exception('Owner of the share does not exist anymore');
+ }
OC_Util::tearDownFS();
OC_Util::setupFS($rootLinkItem['uid_owner']);
$path = Filesystem::getPath($linkItem['file_source']);
+
+ if(!empty($path) && Filesystem::isReadable($path)) {
+ return $path;
+ }
}
}
- return $path;
+
+ throw new \Exception('No file found belonging to file.');
}
}
diff --git a/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php b/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php
index 3508407..3e7cdf4 100644
--- a/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php
+++ b/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php
@@ -11,6 +11,7 @@
namespace OCA\Files_Sharing\Middleware;
use OCP\App\IAppManager;
+use OCP\AppFramework\Http\NotFoundResponse;
use OCP\AppFramework\Middleware;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\IConfig;
@@ -59,7 +60,7 @@ class SharingCheckMiddleware extends Middleware {
* @return TemplateResponse
*/
public function afterException($controller, $methodName, \Exception $exception){
- return new TemplateResponse('core', '404', array(), 'guest');
+ return new NotFoundResponse();
}
/**
diff --git a/apps/files_sharing/tests/controller/sharecontroller.php b/apps/files_sharing/tests/controller/sharecontroller.php
index 931cd50..ed17dd8 100644
--- a/apps/files_sharing/tests/controller/sharecontroller.php
+++ b/apps/files_sharing/tests/controller/sharecontroller.php
@@ -12,6 +12,7 @@ namespace OCA\Files_Sharing\Controllers;
use OC\Files\Filesystem;
use OCA\Files_Sharing\Application;
+use OCP\AppFramework\Http\NotFoundResponse;
use OCP\AppFramework\IAppContainer;
use OCP\Files;
use OCP\AppFramework\Http\RedirectResponse;
@@ -49,6 +50,8 @@ class ShareControllerTest extends \PHPUnit_Framework_TestCase {
->disableOriginalConstructor()->getMock();
$this->container['URLGenerator'] = $this->getMockBuilder('\OC\URLGenerator')
->disableOriginalConstructor()->getMock();
+ $this->container['UserManager'] = $this->getMockBuilder('\OCP\IUserManager')
+ ->disableOriginalConstructor()->getMock();
$this->urlGenerator = $this->container['URLGenerator'];
$this->shareController = $this->container['ShareController'];
@@ -115,7 +118,7 @@ class ShareControllerTest extends \PHPUnit_Framework_TestCase {
public function testAuthenticate() {
// Test without a not existing token
$response = $this->shareController->authenticate('ThisTokenShouldHopefullyNeverExistSoThatTheUnitTestWillAlwaysPass :)');
- $expectedResponse = new TemplateResponse('core', '404', array(), 'guest');
+ $expectedResponse = new NotFoundResponse();
$this->assertEquals($expectedResponse, $response);
// Test with a valid password
@@ -130,9 +133,14 @@ class ShareControllerTest extends \PHPUnit_Framework_TestCase {
}
public function testShowShare() {
+ $this->container['UserManager']->expects($this->exactly(2))
+ ->method('userExists')
+ ->with($this->user)
+ ->will($this->returnValue(true));
+
// Test without a not existing token
$response = $this->shareController->showShare('ThisTokenShouldHopefullyNeverExistSoThatTheUnitTestWillAlwaysPass :)');
- $expectedResponse = new TemplateResponse('core', '404', array(), 'guest');
+ $expectedResponse = new NotFoundResponse();
$this->assertEquals($expectedResponse, $response);
// Test with a password protected share and no authentication
@@ -170,4 +178,54 @@ class ShareControllerTest extends \PHPUnit_Framework_TestCase {
array('token' => $this->token)));
$this->assertEquals($expectedResponse, $response);
}
+
+ /**
+ * @expectedException \Exception
+ * @expectedExceptionMessage No file found belonging to file.
+ */
+ public function testShowShareWithDeletedFile() {
+ $this->container['UserManager']->expects($this->once())
+ ->method('userExists')
+ ->with($this->user)
+ ->will($this->returnValue(true));
+
+ $view = new View('/'. $this->user . '/files');
+ $view->unlink('file1.txt');
+ $linkItem = Share::getShareByToken($this->token, false);
+ \OC::$server->getSession()->set('public_link_authenticated', $linkItem['id']);
+ $this->shareController->showShare($this->token);
+ }
+
+ /**
+ * @expectedException \Exception
+ * @expectedExceptionMessage No file found belonging to file.
+ */
+ public function testDownloadShareWithDeletedFile() {
+ $this->container['UserManager']->expects($this->once())
+ ->method('userExists')
+ ->with($this->user)
+ ->will($this->returnValue(true));
+
+ $view = new View('/'. $this->user . '/files');
+ $view->unlink('file1.txt');
+ $linkItem = Share::getShareByToken($this->token, false);
+ \OC::$server->getSession()->set('public_link_authenticated', $linkItem['id']);
+ $this->shareController->downloadShare($this->token);
+ }
+
+ /**
+ * @expectedException \Exception
+ * @expectedExceptionMessage Owner of the share does not exist anymore
+ */
+ public function testShowShareWithNotExistingUser() {
+ $this->container['UserManager']->expects($this->once())
+ ->method('userExists')
+ ->with($this->user)
+ ->will($this->returnValue(false));
+
+ $linkItem = Share::getShareByToken($this->token, false);
+ \OC::$server->getSession()->set('public_link_authenticated', $linkItem['id']);
+ $this->shareController->showShare($this->token);
+ }
+
}
diff --git a/lib/public/appframework/http/notfoundresponse.php b/lib/public/appframework/http/notfoundresponse.php
new file mode 100644
index 0000000..21f0461
--- /dev/null
+++ b/lib/public/appframework/http/notfoundresponse.php
@@ -0,0 +1,43 @@
+<?php
+/**
+ * @author Lukas Reschke <lukas at owncloud.com>
+ *
+ * @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 <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace OCP\AppFramework\Http;
+
+use OCP\AppFramework\Http;
+use OCP\Template;
+
+/**
+ * A generic 404 response showing an 404 error page as well to the end-user
+ */
+class NotFoundResponse extends Response {
+
+ public function __construct() {
+ $this->setStatus(404);
+ }
+
+ /**
+ * @return string
+ */
+ public function render() {
+ $template = new Template('core', '404', 'guest');
+ return $template->fetchPage();
+ }
+}
--
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-owncloud/owncloud.git
More information about the Pkg-owncloud-commits
mailing list