Fix sending messages to groupsfix/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>

3 files changed, 64 insertions, 51 deletions

diff --git a/lib/Controller/AccountsController.php b/lib/Controller/AccountsController.php
index 4ab70b4cf..aca242180 100644
--- a/lib/Controller/AccountsController.php
+++ b/lib/Controller/AccountsController.php
@@ -410,16 +410,12 @@ class AccountsController extends Controller {
 		$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);
diff --git a/lib/Service/GroupsIntegration.php b/lib/Service/GroupsIntegration.php
index 65be28c5b..d440cd2e9 100644
--- a/lib/Service/GroupsIntegration.php
+++ b/lib/Service/GroupsIntegration.php
@@ -23,10 +23,16 @@ declare(strict_types=1);
 
 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 @@ class GroupsIntegration {
 	}
 
 	/**
-	 * 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);
 	}
 }
diff --git a/lib/Service/MailTransmission.php b/lib/Service/MailTransmission.php
index 9a71343fe..5773d93fa 100644
--- a/lib/Service/MailTransmission.php
+++ b/lib/Service/MailTransmission.php
@@ -71,13 +71,11 @@ use OCP\EventDispatcher\IEventDispatcher;
 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 @@ class MailTransmission implements IMailTransmission {
 								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 @@ class MailTransmission implements IMailTransmission {
 		$this->logger = $logger;
 		$this->performanceLogger = $performanceLogger;
 		$this->aliasesService = $aliasesService;
+		$this->groupsIntegration = $groupsIntegration;
 	}
 
 	public function sendMessage(NewMessageData $messageData,
@@ -221,27 +222,33 @@ class MailTransmission implements IMailTransmission {
 
 	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) {