Queue messages for SMTP before encryption - #8619
Conversation
026e72d to
ce56911
Compare
d688b11 to
6809578
Compare
e657a72 to
a67a5b0
Compare
756d28c to
3d72fe8
Compare
b838513 to
bdfd0d4
Compare
958c526 to
fe41960
Compare
8a42d9b to
b9bba12
Compare
3e8fdaa to
eb1c34c
Compare
eb1c34c to
7eb324f
Compare
| ) -> Result<( | ||
| Option<(QueuedMail, Option<QueueSideEffects>)>, | ||
| (QueuedMail, Option<QueueSideEffects>), | ||
| )> { |
There was a problem hiding this comment.
nit: maybe (QueuedMail, Option<QueueSideEffects>) is worth an alias?
hpk42
left a comment
There was a problem hiding this comment.
Overall i think the PR is larger than it needs to be.
Conceptually there is one main issue IMO: For BCC-self-copies this PR re-resolves current relay list, but not for the recipient addresses. In bad network/partially-offline situations messages can be in the queue for long enough that a keyupdate arrived. Logically, recipient fingerprints should be derived to addresses at sending time, and not fixated at queuing time. At send-time we anyway read the public keys, so it wouldn't cause more IO. Messages then either have fingerprints as recipients, or addresses but never both at the same time. After sending to addresses in a chunk succeeded, they could be written to a new sent_finished column, so that the next address-derivation can skip them.
| // eg. are just freetext and/or do not follow any standard. | ||
| headers.push(( | ||
| "Chat-Disposition-Notification-To", | ||
| mail_builder::headers::raw::Raw::new(self.from_addr.clone()).into(), |
There was a problem hiding this comment.
This would need to be re-rendered on transport change as well but i instead suggest to relax the check in https://github.com/chatmail/core/blob/link2xt/late-encryption/src/mimeparser.rs#L955 and treat the header existence as a boolean, or compare to all relay addresses.
There also is another use of self.from_addr for the Sender header but i think it's not critical on the receiver side. Ideally there would be no self.from_addr left at all.
There was a problem hiding this comment.
Opened #8681 now.
For Chat-Disposition-Notification-To and Sender, i have edited them in the first message #8572 while doing #8645 which is also split out of this PR in attempt to get rid of from_addr.
Sender is essentially a boolean flag saying "this is a bridge bot, show display name with ~ and save it in the message".
| migration_version, | ||
| ) | ||
| .await?; | ||
| } |
There was a problem hiding this comment.
Doesn't this orphan currently queued messages in the existing smtp table? At least they should be marked failed i think.
| }; | ||
| let mut recipients = queued_mail.recipients.clone(); | ||
| if queued_mail.bcc_self { | ||
| add_self_recipients( |
There was a problem hiding this comment.
this adds the self-recipients on every retry without dedup. After a partially sent chunked message, the remainder written back to the db further down already contains them.
| -- Replaced with smtp2. | ||
| CREATE TABLE smtp ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| rfc724_mid TEXT NOT NULL, -- Message-ID | ||
| mime TEXT NOT NULL, -- SMTP payload | ||
| msg_id INTEGER NOT NULL, -- ID of the message in `msgs` table | ||
| recipients TEXT NOT NULL, -- List of recipients separated by space | ||
| retries INTEGER NOT NULL DEFAULT 0 -- Number of failed attempts to send the message | ||
| ); |
There was a problem hiding this comment.
why move this table and increase the diff?
There was a problem hiding this comment.
All unused tables are at the end currently, below the -- Deprecated and unused tables. comment.
| @@ -681,33 +716,70 @@ ORDER BY id" | |||
| } | |||
|
|
|||
| pub async fn get_smtp_rows_for_msg<'a>(&'a self, msg_id: MsgId) -> Vec<SentMessage<'a>> { | |||
There was a problem hiding this comment.
please use a helper instead of the duplications in pop_sent_msg_ext, here and first_row_in_smtp_queue.
| } | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub(crate) enum QueuedEncryption { |
There was a problem hiding this comment.
I don't think this new enum is much "better" as you say in the PR description. It's only about dropping addresses but e.g. keyupdates just used empty strings on main and render_queued_mail dropped the address unconditionally. So overall, it's more diff and more LOCs without much gain IMO.
| let trans_fn = |t: &mut rusqlite::Transaction| { | ||
| let mut row_ids = Vec::<i64>::new(); |
There was a problem hiding this comment.
Why did the transaction have to go? Removing it here seems not neccessary, and produces follow-up Todos.
| let msg_id = message::insert_tombstone(context, &rfc724_mid).await?; | ||
| let keys = chunk.iter().map(|r| r.public_key.clone()).collect(); | ||
| let rendered_message = render_keyupdate_message(context, &rfc724_mid, keys).await?; | ||
| insert_into_smtp(context, &rfc724_mid, &envelope, rendered_message).await?; |
There was a problem hiding this comment.
why not let insert_into_smtp take a QueuedMail instead of the rendered string? Removing the helper and inlining it three times in different ways seems unnecessary.
It was incorrectly converting unused row ID to MsgId type and selecting already known msg_id.
7eb324f to
84dd1f4
Compare
This is similar to mimefactory::Encryption, but does not have email addresses for asymmetrically encrypted messages. Queued messages don't need email addresses for public keys. Addresses are only needed to render Autocrypt-Gossip headers.
Headers like From and Autocrypt are now added late, right before sending the message over SMTP. This way we advertise the latest list of transports and use the correct From address in the encrypted part even for messages queued while being offline. BCC-self recipients are also added late. For unencrypted messages we only want to send a copy to the sending address, but we don't know the sending address when queueing the message. Adding bcc-self recipients when dequeuing the message also makes it possible to send copies to updated list of relays.
84dd1f4 to
8fded29
Compare
Part of #8607, this removes the code that deletes the whole queue when the sending relay is changed, and makes queueing the mail independent of the sending address. Follow-up to #8345 in some way, this now uses
QueuedMailfor real.There is a python test that was previously failing, deleting the current transport while having messages queued.
Large number of lines added is because of better types (new
QueuedEncryptionseparate frommimefactory::Encryption) and duplicatedsmtp2table with documentation.Left for follow-up PRs:
chat::enqueue_mail. This needs refactoring location streaming code etc. that is unrelated.QueuedMail,QueuedEncryptionand related functions from mimefactory to a separate module.ConfiguredAddr.Note re performance of encryption: maybe it even makes UI more responsive because the message bubble can appear on the sending side faster as the message does not need to be encrypted yet.
Commit message
Headers like From and Autocrypt are now added late, right before sending the message over SMTP. This way we advertise the latest list of transports and use the correct From address in the encrypted part even for messages queued while being offline.BCC-self recipients are also added late.
For unencrypted messages we only want to send a copy
to the sending address, but we don't know the sending address
when queueing the message.
Adding bcc-self recipients when dequeuing the message
also makes it possible to send copies to updated list of relays.