Merge pull request #15145 from owncloud/fix-15097-master

Properly catch whether a share is `null`

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

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;
+		$originalSharePath = $this->getPath($token);
-		$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']);
-		}
 
 		// Share is password protected - check whether the user is permitted to access the share
 		if (isset($linkItem['share_with']) && !Helper::authenticate($linkItem)) {
@@ -161,11 +154,13 @@ class ShareController extends Controller {
 		if (Filesystem::isReadable($originalSharePath . $path)) {
 			$getPath = Filesystem::normalizePath($path);
 			$originalSharePath .= $path;
+		} else {
+			throw new OCP\Files\NotFoundException();
 		}
 
 		$file = basename($originalSharePath);
 
-		$shareTmpl = array();
+		$shareTmpl = [];
 		$shareTmpl['displayName'] = User::getDisplayName($shareOwner);
 		$shareTmpl['filename'] = $file;
 		$shareTmpl['directory_path'] = $linkItem['file_target'];
@@ -289,22 +284,29 @@ class ShareController extends Controller {
 	}
 
 	/**
-	 * @param $token
+	 * @param string $token
-	 * @return null|string
+	 * @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;
 				}
 			}
+		}
+
+		throw new \Exception('No file found belonging to file.');
+	}
+}

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

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 \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);
+	}
+
 }

lib/public/appframework/http/notfoundresponse.php

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