From dc38e674a5d547e7fd53d66fb0ac0dbb5490ea77 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 30 Sep 2015 21:35:52 +0200 Subject: Split files_sharing middelware Since for external shares there is no need for link shares to be enabled we should check which controller is actually being called. This makes sure that in all cases we verify that the files_sharing app is enabled. But only for the share controller (public shares) we check if the API is enabled and if links are enabled. TODO: add checks for federated sharing as well --- .../lib/middleware/sharingcheckmiddleware.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) (limited to 'apps/files_sharing/lib') diff --git a/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php b/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php index 61dfd914d0b..f51399b76a8 100644 --- a/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php +++ b/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php @@ -68,6 +68,13 @@ class SharingCheckMiddleware extends Middleware { if(!$this->isSharingEnabled()) { throw new NotFoundException('Sharing is disabled.'); } + + if ($controller instanceof \OCA\Files_Sharing\Controllers\ExternalSharesController) { + //TODO: Proper checks + } else if ($controller instanceof \OCA\Files_Sharing\Controllers\ShareController && + !$this->isLinkSharingEnabled()) { + throw new NotFoundException('Link sharing is disabled'); + } } /** @@ -98,6 +105,14 @@ class SharingCheckMiddleware extends Middleware { return false; } + return true; + } + + /** + * Check if link sharing is allowed + * @return bool + */ + private function isLinkSharingEnabled() { // Check if the shareAPI is enabled if ($this->config->getAppValue('core', 'shareapi_enabled', 'yes') !== 'yes') { return false; -- cgit v1.2.3 From 3d2acb5003a4953d3f422b34f670d87c4afb11c9 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 2 Oct 2015 09:57:33 +0200 Subject: 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 --- apps/files_sharing/appinfo/app.php | 3 - apps/files_sharing/appinfo/application.php | 7 +- .../lib/controllers/externalsharescontroller.php | 25 ++-- apps/files_sharing/lib/exceptions.php | 32 ------ apps/files_sharing/lib/exceptions/brokenpath.php | 32 ++++++ apps/files_sharing/lib/exceptions/s2sexception.php | 26 +++++ .../lib/middleware/sharingcheckmiddleware.php | 38 ++++++- .../tests/controller/externalsharecontroller.php | 36 +----- .../tests/middleware/sharingcheckmiddleware.php | 126 ++++++++++++++++++++- 9 files changed, 227 insertions(+), 98 deletions(-) delete mode 100644 apps/files_sharing/lib/exceptions.php create mode 100644 apps/files_sharing/lib/exceptions/brokenpath.php create mode 100644 apps/files_sharing/lib/exceptions/s2sexception.php (limited to 'apps/files_sharing/lib') diff --git a/apps/files_sharing/appinfo/app.php b/apps/files_sharing/appinfo/app.php index 15c0b864b08..e449843de73 100644 --- 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(); diff --git a/apps/files_sharing/appinfo/application.php b/apps/files_sharing/appinfo/application.php index d0dcadb77e8..ad5d5d63231 100644 --- 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'] ); }); diff --git a/apps/files_sharing/lib/controllers/externalsharescontroller.php b/apps/files_sharing/lib/controllers/externalsharescontroller.php index 6eb9d3c13d9..1b8351da420 100644 --- 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 diff --git a/apps/files_sharing/lib/exceptions.php b/apps/files_sharing/lib/exceptions.php deleted file mode 100644 index 71fe93f8e92..00000000000 --- a/apps/files_sharing/lib/exceptions.php +++ /dev/null @@ -1,32 +0,0 @@ - - * @author Morris Jobke - * - * @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 - * - */ - -namespace OCA\Files_Sharing\Exceptions; - -/** - * Expected path with a different root - * Possible Error Codes: - * 10 - Path not relative to data/ and point to the users file directory - - */ -class BrokenPath extends \Exception { -} diff --git a/apps/files_sharing/lib/exceptions/brokenpath.php b/apps/files_sharing/lib/exceptions/brokenpath.php new file mode 100644 index 00000000000..71fe93f8e92 --- /dev/null +++ b/apps/files_sharing/lib/exceptions/brokenpath.php @@ -0,0 +1,32 @@ + + * @author Morris Jobke + * + * @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 + * + */ + +namespace OCA\Files_Sharing\Exceptions; + +/** + * Expected path with a different root + * Possible Error Codes: + * 10 - Path not relative to data/ and point to the users file directory + + */ +class BrokenPath extends \Exception { +} diff --git a/apps/files_sharing/lib/exceptions/s2sexception.php b/apps/files_sharing/lib/exceptions/s2sexception.php new file mode 100644 index 00000000000..7f5557f8cd3 --- /dev/null +++ b/apps/files_sharing/lib/exceptions/s2sexception.php @@ -0,0 +1,26 @@ + + * + */ + +namespace OCA\Files_Sharing\Exceptions; + +/** + * S2S sharing not allowed + */ +class S2SException extends \Exception { +} diff --git a/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php b/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php index f51399b76a8..942efc0483e 100644 --- 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 diff --git a/apps/files_sharing/tests/controller/externalsharecontroller.php b/apps/files_sharing/tests/controller/externalsharecontroller.php index 3dc34bca7a4..7b3e1af4c74 100644 --- 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') diff --git a/apps/files_sharing/tests/middleware/sharingcheckmiddleware.php b/apps/files_sharing/tests/middleware/sharingcheckmiddleware.php index a43b11c81b6..05cb7497680 100644 --- 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') @@ -141,14 +238,14 @@ class SharingCheckMiddlewareTest extends \Test\TestCase { /** * @expectedException \OCP\Files\NotFoundException - * @expectedExceptionMessage Sharing is disabled. + * @expectedExceptionMessage Link sharing is disabled */ - public function testBeforeControllerWithSharingDisabled() { + public function testBeforeControllerWithShareControllerWithSharingEnabledAPIDisabled() { $this->appManager ->expects($this->once()) ->method('isEnabledForUser') ->with('files_sharing') - ->will($this->returnValue(false)); + ->will($this->returnValue(true)); $controller = $this->getMockBuilder('\OCA\Files_Sharing\Controllers\ShareController') ->disableOriginalConstructor()->getMock(); @@ -156,6 +253,20 @@ class SharingCheckMiddlewareTest extends \Test\TestCase { $this->sharingCheckMiddleware->beforeController($controller, '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 @@ -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'))); + } + + } -- cgit v1.2.3