Fix exit codes of security:certificates commands

.drone.star

@@ -750,7 +750,8 @@ def litmus():
 
 	default = {
 		'phpVersions': ['7.2', '7.3', '7.4'],
-		'logLevel': '2'
+		'logLevel': '2',
+		'useHttps': True,
 	}
 
 	if 'defaults' in config:
@@ -800,7 +801,7 @@ def litmus():
 					composerInstall(phpVersion) +
 					vendorbinInstall(phpVersion) +
 					yarnInstall(phpVersion) +
-					installServer(phpVersion, db, params['logLevel']) +
+					installServer(phpVersion, db, params['logLevel'], params['useHttps']) +
 					setupLocalStorage(phpVersion) +
 					fixPermissions(phpVersion, False) +
 					createShare(phpVersion) +
@@ -890,7 +891,7 @@ def litmus():
 					],
 				'services':
 					databaseService(db) +
-					owncloudService(phpVersion, 'server', '/drone/src', True),
+					owncloudService(phpVersion, 'server', '/drone/src', params['useHttps']),
 				'depends_on': [],
 				'trigger': {
 					'ref': [
@@ -1461,7 +1462,7 @@ def acceptance():
 										composerInstall(phpVersion) +
 										vendorbinInstall(phpVersion) +
 										yarnInstall(phpVersion) +
-										installServer(phpVersion, db, params['logLevel'], params['federatedServerNeeded'], params['proxyNeeded']) +
+										installServer(phpVersion, db, params['logLevel'], params['useHttps'], params['federatedServerNeeded'], params['proxyNeeded']) +
 										(
 											installFederated(federatedServerVersion, params['federatedPhpVersion'], params['logLevel'], protocol, db, federationDbSuffix) +
 											owncloudLog('federated', 'federated') if params['federatedServerNeeded'] else []
@@ -1752,7 +1753,6 @@ def cephService(cephS3):
 		}
 	}]
 
-
 def owncloudService(phpVersion, name = 'server', path = '/drone/src', ssl = True):
 	if ssl:
 		environment = {
@@ -1972,7 +1972,7 @@ def databaseServiceForFederation(db, suffix):
 		}
 	}]
 
-def installServer(phpVersion, db, logLevel, federatedServerNeeded = False, proxyNeeded = False):
+def installServer(phpVersion, db, logLevel, ssl = False, federatedServerNeeded = False, proxyNeeded = False):
 	return [{
 		'name': 'install-server',
 		'image': 'owncloudci/php:%s' % phpVersion,
@@ -1995,10 +1995,11 @@ def installServer(phpVersion, db, logLevel, federatedServerNeeded = False, proxy
 		] if proxyNeeded else []) + [
 			'php occ log:manage --level %s' % logLevel,
 			'php occ config:list',
-			'php occ security:certificates:import /drone/server.crt',
 		] + ([
+			'php occ security:certificates:import /drone/server.crt',
+		] if ssl else []) + ([
 			'php occ security:certificates:import /drone/federated.crt',
-		] if federatedServerNeeded else []) + [
+		] if federatedServerNeeded and ssl else []) + [
 			'php occ security:certificates',
 		]
 	}]

changelog/unreleased/35364

@@ -0,0 +1,8 @@
+Bugfix: Fix exit codes of security:certificates commands
+
+If there is an error when doing occ security:certificates:import or
+occ security:certificates:remove then the command will exit with status 1.
+This allows the caller to reliably detect if there was a problem.
+
+https://github.com/owncloud/core/issues/35364
+https://github.com/owncloud/core/pull/37783

core/Command/Security/ImportCertificate.php

