From 3d2acb5003a4953d3f422b34f670d87c4afb11c9 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 2 Oct 2015 09:57:33 +0200 Subject: [PATCH] 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'))); + } + + }