From cb8a15a9fd3bdfb5cd092461d25b5a58b7a841d9 Mon Sep 17 00:00:00 2001 From: James Houlahan Date: Tue, 19 May 2020 09:04:30 +0200 Subject: [PATCH] fix: crash when removing account while messages are being returned --- Changelog.md | 5 ++++- internal/imap/mailbox_message.go | 21 ++++++++++++++++---- internal/imap/user.go | 9 +++++---- internal/smtp/user.go | 34 ++++++++++++++++---------------- pkg/pmapi/addresses.go | 17 ++++++++++------ pkg/pmapi/client.go | 2 +- pkg/pmapi/mocks/mocks.go | 5 +++-- test/fakeapi/user.go | 4 ++-- 8 files changed, 60 insertions(+), 37 deletions(-) diff --git a/Changelog.md b/Changelog.md index 71934666..f5314b14 100644 --- a/Changelog.md +++ b/Changelog.md @@ -3,13 +3,16 @@ Changelog [format](http://keepachangelog.com/en/1.0.0/) ## Unreleased -## unreleased ### Changed * GODT-386 renamed bridge to general users and keep bridge only for bridge stuff * GODT-308 better user error message when request is canceled * GODT-312 validate recipient emails in send before asking for their public keys +### Fixed +* GODT-356 Fix crash when removing account while mail client is fetching messages (regression from GODT-204) + + ## [v1.2.7] Donghai-hotfix - beta (2020-05-07) ### Added diff --git a/internal/imap/mailbox_message.go b/internal/imap/mailbox_message.go index 16af76b9..9e4646b0 100644 --- a/internal/imap/mailbox_message.go +++ b/internal/imap/mailbox_message.go @@ -548,7 +548,11 @@ func (im *imapMailbox) writeMessageBody(w io.Writer, m *pmapi.Message) (err erro } } - kr := im.user.client.KeyRingForAddressID(m.AddressID) + kr, err := im.user.client().KeyRingForAddressID(m.AddressID) + if err != nil { + return errors.Wrap(err, "failed to get keyring for address ID") + } + err = message.WriteBody(w, kr, m) if err != nil { if customMessageErr := im.makeCustomMessage(m, err, true); customMessageErr != nil { @@ -574,13 +578,17 @@ func (im *imapMailbox) writeAndParseMIMEBody(m *pmapi.Message) (mime *enmime.Env func (im *imapMailbox) writeAttachmentBody(w io.Writer, m *pmapi.Message, att *pmapi.Attachment) (err error) { // Retrieve encrypted attachment. - r, err := im.user.client.GetAttachment(att.ID) + r, err := im.user.client().GetAttachment(att.ID) if err != nil { return } defer r.Close() //nolint[errcheck] - kr := im.user.client.KeyRingForAddressID(m.AddressID) + kr, err := im.user.client().KeyRingForAddressID(m.AddressID) + if err != nil { + return errors.Wrap(err, "failed to get keyring for address ID") + } + if err = message.WriteAttachmentBody(w, kr, m, att, r); err != nil { // Returning an error here makes certain mail clients behave badly, // trying to retrieve the message again and again. @@ -674,7 +682,12 @@ func (im *imapMailbox) buildMessage(m *pmapi.Message) (structure *message.BodySt return } - kr := im.user.client.KeyRingForAddressID(m.AddressID) + kr, err := im.user.client().KeyRingForAddressID(m.AddressID) + if err != nil { + err = errors.Wrap(err, "failed to get keyring for address ID") + return + } + errDecrypt := m.Decrypt(kr) if errDecrypt != nil && errDecrypt != openpgperrors.ErrSignatureExpired { diff --git a/internal/imap/user.go b/internal/imap/user.go index 2b13c1a5..b4e03b12 100644 --- a/internal/imap/user.go +++ b/internal/imap/user.go @@ -34,7 +34,6 @@ type imapUser struct { panicHandler panicHandler backend *imapBackend user bridgeUser - client pmapi.Client storeUser storeUserProvider storeAddress storeAddressProvider @@ -42,6 +41,11 @@ type imapUser struct { currentAddressLowercase string } +// This method should eventually no longer be necessary. Everything should go via store. +func (iu *imapUser) client() pmapi.Client { + return iu.user.GetTemporaryPMAPIClient() +} + // newIMAPUser returns struct implementing go-imap/user interface. func newIMAPUser( panicHandler panicHandler, @@ -62,13 +66,10 @@ func newIMAPUser( return nil, err } - client := user.GetTemporaryPMAPIClient() - return &imapUser{ panicHandler: panicHandler, backend: backend, user: user, - client: client, storeUser: storeUser, storeAddress: storeAddress, diff --git a/internal/smtp/user.go b/internal/smtp/user.go index 208a7588..6eb4fc14 100644 --- a/internal/smtp/user.go +++ b/internal/smtp/user.go @@ -46,7 +46,6 @@ type smtpUser struct { eventListener listener.Listener backend *smtpBackend user bridgeUser - client pmapi.Client storeUser storeUserProvider addressID string } @@ -59,9 +58,6 @@ func newSMTPUser( user bridgeUser, addressID string, ) (goSMTPBackend.User, error) { - // Using client directly is deprecated. Code should be moved to store. - client := user.GetTemporaryPMAPIClient() - storeUser := user.GetStore() if storeUser == nil { return nil, errors.New("user database is not initialized") @@ -72,23 +68,27 @@ func newSMTPUser( eventListener: eventListener, backend: smtpBackend, user: user, - client: client, storeUser: storeUser, addressID: addressID, }, nil } +// This method should eventually no longer be necessary. Everything should go via store. +func (su *smtpUser) client() pmapi.Client { + return su.user.GetTemporaryPMAPIClient() +} + // Send sends an email from the given address to the given addresses with the given body. func (su *smtpUser) Send(from string, to []string, messageReader io.Reader) (err error) { //nolint[funlen] // Called from go-smtp in goroutines - we need to handle panics for each function. defer su.panicHandler.HandlePanic() - mailSettings, err := su.client.GetMailSettings() + mailSettings, err := su.client().GetMailSettings() if err != nil { return err } - var addr *pmapi.Address = su.client.Addresses().ByEmail(from) + var addr *pmapi.Address = su.client().Addresses().ByEmail(from) if addr == nil { err = errors.New("backend: invalid email address: not owned by user") return @@ -138,11 +138,11 @@ func (su *smtpUser) Send(from string, to []string, messageReader io.Reader) (err // but it's better than sending the message many times. If the message was sent, we simply return // nil to indicate it's OK. sendRecorderMessageHash := su.backend.sendRecorder.getMessageHash(message) - isSending, wasSent := su.backend.sendRecorder.isSendingOrSent(su.client, sendRecorderMessageHash) + isSending, wasSent := su.backend.sendRecorder.isSendingOrSent(su.client(), sendRecorderMessageHash) if isSending { log.Debug("Message is in send queue, waiting") time.Sleep(60 * time.Second) - isSending, wasSent = su.backend.sendRecorder.isSendingOrSent(su.client, sendRecorderMessageHash) + isSending, wasSent = su.backend.sendRecorder.isSendingOrSent(su.client(), sendRecorderMessageHash) } if isSending { log.Debug("Message is still in send queue, returning error") @@ -164,7 +164,7 @@ func (su *smtpUser) Send(from string, to []string, messageReader io.Reader) (err // can lead to sending the wrong message. Also clients do not necessarily // delete the old draft. if draftID != "" { - if err := su.client.DeleteMessages([]string{draftID}); err != nil { + if err := su.client().DeleteMessages([]string{draftID}); err != nil { log.WithError(err).WithField("draftID", draftID).Warn("Original draft cannot be deleted") } } @@ -214,7 +214,7 @@ func (su *smtpUser) Send(from string, to []string, messageReader io.Reader) (err } // PMEL 1. - contactEmails, err := su.client.GetContactEmailByEmail(email, 0, 1000) + contactEmails, err := su.client().GetContactEmailByEmail(email, 0, 1000) if err != nil { return err } @@ -224,11 +224,11 @@ func (su *smtpUser) Send(from string, to []string, messageReader io.Reader) (err if contactEmail.Defaults == 1 { // WARNING: in doc it says _ignore for now, future feature_ continue } - contact, err := su.client.GetContactByID(contactEmail.ContactID) + contact, err := su.client().GetContactByID(contactEmail.ContactID) if err != nil { return err } - decryptedCards, err := su.client.DecryptAndVerifyCards(contact.Cards) + decryptedCards, err := su.client().DecryptAndVerifyCards(contact.Cards) if err != nil { return err } @@ -248,7 +248,7 @@ func (su *smtpUser) Send(from string, to []string, messageReader io.Reader) (err } // PMEL 4. - apiRawKeyList, isInternal, err := su.client.GetPublicKeysForEmail(email) + apiRawKeyList, isInternal, err := su.client().GetPublicKeysForEmail(email) if err != nil { err = fmt.Errorf("backend: cannot get recipients' public keys: %v", err) return err @@ -336,7 +336,7 @@ func (su *smtpUser) Send(from string, to []string, messageReader io.Reader) (err return errors.New("error decoding subject message " + message.Header.Get("Subject")) } if !su.continueSendingUnencryptedMail(subject) { - _ = su.client.DeleteMessages([]string{message.ID}) + _ = su.client().DeleteMessages([]string{message.ID}) return errors.New("sending was canceled by user") } } @@ -384,7 +384,7 @@ func (su *smtpUser) handleReferencesHeader(m *pmapi.Message) (draftID, parentID if su.addressID != "" { filter.AddressID = su.addressID } - metadata, _, _ := su.client.ListMessages(filter) + metadata, _, _ := su.client().ListMessages(filter) for _, m := range metadata { if m.IsDraft() { draftID = m.ID @@ -404,7 +404,7 @@ func (su *smtpUser) handleReferencesHeader(m *pmapi.Message) (draftID, parentID if su.addressID != "" { filter.AddressID = su.addressID } - metadata, _, _ := su.client.ListMessages(filter) + metadata, _, _ := su.client().ListMessages(filter) // There can be two or messages with the same external ID and then we cannot // be sure which message should be parent. Better to not choose any. if len(metadata) == 1 { diff --git a/pkg/pmapi/addresses.go b/pkg/pmapi/addresses.go index 0a826656..8042db08 100644 --- a/pkg/pmapi/addresses.go +++ b/pkg/pmapi/addresses.go @@ -18,6 +18,7 @@ package pmapi import ( + "errors" "fmt" "strings" @@ -116,7 +117,7 @@ func (l AddressList) Main() *Address { return addr } } - return l[0] // Should not happen. + return nil } // ByEmail gets an address by email. Returns nil if no address is found. @@ -218,10 +219,14 @@ func (c *client) UnlockAddresses(passphrase []byte) (err error) { return } -func (c *client) KeyRingForAddressID(addrID string) *pmcrypto.KeyRing { - addr := c.addresses.ByID(addrID) - if addr == nil { - addr = c.addresses.Main() +func (c *client) KeyRingForAddressID(addrID string) (*pmcrypto.KeyRing, error) { + if addr := c.addresses.ByID(addrID); addr != nil { + return addr.KeyRing(), nil } - return addr.KeyRing() + + if addr := c.addresses.Main(); addr != nil { + return addr.KeyRing(), nil + } + + return nil, errors.New("no such address ID") } diff --git a/pkg/pmapi/client.go b/pkg/pmapi/client.go index e68f2992..521dae41 100644 --- a/pkg/pmapi/client.go +++ b/pkg/pmapi/client.go @@ -157,7 +157,7 @@ type Client interface { CreateAttachment(att *Attachment, r io.Reader, sig io.Reader) (created *Attachment, err error) DeleteAttachment(attID string) (err error) - KeyRingForAddressID(string) (kr *pmcrypto.KeyRing) + KeyRingForAddressID(string) (kr *pmcrypto.KeyRing, err error) GetPublicKeysForEmail(string) ([]PublicKey, bool, error) } diff --git a/pkg/pmapi/mocks/mocks.go b/pkg/pmapi/mocks/mocks.go index 856af4bc..e6af7c32 100644 --- a/pkg/pmapi/mocks/mocks.go +++ b/pkg/pmapi/mocks/mocks.go @@ -433,11 +433,12 @@ func (mr *MockClientMockRecorder) IsConnected() *gomock.Call { } // KeyRingForAddressID mocks base method -func (m *MockClient) KeyRingForAddressID(arg0 string) *crypto.KeyRing { +func (m *MockClient) KeyRingForAddressID(arg0 string) (*crypto.KeyRing, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "KeyRingForAddressID", arg0) ret0, _ := ret[0].(*crypto.KeyRing) - return ret0 + ret1, _ := ret[1].(error) + return ret0, ret1 } // KeyRingForAddressID indicates an expected call of KeyRingForAddressID diff --git a/test/fakeapi/user.go b/test/fakeapi/user.go index 2b9c4b01..0ad105b1 100644 --- a/test/fakeapi/user.go +++ b/test/fakeapi/user.go @@ -84,8 +84,8 @@ func (api *FakePMAPI) Addresses() pmapi.AddressList { return *api.addresses } -func (api *FakePMAPI) KeyRingForAddressID(addrID string) *pmcrypto.KeyRing { +func (api *FakePMAPI) KeyRingForAddressID(addrID string) (*pmcrypto.KeyRing, error) { return &pmcrypto.KeyRing{ FirstKeyID: "key", - } + }, nil }