sharingcheckmiddleware now handles externalshares as well

Added new annotations for the externalsharescontroller class * @NoOutgoingFederatedSharingRequired * @NoIncomingFederatedSharingRequired By default both are required for all functions in the externalSharesController. A proper exception is thrown and then a 405 is returned instead of the default error page. Since it is only an API endpoint this makes more sense. Unit tests added and updated
2015-10-02 09:57:33 +02:00 · 2015-10-02 09:57:33 +02:00 · 3d2acb5003
parent dc38e674a5
commit 3d2acb5003
8 changed files with 196 additions and 67 deletions
--- a/apps/files_sharing/appinfo/app.php
+++ b/apps/files_sharing/appinfo/app.php
@ -39,9 +39,6 @@ $l = \OC::$server->getL10N('files_sharing');
 \OC::$CLASSPATH['OCA\Files\Share\Maintainer'] = 'files_sharing/lib/maintainer.php';
 \OC::$CLASSPATH['OCA\Files\Share\Proxy'] = 'files_sharing/lib/proxy.php';

-// Exceptions
-\OC::$CLASSPATH['OCA\Files_Sharing\Exceptions\BrokenPath'] = 'files_sharing/lib/exceptions.php';
-
 $application = new Application();
 $application->registerMountProviders();
 $application->setupPropagation();
