when moving a message out of a Rejects mailbox, mark the message as "not seen" so stands out in the destination mailbox (e.g. inbox)

we set the flag both for move in imap and in webmail.

this also ensures the "MailboxDestinedID", used for per-mailbox reputation
analysis, is set in more reject-situations. before this change, some rejects
(such as based on DMARC reject) wouldn't result in reputation being used after
having been moved the message out of the rejects mailbox.

in the future, we need more tests for scenario's like this...

for issue #63 reported by x8x
may also help with issue #64

imapserver/server.go

@@ -3378,9 +3378,7 @@ 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)
 				}
 
-				mc := m.MailboxCounts()
+				mbSrc.Sub(m.MailboxCounts())
-				mbSrc.Sub(mc)
-				mbDst.Add(mc)
 
 				// Copy of message record that we'll insert when UID is freed up.
 				om := *m
@@ -3394,7 +3392,9 @@ func (c *conn) cmdxMove(isUID bool, tag, cmd string, p *parser) {
 					// is used for reputation calculation during future deliveries.
 					m.MailboxOrigID = m.MailboxDestinedID
 					m.IsReject = false
+					m.Seen = false
 				}
+				mbDst.Add(m.MailboxCounts())
 				m.UID = uidnext
 				m.ModSeq = modseq
 				m.JunkFlagsForMailbox(mbDst, conf)

smtpserver/analyze.go

@@ -116,7 +116,49 @@ func analyze(ctx context.Context, log *mlog.Log, resolver dns.Resolver, d delive
 		log.Info("forwarded message, clearing identifying signals of forwarding mail server")
 	}
 
+	assignMailbox := 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 determine a reputation because we also base it on outgoing
+		// messages and those are account-global.
+		mb, err := d.acc.MailboxFind(tx, mailbox)
+		if err != nil {
+			return fmt.Errorf("finding destination mailbox: %w", err)
+		}
+		if mb != nil {
+			// We want to deliver to mb.ID, but this message may be rejected and sent to the
+			// Rejects mailbox instead, with 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))
+		}
+		return nil
+	}
+
 	reject := func(code int, secode string, errmsg string, err error, reason string) analysis {
+		// We may have set MailboxDestinedID below already while we had a transaction. If
+		// not, do it now. This makes it possible to use the per-mailbox reputation when a
+		// user moves the message out of the Rejects mailbox to the intended mailbox
+		// (typically Inbox).
+		if d.m.MailboxDestinedID == 0 {
+			var mberr error
+			d.acc.WithRLock(func() {
+				mberr = d.acc.DB.Read(ctx, func(tx *bstore.Tx) error {
+					return assignMailbox(tx)
+				})
+			})
+			if mberr != nil {
+				return analysis{false, mailbox, smtp.C451LocalErr, smtp.SeSys3Other0, false, "error processing", err, nil, nil, reasonReputationError}
+			}
+			d.m.MailboxID = 0 // We plan to reject, no need to set intended MailboxID.
+		}
+
 		accept := false
 		if rs != nil && rs.AcceptRejectsToMailbox != "" {
 			accept = true
@@ -203,26 +245,8 @@ func analyze(ctx context.Context, log *mlog.Log, resolver dns.Resolver, d delive
 	var err error
 	d.acc.WithRLock(func() {
 		err = d.acc.DB.Read(ctx, func(tx *bstore.Tx) error {
-			// Set message MailboxID to which mail will be delivered. Reputation is
+			if err := assignMailbox(tx); err != nil {
-			// per-mailbox. If referenced mailbox is not found (e.g. does not yet exist), we
+				return err
-			// can still determine a reputation because we also base it on outgoing
-			// messages and those are account-global.
-			mb, err := d.acc.MailboxFind(tx, mailbox)
-			if err != nil {
-				return fmt.Errorf("finding destination mailbox: %w", err)
-			}
-			if mb != nil {
-				// We want to deliver to mb.ID, but this message may be rejected and sent to the
-				// Rejects mailbox instead, with 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))
 			}
 
 			isjunk, conclusive, method, err = reputation(tx, log, d.m)

webmail/api.go

@@ -844,6 +844,7 @@ func (Webmail) MessageMove(ctx context.Context, messageIDs []int64, mailboxID in
 					// is used for reputation calculation during future deliveries.
 					m.MailboxOrigID = m.MailboxDestinedID
 					m.IsReject = false
+					m.Seen = false
 				}
 				m.UID = mbDst.UIDNext
 				m.ModSeq = modseq