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

@@ -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++

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))
 			}

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")