--- a/apps/files_sharing/appinfo/application.php
+++ b/apps/files_sharing/appinfo/application.php
@ -61,7 +61,6 @@ class Application extends App {
 			return new ExternalSharesController(
 				$c->query('AppName'),
 				$c->query('Request'),
-				$c->query('IsIncomingShareEnabled'),
 				$c->query('ExternalManager'),
 				$c->query('HttpClientService')
 			);
@ -82,9 +81,6 @@ class Application extends App {
 		$container->registerService('HttpClientService', function (SimpleContainer $c) use ($server) {
 			return $server->getHTTPClientService();
 		});
-		$container->registerService('IsIncomingShareEnabled', function (SimpleContainer $c) {
-			return Helper::isIncomingServer2serverShareEnabled();
-		});
 		$container->registerService('ExternalManager', function (SimpleContainer $c) use ($server) {
 			$user = $server->getUserSession()->getUser();
 			$uid = $user ? $user->getUID() : null;
@ -105,7 +101,8 @@ class Application extends App {
 			return new SharingCheckMiddleware(
 				$c->query('AppName'),
 				$server->getConfig(),
-				$server->getAppManager()
+				$server->getAppManager(),
+				$c['ControllerMethodReflector']
 			);
 		});

--- a/apps/files_sharing/lib/controllers/externalsharescontroller.php
+++ b/apps/files_sharing/lib/controllers/externalsharescontroller.php
@ -36,8 +36,6 @@ use OCP\AppFramework\Http\DataResponse;
 */
 class ExternalSharesController extends Controller {

-	/** @var bool */
-	private $incomingShareEnabled;
 	/** @var \OCA\Files_Sharing\External\Manager */
 	private $externalManager;
 	/** @var IClientService */
@ -52,53 +50,44 @@ class ExternalSharesController extends Controller {
 	 */
 	public function __construct($appName,
 								IRequest $request,
-								$incomingShareEnabled,
 								\OCA\Files_Sharing\External\Manager $externalManager,
 								IClientService $clientService) {
 		parent::__construct($appName, $request);
-		$this->incomingShareEnabled = $incomingShareEnabled;
 		$this->externalManager = $externalManager;
 		$this->clientService = $clientService;
 	}

 	/**
 	 * @NoAdminRequired
+	 * @NoOutgoingFederatedSharingRequired
 	 *
 	 * @return JSONResponse
 	 */
 	public function index() {
-		$shares = [];
-		if ($this->incomingShareEnabled) {
-			$shares = $this->externalManager->getOpenShares();
-		}
-		return new JSONResponse($shares);
+		return new JSONResponse($this->externalManager->getOpenShares());
 	}

 	/**
 	 * @NoAdminRequired
+	 * @NoOutgoingFederatedSharingRequired
 	 *
 	 * @param int $id
 	 * @return JSONResponse
 	 */
 	public function create($id) {
-		if ($this->incomingShareEnabled) {
-			$this->externalManager->acceptShare($id);
-		}
-
+		$this->externalManager->acceptShare($id);
 		return new JSONResponse();
 	}

 	/**
 	 * @NoAdminRequired
+	 * @NoOutgoingFederatedSharingRequired
 	 *
 	 * @param $id
 	 * @return JSONResponse
 	 */
 	public function destroy($id) {
-		if ($this->incomingShareEnabled) {
-			$this->externalManager->declineShare($id);
-		}
-
+		$this->externalManager->declineShare($id);
 		return new JSONResponse();
 	}

@ -127,6 +116,8 @@ class ExternalSharesController extends Controller {

 	/**
 	 * @PublicPage
+	 * @NoOutgoingFederatedSharingRequired
+	 * @NoIncomingFederatedSharingRequired
 	 *
 	 * @param string $remote
 	 * @return DataResponse
--- a/apps/files_sharing/lib/exceptions/brokenpath.php
+++ b/apps/files_sharing/lib/exceptions/brokenpath.php
--- a/apps/files_sharing/lib/exceptions/s2sexception.php
+++ b/apps/files_sharing/lib/exceptions/s2sexception.php
@ -0,0 +1,26 @@
+<?php
+/**
+ * @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 OCA\Files_Sharing\Exceptions;
+
+/**
+ * S2S sharing not allowed
+ */
+class S2SException extends \Exception {
+}
--- a/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php
+++ b/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php
@ -29,6 +29,9 @@ use OCP\AppFramework\Middleware;
 use OCP\AppFramework\Http\TemplateResponse;
 use OCP\Files\NotFoundException;
 use OCP\IConfig;
+use OCP\AppFramework\Utility\IControllerMethodReflector;
+use OCA\Files_Sharing\Exceptions\S2SException;
+use OCP\AppFramework\Http\JSONResponse;

 /**
 * Checks whether the "sharing check" is enabled
@ -43,6 +46,8 @@ class SharingCheckMiddleware extends Middleware {
 	protected $config;
 	/** @var IAppManager */
 	protected $appManager;
+	/** @var IControllerMethodReflector */
+	protected $reflector;

 	/***
 	 * @param string $appName
@ -51,10 +56,13 @@ class SharingCheckMiddleware extends Middleware {
 	 */
 	public function __construct($appName,
 								IConfig $config,
-								IAppManager $appManager) {
+								IAppManager $appManager,
+								IControllerMethodReflector $reflector
+								) {
 		$this->appName = $appName;
 		$this->config = $config;
 		$this->appManager = $appManager;
+		$this->reflector = $reflector;
 	}

 	/**
@ -69,8 +77,9 @@ class SharingCheckMiddleware extends Middleware {
 			throw new NotFoundException('Sharing is disabled.');
 		}

-		if ($controller instanceof \OCA\Files_Sharing\Controllers\ExternalSharesController) {
-			//TODO: Proper checks
+		if ($controller instanceof \OCA\Files_Sharing\Controllers\ExternalSharesController &&
+			!$this->externalSharesChecks()) {
+			throw new S2SException('Federated sharing not allowed');
 		} else if ($controller instanceof \OCA\Files_Sharing\Controllers\ShareController &&
 			!$this->isLinkSharingEnabled()) {
 			throw new NotFoundException('Link sharing is disabled');
@ -91,9 +100,32 @@ class SharingCheckMiddleware extends Middleware {
 			return new NotFoundResponse();
 		}

+		if (is_a($exception, '\OCA\Files_Sharing\Exceptions\S2SException')) {
+			return new JSONResponse($exception->getMessage(), 405);
+		}
+
 		throw $exception;
 	}

+	/**
+	 * Checks for externalshares controller
+	 * @return bool
+	 */
+	private function externalSharesChecks() {
+
+		if (!$this->reflector->hasAnnotation('NoIncomingFederatedSharingRequired') &&
+			$this->config->getAppValue('files_sharing', 'incoming_server2server_share_enabled', 'yes') !== 'yes') {
+			return false;
+		}
+
+		if (!$this->reflector->hasAnnotation('NoOutgoingFederatedSharingRequired') &&
+		    $this->config->getAppValue('files_sharing', 'outgoing_server2server_share_enabled', 'yes') !== 'yes') {
+			return false;
+		}
+
+		return true;
+	}
+
 	/**
 	 * Check whether sharing is enabled
 	 * @return bool
--- a/apps/files_sharing/tests/controller/externalsharecontroller.php
+++ b/apps/files_sharing/tests/controller/externalsharecontroller.php
@ -32,8 +32,6 @@ use OCP\IRequest;
 * @package OCA\Files_Sharing\Controllers
 */
 class ExternalShareControllerTest extends \Test\TestCase {
-	/** @var bool */
-	private $incomingShareEnabled;
 	/** @var IRequest */
 	private $request;
 	/** @var \OCA\Files_Sharing\External\Manager */
@ -57,22 +55,12 @@ class ExternalShareControllerTest extends \Test\TestCase {
 		return new ExternalSharesController(
 			'files_sharing',
 			$this->request,
-			$this->incomingShareEnabled,
 			$this->externalManager,
 			$this->clientService
 		);
 	}

-	public function testIndexDisabled() {
-		$this->externalManager
-			->expects($this->never())
-			->method('getOpenShares');
-
-		$this->assertEquals(new JSONResponse(), $this->getExternalShareController()->index());
-	}
-
-	public function testIndexEnabled() {
-		$this->incomingShareEnabled = true;
+	public function testIndex() {
 		$this->externalManager
 			->expects($this->once())
 			->method('getOpenShares')
@ -81,16 +69,7 @@ class ExternalShareControllerTest extends \Test\TestCase {
 		$this->assertEquals(new JSONResponse(['MyDummyArray']), $this->getExternalShareController()->index());
 	}

-	public function testCreateDisabled() {
-		$this->externalManager
-			->expects($this->never())
-			->method('acceptShare');
-
-		$this->assertEquals(new JSONResponse(), $this->getExternalShareController()->create(4));
-	}
-
-	public function testCreateEnabled() {
-		$this->incomingShareEnabled = true;
+	public function testCreate() {
 		$this->externalManager
 			->expects($this->once())
 			->method('acceptShare')
@ -99,16 +78,7 @@ class ExternalShareControllerTest extends \Test\TestCase {
 		$this->assertEquals(new JSONResponse(), $this->getExternalShareController()->create(4));
 	}

-	public function testDestroyDisabled() {
-		$this->externalManager
-			->expects($this->never())
-			->method('destroy');
-
-		$this->assertEquals(new JSONResponse(), $this->getExternalShareController()->destroy(4));
-	}
-
-	public function testDestroyEnabled() {
-		$this->incomingShareEnabled = true;
+	public function testDestroy() {
 		$this->externalManager
 			->expects($this->once())
 			->method('declineShare')
--- a/apps/files_sharing/tests/middleware/sharingcheckmiddleware.php
+++ b/apps/files_sharing/tests/middleware/sharingcheckmiddleware.php
@ -25,6 +25,9 @@
 namespace OCA\Files_Sharing\Middleware;
 use OCP\AppFramework\Http\NotFoundResponse;
 use OCP\Files\NotFoundException;
+use OCP\AppFramework\Utility\IControllerMethodReflector;
+use OCA\Files_Sharing\Exceptions\S2SException;
+use OCP\AppFramework\Http\JSONResponse;

 /**
 * @package OCA\Files_Sharing\Middleware\SharingCheckMiddleware
@ -39,6 +42,8 @@ class SharingCheckMiddlewareTest extends \Test\TestCase {
 	private $sharingCheckMiddleware;
 	/** @var \OCP\AppFramework\Controller */
 	private $controllerMock;
+	/** @var IControllerMethodReflector */
+	private $reflector;

 	protected function setUp() {
 		$this->config = $this->getMockBuilder('\OCP\IConfig')
@ -47,8 +52,14 @@ class SharingCheckMiddlewareTest extends \Test\TestCase {
 			->disableOriginalConstructor()->getMock();
 		$this->controllerMock = $this->getMockBuilder('\OCP\AppFramework\Controller')
 			->disableOriginalConstructor()->getMock();
+		$this->reflector = $this->getMockBuilder('\OCP\AppFramework\Utility\IControllerMethodReflector')
+			->disableOriginalConstructor()->getMock();

-		$this->sharingCheckMiddleware = new SharingCheckMiddleware('files_sharing', $this->config, $this->appManager);
+		$this->sharingCheckMiddleware = new SharingCheckMiddleware(
+			'files_sharing',
+			$this->config,
+			$this->appManager,
+			$this->reflector);
 	}

 	public function testIsSharingEnabledWithAppEnabled() {
@ -114,7 +125,93 @@ class SharingCheckMiddlewareTest extends \Test\TestCase {
 		$this->assertFalse(self::invokePrivate($this->sharingCheckMiddleware, 'isLinkSharingEnabled'));
 	}

-	public function testBeforeControllerWithSharingEnabled() {
+	public function externalSharesChecksDataProvider() {
+
+		$data = [];
+
+		foreach ([false, true] as $annIn) {
+			foreach ([false, true] as $annOut) {
+				foreach ([false, true] as $confIn) {
+					foreach ([false, true] as $confOut) {
+
+						$res = true;
+						if (!$annIn && !$confIn) {
+							$res = false;
+						} elseif (!$annOut && !$confOut) {
+							$res = false;
+						}
+
+						$d = [
+							[
+								['NoIncomingFederatedSharingRequired', $annIn],
+								['NoOutgoingFederatedSharingRequired', $annOut],
+							],
+							[
+								['files_sharing', 'incoming_server2server_share_enabled', 'yes', $confIn ? 'yes' : 'no'],
+								['files_sharing', 'outgoing_server2server_share_enabled', 'yes', $confOut ? 'yes' : 'no'],
+							],
+							$res
+						];
+
+						$data[] = $d;
+					}
+				}
+			}
+		}
+
+		return $data;
+	}
+
+	/**
+	 * @dataProvider externalSharesChecksDataProvider
+	 */
+	public function testExternalSharesChecks($annotations, $config, $expectedResult) {
+		$this->reflector
+			->expects($this->atLeastOnce())
+			->method('hasAnnotation')
+			->will($this->returnValueMap($annotations));
+
+		$this->config
+			->method('getAppValue')
+			->will($this->returnValueMap($config));
+
+		$this->assertEquals($expectedResult, self::invokePrivate($this->sharingCheckMiddleware, 'externalSharesChecks'));
+	}
+
+	/**
+	 * @dataProvider externalSharesChecksDataProvider
+	 */
+	public function testBeforeControllerWithExternalShareControllerWithSharingEnabled($annotations, $config, $noException) {
+		$this->appManager
+			->expects($this->once())
+			->method('isEnabledForUser')
+			->with('files_sharing')
+			->will($this->returnValue(true));
+
+		$this->reflector
+			->expects($this->atLeastOnce())
+			->method('hasAnnotation')
+			->will($this->returnValueMap($annotations));
+
+		$this->config
+			->method('getAppValue')
+			->will($this->returnValueMap($config));
+
+		$controller = $this->getMockBuilder('\OCA\Files_Sharing\Controllers\ExternalSharesController')
+			->disableOriginalConstructor()->getMock();
+
+		$exceptionThrown = false;
+
+		try {
+			$this->sharingCheckMiddleware->beforeController($controller, 'myMethod');
+		} catch (\OCA\Files_Sharing\Exceptions\S2SException $exception) {
+			$exceptionThrown = true;
+		}
+
+		$this->assertNotEquals($noException, $exceptionThrown);
+	}
+
+	public function testBeforeControllerWithShareControllerWithSharingEnabled() {
 		$this->appManager
 			->expects($this->once())
 			->method('isEnabledForUser')
@ -139,6 +236,23 @@ class SharingCheckMiddlewareTest extends \Test\TestCase {
 		$this->sharingCheckMiddleware->beforeController($controller, 'myMethod');
 	}

+	/**
+	 * @expectedException \OCP\Files\NotFoundException
+	 * @expectedExceptionMessage Link sharing is disabled
+	 */
+	public function testBeforeControllerWithShareControllerWithSharingEnabledAPIDisabled() {
+		$this->appManager
+			->expects($this->once())
+			->method('isEnabledForUser')
+			->with('files_sharing')
+			->will($this->returnValue(true));
+
+		$controller = $this->getMockBuilder('\OCA\Files_Sharing\Controllers\ShareController')
+			->disableOriginalConstructor()->getMock();
+
+		$this->sharingCheckMiddleware->beforeController($controller, 'myMethod');
+	}
+
 	/**
 	 * @expectedException \OCP\Files\NotFoundException
 	 * @expectedExceptionMessage Sharing is disabled.
@ -150,10 +264,7 @@ class SharingCheckMiddlewareTest extends \Test\TestCase {
 			->with('files_sharing')
 			->will($this->returnValue(false));

-		$controller = $this->getMockBuilder('\OCA\Files_Sharing\Controllers\ShareController')
-			->disableOriginalConstructor()->getMock();
-
-		$this->sharingCheckMiddleware->beforeController($controller, 'myMethod');
+		$this->sharingCheckMiddleware->beforeController($this->controllerMock, 'myMethod');
 	}

 	/**
@ -168,4 +279,9 @@ class SharingCheckMiddlewareTest extends \Test\TestCase {
 		$this->assertEquals(new NotFoundResponse(), $this->sharingCheckMiddleware->afterException($this->controllerMock, 'myMethod', new NotFoundException('My Exception message')));
 	}

+	public function testAfterExceptionWithS2SException() {
+		$this->assertEquals(new JSONResponse('My Exception message', 405), $this->sharingCheckMiddleware->afterException($this->controllerMock, 'myMethod', new S2SException('My Exception message')));
+	}
+
+
 }