Merge pull request #2724 from nextcloud/fix-23591

[downstream] Report failures for SignApp and SignCore
nextcloud · Dec 21, 2016 · 091bf07 · 091bf07
2 parents 0d138c8 + 3eb3e43
commit 091bf07
Show file tree

Hide file tree

Showing 8 changed files with 292 additions and 52 deletions.
diff --git a/core/Command/Integrity/SignApp.php b/core/Command/Integrity/SignApp.php
@@ -101,8 +101,13 @@ protected function execute(InputInterface $input, OutputInterface $output) {
 		$x509 = new X509();
 		$x509->loadX509($keyBundle);
 		$x509->setPrivateKey($rsa);
-		$this->checker->writeAppSignature($path, $x509, $rsa);
-
-		$output->writeln('Successfully signed "'.$path.'"');
+		try {
+			$this->checker->writeAppSignature($path, $x509, $rsa);
+			$output->writeln('Successfully signed "'.$path.'"');
+		} catch (\Exception $e){
+			$output->writeln('Error: ' . $e->getMessage());
+			return 1;
+		}
+		return 0;
 	}
 }
diff --git a/core/Command/Integrity/SignCore.php b/core/Command/Integrity/SignCore.php
@@ -23,12 +23,10 @@
 namespace OC\Core\Command\Integrity;
 
 use OC\IntegrityCheck\Checker;
-use OC\IntegrityCheck\Helpers\EnvironmentHelper;
 use OC\IntegrityCheck\Helpers\FileAccessHelper;
 use phpseclib\Crypt\RSA;
 use phpseclib\File\X509;
 use Symfony\Component\Console\Command\Command;
-use OCP\IConfig;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Input\InputOption;
 use Symfony\Component\Console\Output\OutputInterface;
@@ -94,8 +92,14 @@ protected function execute(InputInterface $input, OutputInterface $output) {
 		$x509 = new X509();
 		$x509->loadX509($keyBundle);
 		$x509->setPrivateKey($rsa);
-		$this->checker->writeCoreSignature($x509, $rsa, $path);
 
-		$output->writeln('Successfully signed "core"');
+		try {
+			$this->checker->writeCoreSignature($x509, $rsa, $path);
+			$output->writeln('Successfully signed "core"');
+		} catch (\Exception $e){
+			$output->writeln('Error: ' . $e->getMessage());
+			return 1;
+		}
+		return 0;
 	}
 }
