Merge pull request #4372 from nextcloud/smtp-password

Don't put the SMTP password into the HTML code

6 files changed, 113 insertions, 134 deletions

diff --git a/lib/private/Settings/Admin/Additional.php b/lib/private/Settings/Admin/Additional.php
index d133e4737a7..59058851a64 100644
--- a/lib/private/Settings/Admin/Additional.php
+++ b/lib/private/Settings/Admin/Additional.php
@@ -65,6 +65,10 @@ class Additional implements ISettings {
 			'mail_smtppassword'     => $this->config->getSystemValue('mail_smtppassword', ''),
 		];
 
+		if ($parameters['mail_smtppassword'] !== '') {
+			$parameters['mail_smtppassword'] = '********';
+		}
+
 		return new TemplateResponse('settings', 'admin/additional-mail', $parameters, '');
 	}
 
diff --git a/settings/Controller/MailSettingsController.php b/settings/Controller/MailSettingsController.php
index 8137b4da53c..f0fd7a52f0b 100644
--- a/settings/Controller/MailSettingsController.php
+++ b/settings/Controller/MailSettingsController.php
@@ -1,5 +1,6 @@
 <?php
 /**
+ * @copyright Copyright (c) 2017  Joas Schilling <coding@schilljs.com>
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
  * @author Joas Schilling <coding@schilljs.com>
@@ -25,6 +26,8 @@
 namespace OC\Settings\Controller;
 
 use OCP\AppFramework\Controller;
+use OCP\AppFramework\Http;
+use OCP\AppFramework\Http\DataResponse;
 use OCP\IRequest;
 use OCP\IL10N;
 use OCP\IConfig;
@@ -84,7 +87,7 @@ class MailSettingsController extends Controller {
 	 * @param string $mail_smtpauthtype
 	 * @param int $mail_smtpauth
 	 * @param string $mail_smtpport
-	 * @return array
+	 * @return DataResponse
 	 */
 	public function setMailSettings($mail_domain,
 									$mail_from_address,
@@ -109,12 +112,7 @@ class MailSettingsController extends Controller {
 
 		$this->config->setSystemValues($configs);
 
-		return array('data' =>
-			array('message' =>
-				(string) $this->l10n->t('Saved')
-			),
-			'status' => 'success'
-		);
+		return new DataResponse();
 	}
 
 	/**
@@ -124,25 +122,24 @@ class MailSettingsController extends Controller {
 	 *
 	 * @param string $mail_smtpname
 	 * @param string $mail_smtppassword
-	 * @return array
+	 * @return DataResponse
 	 */
 	public function storeCredentials($mail_smtpname, $mail_smtppassword) {
+		if ($mail_smtppassword === '********') {
+			return new DataResponse($this->l10n->t('Invalid SMTP password.'), Http::STATUS_BAD_REQUEST);
+		}
+
 		$this->config->setSystemValues([
 			'mail_smtpname'		=> $mail_smtpname,
 			'mail_smtppassword'	=> $mail_smtppassword,
 		]);
 
-		return array('data' =>
-			array('message' =>
-				(string) $this->l10n->t('Saved')
-			),
-			'status' => 'success'
-		);
+		return new DataResponse();
 	}
 
 	/**
 	 * Send a mail to test the settings
-	 * @return array
+	 * @return DataResponse
 	 */
 	public function sendTestMail() {
 		$email = $this->config->getUserValue($this->userSession->getUser()->getUID(), $this->appName, 'email', '');
@@ -157,29 +154,13 @@ class MailSettingsController extends Controller {
 				if (!empty($errors)) {
 					throw new \RuntimeException($this->l10n->t('Mail could not be sent. Check your mail server log'));
 				}
+				return new DataResponse();
 			} catch (\Exception $e) {
-				return [
-					'data' => [
-						'message' => (string) $this->l10n->t('A problem occurred while sending the email. Please revise your settings. (Error: %s)', [$e->getMessage()]),
-					],
-					'status' => 'error',
-				];
+				return new DataResponse($this->l10n->t('A problem occurred while sending the email. Please revise your settings. (Error: %s)', [$e->getMessage()]), Http::STATUS_BAD_REQUEST);
 			}
-
-			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'
-		);
+		return new DataResponse($this->l10n->t('You need to set your user email before being able to send test emails.'), Http::STATUS_BAD_REQUEST);
 	}
 
 }
