Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Release 7.0.2 -- reduce memory consumption of password expiry processing #394

Merged
merged 7 commits into from
Feb 12, 2025
Merged

Release 7.0.2 -- reduce memory consumption of password expiry processing #394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
26bec80
reduce memory consumption of password expiry processing
briskt Feb 10, 2025
03b98d3
added some clarifying comments
briskt Feb 10, 2025
d145728
bind the value of emailRepeatDelayDays into the query
briskt Feb 10, 2025
be02f67
extract user/email search SQL to a new method on User class
briskt Feb 11, 2025
7f0ce24
add test scenario for mfa password extension
briskt Feb 11, 2025
bf5a54c
move actionSendPasswordExpiryEmails to the end in case it's still broken
briskt Feb 12, 2025
8a5b4e5
PSR12 requires no spaces on an empty line
briskt Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions application/common/components/Emailer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,10 +776,10 @@ public function sendPasswordExpiringEmails()
'status' => 'starting',
];

$users = User::getActiveUnlockedUsers();
$users = User::getUsersForEmail('password-expiring', $this->emailRepeatDelayDays);

$this->logger->info(array_merge($logData, [
'active_users' => count($users)
'users' => count($users)
]));

$numEmailsSent = 0;
Expand All @@ -790,7 +790,6 @@ public function sendPasswordExpiringEmails()
$passwordExpiry = strtotime($userPassword->getExpiresOn());
if ($passwordExpiry < strtotime(self::PASSWORD_EXPIRING_CUTOFF)
&& !($passwordExpiry < time())
&& !$this->hasUserReceivedMessageRecently($user->id, EmailLog::MESSAGE_TYPE_PASSWORD_EXPIRING)
) {
$this->sendMessageTo(EmailLog::MESSAGE_TYPE_PASSWORD_EXPIRING, $user);
$numEmailsSent++;
Expand Down Expand Up @@ -818,10 +817,10 @@ public function sendPasswordExpiredEmails()
'status' => 'starting',
];

$users = User::getActiveUnlockedUsers();
$users = User::getUsersForEmail('password-expired', $this->emailRepeatDelayDays);

$this->logger->info(array_merge($logData, [
'active_users' => count($users)
'users' => count($users)
]));

$numEmailsSent = 0;
Expand All @@ -832,7 +831,6 @@ public function sendPasswordExpiredEmails()
$passwordExpiry = strtotime($userPassword->getExpiresOn());
if ($passwordExpiry < time()
&& $passwordExpiry > strtotime(self::PASSWORD_EXPIRED_CUTOFF)
&& !$this->hasUserReceivedMessageRecently($user->id, EmailLog::MESSAGE_TYPE_PASSWORD_EXPIRED)
) {
$this->sendMessageTo(EmailLog::MESSAGE_TYPE_PASSWORD_EXPIRED, $user);
$numEmailsSent++;
Expand Down
32 changes: 32 additions & 0 deletions application/common/models/User.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1581,4 +1581,36 @@ public static function exportToSheets()
'status' => 'finish',
]);
}

/*
* Returns a list of active, unlocked users that haven't recently received a given email message.
* @param $template Email template name to use as search criteria
* @param $days Number of days to consider an email sent recently
* @return User[]
*/
public static function getUsersForEmail(string $template, int $days): array
{
$usersArray = \Yii::$app->getDb()->createCommand("SELECT u.*
FROM `user` u
LEFT JOIN `email_log` e ON u.id = e.user_id
AND e.message_type = :template
AND e.sent_utc >= CURRENT_DATE() - INTERVAL :days DAY
WHERE u.active = 'yes'
AND u.locked = 'no'
AND e.id IS NULL
GROUP BY u.id
HAVING COUNT(*) = 1;")
->bindValue('template', $template)
->bindValue('days', $days)
->queryAll();

$users = [];
foreach ($usersArray as $userData) {
$user = new User();
User::populateRecord($user, $userData);
$users[] = $user;
}

return $users;
}
}
5 changes: 3 additions & 2 deletions application/console/controllers/CronController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@

namespace console\controllers;

use common\components\Emailer;
use common\components\ExternalGroupsSync;
use common\models\Invite;
use common\models\Method;
use common\models\Mfa;
use common\models\User;
use common\components\Emailer;
use yii\console\Controller;

class CronController extends Controller
Expand Down Expand Up @@ -86,14 +86,15 @@ public function actionAll()
'actionSendAbandonedUsersEmail',
'actionSendDelayedMfaRelatedEmails',
'actionSendMethodReminderEmails',
'actionSendPasswordExpiryEmails',
'actionSyncExternalGroups',
];

if (\Yii::$app->params['google']['enableSheetsExport']) {
$actions[] = 'actionExportToSheets';
}

$actions[] = 'actionSendPasswordExpiryEmails';

foreach ($actions as $action) {
try {
$this->$action();
Expand Down
22 changes: 22 additions & 0 deletions application/features/email.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,7 @@ Feature: Email
Scenario Outline: When to send password expiring notice email
Given we are configured <toSendOrNot> password expiring emails
And I remove records of any emails that have been sent
And no mfas exist
And a user already exists
And that user has a password that expires in <number> days
And a "password-expiring" email <hasOrHasNot> been sent to that user
Expand All @@ -383,9 +384,30 @@ Feature: Email
| to send | 15 | has | should NOT |
| NOT to send | 15 | has NOT | should NOT |

Scenario Outline: When to send password expiring notice email for a user with MFA enabled
Given we are configured <toSendOrNot> password expiring emails
And the database has been purged
And a user already exists
And that user has a password that expires in <number> days
And a totp mfa option does exist
And a "password-expiring" email <hasOrHasNot> been sent to that user
When I send password expiring emails
Then a "password-expiring" email <shouldOrNot> have been sent to them

Examples:
| toSendOrNot | number | hasOrHasNot | shouldOrNot |
| to send | -1462 | has NOT | should NOT |
| to send | -1461 | has NOT | should |
| to send | -1447 | has NOT | should |
| to send | -1446 | has NOT | should NOT |
| to send | -1461 | has | should NOT |
| to send | -1447 | has | should NOT |
| NOT to send | -1447 | has NOT | should NOT |

Scenario Outline: When to send password expired notice email
Given we are configured <toSendOrNot> password expired emails
And I remove records of any emails that have been sent
And no mfas exist
And a user already exists
And that user has a password that expires in <number> days
And a "password-expired" email <hasOrHasNot> been sent to that user
Expand Down