Add unit tests and fix rootcerts creation bug

2 files changed, 34 insertions, 30 deletions

diff --git a/lib/private/security/certificate.php b/lib/private/security/certificate.php
index 953111f469d..63c02a124f4 100644
--- a/lib/private/security/certificate.php
+++ b/lib/private/security/certificate.php
@@ -30,17 +30,22 @@ class Certificate implements ICertificate {
 	/**
 	 * @param string $data base64 encoded certificate
 	 * @param string $name
+	 * @throws \Exception If the certificate could not get parsed
 	 */
 	public function __construct($data, $name) {
 		$this->name = $name;
-		$info = openssl_x509_parse($data);
-		$this->commonName = $info['subject']['CN'];
-		$this->organization = isset($info['subject']['O']) ? $info['subject']['O'] : null;
-		$this->serial = $this->formatSerial($info['serialNumber']);
-		$this->issueDate = new \DateTime('@' . $info['validFrom_time_t']);
-		$this->expireDate = new \DateTime('@' . $info['validTo_time_t']);
-		$this->issuerName = $info['issuer']['CN'];
-		$this->issuerOrganization = isset($info['issuer']['O']) ? $info['issuer']['O'] : null;
+		try {
+			$info = openssl_x509_parse($data);
+			$this->commonName = isset($info['subject']['CN']) ? $info['subject']['CN'] : null;
+			$this->organization = isset($info['subject']['O']) ? $info['subject']['O'] : null;
+			$this->serial = $this->formatSerial($info['serialNumber']);
+			$this->issueDate = new \DateTime('@' . $info['validFrom_time_t']);
+			$this->expireDate = new \DateTime('@' . $info['validTo_time_t']);
+			$this->issuerName = isset($info['issuer']['CN']) ? $info['issuer']['CN'] : null;
+			$this->issuerOrganization = isset($info['issuer']['O']) ? $info['issuer']['O'] : null;
+		} catch (\Exception $e) {
+			throw new \Exception('Certificate could not get parsed.');
+		}
 	}
 
 	/**
@@ -62,7 +67,7 @@ class Certificate implements ICertificate {
 	}
 
 	/**
-	 * @return string
+	 * @return string|null
 	 */
 	public function getCommonName() {
 		return $this->commonName;
@@ -105,14 +110,14 @@ class Certificate implements ICertificate {
 	}
 
 	/**
-	 * @return string
+	 * @return string|null
 	 */
 	public function getIssuerName() {
 		return $this->issuerName;
 	}
 
 	/**
-	 * @return string
+	 * @return string|null
 	 */
 	public function getIssuerOrganization() {
 		return $this->issuerOrganization;
diff --git a/lib/private/security/certificatemanager.php b/lib/private/security/certificatemanager.php
index 64a1d6431a4..cae9730eb26 100644
--- a/lib/private/security/certificatemanager.php
+++ b/lib/private/security/certificatemanager.php
@@ -44,7 +44,9 @@ class CertificateManager implements ICertificateManager {
 		}
 		while (false !== ($file = readdir($handle))) {
 			if ($file != '.' && $file != '..') {
-				$result[] = new Certificate(file_get_contents($path . $file), $file);
+				try {
+					$result[] = new Certificate(file_get_contents($path . $file), $file);
+				} catch(\Exception $e) {}
 			}
 		}
 		return $result;
@@ -59,7 +61,7 @@ class CertificateManager implements ICertificateManager {
 
 		$fh_certs = fopen($path . '/rootcerts.crt', 'w');
 		foreach ($certs as $cert) {
-			$file = $path . '/uploads/' . $cert;
+			$file = $path . '/uploads/' . $cert->getName();
 			$data = file_get_contents($file);
 			if (strpos($data, 'BEGIN CERTIFICATE')) {
 				fwrite($fh_certs, $data);
@@ -75,35 +77,32 @@ class CertificateManager implements ICertificateManager {
 	 *
 	 * @param string $certificate the certificate data
 	 * @param string $name the filename for the certificate
-	 * @return bool | \OCP\ICertificate
+	 * @return \OCP\ICertificate|void|bool
+	 * @throws \Exception If the certificate could not get added
 	 */
 	public function addCertificate($certificate, $name) {
 		if (!Filesystem::isValidPath($name) or Filesystem::isFileBlacklisted($name)) {
 			return false;
 		}
-		$isValid = openssl_pkey_get_public($certificate);
 
-		if (!$isValid) {
-			$data = chunk_split(base64_encode($certificate), 64, "\n");
-			$data = "-----BEGIN CERTIFICATE-----\n" . $data . "-----END CERTIFICATE-----\n";
-			$isValid = openssl_pkey_get_public($data);
+		$dir = $this->user->getHome() . '/files_external/uploads/';
+		if (!file_exists($dir)) {
+			//path might not exist (e.g. non-standard OC_User::getHome() value)
+			//in this case create full path using 3rd (recursive=true) parameter.
+			//note that we use "normal" php filesystem functions here since the certs need to be local
+			mkdir($dir, 0700, true);
 		}
 
-		if ($isValid) {
-			$dir = $this->user->getHome() . '/files_external/uploads/';
-			if (!file_exists($dir)) {
-				//path might not exist (e.g. non-standard OC_User::getHome() value)
-				//in this case create full path using 3rd (recursive=true) parameter.
-				//note that we use "normal" php filesystem functions here since the certs need to be local
-				mkdir($dir, 0700, true);
-			}
+		try {
 			$file = $dir . $name;
+			$certificateObject = new Certificate($certificate, $name);
 			file_put_contents($file, $certificate);
 			$this->createCertificateBundle();
-			return new Certificate($certificate, $name);
-		} else {
-			return false;
+			return $certificateObject;
+		} catch (\Exception $e) {
+			throw $e;
 		}
+
 	}
 
 	/**