From dc38e674a5d547e7fd53d66fb0ac0dbb5490ea77 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 30 Sep 2015 21:35:52 +0200 Subject: [PATCH 1/2] 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 +++++ .../middleware/sharingcheckmiddleware.php | 59 +++++++++---------- 2 files changed, 44 insertions(+), 30 deletions(-) diff --git a/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php b/apps/files_sharing/lib/middleware/sharingcheckmiddleware.php index 61dfd914d0..f51399b76a 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; diff --git a/apps/files_sharing/tests/middleware/sharingcheckmiddleware.php b/apps/files_sharing/tests/middleware/sharingcheckmiddleware.php index 3171d45d33..a43b11c81b 100644 --- a/apps/files_sharing/tests/middleware/sharingcheckmiddleware.php +++ b/apps/files_sharing/tests/middleware/sharingcheckmiddleware.php @@ -51,25 +51,13 @@ class SharingCheckMiddlewareTest extends \Test\TestCase { $this->sharingCheckMiddleware = new SharingCheckMiddleware('files_sharing', $this->config, $this->appManager); } - public function testIsSharingEnabledWithEverythingEnabled() { + public function testIsSharingEnabledWithAppEnabled() { $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->assertTrue(self::invokePrivate($this->sharingCheckMiddleware, 'isSharingEnabled')); } @@ -83,13 +71,24 @@ class SharingCheckMiddlewareTest extends \Test\TestCase { $this->assertFalse(self::invokePrivate($this->sharingCheckMiddleware, 'isSharingEnabled')); } - public function testIsSharingEnabledWithLinkSharingDisabled() { - $this->appManager - ->expects($this->once()) - ->method('isEnabledForUser') - ->with('files_sharing') - ->will($this->returnValue(true)); + public function testIsLinkSharingEnabledWithEverythinEnabled() { + $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->assertTrue(self::invokePrivate($this->sharingCheckMiddleware, 'isLinkSharingEnabled')); + } + + + public function testIsLinkSharingEnabledWithLinkSharingDisabled() { $this->config ->expects($this->at(0)) ->method('getAppValue') @@ -102,23 +101,17 @@ class SharingCheckMiddlewareTest extends \Test\TestCase { ->with('core', 'shareapi_allow_links', 'yes') ->will($this->returnValue('no')); - $this->assertFalse(self::invokePrivate($this->sharingCheckMiddleware, 'isSharingEnabled')); + $this->assertFalse(self::invokePrivate($this->sharingCheckMiddleware, 'isLinkSharingEnabled')); } - public function testIsSharingEnabledWithSharingAPIDisabled() { - $this->appManager - ->expects($this->once()) - ->method('isEnabledForUser') - ->with('files_sharing') - ->will($this->returnValue(true)); - + public function testIsLinkSharingEnabledWithSharingAPIDisabled() { $this->config ->expects($this->once()) ->method('getAppValue') ->with('core', 'shareapi_enabled', 'yes') ->will($this->returnValue('no')); - $this->assertFalse(self::invokePrivate($this->sharingCheckMiddleware, 'isSharingEnabled')); + $this->assertFalse(self::invokePrivate($this->sharingCheckMiddleware, 'isLinkSharingEnabled')); } public function testBeforeControllerWithSharingEnabled() { @@ -140,7 +133,10 @@ class SharingCheckMiddlewareTest extends \Test\TestCase { ->with('core', 'shareapi_allow_links', 'yes') ->will($this->returnValue('yes')); - $this->sharingCheckMiddleware->beforeController($this->controllerMock, 'myMethod'); + $controller = $this->getMockBuilder('\OCA\Files_Sharing\Controllers\ShareController') + ->disableOriginalConstructor()->getMock(); + + $this->sharingCheckMiddleware->beforeController($controller, 'myMethod'); } /** @@ -154,7 +150,10 @@ class SharingCheckMiddlewareTest extends \Test\TestCase { ->with('files_sharing') ->will($this->returnValue(false)); - $this->sharingCheckMiddleware->beforeController($this->controllerMock, 'myMethod'); + $controller = $this->getMockBuilder('\OCA\Files_Sharing\Controllers\ShareController') + ->disableOriginalConstructor()->getMock(); + + $this->sharingCheckMiddleware->beforeController($controller, 'myMethod'); } /** From 3d2acb5003a4953d3f422b34f670d87c4afb11c9 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 2 Oct 2015 09:57:33 +0200 Subject: [PATCH 2/2] 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 +- .../controllers/externalsharescontroller.php | 25 ++-- .../brokenpath.php} | 0 .../lib/exceptions/s2sexception.php | 26 ++++ .../lib/middleware/sharingcheckmiddleware.php | 38 +++++- .../controller/externalsharecontroller.php | 36 +---- .../middleware/sharingcheckmiddleware.php | 128 +++++++++++++++++- 8 files changed, 196 insertions(+), 67 deletions(-) rename apps/files_sharing/lib/{exceptions.php => exceptions/brokenpath.php} (100%) create mode 100644 apps/files_sharing/lib/exceptions/s2sexception.php diff --git a/apps/files_sharing/appinfo/app.php b/apps/files_sharing/appinfo/app.php index 15c0b864b0..e449843de7 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 d0dcadb77e..ad5d5d6323 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 6eb9d3c13d..1b8351da42 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/brokenpath.php similarity index 100% rename from apps/files_sharing/lib/exceptions.php rename to apps/files_sharing/lib/exceptions/brokenpath.php diff --git a/apps/files_sharing/lib/exceptions/s2sexception.php b/apps/files_sharing/lib/exceptions/s2sexception.php new file mode 100644 index 0000000000..7f5557f8cd --- /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 f51399b76a..942efc0483 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 3dc34bca7a..7b3e1af4c7 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 a43b11c81b..05cb749768 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') @@ -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'))); + } + + }