fixup! fix(outbox): add recipients check

nextcloud · Oct 12, 2023 · c26c54a · c26c54a
1 parent f3ac92b
commit c26c54a
Show file tree

Hide file tree

Showing 7 changed files with 144 additions and 87 deletions.
diff --git a/lib/Contracts/IMailTransmission.php b/lib/Contracts/IMailTransmission.php
@@ -57,7 +57,7 @@ public function sendMessage(NewMessageData $messageData,
 	 * @throws ServiceException
 	 * @return void
 	 */
-	public function sendLocalMessage(Account $account, LocalMessage $message): void;
+	public function sendLocalMessage(Account $account, LocalMessage $message, bool $force): void;
 
 	/**
 	 * @param Account $account

diff --git a/lib/Controller/OutboxController.php b/lib/Controller/OutboxController.php
@@ -156,12 +156,12 @@ public function create(
 	 * @return JsonResponse
 	 */
 	#[TrapError]
-	public function createFromDraft(DraftsService $draftsService, int $id, int $sendAt = null): JsonResponse {
+	public function createFromDraft(DraftsService $draftsService, int $id, int $sendAt = null, bool $force = false): JsonResponse {
 		$draftMessage = $draftsService->getMessage($id, $this->userId);
 		// Locate the account to check authorization
 		$this->accountService->find($this->userId, $draftMessage->getAccountId());
-
 		$outboxMessage = $this->service->convertDraft($draftMessage, $sendAt);
+		$this->service->checkRecpientsCount($outboxMessage, $force);
 
 		return  JsonResponse::success(
 			$outboxMessage,
@@ -242,14 +242,8 @@ public function update(
 	#[TrapError]
 	public function send(int $id, bool $force = false): JsonResponse {
 		$message = $this->service->getMessage($id, $this->userId);
-
-		if($force === false) {
-			$this->service->recipientCheck($message->getRecipients());
-		}
-
 		$account = $this->accountService->find($this->userId, $message->getAccountId());
-
-		$this->service->sendMessage($message, $account);
+		$this->service->sendMessage($message, $account, $force);
 		return  JsonResponse::success(
 			'Message sent', Http::STATUS_ACCEPTED
 		);

diff --git a/lib/Service/MailTransmission.php b/lib/Service/MailTransmission.php
@@ -57,6 +57,7 @@
 use OCA\Mail\Events\SaveDraftEvent;
 use OCA\Mail\Exception\AttachmentNotFoundException;
 use OCA\Mail\Exception\ClientException;
+use OCA\Mail\Exception\ManyRecipientsException;
 use OCA\Mail\Exception\SentMailboxNotSetException;
 use OCA\Mail\Exception\ServiceException;
 use OCA\Mail\Exception\SmimeEncryptException;
@@ -282,37 +283,8 @@ public function sendMessage(NewMessageData $messageData,
 		);
 	}
 
-	public function sendLocalMessage(Account $account, LocalMessage $message): void {
-		$to = new AddressList(
-			array_map(
-				static function ($recipient) {
-					return Address::fromRaw($recipient->getLabel() ?? $recipient->getEmail(), $recipient->getEmail());
-				},
-				$this->groupsIntegration->expand(array_filter($message->getRecipients(), static function (Recipient $recipient) {
-					return $recipient->getType() === Recipient::TYPE_TO;
-				}))
-			)
-		);
-		$cc = new AddressList(
-			array_map(
-				static function ($recipient) {
-					return Address::fromRaw($recipient->getLabel() ?? $recipient->getEmail(), $recipient->getEmail());
-				},
-				$this->groupsIntegration->expand(array_filter($message->getRecipients(), static function (Recipient $recipient) {
-					return $recipient->getType() === Recipient::TYPE_CC;
-				}))
-			)
-		);
-		$bcc = new AddressList(
-			array_map(
-				static function ($recipient) {
-					return Address::fromRaw($recipient->getLabel() ?? $recipient->getEmail(), $recipient->getEmail());
-				},
-				$this->groupsIntegration->expand(array_filter($message->getRecipients(), static function (Recipient $recipient) {
-					return $recipient->getType() === Recipient::TYPE_BCC;
-				}))
-			)
-		);
+	public function sendLocalMessage(Account $account, LocalMessage $message, bool $force): void {
+		$expanded = $this->getRecipients($message, $force);
 		$attachments = array_map(static function (LocalAttachment $attachment) {
 			// Convert to the untyped nested array used in \OCA\Mail\Controller\AccountsController::send
 			return [
@@ -322,9 +294,9 @@ static function ($recipient) {
 		}, $message->getAttachments());
 		$messageData = new NewMessageData(
 			$account,
-			$to,
-			$cc,
-			$bcc,
+			$expanded['to'],
+			$expanded['cc'],
+			$expanded['bcc'],
 			$message->getSubject(),
 			$message->getBody(),
 			$attachments,
@@ -347,7 +319,7 @@ static function ($recipient) {
 	}
 
 	public function saveLocalDraft(Account $account, LocalMessage $message): void {
-		$messageData = $this->getNewMessageData($message, $account);
+		$messageData = $this->getNewMessageData($message, $account, true);
 
 		$perfLogger = $this->performanceLogger->start('save local draft');
 
@@ -771,54 +743,68 @@ public function sendMdn(Account $account, Mailbox $mailbox, Message $message): v
 	 * @param Account $account
 	 * @return NewMessageData
 	 */
-	private function getNewMessageData(LocalMessage $message, Account $account): NewMessageData {
+	private function getNewMessageData(LocalMessage $message, Account $account, bool $force): NewMessageData {
+		$expanded = $this->getRecipients($message, $force);
+		$attachments = array_map(function (LocalAttachment $attachment) {
+			// Convert to the untyped nested array used in \OCA\Mail\Controller\AccountsController::send
+			return [
+				'type' => 'local',
+				'id' => $attachment->getId(),
+			];
+		}, $message->getAttachments());
+		return new NewMessageData(
+			$account,
+			$expanded['to'],
+			$expanded['cc'],
+			$expanded['bcc'],
+			$message->getSubject(),
+			$message->getBody(),
+			$attachments,
+			$message->isHtml()
+		);
+	}
+
+	public function getRecipients(LocalMessage $outboxMessage, bool $force): array {
 		$to = new AddressList(
 			array_map(
 				static function ($recipient) {
 					return Address::fromRaw($recipient->getLabel() ?? $recipient->getEmail(), $recipient->getEmail());
 				},
-				$this->groupsIntegration->expand(array_filter($message->getRecipients(), static function (Recipient $recipient) {
+				$this->groupsIntegration->expand(array_filter($outboxMessage->getRecipients(), static function (Recipient $recipient) {
 					return $recipient->getType() === Recipient::TYPE_TO;
 				}))
 			)
 		);
-
+		if(count($to) > 10 && $force === false) {
+			throw new ManyRecipientsException();
+		}
 		$cc = new AddressList(
 			array_map(
 				static function ($recipient) {
 					return Address::fromRaw($recipient->getLabel() ?? $recipient->getEmail(), $recipient->getEmail());
 				},
-				$this->groupsIntegration->expand(array_filter($message->getRecipients(), static function (Recipient $recipient) {
+				$this->groupsIntegration->expand(array_filter($outboxMessage->getRecipients(), static function (Recipient $recipient) {
 					return $recipient->getType() === Recipient::TYPE_CC;
 				}))
 			)
 		);
+		if(count($cc) > 10 && $force === false) {
+			throw new ManyRecipientsException();
+		}
 		$bcc = new AddressList(
 			array_map(
 				static function ($recipient) {
 					return Address::fromRaw($recipient->getLabel() ?? $recipient->getEmail(), $recipient->getEmail());
 				},
-				$this->groupsIntegration->expand(array_filter($message->getRecipients(), static function (Recipient $recipient) {
+				$this->groupsIntegration->expand(array_filter($outboxMessage->getRecipients(), static function (Recipient $recipient) {
 					return $recipient->getType() === Recipient::TYPE_BCC;
 				}))
 			)
 		);
-		$attachments = array_map(function (LocalAttachment $attachment) {
-			// Convert to the untyped nested array used in \OCA\Mail\Controller\AccountsController::send
-			return [
-				'type' => 'local',
-				'id' => $attachment->getId(),
-			];
-		}, $message->getAttachments());
-		return new NewMessageData(
-			$account,
-			$to,
-			$cc,
-			$bcc,
-			$message->getSubject(),
-			$message->getBody(),
-			$attachments,
-			$message->isHtml()
-		);
+		if(count($bcc) > 10 && $force === false) {
+			throw new ManyRecipientsException();
+		}
+
+		return ['to' => $to, 'cc' => $cc, 'bcc' => $bcc];
 	}
 }
diff --git a/lib/Service/OutboxService.php b/lib/Service/OutboxService.php
@@ -26,7 +26,10 @@
 
 namespace OCA\Mail\Service;
 
+use OC\Group\Group;
 use OCA\Mail\Account;
+use OCA\Mail\Address;
+use OCA\Mail\AddressList;
 use OCA\Mail\Contracts\IMailManager;
 use OCA\Mail\Contracts\IMailTransmission;
 use OCA\Mail\Db\LocalMessage;
@@ -71,6 +74,7 @@ class OutboxService {
 
 	/** @var LoggerInterface */
 	private $logger;
+	private $groupsIntegration;
 
 	public function __construct(IMailTransmission  $transmission,
 		LocalMessageMapper $mapper,
@@ -80,7 +84,8 @@ public function __construct(IMailTransmission  $transmission,
 		IMailManager $mailManager,
 		AccountService $accountService,
 		ITimeFactory $timeFactory,
-		LoggerInterface $logger) {
+		LoggerInterface $logger,
+		GroupsIntegration $groupsIntegration) {
 		$this->transmission = $transmission;
 		$this->mapper = $mapper;
 		$this->attachmentService = $attachmentService;
@@ -90,6 +95,7 @@ public function __construct(IMailTransmission  $transmission,
 		$this->timeFactory = $timeFactory;
 		$this->logger = $logger;
 		$this->accountService = $accountService;
+		$this->groupsIntegration = $groupsIntegration;
 	}
 
 	/**
@@ -138,9 +144,9 @@ public function deleteMessage(string $userId, LocalMessage $message): void {
 	 * @throws ClientException
 	 * @throws ServiceException
 	 */
-	public function sendMessage(LocalMessage $message, Account $account): void {
+	public function sendMessage(LocalMessage $message, Account $account, bool $force): void {
 		try {
-			$this->transmission->sendLocalMessage($account, $message);
+			$this->transmission->sendLocalMessage($account, $message, $force);
 		} catch (ClientException|ServiceException $e) {
 			// Mark as failed so the message is not sent repeatedly in background
 			$message->setFailed(true);
@@ -228,7 +234,7 @@ public function handleDraft(Account $account, int $draftId): void {
 	/**
 	 * @return void
 	 */
-	public function flush(): void {
+	public function flush(bool $force = false): void {
 		$messages = $this->mapper->findDue(
 			$this->timeFactory->getTime()
 		);
@@ -261,6 +267,7 @@ public function flush(): void {
 				$this->sendMessage(
 					$message,
 					$account,
+					$force
 				);
 				$this->logger->debug('Outbox message {id} sent', [
 					'id' => $message->getId(),
@@ -276,23 +283,14 @@ public function flush(): void {
 		}
 	}
 
-	public function convertDraft(LocalMessage $draftMessage, int $sendAt): LocalMessage {
+	public function convertDraft(LocalMessage $draftMessage, int $sendAt, bool $force = false): LocalMessage {
 		if (empty($draftMessage->getRecipients())) {
 			throw new ClientException('Cannot convert message to outbox message without at least one recipient');
 		}
+		$this->transmission->checkRecipients($draftMessage, $force);
 		$outboxMessage = clone $draftMessage;
 		$outboxMessage->setType(LocalMessage::TYPE_OUTGOING);
 		$outboxMessage->setSendAt($sendAt);
 		return $this->mapper->update($outboxMessage);
 	}
-
-	/**
-	 * @param array $recipients
-	 * @return void
-	 */
-	public function recipientCheck(array $recipients): void {
-		if(count($recipients) > 10) {
-			throw new ManyRecipientsException();
-		}
-	}
 }
diff --git a/tests/Integration/Service/MailTransmissionIntegrationTest.php b/tests/Integration/Service/MailTransmissionIntegrationTest.php
@@ -35,6 +35,7 @@
 use OCA\Mail\Db\MailboxMapper;
 use OCA\Mail\Db\Message;
 use OCA\Mail\Db\Recipient;
+use OCA\Mail\Exception\ManyRecipientsException;
 use OCA\Mail\IMAP\IMAPClientFactory;
 use OCA\Mail\IMAP\MailboxSync;
 use OCA\Mail\IMAP\MessageMapper;
@@ -279,9 +280,55 @@ public function testSendLocalMessage(): void {
 		$localMessage->setRecipients([$to]);
 		$localMessage->setAttachments([]);
 
-		$this->transmission->sendLocalMessage($this->account, $localMessage);
+		$this->transmission->sendLocalMessage($this->account, $localMessage, false);
 
 		$this->assertMailboxExists('Sent');
 		$this->assertMessageCount(1, 'Sent');
 	}
+
+	public function testSendLocalMessageTooManyRecipients(): void {
+		$localMessage = new LocalMessage();
+		$to = [];
+		for($i = 0; $i < 12; $i++) {
+			$r = new Recipient();
+			$r->setLabel('Penny ' . $i);
+			$r->setEmail('library' . $i . '@stardewvalley.edu');
+			$r->setType(Recipient::TYPE_TO);
+			$to[] = $r;
+		}
+
+		$localMessage->setType(LocalMessage::TYPE_OUTGOING);
+		$localMessage->setSubject('hello');
+		$localMessage->setBody('This is a test');
+		$localMessage->setHtml(false);
+		$localMessage->setRecipients($to);
+		$localMessage->setAttachments([]);
+
+		$this->expectException(ManyRecipientsException::class);
+		$this->transmission->sendLocalMessage($this->account, $localMessage, false);
+	}
+
+	public function testSendLocalMessageTooManyRecipientsWithForce(): void {
+		$localMessage = new LocalMessage();
+		$to = [];
+		for($i = 0; $i < 12; $i++) {
+			$r = new Recipient();
+			$r->setLabel('Penny ' . $i);
+			$r->setEmail('library' . $i . '@stardewvalley.edu');
+			$r->setType(Recipient::TYPE_TO);
+			$to[] = $r;
+		}
+
+		$localMessage->setType(LocalMessage::TYPE_OUTGOING);
+		$localMessage->setSubject('hello');
+		$localMessage->setBody('This is a test');
+		$localMessage->setHtml(false);
+		$localMessage->setRecipients($to);
+		$localMessage->setAttachments([]);
+
+		$this->transmission->sendLocalMessage($this->account, $localMessage, true);
+
+		$this->assertMailboxExists('Sent');
+		$this->assertMessageCount(11, 'Sent');
+	}
 }
diff --git a/tests/Unit/Service/MailTransmissionTest.php b/tests/Unit/Service/MailTransmissionTest.php
@@ -467,7 +467,7 @@ public function testSendLocalMessage(): void {
 		$replyMessage = new DbMessage();
 		$replyMessage->setMessageId('abc');
 
-		$this->transmission->sendLocalMessage(new Account($mailAccount), $message);
+		$this->transmission->sendLocalMessage(new Account($mailAccount), $message, false);
 	}
 
 	public function testConvertInlineImageToAttachment() {
@@ -600,6 +600,6 @@ public function testSendLocalDraftNoDraftsMailbox(): void {
 		$replyMessage->setMessageId('abc');
 
 		$this->expectException(ClientException::class);
-		$this->transmission->sendLocalMessage(new Account($mailAccount), $message);
+		$this->transmission->sendLocalMessage(new Account($mailAccount), $message, false);
 	}
 }