Skip to content

Commit

Permalink
Merge pull request #387 from silinternational/feature/IDP-1266-part-2…
Browse files Browse the repository at this point in the history 
…-rate-limit-both-non-user-emails

[IDP-1266, part 2] Rate limit both non user emails, add EmailLog validation
  • Loading branch information
@forevermatt
forevermatt authored Nov 13, 2024
2 parents 4d03cd4 + a076663 commit 5fea506
Show file tree
Hide file tree
Showing 4 changed files with 197 additions and 22 deletions.
68 changes: 49 additions & 19 deletions application/common/components/Emailer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,26 @@ public function shouldSendAbandonedUsersMessage(): bool
return !$haveSentAbandonedUsersEmailRecently;
}

/**
* Whether we should send an external-groups sync-errors email to the given
* email address.
*
* @param string $emailAddress
* @return bool
*/
public function shouldSendExternalGroupsSyncErrorsEmailTo(string $emailAddress): bool
{
if (empty($emailAddress)) {
return false;
}

$haveSentEmailRecently = $this->hasNonUserReceivedMessageRecently(
$emailAddress,
EmailLog::MESSAGE_TYPE_EXT_GROUP_SYNC_ERRORS
);

return !$haveSentEmailRecently;
}

/**
* Whether we should send an invite message to the given User.
Expand Down Expand Up @@ -819,28 +839,38 @@ public function sendExternalGroupSyncErrorsEmail(
) {
$logData = [
'action' => 'send external-groups sync errors email',
'status' => 'starting',
'prefix' => $appPrefix,
];

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

$this->sendMessageTo(
EmailLog::MESSAGE_TYPE_EXT_GROUP_SYNC_ERRORS,
null,
[
'toAddress' => $recipient,
'appPrefix' => $appPrefix,
'errors' => $errors,
'googleSheetUrl' => $googleSheetUrl,
'idpDisplayName' => \Yii::$app->params['idpDisplayName'],
]
);
if (!$this->shouldSendExternalGroupsSyncErrorsEmailTo($recipient)) {
$this->logger->info(array_merge($logData, [
'errors' => count($errors),
'recipient' => $recipient,
'status' => 'skipping (too soon to resend)',
]));
} else {
$this->logger->info(array_merge($logData, [
'errors' => count($errors),
'recipient' => $recipient,
'status' => 'starting',
]));

$this->sendMessageTo(
EmailLog::MESSAGE_TYPE_EXT_GROUP_SYNC_ERRORS,
null,
[
'toAddress' => $recipient,
'appPrefix' => $appPrefix,
'errors' => $errors,
'googleSheetUrl' => $googleSheetUrl,
'idpDisplayName' => \Yii::$app->params['idpDisplayName'],
]
);

$this->logger->info(array_merge($logData, [
'status' => 'finished',
]));
$this->logger->info(array_merge($logData, [
'status' => 'finished',
]));
}
}

/**
Expand Down
47 changes: 47 additions & 0 deletions application/common/models/EmailLog.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,53 @@ public function rules()
'default',
'value' => MySqlDateTime::now(),
],
[
'non_user_address',
'emptyIfUserIsSet',
],
[
'non_user_address',
'requiredIfUserIsEmpty',
'skipOnEmpty' => false,
],
], parent::rules());
}

/**
* Validate an attribute to ensure it is empty (unset) if `user_id` is set.
*
* @param string $attribute Attribute name to be validated
* @return bool
*/
public function emptyIfUserIsSet($attribute)
{
if (!empty($this->user_id) && !empty($this[$attribute])) {
$this->addError(
$attribute,
sprintf(
'Only a user_id or a %s may be set, not both',
$attribute
)
);
}
}

/**
* Validate an attribute to ensure it is not empty if `user_id` is not set.
*
* @param string $attribute Attribute name to be validated
* @return bool
*/
public function requiredIfUserIsEmpty($attribute)
{
if (empty($this->user_id) && empty($this[$attribute])) {
$this->addError(
$attribute,
sprintf(
'Either a user_id or a %s must be set',
$attribute
)
);
}
}
}
79 changes: 77 additions & 2 deletions application/features/bootstrap/EmailContext.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
use common\models\MfaBackupcode;
use common\models\MfaWebauthn;
use common\models\User;
use Sil\SilIdBroker\Behat\Context\YiiContext;
use Webmozart\Assert\Assert;

