Merge pull request #21693 from nextcloud/fix/noid/import-certificates-only-by-system

Improve CertificateManager to not be user context dependent

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() {