From 73bfc58453e8b4b33df0c69ea370b188d54daa90 Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien Date: Fri, 3 Mar 2023 13:19:27 +0100 Subject: [PATCH] fix handling of reputation for messages that were moved out of the rejects mailbox the idea of the rejects mailbox is to show messages that were rejected. you can look there, and if you see a message that should have been delivered, you can move it to your inbox or archive. next time a deliver attempt by that user is attempted, they should be accepted, because you corrected the reject. but that wasn't happening, because the reputation-calculation is per-delivery mailbox (e.g. Inbox) and we look at MailboxOrigID when calculating the reputation. and that was set to the Rejects mailbox id, so the message wasn't considered. the same applies to moving messages from Rejects to Junk (to train your filter). we now keep track of a MailboxDestinedID, that is set to the mailbox that we would have delivered to if we would not have rejected the message. then, when a message is moved out of the Rejects mailbox, we change MailboxOrigID to MailboxDestinedID. this essentially makes the message look like it was delivered normally. --- imapserver/server.go | 13 +++++++++++-- smtpserver/analyze.go | 12 ++++++++++-- store/account.go | 27 ++++++++++++++++++++++----- 3 files changed, 43 insertions(+), 9 deletions(-) diff --git a/imapserver/server.go b/imapserver/server.go index f1ff40f..16264e9 100644 --- a/imapserver/server.go +++ b/imapserver/server.go @@ -2967,7 +2967,11 @@ func (c *conn) cmdxCopy(isUID bool, tag, cmd string, p *parser) { m.ID = 0 m.UID = uidFirst + store.UID(i) m.MailboxID = mbDst.ID - m.MailboxOrigID = mbSrc.ID + if mbSrc.Name == conf.RejectsMailbox && m.MailboxDestinedID != 0 { + // Incorrectly delivered to Rejects mailbox. Adjust MailboxOrigID so this message + // is used for reputation calculation during future deliveries. + m.MailboxOrigID = m.MailboxDestinedID + } m.TrainedJunk = nil m.JunkFlagsForMailbox(mbDst.Name, conf) err := tx.Insert(&m) @@ -3092,7 +3096,7 @@ func (c *conn) cmdxMove(isUID bool, tag, cmd string, p *parser) { c.account.WithWLock(func() { c.xdbwrite(func(tx *bstore.Tx) { - c.xmailboxID(tx, c.mailboxID) // Validate. + mbSrc := c.xmailboxID(tx, c.mailboxID) // Validate. mbDst = c.xmailbox(tx, name, "TRYCREATE") if mbDst.ID == c.mailboxID { xuserErrorf("cannot move to currently selected mailbox") @@ -3128,6 +3132,11 @@ func (c *conn) cmdxMove(isUID bool, tag, cmd string, p *parser) { xserverErrorf("internal error: got uid %d, expected %d, for index %d", m.UID, uids[i], i) } m.MailboxID = mbDst.ID + if mbSrc.Name == conf.RejectsMailbox && m.MailboxDestinedID != 0 { + // Incorrectly delivered to Rejects mailbox. Adjust MailboxOrigID so this message + // is used for reputation calculation during future deliveries. + m.MailboxOrigID = m.MailboxDestinedID + } m.UID = uidnext m.JunkFlagsForMailbox(mbDst.Name, conf) uidnext++ diff --git a/smtpserver/analyze.go b/smtpserver/analyze.go index 61da216..db68ede 100644 --- a/smtpserver/analyze.go +++ b/smtpserver/analyze.go @@ -163,8 +163,8 @@ func analyze(ctx context.Context, log *mlog.Log, resolver dns.Resolver, d delive err = d.acc.DB.Read(func(tx *bstore.Tx) error { // Set message MailboxID to which mail will be delivered. Reputation is // per-mailbox. If referenced mailbox is not found (e.g. does not yet exist), we - // can still use determine a reputation because we also base it on outgoing - // messages and those account-global. + // can still determine a reputation because we also base it on outgoing + // messages and those are account-global. mailbox := d.rcptAcc.destination.Mailbox if mailbox == "" { mailbox = "Inbox" @@ -174,7 +174,15 @@ func analyze(ctx context.Context, log *mlog.Log, resolver dns.Resolver, d delive } mb := d.acc.MailboxFindX(tx, mailbox) if mb != nil { + // We want to deliver to mb.ID, but this message may be rejected and sent to the + // Rejects mailbox instead, which MailboxID overwritten. Record the ID in + // MailboxDestinedID too. If the message is later moved out of the Rejects mailbox, + // we'll adjust the MailboxOrigID so it gets taken into account during reputation + // calculating in future deliveries. If we end up delivering to the intended + // mailbox (i.e. not rejecting), MailboxDestinedID is cleared during delivery so we + // don't store it unnecessarily. d.m.MailboxID = mb.ID + d.m.MailboxDestinedID = mb.ID } else { log.Debug("mailbox not found in database", mlog.Field("mailbox", mailbox)) } diff --git a/store/account.go b/store/account.go index 77eb036..5959109 100644 --- a/store/account.go +++ b/store/account.go @@ -229,11 +229,23 @@ type Message struct { UID UID `bstore:"nonzero"` // UID, for IMAP. Set during deliver. MailboxID int64 `bstore:"nonzero,unique MailboxID+UID,index MailboxID+Received,ref Mailbox"` - // Mailbox message originally delivered to. I.e. not changed when moved to Trash or - // Junk. Useful for per-mailbox reputation. Not a bstore reference to prevent - // having to update all messages in a mailbox when the original mailbox is removed. - // Use of this field requires checking if the mailbox still exists. - MailboxOrigID int64 + // MailboxOrigID is the mailbox the message was originally delivered to. Typically + // Inbox or Rejects, but can also be Postmaster and TLS/DMARC reporting addresses. + // MailboxOrigID is not changed when the message is moved to another mailbox, e.g. + // Archive/Trash/Junk. Used for per-mailbox reputation. + // + // MailboxDestinedID is normally 0, but when a message is delivered to the Rejects + // mailbox, it is set to the intended mailbox according to delivery rules, + // typically that of Inbox. When such a message is moved out of Rejects, the + // MailboxOrigID is corrected by setting it to MailboxDestinedID. This ensures the + // message is used for reputation calculation for future deliveries to that + // mailbox. + // + // These are not bstore references to prevent having to update all messages in a + // mailbox when the original mailbox is removed. Use of these fields requires + // checking if the mailbox still exists. + MailboxOrigID int64 + MailboxDestinedID int64 Received time.Time `bstore:"default now,index"` @@ -600,6 +612,11 @@ func (a *Account) DeliverX(log *mlog.Log, tx *bstore.Tx, m *Message, msgFile *os m.ParsedBuf = buf } + // If we are delivering to the originally intended mailbox, no need to store the mailbox ID again. + if m.MailboxDestinedID != 0 && m.MailboxDestinedID == m.MailboxOrigID { + m.MailboxDestinedID = 0 + } + err = tx.Insert(m) xcheckf(err, "inserting message")