Fix sending messages to groups

Groups were expanded in the accounts controller. Since moving over to
the outbox logic this feature was missing and internal group identifiers
were passed to SMTP.

With this patch groups are expanded again just before a message is sent.
This means the group memberships are read as late as possible and
editing an outbox message looks like the original message because
members have not been expanded there yet.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

lib/Controller/AccountsController.php

@@ -410,16 +410,12 @@ public function send(int $id,
 		$account = $this->accountService->find($this->currentUserId, $id);
 		$alias = $aliasId ? $this->aliasesService->find($aliasId, $this->currentUserId) : null;
 
-		$expandedTo = $this->groupsIntegration->expand($to);
-		$expandedCc = $this->groupsIntegration->expand($cc);
-		$expandedBcc = $this->groupsIntegration->expand($bcc);
-
-		$count = substr_count($expandedTo, ',') + substr_count($expandedCc, ',') + 1;
+		$count = substr_count($to, ',') + substr_count($cc, ',') + 1;
 		if (!$force && $count >= 10) {
 			throw new ManyRecipientsException();
 		}
 
-		$messageData = NewMessageData::fromRequest($account, $expandedTo, $expandedCc, $expandedBcc, $subject, $body, $attachments, $isHtml, $requestMdn);
+		$messageData = NewMessageData::fromRequest($account, $to, $cc, $bcc, $subject, $body, $attachments, $isHtml, $requestMdn);
 		if ($messageId !== null) {
 			try {
 				$repliedMessage = $this->mailManager->getMessage($this->currentUserId, $messageId);

lib/Service/GroupsIntegration.php

@@ -23,10 +23,16 @@
 
 namespace OCA\Mail\Service;
 
+use OCA\Mail\Db\Recipient;
 use OCA\Mail\Service\Group\ContactsGroupService;
 use OCA\Mail\Service\Group\IGroupService;
 use OCA\Mail\Service\Group\NextcloudGroupService;
 use OCA\Mail\Exception\ServiceException;
+use function array_map;
+use function mb_strlen;
+use function mb_strpos;
+use function mb_substr;
+use function OCA\Mail\array_flat_map;
 
 class GroupsIntegration {
 
@@ -82,34 +88,38 @@ public function servicePrefix(IGroupService $gs): string {
 	}
 
 	/**
-	 * Expands a string of group names to its members email addresses.
+	 * Expands group names to its members
 	 *
-	 * @param string $recipients
+	 * @param Recipient[] $recipients
 	 *
-	 * @return null|string
+	 * @return Recipient[]
 	 */
-	public function expand(string $recipients): ?string {
-		return array_reduce($this->groupServices,
-			function ($carry, $service) {
-				return preg_replace_callback(
-					'/' . preg_quote($this->servicePrefix($service)) . '([^,]+)(,?)/',
-					function ($matches) use ($service) {
-						if (empty($matches[1])) {
-							return '';
-						}
-						$members = $service->getUsers($matches[1]);
-						if (empty($members)) {
-							throw new ServiceException($matches[1] . " ({$service->getNamespace()}) has no members");
-						}
-						$addresses = [];
-						foreach ($members as $m) {
-							if (!empty($m['email'])) {
-								$addresses[] = $m['email'];
-							}
-						}
-						return implode(',', $addresses)
-							. (!empty($matches[2]) && !empty($addresses) ? ',' : '');
-					}, $carry);
-			}, $recipients);
+	public function expand(array $recipients): array {
+		return array_flat_map(function (Recipient $recipient) {
+			foreach ($this->groupServices as $service) {
+				if (mb_strpos($recipient->getEmail(), $this->servicePrefix($service)) !== false) {
+					$groupId = mb_substr(
+						$recipient->getEmail(),
+						mb_strlen($this->servicePrefix($service))
+					);
+					$members = array_filter($service->getUsers($groupId), function (array $member) {
+						return !empty($member['email']);
+					});
+					if (empty($members)) {
+						throw new ServiceException($groupId . " ({$service->getNamespace()}) has no members with email addresses");
+					}
+					return array_map(function (array $member) use ($recipient) {
+						return Recipient::fromParams([
+							'messageId' => $recipient->getMessageId(),
+							'type' => $recipient->getType(),
+							'label' => $member['name'] ?? $member['email'],
+							'email' => $member['email'],
+						]);
+					}, $members);
+				}
+			}
+
+			return [$recipient];
+		}, $recipients);
 	}
 }

lib/Service/MailTransmission.php

@@ -71,13 +71,11 @@
 use OCP\Files\File;
 use OCP\Files\Folder;
 use Psr\Log\LoggerInterface;
+use function array_filter;
 use function array_map;
 
 class MailTransmission implements IMailTransmission {
 
-	/** @var AccountService */
-	private $accountService;
-
 	/** @var Folder */
 	private $userFolder;
 
@@ -111,11 +109,13 @@ class MailTransmission implements IMailTransmission {
 	/** @var AliasesService */
 	private $aliasesService;
 
+	/** @var GroupsIntegration */
+	private $groupsIntegration;
+
 	/**
 	 * @param Folder $userFolder
 	 */
 	public function __construct($userFolder,
-								AccountService $accountService,
 								IAttachmentService $attachmentService,
 								IMailManager $mailManager,
 								IMAPClientFactory $imapClientFactory,
@@ -125,8 +125,8 @@ public function __construct($userFolder,
 								MessageMapper $messageMapper,
 								LoggerInterface $logger,
 								PerformanceLogger $performanceLogger,
-								AliasesService $aliasesService) {
-		$this->accountService = $accountService;
+								AliasesService $aliasesService,
+								GroupsIntegration $groupsIntegration) {
 		$this->userFolder = $userFolder;
 		$this->attachmentService = $attachmentService;
 		$this->mailManager = $mailManager;
@@ -138,6 +138,7 @@ public function __construct($userFolder,
 		$this->logger = $logger;
 		$this->performanceLogger = $performanceLogger;
 		$this->aliasesService = $aliasesService;
+		$this->groupsIntegration = $groupsIntegration;
 	}
 
 	public function sendMessage(NewMessageData $messageData,
@@ -221,27 +222,33 @@ public function sendMessage(NewMessageData $messageData,
 
 	public function sendLocalMessage(Account $account, LocalMessage $message): void {
 		$to = new AddressList(
-				array_map(static function ($recipient) {
+			array_map(
+				static function ($recipient) {
 					return Address::fromRaw($recipient->getLabel() ?? $recipient->getEmail(), $recipient->getEmail());
-				}, array_filter($message->getRecipients(), static function (Recipient $recipient) {
+				},
+				$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());
-			}, array_filter($message->getRecipients(), static function (Recipient $recipient) {
-				return $recipient->getType() === Recipient::TYPE_CC;
-			})
+			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());
-			}, array_filter($message->getRecipients(), static function (Recipient $recipient) {
-				return $recipient->getType() === Recipient::TYPE_BCC;
-			})
+			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;
+				}))
 			)
 		);
 		$attachments = array_map(function (LocalAttachment $attachment) {

tests/Integration/Service/MailTransmissionIntegrationTest.php

@@ -40,9 +40,9 @@
 use OCA\Mail\IMAP\MessageMapper;
 use OCA\Mail\Model\NewMessageData;
 use OCA\Mail\Model\RepliedMessageData;
-use OCA\Mail\Service\AccountService;
 use OCA\Mail\Service\AliasesService;
 use OCA\Mail\Service\Attachment\UploadedFile;
+use OCA\Mail\Service\GroupsIntegration;
 use OCA\Mail\Service\MailTransmission;
 use OCA\Mail\SMTP\SmtpClientFactory;
 use OCA\Mail\Support\PerformanceLogger;
@@ -109,7 +109,6 @@ protected function setUp(): void {
 
 		$this->transmission = new MailTransmission(
 			$userFolder,
-			OC::$server->query(AccountService::class),
 			$this->attachmentService,
 			OC::$server->query(IMailManager::class),
 			OC::$server->query(IMAPClientFactory::class),
@@ -119,7 +118,8 @@ protected function setUp(): void {
 			OC::$server->query(MessageMapper::class),
 			OC::$server->query(LoggerInterface::class),
 			OC::$server->query(PerformanceLogger::class),
-			OC::$server->get(AliasesService::class)
+			OC::$server->get(AliasesService::class),
+			OC::$server->get(GroupsIntegration::class)
 		);
 	}