Merge pull request #666 from owncloud/stable9-fix-listing-issuev9.0.3RC1v9.0.3

[stable9] Do not allow access to write-only shares

5 files changed, 81 insertions, 5 deletions

diff --git a/appinfo/application.php b/appinfo/application.php
index c0200a14..bacc4229 100644
--- a/appinfo/application.php
+++ b/appinfo/application.php
@@ -236,6 +236,13 @@ class Application extends App {
 					 ->getUserFolder($c->query('UserId'));
 		}
 		);
+		$container->registerService(
+			'ShareManager', function (IAppContainer $c) {
+			return $c->getServer()
+				->getShareManager();
+		}
+		);
+
 
 		/**
 		 * OCA
@@ -394,7 +401,8 @@ class Application extends App {
 					$c->query('Environment'),
 					$c->query('OCP\AppFramework\Utility\IControllerMethodReflector'),
 					$c->query('OCP\IURLGenerator'),
-					$c->query('Logger')
+					$c->query('Logger'),
+					$c->query('ShareManager')
 				);
 			}
 		);
diff --git a/middleware/checkexception.php b/middleware/checkexception.php
index 3c8cd72e..60320796 100644
--- a/middleware/checkexception.php
+++ b/middleware/checkexception.php
@@ -29,7 +29,9 @@ class CheckException extends \Exception {
 	 * @param int $code the HTTP status code
 	 */
 	public function __construct($msg, $code = 0) {
-		Util::writeLog('gallery', 'Exception: ' . $msg . ' (' . $code . ')', Util::ERROR);
+		if($msg !== 'Share is a write-only share') {
+			Util::writeLog('gallery', 'Exception: ' . $msg . ' (' . $code . ')', Util::ERROR);
+		}
 		parent::__construct($msg, $code);
 	}
 
diff --git a/middleware/envcheckmiddleware.php b/middleware/envcheckmiddleware.php
index 8364e525..7d09661f 100644
--- a/middleware/envcheckmiddleware.php
+++ b/middleware/envcheckmiddleware.php
@@ -21,6 +21,7 @@ use OCP\IURLGenerator;
 use OCP\ISession;
 use OCP\ILogger;
 use OCP\Share;
+use OCP\Share\IManager;
 use OCP\Security\IHasher;
 
 use OCP\AppFramework\Http;
