From c49a55eb08e92438cec0ad0649f5cfea9436d764 Mon Sep 17 00:00:00 2001 From: szaimen Date: Thu, 3 Feb 2022 11:43:17 +0100 Subject: [PATCH 1/4] 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 3c7d5a5c0aba0..11900fad45b9e 100644 --- a/apps/settings/lib/Controller/CheckSetupController.php +++ b/apps/settings/lib/Controller/CheckSetupController.php @@ -572,6 +572,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) { @@ -822,6 +836,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 8e6f17f07ede9..5976f3b701c10 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 11941e824ac71236ca30509b701c0f828db45025 Mon Sep 17 00:00:00 2001 From: szaimen Date: Thu, 3 Feb 2022 12:28:37 +0100 Subject: [PATCH 2/4] 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 478c4519b2f95..20cf2b0106984 100644 --- a/apps/settings/tests/Controller/CheckSetupControllerTest.php +++ b/apps/settings/tests/Controller/CheckSetupControllerTest.php @@ -182,6 +182,7 @@ protected function setUp(): void { ]) ->setMethods([ 'isReadOnlyConfig', + 'wasEmailTestSuccessful', 'hasValidTransactionIsolationLevel', 'hasFileinfoInstalled', 'hasWorkingFileLocking', @@ -506,6 +507,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') @@ -604,6 +609,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 0ae51b96a330355295b5de3277591c26b5332c1d Mon Sep 17 00:00:00 2001 From: szaimen Date: Thu, 3 Feb 2022 15:19:29 +0100 Subject: [PATCH 3/4] 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(); From 4ee3d5ef80032a67dc1e57f8f634868c18e99d44 Mon Sep 17 00:00:00 2001 From: szaimen Date: Fri, 4 Feb 2022 10:16:29 +0100 Subject: [PATCH 4/4] Remove spaces around link See https://github.com/nextcloud/server/pull/31003 Signed-off-by: szaimen --- core/js/setupchecks.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/js/setupchecks.js b/core/js/setupchecks.js index 5976f3b701c10..fdeed4897d091 100644 --- a/core/js/setupchecks.js +++ b/core/js/setupchecks.js @@ -197,9 +197,9 @@ } 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}', ''), + 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 }); }