Improve CertificateManager to not be user context dependent

* removes the ability for users to import their own certificates (for external storage)
* reliably returns the same certificate bundles system wide (and not depending on the user context and available sessions)

The user specific certificates were broken in some cases anyways, as they are only loaded if the specific user is logged in and thus causing unexpected behavior for background jobs and other non-user triggered code paths.

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

3 files changed, 24 insertions, 47 deletions

diff --git a/tests/lib/Http/Client/ClientTest.php b/tests/lib/Http/Client/ClientTest.php
index a462a0848ae..78054725e8c 100644
--- a/tests/lib/Http/Client/ClientTest.php
+++ b/tests/lib/Http/Client/ClientTest.php
@@ -281,7 +281,7 @@ class ClientTest extends \Test\TestCase {
 		$this->certificateManager
 			->expects($this->once())
 			->method('getAbsoluteBundlePath')
-			->with(null)
+			->with()
 			->willReturn('/my/path.crt');
 
 		$this->defaultRequestOptions = [
@@ -493,7 +493,7 @@ class ClientTest extends \Test\TestCase {
 		$this->certificateManager
 			->expects($this->once())
 			->method('getAbsoluteBundlePath')
-			->with(null)
+			->with()
 			->willReturn('/my/path.crt');
 
 		$this->assertEquals([
@@ -529,7 +529,7 @@ class ClientTest extends \Test\TestCase {
 		$this->certificateManager
 			->expects($this->once())
 			->method('getAbsoluteBundlePath')
-			->with(null)
+			->with()
 			->willReturn('/my/path.crt');
 
 		$this->assertEquals([
diff --git a/tests/lib/Security/CertificateManagerTest.php b/tests/lib/Security/CertificateManagerTest.php
index 447ee8719c9..3af4b564612 100644
--- a/tests/lib/Security/CertificateManagerTest.php
+++ b/tests/lib/Security/CertificateManagerTest.php
@@ -53,7 +53,6 @@ class CertificateManagerTest extends \Test\TestCase {
 			->willReturn('random');
 
 		$this->certificateManager = new CertificateManager(
-			$this->username,
 			new \OC\Files\View(),
 			$config,
 			$this->createMock(ILogger::class),
@@ -132,22 +131,18 @@ class CertificateManagerTest extends \Test\TestCase {
 	}
 
 	public function testGetCertificateBundle() {
-		$this->assertSame('/' . $this->username . '/files_external/rootcerts.crt', $this->certificateManager->getCertificateBundle());
+		$this->assertSame('/files_external/rootcerts.crt', $this->certificateManager->getCertificateBundle());
 	}
 
 	/**
 	 * @dataProvider dataTestNeedRebundling
 	 *
-	 * @param string $uid
 	 * @param int $CaBundleMtime
-	 * @param int $systemWideMtime
 	 * @param int $targetBundleMtime
 	 * @param int $targetBundleExists
 	 * @param bool $expected
 	 */
-	public function testNeedRebundling($uid,
-								$CaBundleMtime,
-								$systemWideMtime,
+	public function testNeedRebundling($CaBundleMtime,
 								$targetBundleMtime,
 								$targetBundleExists,
 								$expected
@@ -158,31 +153,24 @@ class CertificateManagerTest extends \Test\TestCase {
 
 		/** @var CertificateManager | \PHPUnit\Framework\MockObject\MockObject $certificateManager */
 		$certificateManager = $this->getMockBuilder('OC\Security\CertificateManager')
-			->setConstructorArgs([$uid, $view, $config, $this->createMock(ILogger::class), $this->random])
+			->setConstructorArgs([$view, $config, $this->createMock(ILogger::class), $this->random])
 			->setMethods(['getFilemtimeOfCaBundle', 'getCertificateBundle'])
 			->getMock();
 
 		$certificateManager->expects($this->any())->method('getFilemtimeOfCaBundle')
 			->willReturn($CaBundleMtime);
 
-		$certificateManager->expects($this->at(1))->method('getCertificateBundle')
-			->with($uid)->willReturn('targetBundlePath');
+		$certificateManager->expects($this->at(0))->method('getCertificateBundle')
+			->willReturn('targetBundlePath');
 
 		$view->expects($this->any())->method('file_exists')
 			->with('targetBundlePath')
 			->willReturn($targetBundleExists);
 
 
-		if ($uid !== null && $targetBundleExists) {
-			$certificateManager->expects($this->at(2))->method('getCertificateBundle')
-				->with(null)->willReturn('SystemBundlePath');
-		}
-
 		$view->expects($this->any())->method('filemtime')
-			->willReturnCallback(function ($path) use ($systemWideMtime, $targetBundleMtime) {
-				if ($path === 'SystemBundlePath') {
-					return $systemWideMtime;
-				} elseif ($path === 'targetBundlePath') {
+			->willReturnCallback(function ($path) use ($targetBundleMtime) {
+				if ($path === 'targetBundlePath') {
 					return $targetBundleMtime;
 				}
 				throw new \Exception('unexpected path');
@@ -190,37 +178,26 @@ class CertificateManagerTest extends \Test\TestCase {
 
 
 		$this->assertSame($expected,
-			$this->invokePrivate($certificateManager, 'needsRebundling', [$uid])
+			$this->invokePrivate($certificateManager, 'needsRebundling')
 		);
 	}
 
 	public function dataTestNeedRebundling() {
 		return [
-			//values: uid, CaBundleMtime, systemWideMtime, targetBundleMtime, targetBundleExists, expected
-
-			// compare minimum of CaBundleMtime and systemWideMtime with targetBundleMtime
-			['user1', 10, 20, 30, true, false],
-			['user1', 10, 20, 15, true, true],
-			['user1', 10, 5, 30, true, false],
-			['user1', 10, 5, 8, true, true],
+			//values: CaBundleMtime, targetBundleMtime, targetBundleExists, expected
 
-			// if no user exists we ignore 'systemWideMtime' because this is the bundle we re-build
-			[null, 10, 20, 30, true, false],
-			[null, 10, 20, 15, true, false],
-			[null, 10, 20, 8, true, true],
-			[null, 10, 5, 30, true, false],
-			[null, 10, 5, 8, true, true],
+			[10, 30, true, false],
+			[10, 15, true, false],
+			[10, 8, true, true],
+			[10, 30, true, false],
+			[10, 8, true, true],
 
 			// if no target bundle exists we always build a new one
-			['user1', 10, 20, 30, false, true],
-			['user1', 10, 20, 15, false, true],
-			['user1', 10, 5, 30, false, true],
-			['user1', 10, 5, 8, false, true],
-			[null, 10, 20, 30, false, true],
-			[null, 10, 20, 15, false, true],
-			[null, 10, 20, 8, false, true],
-			[null, 10, 5, 30, false, true],
-			[null, 10, 5, 8, false, true],
+			[10, 30, false, true],
+			[10, 15, false, true],
+			[10, 8, false, true],
+			[10, 30, false, true],
+			[10, 8, false, true],
 		];
 	}
 }
diff --git a/tests/lib/ServerTest.php b/tests/lib/ServerTest.php
index ada25247393..1054c4195b2 100644
--- a/tests/lib/ServerTest.php
+++ b/tests/lib/ServerTest.php
@@ -175,8 +175,8 @@ class ServerTest extends \Test\TestCase {
 	}
 
 	public function testGetCertificateManager() {
-		$this->assertInstanceOf('\OC\Security\CertificateManager', $this->server->getCertificateManager('test'), 'service returned by "getCertificateManager" did not return the right class');
-		$this->assertInstanceOf('\OCP\ICertificateManager', $this->server->getCertificateManager('test'), 'service returned by "getCertificateManager" did not return the right class');
+		$this->assertInstanceOf('\OC\Security\CertificateManager', $this->server->getCertificateManager(), 'service returned by "getCertificateManager" did not return the right class');
+		$this->assertInstanceOf('\OCP\ICertificateManager', $this->server->getCertificateManager(), 'service returned by "getCertificateManager" did not return the right class');
 	}
 
 	public function testCreateEventSource() {