Skip to content

Commit 0753046

Browse files
joshtrichardskesselb
joshtrichards
authored and
kesselb
committed
fix(MailTransmission): Handle clients/servers picky about empty headers
Signed-off-by: Josh <josh.t.richards@gmail.com>
1 parent bb4d4b7 commit 0753046

File tree

2 files changed

+141
-54
lines changed

2 files changed

+141
-54
lines changed

‎lib/Service/MailTransmission.php‎

Lines changed: 56 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
use Horde_Imap_Client_Data_Fetch;
1515
use Horde_Imap_Client_Fetch_Query;
1616
use Horde_Imap_Client_Ids;
17+
use Horde_Mail_Rfc822_Address;
18+
use Horde_Mail_Rfc822_List;
1719
use Horde_Mail_Transport_Null;
1820
use Horde_Mail_Transport_Smtphorde;
1921
use Horde_Mime_Exception;
@@ -42,7 +44,6 @@
4244
use OCA\Mail\Exception\ServiceException;
4345
use OCA\Mail\IMAP\IMAPClientFactory;
4446
use OCA\Mail\IMAP\MessageMapper;
45-
use OCA\Mail\Model\Message as ModelMessage;
4647
use OCA\Mail\Model\NewMessageData;
4748
use OCA\Mail\Service\DataUri\DataUriParser;
4849
use OCA\Mail\SMTP\SmtpClientFactory;
@@ -107,13 +108,13 @@ public function sendMessage(Account $account, LocalMessage $localMessage): void
107108

108109
$transport = $this->smtpClientFactory->create($account);
109110
// build mime body
110-
$headers = [
111-
'From' => $from->toHorde(),
112-
'To' => $to->toHorde(),
113-
'Cc' => $cc->toHorde(),
114-
'Bcc' => $bcc->toHorde(),
115-
'Subject' => $localMessage->getSubject(),
116-
];
111+
$headers = $this->buildHeaders(
112+
$from,
113+
$to,
114+
$cc,
115+
$bcc,
116+
$localMessage->getSubject()
117+
);
117118

