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
This commit is contained in:
Lukas Reschke 2015-03-24 11:21:58 +01:00
parent 02c0fe8d43
commit e2453d78c0
5 changed files with 128 additions and 23 deletions

View File

@ -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();
});

View File

@ -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'];
@ -289,22 +282,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.');
}
}

View File

@ -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();
}
/**

View File

@ -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 \Test\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 \Test\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 \Test\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
@ -176,4 +184,54 @@ class ShareControllerTest extends \Test\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);
}
}

View File

@ -0,0 +1,43 @@
<?php
/**
* @author Lukas Reschke <lukas@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();
}
}