@@ -46,6 +47,8 @@ class EnvCheckMiddleware extends CheckMiddleware {
 	private $environment;
 	/** @var IControllerMethodReflector */
 	protected $reflector;
+	/** @var IManager */
+	protected $shareManager;
 
 	/***
 	 * Constructor
@@ -58,6 +61,7 @@ class EnvCheckMiddleware extends CheckMiddleware {
 	 * @param IControllerMethodReflector $reflector
 	 * @param IURLGenerator $urlGenerator
 	 * @param ILogger $logger
+	 * @param IManager $shareManager
 	 */
 	public function __construct(
 		$appName,
@@ -67,7 +71,8 @@ class EnvCheckMiddleware extends CheckMiddleware {
 		Environment $environment,
 		IControllerMethodReflector $reflector,
 		IURLGenerator $urlGenerator,
-		ILogger $logger
+		ILogger $logger,
+		IManager $shareManager
 	) {
 		parent::__construct(
 			$appName,
@@ -80,6 +85,7 @@ class EnvCheckMiddleware extends CheckMiddleware {
 		$this->session = $session;
 		$this->environment = $environment;
 		$this->reflector = $reflector;
+		$this->shareManager = $shareManager;
 	}
 
 	/**
@@ -121,6 +127,16 @@ class EnvCheckMiddleware extends CheckMiddleware {
 				"Can't access a public resource without a token", Http::STATUS_NOT_FOUND
 			);
 		} else {
+
+			try {
+				$share = $this->shareManager->getShareByToken($token);
+				if (!($share->getPermissions() & \OCP\Constants::PERMISSION_READ)) {
+					throw new CheckException(
+						"Share is a write-only share", Http::STATUS_FORBIDDEN
+					);
+				}
+			} catch(\OCP\Share\Exceptions\ShareNotFound $e) {}
+
 			$linkItem = $this->getLinkItem($token);
 			$password = $this->request->getParam('password');
 			// Let's see if the user needs to provide a password
diff --git a/service/serviceexception.php b/service/serviceexception.php
index 10cdf0a0..cb446849 100644
--- a/service/serviceexception.php
+++ b/service/serviceexception.php
@@ -25,7 +25,9 @@ class ServiceException extends \Exception {
 	 * @param string $msg the message contained in the exception
 	 */
 	public function __construct($msg) {
-		Util::writeLog('gallery', 'Exception: ' . $msg, Util::ERROR);
+		if($msg !== 'Share is a write-only share') {
+			Util::writeLog('gallery', 'Exception: ' . $msg, Util::ERROR);
+		}
 		parent::__construct($msg);
 	}
 }
diff --git a/tests/unit/middleware/EnvCheckMiddlewareTest.php b/tests/unit/middleware/EnvCheckMiddlewareTest.php
index 5e14193d..a589b5e0 100644
--- a/tests/unit/middleware/EnvCheckMiddlewareTest.php
+++ b/tests/unit/middleware/EnvCheckMiddlewareTest.php
@@ -58,6 +58,8 @@ class EnvCheckMiddlewareTest extends \Codeception\TestCase\Test {
 	protected $logger;
 	/** @var Controller */
 	private $controller;
+	/** @var \OCP\Share\IManager */
+	private $shareManager;
 	/** @var SharingCheckMiddleware */
 	private $middleware;
 
@@ -95,6 +97,9 @@ class EnvCheckMiddlewareTest extends \Codeception\TestCase\Test {
 		$this->controller = $this->getMockBuilder('OCP\AppFramework\Controller')
 								 ->disableOriginalConstructor()
 								 ->getMock();
+		$this->shareManager = $this->getMockBuilder('OCP\Share\IManager')
+			->disableOriginalConstructor()
+			->getMock();
 
 		$this->middleware = new EnvCheckMiddleware(
 			$this->appName,
@@ -104,7 +109,8 @@ class EnvCheckMiddlewareTest extends \Codeception\TestCase\Test {
 			$this->environment,
 			$this->reflector,
 			$this->urlGenerator,
-			$this->logger
+			$this->logger,
+			$this->shareManager
 		);
 
 		/**
@@ -147,8 +153,41 @@ class EnvCheckMiddlewareTest extends \Codeception\TestCase\Test {
 	 * @PublicPage
 	 *
 	 * @expectedException \OCA\Gallery\Middleware\CheckException
+	 * @expectedExceptionMessage Share is a write-only share
+	 */
+	public function testBeforeControllerWithPublicNotationAndReadOnlyToken() {
+		$share = $this->getMock('\OCP\Share\IShare');
+		$share->expects($this->once())
+			->method('getPermissions')
+			->willReturn(\OCP\Constants::PERMISSION_CREATE);
+		$this->shareManager
+			->expects($this->once())
+			->method('getShareByToken')
+			->willReturn($share);
+
+		$this->reflector->reflect(__CLASS__, __FUNCTION__);
+
+		$token = 'aaaabbbbccccdddd';
+		$this->mockGetTokenParam($token);
+
+		$this->middleware->beforeController(__CLASS__, __FUNCTION__);
+	}
+
+	/**
+	 * @PublicPage
+	 *
+	 * @expectedException \OCA\Gallery\Middleware\CheckException
 	 */
 	public function testBeforeControllerWithPublicNotationAndInvalidToken() {
+		$share = $this->getMock('\OCP\Share\IShare');
+		$share->expects($this->once())
+			->method('getPermissions')
+			->willReturn(\OCP\Constants::PERMISSION_READ);
+		$this->shareManager
+			->expects($this->once())
+			->method('getShareByToken')
+			->willReturn($share);
+
 		$this->reflector->reflect(__CLASS__, __FUNCTION__);
 
 		$token = 'aaaabbbbccccdddd';
@@ -163,6 +202,15 @@ class EnvCheckMiddlewareTest extends \Codeception\TestCase\Test {
 	 * Because the method tested is static, we need to load our test environment \Helper\DataSetup
 	 */
 	public function testBeforeControllerWithPublicNotationAndToken() {
+		$share = $this->getMock('\OCP\Share\IShare');
+		$share->expects($this->once())
+			->method('getPermissions')
+			->willReturn(\OCP\Constants::PERMISSION_READ);
+		$this->shareManager
+			->expects($this->once())
+			->method('getShareByToken')
+			->willReturn($share);
+
 		$this->reflector->reflect(__CLASS__, __FUNCTION__);
 
 		$this->mockGetTokenAndPasswordParams(