From 13b1b45ee4bab5b832ca3a1602b4c4fb6d391f86 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Fri, 3 Oct 2014 15:14:22 +0200 Subject: [PATCH 1/2] Refactor MailSettings controller - Do not store the password (fixes https://github.com/owncloud/core/issues/11385) - Refactor to AppFramework - Add unit tests Conflicts: settings/admin/controller.php --- settings/admin/controller.php | 123 ----------- settings/application.php | 70 +++++++ .../controller/mailsettingscontroller.php | 166 +++++++++++++++ settings/css/settings.css | 4 +- settings/js/admin.js | 12 +- settings/routes.php | 17 +- settings/templates/admin.php | 141 +++++++------ tests/phpunit-autotest.xml | 1 + .../controller/mailsettingscontrollertest.php | 196 ++++++++++++++++++ 9 files changed, 527 insertions(+), 203 deletions(-) delete mode 100644 settings/admin/controller.php create mode 100644 settings/application.php create mode 100644 settings/controller/mailsettingscontroller.php create mode 100644 tests/settings/controller/mailsettingscontrollertest.php diff --git a/settings/admin/controller.php b/settings/admin/controller.php deleted file mode 100644 index 395bc7c6e4..0000000000 --- a/settings/admin/controller.php +++ /dev/null @@ -1,123 +0,0 @@ -. -*/ - -namespace OC\Settings\Admin; - -class Controller { - /** - * Set mail settings - */ - public static function setMailSettings() { - \OC_Util::checkAdminUser(); - \OCP\JSON::callCheck(); - - $l = \OC::$server->getL10N('settings'); - - $smtp_settings = array( - 'mail_domain' => null, - 'mail_from_address' => null, - 'mail_smtpmode' => array('sendmail', 'smtp', 'qmail', 'php'), - 'mail_smtpsecure' => array('', 'ssl', 'tls'), - 'mail_smtphost' => null, - 'mail_smtpport' => null, - 'mail_smtpauthtype' => array('LOGIN', 'PLAIN', 'NTLM'), - 'mail_smtpauth' => true, - 'mail_smtpname' => null, - 'mail_smtppassword' => null, - ); - - foreach ($smtp_settings as $setting => $validate) { - if (!$validate) { - if (!isset($_POST[$setting]) || $_POST[$setting] === '') { - \OC_Config::deleteKey( $setting ); - } else { - \OC_Config::setValue( $setting, $_POST[$setting] ); - } - } - else if (is_bool($validate)) { - if (!empty($_POST[$setting])) { - \OC_Config::setValue( $setting, (bool) $_POST[$setting] ); - } else { - \OC_Config::deleteKey( $setting ); - } - } - else if (is_array($validate)) { - if (!isset($_POST[$setting]) || $_POST[$setting] === '') { - \OC_Config::deleteKey( $setting ); - } else if (in_array($_POST[$setting], $validate)) { - \OC_Config::setValue( $setting, $_POST[$setting] ); - } else { - $message = $l->t('Invalid value supplied for %s', array(self::getFieldname($setting, $l))); - \OC_JSON::error( array( "data" => array( "message" => $message)) ); - exit; - } - } - } - - \OC_JSON::success(array("data" => array( "message" => $l->t("Saved") ))); - } - - /** - * Send a mail to test the settings - */ - public static function sendTestMail() { - \OC_Util::checkAdminUser(); - \OCP\JSON::callCheck(); - - $l = \OC::$server->getL10N('settings'); - $email = \OC_Preferences::getValue(\OC_User::getUser(), 'settings', 'email', ''); - if (!empty($email)) { - $defaults = new \OC_Defaults(); - - try { - \OC_Mail::send($email, \OC_User::getDisplayName(), - $l->t('test email settings'), - $l->t('If you received this email, the settings seem to be correct.'), - \OCP\Util::getDefaultEmailAddress('no-reply'), $defaults->getName()); - } catch (\Exception $e) { - $message = $l->t('A problem occurred while sending the e-mail. Please revisit your settings.'); - \OC_JSON::error( array( "data" => array( "message" => $message)) ); - exit; - } - - \OC_JSON::success(array("data" => array( "message" => $l->t("Email sent") ))); - } else { - $message = $l->t('You need to set your user email before being able to send test emails.'); - \OC_JSON::error( array( "data" => array( "message" => $message)) ); - } - } - - /** - * Get the field name to use it in error messages - * - * @param string $setting - * @param \OC_L10N $l - * @return string - */ - public static function getFieldname($setting, $l) { - switch ($setting) { - case 'mail_smtpmode': - return $l->t( 'Send mode' ); - case 'mail_smtpsecure': - return $l->t( 'Encryption' ); - case 'mail_smtpauthtype': - return $l->t( 'Authentication method' ); - } - } -} diff --git a/settings/application.php b/settings/application.php new file mode 100644 index 0000000000..b17ca01c2f --- /dev/null +++ b/settings/application.php @@ -0,0 +1,70 @@ +getContainer(); + + /** + * Controllers + */ + $container->registerService('MailSettingsController', function(SimpleContainer $c) { + return new MailSettingsController( + $c->query('AppName'), + $c->query('Request'), + $c->query('L10N'), + $c->query('Config'), + $c->query('UserSession'), + $c->query('Defaults'), + $c->query('Mail'), + $c->query('DefaultMailAddress') + ); + }); + + /** + * Core class wrappers + */ + $container->registerService('Config', function(SimpleContainer $c) { + return $c->query('ServerContainer')->getConfig(); + }); + $container->registerService('L10N', function(SimpleContainer $c) { + return $c->query('ServerContainer')->getL10N('settings'); + }); + $container->registerService('UserSession', function(SimpleContainer $c) { + return $c->query('ServerContainer')->getUserSession(); + }); + $container->registerService('Mail', function(SimpleContainer $c) { + return new \OC_Mail; + }); + $container->registerService('Defaults', function(SimpleContainer $c) { + return new \OC_Defaults; + }); + $container->registerService('DefaultMailAddress', function(SimpleContainer $c) { + return Util::getDefaultEmailAddress('no-reply'); + }); + } +} diff --git a/settings/controller/mailsettingscontroller.php b/settings/controller/mailsettingscontroller.php new file mode 100644 index 0000000000..1cfb10c6fe --- /dev/null +++ b/settings/controller/mailsettingscontroller.php @@ -0,0 +1,166 @@ +l10n = $l10n; + $this->config = $config; + $this->userSession = $userSession; + $this->defaults = $defaults; + $this->mail = $mail; + $this->defaultMailAddress = $defaultMailAddress; + } + + /** + * Sets the email settings + * @param string $mail_domain + * @param string $mail_from_address + * @param string $mail_smtpmode + * @param string $mail_smtpsecure + * @param string $mail_smtphost + * @param string $mail_smtpauthtype + * @param int $mail_smtpauth + * @param string $mail_smtpport + * @return array + */ + public function setMailSettings($mail_domain, + $mail_from_address, + $mail_smtpmode, + $mail_smtpsecure, + $mail_smtphost, + $mail_smtpauthtype, + $mail_smtpauth, + $mail_smtpport) { + + $params = get_defined_vars(); + foreach($params as $key => $value) { + if(empty($value)) { + $this->config->deleteSystemValue($key); + } else { + $this->config->setSystemValue($key, $value); + } + } + + // Delete passwords from config in case no auth is specified + if($params['mail_smtpauth'] !== 1) { + $this->config->deleteSystemValue('mail_smtpname'); + $this->config->deleteSystemValue('mail_smtppassword'); + } + + return array('data' => + array('message' => + (string) $this->l10n->t('Saved') + ), + 'status' => 'success' + ); + } + + /** + * Store the credentials used for SMTP in the config + * @param string $mail_smtpname + * @param string $mail_smtppassword + * @return array + */ + public function storeCredentials($mail_smtpname, $mail_smtppassword) { + $this->config->setSystemValue('mail_smtpname', $mail_smtpname); + $this->config->setSystemValue('mail_smtppassword', $mail_smtppassword); + + return array('data' => + array('message' => + (string) $this->l10n->t('Saved') + ), + 'status' => 'success' + ); + } + + /** + * Send a mail to test the settings + * @return array + */ + public function sendTestMail() { + $email = $this->config->getUserValue($this->userSession->getUser()->getUID(), $this->appName, 'email', ''); + if (!empty($email)) { + try { + $this->mail->send($email, $this->userSession->getUser()->getDisplayName(), + $this->l10n->t('test email settings'), + $this->l10n->t('If you received this email, the settings seems to be correct.'), + $this->defaultMailAddress, + $this->defaults->getName() + ); + } catch (\Exception $e) { + return array('data' => + array('message' => + (string) $this->l10n->t('A problem occurred while sending the e-mail. Please revisit your settings.'), + ), + 'status' => 'error' + ); + } + + return array('data' => + array('message' => + (string) $this->l10n->t('Email sent') + ), + 'status' => 'success' + ); + } + + return array('data' => + array('message' => + (string) $this->l10n->t('You need to set your user email before being able to send test emails.'), + ), + 'status' => 'error' + ); + } + +} diff --git a/settings/css/settings.css b/settings/css/settings.css index 581904591d..d89c50e411 100644 --- a/settings/css/settings.css +++ b/settings/css/settings.css @@ -178,12 +178,12 @@ span.securitywarning, span.connectionwarning, .setupwarning { padding-left: 56px; } -#mail_settings p label:first-child { +.mail_settings p label:first-child { display: inline-block; width: 300px; text-align: right; } -#mail_settings p select:nth-child(2) { +.mail_settings p select:nth-child(2) { width: 143px; } #mail_smtpport { diff --git a/settings/js/admin.js b/settings/js/admin.js index d8cdae9d11..09e8a1d691 100644 --- a/settings/js/admin.js +++ b/settings/js/admin.js @@ -103,14 +103,22 @@ $(document).ready(function(){ } }); - $('#mail_settings').change(function(){ + $('#mail_general_settings').change(function(){ OC.msg.startSaving('#mail_settings_msg'); - var post = $( "#mail_settings" ).serialize(); + var post = $( "#mail_general_settings" ).serialize(); $.post(OC.generateUrl('/settings/admin/mailsettings'), post, function(data){ OC.msg.finishedSaving('#mail_settings_msg', data); }); }); + $('#mail_credentials_settings_submit').click(function(){ + OC.msg.startSaving('#mail_settings_msg'); + var post = $( "#mail_credentials_settings" ).serialize(); + $.post(OC.generateUrl('/settings/admin/mailsettings/credentials'), post, function(data){ + OC.msg.finishedSaving('#mail_settings_msg', data); + }); + }); + $('#sendtestemail').click(function(event){ event.preventDefault(); OC.msg.startAction('#sendtestmail_msg', t('settings', 'Sending...')); diff --git a/settings/routes.php b/settings/routes.php index 25a8b1da7e..7068c0df72 100644 --- a/settings/routes.php +++ b/settings/routes.php @@ -6,7 +6,16 @@ * See the COPYING-README file. */ -/** @var $this OCP\Route\IRouter */ +namespace OC\Settings; + +$application = new Application(); +$application->registerRoutes($this, array('routes' =>array( + array('name' => 'MailSettings#setMailSettings', 'url' => '/settings/admin/mailsettings', 'verb' => 'POST'), + array('name' => 'MailSettings#storeCredentials', 'url' => '/settings/admin/mailsettings/credentials', 'verb' => 'POST'), + array('name' => 'MailSettings#sendTestMail', 'url' => '/settings/admin/mailtest', 'verb' => 'POST'), +))); + +/** @var $this \OCP\Route\IRouter */ // Settings pages $this->create('settings_help', '/settings/help') @@ -88,12 +97,6 @@ $this->create('settings_ajax_getlog', '/settings/ajax/getlog.php') ->actionInclude('settings/ajax/getlog.php'); $this->create('settings_ajax_setloglevel', '/settings/ajax/setloglevel.php') ->actionInclude('settings/ajax/setloglevel.php'); -$this->create('settings_mail_settings', '/settings/admin/mailsettings') - ->post() - ->action('OC\Settings\Admin\Controller', 'setMailSettings'); -$this->create('settings_admin_mail_test', '/settings/admin/mailtest') - ->post() - ->action('OC\Settings\Admin\Controller', 'sendTestMail'); $this->create('settings_ajax_setsecurity', '/settings/ajax/setsecurity.php') ->actionInclude('settings/ajax/setsecurity.php'); $this->create('settings_ajax_excludegroups', '/settings/ajax/excludegroups.php') diff --git a/settings/templates/admin.php b/settings/templates/admin.php index d6bb298cae..6b4623173a 100644 --- a/settings/templates/admin.php +++ b/settings/templates/admin.php @@ -333,87 +333,90 @@ if ($_['suggestedOverwriteWebroot']) {

-
-

t('Email Server'));?>

