Skip to content

feat: jmap support - part 2b - #12871

Open
SebastianKrupinski wants to merge 16 commits into
mainfrom
feat/jmap-2b
Open

SebastianKrupinski wants to merge 16 commits into
mainfrom
feat/jmap-2b

Conversation

@SebastianKrupinski

@SebastianKrupinski SebastianKrupinski commented May 4, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Testing

  • This PR coverts message sending ONLY

  • Basically every operation that interacts with a external Imap or Jmap server needs to be tested... Twice... once on a Imap account/server then on a Jmap account/server

Message Operations

  • Send Message

@SebastianKrupinski
SebastianKrupinski force-pushed the feat/jmap-2b branch 4 times, most recently from 4655ec5 to 6d90ed1 Compare May 4, 2026 23:02
@SebastianKrupinski SebastianKrupinski changed the title feat: jmap support - 2b feat: jmap support - part 2b May 5, 2026
@SebastianKrupinski SebastianKrupinski self-assigned this May 5, 2026
@github-project-automation github-project-automation Bot moved this to 🏗️ In progress in 💌 📅 👥 Groupware team May 5, 2026
@SebastianKrupinski
SebastianKrupinski force-pushed the feat/jmap-2b branch 2 times, most recently from 9a63ade to 8c758e3 Compare May 5, 2026 14:32
@SebastianKrupinski SebastianKrupinski mentioned this pull request May 5, 2026
60 of 65 tasks
@SebastianKrupinski
SebastianKrupinski force-pushed the feat/jmap-2b branch 2 times, most recently from 8a87336 to bd44a04 Compare May 5, 2026 19:39
@odzhychko

Copy link
Copy Markdown
Contributor

@SebastianKrupinski I finished the first round of testing 😄

@SebastianKrupinski

Copy link
Copy Markdown
Contributor Author

@SebastianKrupinski I finished the first round of testing 😄

Thank you so much!!! Totally appreciate it!!!

@SebastianKrupinski

Copy link
Copy Markdown
Contributor Author

I cannot connect to a server. For some reason HTTP is used instead of HTTPS:

My bad. I used the a wrong configuration value ("ssl" instead of "yes") See https://github.com/nextcloud/mail/pull/12711/changes#r3512730771

I will continue testing.

Fixed

@SebastianKrupinski

Copy link
Copy Markdown
Contributor Author

Hey @odzhychko

You can retest the sending it has been fixed.

I've also rebase to the most recent version of mail

Comment thread lib/Protocol/ProtocolFactory.php
Comment thread lib/Protocol/ProtocolFactory.php
@odzhychko

Copy link
Copy Markdown
Contributor

Ready with retesting the opened issues.

  • One I could reproduce anymore
  • One I could reproduce and added more details
  • Found two new issues

@SebastianKrupinski

Copy link
Copy Markdown
Contributor Author

Ready with retesting the opened issues.

* One I could reproduce anymore

* One I could reproduce and added more details

* Found two new issues

Okay, all these are fix/resolved, and changes pushed. I think that takes care of all the issues, you found

@SebastianKrupinski

Copy link
Copy Markdown
Contributor Author

@kesselb @hamza221

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

JMAP can bypass requested S/MIME protection and contains failure paths that may duplicate sent messages.

8 open findings
What changed in this PR

Adds protocol-neutral transmission support, enabling JMAP sending while moving existing IMAP transmission logic behind connectors.

Changes:

  • Adds IMAP and JMAP transmission connectors.
  • Refactors sending, drafts, attachments, and handlers around protocol abstractions.
  • Updates unit and integration tests for the new architecture.