diff --git a/lib/private/IntegrityCheck/Checker.php b/lib/private/IntegrityCheck/Checker.php
@@ -267,16 +267,23 @@ private function createSignatureData(array $hashes,
 	public function writeAppSignature($path,
 									  X509 $certificate,
 									  RSA $privateKey) {
-		if(!is_dir($path)) {
-			throw new \Exception('Directory does not exist.');
-		}
-		$iterator = $this->getFolderIterator($path);
-		$hashes = $this->generateHashes($iterator, $path);
-		$signature = $this->createSignatureData($hashes, $certificate, $privateKey);
-		$this->fileAccessHelper->file_put_contents(
-				$path . '/appinfo/signature.json',
+		$appInfoDir = $path . '/appinfo';
+		try {
+			$this->fileAccessHelper->assertDirectoryExists($appInfoDir);
+
+			$iterator = $this->getFolderIterator($path);
+			$hashes = $this->generateHashes($iterator, $path);
+			$signature = $this->createSignatureData($hashes, $certificate, $privateKey);
+				$this->fileAccessHelper->file_put_contents(
+					$appInfoDir . '/signature.json',
 				json_encode($signature, JSON_PRETTY_PRINT)
-		);
+			);
+		} catch (\Exception $e){
+			if (!$this->fileAccessHelper->is_writable($appInfoDir)) {
+				throw new \Exception($appInfoDir . ' is not writable');
+			}
+			throw $e;
+		}
 	}
 
 	/**
@@ -285,17 +292,28 @@ public function writeAppSignature($path,
 	 * @param X509 $certificate
 	 * @param RSA $rsa
 	 * @param string $path
+	 * @throws \Exception
 	 */
 	public function writeCoreSignature(X509 $certificate,
 									   RSA $rsa,
 									   $path) {
-		$iterator = $this->getFolderIterator($path, $path);
-		$hashes = $this->generateHashes($iterator, $path);
-		$signatureData = $this->createSignatureData($hashes, $certificate, $rsa);
-		$this->fileAccessHelper->file_put_contents(
-				$path . '/core/signature.json',
+		$coreDir = $path . '/core';
+		try {
+
+			$this->fileAccessHelper->assertDirectoryExists($coreDir);
+			$iterator = $this->getFolderIterator($path, $path);
+			$hashes = $this->generateHashes($iterator, $path);
+			$signatureData = $this->createSignatureData($hashes, $certificate, $rsa);
+			$this->fileAccessHelper->file_put_contents(
+				$coreDir . '/signature.json',
 				json_encode($signatureData, JSON_PRETTY_PRINT)
-		);
+			);
+		} catch (\Exception $e){
+			if (!$this->fileAccessHelper->is_writable($coreDir)) {
+				throw new \Exception($coreDir . ' is not writable');
+			}
+			throw $e;
+		}
 	}
 
 	/**

diff --git a/lib/private/IntegrityCheck/Helpers/FileAccessHelper.php b/lib/private/IntegrityCheck/Helpers/FileAccessHelper.php
@@ -53,10 +53,33 @@ public function file_exists($filename) {
 	 * Wrapper around file_put_contents($filename, $data)
 	 *
 	 * @param string $filename
-	 * @param $data
-	 * @return int|false
+	 * @param string $data
+	 * @return int
+	 * @throws \Exception
 	 */
 	public function file_put_contents($filename, $data) {
-		return file_put_contents($filename, $data);
+		$bytesWritten = @file_put_contents($filename, $data);
+		if ($bytesWritten === false || $bytesWritten !== strlen($data)){
+			throw new \Exception('Failed to write into ' . $filename);
+		}
+		return $bytesWritten;
+	}
+
+	/**
+	 * @param string $path
+	 * @return bool
+	 */
+	public function is_writable($path) {
+		return is_writable($path);
+	}
+
+	/**
+	 * @param string $path
+	 * @throws \Exception
+	 */
+	public function assertDirectoryExists($path) {
+		if (!is_dir($path)) {
+			throw new \Exception('Directory ' . $path . ' does not exist.');
+		}
 	}
 }
