From 84cf39f58248cb8fc441d00802b1f38f268de4a1 Mon Sep 17 00:00:00 2001 From: szaimen Date: Thu, 3 Feb 2022 11:43:17 +0100 Subject: [PATCH 1/3] show if the mail server settings are not set or verified Signed-off-by: szaimen --- .../lib/Controller/CheckSetupController.php | 15 +++++++++++++++ .../lib/Controller/MailSettingsController.php | 17 ++++++++++++++++- .../settings/admin/additional-mail.php | 2 +- core/js/setupchecks.js | 8 ++++++++ 4 files changed, 40 insertions(+), 2 deletions(-) diff --git a/apps/settings/lib/Controller/CheckSetupController.php b/apps/settings/lib/Controller/CheckSetupController.php index ac734e5eb780f..3a8b9bfd4a5d2 100644 --- a/apps/settings/lib/Controller/CheckSetupController.php +++ b/apps/settings/lib/Controller/CheckSetupController.php @@ -560,6 +560,20 @@ protected function isReadOnlyConfig(): bool { return \OC_Helper::isReadOnlyConfigEnabled(); } + protected function wasEmailTestSuccessful(): bool { + // Handle the case that the configuration was set before the check was introduced or it was only set via command line and not from the UI + if ($this->config->getAppValue('core', 'emailTestSuccessful', '') === '' && $this->config->getSystemValue('mail_domain', '') === '') { + return false; + } + + // The mail test was unsuccessful or the config was changed using the UI without verifying with a testmail, hence return false + if ($this->config->getAppValue('core', 'emailTestSuccessful', '') === '0') { + return false; + } + + return true; + } + protected function hasValidTransactionIsolationLevel(): bool { try { if ($this->db->getDatabasePlatform() instanceof SqlitePlatform) { @@ -809,6 +823,7 @@ public function check() { 'isGetenvServerWorking' => !empty(getenv('PATH')), 'isReadOnlyConfig' => $this->isReadOnlyConfig(), 'hasValidTransactionIsolationLevel' => $this->hasValidTransactionIsolationLevel(), + 'wasEmailTestSuccessful' => $this->wasEmailTestSuccessful(), 'hasFileinfoInstalled' => $this->hasFileinfoInstalled(), 'hasWorkingFileLocking' => $this->hasWorkingFileLocking(), 'suggestedOverwriteCliURL' => $this->getSuggestedOverwriteCliURL(), diff --git a/apps/settings/lib/Controller/MailSettingsController.php b/apps/settings/lib/Controller/MailSettingsController.php index 04143cc7fa7d9..22c0622a0722d 100644 --- a/apps/settings/lib/Controller/MailSettingsController.php +++ b/apps/settings/lib/Controller/MailSettingsController.php @@ -33,6 +33,7 @@ use OCP\IConfig; use OCP\IL10N; use OCP\IRequest; +use OCP\IURLGenerator; use OCP\IUserSession; use OCP\Mail\IMailer; @@ -46,6 +47,8 @@ class MailSettingsController extends Controller { private $userSession; /** @var IMailer */ private $mailer; + /** @var IURLGenerator */ + private $urlGenerator; /** * @param string $appName @@ -53,6 +56,7 @@ class MailSettingsController extends Controller { * @param IL10N $l10n * @param IConfig $config * @param IUserSession $userSession + * @param IURLGenerator $urlGenerator, * @param IMailer $mailer */ public function __construct($appName, @@ -60,11 +64,13 @@ public function __construct($appName, IL10N $l10n, IConfig $config, IUserSession $userSession, + IURLGenerator $urlGenerator, IMailer $mailer) { parent::__construct($appName, $request); $this->l10n = $l10n; $this->config = $config; $this->userSession = $userSession; + $this->urlGenerator = $urlGenerator; $this->mailer = $mailer; } @@ -107,6 +113,8 @@ public function setMailSettings($mail_domain, $this->config->setSystemValues($configs); + $this->config->setAppValue('core', 'emailTestSuccessful', '0'); + return new DataResponse(); } @@ -130,6 +138,8 @@ public function storeCredentials($mail_smtpname, $mail_smtppassword) { 'mail_smtppassword' => $mail_smtppassword, ]); + $this->config->setAppValue('core', 'emailTestSuccessful', '0'); + return new DataResponse(); } @@ -159,14 +169,19 @@ public function sendTestMail() { $message->useTemplate($template); $errors = $this->mailer->send($message); if (!empty($errors)) { + $this->config->setAppValue('core', 'emailTestSuccessful', '0'); throw new \RuntimeException($this->l10n->t('Email could not be sent. Check your mail server log')); } + // Store the successful config in the app config + $this->config->setAppValue('core', 'emailTestSuccessful', '1'); return new DataResponse(); } catch (\Exception $e) { + $this->config->setAppValue('core', 'emailTestSuccessful', '0'); 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 new DataResponse($this->l10n->t('You need to set your user email before being able to send test emails.'), Http::STATUS_BAD_REQUEST); + $this->config->setAppValue('core', 'emailTestSuccessful', '0'); + return new DataResponse($this->l10n->t('You need to set your user email before being able to send test emails. Go to %s for that.', [$this->urlGenerator->linkToRouteAbsolute('settings.PersonalSettings.index')]), Http::STATUS_BAD_REQUEST); } } diff --git a/apps/settings/templates/settings/admin/additional-mail.php b/apps/settings/templates/settings/admin/additional-mail.php index 82cd9e09b134b..a7e8382de183c 100644 --- a/apps/settings/templates/settings/admin/additional-mail.php +++ b/apps/settings/templates/settings/admin/additional-mail.php @@ -158,7 +158,7 @@
- t('Test email settings')); ?> + t('Test and verify email settings')); ?> diff --git a/core/js/setupchecks.js b/core/js/setupchecks.js index 59411d67b99b1..94bf4d5403c6e 100644 --- a/core/js/setupchecks.js +++ b/core/js/setupchecks.js @@ -195,6 +195,14 @@ type: OC.SetupChecks.MESSAGE_TYPE_INFO }); } + if (!data.wasEmailTestSuccessful) { + messages.push({ + msg: t('core', 'You have not set or verified your email server configuration, yet. Please head over to the {mailSettingsStart} Basic settings {mailSettingsEnd} in order to set them. Afterwards, use the "Send email" button below the form to verify your settings.',) + .replace('{mailSettingsStart} ', '') + .replace(' {mailSettingsEnd}', ''), + type: OC.SetupChecks.MESSAGE_TYPE_INFO + }); + } if (!data.hasValidTransactionIsolationLevel) { messages.push({ msg: t('core', 'Your database does not run with "READ COMMITTED" transaction isolation level. This can cause problems when multiple actions are executed in parallel.'), From fa1ee8f0c782ed55f60c91980670ee5a67727845 Mon Sep 17 00:00:00 2001 From: szaimen Date: Thu, 3 Feb 2022 12:28:37 +0100 Subject: [PATCH 2/3] fix tests Signed-off-by: szaimen --- .../Controller/CheckSetupControllerTest.php | 6 ++++++ .../Controller/MailSettingsControllerTest.php | 7 ++++++- core/js/tests/specs/setupchecksSpec.js | 18 ++++++++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/apps/settings/tests/Controller/CheckSetupControllerTest.php b/apps/settings/tests/Controller/CheckSetupControllerTest.php index d54c1bb226a7d..d746699106325 100644 --- a/apps/settings/tests/Controller/CheckSetupControllerTest.php +++ b/apps/settings/tests/Controller/CheckSetupControllerTest.php @@ -172,6 +172,7 @@ protected function setUp(): void { ]) ->setMethods([ 'isReadOnlyConfig', + 'wasEmailTestSuccessful', 'hasValidTransactionIsolationLevel', 'hasFileinfoInstalled', 'hasWorkingFileLocking', @@ -496,6 +497,10 @@ public function testCheck() { ->expects($this->once()) ->method('isReadOnlyConfig') ->willReturn(false); + $this->checkSetupController + ->expects($this->once()) + ->method('wasEmailTestSuccessful') + ->willReturn(false); $this->checkSetupController ->expects($this->once()) ->method('hasValidTransactionIsolationLevel') @@ -594,6 +599,7 @@ public function testCheck() { [ 'isGetenvServerWorking' => true, 'isReadOnlyConfig' => false, + 'wasEmailTestSuccessful' => false, 'hasValidTransactionIsolationLevel' => true, 'hasFileinfoInstalled' => true, 'hasWorkingFileLocking' => true, diff --git a/apps/settings/tests/Controller/MailSettingsControllerTest.php b/apps/settings/tests/Controller/MailSettingsControllerTest.php index 93fffadcfe15e..54461201201a0 100644 --- a/apps/settings/tests/Controller/MailSettingsControllerTest.php +++ b/apps/settings/tests/Controller/MailSettingsControllerTest.php @@ -38,6 +38,7 @@ use OCP\IUserSession; use OCP\Mail\IEMailTemplate; use OCP\Mail\IMailer; +use OCP\IURLGenerator; /** * @package Tests\Settings\Controller @@ -52,6 +53,8 @@ class MailSettingsControllerTest extends \Test\TestCase { private $mailer; /** @var IL10N|\PHPUnit\Framework\MockObject\MockObject */ private $l; + /** @var IURLGenerator */ + private $urlGenerator; /** @var MailSettingsController */ private $mailController; @@ -63,6 +66,7 @@ protected function setUp(): void { $this->config = $this->createMock(IConfig::class); $this->userSession = $this->createMock(IUserSession::class); $this->mailer = $this->createMock(IMailer::class); + $this->urlGenerator = $this->createMock(IURLGenerator::class); /** @var IRequest|\PHPUnit\Framework\MockObject\MockObject $request */ $request = $this->createMock(IRequest::class); $this->mailController = new MailSettingsController( @@ -71,6 +75,7 @@ protected function setUp(): void { $this->l, $this->config, $this->userSession, + $this->urlGenerator, $this->mailer, 'no-reply@nextcloud.com' ); @@ -170,7 +175,7 @@ public function testSendTestMail() { // Ensure that it fails when no mail address has been specified $response = $this->mailController->sendTestMail(); $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()); + $this->assertSame('You need to set your user email before being able to send test emails. Go to for that.', $response->getData()); // If no exception is thrown it should work $this->config diff --git a/core/js/tests/specs/setupchecksSpec.js b/core/js/tests/specs/setupchecksSpec.js index c3e7fab14f19b..a226cb9250176 100644 --- a/core/js/tests/specs/setupchecksSpec.js +++ b/core/js/tests/specs/setupchecksSpec.js @@ -226,6 +226,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', @@ -283,6 +284,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', @@ -341,6 +343,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', @@ -396,6 +399,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', @@ -450,6 +454,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', @@ -504,6 +509,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', @@ -560,6 +566,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', @@ -614,6 +621,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', @@ -668,6 +676,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', @@ -742,6 +751,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', @@ -797,6 +807,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', @@ -852,6 +863,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', @@ -907,6 +919,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', @@ -965,6 +978,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', @@ -1020,6 +1034,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', @@ -1072,6 +1087,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', @@ -1126,6 +1142,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', @@ -1180,6 +1197,7 @@ describe('OC.SetupChecks tests', function() { hasFileinfoInstalled: true, isGetenvServerWorking: true, isReadOnlyConfig: false, + wasEmailTestSuccessful: true, hasWorkingFileLocking: true, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', From 445ab85b010f761288e0fce60426588874221c20 Mon Sep 17 00:00:00 2001 From: szaimen Date: Thu, 3 Feb 2022 15:19:29 +0100 Subject: [PATCH 3/3] add a test for wasEmailTestSuccessful Signed-off-by: szaimen --- core/js/tests/specs/setupchecksSpec.js | 56 ++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/core/js/tests/specs/setupchecksSpec.js b/core/js/tests/specs/setupchecksSpec.js index a226cb9250176..cfadcef674651 100644 --- a/core/js/tests/specs/setupchecksSpec.js +++ b/core/js/tests/specs/setupchecksSpec.js @@ -497,6 +497,62 @@ describe('OC.SetupChecks tests', function() { }); }); + it('should return an info if the mail server config was not set or verified, yet', function(done) { + var async = OC.SetupChecks.checkSetup(); + + suite.server.requests[0].respond( + 200, + { + 'Content-Type': 'application/json' + }, + JSON.stringify({ + hasFileinfoInstalled: true, + isGetenvServerWorking: true, + isReadOnlyConfig: false, + wasEmailTestSuccessful: false, + hasWorkingFileLocking: true, + hasValidTransactionIsolationLevel: true, + suggestedOverwriteCliURL: '', + isRandomnessSecure: true, + securityDocs: 'https://docs.nextcloud.com/myDocs.html', + isFairUseOfFreePushService: true, + serverHasInternetConnectionProblems: false, + isMemcacheConfigured: true, + forwardedForHeadersWorking: true, + isCorrectMemcachedPHPModuleInstalled: true, + hasPassedCodeIntegrityCheck: true, + OpcacheSetupRecommendations: [], + phpOpcacheDocumentation: 'https://example.org/link/to/doc', + isSettimelimitAvailable: true, + hasFreeTypeSupport: true, + missingIndexes: [], + missingPrimaryKeys: [], + missingColumns: [], + cronErrors: [], + cronInfo: { + diffInSeconds: 0 + }, + isMemoryLimitSufficient: true, + appDirsWithDifferentOwner: [], + recommendedPHPModules: [], + pendingBigIntConversionColumns: [], + isMysqlUsedWithoutUTF8MB4: false, + isDefaultPhoneRegionSet: true, + isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, + reverseProxyGeneratedURL: 'https://server', + temporaryDirectoryWritable: true, + }) + ); + + async.done(function( data, s, x ){ + expect(data).toEqual([{ + msg: 'You have not set or verified your email server configuration, yet. Please head over to the Basic settings in order to set them. Afterwards, use the "Send email" button below the form to verify your settings.', + type: OC.SetupChecks.MESSAGE_TYPE_INFO + }]); + done(); + }); + }); + it('should return a warning if there are app directories with wrong permissions', function(done) { var async = OC.SetupChecks.checkSetup();