From 63f089540e1f63c581b702450924e1983943faa7 Mon Sep 17 00:00:00 2001 From: Xavier Michelon Date: Thu, 16 Sep 2021 15:37:55 +0200 Subject: [PATCH] GODT-966 Append internal message to AllMail should be no action GODT-966 Fixed CI issues (WIP) WIP GODT-966 modified behavior of APPEND. WIP GODT-966 implemented test for possible order of operations. WIP GODT-966: code cleanup and refactoring. WIP GODT-966 fix for linter. Other: Minor refactoring. --- internal/imap/mailbox_append.go | 33 ++++++++++--- .../imap/message/move_without_support.feature | 36 +++++++++++++- test/imap_actions_messages_test.go | 48 ++++++++++++++++++- test/mocks/imap_response.go | 5 ++ 4 files changed, 111 insertions(+), 11 deletions(-) diff --git a/internal/imap/mailbox_append.go b/internal/imap/mailbox_append.go index 829e36e6..3c6ec225 100644 --- a/internal/imap/mailbox_append.go +++ b/internal/imap/mailbox_append.go @@ -116,11 +116,10 @@ func (im *imapMailbox) createMessage(imapFlags []string, date time.Time, r imap. if internalID != "" { if msg, err := im.storeMailbox.GetMessage(internalID); err == nil { if im.user.user.IsCombinedAddressMode() || im.storeAddress.AddressID() == msg.Message().AddressID { - return im.labelExistingMessage(msg.ID(), msg.IsMarkedDeleted()) + return im.labelExistingMessage(msg) } } } - return im.importMessage(kr, hdr, body, imapFlags, date) } @@ -146,7 +145,7 @@ func (im *imapMailbox) createDraftMessage(kr *crypto.KeyRing, email string, body return uidplus.AppendResponse(im.storeMailbox.UIDValidity(), im.storeMailbox.GetUIDList([]string{draft.ID})) } -func (im *imapMailbox) labelExistingMessage(messageID string, isDeleted bool) error { +func (im *imapMailbox) labelExistingMessage(msg storeMessageProvider) error { //nolint[funlen] im.log.Info("Labelling existing message") // IMAP clients can move message to local folder (setting \Deleted flag) @@ -156,17 +155,37 @@ func (im *imapMailbox) labelExistingMessage(messageID string, isDeleted bool) er // 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. - if isDeleted { - if err := im.storeMailbox.MarkMessagesUndeleted([]string{messageID}); err != nil { + if msg.IsMarkedDeleted() { + if err := im.storeMailbox.MarkMessagesUndeleted([]string{msg.ID()}); err != nil { log.WithError(err).Error("Failed to undelete re-imported message") } } - if err := im.storeMailbox.LabelMessages([]string{messageID}); err != nil { + storeMBox := im.storeMailbox + + // Outlook Uses APPEND instead of COPY. There is no need to copy to All Mail because messages are already there. + // if the message is copied from Spam or Trash, it should stay as is. Return error. + // Otherwise the message is already in All mail, Return OK. + if pmapi.AllMailLabel == storeMBox.LabelID() { + foundArchive := false + for _, mBox := range im.storeAddress.ListMailboxes() { + if mBox.LabelID() == pmapi.ArchiveLabel { + foundArchive = true + storeMBox = mBox + break + } + } + + if !foundArchive { + return errors.New("could not find Archive folder") + } + } + + if err := storeMBox.LabelMessages([]string{msg.ID()}); err != nil { return err } - return uidplus.AppendResponse(im.storeMailbox.UIDValidity(), im.storeMailbox.GetUIDList([]string{messageID})) + return uidplus.AppendResponse(storeMBox.UIDValidity(), storeMBox.GetUIDList([]string{msg.ID()})) } func (im *imapMailbox) importMessage(kr *crypto.KeyRing, hdr textproto.Header, body []byte, imapFlags []string, date time.Time) error { diff --git a/test/features/bridge/imap/message/move_without_support.feature b/test/features/bridge/imap/message/move_without_support.feature index a5e85a3c..eb4c4a09 100644 --- a/test/features/bridge/imap/message/move_without_support.feature +++ b/test/features/bridge/imap/message/move_without_support.feature @@ -12,7 +12,7 @@ Feature: IMAP move messages by append and delete (without MOVE support, e.g., Ou | 2 | jane.doe@mail.com | name@pm.me | bar | world | And there is IMAP client "source" selected in "INBOX" And there is IMAP client "target" selected in "" - When IMAP clients "source" and "target" move message seq "2" of "user" from "INBOX" to "" by append and delete + When IMAP clients "source" and "target" move message seq "2" of "user" from "INBOX" to "" Then IMAP response to "source" is "OK" Then IMAP response to "target" is "OK" When IMAP client "source" sends expunge @@ -38,7 +38,7 @@ Feature: IMAP move messages by append and delete (without MOVE support, e.g., Ou | 2 | jane.doe@mail.com | name@pm.me | bar | world | And there is IMAP client "source" selected in "" And there is IMAP client "target" selected in "INBOX" - When IMAP clients "source" and "target" move message seq "2" of "user" from "" to "INBOX" by append and delete + When IMAP clients "source" and "target" move message seq "2" of "user" from "" to "INBOX" Then IMAP response to "source" is "OK" Then IMAP response to "target" is "OK" When IMAP client "source" sends expunge @@ -54,3 +54,35 @@ Feature: IMAP move messages by append and delete (without MOVE support, e.g., Ou | mailbox | | Spam | | Trash | + + Scenario Outline: Move message from to All Mail by + Given there are messages in mailbox "" for "user" + | id | from | to | subject | body | + | 1 | john.doe@mail.com | user@pm.me | subj1 | body1 | + | 2 | john.doe@mail.com | name@pm.me | subj2 | body2 | + And there is IMAP client "source" selected in "" + And there is IMAP client "target" selected in "All Mail" + When IMAP clients "source" and "target" move message seq "2" of "user" to "All Mail" by + Then IMAP response to "source" is "OK" + Then IMAP response to "target" is "OK" + And mailbox "" for "user" has messages + | from | to | subject | + | john.doe@mail.com | user@pm.me | subj1 | + And mailbox "All Mail" for "user" has messages + | from | to | subject | + | john.doe@mail.com | user@pm.me | subj1 | + | john.doe@mail.com | name@pm.me | subj2 | + Examples: + | mailbox | order | + | INBOX | APPEND DELETE EXPUNGE | + | Archive | APPEND DELETE EXPUNGE | + | Trash | APPEND DELETE EXPUNGE | + | Spam | APPEND DELETE EXPUNGE | + | INBOX | DELETE APPEND EXPUNGE | + | Archive | DELETE APPEND EXPUNGE | + | Trash | DELETE APPEND EXPUNGE | + | Spam | DELETE APPEND EXPUNGE | + | INBOX | DELETE EXPUNGE APPEND | + | Archive | DELETE EXPUNGE APPEND | + | Trash | DELETE EXPUNGE APPEND | + | Spam | DELETE EXPUNGE APPEND | diff --git a/test/imap_actions_messages_test.go b/test/imap_actions_messages_test.go index 0e2881ca..02d88d83 100644 --- a/test/imap_actions_messages_test.go +++ b/test/imap_actions_messages_test.go @@ -18,10 +18,14 @@ package tests import ( + "errors" "fmt" "strconv" + "strings" "sync" + "time" + "github.com/ProtonMail/proton-bridge/test/mocks" "github.com/cucumber/godog" "golang.org/x/net/html/charset" ) @@ -35,7 +39,8 @@ func IMAPActionsMessagesFeatureContext(s *godog.ScenarioContext) { s.Step(`^IMAP client searches for "([^"]*)"$`, imapClientSearchesFor) s.Step(`^IMAP client copies message seq "([^"]*)" to "([^"]*)"$`, imapClientCopiesMessagesTo) s.Step(`^IMAP client moves message seq "([^"]*)" to "([^"]*)"$`, imapClientMovesMessagesTo) - s.Step(`^IMAP clients "([^"]*)" and "([^"]*)" move message seq "([^"]*)" of "([^"]*)" from "([^"]*)" to "([^"]*)" by append and delete$`, imapClientsMoveMessageSeqOfUserFromToByAppendAndDelete) + s.Step(`^IMAP clients "([^"]*)" and "([^"]*)" move message seq "([^"]*)" of "([^"]*)" from "([^"]*)" to "([^"]*)"$`, imapClientsMoveMessageSeqOfUserFromTo) + s.Step(`^IMAP clients "([^"]*)" and "([^"]*)" move message seq "([^"]*)" of "([^"]*)" to "([^"]*)" by ([^"]*) ([^"]*) ([^"]*)`, imapClientsMoveMessageSeqOfUserFromToByOrederedOperations) s.Step(`^IMAP client imports message to "([^"]*)"$`, imapClientCreatesMessage) s.Step(`^IMAP client imports message to "([^"]*)" with encoding "([^"]*)"$`, imapClientCreatesMessageWithEncoding) s.Step(`^IMAP client creates message "([^"]*)" from "([^"]*)" to "([^"]*)" with body "([^"]*)" in "([^"]*)"$`, imapClientCreatesMessageFromToWithBody) @@ -113,7 +118,7 @@ func imapClientMovesMessagesTo(messageSeq, newMailboxName string) error { return nil } -func imapClientsMoveMessageSeqOfUserFromToByAppendAndDelete(sourceIMAPClient, targetIMAPClient, messageSeq, bddUserID, sourceMailboxName, targetMailboxName string) error { +func imapClientsMoveMessageSeqOfUserFromTo(sourceIMAPClient, targetIMAPClient, messageSeq, bddUserID, sourceMailboxName, targetMailboxName string) error { account := ctx.GetTestAccount(bddUserID) if account == nil { return godog.ErrPending @@ -160,6 +165,45 @@ func imapClientsMoveMessageSeqOfUserFromToByAppendAndDelete(sourceIMAPClient, ta return nil } +func extractMessageBodyFromImapResponse(response *mocks.IMAPResponse) (string, error) { + sections := response.Sections() + if len(sections) != 1 { + return "", internalError(errors.New("unexpected result from FETCH"), "retrieving message body using FETCH") + } + sections = strings.Split(sections[0], "\n") + if len(sections) < 2 { + return "", internalError(errors.New("failed to parse FETCH result"), "extraction body from FETCH reply") + } + return strings.Join(sections[1:], "\n"), nil +} + +func imapClientsMoveMessageSeqOfUserFromToByOrederedOperations(sourceIMAPClient, targetIMAPClient, messageSeq, bddUserID, targetMailboxName, op1, op2, op3 string) error { + account := ctx.GetTestAccount(bddUserID) + if account == nil { + return godog.ErrPending + } + msgStr, err := extractMessageBodyFromImapResponse(ctx.GetIMAPClient(sourceIMAPClient).Fetch(messageSeq, "BODY.PEEK[]").AssertOK()) + if err != nil { + return err + } + + for _, op := range []string{op1, op2, op3} { + switch op { + case "APPEND": + res := ctx.GetIMAPClient(targetIMAPClient).Append(targetMailboxName, msgStr) + ctx.SetIMAPLastResponse(targetIMAPClient, res) + case "DELETE": + _ = imapClientNamedMarksMessageSeqAsDeleted(sourceIMAPClient, messageSeq) + case "EXPUNGE": + _ = imapClientNamedExpunge(sourceIMAPClient) + default: + return errors.New("unknow IMAP operation " + op) + } + time.Sleep(100 * time.Millisecond) + } + return nil +} + func imapClientCreatesMessage(mailboxName string, message *godog.DocString) error { return imapClientCreatesMessageWithEncoding(mailboxName, "utf8", message) } diff --git a/test/mocks/imap_response.go b/test/mocks/imap_response.go index 7c609bb7..606b986e 100644 --- a/test/mocks/imap_response.go +++ b/test/mocks/imap_response.go @@ -107,6 +107,11 @@ func (ir *IMAPResponse) AssertOK() *IMAPResponse { return ir } +func (ir *IMAPResponse) Sections() []string { + ir.wait() + return ir.sections +} + func (ir *IMAPResponse) AssertResult(wantResult string) *IMAPResponse { ir.wait() a.NoError(ir.t, ir.err)