diff --git a/Makefile b/Makefile index 2980bdff..e781f4f2 100644 --- a/Makefile +++ b/Makefile @@ -209,7 +209,7 @@ coverage: test mocks: mockgen --package mocks github.com/ProtonMail/proton-bridge/internal/users Configer,PanicHandler,ClientManager,CredentialsStorer,StoreMaker > internal/users/mocks/mocks.go mockgen --package mocks github.com/ProtonMail/proton-bridge/internal/transfer PanicHandler,ClientManager,IMAPClientProvider > internal/transfer/mocks/mocks.go - mockgen --package mocks github.com/ProtonMail/proton-bridge/internal/store PanicHandler,ClientManager,BridgeUser > internal/store/mocks/mocks.go + mockgen --package mocks github.com/ProtonMail/proton-bridge/internal/store PanicHandler,ClientManager,BridgeUser,ChangeNotifier > internal/store/mocks/mocks.go mockgen --package mocks github.com/ProtonMail/proton-bridge/pkg/listener Listener > internal/store/mocks/utils_mocks.go mockgen --package mocks github.com/ProtonMail/proton-bridge/pkg/pmapi Client > pkg/pmapi/mocks/mocks.go diff --git a/internal/imap/backend.go b/internal/imap/backend.go index 5a8063cf..d80fb5e1 100644 --- a/internal/imap/backend.go +++ b/internal/imap/backend.go @@ -46,6 +46,9 @@ type imapBackend struct { imapCache map[string]map[string]string imapCachePath string imapCacheLock *sync.RWMutex + + updatesBlocking map[string]bool + updatesBlockingLocker sync.Locker } // NewIMAPBackend returns struct implementing go-imap/backend interface. @@ -58,10 +61,6 @@ func NewIMAPBackend( bridgeWrap := newBridgeWrap(bridge) backend := newIMAPBackend(panicHandler, cfg, bridgeWrap, eventListener) - // We want idle updates coming from bridge's updates channel (which in turn come - // from the bridge users' stores) to be sent to the imap backend's update channel. - backend.updates = bridge.GetIMAPUpdatesChannel() - go backend.monitorDisconnectedUsers() return backend @@ -84,6 +83,9 @@ func newIMAPBackend( imapCachePath: cfg.GetIMAPCachePath(), imapCacheLock: &sync.RWMutex{}, + + updatesBlocking: map[string]bool{}, + updatesBlockingLocker: &sync.Mutex{}, } } @@ -169,7 +171,9 @@ func (ib *imapBackend) Login(_ *imap.ConnInfo, username, password string) (goIMA // The update channel should be nil until we try to login to IMAP for the first time // so that it doesn't make bridge slow for users who are only using bridge for SMTP // (otherwise the store will be locked for 1 sec per email during synchronization). - imapUser.user.SetIMAPIdleUpdateChannel() + if store := imapUser.user.GetStore(); store != nil { + store.SetChangeNotifier(ib) + } return imapUser, nil } diff --git a/internal/imap/backend_updates.go b/internal/imap/backend_updates.go new file mode 100644 index 00000000..85db3a4a --- /dev/null +++ b/internal/imap/backend_updates.go @@ -0,0 +1,169 @@ +// Copyright (c) 2020 Proton Technologies AG +// +// This file is part of ProtonMail Bridge. +// +// ProtonMail Bridge is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// ProtonMail Bridge is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with ProtonMail Bridge. If not, see . + +package imap + +import ( + "strings" + "time" + + "github.com/ProtonMail/proton-bridge/internal/store" + "github.com/ProtonMail/proton-bridge/pkg/message" + "github.com/ProtonMail/proton-bridge/pkg/pmapi" + imap "github.com/emersion/go-imap" + goIMAPBackend "github.com/emersion/go-imap/backend" + "github.com/sirupsen/logrus" +) + +type operation string + +const ( + operationUpdateMessage operation = "store" + operationDeleteMessage operation = "expunge" +) + +func (ib *imapBackend) setUpdatesBeBlocking(address, mailboxName string, op operation) { + ib.changeUpdatesBlocking(address, mailboxName, op, true) +} + +func (ib *imapBackend) unsetUpdatesBeBlocking(address, mailboxName string, op operation) { + ib.changeUpdatesBlocking(address, mailboxName, op, false) +} + +func (ib *imapBackend) changeUpdatesBlocking(address, mailboxName string, op operation, block bool) { + ib.updatesBlockingLocker.Lock() + defer ib.updatesBlockingLocker.Unlock() + + key := strings.ToLower(address + "_" + mailboxName + "_" + string(op)) + if block { + ib.updatesBlocking[key] = true + } else { + delete(ib.updatesBlocking, key) + } +} + +func (ib *imapBackend) isBlocking(address, mailboxName string, op operation) bool { + key := strings.ToLower(address + "_" + mailboxName + "_" + string(op)) + return ib.updatesBlocking[key] +} + +func (ib *imapBackend) Notice(address, notice string) { + update := new(goIMAPBackend.StatusUpdate) + update.Update = goIMAPBackend.NewUpdate(address, "") + update.StatusResp = &imap.StatusResp{ + Type: imap.StatusRespOk, + Code: imap.CodeAlert, + Info: notice, + } + ib.sendIMAPUpdate(update, false) +} + +func (ib *imapBackend) UpdateMessage( + address, mailboxName string, + uid, sequenceNumber uint32, + msg *pmapi.Message, hasDeletedFlag bool, +) { + log.WithFields(logrus.Fields{ + "address": address, + "mailbox": mailboxName, + "seqNum": sequenceNumber, + "uid": uid, + "flags": message.GetFlags(msg), + "deleted": hasDeletedFlag, + }).Trace("IDLE update") + update := new(goIMAPBackend.MessageUpdate) + update.Update = goIMAPBackend.NewUpdate(address, mailboxName) + update.Message = imap.NewMessage(sequenceNumber, []imap.FetchItem{imap.FetchFlags, imap.FetchUid}) + update.Message.Flags = message.GetFlags(msg) + if hasDeletedFlag { + update.Message.Flags = append(update.Message.Flags, imap.DeletedFlag) + } + update.Message.Uid = uid + ib.sendIMAPUpdate(update, ib.isBlocking(address, mailboxName, operationUpdateMessage)) +} + +func (ib *imapBackend) DeleteMessage(address, mailboxName string, sequenceNumber uint32) { + log.WithFields(logrus.Fields{ + "address": address, + "mailbox": mailboxName, + "seqNum": sequenceNumber, + }).Trace("IDLE delete") + update := new(goIMAPBackend.ExpungeUpdate) + update.Update = goIMAPBackend.NewUpdate(address, mailboxName) + update.SeqNum = sequenceNumber + ib.sendIMAPUpdate(update, ib.isBlocking(address, mailboxName, operationDeleteMessage)) +} + +func (ib *imapBackend) MailboxCreated(address, mailboxName string) { + log.WithFields(logrus.Fields{ + "address": address, + "mailbox": mailboxName, + }).Trace("IDLE mailbox info") + update := new(goIMAPBackend.MailboxInfoUpdate) + update.Update = goIMAPBackend.NewUpdate(address, "") + update.MailboxInfo = &imap.MailboxInfo{ + Attributes: []string{imap.NoInferiorsAttr}, + Delimiter: store.PathDelimiter, + Name: mailboxName, + } + ib.sendIMAPUpdate(update, false) +} + +func (ib *imapBackend) MailboxStatus(address, mailboxName string, total, unread, unreadSeqNum uint32) { + log.WithFields(logrus.Fields{ + "address": address, + "mailbox": mailboxName, + "total": total, + "unread": unread, + "unreadSeqNum": unreadSeqNum, + }).Trace("IDLE status") + update := new(goIMAPBackend.MailboxUpdate) + update.Update = goIMAPBackend.NewUpdate(address, mailboxName) + update.MailboxStatus = imap.NewMailboxStatus(mailboxName, []imap.StatusItem{imap.StatusMessages, imap.StatusUnseen}) + update.MailboxStatus.Messages = total + update.MailboxStatus.Unseen = unread + update.MailboxStatus.UnseenSeqNum = unreadSeqNum + ib.sendIMAPUpdate(update, false) +} + +func (ib *imapBackend) sendIMAPUpdate(update goIMAPBackend.Update, block bool) { + if ib.updates == nil { + log.Trace("IMAP IDLE unavailable") + return + } + + done := update.Done() + go func() { + select { + case <-time.After(1 * time.Second): + log.Warn("IMAP update could not be sent (timeout)") + return + case ib.updates <- update: + } + }() + + if !block { + return + } + + select { + case <-done: + case <-time.After(1 * time.Second): + log.Warn("IMAP update could not be delivered (timeout).") + return + } +} diff --git a/internal/imap/bridge.go b/internal/imap/bridge.go index 947cc59d..271775cb 100644 --- a/internal/imap/bridge.go +++ b/internal/imap/bridge.go @@ -40,7 +40,6 @@ type bridgeUser interface { IsCombinedAddressMode() bool GetAddressID(address string) (string, error) GetPrimaryAddress() string - SetIMAPIdleUpdateChannel() UpdateUser() error Logout() error CloseConnection(address string) diff --git a/internal/imap/mailbox.go b/internal/imap/mailbox.go index 746382dc..07e6f3d4 100644 --- a/internal/imap/mailbox.go +++ b/internal/imap/mailbox.go @@ -177,6 +177,9 @@ func (im *imapMailbox) Check() error { // Expunge permanently removes all messages that have the \Deleted flag set // from the currently selected mailbox. func (im *imapMailbox) Expunge() error { + im.user.backend.setUpdatesBeBlocking(im.user.currentAddressLowercase, im.name, operationDeleteMessage) + defer im.user.backend.unsetUpdatesBeBlocking(im.user.currentAddressLowercase, im.name, operationDeleteMessage) + return im.storeMailbox.RemoveDeleted() } diff --git a/internal/imap/mailbox_messages.go b/internal/imap/mailbox_messages.go index 95419dfd..a18da1b4 100644 --- a/internal/imap/mailbox_messages.go +++ b/internal/imap/mailbox_messages.go @@ -46,6 +46,9 @@ func (im *imapMailbox) UpdateMessagesFlags(uid bool, seqSet *imap.SeqSet, operat // Called from go-imap in goroutines - we need to handle panics for each function. defer im.panicHandler.HandlePanic() + im.user.backend.setUpdatesBeBlocking(im.user.currentAddressLowercase, im.name, operationUpdateMessage) + defer im.user.backend.unsetUpdatesBeBlocking(im.user.currentAddressLowercase, im.name, operationUpdateMessage) + messageIDs, err := im.apiIDsFromSeqSet(uid, seqSet) if err != nil || len(messageIDs) == 0 { return err diff --git a/internal/imap/store.go b/internal/imap/store.go index d20bf10a..61694b16 100644 --- a/internal/imap/store.go +++ b/internal/imap/store.go @@ -43,6 +43,8 @@ type storeUserProvider interface { parentID string) (*pmapi.Message, []*pmapi.Attachment, error) PauseEventLoop(bool) + + SetChangeNotifier(store.ChangeNotifier) } type storeAddressProvider interface { diff --git a/internal/store/address_mailbox.go b/internal/store/address_mailbox.go index 33efcdf7..35bcad96 100644 --- a/internal/store/address_mailbox.go +++ b/internal/store/address_mailbox.go @@ -78,7 +78,7 @@ func (storeAddress *Address) createOrUpdateMailboxEvent(label *pmapi.Label) erro return err } storeAddress.mailboxes[label.ID] = mailbox - mailbox.store.imapMailboxCreated(storeAddress.address, mailbox.labelName) + mailbox.store.notifyMailboxCreated(storeAddress.address, mailbox.labelName) } else { mailbox.labelName = prefix + label.Path mailbox.color = label.Color diff --git a/internal/store/change.go b/internal/store/change.go index 0eade6ff..b6894b1e 100644 --- a/internal/store/change.go +++ b/internal/store/change.go @@ -18,119 +18,56 @@ package store import ( - "time" - - "github.com/ProtonMail/proton-bridge/pkg/message" "github.com/ProtonMail/proton-bridge/pkg/pmapi" - imap "github.com/emersion/go-imap" - imapBackend "github.com/emersion/go-imap/backend" - "github.com/sirupsen/logrus" ) -// SetIMAPUpdateChannel sets the channel on which imap update messages will be sent. This should be the channel -// on which the imap backend listens for imap updates. -func (store *Store) SetIMAPUpdateChannel(updates chan imapBackend.Update) { - store.log.Debug("Listening for IMAP updates") - - if store.imapUpdates = updates; store.imapUpdates == nil { - store.log.Error("The IMAP Updates channel is nil") - } +type ChangeNotifier interface { + Notice(address, notice string) + UpdateMessage( + address, mailboxName string, + uid, sequenceNumber uint32, + msg *pmapi.Message, hasDeletedFlag bool) + DeleteMessage(address, mailboxName string, sequenceNumber uint32) + MailboxCreated(address, mailboxName string) + MailboxStatus(address, mailboxName string, total, unread, unreadSeqNum uint32) } -func (store *Store) imapNotice(address, notice string) *imapBackend.StatusUpdate { - update := new(imapBackend.StatusUpdate) - update.Update = imapBackend.NewUpdate(address, "") - update.StatusResp = &imap.StatusResp{ - Type: imap.StatusRespOk, - Code: imap.CodeAlert, - Info: notice, - } - store.imapSendUpdate(update) - return update +// SetChangeNotifier sets notifier to be called once mailbox or message changes. +func (store *Store) SetChangeNotifier(notifier ChangeNotifier) { + store.notifier = notifier } -func (store *Store) imapUpdateMessage( - address, mailboxName string, - uid, sequenceNumber uint32, - msg *pmapi.Message, hasDeletedFlag bool, -) *imapBackend.MessageUpdate { - store.log.WithFields(logrus.Fields{ - "address": address, - "mailbox": mailboxName, - "seqNum": sequenceNumber, - "uid": uid, - "flags": message.GetFlags(msg), - "deleted": hasDeletedFlag, - }).Trace("IDLE update") - update := new(imapBackend.MessageUpdate) - update.Update = imapBackend.NewUpdate(address, mailboxName) - update.Message = imap.NewMessage(sequenceNumber, []imap.FetchItem{imap.FetchFlags, imap.FetchUid}) - update.Message.Flags = message.GetFlags(msg) - if hasDeletedFlag { - update.Message.Flags = append(update.Message.Flags, imap.DeletedFlag) - } - update.Message.Uid = uid - store.imapSendUpdate(update) - return update -} - -func (store *Store) imapDeleteMessage(address, mailboxName string, sequenceNumber uint32) *imapBackend.ExpungeUpdate { - store.log.WithFields(logrus.Fields{ - "address": address, - "mailbox": mailboxName, - "seqNum": sequenceNumber, - }).Trace("IDLE delete") - update := new(imapBackend.ExpungeUpdate) - update.Update = imapBackend.NewUpdate(address, mailboxName) - update.SeqNum = sequenceNumber - store.imapSendUpdate(update) - return update -} - -func (store *Store) imapMailboxCreated(address, mailboxName string) *imapBackend.MailboxInfoUpdate { - store.log.WithFields(logrus.Fields{ - "address": address, - "mailbox": mailboxName, - }).Trace("IDLE mailbox info") - update := new(imapBackend.MailboxInfoUpdate) - update.Update = imapBackend.NewUpdate(address, "") - update.MailboxInfo = &imap.MailboxInfo{ - Attributes: []string{imap.NoInferiorsAttr}, - Delimiter: PathDelimiter, - Name: mailboxName, - } - store.imapSendUpdate(update) - return update -} - -func (store *Store) imapMailboxStatus(address, mailboxName string, total, unread, unreadSeqNum uint) *imapBackend.MailboxUpdate { - store.log.WithFields(logrus.Fields{ - "address": address, - "mailbox": mailboxName, - "total": total, - "unread": unread, - "unreadSeqNum": unreadSeqNum, - }).Trace("IDLE status") - update := new(imapBackend.MailboxUpdate) - update.Update = imapBackend.NewUpdate(address, mailboxName) - update.MailboxStatus = imap.NewMailboxStatus(mailboxName, []imap.StatusItem{imap.StatusMessages, imap.StatusUnseen}) - update.MailboxStatus.Messages = uint32(total) - update.MailboxStatus.Unseen = uint32(unread) - update.MailboxStatus.UnseenSeqNum = uint32(unreadSeqNum) - store.imapSendUpdate(update) - return update -} - -func (store *Store) imapSendUpdate(update imapBackend.Update) { - if store.imapUpdates == nil { - store.log.Trace("IMAP IDLE unavailable") +func (store *Store) notifyNotice(address, notice string) { + if store.notifier == nil { return } - - select { - case <-time.After(1 * time.Second): - store.log.Warn("IMAP update could not be sent (timeout)") - return - case store.imapUpdates <- update: - } + store.notifier.Notice(address, notice) +} + +func (store *Store) notifyUpdateMessage(address, mailboxName string, uid, sequenceNumber uint32, msg *pmapi.Message, hasDeletedFlag bool) { + if store.notifier == nil { + return + } + store.notifier.UpdateMessage(address, mailboxName, uid, sequenceNumber, msg, hasDeletedFlag) +} + +func (store *Store) notifyDeleteMessage(address, mailboxName string, sequenceNumber uint32) { + if store.notifier == nil { + return + } + store.notifier.DeleteMessage(address, mailboxName, sequenceNumber) +} + +func (store *Store) notifyMailboxCreated(address, mailboxName string) { + if store.notifier == nil { + return + } + store.notifier.MailboxCreated(address, mailboxName) +} + +func (store *Store) notifyMailboxStatus(address, mailboxName string, total, unread, unreadSeqNum uint) { + if store.notifier == nil { + return + } + store.notifier.MailboxStatus(address, mailboxName, uint32(total), uint32(unread), uint32(unreadSeqNum)) } diff --git a/internal/store/change_test.go b/internal/store/change_test.go index 05ecdec9..316ced32 100644 --- a/internal/store/change_test.go +++ b/internal/store/change_test.go @@ -21,52 +21,43 @@ import ( "testing" "github.com/ProtonMail/proton-bridge/pkg/pmapi" - imapBackend "github.com/emersion/go-imap/backend" + "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" ) -func TestCreateOrUpdateMessageIMAPUpdates(t *testing.T) { +func TestNotifyChangeCreateOrUpdateMessage(t *testing.T) { m, clear := initMocks(t) defer clear() - updates := make(chan imapBackend.Update) + m.changeNotifier.EXPECT().MailboxStatus(addr1, "All Mail", uint32(1), uint32(0), uint32(0)) + m.changeNotifier.EXPECT().MailboxStatus(addr1, "All Mail", uint32(2), uint32(0), uint32(0)) + m.changeNotifier.EXPECT().UpdateMessage(addr1, "All Mail", uint32(1), uint32(1), gomock.Any(), false) + m.changeNotifier.EXPECT().UpdateMessage(addr1, "All Mail", uint32(2), uint32(2), gomock.Any(), false) m.newStoreNoEvents(true) - m.store.SetIMAPUpdateChannel(updates) - - go checkIMAPUpdates(t, updates, []func(interface{}) bool{ - checkMessageUpdate(addr1, "All Mail", 1, 1), - checkMessageUpdate(addr1, "All Mail", 2, 2), - }) + m.store.SetChangeNotifier(m.changeNotifier) insertMessage(t, m, "msg1", "Test message 1", addrID1, 0, []string{pmapi.AllMailLabel}) insertMessage(t, m, "msg2", "Test message 2", addrID1, 0, []string{pmapi.AllMailLabel}) - - close(updates) } -func TestCreateOrUpdateMessageIMAPUpdatesBulkUpdate(t *testing.T) { +func TestNotifyChangeCreateOrUpdateMessages(t *testing.T) { m, clear := initMocks(t) defer clear() - updates := make(chan imapBackend.Update) + m.changeNotifier.EXPECT().MailboxStatus(addr1, "All Mail", uint32(2), uint32(0), uint32(0)) + m.changeNotifier.EXPECT().UpdateMessage(addr1, "All Mail", uint32(1), uint32(1), gomock.Any(), false) + m.changeNotifier.EXPECT().UpdateMessage(addr1, "All Mail", uint32(2), uint32(2), gomock.Any(), false) m.newStoreNoEvents(true) - m.store.SetIMAPUpdateChannel(updates) - - go checkIMAPUpdates(t, updates, []func(interface{}) bool{ - checkMessageUpdate(addr1, "All Mail", 1, 1), - checkMessageUpdate(addr1, "All Mail", 2, 2), - }) + m.store.SetChangeNotifier(m.changeNotifier) msg1 := getTestMessage("msg1", "Test message 1", addrID1, 0, []string{pmapi.AllMailLabel}) msg2 := getTestMessage("msg2", "Test message 2", addrID1, 0, []string{pmapi.AllMailLabel}) require.Nil(t, m.store.createOrUpdateMessagesEvent([]*pmapi.Message{msg1, msg2})) - - close(updates) } -func TestDeleteMessageIMAPUpdate(t *testing.T) { +func TestNotifyChangeDeleteMessage(t *testing.T) { m, clear := initMocks(t) defer clear() @@ -75,55 +66,10 @@ func TestDeleteMessageIMAPUpdate(t *testing.T) { insertMessage(t, m, "msg1", "Test message 1", addrID1, 0, []string{pmapi.AllMailLabel}) insertMessage(t, m, "msg2", "Test message 2", addrID1, 0, []string{pmapi.AllMailLabel}) - updates := make(chan imapBackend.Update) - m.store.SetIMAPUpdateChannel(updates) - go checkIMAPUpdates(t, updates, []func(interface{}) bool{ - checkMessageDelete(addr1, "All Mail", 2), - checkMessageDelete(addr1, "All Mail", 1), - }) + m.changeNotifier.EXPECT().DeleteMessage(addr1, "All Mail", uint32(2)) + m.changeNotifier.EXPECT().DeleteMessage(addr1, "All Mail", uint32(1)) + m.store.SetChangeNotifier(m.changeNotifier) require.Nil(t, m.store.deleteMessageEvent("msg2")) require.Nil(t, m.store.deleteMessageEvent("msg1")) - close(updates) -} - -func checkIMAPUpdates(t *testing.T, updates chan imapBackend.Update, checkFunctions []func(interface{}) bool) { - idx := 0 - for update := range updates { - if idx >= len(checkFunctions) { - continue - } - if !checkFunctions[idx](update) { - continue - } - idx++ - } - require.True(t, idx == len(checkFunctions), "Less updates than expected: %+v of %+v", idx, len(checkFunctions)) -} - -func checkMessageUpdate(username, mailbox string, seqNum, uid int) func(interface{}) bool { //nolint[unparam] - return func(update interface{}) bool { - switch u := update.(type) { - case *imapBackend.MessageUpdate: - return (u.Update.Username() == username && - u.Update.Mailbox() == mailbox && - u.Message.SeqNum == uint32(seqNum) && - u.Message.Uid == uint32(uid)) - default: - return false - } - } -} - -func checkMessageDelete(username, mailbox string, seqNum int) func(interface{}) bool { //nolint[unparam] - return func(update interface{}) bool { - switch u := update.(type) { - case *imapBackend.ExpungeUpdate: - return (u.Update.Username() == username && - u.Update.Mailbox() == mailbox && - u.SeqNum == uint32(seqNum)) - default: - return false - } - } } diff --git a/internal/store/event_loop.go b/internal/store/event_loop.go index 12500166..4ef94a16 100644 --- a/internal/store/event_loop.go +++ b/internal/store/event_loop.go @@ -571,7 +571,7 @@ func (loop *eventLoop) processNotices(l *logrus.Entry, notices []string) { for _, notice := range notices { l.Infof("Notice: %q", notice) for _, address := range loop.user.GetStoreAddresses() { - loop.store.imapNotice(address, notice) + loop.store.notifyNotice(address, notice) } } } diff --git a/internal/store/mailbox_message.go b/internal/store/mailbox_message.go index 114f7350..313f01a1 100644 --- a/internal/store/mailbox_message.go +++ b/internal/store/mailbox_message.go @@ -18,8 +18,6 @@ package store import ( - "time" - "github.com/ProtonMail/proton-bridge/pkg/pmapi" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -356,7 +354,7 @@ func (storeMailbox *Mailbox) txCreateOrUpdateMessages(tx *bolt.Tx, msgs []*pmapi } isMarkedAsDeleted := deletedBucket.Get([]byte(msg.ID)) != nil if seqErr == nil { - storeMailbox.store.imapUpdateMessage( + storeMailbox.store.notifyUpdateMessage( storeMailbox.storeAddress.address, storeMailbox.labelName, btoi(uidb), @@ -390,7 +388,7 @@ func (storeMailbox *Mailbox) txCreateOrUpdateMessages(tx *bolt.Tx, msgs []*pmapi if err != nil { return errors.Wrap(err, "cannot get sequence number from UID") } - storeMailbox.store.imapUpdateMessage( + storeMailbox.store.notifyUpdateMessage( storeMailbox.storeAddress.address, storeMailbox.labelName, uid, @@ -441,7 +439,7 @@ func (storeMailbox *Mailbox) txDeleteMessage(tx *bolt.Tx, apiID string) error { } if seqNumErr == nil { - storeMailbox.store.imapDeleteMessage( + storeMailbox.store.notifyDeleteMessage( storeMailbox.storeAddress.address, storeMailbox.labelName, seqNum, @@ -459,7 +457,7 @@ func (storeMailbox *Mailbox) txMailboxStatusUpdate(tx *bolt.Tx) error { if err != nil { return errors.Wrap(err, "cannot get counts for mailbox status update") } - storeMailbox.store.imapMailboxStatus( + storeMailbox.store.notifyMailboxStatus( storeMailbox.storeAddress.address, storeMailbox.labelName, total, @@ -503,7 +501,7 @@ func (storeMailbox *Mailbox) txMarkMessagesAsDeleted(tx *bolt.Tx, apiIDs []strin // In order to send flags in format // S: * 2 FETCH (FLAGS (\Deleted \Seen)) - update := storeMailbox.store.imapUpdateMessage( + storeMailbox.store.notifyUpdateMessage( storeMailbox.storeAddress.address, storeMailbox.labelName, uid, @@ -511,14 +509,6 @@ func (storeMailbox *Mailbox) txMarkMessagesAsDeleted(tx *bolt.Tx, apiIDs []strin msg, markAsDeleted, ) - - // txMarkMessagesAsDeleted is called only during processing request - // from IMAP call (i.e., not from event loop) and in such cases we - // have to wait to propagate update back before closing the response. - select { - case <-time.After(1 * time.Second): - case <-update.Done(): - } } return nil diff --git a/internal/store/mocks/mocks.go b/internal/store/mocks/mocks.go index ef208982..86c96a50 100644 --- a/internal/store/mocks/mocks.go +++ b/internal/store/mocks/mocks.go @@ -1,14 +1,13 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: github.com/ProtonMail/proton-bridge/internal/store (interfaces: PanicHandler,ClientManager,BridgeUser) +// Source: github.com/ProtonMail/proton-bridge/internal/store (interfaces: PanicHandler,ClientManager,BridgeUser,ChangeNotifier) // Package mocks is a generated GoMock package. package mocks import ( - reflect "reflect" - pmapi "github.com/ProtonMail/proton-bridge/pkg/pmapi" gomock "github.com/golang/mock/gomock" + reflect "reflect" ) // MockPanicHandler is a mock of PanicHandler interface @@ -242,3 +241,86 @@ func (mr *MockBridgeUserMockRecorder) UpdateUser() *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateUser", reflect.TypeOf((*MockBridgeUser)(nil).UpdateUser)) } + +// MockChangeNotifier is a mock of ChangeNotifier interface +type MockChangeNotifier struct { + ctrl *gomock.Controller + recorder *MockChangeNotifierMockRecorder +} + +// MockChangeNotifierMockRecorder is the mock recorder for MockChangeNotifier +type MockChangeNotifierMockRecorder struct { + mock *MockChangeNotifier +} + +// NewMockChangeNotifier creates a new mock instance +func NewMockChangeNotifier(ctrl *gomock.Controller) *MockChangeNotifier { + mock := &MockChangeNotifier{ctrl: ctrl} + mock.recorder = &MockChangeNotifierMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockChangeNotifier) EXPECT() *MockChangeNotifierMockRecorder { + return m.recorder +} + +// DeleteMessage mocks base method +func (m *MockChangeNotifier) DeleteMessage(arg0, arg1 string, arg2 uint32) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "DeleteMessage", arg0, arg1, arg2) +} + +// DeleteMessage indicates an expected call of DeleteMessage +func (mr *MockChangeNotifierMockRecorder) DeleteMessage(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteMessage", reflect.TypeOf((*MockChangeNotifier)(nil).DeleteMessage), arg0, arg1, arg2) +} + +// MailboxCreated mocks base method +func (m *MockChangeNotifier) MailboxCreated(arg0, arg1 string) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "MailboxCreated", arg0, arg1) +} + +// MailboxCreated indicates an expected call of MailboxCreated +func (mr *MockChangeNotifierMockRecorder) MailboxCreated(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "MailboxCreated", reflect.TypeOf((*MockChangeNotifier)(nil).MailboxCreated), arg0, arg1) +} + +// MailboxStatus mocks base method +func (m *MockChangeNotifier) MailboxStatus(arg0, arg1 string, arg2, arg3, arg4 uint32) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "MailboxStatus", arg0, arg1, arg2, arg3, arg4) +} + +// MailboxStatus indicates an expected call of MailboxStatus +func (mr *MockChangeNotifierMockRecorder) MailboxStatus(arg0, arg1, arg2, arg3, arg4 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "MailboxStatus", reflect.TypeOf((*MockChangeNotifier)(nil).MailboxStatus), arg0, arg1, arg2, arg3, arg4) +} + +// Notice mocks base method +func (m *MockChangeNotifier) Notice(arg0, arg1 string) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "Notice", arg0, arg1) +} + +// Notice indicates an expected call of Notice +func (mr *MockChangeNotifierMockRecorder) Notice(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Notice", reflect.TypeOf((*MockChangeNotifier)(nil).Notice), arg0, arg1) +} + +// UpdateMessage mocks base method +func (m *MockChangeNotifier) UpdateMessage(arg0, arg1 string, arg2, arg3 uint32, arg4 *pmapi.Message, arg5 bool) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "UpdateMessage", arg0, arg1, arg2, arg3, arg4, arg5) +} + +// UpdateMessage indicates an expected call of UpdateMessage +func (mr *MockChangeNotifierMockRecorder) UpdateMessage(arg0, arg1, arg2, arg3, arg4, arg5 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateMessage", reflect.TypeOf((*MockChangeNotifier)(nil).UpdateMessage), arg0, arg1, arg2, arg3, arg4, arg5) +} diff --git a/internal/store/store.go b/internal/store/store.go index a299c65c..3a4f53fd 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -26,7 +26,6 @@ import ( "github.com/ProtonMail/proton-bridge/pkg/listener" "github.com/ProtonMail/proton-bridge/pkg/pmapi" - imapBackend "github.com/emersion/go-imap/backend" "github.com/hashicorp/go-multierror" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -100,12 +99,12 @@ type Store struct { log *logrus.Entry - cache *Cache - filePath string - db *bolt.DB - lock *sync.RWMutex - addresses map[string]*Address - imapUpdates chan imapBackend.Update + cache *Cache + filePath string + db *bolt.DB + lock *sync.RWMutex + addresses map[string]*Address + notifier ChangeNotifier isSyncRunning bool syncCooldown cooldown diff --git a/internal/store/store_test.go b/internal/store/store_test.go index ccf717fb..3e4b5130 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -44,13 +44,14 @@ const ( type mocksForStore struct { tb testing.TB - ctrl *gomock.Controller - events *storemocks.MockListener - user *storemocks.MockBridgeUser - client *pmapimocks.MockClient - clientManager *storemocks.MockClientManager - panicHandler *storemocks.MockPanicHandler - store *Store + ctrl *gomock.Controller + events *storemocks.MockListener + user *storemocks.MockBridgeUser + client *pmapimocks.MockClient + clientManager *storemocks.MockClientManager + panicHandler *storemocks.MockPanicHandler + changeNotifier *storemocks.MockChangeNotifier + store *Store tmpDir string cache *Cache @@ -59,13 +60,14 @@ type mocksForStore struct { func initMocks(tb testing.TB) (*mocksForStore, func()) { ctrl := gomock.NewController(tb) mocks := &mocksForStore{ - tb: tb, - ctrl: ctrl, - events: storemocks.NewMockListener(ctrl), - user: storemocks.NewMockBridgeUser(ctrl), - client: pmapimocks.NewMockClient(ctrl), - clientManager: storemocks.NewMockClientManager(ctrl), - panicHandler: storemocks.NewMockPanicHandler(ctrl), + tb: tb, + ctrl: ctrl, + events: storemocks.NewMockListener(ctrl), + user: storemocks.NewMockBridgeUser(ctrl), + client: pmapimocks.NewMockClient(ctrl), + clientManager: storemocks.NewMockClientManager(ctrl), + panicHandler: storemocks.NewMockPanicHandler(ctrl), + changeNotifier: storemocks.NewMockChangeNotifier(ctrl), } // Called during clean-up. diff --git a/internal/users/user.go b/internal/users/user.go index b1e24e57..b4d6abde 100644 --- a/internal/users/user.go +++ b/internal/users/user.go @@ -27,7 +27,6 @@ import ( "github.com/ProtonMail/proton-bridge/internal/users/credentials" "github.com/ProtonMail/proton-bridge/pkg/listener" "github.com/ProtonMail/proton-bridge/pkg/pmapi" - imapBackend "github.com/emersion/go-imap/backend" "github.com/pkg/errors" "github.com/sirupsen/logrus" ) @@ -43,8 +42,6 @@ type User struct { clientManager ClientManager credStorer CredentialsStorer - imapUpdatesChannel chan imapBackend.Update - storeFactory StoreMaker store *store.Store @@ -95,7 +92,7 @@ func (u *User) client() pmapi.Client { // have the apitoken and password), authorising the user against the api, loading the user store (creating a new one // if necessary), and setting the imap idle updates channel (used to send imap idle updates to the imap backend if // something in the store changed). -func (u *User) init(idleUpdates chan imapBackend.Update) (err error) { +func (u *User) init() (err error) { u.log.Info("Initialising user") // Reload the user's credentials (if they log out and back in we need the new @@ -134,20 +131,9 @@ func (u *User) init(idleUpdates chan imapBackend.Update) (err error) { } u.store = store - // Save the imap updates channel here so it can be set later when imap connects. - u.imapUpdatesChannel = idleUpdates - return err } -func (u *User) SetIMAPIdleUpdateChannel() { - if u.store == nil { - return - } - - u.store.SetIMAPUpdateChannel(u.imapUpdatesChannel) -} - // authorizeIfNecessary checks whether user is logged in and is connected to api auth channel. // If user is not already connected to the api auth channel (for example there was no internet during start), // it tries to connect it. @@ -539,7 +525,7 @@ func (u *User) CloseAllConnections() { } if u.store != nil { - u.store.SetIMAPUpdateChannel(nil) + u.store.SetChangeNotifier(nil) } } diff --git a/internal/users/user_credentials_test.go b/internal/users/user_credentials_test.go index 5a2eda19..9ee6a73f 100644 --- a/internal/users/user_credentials_test.go +++ b/internal/users/user_credentials_test.go @@ -221,7 +221,7 @@ func TestCheckBridgeLoginLoggedOut(t *testing.T) { m.pmapiClient.EXPECT().Addresses().Return(nil), ) - err = user.init(nil) + err = user.init() assert.Error(t, err) defer cleanUpUserData(user) diff --git a/internal/users/user_new_test.go b/internal/users/user_new_test.go index 7bac94f5..e4bb8116 100644 --- a/internal/users/user_new_test.go +++ b/internal/users/user_new_test.go @@ -139,7 +139,7 @@ func checkNewUserHasCredentials(creds *credentials.Credentials, m mocks) { user, _ := newUser(m.PanicHandler, "user", m.eventListener, m.credentialsStore, m.clientManager, m.storeMaker) defer cleanUpUserData(user) - _ = user.init(nil) + _ = user.init() waitForEvents() diff --git a/internal/users/user_test.go b/internal/users/user_test.go index 6d5285e3..cba28dd8 100644 --- a/internal/users/user_test.go +++ b/internal/users/user_test.go @@ -34,7 +34,7 @@ func testNewUser(m mocks) *User { user, err := newUser(m.PanicHandler, "user", m.eventListener, m.credentialsStore, m.clientManager, m.storeMaker) assert.NoError(m.t, err) - err = user.init(nil) + err = user.init() assert.NoError(m.t, err) mockAuthUpdate(user, "reftok", m) @@ -51,7 +51,7 @@ func testNewUserForLogout(m mocks) *User { user, err := newUser(m.PanicHandler, "user", m.eventListener, m.credentialsStore, m.clientManager, m.storeMaker) assert.NoError(m.t, err) - err = user.init(nil) + err = user.init() assert.NoError(m.t, err) return user diff --git a/internal/users/users.go b/internal/users/users.go index 7cf4a318..a26e1399 100644 --- a/internal/users/users.go +++ b/internal/users/users.go @@ -26,7 +26,6 @@ import ( "github.com/ProtonMail/proton-bridge/internal/metrics" "github.com/ProtonMail/proton-bridge/pkg/listener" "github.com/ProtonMail/proton-bridge/pkg/pmapi" - imapBackend "github.com/emersion/go-imap/backend" "github.com/hashicorp/go-multierror" "github.com/pkg/errors" logrus "github.com/sirupsen/logrus" @@ -58,11 +57,6 @@ type Users struct { // as is, without requesting server again. useOnlyActiveAddresses bool - // idleUpdates is a channel which the imap backend listens to and which it - // uses to send idle updates to the mail client (eg thunderbird). - // The user stores should send idle updates on this channel. - idleUpdates chan imapBackend.Update - lock sync.RWMutex // stopAll can be closed to stop all goroutines from looping (watchAppOutdated, watchAPIAuths, heartbeat etc). @@ -88,7 +82,6 @@ func New( credStorer: credStorer, storeFactory: storeFactory, useOnlyActiveAddresses: useOnlyActiveAddresses, - idleUpdates: make(chan imapBackend.Update), lock: sync.RWMutex{}, stopAll: make(chan struct{}), } @@ -132,7 +125,7 @@ func (u *Users) loadUsersFromCredentialsStore() (err error) { u.users = append(u.users, user) - if initUserErr := user.init(u.idleUpdates); initUserErr != nil { + if initUserErr := user.init(); initUserErr != nil { l.WithField("user", userID).WithError(initUserErr).Warn("Could not initialise user") } } @@ -285,7 +278,7 @@ func (u *Users) connectExistingUser(user *User, auth *pmapi.Auth, hashedPassphra return errors.Wrap(err, "failed to update token of user in credentials store") } - if err = user.init(u.idleUpdates); err != nil { + if err = user.init(); err != nil { return errors.Wrap(err, "failed to initialise user") } @@ -326,7 +319,7 @@ func (u *Users) addNewUser(apiUser *pmapi.User, auth *pmapi.Auth, hashedPassphra // The user needs to be part of the users list in order for it to receive an auth during initialisation. u.users = append(u.users, user) - if err = user.init(u.idleUpdates); err != nil { + if err = user.init(); err != nil { u.users = u.users[:len(u.users)-1] return errors.Wrap(err, "failed to initialise user") } @@ -464,15 +457,6 @@ func (u *Users) SendMetric(m metrics.Metric) { }).Debug("Metric successfully sent") } -// GetIMAPUpdatesChannel sets the channel on which idle events should be sent. -func (u *Users) GetIMAPUpdatesChannel() chan imapBackend.Update { - if u.idleUpdates == nil { - log.Warn("IMAP updates channel is nil") - } - - return u.idleUpdates -} - // AllowProxy instructs the app to use DoH to access an API proxy if necessary. // It also needs to work before the app is initialised (because we may need to use the proxy at startup). func (u *Users) AllowProxy() { diff --git a/test/features/bridge/imap/message/delete.feature b/test/features/bridge/imap/message/delete.feature index 0d138fcc..3c0e204c 100644 --- a/test/features/bridge/imap/message/delete.feature +++ b/test/features/bridge/imap/message/delete.feature @@ -4,7 +4,6 @@ Feature: IMAP remove messages from mailbox And there is "user" with mailbox "Folders/mbox" And there is "user" with mailbox "Labels/label" - @ignore Scenario Outline: Mark message as deleted and EXPUNGE Given there are 10 messages in mailbox "" for "user" And there is IMAP client logged in as "user" @@ -27,7 +26,6 @@ Feature: IMAP remove messages from mailbox | Spam | | Trash | - @ignore Scenario Outline: Mark all messages as deleted and EXPUNGE Given there are 5 messages in mailbox "" for "user" And there is IMAP client logged in as "user" @@ -51,7 +49,6 @@ Feature: IMAP remove messages from mailbox | Spam | | Trash | - @ignore Scenario Outline: Mark messages as undeleted and EXPUNGE Given there are 5 messages in mailbox "" for "user" And there is IMAP client logged in as "user" @@ -74,7 +71,6 @@ Feature: IMAP remove messages from mailbox | Spam | | Trash | - @ignore Scenario Outline: Mark message as deleted and leave mailbox Given there are 10 messages in mailbox "INBOX" for "user" And there is IMAP client logged in as "user" @@ -97,7 +93,6 @@ Feature: IMAP remove messages from mailbox | LOGOUT | 9 | | UNSELECT | 10 | - @ignore Scenario: Not possible to delete from All Mail Given there are 1 messages in mailbox "INBOX" for "user" And there is IMAP client logged in as "user" diff --git a/unreleased.md b/unreleased.md index 214e18d0..5686e88a 100644 --- a/unreleased.md +++ b/unreleased.md @@ -9,6 +9,7 @@ Changelog [format](http://keepachangelog.com/en/1.0.0/) ### Changed * GODT-893 Bump go-rfc5322 dependency to v0.2.1 to properly detect syntax errors during parsing. * GODT-892 Swap type and value from sentry exception and cut panic handlers from the traceback. +* GODT-854 EXPUNGE and FETCH unilateral responses are returned before OK EXPUNGE or OK STORE, respectively. ### Removed * GODT-651 Build creates proper binary names.