From 9f61cf60d49367726bc9b147993dab39eafa0c7b Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Wed, 15 Apr 2015 13:33:37 +0200 Subject: [PATCH 1/6] Verify if returned object is an array The error has to be thrown at this point as otherwise errors and notices are thrown since the time cannot be parsed in L60 and L61 --- lib/private/security/certificate.php | 20 ++++++------- tests/lib/security/certificate.php | 42 ++++++++++++++++++---------- 2 files changed, 38 insertions(+), 24 deletions(-) diff --git a/lib/private/security/certificate.php b/lib/private/security/certificate.php index 468427d0702..0d7fcc4148d 100644 --- a/lib/private/security/certificate.php +++ b/lib/private/security/certificate.php @@ -49,18 +49,18 @@ class Certificate implements ICertificate { */ public function __construct($data, $name) { $this->name = $name; - try { - $gmt = new \DateTimeZone('GMT'); - $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->issueDate = new \DateTime('@' . $info['validFrom_time_t'], $gmt); - $this->expireDate = new \DateTime('@' . $info['validTo_time_t'], $gmt); - $this->issuerName = isset($info['issuer']['CN']) ? $info['issuer']['CN'] : null; - $this->issuerOrganization = isset($info['issuer']['O']) ? $info['issuer']['O'] : null; - } catch (\Exception $e) { + $gmt = new \DateTimeZone('GMT'); + $info = openssl_x509_parse($data); + if(!is_array($info)) { throw new \Exception('Certificate could not get parsed.'); } + + $this->commonName = isset($info['subject']['CN']) ? $info['subject']['CN'] : null; + $this->organization = isset($info['subject']['O']) ? $info['subject']['O'] : null; + $this->issueDate = new \DateTime('@' . $info['validFrom_time_t'], $gmt); + $this->expireDate = new \DateTime('@' . $info['validTo_time_t'], $gmt); + $this->issuerName = isset($info['issuer']['CN']) ? $info['issuer']['CN'] : null; + $this->issuerOrganization = isset($info['issuer']['O']) ? $info['issuer']['O'] : null; } /** diff --git a/tests/lib/security/certificate.php b/tests/lib/security/certificate.php index 361f2f8c38d..7fc8bbbdf25 100644 --- a/tests/lib/security/certificate.php +++ b/tests/lib/security/certificate.php @@ -1,9 +1,22 @@ - * This file is licensed under the Affero General Public License version 3 or - * later. - * See the COPYING-README file. + * @author Lukas Reschke + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @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 + * */ use \OC\Security\Certificate; @@ -32,33 +45,34 @@ class CertificateTest extends \Test\TestCase { * @expectedException \Exception * @expectedExceptionMessage Certificate could not get parsed. */ - function testBogusData() { - new Certificate('foo', 'bar'); + public function testBogusData() { + $certificate = new Certificate('foo', 'bar'); + $certificate->getIssueDate(); } - function testGetName() { + public function testGetName() { $this->assertSame('GoodCertificate', $this->goodCertificate->getName()); $this->assertSame('BadCertificate', $this->invalidCertificate->getName()); } - function testGetCommonName() { + public function testGetCommonName() { $this->assertSame('security.owncloud.com', $this->goodCertificate->getCommonName()); $this->assertSame(null, $this->invalidCertificate->getCommonName()); } - function testGetOrganization() { + public function testGetOrganization() { $this->assertSame('ownCloud Inc.', $this->goodCertificate->getOrganization()); $this->assertSame('Internet Widgits Pty Ltd', $this->invalidCertificate->getOrganization()); } - function testGetIssueDate() { + public function testGetIssueDate() { $expected = new DateTime('2014-08-27 08:45:52 GMT'); $this->assertEquals($expected->getTimestamp(), $this->goodCertificate->getIssueDate()->getTimestamp()); $expected = new DateTime('2014-08-27 08:48:51 GMT'); $this->assertEquals($expected->getTimestamp(), $this->invalidCertificate->getIssueDate()->getTimestamp()); } - function testGetExpireDate() { + public function testGetExpireDate() { $expected = new DateTime('2015-08-27 08:45:52 GMT'); $this->assertEquals($expected->getTimestamp(), $this->goodCertificate->getExpireDate()->getTimestamp()); $expected = new DateTime('2015-08-27 08:48:51 GMT'); @@ -70,19 +84,19 @@ class CertificateTest extends \Test\TestCase { /** * Obviously the following test case might fail after 2015-08-27, just create a new certificate with longer validity then */ - function testIsExpired() { + public function testIsExpired() { $this->assertSame(false, $this->goodCertificate->isExpired()); $this->assertSame(false, $this->invalidCertificate->isExpired()); $this->assertSame(true, $this->expiredCertificate->isExpired()); } - function testGetIssuerName() { + public function testGetIssuerName() { $this->assertSame('security.owncloud.com', $this->goodCertificate->getIssuerName()); $this->assertSame(null, $this->invalidCertificate->getIssuerName()); $this->assertSame(null, $this->expiredCertificate->getIssuerName()); } - function testGetIssuerOrganization() { + public function testGetIssuerOrganization() { $this->assertSame('ownCloud Inc.', $this->goodCertificate->getIssuerOrganization()); $this->assertSame('Internet Widgits Pty Ltd', $this->invalidCertificate->getIssuerOrganization()); $this->assertSame('Internet Widgits Pty Ltd', $this->expiredCertificate->getIssuerOrganization()); From 20a6073a9feb6b0eb4c004e58bb47912c447fef9 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Wed, 15 Apr 2015 14:21:23 +0200 Subject: [PATCH 2/6] Migrate personal certificate handling into AppFramework controllers Also added unit-tests and better error-handling --- apps/files_external/appinfo/routes.php | 6 - settings/ajax/addRootCertificate.php | 52 ------ settings/ajax/removeRootCertificate.php | 28 --- settings/application.php | 12 ++ settings/controller/certificatecontroller.php | 93 ++++++++++ settings/js/personal.js | 22 ++- settings/personal.php | 2 + settings/routes.php | 6 +- settings/templates/personal.php | 3 +- .../controller/CertificateControllerTest.php | 174 ++++++++++++++++++ 10 files changed, 298 insertions(+), 100 deletions(-) delete mode 100644 settings/ajax/addRootCertificate.php delete mode 100644 settings/ajax/removeRootCertificate.php create mode 100644 settings/controller/certificatecontroller.php create mode 100644 tests/settings/controller/CertificateControllerTest.php diff --git a/apps/files_external/appinfo/routes.php b/apps/files_external/appinfo/routes.php index 98eb2fcccbb..8c6dff7a901 100644 --- a/apps/files_external/appinfo/routes.php +++ b/apps/files_external/appinfo/routes.php @@ -46,12 +46,6 @@ $application->registerRoutes( ) ); -// TODO: move these to app framework -$this->create('files_external_add_root_certificate', 'ajax/addRootCertificate.php') - ->actionInclude('files_external/ajax/addRootCertificate.php'); -$this->create('files_external_remove_root_certificate', 'ajax/removeRootCertificate.php') - ->actionInclude('files_external/ajax/removeRootCertificate.php'); - $this->create('files_external_dropbox', 'ajax/dropbox.php') ->actionInclude('files_external/ajax/dropbox.php'); $this->create('files_external_google', 'ajax/google.php') diff --git a/settings/ajax/addRootCertificate.php b/settings/ajax/addRootCertificate.php deleted file mode 100644 index 64a55eaede9..00000000000 --- a/settings/ajax/addRootCertificate.php +++ /dev/null @@ -1,52 +0,0 @@ - - * @author Robin Appelman - * - * @copyright Copyright (c) 2015, ownCloud, Inc. - * @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 - * - */ -OCP\JSON::checkLoggedIn(); -OCP\JSON::callCheck(); - -$l = new OC_L10N('core'); - -if (!isset($_FILES['rootcert_import'])) { - OCP\JSON::error(array('error' => 'No certificate uploaded')); - exit; -} - -$data = file_get_contents($_FILES['rootcert_import']['tmp_name']); -$filename = basename($_FILES['rootcert_import']['name']); - -$certificateManager = \OC::$server->getCertificateManager(); - -try { - $cert = $certificateManager->addCertificate($data, $filename); - OCP\JSON::success(array( - 'name' => $cert->getName(), - 'commonName' => $cert->getCommonName(), - 'organization' => $cert->getOrganization(), - 'validFrom' => $cert->getIssueDate()->getTimestamp(), - 'validTill' => $cert->getExpireDate()->getTimestamp(), - 'validFromString' => $l->l('date', $cert->getIssueDate()), - 'validTillString' => $l->l('date', $cert->getExpireDate()), - 'issuer' => $cert->getIssuerName(), - 'issuerOrganization' => $cert->getIssuerOrganization() - )); -} catch(\Exception $e) { - OCP\JSON::error(array('error' => 'Couldn\'t import SSL root certificate, allowed formats: PEM and DER')); -} diff --git a/settings/ajax/removeRootCertificate.php b/settings/ajax/removeRootCertificate.php deleted file mode 100644 index 4ef5fe32aed..00000000000 --- a/settings/ajax/removeRootCertificate.php +++ /dev/null @@ -1,28 +0,0 @@ - - * @author Lukas Reschke - * @author Robin Appelman - * - * @copyright Copyright (c) 2015, ownCloud, Inc. - * @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 - * - */ -OCP\JSON::checkLoggedIn(); -OCP\JSON::callCheck(); - -$name = (string)$_POST['cert']; -$certificateManager = \OC::$server->getCertificateManager(); -$certificateManager->removeCertificate($name); diff --git a/settings/application.php b/settings/application.php index 59fe9f6b65a..920d172c93d 100644 --- a/settings/application.php +++ b/settings/application.php @@ -25,6 +25,7 @@ namespace OC\Settings; use OC\Files\View; use OC\Settings\Controller\AppSettingsController; +use OC\Settings\Controller\CertificateController; use OC\Settings\Controller\CheckSetupController; use OC\Settings\Controller\EncryptionController; use OC\Settings\Controller\GroupsController; @@ -97,6 +98,14 @@ class Application extends App { $c->query('Config') ); }); + $container->registerService('CertificateController', function(IContainer $c) { + return new CertificateController( + $c->query('AppName'), + $c->query('Request'), + $c->query('CertificateManager'), + $c->query('L10N') + ); + }); $container->registerService('GroupsController', function(IContainer $c) { return new GroupsController( $c->query('AppName'), @@ -223,5 +232,8 @@ class Application extends App { $container->registerService('DatabaseConnection', function(IContainer $c) { return $c->query('ServerContainer')->getDatabaseConnection(); }); + $container->registerService('CertificateManager', function(IContainer $c){ + return $c->query('ServerContainer')->getCertificateManager(); + }); } } diff --git a/settings/controller/certificatecontroller.php b/settings/controller/certificatecontroller.php new file mode 100644 index 00000000000..c8d05a51240 --- /dev/null +++ b/settings/controller/certificatecontroller.php @@ -0,0 +1,93 @@ + + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @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 + * + */ + +namespace OC\Settings\Controller; + +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http; +use OCP\AppFramework\Http\DataResponse; +use OCP\ICertificateManager; +use OCP\IL10N; +use OCP\IRequest; + +/** + * @package OC\Settings\Controller + */ +class CertificateController extends Controller { + /** @var ICertificateManager */ + private $certificateManager; + /** @var IL10N */ + private $l10n; + + /** + * @param string $appName + * @param IRequest $request + * @param ICertificateManager $certificateManager + * @param IL10N $l10n + */ + public function __construct($appName, + IRequest $request, + ICertificateManager $certificateManager, + IL10N $l10n) { + parent::__construct($appName, $request); + $this->certificateManager = $certificateManager; + $this->l10n = $l10n; + } + + /** + * Add a new personal root certificate to the users' trust store + * @return array + */ + public function addPersonalRootCertificate() { + $file = $this->request->getUploadedFile('rootcert_import'); + if(empty($file)) { + return new DataResponse(['message' => 'No file uploaded'], Http::STATUS_UNPROCESSABLE_ENTITY); + } + + try { + $certificate = $this->certificateManager->addCertificate(file_get_contents($file['tmp_name']), $file['name']); + return new DataResponse([ + 'name' => $certificate->getName(), + 'commonName' => $certificate->getCommonName(), + 'organization' => $certificate->getOrganization(), + 'validFrom' => $certificate->getIssueDate()->getTimestamp(), + 'validTill' => $certificate->getExpireDate()->getTimestamp(), + 'validFromString' => $this->l10n->l('date', $certificate->getIssueDate()), + 'validTillString' => $this->l10n->l('date', $certificate->getExpireDate()), + 'issuer' => $certificate->getIssuerName(), + 'issuerOrganization' => $certificate->getIssuerOrganization(), + ]); + } catch (\Exception $e) { + return new DataResponse('An error occurred.', Http::STATUS_INTERNAL_SERVER_ERROR); + } + } + + /** + * Removes a personal root certificate from the users' trust store + * @param string $certificateIdentifier + * @return DataResponse + */ + public function removePersonalRootCertificate($certificateIdentifier) { + $this->certificateManager->removeCertificate($certificateIdentifier); + return new DataResponse(); + } + +} diff --git a/settings/js/personal.js b/settings/js/personal.js index 687b02399a7..c251d1572b2 100644 --- a/settings/js/personal.js +++ b/settings/js/personal.js @@ -297,8 +297,8 @@ $(document).ready(function () { $('#sslCertificate').on('click', 'td.remove > img', function () { var row = $(this).parent().parent(); - $.post(OC.generateUrl('settings/ajax/removeRootCertificate'), { - cert: row.data('name') + $.ajax(OC.generateUrl('settings/personal/certificate/{certificate}', {certificate: row.data('name')}), { + type: 'DELETE' }); row.remove(); return true; @@ -307,18 +307,19 @@ $(document).ready(function () { $('#sslCertificate tr > td').tipsy({gravity: 'n', live: true}); $('#rootcert_import').fileupload({ - done: function (e, data) { - var issueDate = new Date(data.result.validFrom * 1000); - var expireDate = new Date(data.result.validTill * 1000); + success: function (data) { + var issueDate = new Date(data.validFrom * 1000); + var expireDate = new Date(data.validTill * 1000); var now = new Date(); var isExpired = !(issueDate <= now && now <= expireDate); var row = $(''); + row.data('name', data.name); row.addClass(isExpired? 'expired': 'valid'); - row.append($('').attr('title', data.result.organization).text(data.result.commonName)); - row.append($('').attr('title', t('core,', 'Valid until {date}', {date: data.result.validFromString})) - .text(data.result.validTillString)); - row.append($('').attr('title', data.result.issuerOrganization).text(data.result.issuer)); + row.append($('').attr('title', data.organization).text(data.commonName)); + row.append($('').attr('title', t('core,', 'Valid until {date}', {date: data.validFromString})) + .text(data.validTillString)); + row.append($('').attr('title', data.issuerOrganization).text(data.issuer)); row.append($('').addClass('remove').append( $('').attr({ alt: t('core', 'Delete'), @@ -328,6 +329,9 @@ $(document).ready(function () { )); $('#sslCertificate tbody').append(row); + }, + fail: function (e, data) { + OC.Notification.showTemporary(t('settings', 'An error occured. Please upload an ASCII-encoded PEM certificate.')); } }); diff --git a/settings/personal.php b/settings/personal.php index 12b320ac001..7bf1110c03e 100644 --- a/settings/personal.php +++ b/settings/personal.php @@ -37,6 +37,7 @@ OC_Util::checkLoggedIn(); $defaults = new OC_Defaults(); // initialize themable default strings and urls $certificateManager = \OC::$server->getCertificateManager(); $config = \OC::$server->getConfig(); +$urlGenerator = \OC::$server->getURLGenerator(); // Highlight navigation entry OC_Util::addScript( 'settings', 'personal' ); @@ -118,6 +119,7 @@ $tmpl->assign('displayName', OC_User::getDisplayName()); $tmpl->assign('enableAvatars', $config->getSystemValue('enable_avatars', true)); $tmpl->assign('avatarChangeSupported', OC_User::canUserChangeAvatar(OC_User::getUser())); $tmpl->assign('certs', $certificateManager->listCertificates()); +$tmpl->assign('urlGenerator', $urlGenerator); // Get array of group ids for this user $groups = \OC::$server->getGroupManager()->getUserIdGroups(OC_User::getUser()); diff --git a/settings/routes.php b/settings/routes.php index 462b4ab543f..52b320cbdb5 100644 --- a/settings/routes.php +++ b/settings/routes.php @@ -53,6 +53,8 @@ $application->registerRoutes($this, [ ['name' => 'LogSettings#getEntries', 'url' => '/settings/admin/log/entries', 'verb' => 'GET'], ['name' => 'LogSettings#download', 'url' => '/settings/admin/log/download', 'verb' => 'GET'], ['name' => 'CheckSetup#check', 'url' => '/settings/ajax/checksetup', 'verb' => 'GET'], + ['name' => 'Certificate#addPersonalRootCertificate', 'url' => '/settings/personal/certificate', 'verb' => 'POST'], + ['name' => 'Certificate#removePersonalRootCertificate', 'url' => '/settings/personal/certificate/{certificateIdentifier}', 'verb' => 'DELETE'], ] ]); @@ -90,10 +92,6 @@ $this->create('settings_personal_changepassword', '/settings/personal/changepass ->action('OC\Settings\ChangePassword\Controller', 'changePersonalPassword'); $this->create('settings_ajax_setlanguage', '/settings/ajax/setlanguage.php') ->actionInclude('settings/ajax/setlanguage.php'); -$this->create('settings_cert_post', '/settings/ajax/addRootCertificate') - ->actionInclude('settings/ajax/addRootCertificate.php'); -$this->create('settings_cert_remove', '/settings/ajax/removeRootCertificate') - ->actionInclude('settings/ajax/removeRootCertificate.php'); // apps $this->create('settings_ajax_enableapp', '/settings/ajax/enableapp.php') ->actionInclude('settings/ajax/enableapp.php'); diff --git a/settings/templates/personal.php b/settings/templates/personal.php index dfdc6191805..02ee261cd1d 100644 --- a/settings/templates/personal.php +++ b/settings/templates/personal.php @@ -5,6 +5,7 @@ */ /** @var $_ array */ +/** @var $_['urlGenerator'] */ ?>
@@ -236,7 +237,7 @@ if($_['passwordChangeSupported']) { -
+
diff --git a/tests/settings/controller/CertificateControllerTest.php b/tests/settings/controller/CertificateControllerTest.php new file mode 100644 index 00000000000..363175cca41 --- /dev/null +++ b/tests/settings/controller/CertificateControllerTest.php @@ -0,0 +1,174 @@ + + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @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 + * + */ + +namespace OC\Settings\Controller; + +use OCP\AppFramework\Http; +use OCP\AppFramework\Http\DataResponse; +use OCP\IRequest; +use OCP\IL10N; +use OCP\ICertificateManager; + +/** + * Class CertificateControllerTest + * + * @package OC\Settings\Controller + */ +class CertificateControllerTest extends \Test\TestCase { + /** @var CertificateController */ + private $certificateController; + /** @var IRequest */ + private $request; + /** @var ICertificateManager */ + private $certificateManager; + /** @var IL10N */ + private $l10n; + + public function setUp() { + parent::setUp(); + + $this->request = $this->getMock('\OCP\IRequest'); + $this->certificateManager = $this->getMock('\OCP\ICertificateManager'); + $this->l10n = $this->getMock('\OCP\IL10N'); + + $this->certificateController = new CertificateController( + 'settings', + $this->request, + $this->certificateManager, + $this->l10n + ); + } + + public function testAddPersonalRootCertificateWithEmptyFile() { + $this->request + ->expects($this->once()) + ->method('getUploadedFile') + ->with('rootcert_import') + ->will($this->returnValue(null)); + + $expected = new DataResponse(['message' => 'No file uploaded'], Http::STATUS_UNPROCESSABLE_ENTITY); + $this->assertEquals($expected, $this->certificateController->addPersonalRootCertificate()); + } + + public function testAddPersonalRootCertificateValidCertificate() { + $uploadedFile = [ + 'tmp_name' => __DIR__ . '/../../data/certificates/goodCertificate.crt', + 'name' => 'goodCertificate.crt', + ]; + + $certificate = $this->getMock('\OCP\ICertificate'); + $certificate + ->expects($this->once()) + ->method('getName') + ->will($this->returnValue('Name')); + $certificate + ->expects($this->once()) + ->method('getCommonName') + ->will($this->returnValue('CommonName')); + $certificate + ->expects($this->once()) + ->method('getOrganization') + ->will($this->returnValue('Organization')); + $certificate + ->expects($this->exactly(2)) + ->method('getIssueDate') + ->will($this->returnValue(new \DateTime('@1429099555'))); + $certificate + ->expects($this->exactly(2)) + ->method('getExpireDate') + ->will($this->returnValue(new \DateTime('@1529099555'))); + $certificate + ->expects($this->once()) + ->method('getIssuerName') + ->will($this->returnValue('Issuer')); + $certificate + ->expects($this->once()) + ->method('getIssuerOrganization') + ->will($this->returnValue('IssuerOrganization')); + + $this->request + ->expects($this->once()) + ->method('getUploadedFile') + ->with('rootcert_import') + ->will($this->returnValue($uploadedFile)); + $this->certificateManager + ->expects($this->once()) + ->method('addCertificate') + ->with(file_get_contents($uploadedFile['tmp_name'], 'goodCertificate.crt')) + ->will($this->returnValue($certificate)); + + $this->l10n + ->expects($this->at(0)) + ->method('l') + ->with('date', new \DateTime('@1429099555')) + ->will($this->returnValue('Valid From as String')); + $this->l10n + ->expects($this->at(1)) + ->method('l') + ->with('date', new \DateTime('@1529099555')) + ->will($this->returnValue('Valid Till as String')); + + + $expected = new DataResponse([ + 'name' => 'Name', + 'commonName' => 'CommonName', + 'organization' => 'Organization', + 'validFrom' => 1429099555, + 'validTill' => 1529099555, + 'validFromString' => 'Valid From as String', + 'validTillString' => 'Valid Till as String', + 'issuer' => 'Issuer', + 'issuerOrganization' => 'IssuerOrganization', + ]); + $this->assertEquals($expected, $this->certificateController->addPersonalRootCertificate()); + } + + public function testAddPersonalRootCertificateInValidCertificate() { + $uploadedFile = [ + 'tmp_name' => __DIR__ . '/../../data/certificates/badCertificate.crt', + 'name' => 'badCertificate.crt', + ]; + + $this->request + ->expects($this->once()) + ->method('getUploadedFile') + ->with('rootcert_import') + ->will($this->returnValue($uploadedFile)); + $this->certificateManager + ->expects($this->once()) + ->method('addCertificate') + ->with(file_get_contents($uploadedFile['tmp_name'], 'goodCertificate.crt')) + ->will($this->throwException(new \Exception())); + + $expected = new DataResponse('An error occurred.', Http::STATUS_INTERNAL_SERVER_ERROR); + $this->assertEquals($expected, $this->certificateController->addPersonalRootCertificate()); + } + + public function testRemoveCertificate() { + $this->certificateManager + ->expects($this->once()) + ->method('removeCertificate') + ->with('CertificateToRemove'); + + $this->assertEquals(new DataResponse(), $this->certificateController->removePersonalRootCertificate('CertificateToRemove')); + } + +} From 1cc2aefa46a2db29b7bfb8c70fe3c9928d86da25 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Wed, 15 Apr 2015 15:56:10 +0200 Subject: [PATCH 3/6] Proper return types --- lib/private/security/certificatemanager.php | 4 ++-- lib/public/icertificatemanager.php | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/private/security/certificatemanager.php b/lib/private/security/certificatemanager.php index beeb87a8316..d61c7f29327 100644 --- a/lib/private/security/certificatemanager.php +++ b/lib/private/security/certificatemanager.php @@ -107,12 +107,12 @@ class CertificateManager implements ICertificateManager { * * @param string $certificate the certificate data * @param string $name the filename for the certificate - * @return \OCP\ICertificate|void|bool + * @return \OCP\ICertificate * @throws \Exception If the certificate could not get added */ public function addCertificate($certificate, $name) { if (!Filesystem::isValidPath($name) or Filesystem::isFileBlacklisted($name)) { - return false; + throw new \Exception('Filename is not valid'); } $dir = $this->getPathToCertificates() . 'uploads/'; diff --git a/lib/public/icertificatemanager.php b/lib/public/icertificatemanager.php index ec88f32e291..3014cd8f633 100644 --- a/lib/public/icertificatemanager.php +++ b/lib/public/icertificatemanager.php @@ -38,7 +38,8 @@ interface ICertificateManager { /** * @param string $certificate the certificate data * @param string $name the filename for the certificate - * @return bool | \OCP\ICertificate + * @return \OCP\ICertificate + * @throws \Exception If the certificate could not get added * @since 8.0.0 */ public function addCertificate($certificate, $name); From 21f0476d3179f7f9b9085f02404f79c392520bc3 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Wed, 15 Apr 2015 16:20:05 +0200 Subject: [PATCH 4/6] Fix files --- tests/lib/security/certificatemanager.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/lib/security/certificatemanager.php b/tests/lib/security/certificatemanager.php index 1167fe3d868..aea1fb71bc5 100644 --- a/tests/lib/security/certificatemanager.php +++ b/tests/lib/security/certificatemanager.php @@ -67,9 +67,13 @@ class CertificateManagerTest extends \Test\TestCase { $this->certificateManager->addCertificate('InvalidCertificate', 'invalidCertificate'); } + /** + * @expectedException \Exception + * @expectedExceptionMessage Filename is not valid + */ function testAddDangerousFile() { - $this->assertFalse($this->certificateManager->addCertificate(file_get_contents(__DIR__.'/../../data/certificates/expiredCertificate.crt'), '.htaccess')); - $this->assertFalse($this->certificateManager->addCertificate(file_get_contents(__DIR__.'/../../data/certificates/expiredCertificate.crt'), '../../foo.txt')); + $this->certificateManager->addCertificate(file_get_contents(__DIR__.'/../../data/certificates/expiredCertificate.crt'), '.htaccess'); + $this->certificateManager->addCertificate(file_get_contents(__DIR__.'/../../data/certificates/expiredCertificate.crt'), '../../foo.txt'); } function testRemoveDangerousFile() { From 89353695648eb686e6fffd49f86a6d014121f551 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Wed, 15 Apr 2015 18:06:57 +0200 Subject: [PATCH 5/6] fix date variable --- settings/js/personal.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/settings/js/personal.js b/settings/js/personal.js index c251d1572b2..165b55bcdae 100644 --- a/settings/js/personal.js +++ b/settings/js/personal.js @@ -317,7 +317,7 @@ $(document).ready(function () { row.data('name', data.name); row.addClass(isExpired? 'expired': 'valid'); row.append($('').attr('title', data.organization).text(data.commonName)); - row.append($('').attr('title', t('core,', 'Valid until {date}', {date: data.validFromString})) + row.append($('').attr('title', t('core,', 'Valid until {date}', {date: data.validTillString})) .text(data.validTillString)); row.append($('').attr('title', data.issuerOrganization).text(data.issuer)); row.append($('').addClass('remove').append( From 9bc48451b939df58b7e7e90300c9c716c348027f Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Thu, 16 Apr 2015 10:48:25 +0200 Subject: [PATCH 6/6] Adjust tests and statuscode --- settings/controller/certificatecontroller.php | 2 +- tests/lib/security/certificatemanager.php | 20 ++++++++++++++----- .../controller/CertificateControllerTest.php | 6 +++--- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/settings/controller/certificatecontroller.php b/settings/controller/certificatecontroller.php index c8d05a51240..d9026cd42af 100644 --- a/settings/controller/certificatecontroller.php +++ b/settings/controller/certificatecontroller.php @@ -76,7 +76,7 @@ class CertificateController extends Controller { 'issuerOrganization' => $certificate->getIssuerOrganization(), ]); } catch (\Exception $e) { - return new DataResponse('An error occurred.', Http::STATUS_INTERNAL_SERVER_ERROR); + return new DataResponse('An error occurred.', Http::STATUS_UNPROCESSABLE_ENTITY); } } diff --git a/tests/lib/security/certificatemanager.php b/tests/lib/security/certificatemanager.php index aea1fb71bc5..fab1c208443 100644 --- a/tests/lib/security/certificatemanager.php +++ b/tests/lib/security/certificatemanager.php @@ -14,8 +14,6 @@ class CertificateManagerTest extends \Test\TestCase { private $certificateManager; /** @var String */ private $username; - /** @var \OC\User\User */ - private $user; protected function setUp() { parent::setUp(); @@ -67,13 +65,25 @@ class CertificateManagerTest extends \Test\TestCase { $this->certificateManager->addCertificate('InvalidCertificate', 'invalidCertificate'); } + /** + * @return array + */ + public function dangerousFileProvider() { + return [ + ['.htaccess'], + ['../../foo.txt'], + ['..\..\foo.txt'], + ]; + } + /** * @expectedException \Exception * @expectedExceptionMessage Filename is not valid + * @dataProvider dangerousFileProvider + * @param string $filename */ - function testAddDangerousFile() { - $this->certificateManager->addCertificate(file_get_contents(__DIR__.'/../../data/certificates/expiredCertificate.crt'), '.htaccess'); - $this->certificateManager->addCertificate(file_get_contents(__DIR__.'/../../data/certificates/expiredCertificate.crt'), '../../foo.txt'); + function testAddDangerousFile($filename) { + $this->certificateManager->addCertificate(file_get_contents(__DIR__.'/../../data/certificates/expiredCertificate.crt'), $filename); } function testRemoveDangerousFile() { diff --git a/tests/settings/controller/CertificateControllerTest.php b/tests/settings/controller/CertificateControllerTest.php index 363175cca41..b6981195034 100644 --- a/tests/settings/controller/CertificateControllerTest.php +++ b/tests/settings/controller/CertificateControllerTest.php @@ -141,7 +141,7 @@ class CertificateControllerTest extends \Test\TestCase { $this->assertEquals($expected, $this->certificateController->addPersonalRootCertificate()); } - public function testAddPersonalRootCertificateInValidCertificate() { + public function testAddPersonalRootCertificateInvalidCertificate() { $uploadedFile = [ 'tmp_name' => __DIR__ . '/../../data/certificates/badCertificate.crt', 'name' => 'badCertificate.crt', @@ -155,10 +155,10 @@ class CertificateControllerTest extends \Test\TestCase { $this->certificateManager ->expects($this->once()) ->method('addCertificate') - ->with(file_get_contents($uploadedFile['tmp_name'], 'goodCertificate.crt')) + ->with(file_get_contents($uploadedFile['tmp_name'], 'badCertificate.crt')) ->will($this->throwException(new \Exception())); - $expected = new DataResponse('An error occurred.', Http::STATUS_INTERNAL_SERVER_ERROR); + $expected = new DataResponse('An error occurred.', Http::STATUS_UNPROCESSABLE_ENTITY); $this->assertEquals($expected, $this->certificateController->addPersonalRootCertificate()); }