From f9b64be379c543133786f1582178498e453b169d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=8Ceslav=20Przywara?= Date: Thu, 7 Mar 2024 15:40:26 +0100 Subject: [PATCH] Refactor code that sents notifications Make sure no notification is sent twice in the same request. --- .../Modules/Notifications/Mailman.php | 16 +-- .../Modules/Notifications/Message.php | 65 +++++++++ .../Modules/Notifications/Watchman.php | 130 ++++++++++++------ 3 files changed, 160 insertions(+), 51 deletions(-) create mode 100644 classes/BlueChip/Security/Modules/Notifications/Message.php diff --git a/classes/BlueChip/Security/Modules/Notifications/Mailman.php b/classes/BlueChip/Security/Modules/Notifications/Mailman.php index 6be81a9..b8d0212 100644 --- a/classes/BlueChip/Security/Modules/Notifications/Mailman.php +++ b/classes/BlueChip/Security/Modules/Notifications/Mailman.php @@ -28,28 +28,24 @@ abstract class Mailman * * @param string|string[] $to Email address(es) of notification recipient(s). * @param string $subject Subject of notification. - * @param string|string[] $message Body of notification. + * @param Message $message Body of notification. * * @return bool True if notification has been sent successfully, false otherwise. */ - public static function send($to, string $subject, $message): bool + public static function send(array|string $to, string $subject, Message $message): bool { - return \wp_mail( + return wp_mail( $to, self::formatSubject($subject), - self::formatMessage(\is_array($message) ? $message : [$message]) + self::formatMessage($message) ); } /** * Strip any HTML tags from $message and add plugin boilerplate to it. - * - * @param string[] $message Message body as list of lines. - * - * @return string */ - private static function formatMessage(array $message): string + private static function formatMessage(Message $message): string { $boilerplate_intro = [ \sprintf( @@ -71,7 +67,7 @@ private static function formatMessage(array $message): string ), ]; - return \implode(self::EOL, \array_merge($boilerplate_intro, self::stripTags($message), $boilerplate_outro)); + return \implode(self::EOL, \array_merge($boilerplate_intro, self::stripTags($message->getRaw()), $boilerplate_outro)); } diff --git a/classes/BlueChip/Security/Modules/Notifications/Message.php b/classes/BlueChip/Security/Modules/Notifications/Message.php new file mode 100644 index 0000000..9fbb72c --- /dev/null +++ b/classes/BlueChip/Security/Modules/Notifications/Message.php @@ -0,0 +1,65 @@ +body = ($text !== '') ? [$text] : []; + } + + + public function __toString(): string + { + return \implode(PHP_EOL, $this->body); + } + + + public function addEmptyLine(): self + { + return $this->addLine(''); + } + + + public function addLine(string $text = ''): self + { + $this->body[] = $text; + return $this; + } + + + /** + * @param string[] $text + * + * @return self + */ + public function addLines(array $text): self + { + $this->body = \array_merge($this->body, \array_is_list($text) ? $text : \array_values($text)); + return $this; + } + + + public function getFingerprint(): string + { + return \sha1((string) $this); + } + + + /** + * @return string[] + */ + public function getRaw(): array + { + return $this->body; + } +} diff --git a/classes/BlueChip/Security/Modules/Notifications/Watchman.php b/classes/BlueChip/Security/Modules/Notifications/Watchman.php index d284870..8e04120 100644 --- a/classes/BlueChip/Security/Modules/Notifications/Watchman.php +++ b/classes/BlueChip/Security/Modules/Notifications/Watchman.php @@ -23,7 +23,13 @@ class Watchman implements Activable, Initializable /** * @var string[] List of notifications recipients */ - private $recipients; + private array $recipients; + + + /** + * @var string[] List of sent notifications. + */ + private array $sent_notifications = []; /** @@ -124,14 +130,16 @@ public function deactivate(): void $user = wp_get_current_user(); if ($user->ID) { // Name the bastard that turned us off! - $message = \sprintf( - __('User "%s" had just deactivated BC Security plugin on your website!', 'bc-security'), - $user->user_login + $message = new Message( + \sprintf( + __('User "%s" had just deactivated BC Security plugin on your website!', 'bc-security'), + $user->user_login + ) ); } else { // No user means plugin has been probably deactivated via WP-CLI. // See: https://github.com/chesio/bc-security/issues/16#issuecomment-321541102 - $message = __('BC Security plugin on your website has been deactivated!', 'bc-security'); + $message = new Message(__('BC Security plugin on your website has been deactivated!', 'bc-security')); } $this->notify($subject, $message); @@ -170,9 +178,11 @@ private function watchCoreUpdateAvailable($update_transient): void } $subject = __('WordPress update available', 'bc-security'); - $message = \sprintf( - __('WordPress has an update to version %s available.', 'bc-security'), - $latest_version + $message = new Message( + \sprintf( + __('WordPress has an update to version %s available.', 'bc-security'), + $latest_version, + ) ); // Send notification. @@ -206,7 +216,7 @@ private function watchPluginUpdatesAvailable($update_transient): void } $subject = __('Plugin updates available', 'bc-security'); - $message = []; + $message = new Message(); foreach ($plugin_updates as $plugin_file => $plugin_update_data) { $plugin_data = Plugin::getPluginData($plugin_file); @@ -224,7 +234,7 @@ private function watchPluginUpdatesAvailable($update_transient): void ); } - $message[] = $plugin_message; + $message->addLine($plugin_message); } // Send notification. @@ -260,14 +270,16 @@ private function watchThemeUpdatesAvailable($update_transient): void } $subject = __('Theme updates available', 'bc-security'); - $message = []; + $message = new Message(); foreach ($theme_updates as $theme_slug => $theme_update_data) { $theme = wp_get_theme($theme_slug); - $message[] = \sprintf( - __('Theme "%1$s" has an update to version %2$s available.', 'bc-security'), - $theme, - $theme_update_data['new_version'] + $message->addLine( + \sprintf( + __('Theme "%1$s" has an update to version %2$s available.', 'bc-security'), + $theme, + $theme_update_data['new_version'], + ) ); } @@ -288,11 +300,13 @@ private function watchBadRequestBanEvents(string $remote_address, string $uri, B { if (\in_array($remote_address, $this->logger->getKnownIps(), true)) { $subject = __('Known IP locked out', 'bc-security'); - $message = \sprintf( - __('A known IP address %1$s has been locked out due to bad request rule "%2$s" after someone tried to access following URL: %3$s', 'bc-security'), - self::formatRemoteAddress($remote_address), - $ban_rule->getName(), - $uri + $message = new Message( + \sprintf( + __('A known IP address %1$s has been locked out due to bad request rule "%2$s" after someone tried to access following URL: %3$s', 'bc-security'), + self::formatRemoteAddress($remote_address), + $ban_rule->getName(), + $uri, + ) ); $this->notify($subject, $message); @@ -307,11 +321,13 @@ private function watchLockoutEvents(string $remote_address, string $username, in { if (\in_array($remote_address, $this->logger->getKnownIps(), true)) { $subject = __('Known IP locked out', 'bc-security'); - $message = \sprintf( - __('A known IP address %1$s has been locked out for %2$d seconds after someone tried to log in with username "%3$s".', 'bc-security'), - self::formatRemoteAddress($remote_address), - $duration, - $username + $message = new Message( + \sprintf( + __('A known IP address %1$s has been locked out for %2$d seconds after someone tried to log in with username "%3$s".', 'bc-security'), + self::formatRemoteAddress($remote_address), + $duration, + $username, + ) ); $this->notify($subject, $message); @@ -326,10 +342,12 @@ private function watchWpLogin(string $username, WP_User $user): void { if (Is::admin($user)) { $subject = __('Admin user login', 'bc-security'); - $message = \sprintf( - __('User "%1$s" with administrator privileges just logged in to your WordPress site from IP address %2$s.', 'bc-security'), - $username, - self::formatRemoteAddress($this->remote_address) + $message = new Message( + \sprintf( + __('User "%1$s" with administrator privileges just logged in to your WordPress site from IP address %2$s.', 'bc-security'), + $username, + self::formatRemoteAddress($this->remote_address), + ) ); $this->notify($subject, $message); @@ -343,12 +361,17 @@ private function watchWpLogin(string $username, WP_User $user): void private function watchChecklistSingleCheckAlert(Check $check, CheckResult $result): void { $subject = __('Checklist monitoring alert', 'bc-security'); - $preamble = [ - \sprintf(__('An issue has been found during checklist monitoring of "%s" check:', 'bc-security'), $check->getName()), - '', - ]; + $message = new Message( + \sprintf( + __('An issue has been found during checklist monitoring of "%s" check:', 'bc-security'), + $check->getName(), + ) + ); + + $message->addEmptyLine(); + $message->addLines($result->getMessage()); - $this->notify($subject, \array_merge($preamble, $result->getMessage())); + $this->notify($subject, $message); } @@ -360,13 +383,15 @@ private function watchChecklistSingleCheckAlert(Check $check, CheckResult $resul private function watchChecklistMultipleChecksAlert(array $issues): void { $subject = __('Checklist monitoring alert', 'bc-security'); - $message = [ + $message = new Message( __('Following checks had failed during checklist monitoring:', 'bc-security'), - ]; + ); foreach ($issues as $issue) { - $message[] = ''; - $message[] = \sprintf("%s: %s", $issue['check']->getName(), $issue['result']->getMessageAsPlainText()); + $message + ->addEmptyLine() + ->addLine(\sprintf("%s: %s", $issue['check']->getName(), $issue['result']->getMessageAsPlainText())) + ; } $this->notify($subject, $message); @@ -377,12 +402,35 @@ private function watchChecklistMultipleChecksAlert(array $issues): void * Send email notification with given $subject and $message to recipients configured in plugin settings. * * @param string $subject - * @param string|string[] $message + * @param Message $message * * @return bool|null Null if there are no recipients configured. True if email has been sent, false otherwise. */ - private function notify(string $subject, $message): ?bool + private function notify(string $subject, Message $message): ?bool + { + if ($this->hasMessageBeenSent($message)) { + // Given message has been sent already. + return true; + } + + $status = empty($this->recipients) ? null : Mailman::send($this->recipients, $subject, $message); + + if ($status) { + $this->markMessageAsSent($message); + } + + return $status; + } + + + private function hasMessageBeenSent(Message $message): bool + { + return \in_array($message->getFingerprint(), $this->sent_notifications, true); + } + + + private function markMessageAsSent(Message $message): void { - return empty($this->recipients) ? null : Mailman::send($this->recipients, $subject, $message); + $this->sent_notifications[] = $message->getFingerprint(); } }