From 026c2ac149464e0ab44716e0ff7b1351c2581949 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Thu, 5 Oct 2023 14:01:27 +0200 Subject: [PATCH] fix: UI and tests --- config/config.sample.php | 3 +- lib/private/Mail/Logger.php | 20 +++++++ lib/private/Mail/Mailer.php | 45 +++++++++------ lib/private/Mail/Message.php | 13 ++--- .../Controller/MailSettingsController.php | 56 +++++++++++-------- settings/templates/panels/admin/mail.php | 1 - tests/lib/Mail/MailerTest.php | 21 ++++--- tests/lib/Mail/MessageTest.php | 3 +- 8 files changed, 101 insertions(+), 61 deletions(-) create mode 100644 lib/private/Mail/Logger.php diff --git a/config/config.sample.php b/config/config.sample.php index 7cbf197443f0..e3d383ee1ce6 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -396,7 +396,6 @@ /** * Enable or disable SMTP class debugging */ -# TODO: unused / deprecated 'mail_smtpdebug' => false, /** @@ -439,7 +438,7 @@ /** * Define the SMTP security style - * Depends on `mail_smtpmode`. Specify when you are using `ssl` or `tls`. + * Depends on `mail_smtpmode`. Specify when you are using `ssl` or not. * Leave empty for no encryption. */ 'mail_smtpsecure' => '', diff --git a/lib/private/Mail/Logger.php b/lib/private/Mail/Logger.php new file mode 100644 index 000000000000..17b51e36a39b --- /dev/null +++ b/lib/private/Mail/Logger.php @@ -0,0 +1,20 @@ +log[] = [$level, $message, $context]; + } + + /** + * @throws \JsonException + */ + public function toJSON(): string { + return json_encode($this->log, JSON_THROW_ON_ERROR); + } +} diff --git a/lib/private/Mail/Mailer.php b/lib/private/Mail/Mailer.php index f2f2a8a0882e..1c9a88ec0dd5 100644 --- a/lib/private/Mail/Mailer.php +++ b/lib/private/Mail/Mailer.php @@ -27,6 +27,7 @@ use OCP\IConfig; use OCP\Mail\IMailer; use OCP\ILogger; +use Psr\Log\LoggerInterface; use Symfony\Component\Mailer\Exception\TransportExceptionInterface; use Symfony\Component\Mailer\Transport\SendmailTransport; use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport; @@ -86,19 +87,31 @@ public function createMessage(): Message { * has been supplied.) */ public function send(Message $message): array { - # TODO: implement this ???? - $debugMode = $this->config->getSystemValue('mail_smtpdebug', false); - if (!\is_array($message->getFrom()) || \count($message->getFrom()) === 0) { $message->setFrom([\OCP\Util::getDefaultEmailAddress($this->defaults->getName())]); } - $failedRecipients = []; + $debugMode = $this->config->getSystemValue('mail_smtpdebug', false); + $logger = $debugMode ? new Logger() : null; try { - $this->getInstance()->send($message->getMessage()); + $this->getInstance($logger ?? null)->send($message->getMessage()); } catch (TransportExceptionInterface $e) { - # TODO: handle exception + $this->logger->logException($e); + + # in case of exception it is expected that none of the mails has been sent + $failedRecipients = []; + + $recipients = array_merge($message->getTo(), $message->getCc(), $message->getBcc()); + array_walk($recipients, static function ($value, $key) use (&$failedRecipients) { + if (is_numeric($key)) { + $failedRecipients[] = $value; + } else { + $failedRecipients[] = $key; + } + }); + + return $failedRecipients; } $allRecipients = []; @@ -118,10 +131,11 @@ public function send(Message $message): array { 'app' => 'core', 'from' => \json_encode($message->getFrom()), 'recipients' => \json_encode($allRecipients), - 'subject' => $message->getSubject() + 'subject' => $message->getSubject(), + 'mail_log' => ($logger !== null) ? $logger->toJSON() : null, ]); - return $failedRecipients; + return []; } /** @@ -157,16 +171,16 @@ protected function convertEmail(string $email): string { return $name.'@'.$domain; } - protected function getInstance(): TransportInterface { + protected function getInstance(?LoggerInterface $logger = null): TransportInterface { if ($this->instance !== null) { return $this->instance; } $mailMode = $this->config->getSystemValue('mail_smtpmode', 'php'); if ($mailMode === 'smtp') { - $transport = $this->getSmtpInstance(); + $transport = $this->getSmtpInstance($logger ?? null); } else { - $transport = $this->getSendMailInstance(); + $transport = $this->getSendMailInstance($logger ?? null); } $this->instance = $transport; @@ -174,13 +188,13 @@ protected function getInstance(): TransportInterface { return $this->instance; } - protected function getSmtpInstance(): EsmtpTransport { + protected function getSmtpInstance(LoggerInterface $logger): EsmtpTransport { $timeout = $this->config->getSystemValue('mail_smtptimeout', 10); $host = $this->config->getSystemValue('mail_smtphost', '127.0.0.1'); $port = $this->config->getSystemValue('mail_smtpport', 25); $smtpSecurity = $this->config->getSystemValue('mail_smtpsecure', ''); $tls = $smtpSecurity === 'ssl' ? true : null; - $transport = new EsmtpTransport($host, $port, $tls); + $transport = new EsmtpTransport($host, $port, $tls, null, $logger); if ($this->config->getSystemValue('mail_smtpauth', false)) { $transport->setUsername($this->config->getSystemValue('mail_smtpname', '')); $transport->setPassword($this->config->getSystemValue('mail_smtppassword', '')); @@ -193,7 +207,7 @@ protected function getSmtpInstance(): EsmtpTransport { return $transport; } - protected function getSendMailInstance(): SendmailTransport { + protected function getSendMailInstance(?LoggerInterface $logger = null): SendmailTransport { $i = $this->config->getSystemValue('mail_smtpmode', 'sendmail'); if ($i === 'qmail') { $binaryPath = '/var/qmail/bin/sendmail'; @@ -201,7 +215,6 @@ protected function getSendMailInstance(): SendmailTransport { $binaryPath = '/usr/sbin/sendmail'; } - # TODO: add logger ??? - return new SendmailTransport($binaryPath . ' -bs'); + return new SendmailTransport($binaryPath . ' -bs', null, $logger); } } diff --git a/lib/private/Mail/Message.php b/lib/private/Mail/Message.php index d716531b6e10..88ce719abf07 100644 --- a/lib/private/Mail/Message.php +++ b/lib/private/Mail/Message.php @@ -32,14 +32,11 @@ */ class Message { private Email $message; - /** - * @var Address[] - */ - private array $from; - private array $replyTo; - private array $to; - private array $cc; - private array $bcc; + private array $from = []; + private array $replyTo = []; + private array $to = []; + private array $cc = []; + private array $bcc = []; public function __construct(Email $swiftMessage) { $this->message = $swiftMessage; diff --git a/settings/Controller/MailSettingsController.php b/settings/Controller/MailSettingsController.php index ec11448c0fbb..2d1743fa8a7d 100644 --- a/settings/Controller/MailSettingsController.php +++ b/settings/Controller/MailSettingsController.php @@ -160,36 +160,44 @@ public function sendTestMail() { $email = $this->userSession->getUser()->getEMailAddress(); } - if (!empty($email)) { - try { - $message = $this->mailer->createMessage(); - $message->setTo([$email => $this->userSession->getUser()->getDisplayName()]); - $message->setFrom([$this->defaultMailAddress]); - $message->setSubject($this->l10n->t('test email settings')); - $message->setPlainBody('If you received this email, the settings seem to be correct.'); - $this->mailer->send($message); - } 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()]), + if (empty($email)) { + return ['data' => + ['message' => + (string) $this->l10n->t('You need to set your user email before being able to send test emails.'), + ], + 'status' => 'error' + ]; + } + + try { + $message = $this->mailer->createMessage(); + $message->setTo([$email => $this->userSession->getUser()->getDisplayName()]); + $message->setFrom([$this->defaultMailAddress]); + $message->setSubject($this->l10n->t('test email settings')); + $message->setPlainBody('If you received this email, the settings seem to be correct.'); + $failed = $this->mailer->send($message); + if (empty($failed)) { + return ['data' => + ['message' => + (string) $this->l10n->t('Email sent') ], - 'status' => 'error', + 'status' => 'success' ]; } - return ['data' => - ['message' => - (string) $this->l10n->t('Email sent') + return [ + 'data' => [ + 'message' => (string) $this->l10n->t('A problem occurred while sending the email. Please revise your settings. (Error: %s)', ['not delivered']), + ], + 'status' => 'error', + ]; + } 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' => 'success' + 'status' => 'error', ]; } - - return ['data' => - ['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/templates/panels/admin/mail.php b/settings/templates/panels/admin/mail.php index 135491b1e82d..45394de5ffc9 100644 --- a/settings/templates/panels/admin/mail.php +++ b/settings/templates/panels/admin/mail.php @@ -3,7 +3,6 @@ $mail_smtpsecure = [ '' => $l->t('None'), 'ssl' => $l->t('SSL/TLS'), - 'tls' => $l->t('STARTTLS'), ]; $mail_smtpmode = [ 'php', diff --git a/tests/lib/Mail/MailerTest.php b/tests/lib/Mail/MailerTest.php index 90d2aee29608..d1b50f8ae1e3 100644 --- a/tests/lib/Mail/MailerTest.php +++ b/tests/lib/Mail/MailerTest.php @@ -8,21 +8,25 @@ namespace Test\Mail; +use Exception; use OC\Mail\Mailer; use OC_Defaults; use OCP\IConfig; use OCP\ILogger; +use PHPUnit\Framework\MockObject\MockObject; use Symfony\Component\Mailer\Transport\SendmailTransport; use Symfony\Component\Mime\Email; use Test\TestCase; use OC\Mail\Message; +use function array_merge; +use function json_encode; class MailerTest extends TestCase { - /** @var IConfig | \PHPUnit\Framework\MockObject\MockObject */ + /** @var IConfig | MockObject */ private $config; /** @var OC_Defaults */ private $defaults; - /** @var ILogger | \PHPUnit\Framework\MockObject\MockObject */ + /** @var ILogger | MockObject */ private $logger; /** @var Mailer */ private $mailer; @@ -76,9 +80,9 @@ public function testGetInstanceSendmail(): void { } public function testSendInvalidMailException(): void { - $this->expectException(\Exception::class); + $this->expectException(Exception::class); - /** @var Message | \PHPUnit\Framework\MockObject\MockObject $message */ + /** @var Message | MockObject $message */ $message = $this->getMockBuilder(Message::class) ->disableOriginalConstructor()->getMock(); $message->expects($this->once()) @@ -118,7 +122,7 @@ public function testLogEntry(): void { $this->mailer->method('getInstance')->willReturn($this->createMock(SendmailTransport::class)); - /** @var Message | \PHPUnit\Framework\MockObject\MockObject $message */ + /** @var Message | MockObject $message */ $message = $this->getMockBuilder(Message::class) ->disableOriginalConstructor()->getMock(); $message->expects($this->once()) @@ -140,9 +144,10 @@ public function testLogEntry(): void { ->method('debug') ->with('Sent mail from "{from}" to "{recipients}" with subject "{subject}"', [ 'app' => 'core', - 'from' => \json_encode($from), - 'recipients' => \json_encode(\array_merge($to, $cc, $bcc)), - 'subject' => 'Email subject' + 'from' => json_encode($from), + 'recipients' => json_encode(array_merge($to, $cc, $bcc)), + 'subject' => 'Email subject', + 'mail_log' => null ]); $this->mailer->send($message); diff --git a/tests/lib/Mail/MessageTest.php b/tests/lib/Mail/MessageTest.php index e12cb1c3c8c7..cc4dbe64bc93 100644 --- a/tests/lib/Mail/MessageTest.php +++ b/tests/lib/Mail/MessageTest.php @@ -14,8 +14,7 @@ use Test\TestCase; class MessageTest extends TestCase { - /** @var Email */ - private $symfonyMail; + private Email $symfonyMail; private Message $message; public function setUp(): void {