From 94703bcf378e02fe523ea8f2f3c20eb157f1ed78 Mon Sep 17 00:00:00 2001 From: James Houlahan Date: Fri, 20 Jan 2023 14:14:35 +0100 Subject: [PATCH] GODT-2223: Treat label/message deletion errors as non-critical --- go.mod | 2 +- go.sum | 2 + internal/bridge/sync_test.go | 12 ++ internal/bridge/user_event_test.go | 169 +++++++++++++++++++++++++++++ internal/bridge/user_test.go | 59 ---------- internal/user/events.go | 14 +-- 6 files changed, 189 insertions(+), 69 deletions(-) create mode 100644 internal/bridge/user_event_test.go diff --git a/go.mod b/go.mod index 6abe7051..089ccd44 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.18 require ( github.com/0xAX/notificator v0.0.0-20220220101646-ee9b8921e557 github.com/Masterminds/semver/v3 v3.1.1 - github.com/ProtonMail/gluon v0.14.2-0.20230118120413-542c2bf244a0 + github.com/ProtonMail/gluon v0.14.2-0.20230123154940-b7793a0c0bd4 github.com/ProtonMail/go-autostart v0.0.0-20210130080809-00ed301c8e9a github.com/ProtonMail/go-proton-api v0.3.1-0.20230118091111-93ad9245e8ee github.com/ProtonMail/go-rfc5322 v0.11.0 diff --git a/go.sum b/go.sum index d1de9bfd..a5bb1547 100644 --- a/go.sum +++ b/go.sum @@ -30,6 +30,8 @@ github.com/ProtonMail/docker-credential-helpers v1.1.0 h1:+kvUIpwWcbtP3WFv5sSvkF github.com/ProtonMail/docker-credential-helpers v1.1.0/go.mod h1:mK0aBveCxhnQ756AmaTfXMZDeULvheYVhF/MWMErN5g= github.com/ProtonMail/gluon v0.14.2-0.20230118120413-542c2bf244a0 h1:T9wtn35Mqy4Mkr707iECwqo+FhbTVOI9PlRozR5wCO8= github.com/ProtonMail/gluon v0.14.2-0.20230118120413-542c2bf244a0/go.mod h1:z2AxLIiBCT1K+0OBHyaDI7AEaO5qI6/BEC2TE42vs4Q= +github.com/ProtonMail/gluon v0.14.2-0.20230123154940-b7793a0c0bd4 h1:AkRcjX1iArf8fVL4vZd6eaBjE3+SFnwZQKsH3OMyExU= +github.com/ProtonMail/gluon v0.14.2-0.20230123154940-b7793a0c0bd4/go.mod h1:z2AxLIiBCT1K+0OBHyaDI7AEaO5qI6/BEC2TE42vs4Q= github.com/ProtonMail/go-autostart v0.0.0-20210130080809-00ed301c8e9a h1:D+aZah+k14Gn6kmL7eKxoo/4Dr/lK3ChBcwce2+SQP4= github.com/ProtonMail/go-autostart v0.0.0-20210130080809-00ed301c8e9a/go.mod h1:oTGdE7/DlWIr23G0IKW3OXK9wZ5Hw1GGiaJFccTvZi4= github.com/ProtonMail/go-crypto v0.0.0-20210428141323-04723f9f07d7/go.mod h1:z4/9nQmJSSwwds7ejkxaJwO37dru3geImFUdJlaLzQo= diff --git a/internal/bridge/sync_test.go b/internal/bridge/sync_test.go index fa82a338..357cbab1 100644 --- a/internal/bridge/sync_test.go +++ b/internal/bridge/sync_test.go @@ -393,6 +393,18 @@ func clientStore(client *client.Client, from, to int, isUID bool, item imap.Stor ) } +func clientList(client *client.Client) []*imap.MailboxInfo { + resCh := make(chan *imap.MailboxInfo) + + go func() { + if err := client.List("", "*", resCh); err != nil { + panic(err) + } + }() + + return iterator.Collect(iterator.Chan(resCh)) +} + func createNumMessages(ctx context.Context, t *testing.T, c *proton.Client, addrID, labelID string, count int) []string { literal, err := os.ReadFile(filepath.Join("testdata", "text-plain.eml")) require.NoError(t, err) diff --git a/internal/bridge/user_event_test.go b/internal/bridge/user_event_test.go new file mode 100644 index 00000000..5359c851 --- /dev/null +++ b/internal/bridge/user_event_test.go @@ -0,0 +1,169 @@ +// Copyright (c) 2023 Proton AG +// +// This file is part of Proton Mail Bridge. +// +// Proton Mail 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. +// +// Proton Mail 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 Proton Mail Bridge. If not, see . + +package bridge_test + +import ( + "context" + "fmt" + "net/http" + "testing" + "time" + + "github.com/ProtonMail/go-proton-api" + "github.com/ProtonMail/go-proton-api/server" + "github.com/ProtonMail/proton-bridge/v3/internal/bridge" + "github.com/ProtonMail/proton-bridge/v3/internal/constants" + "github.com/ProtonMail/proton-bridge/v3/internal/events" + "github.com/bradenaw/juniper/xslices" + "github.com/emersion/go-imap" + "github.com/emersion/go-imap/client" + "github.com/golang/mock/gomock" + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +func TestBridge_User_BadEvents(t *testing.T) { + withEnv(t, func(ctx context.Context, s *server.Server, netCtl *proton.NetCtl, locator bridge.Locator, storeKey []byte) { + // Create a user. + userID, addrID, err := s.CreateUser("user", password) + require.NoError(t, err) + + labelID, err := s.CreateLabel(userID, "folder", "", proton.LabelTypeFolder) + require.NoError(t, err) + + // Create 10 messages for the user. + withClient(ctx, t, s, "user", password, func(ctx context.Context, c *proton.Client) { + createNumMessages(ctx, t, c, addrID, labelID, 10) + }) + + // The initial user should be fully synced. + withBridge(ctx, t, s.GetHostURL(), netCtl, locator, storeKey, func(bridge *bridge.Bridge, _ *bridge.Mocks) { + syncCh, done := chToType[events.Event, events.SyncFinished](bridge.GetEvents(events.SyncFinished{})) + defer done() + + userID, err := bridge.LoginFull(ctx, "user", password, nil, nil) + require.NoError(t, err) + + require.Equal(t, userID, (<-syncCh).UserID) + }) + + var messageIDs []string + + // Create 10 more messages for the user, generating events. + withClient(ctx, t, s, "user", password, func(ctx context.Context, c *proton.Client) { + messageIDs = createNumMessages(ctx, t, c, addrID, labelID, 10) + }) + + // If bridge attempts to sync the new messages, it should get a BadRequest error. + s.AddStatusHook(func(req *http.Request) (int, bool) { + if xslices.Index(xslices.Map(messageIDs, func(messageID string) string { + return "/mail/v4/messages/" + messageID + }), req.URL.Path) < 0 { + return 0, false + } + + return http.StatusBadRequest, true + }) + + // The user will continue to process events and will receive bad request errors. + withMocks(t, func(mocks *bridge.Mocks) { + mocks.Reporter.EXPECT().ReportMessageWithContext(gomock.Any(), gomock.Any()).MinTimes(1) + + // The user will eventually be logged out due to the bad request errors. + withBridgeNoMocks(ctx, t, mocks, s.GetHostURL(), netCtl, locator, storeKey, func(bridge *bridge.Bridge) { + require.Eventually(t, func() bool { + return len(bridge.GetUserIDs()) == 1 && len(getConnectedUserIDs(t, bridge)) == 0 + }, 10*time.Second, 100*time.Millisecond) + }) + }) + }) +} + +func TestBridge_User_BadEvents_MessageLabelDeleted(t *testing.T) { + withEnv(t, func(ctx context.Context, s *server.Server, netCtl *proton.NetCtl, locator bridge.Locator, storeKey []byte) { + // Create a user. + userID, addrID, err := s.CreateUser("user", password) + require.NoError(t, err) + + labelID, err := s.CreateLabel(userID, "folder", "", proton.LabelTypeFolder) + require.NoError(t, err) + + // Create 10 messages for the user. + withClient(ctx, t, s, "user", password, func(ctx context.Context, c *proton.Client) { + createNumMessages(ctx, t, c, addrID, labelID, 10) + }) + + // The initial user should be fully synced. + withBridge(ctx, t, s.GetHostURL(), netCtl, locator, storeKey, func(bridge *bridge.Bridge, _ *bridge.Mocks) { + syncCh, done := chToType[events.Event, events.SyncFinished](bridge.GetEvents(events.SyncFinished{})) + defer done() + + userID, err := bridge.LoginFull(ctx, "user", password, nil, nil) + require.NoError(t, err) + + require.Equal(t, userID, (<-syncCh).UserID) + }) + + // Create and delete 10 more messages for the user, generating delete events. + withClient(ctx, t, s, "user", password, func(ctx context.Context, c *proton.Client) { + messageIDs := createNumMessages(ctx, t, c, addrID, labelID, 10) + require.NoError(t, c.DeleteMessage(ctx, messageIDs...)) + }) + + // Create and delete 10 labels for the user, generating delete events. + withClient(ctx, t, s, "user", password, func(ctx context.Context, c *proton.Client) { + for i := 0; i < 10; i++ { + label, err := c.CreateLabel(ctx, proton.CreateLabelReq{ + Name: uuid.NewString(), + Color: "#f66", + Type: proton.LabelTypeLabel, + }) + require.NoError(t, err) + + require.NoError(t, c.DeleteLabel(ctx, label.ID)) + } + }) + + // The user will continue to process events and will not receive any bad request errors. + withBridge(ctx, t, s.GetHostURL(), netCtl, locator, storeKey, func(bridge *bridge.Bridge, _ *bridge.Mocks) { + info, err := bridge.QueryUserInfo("user") + require.NoError(t, err) + + client, err := client.Dial(fmt.Sprintf("%v:%v", constants.Host, bridge.GetIMAPPort())) + require.NoError(t, err) + require.NoError(t, client.Login(info.Addresses[0], string(info.BridgePass))) + defer func() { _ = client.Logout() }() + + // Create a new label. + withClient(ctx, t, s, "user", password, func(ctx context.Context, c *proton.Client) { + require.NoError(t, getErr(c.CreateLabel(ctx, proton.CreateLabelReq{ + Name: "blabla", + Color: "#f66", + Type: proton.LabelTypeLabel, + }))) + }) + + // Wait for the label to be created. + require.Eventually(t, func() bool { + return xslices.IndexFunc(clientList(client), func(mailbox *imap.MailboxInfo) bool { + return mailbox.Name == "Labels/blabla" + }) >= 0 + }, 10*time.Second, 100*time.Millisecond) + }) + }) +} diff --git a/internal/bridge/user_test.go b/internal/bridge/user_test.go index 7fb317d3..4c70f3a9 100644 --- a/internal/bridge/user_test.go +++ b/internal/bridge/user_test.go @@ -20,7 +20,6 @@ package bridge_test import ( "context" "fmt" - "net/http" "testing" "time" @@ -30,7 +29,6 @@ import ( mocksPkg "github.com/ProtonMail/proton-bridge/v3/internal/bridge/mocks" "github.com/ProtonMail/proton-bridge/v3/internal/events" "github.com/ProtonMail/proton-bridge/v3/internal/vault" - "github.com/bradenaw/juniper/xslices" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" ) @@ -664,63 +662,6 @@ func TestBridge_User_Refresh(t *testing.T) { }) } -func TestBridge_User_BadEvents(t *testing.T) { - withEnv(t, func(ctx context.Context, s *server.Server, netCtl *proton.NetCtl, locator bridge.Locator, storeKey []byte) { - // Create a user. - userID, addrID, err := s.CreateUser("user", password) - require.NoError(t, err) - - labelID, err := s.CreateLabel(userID, "folder", "", proton.LabelTypeFolder) - require.NoError(t, err) - - // Create 10 messages for the user. - withClient(ctx, t, s, "user", password, func(ctx context.Context, c *proton.Client) { - createNumMessages(ctx, t, c, addrID, labelID, 10) - }) - - // The initial user should be fully synced. - withBridge(ctx, t, s.GetHostURL(), netCtl, locator, storeKey, func(bridge *bridge.Bridge, _ *bridge.Mocks) { - syncCh, done := chToType[events.Event, events.SyncFinished](bridge.GetEvents(events.SyncFinished{})) - defer done() - - userID, err := bridge.LoginFull(ctx, "user", password, nil, nil) - require.NoError(t, err) - - require.Equal(t, userID, (<-syncCh).UserID) - }) - - var messageIDs []string - - // Create 10 more messages for the user, generating events. - withClient(ctx, t, s, "user", password, func(ctx context.Context, c *proton.Client) { - messageIDs = createNumMessages(ctx, t, c, addrID, labelID, 10) - }) - - // If bridge attempts to sync the new messages, it should get a BadRequest error. - s.AddStatusHook(func(req *http.Request) (int, bool) { - if xslices.Index(xslices.Map(messageIDs, func(messageID string) string { - return "/mail/v4/messages/" + messageID - }), req.URL.Path) < 0 { - return 0, false - } - - return http.StatusBadRequest, true - }) - - // The user will continue to process events and will receive bad request errors. - withMocks(t, func(mocks *bridge.Mocks) { - mocks.Reporter.EXPECT().ReportMessageWithContext(gomock.Any(), gomock.Any()).MinTimes(1) - - // The user will eventually be logged out due to the bad request errors. - withBridgeNoMocks(ctx, t, mocks, s.GetHostURL(), netCtl, locator, storeKey, func(bridge *bridge.Bridge) { - require.Eventually(t, func() bool { - return len(bridge.GetUserIDs()) == 1 && len(getConnectedUserIDs(t, bridge)) == 0 - }, 10*time.Second, 100*time.Millisecond) - }) - }) - }) -} - // getErr returns the error that was passed to it. func getErr[T any](val T, err error) error { return err diff --git a/internal/user/events.go b/internal/user/events.go index 2f2e3af7..1bc375e7 100644 --- a/internal/user/events.go +++ b/internal/user/events.go @@ -44,9 +44,7 @@ func (user *User) handleAPIEvent(ctx context.Context, event proton.Event) error } if event.User != nil { - if err := user.handleUserEvent(ctx, *event.User); err != nil { - return err - } + user.handleUserEvent(ctx, *event.User) } if len(event.Addresses) > 0 { @@ -133,8 +131,8 @@ func (user *User) handleRefreshEvent(ctx context.Context, refresh proton.Refresh } // handleUserEvent handles the given user event. -func (user *User) handleUserEvent(_ context.Context, userEvent proton.User) error { - return safe.LockRet(func() error { +func (user *User) handleUserEvent(_ context.Context, userEvent proton.User) { + safe.Lock(func() { user.log.WithFields(logrus.Fields{ "userID": userEvent.ID, "username": logging.Sensitive(userEvent.Name), @@ -145,8 +143,6 @@ func (user *User) handleUserEvent(_ context.Context, userEvent proton.User) erro user.eventCh.Enqueue(events.UserChanged{ UserID: user.apiUser.ID, }) - - return nil }, user.apiUserLock) } @@ -322,7 +318,7 @@ func (user *User) handleLabelEvents(ctx context.Context, labelEvents []proton.La } if err := waitOnIMAPUpdates(ctx, updates); err != nil { - return err + return fmt.Errorf("failed to handle delete label event in gluon: %w", err) } } } @@ -497,7 +493,7 @@ func (user *User) handleMessageEvents(ctx context.Context, messageEvents []proto } if err := waitOnIMAPUpdates(ctx, updates); err != nil { - return err + return fmt.Errorf("failed to handle delete message event in gluon: %w", err) } } }