Fix move to local folder and back - remove deleted flag

internal/imap/mailbox_message.go

@@ -166,6 +166,13 @@ func (im *imapMailbox) CreateMessage(flags []string, date time.Time, body imap.L
 		if err == nil && (im.user.user.IsCombinedAddressMode() || (im.storeAddress.AddressID() == msg.Message().AddressID)) {
 			IDs := []string{internalID}
 
+			// See the comment bellow.
+			if msg.IsMarkedDeleted() {
+				if err := im.storeMailbox.MarkMessagesUndeleted(IDs); err != nil {
+					log.WithError(err).Error("Failed to undelete re-imported internal message")
+				}
+			}
+
 			err = im.storeMailbox.LabelMessages(IDs)
 			if err != nil {
 				return err
@@ -182,6 +189,20 @@ func (im *imapMailbox) CreateMessage(flags []string, date time.Time, body imap.L
 		return err
 	}
 
+	// IMAP clients can move message to local folder (setting \Deleted flag)
+	// and then move it back (IMAP client does not remember the message,
+	// so instead removing the flag it imports duplicate message).
+	// Regular IMAP server would keep the message twice and later EXPUNGE would
+	// not delete the message (EXPUNGE would delete the original message and
+	// the new duplicate one would stay). API detects duplicates; therefore
+	// we need to remove \Deleted flag if IMAP client re-imports.
+	msg, err := im.storeMailbox.GetMessage(m.ID)
+	if err == nil && msg.IsMarkedDeleted() {
+		if err := im.storeMailbox.MarkMessagesUndeleted([]string{m.ID}); err != nil {
+			log.WithError(err).Error("Failed to undelete re-imported message")
+		}
+	}
+
 	targetSeq := im.storeMailbox.GetUIDList([]string{m.ID})
 	return uidplus.AppendResponse(im.storeMailbox.UIDValidity(), targetSeq)
 }

pkg/message/parser.go

@@ -484,6 +484,9 @@ func parseMessageHeader(m *pmapi.Message, h message.Header) error { // nolint[fu
 				return errors.Wrap(err, "failed to parse date")
 			}
 			m.Time = date.Unix()
+
+		case "message-id":
+			m.ExternalID = fields.Value()
 		}
 	}
 

test/fakeapi/messages.go

@@ -216,24 +216,47 @@ func (api *FakePMAPI) generateMessageFromImportRequest(msgReq *pmapi.ImportMsgRe
 		return nil, err
 	}
 
-	messageID := api.controller.messageIDGenerator.next("")
+	existingMsg := api.findMessage(m)
+	if existingMsg != nil {
+		return existingMsg, nil
+	}
 
+	messageID := api.controller.messageIDGenerator.next("")
 	return &pmapi.Message{
-		ID:        messageID,
+		ID:         messageID,
-		AddressID: msgReq.AddressID,
+		ExternalID: m.ExternalID,
-		Sender:    m.Sender,
+		AddressID:  msgReq.AddressID,
-		ToList:    m.ToList,
+		Sender:     m.Sender,
-		Subject:   m.Subject,
+		ToList:     m.ToList,
-		Unread:    msgReq.Unread,
+		Subject:    m.Subject,
-		LabelIDs:  append(msgReq.LabelIDs, pmapi.AllMailLabel),
+		Unread:     msgReq.Unread,
-		Body:      m.Body,
+		LabelIDs:   append(msgReq.LabelIDs, pmapi.AllMailLabel),
-		Header:    m.Header,
+		Body:       m.Body,
-		Flags:     msgReq.Flags,
+		Header:     m.Header,
-		Time:      msgReq.Time,
+		Flags:      msgReq.Flags,
+		Time:       msgReq.Time,
 	}, nil
 }
 
+func (api *FakePMAPI) findMessage(newMsg *pmapi.Message) *pmapi.Message {
+	if newMsg.ExternalID == "" {
+		return nil
+	}
+	for _, msg := range api.messages {
+		// API surely has better algorithm, but this one is enough for us for now.
+		if !msg.IsDraft() &&
+			msg.Subject == newMsg.Subject &&
+			msg.ExternalID == newMsg.ExternalID {
+			return msg
+		}
+	}
+	return nil
+}
+
 func (api *FakePMAPI) addMessage(message *pmapi.Message) {
+	if api.findMessage(message) != nil {
+		return
+	}
 	api.messages = append(api.messages, message)
 	api.addEventMessage(pmapi.EventCreate, message)
 }

test/features/bridge/imap/message/move_local_folder.feature

