From 32a5c1eed111258272e6be895153e5e19a4bbfcc Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Wed, 6 Nov 2024 11:15:22 -0500 Subject: [PATCH 01/13] Allow `email_log.user_id` to be null, and add `non_user_address` column --- application/common/models/EmailLog.php | 1 + application/common/models/EmailLogBase.php | 7 +++- ..._155829_enable_logging_non_user_emails.php | 42 +++++++++++++++++++ 3 files changed, 48 insertions(+), 2 deletions(-) create mode 100644 application/console/migrations/m241106_155829_enable_logging_non_user_emails.php diff --git a/application/common/models/EmailLog.php b/application/common/models/EmailLog.php index a2e82068..ec95be4c 100644 --- a/application/common/models/EmailLog.php +++ b/application/common/models/EmailLog.php @@ -41,6 +41,7 @@ public function attributeLabels() { return ArrayHelper::merge(parent::attributeLabels(), [ 'sent_utc' => Yii::t('app', 'Sent (UTC)'), + 'non_user_address' => Yii::t('app', 'Non-User Address'), ]); } diff --git a/application/common/models/EmailLogBase.php b/application/common/models/EmailLogBase.php index c7a8ede9..7ffe51ab 100644 --- a/application/common/models/EmailLogBase.php +++ b/application/common/models/EmailLogBase.php @@ -8,9 +8,10 @@ * This is the model class for table "email_log". * * @property int $id - * @property int $user_id + * @property int|null $user_id * @property string|null $message_type * @property string $sent_utc + * @property string|null $non_user_address * * @property User $user */ @@ -30,10 +31,11 @@ public static function tableName() public function rules() { return [ - [['user_id', 'sent_utc'], 'required'], [['user_id'], 'integer'], [['message_type'], 'string'], + [['sent_utc'], 'required'], [['sent_utc'], 'safe'], + [['non_user_address'], 'string', 'max' => 255], [['user_id'], 'exist', 'skipOnError' => true, 'targetClass' => User::class, 'targetAttribute' => ['user_id' => 'id']], ]; } @@ -48,6 +50,7 @@ public function attributeLabels() 'user_id' => Yii::t('app', 'User ID'), 'message_type' => Yii::t('app', 'Message Type'), 'sent_utc' => Yii::t('app', 'Sent Utc'), + 'non_user_address' => Yii::t('app', 'Non User Address'), ]; } diff --git a/application/console/migrations/m241106_155829_enable_logging_non_user_emails.php b/application/console/migrations/m241106_155829_enable_logging_non_user_emails.php new file mode 100644 index 00000000..65e8adda --- /dev/null +++ b/application/console/migrations/m241106_155829_enable_logging_non_user_emails.php @@ -0,0 +1,42 @@ +addColumn( + '{{email_log}}', + 'non_user_address', + $this->string()->null() + ); + $this->alterColumn( + '{{email_log}}', + 'user_id', + $this->integer()->null() + ); + } + + public function safeDown() + { + $this->delete( + '{{email_log}}', + [ + 'user_id' => null, + ] + ); + $this->alterColumn( + '{{email_log}}', + 'user_id', + $this->integer()->notNull() + ); + $this->dropColumn( + '{{email_log}}', + 'non_user_address' + ); + } +} From f0da1fcb122e7123f9fcd8b5c8a468f1ecf9a9c9 Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Wed, 6 Nov 2024 11:26:10 -0500 Subject: [PATCH 02/13] Log emails sent to non-user addresses, too --- application/common/components/Emailer.php | 6 ++++-- application/common/models/EmailLog.php | 21 ++++++++++++++++++++- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/application/common/components/Emailer.php b/application/common/components/Emailer.php index 850f5966..b0a51312 100644 --- a/application/common/components/Emailer.php +++ b/application/common/components/Emailer.php @@ -341,7 +341,7 @@ public function init() } /** - * Send the specified type of message to the given User. + * Send the specified type of message to the given User (or non-User address). * * @param string $messageType The message type. Must be one of the * EmailLog::MESSAGE_TYPE_* values. @@ -385,7 +385,9 @@ public function sendMessageTo( $this->email($toAddress, $subject, $htmlBody, strip_tags($htmlBody), $ccAddress, $bccAddress, $delaySeconds); if ($user !== null) { - EmailLog::logMessage($messageType, $user->id); + EmailLog::logMessageToUser($messageType, $user->id); + } else { + EmailLog::logMessageToNonUser($messageType, $toAddress); } } diff --git a/application/common/models/EmailLog.php b/application/common/models/EmailLog.php index ec95be4c..1a95075a 100644 --- a/application/common/models/EmailLog.php +++ b/application/common/models/EmailLog.php @@ -70,7 +70,7 @@ public static function getMessageTypes() ]; } - public static function logMessage(string $messageType, $userId) + public static function logMessageToUser(string $messageType, $userId) { $emailLog = new EmailLog([ 'user_id' => $userId, @@ -89,6 +89,25 @@ public static function logMessage(string $messageType, $userId) } } + public static function logMessageToNonUser(string $messageType, string $emailAddress) + { + $emailLog = new EmailLog([ + 'non_user_address' => $emailAddress, + 'message_type' => $messageType, + ]); + + if (!$emailLog->save()) { + $errorMessage = sprintf( + 'Failed to log %s email to %s: %s', + var_export($messageType, true), + var_export($emailAddress, true), + \json_encode($emailLog->getFirstErrors(), JSON_PRETTY_PRINT) + ); + \Yii::warning($errorMessage); + throw new \Exception($errorMessage, 1730909932); + } + } + /** * {@inheritdoc} */ From 56a77b90114b7c5dbce3e278f552a9758b4bba3f Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Tue, 29 Oct 2024 16:26:38 -0400 Subject: [PATCH 03/13] Add 'ext-group-sync-errors' and 'abandoned-users' EmailLog message types --- ...029_200547_add_email_log_message_types.php | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 application/console/migrations/m241029_200547_add_email_log_message_types.php diff --git a/application/console/migrations/m241029_200547_add_email_log_message_types.php b/application/console/migrations/m241029_200547_add_email_log_message_types.php new file mode 100644 index 00000000..b80f728e --- /dev/null +++ b/application/console/migrations/m241029_200547_add_email_log_message_types.php @@ -0,0 +1,27 @@ +alterColumn( + '{{email_log}}', + 'message_type', + "enum('invite','welcome','mfa-rate-limit','password-changed','get-backup-codes','refresh-backup-codes','lost-security-key','mfa-option-added','mfa-option-removed','mfa-enabled','mfa-disabled','method-verify','mfa-manager','mfa-manager-help','method-reminder','method-purged','password-expiring','password-expired','password-pwned','ext-group-sync-errors','abandoned-users') null" + ); + } + + public function safeDown() + { + $this->alterColumn( + '{{email_log}}', + 'message_type', + "enum('invite','welcome','mfa-rate-limit','password-changed','get-backup-codes','refresh-backup-codes','lost-security-key','mfa-option-added','mfa-option-removed','mfa-enabled','mfa-disabled','method-verify','mfa-manager','mfa-manager-help','method-reminder','method-purged','password-expiring','password-expired','password-pwned') null" + ); + } +} From 6c1a9d348c2af5078f72add10658373611163ac9 Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Tue, 5 Nov 2024 15:14:04 -0500 Subject: [PATCH 04/13] Include "abandoned-users" email's subject in the list of email subjects --- application/common/components/Emailer.php | 8 ++++++++ application/common/models/EmailLog.php | 1 + 2 files changed, 9 insertions(+) diff --git a/application/common/components/Emailer.php b/application/common/components/Emailer.php index b0a51312..5bd79129 100644 --- a/application/common/components/Emailer.php +++ b/application/common/components/Emailer.php @@ -313,6 +313,7 @@ public function init() EmailLog::MESSAGE_TYPE_MFA_RATE_LIMIT => $this->subjectForMfaRateLimit, EmailLog::MESSAGE_TYPE_PASSWORD_CHANGED => $this->subjectForPasswordChanged, EmailLog::MESSAGE_TYPE_WELCOME => $this->subjectForWelcome, + EmailLog::MESSAGE_TYPE_ABANDONED_USERS => $this->subjectForAbandonedUsers, EmailLog::MESSAGE_TYPE_EXT_GROUP_SYNC_ERRORS => $this->subjectForExtGroupSyncErrors, EmailLog::MESSAGE_TYPE_GET_BACKUP_CODES => $this->subjectForGetBackupCodes, EmailLog::MESSAGE_TYPE_REFRESH_BACKUP_CODES => $this->subjectForRefreshBackupCodes, @@ -382,6 +383,13 @@ public function sendMessageTo( $bccAddress = $data['bccAddress'] ?? ''; $subject = $this->getSubjectForMessage($messageType, $dataForEmail); + if (empty($subject)) { + \Yii::error(sprintf( + 'No subject known for %s email messages.', + $messageType + )); + } + $this->email($toAddress, $subject, $htmlBody, strip_tags($htmlBody), $ccAddress, $bccAddress, $delaySeconds); if ($user !== null) { diff --git a/application/common/models/EmailLog.php b/application/common/models/EmailLog.php index 1a95075a..c26068b1 100644 --- a/application/common/models/EmailLog.php +++ b/application/common/models/EmailLog.php @@ -17,6 +17,7 @@ class EmailLog extends EmailLogBase public const MESSAGE_TYPE_MFA_RATE_LIMIT = 'mfa-rate-limit'; public const MESSAGE_TYPE_PASSWORD_CHANGED = 'password-changed'; public const MESSAGE_TYPE_WELCOME = 'welcome'; + public const MESSAGE_TYPE_ABANDONED_USERS = 'abandoned-users'; public const MESSAGE_TYPE_EXT_GROUP_SYNC_ERRORS = 'ext-group-sync-errors'; public const MESSAGE_TYPE_GET_BACKUP_CODES = 'get-backup-codes'; public const MESSAGE_TYPE_REFRESH_BACKUP_CODES = 'refresh-backup-codes'; From cbcf0dd9d41564a268e73c80d73adaad19660840 Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Tue, 5 Nov 2024 15:20:06 -0500 Subject: [PATCH 05/13] Use reflection to avoid need to maintain duplicate list of message types --- application/common/models/EmailLog.php | 30 ++++++++------------------ 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/application/common/models/EmailLog.php b/application/common/models/EmailLog.php index c26068b1..1a436870 100644 --- a/application/common/models/EmailLog.php +++ b/application/common/models/EmailLog.php @@ -3,6 +3,7 @@ namespace common\models; use common\helpers\MySqlDateTime; +use ReflectionClass; use Yii; use yii\helpers\ArrayHelper; @@ -48,27 +49,14 @@ public function attributeLabels() public static function getMessageTypes() { - return [ - self::MESSAGE_TYPE_INVITE, - self::MESSAGE_TYPE_MFA_RATE_LIMIT, - self::MESSAGE_TYPE_PASSWORD_CHANGED, - self::MESSAGE_TYPE_WELCOME, - self::MESSAGE_TYPE_GET_BACKUP_CODES, - self::MESSAGE_TYPE_REFRESH_BACKUP_CODES, - self::MESSAGE_TYPE_LOST_SECURITY_KEY, - self::MESSAGE_TYPE_MFA_OPTION_ADDED, - self::MESSAGE_TYPE_MFA_OPTION_REMOVED, - self::MESSAGE_TYPE_MFA_ENABLED, - self::MESSAGE_TYPE_MFA_DISABLED, - self::MESSAGE_TYPE_METHOD_VERIFY, - self::MESSAGE_TYPE_METHOD_REMINDER, - self::MESSAGE_TYPE_METHOD_PURGED, - self::MESSAGE_TYPE_MFA_MANAGER, - self::MESSAGE_TYPE_MFA_MANAGER_HELP, - self::MESSAGE_TYPE_PASSWORD_EXPIRING, - self::MESSAGE_TYPE_PASSWORD_EXPIRED, - self::MESSAGE_TYPE_PASSWORD_PWNED, - ]; + $reflectionClass = new ReflectionClass(__CLASS__); + $messageTypes = []; + foreach ($reflectionClass->getConstants() as $name => $value) { + if (str_starts_with($name, 'MESSAGE_TYPE_')) { + $messageTypes[] = $value; + } + } + return $messageTypes; } public static function logMessageToUser(string $messageType, $userId) From eafcaaf452ad0c15b57bc3e140281a726f49b5b8 Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Wed, 6 Nov 2024 09:58:41 -0500 Subject: [PATCH 06/13] Move empty-subject check into the `getSubjectForMessage()` function --- application/common/components/Emailer.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/application/common/components/Emailer.php b/application/common/components/Emailer.php index 5bd79129..e1bbbc07 100644 --- a/application/common/components/Emailer.php +++ b/application/common/components/Emailer.php @@ -261,6 +261,13 @@ protected function getSubjectForMessage(string $messageType, array $data): strin { $subject = $this->subjects[$messageType] ?? ''; + if (empty($subject)) { + \Yii::error(sprintf( + 'No subject known for %s email messages.', + $messageType + )); + } + foreach ($data as $key => $value) { if (is_scalar($value)) { $subject = str_replace('{' . $key . '}', $value, $subject); @@ -383,13 +390,6 @@ public function sendMessageTo( $bccAddress = $data['bccAddress'] ?? ''; $subject = $this->getSubjectForMessage($messageType, $dataForEmail); - if (empty($subject)) { - \Yii::error(sprintf( - 'No subject known for %s email messages.', - $messageType - )); - } - $this->email($toAddress, $subject, $htmlBody, strip_tags($htmlBody), $ccAddress, $bccAddress, $delaySeconds); if ($user !== null) { From 21f82e645eeb681d94d5fe4c34a16b7c0e0a8995 Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Tue, 5 Nov 2024 15:27:09 -0500 Subject: [PATCH 07/13] Send the abandoned-users email the normal way, so it gets logged --- application/common/components/Emailer.php | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/application/common/components/Emailer.php b/application/common/components/Emailer.php index e1bbbc07..ce152d50 100644 --- a/application/common/components/Emailer.php +++ b/application/common/components/Emailer.php @@ -194,6 +194,11 @@ protected function assertConfigIsValid() /** * Use the email service to send an email. * + * WARNING: + * You probably shouldn't be calling this directly. Instead, call the + * `sendMessageTo()` method so that the sending of this email will be + * logged. + * * @param string $toAddress The recipient's email address. * @param string $subject The subject. * @param string $htmlBody The email body (as HTML). @@ -811,9 +816,14 @@ public function sendAbandonedUsersEmail() $dataForEmail['users'] = User::getAbandonedUsers(); if (!empty($dataForEmail['users'])) { - $htmlBody = \Yii::$app->view->render('@common/mail/abandoned-users.html.php', $dataForEmail); - - $this->email($this->hrNotificationsEmail, $this->subjectForAbandonedUsers, $htmlBody, strip_tags($htmlBody)); + $this->sendMessageTo( + 'abandoned-users', + null, + ArrayHelper::merge( + $dataForEmail, + ['toAddress' => $this->hrNotificationsEmail] + ) + ); } } } From a35af5032339659ede0dabe468d8639251ab8fdf Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Tue, 5 Nov 2024 16:13:50 -0500 Subject: [PATCH 08/13] Avoid sending abandoned-users email to HR too frequently --- application/common/components/Emailer.php | 38 ++++++++++++++++++----- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/application/common/components/Emailer.php b/application/common/components/Emailer.php index ce152d50..3b2085c2 100644 --- a/application/common/components/Emailer.php +++ b/application/common/components/Emailer.php @@ -443,6 +443,26 @@ public function hasReceivedMessageRecently($userId, string $messageType) return MySqlDateTime::dateIsRecent($latestEmail->sent_utc, $this->emailRepeatDelayDays); } + /** + * Whether we should send an abandoned-users message to HR. + * + * @return bool + */ + public function shouldSendAbandonedUsersMessage(): bool + { + if (empty($this->hrNotificationsEmail)) { + return false; + } + + $haveSentAbandonedUsersEmailRecently = $this->hasAddressReceivedMessageRecently( + $this->hrNotificationsEmail, + EmailLog::MESSAGE_TYPE_ABANDONED_USERS + ); + + return !$haveSentAbandonedUsersEmailRecently; + } + + /** * Whether we should send an invite message to the given User. * @@ -816,14 +836,16 @@ public function sendAbandonedUsersEmail() $dataForEmail['users'] = User::getAbandonedUsers(); if (!empty($dataForEmail['users'])) { - $this->sendMessageTo( - 'abandoned-users', - null, - ArrayHelper::merge( - $dataForEmail, - ['toAddress' => $this->hrNotificationsEmail] - ) - ); + if ($this->shouldSendAbandonedUsersMessage()) { + $this->sendMessageTo( + EmailLog::MESSAGE_TYPE_ABANDONED_USERS, + null, + ArrayHelper::merge( + $dataForEmail, + ['toAddress' => $this->hrNotificationsEmail] + ) + ); + } } } } From d56d2ac20bec3bfa2b0496ad17dd800b532ffa38 Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Wed, 6 Nov 2024 16:01:10 -0500 Subject: [PATCH 09/13] Split `hasReceivedMessageRecently()` into User and Non-User methods --- application/common/components/Emailer.php | 37 +++++++++++++++---- .../features/bootstrap/EmailContext.php | 27 +++++++++++--- 2 files changed, 51 insertions(+), 13 deletions(-) diff --git a/application/common/components/Emailer.php b/application/common/components/Emailer.php index 3b2085c2..5643d2db 100644 --- a/application/common/components/Emailer.php +++ b/application/common/components/Emailer.php @@ -432,7 +432,7 @@ public function sendDelayedMfaRelatedEmails() * @param string $messageType * @return bool */ - public function hasReceivedMessageRecently($userId, string $messageType) + public function hasUserReceivedMessageRecently($userId, string $messageType) { $latestEmail = EmailLog::find()->where(['user_id' => $userId, 'message_type' => $messageType]) ->orderBy('sent_utc DESC')->one(); @@ -443,6 +443,29 @@ public function hasReceivedMessageRecently($userId, string $messageType) return MySqlDateTime::dateIsRecent($latestEmail->sent_utc, $this->emailRepeatDelayDays); } + /** + * Whether the non-user address has already been sent this type of email in the last X days + * + * @param int $userId + * @param string $messageType + * @return bool + */ + public function hasNonUserReceivedMessageRecently(string $emailAddress, string $messageType) + { + $latestEmail = EmailLog::find()->where([ + 'message_type' => $messageType, + 'non_user_address' => $emailAddress, + 'user_id' => null, + ])->orderBy( + 'sent_utc DESC' + )->one(); + if (empty($latestEmail)) { + return false; + } + + return MySqlDateTime::dateIsRecent($latestEmail->sent_utc, $this->emailRepeatDelayDays); + } + /** * Whether we should send an abandoned-users message to HR. * @@ -454,7 +477,7 @@ public function shouldSendAbandonedUsersMessage(): bool return false; } - $haveSentAbandonedUsersEmailRecently = $this->hasAddressReceivedMessageRecently( + $haveSentAbandonedUsersEmailRecently = $this->hasNonUserReceivedMessageRecently( $this->hrNotificationsEmail, EmailLog::MESSAGE_TYPE_ABANDONED_USERS ); @@ -521,7 +544,7 @@ public function shouldSendGetBackupCodesMessageTo($user) return $this->sendGetBackupCodesEmails && $user->getVerifiedMfaOptionsCount() === 1 && !$user->hasMfaBackupCodes() - && !$this->hasReceivedMessageRecently($user->id, EmailLog::MESSAGE_TYPE_GET_BACKUP_CODES); + && !$this->hasUserReceivedMessageRecently($user->id, EmailLog::MESSAGE_TYPE_GET_BACKUP_CODES); } /** @@ -548,7 +571,7 @@ public function shouldSendLostSecurityKeyMessageTo($user) return false; } - if ($this->hasReceivedMessageRecently($user->id, EmailLog::MESSAGE_TYPE_LOST_SECURITY_KEY)) { + if ($this->hasUserReceivedMessageRecently($user->id, EmailLog::MESSAGE_TYPE_LOST_SECURITY_KEY)) { return false; } @@ -687,7 +710,7 @@ public function sendMethodReminderEmails() foreach ($methods as $method) { $user = $method->user; if (!MySqlDateTime::dateIsRecent($method->created, 3) && - !$this->hasReceivedMessageRecently($user->id, EmailLog::MESSAGE_TYPE_METHOD_REMINDER) + !$this->hasUserReceivedMessageRecently($user->id, EmailLog::MESSAGE_TYPE_METHOD_REMINDER) ) { $this->sendMessageTo( EmailLog::MESSAGE_TYPE_METHOD_REMINDER, @@ -733,7 +756,7 @@ public function sendPasswordExpiringEmails() $passwordExpiry = strtotime($userPassword->getExpiresOn()); if ($passwordExpiry < strtotime(self::PASSWORD_EXPIRING_CUTOFF) && !($passwordExpiry < time()) - && !$this->hasReceivedMessageRecently($user->id, EmailLog::MESSAGE_TYPE_PASSWORD_EXPIRING) + && !$this->hasUserReceivedMessageRecently($user->id, EmailLog::MESSAGE_TYPE_PASSWORD_EXPIRING) ) { $this->sendMessageTo(EmailLog::MESSAGE_TYPE_PASSWORD_EXPIRING, $user); $numEmailsSent++; @@ -775,7 +798,7 @@ public function sendPasswordExpiredEmails() $passwordExpiry = strtotime($userPassword->getExpiresOn()); if ($passwordExpiry < time() && $passwordExpiry > strtotime(self::PASSWORD_EXPIRED_CUTOFF) - && !$this->hasReceivedMessageRecently($user->id, EmailLog::MESSAGE_TYPE_PASSWORD_EXPIRED) + && !$this->hasUserReceivedMessageRecently($user->id, EmailLog::MESSAGE_TYPE_PASSWORD_EXPIRED) ) { $this->sendMessageTo(EmailLog::MESSAGE_TYPE_PASSWORD_EXPIRED, $user); $numEmailsSent++; diff --git a/application/features/bootstrap/EmailContext.php b/application/features/bootstrap/EmailContext.php index f7c89f94..65ccd7c7 100644 --- a/application/features/bootstrap/EmailContext.php +++ b/application/features/bootstrap/EmailContext.php @@ -783,7 +783,10 @@ public function iCheckIfAMfaDisabledEmailShouldBeSent() public function iCheckIfAGetBackupCodesEmailHasBeenSentRecently() { $messageType = EmailLog::MESSAGE_TYPE_GET_BACKUP_CODES; - $this->getBackupCodesEmailHasBeenSent = $this->fakeEmailer->hasReceivedMessageRecently($this->tempUser, $messageType); + $this->getBackupCodesEmailHasBeenSent = $this->fakeEmailer->hasUserReceivedMessageRecently( + $this->tempUser, + $messageType + ); } /** @@ -952,7 +955,10 @@ public function iSeeThatARefreshBackupCodesEmailShouldBeSent() */ public function iSeeThatTheFirstUserHasReceivedALostSecurityKeyEmail() { - Assert::true($this->fakeEmailer->hasReceivedMessageRecently($this->tempUser->id, EmailLog::MESSAGE_TYPE_LOST_SECURITY_KEY)); + Assert::true($this->fakeEmailer->hasUserReceivedMessageRecently( + $this->tempUser->id, + EmailLog::MESSAGE_TYPE_LOST_SECURITY_KEY + )); } /** @@ -960,7 +966,10 @@ public function iSeeThatTheFirstUserHasReceivedALostSecurityKeyEmail() */ public function iSeeThatTheSecondUserHasReceivedAGetBackupCodesEmail() { - Assert::true($this->fakeEmailer->hasReceivedMessageRecently($this->tempUser2->id, EmailLog::MESSAGE_TYPE_GET_BACKUP_CODES)); + Assert::true($this->fakeEmailer->hasUserReceivedMessageRecently( + $this->tempUser2->id, + EmailLog::MESSAGE_TYPE_GET_BACKUP_CODES + )); } /** @@ -968,7 +977,10 @@ public function iSeeThatTheSecondUserHasReceivedAGetBackupCodesEmail() */ public function iSeeThatTheFirstUserHasNotReceivedALostSecurityKeyEmail() { - Assert::false($this->fakeEmailer->hasReceivedMessageRecently($this->tempUser->id, EmailLog::MESSAGE_TYPE_LOST_SECURITY_KEY)); + Assert::false($this->fakeEmailer->hasUserReceivedMessageRecently( + $this->tempUser->id, + EmailLog::MESSAGE_TYPE_LOST_SECURITY_KEY + )); } /** @@ -976,7 +988,10 @@ public function iSeeThatTheFirstUserHasNotReceivedALostSecurityKeyEmail() */ public function iSeeThatTheSecondUserHasNotReceivedAGetBackupCodesEmail() { - Assert::false($this->fakeEmailer->hasReceivedMessageRecently($this->tempUser2->id, EmailLog::MESSAGE_TYPE_GET_BACKUP_CODES)); + Assert::false($this->fakeEmailer->hasUserReceivedMessageRecently( + $this->tempUser2->id, + EmailLog::MESSAGE_TYPE_GET_BACKUP_CODES + )); } /** @@ -1108,7 +1123,7 @@ public function iSendRecoveryMethodReminderEmails() */ public function iSeeThatARecoveryMethodReminderHasNotBeenSent($hasOrHasNot) { - $hasBeenSent = $this->fakeEmailer->hasReceivedMessageRecently( + $hasBeenSent = $this->fakeEmailer->hasUserReceivedMessageRecently( $this->tempUser->id, EmailLog::MESSAGE_TYPE_METHOD_REMINDER ); From 15bde1ed2f878c46d1bcc03fc61fd25a32a18034 Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Wed, 6 Nov 2024 16:03:00 -0500 Subject: [PATCH 10/13] Fix a test step's check for whether a get-backup-codes email was sent --- application/features/bootstrap/EmailContext.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/application/features/bootstrap/EmailContext.php b/application/features/bootstrap/EmailContext.php index 65ccd7c7..a6bcfd03 100644 --- a/application/features/bootstrap/EmailContext.php +++ b/application/features/bootstrap/EmailContext.php @@ -784,7 +784,7 @@ public function iCheckIfAGetBackupCodesEmailHasBeenSentRecently() { $messageType = EmailLog::MESSAGE_TYPE_GET_BACKUP_CODES; $this->getBackupCodesEmailHasBeenSent = $this->fakeEmailer->hasUserReceivedMessageRecently( - $this->tempUser, + $this->tempUser->id, $messageType ); } From be5fb42d10ce107bdc3d46cd81d943fd27ee107b Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Wed, 6 Nov 2024 16:06:03 -0500 Subject: [PATCH 11/13] Clean up / fix some doc-blocks and data types for "received recently" --- application/common/components/Emailer.php | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/application/common/components/Emailer.php b/application/common/components/Emailer.php index 5643d2db..f45a4011 100644 --- a/application/common/components/Emailer.php +++ b/application/common/components/Emailer.php @@ -425,14 +425,13 @@ public function sendDelayedMfaRelatedEmails() } /** - * * Whether the user has already been sent this type of email in the last X days * * @param int $userId * @param string $messageType * @return bool */ - public function hasUserReceivedMessageRecently($userId, string $messageType) + public function hasUserReceivedMessageRecently(int $userId, string $messageType): bool { $latestEmail = EmailLog::find()->where(['user_id' => $userId, 'message_type' => $messageType]) ->orderBy('sent_utc DESC')->one(); @@ -446,11 +445,11 @@ public function hasUserReceivedMessageRecently($userId, string $messageType) /** * Whether the non-user address has already been sent this type of email in the last X days * - * @param int $userId + * @param string $emailAddress * @param string $messageType * @return bool */ - public function hasNonUserReceivedMessageRecently(string $emailAddress, string $messageType) + public function hasNonUserReceivedMessageRecently(string $emailAddress, string $messageType): bool { $latestEmail = EmailLog::find()->where([ 'message_type' => $messageType, From 990b1ac24fccc8ed4db277a7521fd0eac30061ca Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Wed, 6 Nov 2024 16:14:45 -0500 Subject: [PATCH 12/13] Add return data type to `EmailLog::getMessageTypes()` --- application/common/models/EmailLog.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/application/common/models/EmailLog.php b/application/common/models/EmailLog.php index 1a436870..9b8a98cc 100644 --- a/application/common/models/EmailLog.php +++ b/application/common/models/EmailLog.php @@ -47,7 +47,7 @@ public function attributeLabels() ]); } - public static function getMessageTypes() + public static function getMessageTypes(): array { $reflectionClass = new ReflectionClass(__CLASS__); $messageTypes = []; From 39e68fb9e1a2401bb555f7cfbd78550e92af51c3 Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Wed, 6 Nov 2024 16:15:25 -0500 Subject: [PATCH 13/13] Fix checking FakeEmailer for specific types of messages (in tests) --- .../features/bootstrap/EmailContext.php | 25 ++++++++++++------- .../features/bootstrap/fakes/FakeEmailer.php | 7 ++++-- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/application/features/bootstrap/EmailContext.php b/application/features/bootstrap/EmailContext.php index a6bcfd03..262c28b3 100644 --- a/application/features/bootstrap/EmailContext.php +++ b/application/features/bootstrap/EmailContext.php @@ -1255,15 +1255,8 @@ public function iSendAbandonedUserEmail() */ public function theAbandonedUserEmailHasOrHasNotBeenSent($hasOrHasNot) { - $emails = $this->fakeEmailer->getFakeEmailsSent(); - $hasBeenSent = false; - - foreach ($emails as $email) { - if ($email[Emailer::PROP_SUBJECT] === $this->fakeEmailer->subjectForAbandonedUsers) { - $hasBeenSent = true; - break; - } - } + $numberSent = $this->countEmailsSent(EmailLog::MESSAGE_TYPE_ABANDONED_USERS); + $hasBeenSent = ($numberSent > 0); if ($hasOrHasNot === 'has') { Assert::true($hasBeenSent); @@ -1272,6 +1265,20 @@ public function theAbandonedUserEmailHasOrHasNotBeenSent($hasOrHasNot) } } + protected function countEmailsSent(string $messageType): int + { + $emails = $this->fakeEmailer->getFakeEmailsSent(); + $actualCount = 0; + + foreach ($emails as $email) { + $subject = $email[Emailer::PROP_SUBJECT]; + if ($this->fakeEmailer->isSubjectForMessageType($subject, $messageType)) { + $actualCount++; + } + } + return $actualCount; + } + /** * @Given the database has been purged */ diff --git a/application/features/bootstrap/fakes/FakeEmailer.php b/application/features/bootstrap/fakes/FakeEmailer.php index ce73f776..eb7603af 100644 --- a/application/features/bootstrap/fakes/FakeEmailer.php +++ b/application/features/bootstrap/fakes/FakeEmailer.php @@ -64,8 +64,11 @@ public function getFakeEmailsSent() return $this->getEmailServiceClient()->emailsSent; } - public function isSubjectForMessageType(string $subject, string $messageType, ?User $user) - { + public function isSubjectForMessageType( + string $subject, + string $messageType, + ?User $user = null + ): bool { $dataForEmail = ArrayHelper::merge( $user ? $user->getAttributesForEmail() : [], $this->otherDataForEmails