Skip to content

Commit

Permalink
fix: bring back too many recipients check
Browse files Browse the repository at this point in the history
Signed-off-by: Anna Larch <anna@nextcloud.com>
  • Loading branch information
miaulalala committed Mar 19, 2024
1 parent a487baf commit 6dc7866
Show file tree
Hide file tree
Showing 9 changed files with 201 additions and 33 deletions.
5 changes: 4 additions & 1 deletion lib/Controller/DraftsController.php
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ public function create(
array $cc = [],
array $bcc = [],
array $attachments = [],
bool $force = false,
?int $aliasId = null,
?string $inReplyToMessageId = null,
?int $smimeCertificateId = null,
Expand All @@ -121,6 +122,7 @@ public function create(
$message->setSendAt($sendAt);
$message->setSmimeSign($smimeSign);
$message->setSmimeEncrypt($smimeEncrypt);
$message->setForce($force);

if (!empty($smimeCertificateId)) {
$smimeCertificate = $this->smimeService->findCertificate($smimeCertificateId, $this->userId);
Expand Down Expand Up @@ -165,14 +167,14 @@ public function update(int $id,
array $cc = [],
array $bcc = [],
array $attachments = [],
bool $force = false,
?int $aliasId = null,
?string $inReplyToMessageId = null,
?int $smimeCertificateId = null,
?int $sendAt = null): JsonResponse {
$message = $this->service->getMessage($id, $this->userId);
$account = $this->accountService->find($this->userId, $accountId);


$message->setType(LocalMessage::TYPE_DRAFT);
$message->setAccountId($accountId);
$message->setAliasId($aliasId);
Expand All @@ -186,6 +188,7 @@ public function update(int $id,
$message->setUpdatedAt($this->timeFactory->getTime());
$message->setSmimeSign($smimeSign);
$message->setSmimeEncrypt($smimeEncrypt);
$message->setForce($force);

if (!empty($smimeCertificateId)) {
$smimeCertificate = $this->smimeService->findCertificate($smimeCertificateId, $this->userId);
Expand Down
5 changes: 4 additions & 1 deletion lib/Controller/OutboxController.php
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ public function create(
array $cc = [],
array $bcc = [],
array $attachments = [],
bool $force,
?int $draftId = null,
?int $aliasId = null,
?string $inReplyToMessageId = null,
Expand All @@ -139,6 +140,7 @@ public function create(
$message->setSendAt($sendAt);
$message->setSmimeSign($smimeSign);
$message->setSmimeEncrypt($smimeEncrypt);
$message->setForce($force);

if (!empty($smimeCertificateId)) {
$smimeCertificate = $this->smimeService->findCertificate($smimeCertificateId, $this->userId);
Expand Down Expand Up @@ -198,11 +200,11 @@ public function update(
bool $isHtml,
bool $smimeSign,
bool $smimeEncrypt,
bool $failed = false,
array $to = [],
array $cc = [],
array $bcc = [],
array $attachments = [],
bool $force,
?int $aliasId = null,
?string $inReplyToMessageId = null,
?int $smimeCertificateId = null,
Expand All @@ -224,6 +226,7 @@ public function update(
$message->setSendAt($sendAt);
$message->setSmimeSign($smimeSign);
$message->setSmimeEncrypt($smimeEncrypt);
$message->setForce($force);

if (!empty($smimeCertificateId)) {
$smimeCertificate = $this->smimeService->findCertificate($smimeCertificateId, $this->userId);
Expand Down
8 changes: 8 additions & 0 deletions lib/Db/LocalMessage.php
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@
* @method setStatus(?int $status);
* @method string|null getRaw()
* @method setRaw(string|null $raw)
* @method bool getForce()
* @method setForce(bool $force)
*/
class LocalMessage extends Entity implements JsonSerializable {
public const TYPE_OUTGOING = 0;
Expand Down Expand Up @@ -142,6 +144,9 @@ class LocalMessage extends Entity implements JsonSerializable {
/** @var string|null */
protected $raw;

/** @var bool */
protected $force;

public function __construct() {
$this->addType('type', 'integer');
$this->addType('accountId', 'integer');
Expand All @@ -154,6 +159,8 @@ public function __construct() {
$this->addType('smimeCertificateId', 'integer');
$this->addType('smimeEncrypt', 'boolean');
$this->addType('status', 'integer');
$this->addType('force', 'boolean');

}

#[ReturnTypeWillChange]
Expand Down Expand Up @@ -197,6 +204,7 @@ public function jsonSerialize() {
'smimeEncrypt' => $this->getSmimeEncrypt() === true,
'status' => $this->getStatus(),
'raw' => $this->getRaw(),
'force' => $this->getForce(),
];
}

Expand Down
57 changes: 57 additions & 0 deletions lib/Migration/Version3600Date20240319181700.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2024 FIXME Your name <your@email.com>
*
* FIXME @author Your name <your@email.com>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace OCA\Mail\Migration;

use Closure;
use OCP\DB\ISchemaWrapper;
use OCP\DB\Types;
use OCP\Migration\IOutput;
use OCP\Migration\SimpleMigrationStep;

class Version3600Date20240319181700 extends SimpleMigrationStep {

/**
* @param IOutput $output
* @param Closure(): ISchemaWrapper $schemaClosure
* @param array $options
* @return null|ISchemaWrapper
*/
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
/** @var ISchemaWrapper $schema */
$schema = $schemaClosure();

$localMessagesTable = $schema->getTable('mail_local_messages');
if (!$localMessagesTable->hasColumn('force')) {
$localMessagesTable->addColumn('force', Types::BOOLEAN, [
'notnull' => false,
'default' => false,
]);
}

return $schema;
}
}
16 changes: 10 additions & 6 deletions lib/Send/AntiAbuseHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,15 @@
use OCA\Mail\Account;
use OCA\Mail\Db\LocalMessage;
use OCA\Mail\Service\AntiAbuseService;
use OCA\Mail\Service\RecipientsService;
use OCP\IUserManager;
use Psr\Log\LoggerInterface;

class AntiAbuseHandler extends AHandler {

public function __construct(private IUserManager $userManager,
private AntiAbuseService $service,
private AntiAbuseService $antiAbuseService,
private RecipientsService $recipientsService,
private LoggerInterface $logger) {
parent::__construct();
}
Expand All @@ -51,14 +53,16 @@ public function process(Account $account, LocalMessage $localMessage): LocalMess
return $localMessage;
}

$this->service->onBeforeMessageSent(
$this->recipientsService->checkNumberOfRecipients($account, $localMessage);
if($localMessage->getStatus() === LocalMessage::STATUS_TOO_MANY_RECIPIENTS && $localMessage->getForce() === false) {
return $localMessage;
}

// We don't react to the ratelimit at the moment.
$this->antiAbuseService->onBeforeMessageSent(
$user,
$localMessage,
);
// We don't react to a ratelimited message / a message that has too many recipients
// at this point.
// Any future improvement from https://github.com/nextcloud/mail/issues/6461
// should refactor the chain to stop at this point unless the force send option is true
return $this->processNext($account, $localMessage);
}
}
23 changes: 0 additions & 23 deletions lib/Service/AntiAbuseService.php
Original file line number Diff line number Diff line change
Expand Up @@ -69,32 +69,9 @@ public function onBeforeMessageSent(IUser $user, LocalMessage $localMessage): vo
return;
}

$this->checkNumberOfRecipients($user, $localMessage);
$this->checkRateLimits($user, $localMessage);
}

private function checkNumberOfRecipients(IUser $user, LocalMessage $message): void {
$numberOfRecipientsThreshold = (int)$this->config->getAppValue(
Application::APP_ID,
'abuse_number_of_recipients_per_message_threshold',
'0',
);
if ($numberOfRecipientsThreshold <= 1) {
return;
}

$actualNumberOfRecipients = count($message->getRecipients());

if ($actualNumberOfRecipients >= $numberOfRecipientsThreshold) {
$message->setStatus(LocalMessage::STATUS_TOO_MANY_RECIPIENTS);
$this->logger->alert('User {user} sends to a suspicious number of recipients. {expected} are allowed. {actual} are used', [
'user' => $user->getUID(),
'expected' => $numberOfRecipientsThreshold,
'actual' => $actualNumberOfRecipients,
]);
}
}

private function checkRateLimits(IUser $user, LocalMessage $message): void {
if (!$this->cacheFactory->isAvailable()) {
// No cache, no rate limits
Expand Down
15 changes: 13 additions & 2 deletions lib/Service/DraftsService.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
use OCA\Mail\Db\Recipient;
use OCA\Mail\Events\DraftMessageCreatedEvent;
use OCA\Mail\Exception\ClientException;
use OCA\Mail\Exception\ManyRecipientsException;
use OCA\Mail\Exception\ServiceException;
use OCA\Mail\IMAP\IMAPClientFactory;
use OCA\Mail\Service\Attachment\AttachmentService;
Expand Down Expand Up @@ -62,7 +63,8 @@ public function __construct(IMailTransmission $transmission,
IMailManager $mailManager,
LoggerInterface $logger,
AccountService $accountService,
ITimeFactory $time) {
ITimeFactory $time,
private RecipientsService $recipientsService) {
$this->transmission = $transmission;
$this->mapper = $mapper;
$this->attachmentService = $attachmentService;
Expand Down Expand Up @@ -109,6 +111,11 @@ public function deleteMessage(string $userId, LocalMessage $message): void {
* @return LocalMessage
*/
public function saveMessage(Account $account, LocalMessage $message, array $to, array $cc, array $bcc, array $attachments = []): LocalMessage {
$this->recipientsService->checkNumberOfRecipients($account, $message);
if($message->getStatus() === LocalMessage::STATUS_TOO_MANY_RECIPIENTS && $message->getForce() === false) {
throw new ManyRecipientsException();
}

$toRecipients = self::convertToRecipient($to, Recipient::TYPE_TO);
$ccRecipients = self::convertToRecipient($cc, Recipient::TYPE_CC);
$bccRecipients = self::convertToRecipient($bcc, Recipient::TYPE_BCC);
Expand Down Expand Up @@ -147,11 +154,15 @@ public function saveMessage(Account $account, LocalMessage $message, array $to,
* @return LocalMessage
*/
public function updateMessage(Account $account, LocalMessage $message, array $to, array $cc, array $bcc, array $attachments = []): LocalMessage {
$this->recipientsService->checkNumberOfRecipients($account, $message);
if($message->getStatus() === LocalMessage::STATUS_TOO_MANY_RECIPIENTS && $message->getForce() === false) {
throw new ManyRecipientsException();
}

$toRecipients = self::convertToRecipient($to, Recipient::TYPE_TO);
$ccRecipients = self::convertToRecipient($cc, Recipient::TYPE_CC);
$bccRecipients = self::convertToRecipient($bcc, Recipient::TYPE_BCC);


$message = $this->mapper->updateWithRecipients($message, $toRecipients, $ccRecipients, $bccRecipients);

if ($attachments === []) {
Expand Down
102 changes: 102 additions & 0 deletions lib/Service/RecipientsService.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
<?php

declare(strict_types=1);
/*
* @copyright 2024 Anna Larch <anna.larch@gmx.net>
*
* @author Anna Larch <anna.larch@gmx.net>
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU AFFERO GENERAL PUBLIC LICENSE
* License as published by the Free Software Foundation; either
* version 3 of the License, or any later version.
*
* This library is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU AFFERO GENERAL PUBLIC LICENSE for more details.
*
* You should have received a copy of the GNU Affero General Public
* License along with this library. If not, see <http://www.gnu.org/licenses/>.
*/

namespace OCA\Mail\Service;

use OCA\Mail\Account;
use OCA\Mail\AppInfo\Application;
use OCA\Mail\Db\LocalMessage;
use OCA\Mail\Db\Recipient;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\IConfig;
use Psr\Log\LoggerInterface;

class RecipientsService {

public function __construct(
private LoggerInterface $logger,
private IConfig $config,
private GroupsIntegration $groupsIntegration,
) {
}

/**
* @param Account $account
* @param LocalMessage $message
* @return LocalMessage
* @throws DoesNotExistException
*/
public function checkNumberOfRecipients(Account $account, LocalMessage $message): void {
if($message->getForce() === true) {
return;
}

$numberOfRecipientsThreshold = (int)$this->config->getAppValue(
Application::APP_ID,
'abuse_number_of_recipients_per_message_threshold',
'0',
);
if ($numberOfRecipientsThreshold <= 1) {
return;
}

$recipients = $this->groupsIntegration->expand($message->getRecipients());
$to = count(array_filter($recipients, function ($recipient) {
return $recipient->getType() === Recipient::TYPE_TO;
}));
if ($to >= $numberOfRecipientsThreshold) {
$message->setStatus(LocalMessage::STATUS_TOO_MANY_RECIPIENTS);
$this->logger->alert('User {user} sends to a suspicious number of "TO" recipients. {expected} are allowed. {actual} are used', [
'user' => $account->getUserId(),
'expected' => $numberOfRecipientsThreshold,
'actual' => $to,
]);
return;
}

$cc = count(array_filter($recipients, function ($recipient) {
return $recipient->getType() === Recipient::TYPE_CC;
}));
if ($cc >= $numberOfRecipientsThreshold) {
$message->setStatus(LocalMessage::STATUS_TOO_MANY_RECIPIENTS);
$this->logger->alert('User {user} sends to a suspicious number of "CC" recipients. {expected} are allowed. {actual} are used', [
'user' => $account->getUserId(),
'expected' => $numberOfRecipientsThreshold,
'actual' => $cc,
]);
return;
}

$bcc = count(array_filter($recipients, function ($recipient) {
return $recipient->getType() === Recipient::TYPE_BCC;
}));
if ($bcc >= $numberOfRecipientsThreshold) {
$message->setStatus(LocalMessage::STATUS_TOO_MANY_RECIPIENTS);
$this->logger->alert('User {user} sends to a suspicious number of "BCC" recipients. {expected} are allowed. {actual} are used', [
'user' => $account->getUserId(),
'expected' => $numberOfRecipientsThreshold,
'actual' => $bcc,
]);
return;
}
}
}
Loading

0 comments on commit 6dc7866

Please sign in to comment.