From c044aa34fa06ecef90dde2f6793e5e4d77f5200b Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 15 Aug 2016 20:22:48 +0200 Subject: [PATCH] Make the capabilities manager more error proof If an app registers an invalid capabilty we should not crash hard. Instead we should catch the exception. Log it (error) and carry on. * Added tests --- lib/private/CapabilitiesManager.php | 21 ++++++-- lib/private/Server.php | 2 +- tests/lib/CapabilitiesManagerTest.php | 71 +++++++++++++++++---------- 3 files changed, 63 insertions(+), 31 deletions(-) diff --git a/lib/private/CapabilitiesManager.php b/lib/private/CapabilitiesManager.php index 99a37d652a1..159fa97c708 100644 --- a/lib/private/CapabilitiesManager.php +++ b/lib/private/CapabilitiesManager.php @@ -22,15 +22,22 @@ namespace OC; +use OCP\AppFramework\QueryException; use OCP\Capabilities\ICapability; +use OCP\ILogger; class CapabilitiesManager { - /** - * @var \Closure[] - */ + /** @var \Closure[] */ private $capabilities = array(); + /** @var ILogger */ + private $logger; + + public function __construct(ILogger $logger) { + $this->logger = $logger; + } + /** * Get an array of al the capabilities that are registered at this manager * @@ -40,7 +47,13 @@ class CapabilitiesManager { public function getCapabilities() { $capabilities = []; foreach($this->capabilities as $capability) { - $c = $capability(); + try { + $c = $capability(); + } catch (QueryException $e) { + $this->logger->error('CapabilitiesManager: {message}', ['app' => 'core', 'message' => $e->getMessage()]); + continue; + } + if ($c instanceof ICapability) { $capabilities = array_replace_recursive($capabilities, $c->getCapabilities()); } else { diff --git a/lib/private/Server.php b/lib/private/Server.php index d5808d7f17c..03c3633c9f2 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -629,7 +629,7 @@ class Server extends ServerContainer implements IServerContainer { return new Manager(); }); $this->registerService('CapabilitiesManager', function (Server $c) { - $manager = new \OC\CapabilitiesManager(); + $manager = new \OC\CapabilitiesManager($c->getLogger()); $manager->registerCapability(function () use ($c) { return new \OC\OCS\CoreCapabilities($c->getConfig()); }); diff --git a/tests/lib/CapabilitiesManagerTest.php b/tests/lib/CapabilitiesManagerTest.php index d4dd52d07f1..75fbdb8d89f 100644 --- a/tests/lib/CapabilitiesManagerTest.php +++ b/tests/lib/CapabilitiesManagerTest.php @@ -21,14 +21,29 @@ namespace Test; +use OC\CapabilitiesManager; +use OCP\AppFramework\QueryException; +use OCP\Capabilities\ICapability; +use OCP\ILogger; + class CapabilitiesManagerTest extends TestCase { + /** @var CapabilitiesManager */ + private $manager; + + /** @var ILogger */ + private $logger; + + public function setUp() { + $this->logger = $this->getMockBuilder('OCP\ILogger')->getMock(); + $this->manager = new CapabilitiesManager($this->logger); + } + /** * Test no capabilities */ public function testNoCapabilities() { - $manager = new \OC\CapabilitiesManager(); - $res = $manager->getCapabilities(); + $res = $this->manager->getCapabilities(); $this->assertEmpty($res); } @@ -36,13 +51,11 @@ class CapabilitiesManagerTest extends TestCase { * Test a valid capabilitie */ public function testValidCapability() { - $manager = new \OC\CapabilitiesManager(); - - $manager->registerCapability(function() { + $this->manager->registerCapability(function() { return new SimpleCapability(); }); - $res = $manager->getCapabilities(); + $res = $this->manager->getCapabilities(); $this->assertEquals(['foo' => 1], $res); } @@ -52,13 +65,11 @@ class CapabilitiesManagerTest extends TestCase { * @expectedExceptionMessage The given Capability (Test\NoCapability) does not implement the ICapability interface */ public function testNoICapability() { - $manager = new \OC\CapabilitiesManager(); - - $manager->registerCapability(function() { + $this->manager->registerCapability(function() { return new NoCapability(); }); - $res = $manager->getCapabilities(); + $res = $this->manager->getCapabilities(); $this->assertEquals([], $res); } @@ -66,19 +77,17 @@ class CapabilitiesManagerTest extends TestCase { * Test a bunch of merged Capabilities */ public function testMergedCapabilities() { - $manager = new \OC\CapabilitiesManager(); - - $manager->registerCapability(function() { + $this->manager->registerCapability(function() { return new SimpleCapability(); }); - $manager->registerCapability(function() { + $this->manager->registerCapability(function() { return new SimpleCapability2(); }); - $manager->registerCapability(function() { + $this->manager->registerCapability(function() { return new SimpleCapability3(); }); - $res = $manager->getCapabilities(); + $res = $this->manager->getCapabilities(); $expected = [ 'foo' => 1, 'bar' => [ @@ -94,16 +103,14 @@ class CapabilitiesManagerTest extends TestCase { * Test deep identical capabilities */ public function testDeepIdenticalCapabilities() { - $manager = new \OC\CapabilitiesManager(); - - $manager->registerCapability(function() { + $this->manager->registerCapability(function() { return new DeepCapability(); }); - $manager->registerCapability(function() { + $this->manager->registerCapability(function() { return new DeepCapability(); }); - $res = $manager->getCapabilities(); + $res = $this->manager->getCapabilities(); $expected = [ 'foo' => [ 'bar' => [ @@ -114,9 +121,22 @@ class CapabilitiesManagerTest extends TestCase { $this->assertEquals($expected, $res); } + + public function testInvalidCapability() { + $this->manager->registerCapability(function () { + throw new QueryException(); + }); + + $this->logger->expects($this->once()) + ->method('error'); + + $res = $this->manager->getCapabilities(); + + $this->assertEquals([], $res); + } } -class SimpleCapability implements \OCP\Capabilities\ICapability { +class SimpleCapability implements ICapability { public function getCapabilities() { return [ 'foo' => 1 @@ -124,7 +144,7 @@ class SimpleCapability implements \OCP\Capabilities\ICapability { } } -class SimpleCapability2 implements \OCP\Capabilities\ICapability { +class SimpleCapability2 implements ICapability { public function getCapabilities() { return [ 'bar' => ['x' => 1] @@ -132,7 +152,7 @@ class SimpleCapability2 implements \OCP\Capabilities\ICapability { } } -class SimpleCapability3 implements \OCP\Capabilities\ICapability { +class SimpleCapability3 implements ICapability { public function getCapabilities() { return [ 'bar' => ['y' => 2] @@ -148,7 +168,7 @@ class NoCapability { } } -class DeepCapability implements \OCP\Capabilities\ICapability { +class DeepCapability implements ICapability { public function getCapabilities() { return [ 'foo' => [ @@ -159,4 +179,3 @@ class DeepCapability implements \OCP\Capabilities\ICapability { ]; } } - -- 2.39.5