diff --git a/tests/lib/Command/Integrity/SignAppTest.php b/tests/lib/Command/Integrity/SignAppTest.php
@@ -29,13 +29,13 @@
 use Test\TestCase;
 
 class SignAppTest extends TestCase {
-	/** @var Checker */
+	/** @var Checker|\PHPUnit_Framework_MockObject_MockObject */
 	private $checker;
 	/** @var SignApp */
 	private $signApp;
-	/** @var FileAccessHelper */
+	/** @var FileAccessHelper|\PHPUnit_Framework_MockObject_MockObject */
 	private $fileAccessHelper;
-	/** @var IURLGenerator */
+	/** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */
 	private $urlGenerator;
 
 	public function setUp() {
@@ -75,7 +75,7 @@ public function testExecuteWithMissingPath() {
 			->method('writeln')
 			->with('This command requires the --path, --privateKey and --certificate.');
 
-		$this->invokePrivate($this->signApp, 'execute', [$inputInterface, $outputInterface]);
+		$this->assertNull(self::invokePrivate($this->signApp, 'execute', [$inputInterface, $outputInterface]));
 	}
 
 	public function testExecuteWithMissingPrivateKey() {
@@ -103,7 +103,7 @@ public function testExecuteWithMissingPrivateKey() {
 				->method('writeln')
 				->with('This command requires the --path, --privateKey and --certificate.');
 
-		$this->invokePrivate($this->signApp, 'execute', [$inputInterface, $outputInterface]);
+		$this->assertNull(self::invokePrivate($this->signApp, 'execute', [$inputInterface, $outputInterface]));
 	}
 
 	public function testExecuteWithMissingCertificate() {
@@ -131,7 +131,7 @@ public function testExecuteWithMissingCertificate() {
 			->method('writeln')
 			->with('This command requires the --path, --privateKey and --certificate.');
 
-		$this->invokePrivate($this->signApp, 'execute', [$inputInterface, $outputInterface]);
+		$this->assertNull(self::invokePrivate($this->signApp, 'execute', [$inputInterface, $outputInterface]));
 	}
 
 	public function testExecuteWithNotExistingPrivateKey() {
@@ -165,7 +165,7 @@ public function testExecuteWithNotExistingPrivateKey() {
 			->method('writeln')
 			->with('Private key "privateKey" does not exists.');
 
-		$this->invokePrivate($this->signApp, 'execute', [$inputInterface, $outputInterface]);
+		$this->assertNull(self::invokePrivate($this->signApp, 'execute', [$inputInterface, $outputInterface]));
 	}
 
 	public function testExecuteWithNotExistingCertificate() {
@@ -204,7 +204,51 @@ public function testExecuteWithNotExistingCertificate() {
 			->method('writeln')
 			->with('Certificate "certificate" does not exists.');
 
-		$this->invokePrivate($this->signApp, 'execute', [$inputInterface, $outputInterface]);
+		$this->assertNull(self::invokePrivate($this->signApp, 'execute', [$inputInterface, $outputInterface]));
+	}
+
+	public function testExecuteWithException() {
+		$inputInterface = $this->createMock(InputInterface::class);
+		$outputInterface = $this->createMock(OutputInterface::class);
+
+		$inputInterface
+			->expects($this->at(0))
+			->method('getOption')
+			->with('path')
+			->will($this->returnValue('AppId'));
+		$inputInterface
+			->expects($this->at(1))
+			->method('getOption')
+			->with('privateKey')
+			->will($this->returnValue('privateKey'));
+		$inputInterface
+			->expects($this->at(2))
+			->method('getOption')
+			->with('certificate')
+			->will($this->returnValue('certificate'));
+
+		$this->fileAccessHelper
+			->expects($this->at(0))
+			->method('file_get_contents')
+			->with('privateKey')
+			->will($this->returnValue(\OC::$SERVERROOT . '/tests/data/integritycheck/core.key'));
+		$this->fileAccessHelper
+			->expects($this->at(1))
+			->method('file_get_contents')
+			->with('certificate')
+			->will($this->returnValue(\OC::$SERVERROOT . '/tests/data/integritycheck/core.crt'));
+
+		$this->checker
+			->expects($this->once())
+			->method('writeAppSignature')
+			->willThrowException(new \Exception('My error message'));
+
+		$outputInterface
+			->expects($this->at(0))
+			->method('writeln')
+			->with('Error: My error message');
+
+		$this->assertSame(1, self::invokePrivate($this->signApp, 'execute', [$inputInterface, $outputInterface]));
 	}
 
 	public function testExecute() {
@@ -247,6 +291,6 @@ public function testExecute() {
 			->method('writeln')
 			->with('Successfully signed "AppId"');
 
-		$this->invokePrivate($this->signApp, 'execute', [$inputInterface, $outputInterface]);
+		$this->assertSame(0, self::invokePrivate($this->signApp, 'execute', [$inputInterface, $outputInterface]));
 	}
 }
diff --git a/tests/lib/Command/Integrity/SignCoreTest.php b/tests/lib/Command/Integrity/SignCoreTest.php
@@ -28,12 +28,12 @@
 use Test\TestCase;
 
 class SignCoreTest extends TestCase {
-	/** @var Checker */
+	/** @var Checker|\PHPUnit_Framework_MockObject_MockObject */
 	private $checker;
+	/** @var FileAccessHelper|\PHPUnit_Framework_MockObject_MockObject */
+	private $fileAccessHelper;
 	/** @var SignCore */
 	private $signCore;
-	/** @var FileAccessHelper */
-	private $fileAccessHelper;
 
 	public function setUp() {
 		parent::setUp();
@@ -65,7 +65,7 @@ public function testExecuteWithMissingPrivateKey() {
 			->method('writeln')
 			->with('--privateKey, --certificate and --path are required.');
 
-		$this->invokePrivate($this->signCore, 'execute', [$inputInterface, $outputInterface]);
+		$this->assertNull(self::invokePrivate($this->signCore, 'execute', [$inputInterface, $outputInterface]));
 	}
 
 	public function testExecuteWithMissingCertificate() {
@@ -88,7 +88,7 @@ public function testExecuteWithMissingCertificate() {
 			->method('writeln')
 			->with('--privateKey, --certificate and --path are required.');
 
-		$this->invokePrivate($this->signCore, 'execute', [$inputInterface, $outputInterface]);
+		$this->assertNull(self::invokePrivate($this->signCore, 'execute', [$inputInterface, $outputInterface]));
 	}
 
 	public function testExecuteWithNotExistingPrivateKey() {
@@ -122,7 +122,7 @@ public function testExecuteWithNotExistingPrivateKey() {
 			->method('writeln')
 			->with('Private key "privateKey" does not exists.');
 
-		$this->invokePrivate($this->signCore, 'execute', [$inputInterface, $outputInterface]);
+		$this->assertNull(self::invokePrivate($this->signCore, 'execute', [$inputInterface, $outputInterface]));
 	}
 
 	public function testExecuteWithNotExistingCertificate() {
@@ -161,7 +161,51 @@ public function testExecuteWithNotExistingCertificate() {
 			->method('writeln')
 			->with('Certificate "certificate" does not exists.');
 
-		$this->invokePrivate($this->signCore, 'execute', [$inputInterface, $outputInterface]);
+		$this->assertNull(self::invokePrivate($this->signCore, 'execute', [$inputInterface, $outputInterface]));
+	}
+
+	public function testExecuteWithException() {
+		$inputInterface = $this->createMock(InputInterface::class);
+		$outputInterface = $this->createMock(OutputInterface::class);
+
+		$inputInterface
+			->expects($this->at(0))
+			->method('getOption')
+			->with('privateKey')
+			->will($this->returnValue('privateKey'));
+		$inputInterface
+			->expects($this->at(1))
+			->method('getOption')
+			->with('certificate')
+			->will($this->returnValue('certificate'));
+		$inputInterface
+			->expects($this->at(2))
+			->method('getOption')
+			->with('path')
+			->will($this->returnValue('certificate'));
+
+		$this->fileAccessHelper
+			->expects($this->at(0))
+			->method('file_get_contents')
+			->with('privateKey')
+			->will($this->returnValue(\OC::$SERVERROOT . '/tests/data/integritycheck/core.key'));
+		$this->fileAccessHelper
+			->expects($this->at(1))
+			->method('file_get_contents')
+			->with('certificate')
+			->will($this->returnValue(\OC::$SERVERROOT . '/tests/data/integritycheck/core.crt'));
+
+		$this->checker
+			->expects($this->once())
+			->method('writeCoreSignature')
+			->willThrowException(new \Exception('My exception message'));
+
+		$outputInterface
+			->expects($this->at(0))
+			->method('writeln')
+			->with('Error: My exception message');
+
+		$this->assertEquals(1, self::invokePrivate($this->signCore, 'execute', [$inputInterface, $outputInterface]));
 	}
 
 	public function testExecute() {
@@ -204,6 +248,6 @@ public function testExecute() {
 			->method('writeln')
 			->with('Successfully signed "core"');
 
-		$this->invokePrivate($this->signCore, 'execute', [$inputInterface, $outputInterface]);
+		$this->assertEquals(0, self::invokePrivate($this->signCore, 'execute', [$inputInterface, $outputInterface]));
 	}
 }