diff --git a/settings/js/admin.js b/settings/js/admin.js
index 985e318e34b..475fecf604e 100644
--- a/settings/js/admin.js
+++ b/settings/js/admin.js
@@ -186,11 +186,11 @@ $(document).ready(function(){
 			url: OC.generateUrl('/settings/admin/mailsettings'),
 			type: 'POST',
 			data: $('#mail_general_settings_form').serialize(),
-			success: function(data){
-				OC.msg.finishedSaving('#mail_settings_msg', data);
+			success: function(){
+				OC.msg.finishedSuccess('#mail_settings_msg', t('settings', 'Saved'));
 			},
-			error: function(data){
-				OC.msg.finishedError('#mail_settings_msg', data.responseJSON.message);
+			error: function(xhr){
+				OC.msg.finishedError('#mail_settings_msg', xhr.responseJSON);
 			}
 		});
 	};
@@ -206,21 +206,39 @@ $(document).ready(function(){
 			url: OC.generateUrl('/settings/admin/mailsettings/credentials'),
 			type: 'POST',
 			data: $('#mail_credentials_settings').serialize(),
-			success: function(data){
-				OC.msg.finishedSaving('#mail_settings_msg', data);
+			success: function(){
+				OC.msg.finishedSuccess('#mail_settings_msg', t('settings', 'Saved'));
 			},
-			error: function(data){
-				OC.msg.finishedError('#mail_settings_msg', data.responseJSON.message);
+			error: function(xhr){
+				OC.msg.finishedError('#mail_settings_msg', xhr.responseJSON);
 			}
 		});
 	};
 
 	$('#mail_general_settings_form').change(changeEmailSettings);
 	$('#mail_credentials_settings_submit').click(toggleEmailCredentials);
+	$('#mail_smtppassword').click(function() {
+		if (this.type === 'text' && this.value === '********') {
+			this.type = 'password';
+			this.value = '';
+		}
+	});
 
 	$('#sendtestemail').click(function(event){
 		event.preventDefault();
-		OC.msg.startAction('#sendtestmail_msg', t('settings', 'Sending...'));
+		OC.msg.startAction('#sendtestmail_msg', t('settings', 'Sending…'));
+
+		$.ajax({
+			url: OC.generateUrl('/settings/admin/mailtest'),
+			type: 'POST',
+			data: $('#mail_credentials_settings').serialize(),
+			success: function(){
+				OC.msg.finishedSuccess('#sendtestmail_msg', t('settings', 'Email sent'));
+			},
+			error: function(xhr){
+				OC.msg.finishedError('#sendtestmail_msg', xhr.responseJSON);
+			}
+		});
 		$.post(OC.generateUrl('/settings/admin/mailtest'), '', function(data){
 			OC.msg.finishedAction('#sendtestmail_msg', data);
 		});
diff --git a/settings/templates/admin/additional-mail.php b/settings/templates/admin/additional-mail.php
index 23723a423c0..7f8706274f9 100644
--- a/settings/templates/admin/additional-mail.php
+++ b/settings/templates/admin/additional-mail.php
@@ -44,7 +44,7 @@ $mail_smtpmode = [
 if ($_['sendmail_is_available']) {
 	$mail_smtpmode[] = ['sendmail', 'Sendmail'];
 }
-if ($_['mail_smtpmode'] == 'qmail') {
+if ($_['mail_smtpmode'] === 'qmail') {
 	$mail_smtpmode[] = ['qmail', 'qmail'];
 }
 
@@ -60,81 +60,81 @@ if ($_['mail_smtpmode'] == 'qmail') {
 		<p><?php p($l->t('This is used for sending out notifications.')); ?> <span id="mail_settings_msg" class="msg"></span></p>
 
 		<p>
-			<label for="mail_smtpmode"><?php p($l->t( 'Send mode' )); ?></label>
-			<select name='mail_smtpmode' id='mail_smtpmode'>
+			<label for="mail_smtpmode"><?php p($l->t('Send mode')); ?></label>
+			<select name="mail_smtpmode" id="mail_smtpmode'>
 				<?php foreach ($mail_smtpmode as $smtpmode):
 					$selected = '';
 					if ($smtpmode[0] == $_['mail_smtpmode']):
 						$selected = 'selected="selected"';
 					endif; ?>
-					<option value='<?php p($smtpmode[0])?>' <?php p($selected) ?>><?php p($smtpmode[1]) ?></option>
+					<option value="<?php p($smtpmode[0])?>" <?php p($selected) ?>><?php p($smtpmode[1]) ?></option>
 				<?php endforeach;?>
 			</select>
 
 			<label id="mail_smtpsecure_label" for="mail_smtpsecure"
-				<?php if ($_['mail_smtpmode'] != 'smtp') print_unescaped(' class="hidden"'); ?>>
-				<?php p($l->t( 'Encryption' )); ?>
+				<?php if ($_['mail_smtpmode'] !== 'smtp') print_unescaped(' class="hidden"'); ?>>
+				<?php p($l->t('Encryption')); ?>
 			</label>
 			<select name="mail_smtpsecure" id="mail_smtpsecure"
-				<?php if ($_['mail_smtpmode'] != 'smtp') print_unescaped(' class="hidden"'); ?>>
+				<?php if ($_['mail_smtpmode'] !== 'smtp') print_unescaped(' class="hidden"'); ?>>
 				<?php foreach ($mail_smtpsecure as $secure => $name):
 					$selected = '';
 					if ($secure == $_['mail_smtpsecure']):
 						$selected = 'selected="selected"';
 					endif; ?>
-					<option value='<?php p($secure)?>' <?php p($selected) ?>><?php p($name) ?></option>
+					<option value="<?php p($secure)?>" <?php p($selected) ?>><?php p($name) ?></option>
 				<?php endforeach;?>
 			</select>
 		</p>
 
 		<p>
-			<label for="mail_from_address"><?php p($l->t( 'From address' )); ?></label>
-			<input type="text" name='mail_from_address' id="mail_from_address" placeholder="<?php p($l->t('mail'))?>"
-				   value='<?php p($_['mail_from_address']) ?>' />@
-			<input type="text" name='mail_domain' id="mail_domain" placeholder="example.com"
-				   value='<?php p($_['mail_domain']) ?>' />
+			<label for="mail_from_address"><?php p($l->t('From address')); ?></label>
+			<input type="text" name="mail_from_address" id="mail_from_address" placeholder="<?php p($l->t('mail'))?>"
+				   value="<?php p($_['mail_from_address']) ?>" />@
+			<input type="text" name="mail_domain" id="mail_domain" placeholder="example.com"
+				   value="<?php p($_['mail_domain']) ?>" />
 		</p>
 
-		<p id="setting_smtpauth" <?php if ($_['mail_smtpmode'] != 'smtp') print_unescaped(' class="hidden"'); ?>>
-			<label for="mail_smtpauthtype"><?php p($l->t( 'Authentication method' )); ?></label>
-			<select name='mail_smtpauthtype' id='mail_smtpauthtype'>
+		<p id="setting_smtpauth" <?php if ($_['mail_smtpmode'] !== 'smtp') print_unescaped(' class="hidden"'); ?>>
+			<label for="mail_smtpauthtype"><?php p($l->t('Authentication method')); ?></label>
+			<select name="mail_smtpauthtype" id="mail_smtpauthtype'>
 				<?php foreach ($mail_smtpauthtype as $authtype => $name):
 					$selected = '';
 					if ($authtype == $_['mail_smtpauthtype']):
 						$selected = 'selected="selected"';
 					endif; ?>
-					<option value='<?php p($authtype)?>' <?php p($selected) ?>><?php p($name) ?></option>
+					<option value="<?php p($authtype)?>" <?php p($selected) ?>><?php p($name) ?></option>
 				<?php endforeach;?>
 			</select>
 
 			<input type="checkbox" name="mail_smtpauth" id="mail_smtpauth" class="checkbox" value="1"
 				<?php if ($_['mail_smtpauth']) print_unescaped('checked="checked"'); ?> />
-			<label for="mail_smtpauth"><?php p($l->t( 'Authentication required' )); ?></label>
+			<label for="mail_smtpauth"><?php p($l->t('Authentication required')); ?></label>
 		</p>
 
-		<p id="setting_smtphost" <?php if ($_['mail_smtpmode'] != 'smtp') print_unescaped(' class="hidden"'); ?>>
-			<label for="mail_smtphost"><?php p($l->t( 'Server address' )); ?></label>
-			<input type="text" name='mail_smtphost' id="mail_smtphost" placeholder="smtp.example.com"
-				   value='<?php p($_['mail_smtphost']) ?>' />
+		<p id="setting_smtphost" <?php if ($_['mail_smtpmode'] !== 'smtp') print_unescaped(' class="hidden"'); ?>>
+			<label for="mail_smtphost"><?php p($l->t('Server address')); ?></label>
+			<input type="text" name="mail_smtphost" id="mail_smtphost" placeholder="smtp.example.com"
+				   value="<?php p($_['mail_smtphost']) ?>" />
 			:
-			<input type="text" name='mail_smtpport' id="mail_smtpport" placeholder="<?php p($l->t('Port'))?>"
-				   value='<?php p($_['mail_smtpport']) ?>' />
+			<input type="text" name="mail_smtpport" id="mail_smtpport" placeholder="<?php p($l->t('Port'))?>"
+				   value="<?php p($_['mail_smtpport']) ?>" />
 		</p>
 	</form>
 	<form class="mail_settings" id="mail_credentials_settings">
-		<p id="mail_credentials" <?php if (!$_['mail_smtpauth'] || $_['mail_smtpmode'] != 'smtp') print_unescaped(' class="hidden"'); ?>>
-			<label for="mail_smtpname"><?php p($l->t( 'Credentials' )); ?></label>
-			<input type="text" name='mail_smtpname' id="mail_smtpname" placeholder="<?php p($l->t('SMTP Username'))?>"
-				   value='<?php p($_['mail_smtpname']) ?>' />
-			<input type="password" name='mail_smtppassword' id="mail_smtppassword" autocomplete="off"
-				   placeholder="<?php p($l->t('SMTP Password'))?>" value='<?php p($_['mail_smtppassword']) ?>' />
+		<p id="mail_credentials" <?php if (!$_['mail_smtpauth'] || $_['mail_smtpmode'] !== 'smtp') print_unescaped(' class="hidden"'); ?>>
+			<label for="mail_smtpname"><?php p($l->t('Credentials')); ?></label>
+			<input type="text" name="mail_smtpname" id="mail_smtpname" placeholder="<?php p($l->t('SMTP Username'))?>"
+				   value="<?php p($_['mail_smtpname']) ?>" />
+			<input type="text" name="mail_smtppassword" id="mail_smtppassword" autocomplete="off"
+				   placeholder="<?php p($l->t('SMTP Password'))?>" value="<?php p($_['mail_smtppassword']) ?>" />
 			<input id="mail_credentials_settings_submit" type="button" value="<?php p($l->t('Store credentials')) ?>">
 		</p>
 	</form>
 
 	<br />
-	<em><?php p($l->t( 'Test email settings' )); ?></em>
-	<input type="submit" name="sendtestemail" id="sendtestemail" value="<?php p($l->t( 'Send email' )); ?>"/>
+	<em><?php p($l->t('Test email settings')); ?></em>
+	<input type="submit" name="sendtestemail" id="sendtestemail" value="<?php p($l->t('Send email')); ?>"/>
 	<span id="sendtestmail_msg" class="msg"></span>
 </div>
 
diff --git a/tests/Settings/Controller/MailSettingsControllerTest.php b/tests/Settings/Controller/MailSettingsControllerTest.php
index 2012de886c8..c8f9e476801 100644
--- a/tests/Settings/Controller/MailSettingsControllerTest.php
+++ b/tests/Settings/Controller/MailSettingsControllerTest.php
@@ -1,7 +1,10 @@
 <?php
 /**
- * @author Lukas Reschke
- * @copyright 2014 Lukas Reschke lukas@owncloud.com
+ * @copyright 2014 Lukas Reschke lukas@nextcloud.com
+ * @copyright Copyright (c) 2017  Joas Schilling <coding@schilljs.com>
+ *
+ * @author Lukas Reschke <lukas@statuscode.ch>
+ * @author Joas Schilling <coding@schilljs.com>
  *
  * This file is licensed under the Affero General Public License version 3 or
  * later.
@@ -12,11 +15,13 @@ namespace Tests\Settings\Controller;
 
 use OC\Mail\Message;
 use OC\Settings\Controller\MailSettingsController;
+use OCP\AppFramework\Http;
 use OCP\IConfig;
 use OCP\IL10N;
 use OCP\IRequest;
 use OCP\IUserSession;
 use OCP\Mail\IMailer;
+use OC\User\User;
 
 /**
  * @package Tests\Settings\Controller
@@ -42,45 +47,39 @@ class MailSettingsControllerTest extends \Test\TestCase {
 		$this->config = $this->createMock(IConfig::class);
 		$this->userSession = $this->createMock(IUserSession::class);
 		$this->mailer = $this->createMock(IMailer::class);
-//		$this->mailer = $this->getMockBuilder(IMailer::class)
-//			->setMethods(['send'])
-//			->getMock();
+		/** @var IRequest|\PHPUnit_Framework_MockObject_MockObject $request */
+		$request = $this->createMock(IRequest::class);
 		$this->mailController = new MailSettingsController(
 			'settings',
-			$this->createMock(IRequest::class),
+			$request,
 			$this->l,
 			$this->config,
 			$this->userSession,
 			$this->mailer,
-			'no-reply@owncloud.com'
+			'no-reply@nextcloud.com'
 		);
 	}
 
 	public function testSetMailSettings() {
-		$this->l
-			->expects($this->exactly(2))
-			->method('t')
-			->will($this->returnValue('Saved'));
-
 		$this->config->expects($this->exactly(2))
 			->method('setSystemValues')
 			->withConsecutive(
 				[[
-					'mail_domain' => 'owncloud.com',
-					'mail_from_address' => 'demo@owncloud.com',
+					'mail_domain' => 'nextcloud.com',
+					'mail_from_address' => 'demo@nextcloud.com',
 					'mail_smtpmode' => 'smtp',
 					'mail_smtpsecure' => 'ssl',
-					'mail_smtphost' => 'mx.owncloud.org',
+					'mail_smtphost' => 'mx.nextcloud.org',
 					'mail_smtpauthtype' => 'NTLM',
 					'mail_smtpauth' => 1,
 					'mail_smtpport' => '25',
 				]],
 				[[
-					'mail_domain' => 'owncloud.com',
-					'mail_from_address' => 'demo@owncloud.com',
+					'mail_domain' => 'nextcloud.com',
+					'mail_from_address' => 'demo@nextcloud.com',
 					'mail_smtpmode' => 'smtp',
 					'mail_smtpsecure' => 'ssl',
-					'mail_smtphost' => 'mx.owncloud.org',
+					'mail_smtphost' => 'mx.nextcloud.org',
 					'mail_smtpauthtype' => 'NTLM',
 					'mail_smtpauth' => null,
 					'mail_smtpport' => '25',
@@ -91,40 +90,33 @@ class MailSettingsControllerTest extends \Test\TestCase {
 
 		// With authentication
 		$response = $this->mailController->setMailSettings(
-			'owncloud.com',
-			'demo@owncloud.com',
+			'nextcloud.com',
+			'demo@nextcloud.com',
 			'smtp',
 			'ssl',
-			'mx.owncloud.org',
+			'mx.nextcloud.org',
 			'NTLM',
 			1,
 			'25'
 		);
-		$expectedResponse = array('data' => array('message' =>'Saved'), 'status' => 'success');
-		$this->assertSame($expectedResponse, $response);
+		$this->assertSame(Http::STATUS_OK, $response->getStatus());
 
 		// Without authentication (testing the deletion of the stored password)
 		$response = $this->mailController->setMailSettings(
-			'owncloud.com',
-			'demo@owncloud.com',
+			'nextcloud.com',
+			'demo@nextcloud.com',
 			'smtp',
 			'ssl',
-			'mx.owncloud.org',
+			'mx.nextcloud.org',
 			'NTLM',
 			0,
 			'25'
 		);
-		$expectedResponse = array('data' => array('message' =>'Saved'), 'status' => 'success');
-		$this->assertSame($expectedResponse, $response);
+		$this->assertSame(Http::STATUS_OK, $response->getStatus());
 
 	}
 
 	public function testStoreCredentials() {
-		$this->l
-			->expects($this->once())
-			->method('t')
-			->will($this->returnValue('Saved'));
-
 		$this->config
 			->expects($this->once())
 			->method('setSystemValues')
@@ -134,15 +126,11 @@ class MailSettingsControllerTest extends \Test\TestCase {
 			]);
 
 		$response = $this->mailController->storeCredentials('UsernameToStore', 'PasswordToStore');
-		$expectedResponse = array('data' => array('message' =>'Saved'), 'status' => 'success');
-
-		$this->assertSame($expectedResponse, $response);
+		$this->assertSame(Http::STATUS_OK, $response->getStatus());
 	}
 
 	public function testSendTestMail() {
-		$user = $this->getMockBuilder('\OC\User\User')
-			->disableOriginalConstructor()
-			->getMock();
+		$user = $this->createMock(User::class);
 		$user->expects($this->any())
 			->method('getUID')
 			->will($this->returnValue('Werner'));
@@ -150,22 +138,11 @@ class MailSettingsControllerTest extends \Test\TestCase {
 			->method('getDisplayName')
 			->will($this->returnValue('Werner Brösel'));
 
-		$this->l
-			->expects($this->any())
+		$this->l->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 seem to be correct.', array(),
-							'If you received this email, the settings seem to be correct.')
-					)
-				));
+			->willReturnCallback(function($text, $parameters = []) {
+				return vsprintf($text, $parameters);
+			});
 		$this->userSession
 			->expects($this->any())
 			->method('getUser')
@@ -173,8 +150,8 @@ class MailSettingsControllerTest extends \Test\TestCase {
 
 		// Ensure that it fails when no mail address has been specified
 		$response = $this->mailController->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);
+		$this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus());
+		$this->assertSame('You need to set your user email before being able to send test emails.', $response->getData());
 
 		// If no exception is thrown it should work
 		$this->config
@@ -185,8 +162,7 @@ class MailSettingsControllerTest extends \Test\TestCase {
 			->method('createMessage')
 			->willReturn($this->createMock(Message::class));
 		$response = $this->mailController->sendTestMail();
-		$expectedResponse = array('data' => array('message' =>'Email sent'), 'status' => 'success');
-		$this->assertSame($expectedResponse, $response);
+		$this->assertSame(Http::STATUS_OK, $response->getStatus(), $response->getData());
 	}
 
 }
diff --git a/tests/lib/Settings/Admin/AdditionalTest.php b/tests/lib/Settings/Admin/AdditionalTest.php
index 3a99893cf7c..420a7110c13 100644
--- a/tests/lib/Settings/Admin/AdditionalTest.php
+++ b/tests/lib/Settings/Admin/AdditionalTest.php
@@ -109,7 +109,7 @@ class AdditionalTest extends TestCase {
 				'mail_smtpauthtype'     => 'login',
 				'mail_smtpauth'         => true,
 				'mail_smtpname'         => 'smtp.sender.com',
-				'mail_smtppassword'     => 'mypassword',
+				'mail_smtppassword'     => '********',
 			],
 			''
 		);