From c044aa34fa06ecef90dde2f6793e5e4d77f5200b Mon Sep 17 00:00:00 2001
From: Roeland Jago Douma <roeland@famdouma.nl>
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 99a37d652a..159fa97c70 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 d5808d7f17..03c3633c9f 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 d4dd52d07f..75fbdb8d89 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 {
 		];
 	}
 }
-