fix: crash when removing account while messages are being returned

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

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 {

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,

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 {

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 {
+func (c *client) KeyRingForAddressID(addrID string) (*pmcrypto.KeyRing, error) {
-	addr := c.addresses.ByID(addrID)
+	if addr := c.addresses.ByID(addrID); addr != nil {
-	if addr == nil {
+		return addr.KeyRing(), nil
-		addr = c.addresses.Main()
 	}
-	return addr.KeyRing()
+
+	if addr := c.addresses.Main(); addr != nil {
+		return addr.KeyRing(), nil
+	}
+
+	return nil, errors.New("no such address ID")
 }

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)
 }
 

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

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
 }