@@ -0,0 +1,76 @@
+# IMAP clients can move message to local folder (setting \Deleted flag)
+# and then move it back (IMAP client does not remember the message,
+# so instead removing the flag it imports duplicate message).
+# Regular IMAP server would keep the message twice and later EXPUNGE would
+# not delete the message (EXPUNGE would delete the original message and
+# the new duplicate one would stay). Both Bridge and API detects duplicates;
+# therefore we need to remove \Deleted flag if IMAP client re-imports.
+Feature: IMAP move message out to and back from local folder
+  Background:
+    Given there is connected user "user"
+    Given there is IMAP client logged in as "user"
+    And there is IMAP client selected in "INBOX"
+
+  Scenario: Mark message as deleted and re-append again
+    When IMAP client imports message to "INBOX"
+      """
+      From: <john.doe@mail.com>
+      To: <user@pm.me>
+      Subject: foo
+      Date: Mon, 02 Jan 2006 15:04:05 +0000
+      Message-Id: <msgID>
+
+      hello
+      """
+    Then IMAP response is "OK"
+    When IMAP client marks message seq "1" as deleted
+    Then IMAP response is "OK"
+    When IMAP client imports message to "INBOX"
+      """
+      From: <john.doe@mail.com>
+      To: <user@pm.me>
+      Subject: foo
+      Date: Mon, 02 Jan 2006 15:04:05 +0000
+      Message-Id: <msgID>
+
+      hello
+      """
+    Then IMAP response is "OK"
+    And mailbox "INBOX" for "user" has 1 message
+    And mailbox "INBOX" for "user" has messages
+      | from              | to         | subject | deleted |
+      | john.doe@mail.com | user@pm.me | foo     | false   |
+
+  # We cannot control ID generation on API.
+  @ignore-live
+  Scenario: Mark internal message as deleted and re-append again
+    # Each message has different subject so if the ID generations on fake API
+    # changes, test will fail because not even external ID mechanism will work.
+    When IMAP client imports message to "INBOX"
+      """
+      From: <john.doe@mail.com>
+      To: <user@pm.me>
+      Subject: foo
+      Date: Mon, 02 Jan 2006 15:04:05 +0000
+
+      hello
+      """
+    Then IMAP response is "OK"
+    When IMAP client marks message seq "1" as deleted
+    Then IMAP response is "OK"
+    # Fake API generates for the first message simple ID 1.
+    When IMAP client imports message to "INBOX"
+      """
+      From: <john.doe@mail.com>
+      To: <user@pm.me>
+      Subject: bar
+      Date: Mon, 02 Jan 2006 15:04:05 +0000
+      X-Pm-Internal-Id: 1
+
+      hello
+      """
+    Then IMAP response is "OK"
+    And mailbox "INBOX" for "user" has 1 message
+    And mailbox "INBOX" for "user" has messages
+      | from              | to         | subject | deleted |
+      | john.doe@mail.com | user@pm.me | foo     | false   |

test/mocks/imap_client.go

@@ -21,8 +21,10 @@ import (
 	"bufio"
 	"fmt"
 	"net"
+	"strings"
 	"sync"
 
+	"github.com/ProtonMail/proton-bridge/pkg/message"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -163,7 +165,7 @@ func (c *IMAPClient) Search(query string) *IMAPResponse {
 // Message
 
 func (c *IMAPClient) Append(mailboxName, msg string) *IMAPResponse {
-	cmd := fmt.Sprintf("APPEND \"%s\" (\\Seen) \"25-Mar-2021 00:30:00 +0100\" {%d}\r\n%s", mailboxName, len(msg), msg)
+	cmd := fmt.Sprintf("APPEND \"%s\" (\\Seen) \"%s\" {%d}\r\n%s", mailboxName, parseAppendDate(msg), len(msg), msg)
 	return c.SendCommand(cmd)
 }
 
@@ -175,10 +177,20 @@ func (c *IMAPClient) AppendBody(mailboxName, subject, from, to, body string) *IM
 	msg += body
 	msg += "\r\n"
 
-	cmd := fmt.Sprintf("APPEND \"%s\" (\\Seen) \"25-Mar-2021 00:30:00 +0100\" {%d}\r\n%s", mailboxName, len(msg), msg)
+	cmd := fmt.Sprintf("APPEND \"%s\" (\\Seen) \"%s\" {%d}\r\n%s", mailboxName, parseAppendDate(msg), len(msg), msg)
 	return c.SendCommand(cmd)
 }
 
+func parseAppendDate(msg string) string {
+	date := "25-Mar-2021 00:30:00 +0100"
+	if m, _, _, _, err := message.Parse(strings.NewReader(msg)); err == nil {
+		if t, err := m.Header.Date(); err == nil {
+			date = t.Format("02-Jan-2006 15:04:05 -0700")
+		}
+	}
+	return date
+}
+
 func (c *IMAPClient) Copy(ids, newMailboxName string) *IMAPResponse {
 	return c.SendCommand(fmt.Sprintf("COPY %s \"%s\"", ids, newMailboxName))
 }

unreleased.md

@@ -49,3 +49,6 @@ Changelog [format](http://keepachangelog.com/en/1.0.0/)
 * GODT-732 Fix usage of fontawesome
 * GODT-915 Bump go-imap dependency and remove go-imap-specialuse dependency.
 * GODT-831 Cancel request of uploading attachment if reading/writing it fails.
+
+### Fixed
+* GODT-900 Remove \Deleted flag after re-importing the message (do not delete messages by moving to local folder and back).