class EmailContext extends YiiContext
Expand Down Expand Up @@ -59,6 +58,10 @@ class EmailContext extends YiiContext
/** @var array<array> */
protected $matchingFakeEmails;

private string $dummyExtGroupsAppPrefix = 'ext-dummy';

private ?EmailLog $tempEmailLog;

public const METHOD_EMAIL_ADDRESS = '[email protected]';
public const MANAGER_EMAIL = '[email protected]';

Expand Down Expand Up @@ -1243,7 +1246,7 @@ public function theUserHasNotLoggedInFor($months)
}

/**
* @When I send abandoned user email
* @When I send abandoned user email (again)
*/
public function iSendAbandonedUserEmail()
{
Expand Down Expand Up @@ -1292,4 +1295,76 @@ public function theDatabaseHasBeenPurged()
User::deleteAll();
$this->fakeEmailer->forgetFakeEmailsSent();
}

/**
* @Then the abandoned user email has been sent :expectedCount time(s)
*/
public function theAbandonedUserEmailHasBeenSentTime($expectedCount)
{
$actualCount = $this->countEmailsSent(EmailLog::MESSAGE_TYPE_ABANDONED_USERS);
Assert::eq($actualCount, $expectedCount);
}

/**
* @Given I send an external-groups sync-error email (again)
*/
public function iSendAnExternalGroupsSyncErrorEmail()
{
$this->fakeEmailer->sendExternalGroupSyncErrorsEmail(
$this->dummyExtGroupsAppPrefix,
['dummy error'],
'[email protected]',
''
);
}

/**
* @Then the external-groups sync-error email has been sent :expectedCount time(s)
*/
public function theExternalGroupsSyncErrorEmailHasBeenSentTime($expectedCount)
{
// The $appPrefix is needed by the FakeEmailer, to find the appropriate
// emails (by being able to accurately generate the expected subject).
$this->fakeEmailer->otherDataForEmails['appPrefix'] = $this->dummyExtGroupsAppPrefix;

$actualCount = $this->countEmailsSent(
EmailLog::MESSAGE_TYPE_EXT_GROUP_SYNC_ERRORS
);
Assert::eq($actualCount, $expectedCount);
}

/**
* @When I try to log an email as sent to that User and to a non-user address
*/
public function iTryToLogAnEmailAsSentToThatUserAndToANonUserAddress()
{
$this->tempEmailLog = new EmailLog([
'message_type' => EmailLog::MESSAGE_TYPE_ABANDONED_USERS,
'non_user_address' => '[email protected]',
'user_id' => $this->tempUser->id,
]);
$this->tempEmailLog->save();
}

/**
* @Then an email log validation error should have occurred
*/
public function anEmailLogValidationErrorShouldHaveOccurred()
{
Assert::notEmpty(
$this->tempEmailLog->errors,
'No EmailLog validation errors were found'
);
}

/**
* @When I try to log an email as sent to neither a User nor a non-user address
*/
public function iTryToLogAnEmailAsSentToNeitherAUserNorANonUserAddress()
{
$this->tempEmailLog = new EmailLog([
'message_type' => EmailLog::MESSAGE_TYPE_ABANDONED_USERS,
]);
$this->tempEmailLog->save();
}
}
25 changes: 24 additions & 1 deletion application/features/email.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,6 @@ Feature: Email
| to send | -1 | has | should NOT |
| NOT to send | -1 | has NOT | should NOT |


Scenario Outline: HR notification users
Given hr notification email <isOrIsNot> set
And the database has been purged
Expand All @@ -423,3 +422,27 @@ Feature: Email
| is | an inactive | 7 months | has NOT |
| is | a | 5 months | has NOT |
| is | a | 7 months | has |

Scenario: Not sending HR notification emails too frequently
Given hr notification email is set
And the database has been purged
And a user already exists
And the user has not logged in for "7 months"
And I send abandoned user email
When I send abandoned user email again
Then the abandoned user email has been sent 1 time

Scenario: Not sending external-group sync-error emails too frequently
Given the database has been purged
And I send an external-groups sync-error email
When I send an external-groups sync-error email again
Then the external-groups sync-error email has been sent 1 time

Scenario: Ensure no EmailLog is to both a User and a non-user address
Given a user already exists
When I try to log an email as sent to that User and to a non-user address
Then an email log validation error should have occurred

Scenario: Ensure each EmailLog is to either a User or a non-user address
When I try to log an email as sent to neither a User nor a non-user address
Then an email log validation error should have occurred

0 comments on commit 5fea506

Please sign in to comment.