+
+ +

t('Email Server'));?>

-

t('This is used for sending out notifications.')); ?>

+

t('This is used for sending out notifications.')); ?>

-

- - +

+ + - - -

+ + +

-

- - ' /> - @ - ' /> -

+

+ + ' /> + ' /> +

- + /> + +

- - - + + +
+ +

t( 'Test email settings' )); ?> -
+

t('Log'));?>

diff --git a/tests/phpunit-autotest.xml b/tests/phpunit-autotest.xml index 872ff2c259..3805bb1ac7 100644 --- a/tests/phpunit-autotest.xml +++ b/tests/phpunit-autotest.xml @@ -8,6 +8,7 @@ > lib/ + settings/ apps.php diff --git a/tests/settings/controller/mailsettingscontrollertest.php b/tests/settings/controller/mailsettingscontrollertest.php new file mode 100644 index 0000000000..5a1add9544 --- /dev/null +++ b/tests/settings/controller/mailsettingscontrollertest.php @@ -0,0 +1,196 @@ +container = $app->getContainer(); + $this->container['Config'] = $this->getMockBuilder('\OCP\IConfig') + ->disableOriginalConstructor()->getMock(); + $this->container['L10N'] = $this->getMockBuilder('\OCP\IL10N') + ->disableOriginalConstructor()->getMock(); + $this->container['AppName'] = 'settings'; + $this->container['UserSession'] = $this->getMockBuilder('\OC\User\Session') + ->disableOriginalConstructor()->getMock(); + $this->container['Mail'] = $this->getMockBuilder('\OC_Mail') + ->disableOriginalConstructor()->getMock(); + $this->container['Defaults'] = $this->getMockBuilder('\OC_Defaults') + ->disableOriginalConstructor()->getMock(); + $this->container['DefaultMailAddress'] = 'no-reply@owncloud.com'; + } + + public function testSetMailSettings() { + $this->container['L10N'] + ->expects($this->exactly(2)) + ->method('t') + ->will($this->returnValue('Saved')); + + /** + * FIXME: Use the following block once Jenkins uses PHPUnit >= 4.1 + */ + /* + $this->container['Config'] + ->expects($this->exactly(15)) + ->method('setSystemValue') + ->withConsecutive( + array($this->equalTo('mail_domain'), $this->equalTo('owncloud.com')), + array($this->equalTo('mail_from_address'), $this->equalTo('demo@owncloud.com')), + array($this->equalTo('mail_smtpmode'), $this->equalTo('smtp')), + array($this->equalTo('mail_smtpsecure'), $this->equalTo('ssl')), + array($this->equalTo('mail_smtphost'), $this->equalTo('mx.owncloud.org')), + array($this->equalTo('mail_smtpauthtype'), $this->equalTo('NTLM')), + array($this->equalTo('mail_smtpauth'), $this->equalTo(1)), + array($this->equalTo('mail_smtpport'), $this->equalTo('25')), + array($this->equalTo('mail_domain'), $this->equalTo('owncloud.com')), + array($this->equalTo('mail_from_address'), $this->equalTo('demo@owncloud.com')), + array($this->equalTo('mail_smtpmode'), $this->equalTo('smtp')), + array($this->equalTo('mail_smtpsecure'), $this->equalTo('ssl')), + array($this->equalTo('mail_smtphost'), $this->equalTo('mx.owncloud.org')), + array($this->equalTo('mail_smtpauthtype'), $this->equalTo('NTLM')), + array($this->equalTo('mail_smtpport'), $this->equalTo('25')) + ); + */ + + $this->container['Config'] + ->expects($this->exactly(15)) + ->method('setSystemValue'); + + /** + * FIXME: Use the following block once Jenkins uses PHPUnit >= 4.1 + */ + /* + $this->container['Config'] + ->expects($this->exactly(3)) + ->method('deleteSystemValue') + ->withConsecutive( + array($this->equalTo('mail_smtpauth')), + array($this->equalTo('mail_smtpname')), + array($this->equalTo('mail_smtppassword')) + ); + */ + $this->container['Config'] + ->expects($this->exactly(3)) + ->method('deleteSystemValue'); + + // With authentication + $response = $this->container['MailSettingsController']->setMailSettings( + 'owncloud.com', + 'demo@owncloud.com', + 'smtp', + 'ssl', + 'mx.owncloud.org', + 'NTLM', + 1, + '25' + ); + $expectedResponse = array('data' => array('message' =>'Saved'), 'status' => 'success'); + $this->assertSame($expectedResponse, $response); + + // Without authentication (testing the deletion of the stored password) + $response = $this->container['MailSettingsController']->setMailSettings( + 'owncloud.com', + 'demo@owncloud.com', + 'smtp', + 'ssl', + 'mx.owncloud.org', + 'NTLM', + 0, + '25' + ); + $expectedResponse = array('data' => array('message' =>'Saved'), 'status' => 'success'); + $this->assertSame($expectedResponse, $response); + + } + + public function testStoreCredentials() { + $this->container['L10N'] + ->expects($this->once()) + ->method('t') + ->will($this->returnValue('Saved')); + + /** + * FIXME: Use this block once Jenkins uses PHPUnit >= 4.1 + */ + /* + $this->container['Config'] + ->expects($this->exactly(2)) + ->method('setSystemValue') + ->withConsecutive( + array($this->equalTo('mail_smtpname'), $this->equalTo('UsernameToStore')), + array($this->equalTo('mail_smtppassword'), $this->equalTo('PasswordToStore')) + ); + */ + $this->container['Config'] + ->expects($this->exactly(2)) + ->method('setSystemValue'); + + $response = $this->container['MailSettingsController']->storeCredentials('UsernameToStore', 'PasswordToStore'); + $expectedResponse = array('data' => array('message' =>'Saved'), 'status' => 'success'); + + $this->assertSame($expectedResponse, $response); + } + + public function testSendTestMail() { + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor() + ->getMock(); + $user->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('Werner')); + $user->expects($this->any()) + ->method('getDisplayName') + ->will($this->returnValue('Werner Brösel')); + + $this->container['L10N'] + ->expects($this->any()) + ->method('t') + ->will( + $this->returnValueMap( + array( + array('You need to set your user email before being able to send test emails.', array(), + 'You need to set your user email before being able to send test emails.'), + array('A problem occurred while sending the e-mail. Please revisit your settings.', array(), + 'A problem occurred while sending the e-mail. Please revisit your settings.'), + array('Email sent', array(), 'Email sent'), + array('test email settings', array(), 'test email settings'), + array('If you received this email, the settings seems to be correct.', array(), + 'If you received this email, the settings seems to be correct.') + ) + )); + $this->container['UserSession'] + ->expects($this->any()) + ->method('getUser') + ->will($this->returnValue($user)); + + // Ensure that it fails when no mail address has been specified + $response = $this->container['MailSettingsController']->sendTestMail(); + $expectedResponse = array('data' => array('message' =>'You need to set your user email before being able to send test emails.'), 'status' => 'error'); + $this->assertSame($expectedResponse, $response); + + // If no exception is thrown it should work + $this->container['Config'] + ->expects($this->any()) + ->method('getUserValue') + ->will($this->returnValue('mail@example.invalid')); + $response = $this->container['MailSettingsController']->sendTestMail(); + $expectedResponse = array('data' => array('message' =>'Email sent'), 'status' => 'success'); + $this->assertSame($expectedResponse, $response); + } + +} \ No newline at end of file From d3eebad59104355bb6e07c9f63a6fa868f5a2c4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Tue, 14 Oct 2014 04:49:29 +0200 Subject: [PATCH 2/2] fixing typos --- settings/controller/mailsettingscontroller.php | 2 +- settings/templates/admin.php | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/settings/controller/mailsettingscontroller.php b/settings/controller/mailsettingscontroller.php index 1cfb10c6fe..583aa98dc8 100644 --- a/settings/controller/mailsettingscontroller.php +++ b/settings/controller/mailsettingscontroller.php @@ -141,7 +141,7 @@ class MailSettingsController extends Controller { } catch (\Exception $e) { return array('data' => array('message' => - (string) $this->l10n->t('A problem occurred while sending the e-mail. Please revisit your settings.'), + (string) $this->l10n->t('A problem occurred while sending the email. Please revise your settings.'), ), 'status' => 'error' ); diff --git a/settings/templates/admin.php b/settings/templates/admin.php index 6b4623173a..2ea5d82490 100644 --- a/settings/templates/admin.php +++ b/settings/templates/admin.php @@ -6,6 +6,7 @@ */ /** * @var array $_ + * @var \OCP\IL10N $l */ $levels = array('Debug', 'Info', 'Warning', 'Error', 'Fatal'); $levelLabels = array(