File Description
tests/​Unit/​Service/​TransmissionServiceTest.php Updates attachment-content tests.
tests/​Unit/​Service/​OutboxServiceTest.php Removes direct IMAP dependencies.
tests/​Unit/​Service/​MailTransmissionTest.php Removes obsolete service tests.
tests/​Unit/​Service/​JMAP/​JmapOperationsServiceTest.php Tests JMAP save and upload operations.
tests/​Unit/​Service/​DraftsServiceTest.php Tests connector-based draft storage.
tests/​Unit/​Service/​Attachment/​AttachmentServiceTest.php Updates protocol-neutral attachment tests.
tests/​Unit/​Service/​AntiSpamServiceTest.php Removes obsolete transmission mock.
tests/​Unit/​Send/​SentMailboxHandlerTest.php Removes obsolete handler tests.
tests/​Unit/​Send/​SendHandlerTest.php Tests connector-based sending.
tests/​Unit/​Send/​FlagRepliedMessageHandlerTest.php Updates reply-flagging tests.
tests/​Unit/​Send/​CopySendMessageHandlerTest.php Removes obsolete copy-handler tests.
tests/​Unit/​Send/​ChainTest.php Updates the simplified send chain.
tests/​Unit/​Listener/​DeleteDraftListenerTest.php Tests connector-based draft deletion.
tests/​Unit/​JMAP/​JmapTransmissionConnectorTest.php Tests JMAP attachment saving.
tests/​Unit/​IMAP/​ImapTransmissionConnectorTest.php Tests extracted IMAP transmission behavior.
tests/​Unit/​Controller/​MessagesControllerTest.php Updates transmission dependency.
tests/​Unit/​Controller/​AccountsControllerTest.php Removes legacy draft endpoint tests.
tests/​Integration/​Service/​OutboxServiceIntegrationTest.php Updates outbox construction.
tests/​Integration/​Service/​MailTransmissionIntegrationTest.php Uses the registered send chain.
tests/​Integration/​Service/​DraftServiceIntegrationTest.php Updates draft service construction.
lib/​Service/​TransmissionService.php Exposes protocol-neutral attachment content.
lib/​Service/​OutboxService.php Removes direct IMAP attachment handling.
lib/​Service/​MailTransmission.php Removes legacy transmission service.
lib/​Service/​MailManager.php Adds optional raw-message decryption.
lib/​Service/​JMAP/​JmapOperationsService.php Adds JMAP submission, saving, and uploads.
lib/​Service/​DraftsService.php Saves drafts through transmission connectors.
lib/​Service/​Attachment/​AttachmentService.php Uses protocol-neutral message access.
lib/​Send/​SentMailboxHandler.php Removes obsolete mailbox handler.
lib/​Send/​SendHandler.php Dispatches sending through connectors.
lib/​Send/​FlagRepliedMessageHandler.php Uses protocol-neutral flagging.
lib/​Send/​CopySentMessageHandler.php Removes obsolete sent-copy handler.
lib/​Send/​Chain.php Simplifies the sending chain.
lib/​Send/​AntiAbuseHandler.php Removes IMAP-client coupling.
lib/​Send/​AHandler.php Removes IMAP-client parameters.
lib/​Protocol/​ProtocolFactory.php Registers transmission connectors.
lib/​Listener/​DeleteDraftListener.php Deletes drafts through message connectors.
lib/​JMAP/​JmapTransmissionConnector.php Implements JMAP draft and send operations.
lib/​JMAP/​JmapMessageConnector.php Accepts the raw-message decrypt option.
lib/​IMAP/​ImapTransmissionConnector.php Implements extracted IMAP transmission.
lib/​IMAP/​ImapMessageConnector.php Supports optional raw-message decryption.
lib/​Db/​MailboxMapper.php Adds account-scoped name lookup.
lib/​Controller/​MessagesController.php Routes MDNs through connectors.
lib/​Controller/​AccountsController.php Removes the legacy draft action.
lib/​Contracts/​IMessageConnector.php Extends raw-fetch contract.
lib/​Contracts/​IMailTransmission.php Removes the legacy transmission contract.
lib/​AppInfo/​Application.php Removes legacy service registration.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +90 to +92
$rcptTo = $this->collectEnvelopeRecipients($to, $cc, $bcc);
$attachments = $this->collectAttachments($account, $message);
$jMessage = $this->convertLocalMessage($from, $to, $cc, $bcc, $message);
Comment thread lib/JMAP/JmapTransmissionConnector.php Outdated
Comment on lines +301 to +302
// Network / HTTP errors and other transient failures are retriable
return LocalMessage::STATUS_SMPT_SEND_FAIL;
Comment thread lib/Send/FlagRepliedMessageHandler.php Outdated
$message->setFlagAnswered(true);
$this->dbMessageMapper->update($message);
} catch (DoesNotExistException|Horde_Imap_Client_Exception $e) {
} catch (DoesNotExistException $e) {
Comment on lines +1178 to +1182
$part = $message->bodyPartStructure()->addPart();
$part->blob($blobId)
->type($attachment['type'])
->name($attachment['name'])
->disposition($attachment['disposition'] ?? 'attachment');
Comment on lines +149 to +151
if ($message->getSubject() !== null) {
$fccHeaders->addHeader('Subject', $message->getSubject());
}
Comment on lines +174 to +179
if (($inReplyTo = $lMessage->getInReplyToMessageId()) !== null) {
$jMessage->inReplyTo($inReplyTo);
$jMessage->references($inReplyTo);
}

$jMessage->subject($lMessage->getSubject() ?? '');
Comment thread lib/Listener/DeleteDraftListener.php Outdated
Comment on lines 62 to 66
// TODO: the UID could be from another mailbox
$this->protocolFactory->messageConnector($account)->deleteMessages($account, $draftsMailbox, $draft);

$this->eventDispatcher->dispatchTyped(
new MessageDeletedEvent($account, $draftsMailbox, $draft->getUid())
Comment on lines 144 to 148
public function sendMessage(LocalMessage $message, Account $account): void {
$draftsMailbox = $this->findOrCreateDraftsMailbox($account);
try {
$this->transmission->saveLocalDraft($account, $message);
$this->protocolFactory->transmissionConnector($account)->saveMessage($account, $draftsMailbox, $message, ['$draft']);
} catch (ClientException|ServiceException $e) {
@kesselb

kesselb commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Note

This review was generated with Claude Code (claude-opus-5-5) and checked against the code at ee4156a. It includes the open Copilot findings. Line numbers refer to that commit.

Overview

Duplicate send / endless retry

  • 1. FlagRepliedMessageHandler only catches DoesNotExistException, so the reply is sent again
  • 2. FlagRepliedMessageHandler passes \answered instead of answered, which breaks every JMAP reply
  • 3. SendHandler sets STATUS_NO_SENT_MAILBOX on an already sent message
  • 4. JMAP: every unknown send error is retriable
  • 5. DraftsService::sendMessage: errors resolving the drafts mailbox skip setFailed()

Lost behaviour

Cleanup

  • 12. Dead accounts#draft route, saveDraft() JS helper and SaveDraftEvent
  • 13. MailboxMapper::findByName is unused and duplicates find()
  • 14. Unused MailboxMapper in ImapTransmissionConnector
  • 15. AttachmentService::handleAttachments opens one IMAP connection per forwarded attachment
  • 16. Blanket \Throwable catches and narrating comments

Details

1. Reply flagging failure resends the mail (lib/Send/FlagRepliedMessageHandler.php:68)
MailManager::flagMessages throws ServiceException (IMAP wraps Horde errors, JmapMessageConnector.php:277). The exception escapes Chain::process before update()/deleteWithRecipients(), so the outbox row keeps its old status and is sent again. On main, Horde_Imap_Client_Exception was caught here.
Trigger: see 2 (JMAP), or IMAP failing between SMTP and flagging.

2. Wrong flag name (lib/Send/FlagRepliedMessageHandler.php:62)
Horde_Imap_Client::FLAG_ANSWERED is \answered, while flagMessages expects answered. JMAP: normalizeFlagForRemote passes \answered through as a keyword, the server rejects it (RFC 8621 keywords must not contain \), and 1 kicks in. IMAP: it skips the system-flag map and needs an extra STATUS PERMFLAGS, and Message::setFlag ignores the value.
Trigger: JMAP account, reply to a message in the Inbox.

3. Resend after a failed Sent copy (lib/Send/SendHandler.php:39-50)
A message with STATUS_IMAP_SENT_MAILBOX_FAIL (already sent over SMTP) is set to STATUS_NO_SENT_MAILBOX when the Sent mailbox is unset or gone. Once a Sent mailbox is configured again, it goes through SMTP a second time. The removed CopySentMessageHandler kept status 11 on purpose for this case.
Trigger: make the copy to Sent fail (e.g. quota/ACL on Sent), then unset the Sent mailbox in the account settings, then set it again.

4. JMAP error classification (lib/JMAP/JmapTransmissionConnector.php:293-303)
Everything except three strings maps to STATUS_SMPT_SEND_FAIL, and the outbox retries every status except STATUS_ERROR. Permanent rejections (invalidEmail, forbiddenToSend, …) are retried forever. Each retry leaves another staged copy in Drafts, because completionUpdate only runs on success. A lost response after an accepted submission sends the mail twice.
Trigger: JMAP account, send to a recipient the server rejects.

5. Drafts mailbox errors outside the try (lib/Service/DraftsService.php:145)
findOrCreateDraftsMailbox() throws ServiceException (JMAP without drafts mailbox) or DoesNotExistException (stale id, missing from @throws) before the try block, so setFailed(true) never runs. flush() retries and logs a warning on every cron run.
Trigger: JMAP account with no drafts mailbox configured, write a draft and close the composer.

6. JMAP ignores security options (lib/JMAP/JmapTransmissionConnector.php:42, :158)
smimeSign, smimeEncrypt, isPgpMime and requestMdn are not looked at. The message goes out unsigned/unencrypted with STATUS_PROCESSED. Either implement them or fail with a terminal status.
Trigger: JMAP account with an S/MIME certificate, send with "Encrypt with S/MIME".

7. X-AI-Generated missing (lib/IMAP/ImapTransmissionConnector.php:138, :261, lib/JMAP/JmapTransmissionConnector.php:158)
main added LocalMessage::HEADER_AI_GENERATED in sendMessage and saveLocalDraft. ImapMessageFetcher/JmapMessageAdapter read it to restore isAiGenerated.
Trigger: compose with an AI-generated text and check the source of the sent message.

8. Bcc dropped from drafts (lib/IMAP/ImapTransmissionConnector.php:270)
Horde_Mime_Mail::addHeader('Bcc') stores the value in $_bcc, and getRaw() does not write it. main built Horde_Mime_Headers with Bcc.
Trigger: IMAP account, new message with Bcc, close the composer, reopen the draft from the Drafts folder.

9. iMIP via JMAP (lib/Service/JMAP/JmapOperationsService.php:1182)
$attachment['disposition'] ?? 'attachment' turns DISPOSITION_OMIT (null) into attachment. The IMAP path keeps omitting it (ImapTransmissionConnector.php:414). name is '' here (cast in MessageSend.php:113), so it should not be set either.
Trigger: Calendar invitation sent through the mail provider from a JMAP account.

10. MessageSentEvent timing (lib/Send/SendHandler.php:54-57)
The event is only dispatched when the Sent copy succeeds too. With status 11, AddressCollectionListener/InteractionListener miss the send, and only get it late on a successful retry. main dispatched it right after SMTP.
Trigger: make the copy to Sent fail.

11. DeleteDraftListener (lib/Listener/DeleteDraftListener.php:63-67)
ImapMessageConnector::deleteMessages only catches Horde exceptions, so ServiceException from expunge escapes and fails the request that saved the draft or outbox message. deleteMessages() returns the deleted messages; dispatching MessageDeletedEvent only for those would stop the cache from dropping drafts that still exist remotely.
Trigger: IMAP expunge failure while saving over an existing draft.

12. Dead code
appinfo/routes.php:79 still routes to the removed AccountsController::draft, which gives a 500. saveDraft() in src/service/MessageService.js:245 has no callers. lib/Events/SaveDraftEvent.php (+ test) is no longer dispatched.

13. lib/Db/MailboxMapper.php:203 findByName has no callers. find(Account, string) already does the same via name_hash.

14. lib/IMAP/ImapTransmissionConnector.php:70 $mailboxMapper is injected but never used.

15. lib/Service/Attachment/AttachmentService.php:217 On main, one Horde_Imap_Client_Socket was passed through. Now each forwarded message/attachment goes through MailManager and connects separately.

16. lib/IMAP/ImapTransmissionConnector.php:88, :244 catch \Throwable around messageMapper->save(). Several new comments only narrate the next line (e.g. SendHandler.php:38, JmapTransmissionConnector.php:268, :301, JmapOperationsService.php "construct/transceive/extract").

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
@SebastianKrupinski

Copy link
Copy Markdown
Contributor Author

Hi @kesselb

I have addressed the issues you posted. 2 are left out for now, 6 will be addressed in a follow up, and 15 is already addressed i the followup in the stack

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: 📄 To do

Development

Successfully merging this pull request may close these issues.

JMAP Write Operations Read-only JMAP Support

4 participants