@@ -53,7 +53,7 @@ protected function execute(InputInterface $input, OutputInterface $output) {
 
 		if (!\file_exists($path)) {
 			$output->writeln('<error>certificate not found</error>');
-			return;
+			return 1;
 		}
 
 		$certData = \file_get_contents($path);

core/Command/Security/RemoveCertificate.php

@@ -52,6 +52,11 @@ protected function configure() {
 	protected function execute(InputInterface $input, OutputInterface $output) {
 		$name = $input->getArgument('name');
 
-		$this->certificateManager->removeCertificate($name);
+		if ($this->certificateManager->removeCertificate($name)) {
+			return 0;
+		}
+
+		$output->writeln('<error>certificate not found</error>');
+		return 1;
 	}
 }

lib/private/Security/CertificateManager.php

@@ -172,6 +172,8 @@ public function removeCertificate($name) {
 		if ($this->view->file_exists($path . $name)) {
 			$this->view->unlink($path . $name);
 			$this->createCertificateBundle();
+		} else {
+			return false;
 		}
 		return true;
 	}

tests/Core/Command/Security/ImportCertificateTest.php

@@ -0,0 +1,80 @@
+<?php
+/**
+ * @author Phil Davis <phil@jankaritech.com>
+ *
+ * @copyright Copyright (c) 2020, ownCloud GmbH
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace Tests\Core\Command\Security;
+
+use OC\Core\Command\Security\ImportCertificate;
+use OC\Files\View;
+use OC\Security\CertificateManager;
+use Symfony\Component\Console\Application;
+use Symfony\Component\Console\Tester\CommandTester;
+use Test\TestCase;
+
+/**
+ * Class ImportCertificateTest
+ *
+ * @group DB
+ */
+class ImportCertificateTest extends TestCase {
+
+	/** @var CommandTester */
+	private $commandTester;
+
+	/** @var CertificateManager */
+	private $certificateManager;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->certificateManager = new CertificateManager(
+			$this->getUniqueID('', 20),
+			new View(),
+			$this->createMock('OCP\IConfig')
+		);
+		$command = new ImportCertificate(
+			$this->certificateManager
+		);
+		$command->setApplication(new Application());
+		$this->commandTester = new CommandTester($command);
+	}
+
+	/**
+	 * @dataProvider inputProvider
+	 * @param array $input
+	 * @param array $answers
+	 * @param int $expectedStatus
+	 * @param string $expectedOutput
+	 */
+	public function testCommandInput($input, $answers, $expectedStatus, $expectedOutput) {
+		$this->commandTester->setInputs($answers);
+		$actualStatus = $this->commandTester->execute($input);
+		$this->assertEquals($expectedStatus, $actualStatus);
+		$output = $this->commandTester->getDisplay();
+		$this->assertStringContainsString($expectedOutput, $output);
+	}
+
+	public function inputProvider() {
+		return [
+			[['path' => 'doesNotExist.crt'], [], 1, 'certificate not found'],
+			[['path' => 'data/certificates/goodCertificate.crt'], [], 0, ''],
+		];
+	}
+}

tests/Core/Command/Security/RemoveCertificateTest.php

@@ -0,0 +1,90 @@
+<?php
+/**
+ * @author Phil Davis <phil@jankaritech.com>
+ *
+ * @copyright Copyright (c) 2020, ownCloud GmbH
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace Tests\Core\Command\Security;
+
+use OC\Core\Command\Security\RemoveCertificate;
+use OC\Files\View;
+use OC\Security\CertificateManager;
+use Symfony\Component\Console\Application;
+use Symfony\Component\Console\Tester\CommandTester;
+use Test\TestCase;
+
+/**
+ * Class RemoveCertificateTest
+ *
+ * @group DB
+ */
+class RemoveCertificateTest extends TestCase {
+
+	/** @var CommandTester */
+	private $commandTester;
+
+	/** @var CertificateManager */
+	private $certificateManager;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->certificateManager = new CertificateManager(
+			$this->getUniqueID('', 20),
+			new View(),
+			$this->createMock('OCP\IConfig')
+		);
+		$command = new RemoveCertificate(
+			$this->certificateManager
+		);
+		$command->setApplication(new Application());
+		$this->commandTester = new CommandTester($command);
+	}
+
+	/**
+	 * @dataProvider inputProvider
+	 * @param array $input
+	 * @param array $answers
+	 * @param int $expectedStatus
+	 * @param string $expectedOutput
+	 */
+	public function testCommandInput($input, $answers, $expectedStatus, $expectedOutput) {
+		$this->commandTester->setInputs($answers);
+		$actualStatus = $this->commandTester->execute($input);
+		$this->assertEquals($expectedStatus, $actualStatus);
+		$output = $this->commandTester->getDisplay();
+		$this->assertStringContainsString($expectedOutput, $output);
+	}
+
+	public function inputProvider() {
+		return [
+			[['name' => 'doesNotExist.crt'], [], 1, 'certificate not found'],
+		];
+	}
+
+	public function testRemoveCertificate() {
+		$this->certificateManager->addCertificate(
+			\file_get_contents('data/certificates/goodCertificate.crt'),
+			'goodCertificate.crt');
+		$this->commandTester->setInputs([]);
+		$actualStatus = $this->commandTester->execute(['name' => 'goodCertificate.crt']);
+		$this->assertEquals(0, $actualStatus);
+		$output = $this->commandTester->getDisplay();
+		$this->assertEquals('', $output);
+	}
+}