118119
// The table (oc_local_messages) currently only allows for a single reply to message id
119120
// but we already set the 'references' header for an email so we could support multiple references
@@ -197,41 +198,27 @@ public function saveLocalDraft(Account $account, LocalMessage $message): void {
197198

198199
$perfLogger = $this->performanceLogger->start('save local draft');
199200

200-
$imapMessage = new ModelMessage();
201-
$imapMessage->setTo($to);
202-
$imapMessage->setSubject($message->getSubject());
203-
$from = new AddressList([
204-
Address::fromRaw($account->getName(), $account->getEMailAddress()),
205-
]);
206-
$imapMessage->setFrom($from);
207-
$imapMessage->setCC($cc);
208-
$imapMessage->setBcc($bcc);
209-
if ($message->isHtml() === true) {
210-
$imapMessage->setContent($message->getBodyHtml());
211-
} else {
212-
$imapMessage->setContent($message->getBodyPlain());
213-
}
201+
$from = Address::fromRaw($account->getName(), $account->getEMailAddress());
214202

215203
foreach ($attachments as $attachment) {
216204
$this->transmissionService->handleAttachment($account, $attachment);
217205
}
218206

219207
// build mime body
220-
$headers = [
221-
'From' => $imapMessage->getFrom()->first()->toHorde(),
222-
'To' => $imapMessage->getTo()->toHorde(),
223-
'Cc' => $imapMessage->getCC()->toHorde(),
224-
'Bcc' => $imapMessage->getBCC()->toHorde(),
225-
'Subject' => $imapMessage->getSubject(),
226-
'Date' => Horde_Mime_Headers_Date::create(),
227-
];
208+
$headers = $this->buildHeaders(
209+
$from,
210+
$to,
211+
$cc,
212+
$bcc,
213+
$message->getSubject()
214+
);
228215

229216
$mail = new Horde_Mime_Mail();
230217
$mail->addHeaders($headers);
231218
if ($message->isHtml()) {
232-
$mail->setHtmlBody($imapMessage->getContent());
219+
$mail->setHtmlBody($message->getBodyHtml());
233220
} else {
234-
$mail->setBody($imapMessage->getContent());
221+
$mail->setBody($message->getBodyPlain());
235222
}
236223
$mail->addHeaderOb(Horde_Mime_Headers_MessageId::create());
237224
$perfLogger->step('build local draft message');
@@ -297,33 +284,22 @@ public function saveDraft(NewMessageData $message, ?Message $previousDraft = nul
297284
$perfLogger->step('emit pre event');
298285

299286
$account = $message->getAccount();
300-
$imapMessage = new ModelMessage();
301-
$imapMessage->setTo($message->getTo());
302-
$imapMessage->setSubject($message->getSubject());
303-
$from = new AddressList([
304-
Address::fromRaw($account->getName(), $account->getEMailAddress()),
305-
]);
306-
$imapMessage->setFrom($from);
307-
$imapMessage->setCC($message->getCc());
308-
$imapMessage->setBcc($message->getBcc());
309-
$imapMessage->setContent($message->getBody());
310-
311-
// build mime body
312-
$headers = [
313-
'From' => $imapMessage->getFrom()->first()->toHorde(),
314-
'To' => $imapMessage->getTo()->toHorde(),
315-
'Cc' => $imapMessage->getCC()->toHorde(),
316-
'Bcc' => $imapMessage->getBCC()->toHorde(),
317-
'Subject' => $imapMessage->getSubject(),
318-
'Date' => Horde_Mime_Headers_Date::create(),
319-
];
287+
$from = Address::fromRaw($account->getName(), $account->getEMailAddress());
288+
289+
$headers = $this->buildHeaders(
290+
$from,
291+
$message->getTo(),
292+
$message->getCc(),
293+
$message->getBcc(),
294+
$message->getSubject()
295+
);
320296

321297
$mail = new Horde_Mime_Mail();
322298
$mail->addHeaders($headers);
323299
if ($message->isHtml()) {
324-
$mail->setHtmlBody($imapMessage->getContent());
300+
$mail->setHtmlBody($message->getBody());
325301
} else {
326-
$mail->setBody($imapMessage->getContent());
302+
$mail->setBody($message->getBody());
327303
}
328304
$mail->addHeaderOb(Horde_Mime_Headers_MessageId::create());
329305
$perfLogger->step('build draft message');
@@ -365,6 +341,32 @@ public function saveDraft(NewMessageData $message, ?Message $previousDraft = nul
365341
return [$account, $draftsMailbox, $newUid];
366342
}
367343

344+
/**
345+
* @return array{
346+
* From: Horde_Mail_Rfc822_Address,
347+
* To: Horde_Mail_Rfc822_List,
348+
* Subject: string|null,
349+
* Cc?: Horde_Mail_Rfc822_List,
350+
* Bcc?: Horde_Mail_Rfc822_List,
351+
* }
352+
*/
353+
private function buildHeaders(Address $from, AddressList $to, AddressList $cc, AddressList $bcc, ?string $subject): array {
354+
$headers = [
355+
'From' => $from->toHorde(),
356+
'To' => $to->toHorde(),
357+
'Subject' => $subject,
358+
];
359+
360+
if (count($cc) > 0) {
361+
$headers['Cc'] = $cc->toHorde();
362+
}
363+
if (count($bcc) > 0) {
364+
$headers['Bcc'] = $bcc->toHorde();
365+
}
366+
367+
return $headers;
368+
}
369+
368370
#[\Override]
369371
public function sendMdn(Account $account, Mailbox $mailbox, Message $message): void {
370372
$query = new Horde_Imap_Client_Fetch_Query();

‎tests/Unit/Service/MailTransmissionTest.php‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
use Horde_Imap_Client_Socket;
1414
use Horde_Mail_Transport;
1515
use OCA\Mail\Account;
16+
use OCA\Mail\Address;
17+
use OCA\Mail\AddressList;
1618
use OCA\Mail\Contracts\IMailManager;
1719
use OCA\Mail\Db\Alias;
1820
use OCA\Mail\Db\LocalAttachment;
@@ -433,4 +435,87 @@ public function testCreateDraftsMailboxAndSave(): void {
433435

434436
$this->transmission->saveLocalDraft(new Account($mailAccount), $localMessage);
435437
}
438+
439+
public function testSendMessageCc() {
440+
// Arrange
441+
$mailAccount = new MailAccount();
442+
$mailAccount->setName('Bob');
443+
$mailAccount->setEmail('bob@mail.example');
444+
$mailAccount->setUserId('bob');
445+
$mailAccount->setSentMailboxId(123);
446+
$account = new Account($mailAccount);
447+
$localMessage = new LocalMessage();
448+
$localMessage->setSubject('Test');
449+
$localMessage->setBodyPlain('Test');
450+
$localMessage->setHtml(false);
451+
$transport = $this->createMock(Horde_Mail_Transport::class);
452+
$this->smtpClientFactory->expects($this->once())
453+
->method('create')
454+
->willReturn($transport);
455+
$this->transmissionService->expects(self::once())
456+
->method('getSignMimePart')
457+
->willReturnCallback(static fn ($localMessage, $account, $mimePart) => $mimePart);
458+
$this->transmissionService->expects(self::once())
459+
->method('getEncryptMimePart')
460+
->willReturnCallback(static fn ($localMessage, $to, $cc, $bcc, $account, $mimePart) => $mimePart);
461+
$this->transmissionService->expects(self::exactly(3))
462+
->method('getAddressList')
463+
->willReturnCallback(function ($localMessage, $type) {
464+
$addresses = [];
465+
466+
if ($type === Recipient::TYPE_CC) {
467+
$addresses[] = Address::fromRaw('Alice', 'alice@mail.example');
468+
}
469+
470+
if ($type === Recipient::TYPE_BCC) {
471+
$addresses[] = Address::fromRaw('Jane', 'jane@mail.example');
472+
}
473+
474+
return new AddressList($addresses);
475+
});
476+
477+
478+
// Act
479+
$this->transmission->sendMessage($account, $localMessage);
480+
481+
// Assert
482+
$this->assertEquals(LocalMessage::STATUS_RAW, $localMessage->getStatus());
483+
$this->assertStringContainsString('From: Bob <bob@mail.example>', $localMessage->getRaw());
484+
$this->assertStringContainsString('Cc: Alice <alice@mail.example>', $localMessage->getRaw());
485+
}
486+
487+
public function testSendMessageOmitCc() {
488+
// Arrange
489+
$mailAccount = new MailAccount();
490+
$mailAccount->setName('Bob');
491+
$mailAccount->setEmail('bob@example.org');
492+
$mailAccount->setUserId('bob');
493+
$mailAccount->setSentMailboxId(123);
494+
$account = new Account($mailAccount);
495+
$localMessage = new LocalMessage();
496+
$localMessage->setSubject('Test');
497+
$localMessage->setBodyPlain('Test');
498+
$localMessage->setHtml(false);
499+
$transport = $this->createMock(Horde_Mail_Transport::class);
500+
$this->smtpClientFactory->expects($this->once())
501+
->method('create')
502+
->willReturn($transport);
503+
$this->transmissionService->expects(self::once())
504+
->method('getSignMimePart')
505+
->willReturnCallback(static fn ($localMessage, $account, $mimePart) => $mimePart);
506+
$this->transmissionService->expects(self::once())
507+
->method('getEncryptMimePart')
508+
->willReturnCallback(static fn ($localMessage, $to, $cc, $bcc, $account, $mimePart) => $mimePart);
509+
$this->transmissionService->expects(self::exactly(3))
510+
->method('getAddressList')
511+
->willReturnCallback(static fn ($message, $type) => new AddressList([]));
512+
513+
// Act
514+
$this->transmission->sendMessage($account, $localMessage);
515+
516+
// Assert
517+
$this->assertEquals(LocalMessage::STATUS_RAW, $localMessage->getStatus());
518+
$this->assertStringContainsString('From: Bob <bob@example.org', $localMessage->getRaw());
519+
$this->assertStringNotContainsString('Cc:', $localMessage->getRaw());
520+
}
436521
}

0 commit comments

Comments
 (0)