Merge pull request #19470 from owncloud/pass-exception

Only intercept exceptions of type "NotFoundException" instead of any …

3 files changed, 76 insertions, 10 deletions

diff --git a/apps/files_sharing/lib/controllers/sharecontroller.php b/apps/files_sharing/lib/controllers/sharecontroller.php
index ecf3ee853ee..28feb3110b4 100644
--- a/apps/files_sharing/lib/controllers/sharecontroller.php
+++ b/apps/files_sharing/lib/controllers/sharecontroller.php
@@ -46,6 +46,7 @@ use OCA\Files_Sharing\Helper;
 use OCP\User;
 use OCP\Util;
 use OCA\Files_Sharing\Activity;
+use \OCP\Files\NotFoundException;
 
 /**
  * Class ShareController
@@ -148,6 +149,7 @@ class ShareController extends Controller {
 	 * @param string $token
 	 * @param string $path
 	 * @return TemplateResponse|RedirectResponse
+	 * @throws NotFoundException
 	 */
 	public function showShare($token, $path = '') {
 		\OC_User::setIncognitoMode(true);
@@ -171,7 +173,7 @@ class ShareController extends Controller {
 			$getPath = Filesystem::normalizePath($path);
 			$originalSharePath .= $path;
 		} else {
-			throw new OCP\Files\NotFoundException();
+			throw new NotFoundException();
 		}
 
 		$file = basename($originalSharePath);
@@ -303,7 +305,7 @@ class ShareController extends Controller {
 	/**
 	 * @param string $token
 	 * @return string Resolved file path of the token
-	 * @throws \Exception In case share could not get properly resolved
+	 * @throws NotFoundException In case share could not get properly resolved
 	 */
 	private function getPath($token) {
 		$linkItem = Share::getShareByToken($token, false);
@@ -312,7 +314,7 @@ class ShareController extends Controller {
 			$rootLinkItem = Share::resolveReShare($linkItem);
 			if (isset($rootLinkItem['uid_owner'])) {
 				if(!$this->userManager->userExists($rootLinkItem['uid_owner'])) {
-					throw new \Exception('Owner of the share does not exist anymore');
+					throw new NotFoundException('Owner of the share does not exist anymore');
 				}
 				OC_Util::tearDownFS();
 				OC_Util::setupFS($rootLinkItem['uid_owner']);
@@ -324,6 +326,6 @@ class ShareController extends Controller {
 			}
 		}
 
-		throw new \Exception('No file found belonging to file.');
+		throw new NotFoundException('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 1c29b1da736..61dfd914d0b 100644
--- a/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php
+++ b/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php
@@ -27,6 +27,7 @@ use OCP\App\IAppManager;
 use OCP\AppFramework\Http\NotFoundResponse;
 use OCP\AppFramework\Middleware;
 use OCP\AppFramework\Http\TemplateResponse;
+use OCP\Files\NotFoundException;
 use OCP\IConfig;
 
 /**
@@ -58,22 +59,32 @@ class SharingCheckMiddleware extends Middleware {
 
 	/**
 	 * Check if sharing is enabled before the controllers is executed
+	 *
+	 * @param \OCP\AppFramework\Controller $controller
+	 * @param string $methodName
+	 * @throws NotFoundException
 	 */
 	public function beforeController($controller, $methodName) {
 		if(!$this->isSharingEnabled()) {
-			throw new \Exception('Sharing is disabled.');
+			throw new NotFoundException('Sharing is disabled.');
 		}
 	}
 
 	/**
-	 * Return 404 page in case of an exception
+	 * Return 404 page in case of a not found exception
+	 *
 	 * @param \OCP\AppFramework\Controller $controller
 	 * @param string $methodName
 	 * @param \Exception $exception
-	 * @return TemplateResponse
+	 * @return NotFoundResponse
+	 * @throws \Exception
 	 */
-	public function afterException($controller, $methodName, \Exception $exception){
-		return new NotFoundResponse();
+	public function afterException($controller, $methodName, \Exception $exception) {
+		if(is_a($exception, '\OCP\Files\NotFoundException')) {
+			return new NotFoundResponse();
+		}
+
+		throw $exception;
 	}
 
 	/**
diff --git a/apps/files_sharing/tests/middleware/sharingcheckmiddleware.php b/apps/files_sharing/tests/middleware/sharingcheckmiddleware.php
index 58f4b841339..3171d45d331 100644
--- a/apps/files_sharing/tests/middleware/sharingcheckmiddleware.php
+++ b/apps/files_sharing/tests/middleware/sharingcheckmiddleware.php
@@ -23,7 +23,8 @@
  */
 
 namespace OCA\Files_Sharing\Middleware;
-
+use OCP\AppFramework\Http\NotFoundResponse;
+use OCP\Files\NotFoundException;
 
 /**
  * @package OCA\Files_Sharing\Middleware\SharingCheckMiddleware
@@ -36,12 +37,16 @@ class SharingCheckMiddlewareTest extends \Test\TestCase {
 	private $appManager;
 	/** @var SharingCheckMiddleware */
 	private $sharingCheckMiddleware;
+	/** @var \OCP\AppFramework\Controller */
+	private $controllerMock;
 
 	protected function setUp() {
 		$this->config = $this->getMockBuilder('\OCP\IConfig')
 			->disableOriginalConstructor()->getMock();
 		$this->appManager = $this->getMockBuilder('\OCP\App\IAppManager')
 			->disableOriginalConstructor()->getMock();
+		$this->controllerMock = $this->getMockBuilder('\OCP\AppFramework\Controller')
+			->disableOriginalConstructor()->getMock();
 
 		$this->sharingCheckMiddleware = new SharingCheckMiddleware('files_sharing', $this->config, $this->appManager);
 	}
@@ -116,4 +121,52 @@ class SharingCheckMiddlewareTest extends \Test\TestCase {
 		$this->assertFalse(self::invokePrivate($this->sharingCheckMiddleware, 'isSharingEnabled'));
 	}
 
+	public function testBeforeControllerWithSharingEnabled() {
+		$this->appManager
+			->expects($this->once())
+			->method('isEnabledForUser')
+			->with('files_sharing')
+			->will($this->returnValue(true));
+
+		$this->config
+			->expects($this->at(0))
+			->method('getAppValue')
+			->with('core', 'shareapi_enabled', 'yes')
+			->will($this->returnValue('yes'));
+
+		$this->config
+			->expects($this->at(1))
+			->method('getAppValue')
+			->with('core', 'shareapi_allow_links', 'yes')
+			->will($this->returnValue('yes'));
+
+		$this->sharingCheckMiddleware->beforeController($this->controllerMock, 'myMethod');
+	}
+
+	/**
+	 * @expectedException \OCP\Files\NotFoundException
+	 * @expectedExceptionMessage Sharing is disabled.
+	 */
+	public function testBeforeControllerWithSharingDisabled() {
+		$this->appManager
+			->expects($this->once())
+			->method('isEnabledForUser')
+			->with('files_sharing')
+			->will($this->returnValue(false));
+
+		$this->sharingCheckMiddleware->beforeController($this->controllerMock, 'myMethod');
+	}
+
+	/**
+	 * @expectedException \Exception
+	 * @expectedExceptionMessage My Exception message
+	 */
+	public function testAfterExceptionWithRegularException() {
+		$this->sharingCheckMiddleware->afterException($this->controllerMock, 'myMethod', new \Exception('My Exception message'));
+	}
+
+	public function testAfterExceptionWithNotFoundException() {
+		$this->assertEquals(new NotFoundResponse(), $this->sharingCheckMiddleware->afterException($this->controllerMock, 'myMethod', new NotFoundException('My Exception message')));
+	}
+
 }