tests/acceptance/features/bootstrap/OccContext.php

@@ -132,9 +132,11 @@ public function invokingTheCommand($cmd) {
 	 */
 	public function importSecurityCertificateFromPath($path) {
 		$this->invokingTheCommand("security:certificates:import " . $path);
-		$pathComponents = \explode("/", $path);
-		$certificate = \end($pathComponents);
-		\array_push($this->importedCertificates, $certificate);
+		if ($this->featureContext->getExitStatusCodeOfOccCommand() === 0) {
+			$pathComponents = \explode("/", $path);
+			$certificate = \end($pathComponents);
+			\array_push($this->importedCertificates, $certificate);
+		}
 	}
 
 	/**

tests/acceptance/features/cliMain/securityCertificates.feature

@@ -13,6 +13,11 @@ Feature: security certificates
       | table_column        |
       | goodCertificate.crt |
 
+  Scenario: Import a security certificate specifying a file that does not exist
+    When the administrator imports security certificate from the path "tests/data/certificates/aFileThatDoesNotExist.crt"
+    Then the command should have failed with exit code 1
+    And the command output should contain the text "certificate not found"
+
   Scenario: List security certificates when multiple certificates are imported
     Given the administrator has imported security certificate from the path "tests/data/certificates/goodCertificate.crt"
     And the administrator has imported security certificate from the path "tests/data/certificates/badCertificate.crt"
@@ -32,11 +37,10 @@ Feature: security certificates
       | table_column       |
       | badCertificate.crt |
 
-  @issue-35364
   Scenario: Remove a security certificate that is not installed
     When the administrator removes the security certificate "someCertificate.crt"
-    Then the command should have been successful
-    # Then the command should not have been successful
+    Then the command should have failed with exit code 1
+    And the command output should contain the text "certificate not found"
 
   Scenario: Import random file as certificate
     When the administrator imports security certificate from the path "tests/data/lorem.txt"

tests/lib/Security/CertificateManagerTest.php

@@ -103,11 +103,15 @@ public function testRemoveDangerousFile() {
 		$this->assertFalse($this->certificateManager->removeCertificate('../../foo.txt'));
 	}
 
-	public function testRemoveExistingFile() {
+	public function testRemoveCertificate() {
 		$this->certificateManager->addCertificate(\file_get_contents(__DIR__ . '/../../data/certificates/goodCertificate.crt'), 'GoodCertificate');
 		$this->assertTrue($this->certificateManager->removeCertificate('GoodCertificate'));
 	}
 
+	public function testRemoveNonExistentCertificate() {
+		$this->assertFalse($this->certificateManager->removeCertificate('NonExistentCertificate'));
+	}
+
 	public function testGetCertificateBundle() {
 		$this->assertSame('/' . $this->username . '/files_external/rootcerts.crt', $this->certificateManager->getCertificateBundle());
 	}