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

diff --git a/lib/private/CapabilitiesManager.php b/lib/private/CapabilitiesManager.php
index 99a37d652a1139c862e6dc7ee00b8ca226681e73..159fa97c70810da853c5b8f86ef64cadfbea4e3a 100644 (file)
--- 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 d5808d7f17cafec723c30864c05d65e62ddc7e3a..03c3633c9f209a5f44c84b849a74c9e105a96ada 100644 (file)
--- 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 d4dd52d07f1b9e05f31bad450b58036c9359139d..75fbdb8d89fdcdd8fc9652ec5efc1e88940e7877 100644 (file)
--- 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 {
                ];
        }
 }
-