From 39f2362996a6862a362a9d996772d1ea48877a12 Mon Sep 17 00:00:00 2001 From: Atanas Janeshliev Date: Tue, 3 Jun 2025 17:30:30 +0200 Subject: [PATCH] feat(BRIDGE-379): mailbox pre-checker on startup & conflict resolver for bridge internal mailboxes; TODO potentially add this for system mailboxes as well --- internal/bridge/user_event_test.go | 4 + internal/services/imapservice/conflicts.go | 78 ++++++- .../services/imapservice/conflicts_test.go | 204 +++++++++++++++- internal/services/imapservice/labelchecker.go | 219 ++++++++++++++++++ internal/services/imapservice/service.go | 12 +- .../imapservice/service_label_events.go | 4 +- .../imapservice/sync_update_applier.go | 14 +- internal/services/syncservice/handler.go | 28 ++- internal/services/syncservice/handler_test.go | 9 +- internal/unleash/service.go | 1 + 10 files changed, 541 insertions(+), 32 deletions(-) create mode 100644 internal/services/imapservice/labelchecker.go diff --git a/internal/bridge/user_event_test.go b/internal/bridge/user_event_test.go index 51246ba7..0da076a9 100644 --- a/internal/bridge/user_event_test.go +++ b/internal/bridge/user_event_test.go @@ -23,6 +23,7 @@ import ( "net" "net/http" "net/mail" + "runtime" "strings" "sync/atomic" "testing" @@ -76,6 +77,9 @@ func TestBridge_User_RefreshEvent(t *testing.T) { withBridge(ctx, t, s.GetHostURL(), netCtl, locator, storeKey, func(bridge *bridge.Bridge, _ *bridge.Mocks) { syncCh, closeCh := chToType[events.Event, events.SyncFinished](bridge.GetEvents(events.SyncFinished{})) + if runtime.GOOS != "windows" { + require.Equal(t, userID, (<-syncCh).UserID) + } require.Equal(t, userID, (<-syncCh).UserID) closeCh() diff --git a/internal/services/imapservice/conflicts.go b/internal/services/imapservice/conflicts.go index f9ac6851..9c6eb2cf 100644 --- a/internal/services/imapservice/conflicts.go +++ b/internal/services/imapservice/conflicts.go @@ -86,42 +86,42 @@ func (m *LabelConflictManager) generateMailboxFetcher(connectors []*Connector) m } } -type LabelConflictResolver interface { +type UserLabelConflictResolver interface { ResolveConflict(ctx context.Context, label proton.Label, visited map[string]bool) (func() []imap.Update, error) } -type labelConflictResolverImpl struct { +type userLabelConflictResolverImpl struct { mailboxFetch mailboxFetcherFn client apiClient reporter sentryReporter log *logrus.Entry } -type nullLabelConflictResolverImpl struct { +type nullUserLabelConflictResolverImpl struct { } -func (r *nullLabelConflictResolverImpl) ResolveConflict(_ context.Context, _ proton.Label, _ map[string]bool) (func() []imap.Update, error) { +func (r *nullUserLabelConflictResolverImpl) ResolveConflict(_ context.Context, _ proton.Label, _ map[string]bool) (func() []imap.Update, error) { return func() []imap.Update { return []imap.Update{} }, nil } -func (m *LabelConflictManager) NewConflictResolver(connectors []*Connector) LabelConflictResolver { +func (m *LabelConflictManager) NewUserConflictResolver(connectors []*Connector) UserLabelConflictResolver { if m.featureFlagProvider.GetFlagValue(unleash.LabelConflictResolverDisabled) { - return &nullLabelConflictResolverImpl{} + return &nullUserLabelConflictResolverImpl{} } - return &labelConflictResolverImpl{ + return &userLabelConflictResolverImpl{ mailboxFetch: m.generateMailboxFetcher(connectors), client: m.client, reporter: m.reporter, log: logrus.WithFields(logrus.Fields{ - "pkg": "imapservice/labelConflictResolver", + "pkg": "imapservice/userLabelConflictResolver", "numberOfConnectors": len(connectors), }), } } -func (r *labelConflictResolverImpl) ResolveConflict(ctx context.Context, label proton.Label, visited map[string]bool) (func() []imap.Update, error) { +func (r *userLabelConflictResolverImpl) ResolveConflict(ctx context.Context, label proton.Label, visited map[string]bool) (func() []imap.Update, error) { logger := r.log.WithFields(logrus.Fields{ "labelID": label.ID, "labelPath": hashLabelPaths(GetMailboxName(label)), @@ -238,3 +238,63 @@ func compareLabelNames(labelName1, labelName2 []string) bool { func hashLabelPaths(path []string) string { return algo.HashBase64SHA256(strings.Join(path, "")) } + +type InternalLabelConflictResolver interface { + ResolveConflict(ctx context.Context) (func() []imap.Update, error) +} + +type internalLabelConflictResolverImpl struct { + mailboxFetch mailboxFetcherFn + client apiClient + reporter sentryReporter + log *logrus.Entry +} + +type nullInternalLabelConflictResolver struct{} + +func (r *nullInternalLabelConflictResolver) ResolveConflict(_ context.Context) (func() []imap.Update, error) { + return func() []imap.Update { return []imap.Update{} }, nil +} + +func (m *LabelConflictManager) NewInternalLabelConflictResolver(connectors []*Connector) InternalLabelConflictResolver { + if m.featureFlagProvider.GetFlagValue(unleash.InternalLabelConflictResolverDisabled) { + return &nullInternalLabelConflictResolver{} + } + + return &internalLabelConflictResolverImpl{ + mailboxFetch: m.generateMailboxFetcher(connectors), + client: m.client, + reporter: m.reporter, + log: logrus.WithFields(logrus.Fields{ + "pkg": "imapservice/internalLabelConflictResolver", + "numberOfConnectors": len(connectors), + }), + } +} + +func (r *internalLabelConflictResolverImpl) ResolveConflict(ctx context.Context) (func() []imap.Update, error) { + var updateFns []func() []imap.Update + + for _, prefix := range []string{folderPrefix, labelPrefix} { + label := proton.Label{ + Path: []string{prefix}, + ID: prefix, + Name: prefix, + } + + mbox, err := r.mailboxFetch(ctx, label) + if err != nil { + if db.IsErrNotFound(err) { + continue + } + return nil, err + } + + if mbox.RemoteID != label.ID { + // If the ID's don't match we should delete these. + fn := func() []imap.Update { return []imap.Update{imap.NewMailboxDeleted(imap.MailboxID(prefix))} } + updateFns = append(updateFns, fn) + } + } + return combineIMAPUpdateFns(updateFns), nil +} diff --git a/internal/services/imapservice/conflicts_test.go b/internal/services/imapservice/conflicts_test.go index 22115389..ad11c29f 100644 --- a/internal/services/imapservice/conflicts_test.go +++ b/internal/services/imapservice/conflicts_test.go @@ -121,7 +121,7 @@ func TestResolveConflict_UnexpectedLabelConflict(t *testing.T) { connector := &imapservice.Connector{} connector.SetAddrIDTest("addr-1") resolver := imapservice.NewLabelConflictManager(mockLabelProvider, mockIDProvider, mockClient, mockReporter, ffProviderFalse{}). - NewConflictResolver([]*imapservice.Connector{connector}) + NewUserConflictResolver([]*imapservice.Connector{connector}) visited := make(map[string]bool) _, err := resolver.ResolveConflict(ctx, label, visited) @@ -152,7 +152,7 @@ func TestResolveDiscrepancy_LabelDoesNotExist(t *testing.T) { connectors := []*imapservice.Connector{connector} manager := imapservice.NewLabelConflictManager(mockLabelProvider, mockIDProvider, mockClient, mockReporter, ffProviderFalse{}) - resolver := manager.NewConflictResolver(connectors) + resolver := manager.NewUserConflictResolver(connectors) visited := make(map[string]bool) fn, err := resolver.ResolveConflict(ctx, label, visited) @@ -185,7 +185,7 @@ func TestResolveConflict_MailboxFetchError(t *testing.T) { connector := &imapservice.Connector{} connector.SetAddrIDTest("addr-1") resolver := imapservice.NewLabelConflictManager(mockLabelProvider, mockIDProvider, mockClient, mockReporter, ffProviderFalse{}). - NewConflictResolver([]*imapservice.Connector{connector}) + NewUserConflictResolver([]*imapservice.Connector{connector}) visited := make(map[string]bool) _, err := resolver.ResolveConflict(ctx, label, visited) @@ -223,7 +223,7 @@ func TestResolveDiscrepancy_ConflictingLabelDeletedRemotely(t *testing.T) { connectors := []*imapservice.Connector{connector} manager := imapservice.NewLabelConflictManager(mockLabelProvider, mockIDProvider, mockClient, mockReporter, ffProviderFalse{}) - resolver := manager.NewConflictResolver(connectors) + resolver := manager.NewUserConflictResolver(connectors) visited := make(map[string]bool) fn, err := resolver.ResolveConflict(ctx, label, visited) @@ -266,7 +266,7 @@ func TestResolveDiscrepancy_LabelAlreadyCorrect(t *testing.T) { connectors := []*imapservice.Connector{connector} manager := imapservice.NewLabelConflictManager(mockLabelProvider, mockIDProvider, mockClient, mockReporter, ffProviderFalse{}) - resolver := manager.NewConflictResolver(connectors) + resolver := manager.NewUserConflictResolver(connectors) visited := make(map[string]bool) fn, err := resolver.ResolveConflict(ctx, label, visited) @@ -295,7 +295,7 @@ func TestResolveConflict_DeepNestedPath(t *testing.T) { connector := &imapservice.Connector{} connector.SetAddrIDTest("addr-1") resolver := imapservice.NewLabelConflictManager(mockLabelProvider, mockIDProvider, mockClient, mockReporter, ffProviderFalse{}). - NewConflictResolver([]*imapservice.Connector{connector}) + NewUserConflictResolver([]*imapservice.Connector{connector}) visited := make(map[string]bool) fn, err := resolver.ResolveConflict(ctx, label, visited) @@ -360,7 +360,7 @@ func TestResolveLabelDiscrepancy_LabelSwap(t *testing.T) { connectors := []*imapservice.Connector{connector} manager := imapservice.NewLabelConflictManager(mockLabelProvider, mockIDProvider, mockClient, mockReporter, ffProviderFalse{}) - resolver := manager.NewConflictResolver(connectors) + resolver := manager.NewUserConflictResolver(connectors) visited := make(map[string]bool) fn, err := resolver.ResolveConflict(context.Background(), apiLabels[0], visited) @@ -453,7 +453,7 @@ func TestResolveLabelDiscrepancy_LabelSwapExtended(t *testing.T) { connectors := []*imapservice.Connector{connector} manager := imapservice.NewLabelConflictManager(mockLabelProvider, mockIDProvider, mockClient, mockReporter, ffProviderFalse{}) - resolver := manager.NewConflictResolver(connectors) + resolver := manager.NewUserConflictResolver(connectors) fn, err := resolver.ResolveConflict(context.Background(), apiLabels[0], make(map[string]bool)) require.NoError(t, err) @@ -538,7 +538,7 @@ func TestResolveLabelDiscrepancy_LabelSwapCyclic(t *testing.T) { connectors := []*imapservice.Connector{connector} manager := imapservice.NewLabelConflictManager(mockLabelProvider, mockIDProvider, mockClient, mockReporter, ffProviderFalse{}) - resolver := manager.NewConflictResolver(connectors) + resolver := manager.NewUserConflictResolver(connectors) fn, err := resolver.ResolveConflict(context.Background(), apiLabels[0], make(map[string]bool)) require.NoError(t, err) @@ -612,7 +612,7 @@ func TestResolveLabelDiscrepancy_LabelSwapCyclicWithDeletedLabel(t *testing.T) { connectors := []*imapservice.Connector{connector} manager := imapservice.NewLabelConflictManager(mockLabelProvider, mockIDProvider, mockClient, mockReporter, ffProviderFalse{}) - resolver := manager.NewConflictResolver(connectors) + resolver := manager.NewUserConflictResolver(connectors) fn, err := resolver.ResolveConflict(context.Background(), apiLabels[2], make(map[string]bool)) require.NoError(t, err) @@ -675,7 +675,7 @@ func TestResolveLabelDiscrepancy_LabelSwapCyclicWithDeletedLabel_KillSwitchEnabl connectors := []*imapservice.Connector{connector} manager := imapservice.NewLabelConflictManager(mockLabelProvider, mockIDProvider, mockClient, mockReporter, ffProviderTrue{}) - resolver := manager.NewConflictResolver(connectors) + resolver := manager.NewUserConflictResolver(connectors) fn, err := resolver.ResolveConflict(context.Background(), apiLabels[2], make(map[string]bool)) require.NoError(t, err) @@ -683,3 +683,185 @@ func TestResolveLabelDiscrepancy_LabelSwapCyclicWithDeletedLabel_KillSwitchEnabl updates := fn() assert.Empty(t, updates) } + +func TestInternalLabelConflictResolver_NoConflicts(t *testing.T) { + ctx := context.Background() + + mockLabelProvider := new(mockLabelNameProvider) + mockClient := new(mockAPIClient) + mockIDProvider := new(mockIDProvider) + mockReporter := new(mockReporter) + + mockIDProvider.On("GetGluonID", "addr-1").Return("gluon-id-1", true) + + mockLabelProvider.On("GetUserMailboxByName", mock.Anything, "gluon-id-1", []string{"Folders"}). + Return(imap.MailboxData{}, db.ErrNotFound) + mockLabelProvider.On("GetUserMailboxByName", mock.Anything, "gluon-id-1", []string{"Labels"}). + Return(imap.MailboxData{}, db.ErrNotFound) + + connector := &imapservice.Connector{} + connector.SetAddrIDTest("addr-1") + connectors := []*imapservice.Connector{connector} + + manager := imapservice.NewLabelConflictManager(mockLabelProvider, mockIDProvider, mockClient, mockReporter, ffProviderFalse{}) + resolver := manager.NewInternalLabelConflictResolver(connectors) + + fn, err := resolver.ResolveConflict(ctx) + assert.NoError(t, err) + + updates := fn() + assert.Empty(t, updates) +} + +func TestInternalLabelConflictResolver_CorrectIDs(t *testing.T) { + ctx := context.Background() + + mockLabelProvider := new(mockLabelNameProvider) + mockClient := new(mockAPIClient) + mockIDProvider := new(mockIDProvider) + mockReporter := new(mockReporter) + + mockIDProvider.On("GetGluonID", "addr-1").Return("gluon-id-1", true) + + mockLabelProvider.On("GetUserMailboxByName", mock.Anything, "gluon-id-1", []string{"Folders"}). + Return(imap.MailboxData{RemoteID: "Folders", BridgeName: []string{"Folders"}}, nil) + mockLabelProvider.On("GetUserMailboxByName", mock.Anything, "gluon-id-1", []string{"Labels"}). + Return(imap.MailboxData{RemoteID: "Labels", BridgeName: []string{"Labels"}}, nil) + + connector := &imapservice.Connector{} + connector.SetAddrIDTest("addr-1") + connectors := []*imapservice.Connector{connector} + + manager := imapservice.NewLabelConflictManager(mockLabelProvider, mockIDProvider, mockClient, mockReporter, ffProviderFalse{}) + resolver := manager.NewInternalLabelConflictResolver(connectors) + + fn, err := resolver.ResolveConflict(ctx) + assert.NoError(t, err) + + updates := fn() + assert.Empty(t, updates) +} + +func TestInternalLabelConflictResolver_ConflictingFoldersID(t *testing.T) { + ctx := context.Background() + + mockLabelProvider := new(mockLabelNameProvider) + mockClient := new(mockAPIClient) + mockIDProvider := new(mockIDProvider) + mockReporter := new(mockReporter) + + mockIDProvider.On("GetGluonID", "addr-1").Return("gluon-id-1", true) + + mockLabelProvider.On("GetUserMailboxByName", mock.Anything, "gluon-id-1", []string{"Folders"}). + Return(imap.MailboxData{RemoteID: "wrong-id", BridgeName: []string{"Folders"}}, nil) + mockLabelProvider.On("GetUserMailboxByName", mock.Anything, "gluon-id-1", []string{"Labels"}). + Return(imap.MailboxData{}, db.ErrNotFound) + + connector := &imapservice.Connector{} + connector.SetAddrIDTest("addr-1") + connectors := []*imapservice.Connector{connector} + + manager := imapservice.NewLabelConflictManager(mockLabelProvider, mockIDProvider, mockClient, mockReporter, ffProviderFalse{}) + resolver := manager.NewInternalLabelConflictResolver(connectors) + + fn, err := resolver.ResolveConflict(ctx) + assert.NoError(t, err) + + updates := fn() + assert.Len(t, updates, 1) + + deleted, ok := updates[0].(*imap.MailboxDeleted) + assert.True(t, ok) + assert.Equal(t, imap.MailboxID("Folders"), deleted.MailboxID) +} + +func TestInternalLabelConflictResolver_BothConflicting(t *testing.T) { + ctx := context.Background() + + mockLabelProvider := new(mockLabelNameProvider) + mockClient := new(mockAPIClient) + mockIDProvider := new(mockIDProvider) + mockReporter := new(mockReporter) + + mockIDProvider.On("GetGluonID", "addr-1").Return("gluon-id-1", true) + + mockLabelProvider.On("GetUserMailboxByName", mock.Anything, "gluon-id-1", []string{"Folders"}). + Return(imap.MailboxData{RemoteID: "wrong-folders-id", BridgeName: []string{"Folders"}}, nil) + mockLabelProvider.On("GetUserMailboxByName", mock.Anything, "gluon-id-1", []string{"Labels"}). + Return(imap.MailboxData{RemoteID: "wrong-labels-id", BridgeName: []string{"Labels"}}, nil) + + connector := &imapservice.Connector{} + connector.SetAddrIDTest("addr-1") + connectors := []*imapservice.Connector{connector} + + manager := imapservice.NewLabelConflictManager(mockLabelProvider, mockIDProvider, mockClient, mockReporter, ffProviderFalse{}) + resolver := manager.NewInternalLabelConflictResolver(connectors) + + fn, err := resolver.ResolveConflict(ctx) + assert.NoError(t, err) + + updates := fn() + assert.Len(t, updates, 2) + + updateOne, ok := updates[0].(*imap.MailboxDeleted) + assert.True(t, ok) + assert.Equal(t, imap.MailboxID("Folders"), updateOne.MailboxID) + + updateTwo, ok := updates[1].(*imap.MailboxDeleted) + assert.True(t, ok) + assert.Equal(t, imap.MailboxID("Labels"), updateTwo.MailboxID) +} + +func TestInternalLabelConflictResolver_MailboxFetchError(t *testing.T) { + ctx := context.Background() + + mockLabelProvider := new(mockLabelNameProvider) + mockClient := new(mockAPIClient) + mockIDProvider := new(mockIDProvider) + mockReporter := new(mockReporter) + + mockIDProvider.On("GetGluonID", "addr-1").Return("gluon-id-1", true) + + mockLabelProvider.On("GetUserMailboxByName", mock.Anything, "gluon-id-1", []string{"Folders"}). + Return(imap.MailboxData{}, errors.New("database connection error")) + + connector := &imapservice.Connector{} + connector.SetAddrIDTest("addr-1") + connectors := []*imapservice.Connector{connector} + + manager := imapservice.NewLabelConflictManager(mockLabelProvider, mockIDProvider, mockClient, mockReporter, ffProviderFalse{}) + resolver := manager.NewInternalLabelConflictResolver(connectors) + + _, err := resolver.ResolveConflict(ctx) + assert.Error(t, err) + assert.Contains(t, err.Error(), "database connection error") +} + +func TestNewInternalLabelConflictResolver_KillSwitchEnabled(t *testing.T) { + ctx := context.Background() + + mockLabelProvider := new(mockLabelNameProvider) + mockClient := new(mockAPIClient) + mockIDProvider := new(mockIDProvider) + mockReporter := new(mockReporter) + + mockIDProvider.On("GetGluonID", "addr-1").Return("gluon-id-1", true) + + mockLabelProvider.On("GetUserMailboxByName", mock.Anything, "gluon-id-1", []string{"Folders"}). + Return(imap.MailboxData{RemoteID: "wrong-folders-id", BridgeName: []string{"Folders"}}, nil) + mockLabelProvider.On("GetUserMailboxByName", mock.Anything, "gluon-id-1", []string{"Labels"}). + Return(imap.MailboxData{RemoteID: "wrong-labels-id", BridgeName: []string{"Labels"}}, nil) + + connector := &imapservice.Connector{} + connector.SetAddrIDTest("addr-1") + connectors := []*imapservice.Connector{connector} + + manager := imapservice.NewLabelConflictManager(mockLabelProvider, mockIDProvider, mockClient, mockReporter, ffProviderTrue{}) + resolver := manager.NewInternalLabelConflictResolver(connectors) + + fn, err := resolver.ResolveConflict(ctx) + assert.NoError(t, err) + + updates := fn() + assert.Empty(t, updates) +} diff --git a/internal/services/imapservice/labelchecker.go b/internal/services/imapservice/labelchecker.go new file mode 100644 index 00000000..6e187e1a --- /dev/null +++ b/internal/services/imapservice/labelchecker.go @@ -0,0 +1,219 @@ +// Copyright (c) 2025 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 imapservice + +import ( + "context" + "errors" + "fmt" + "strings" + + "github.com/ProtonMail/gluon/db" + "github.com/ProtonMail/gluon/imap" + "github.com/ProtonMail/gluon/reporter" + "github.com/ProtonMail/go-proton-api" + "github.com/ProtonMail/proton-bridge/v3/pkg/algo" + "github.com/sirupsen/logrus" +) + +type labelDiscrepancyType int + +const ( + discrepancyInternal labelDiscrepancyType = iota + discrepancySystem + discrepancyUser +) + +func (t labelDiscrepancyType) String() string { + switch t { + case discrepancyInternal: + return "internal" + case discrepancySystem: + return "system" + case discrepancyUser: + return "user" + default: + return "unknown" + } +} + +type labelDiscrepancy struct { + labelName string + labelPath string + labelID string + conflictingLabelName string + conflictingLabelID string + Type labelDiscrepancyType +} + +func joinStrings(input []string) string { + return strings.Join(input, "/") +} + +func newLabelDiscrepancy(label proton.Label, mbox imap.MailboxData, dType labelDiscrepancyType) labelDiscrepancy { + discrepancy := labelDiscrepancy{ + labelName: label.Name, + labelID: label.ID, + conflictingLabelID: mbox.RemoteID, + Type: dType, + } + + if dType == discrepancyUser { + discrepancy.labelName = algo.HashBase64SHA256(label.Name) + discrepancy.labelPath = algo.HashBase64SHA256(joinStrings(label.Path)) + discrepancy.conflictingLabelName = algo.HashBase64SHA256(joinStrings(mbox.BridgeName)) + } else { + discrepancy.labelName = label.Name + discrepancy.labelPath = joinStrings(label.Path) + discrepancy.conflictingLabelName = joinStrings(mbox.BridgeName) + } + + return discrepancy +} + +func discrepanciesToContext(discrepancies []labelDiscrepancy) reporter.Context { + ctx := make(reporter.Context) + + for i, d := range discrepancies { + prefix := fmt.Sprintf("discrepancy_%d_", i) + + ctx[prefix+"type"] = d.Type.String() + ctx[prefix+"label_name"] = d.labelName + ctx[prefix+"label_path"] = d.labelPath + ctx[prefix+"label_id"] = d.labelID + ctx[prefix+"conflicting_label_name"] = d.conflictingLabelName + ctx[prefix+"conflicting_label_id"] = d.conflictingLabelID + } + + ctx["discrepancy_count"] = len(discrepancies) + return ctx +} + +type ConnectorGetter interface { + getConnectors() []*Connector +} + +type LabelConflictChecker struct { + gluonLabelNameProvider GluonLabelNameProvider + gluonIDProvider gluonIDProvider + connectorGetter ConnectorGetter + reporter reporter.Reporter + logger *logrus.Entry +} + +func NewConflictChecker(connectorGetter ConnectorGetter, reporter reporter.Reporter, provider gluonIDProvider, nameProvider GluonLabelNameProvider) *LabelConflictChecker { + return &LabelConflictChecker{ + gluonLabelNameProvider: nameProvider, + gluonIDProvider: provider, + connectorGetter: connectorGetter, + reporter: reporter, + logger: logrus.WithFields(logrus.Fields{ + "pkg": "imapservice/labelConflictChecker", + }), + } +} + +func (c *LabelConflictChecker) getFn() mailboxFetcherFn { + connectors := c.connectorGetter.getConnectors() + + return func(ctx context.Context, label proton.Label) (imap.MailboxData, error) { + for _, updateCh := range connectors { + addrID, ok := c.gluonIDProvider.GetGluonID(updateCh.addrID) + if !ok { + continue + } + return c.gluonLabelNameProvider.GetUserMailboxByName(ctx, addrID, GetMailboxName(label)) + } + return imap.MailboxData{}, errors.New("no gluon connectors found") + } +} + +func (c *LabelConflictChecker) CheckAndReportConflicts(ctx context.Context, labels map[string]proton.Label) error { + labelDiscrepancies, err := c.checkConflicts(ctx, labels, c.getFn()) + if err != nil { + return err + } + + if len(labelDiscrepancies) == 0 { + return nil + } + + reporterCtx := discrepanciesToContext(labelDiscrepancies) + if err := c.reporter.ReportMessageWithContext("Found label conflicts on Bridge start", reporterCtx); err != nil { + c.logger.WithError(err).Error("Failed to report label conflicts to Sentry") + } + + return nil +} + +func (c *LabelConflictChecker) checkConflicts(ctx context.Context, labels map[string]proton.Label, mboxFetch mailboxFetcherFn) ([]labelDiscrepancy, error) { + discrepancies := []labelDiscrepancy{} + + // Verify bridge internal mailboxes. + for _, prefix := range []string{folderPrefix, labelPrefix} { + label := proton.Label{ + Path: []string{prefix}, + ID: prefix, + Name: prefix, + } + + mbox, err := mboxFetch(ctx, label) + if err != nil { + if db.IsErrNotFound(err) { + continue + } + return nil, err + } + + if mbox.RemoteID != label.ID { + discrepancies = append(discrepancies, newLabelDiscrepancy(label, mbox, discrepancyInternal)) + } + } + + // Verify system and user mailboxes. + for _, label := range labels { + if !WantLabel(label) { + continue + } + + mbox, err := mboxFetch(ctx, label) + if err != nil { + if db.IsErrNotFound(err) { + continue + } + return nil, err + } + + if mbox.RemoteID != label.ID { + var dType labelDiscrepancyType + switch label.Type { + case proton.LabelTypeSystem: + dType = discrepancySystem + case proton.LabelTypeFolder, proton.LabelTypeLabel: + dType = discrepancyUser + case proton.LabelTypeContactGroup: + fallthrough + default: + dType = discrepancySystem + } + discrepancies = append(discrepancies, newLabelDiscrepancy(label, mbox, dType)) + } + } + + return discrepancies, nil +} diff --git a/internal/services/imapservice/service.go b/internal/services/imapservice/service.go index cb2b4a1f..f018f385 100644 --- a/internal/services/imapservice/service.go +++ b/internal/services/imapservice/service.go @@ -94,6 +94,7 @@ type Service struct { observabilitySender observability.Sender labelConflictManager *LabelConflictManager + LabelConflictChecker *LabelConflictChecker } func NewService( @@ -129,7 +130,7 @@ func NewService( syncMessageBuilder := NewSyncMessageBuilder(rwIdentity) syncReporter := newSyncReporter(identityState.User.ID, eventPublisher, time.Second) - return &Service{ + service := &Service{ cpc: cpc.NewCPC(), client: client, log: log, @@ -163,6 +164,9 @@ func NewService( observabilitySender: observabilitySender, labelConflictManager: labelConflictManager, } + + service.LabelConflictChecker = NewConflictChecker(service, reporter, gluonIDProvider, serverManager) + return service } func (s *Service) Start( @@ -663,7 +667,7 @@ func (s *Service) setShowAllMail(v bool) { func (s *Service) startSyncing() { s.isSyncing.Store(true) - s.syncHandler.Execute(s.syncReporter, s.labels.GetLabelMap(), s.syncUpdateApplier, s.syncMessageBuilder, syncservice.DefaultRetryCoolDown) + s.syncHandler.Execute(s.syncReporter, s.labels.GetLabelMap(), s.syncUpdateApplier, s.syncMessageBuilder, syncservice.DefaultRetryCoolDown, s.LabelConflictChecker) } func (s *Service) cancelSync() { @@ -671,6 +675,10 @@ func (s *Service) cancelSync() { s.isSyncing.Store(false) } +func (s *Service) getConnectors() []*Connector { + return maps.Values(s.connectors) +} + type resyncReq struct{} type getLabelsReq struct{} diff --git a/internal/services/imapservice/service_label_events.go b/internal/services/imapservice/service_label_events.go index 766ec201..529755ad 100644 --- a/internal/services/imapservice/service_label_events.go +++ b/internal/services/imapservice/service_label_events.go @@ -90,7 +90,7 @@ func onLabelCreated(ctx context.Context, s *Service, event proton.LabelEvent) ([ wr.SetLabel(event.Label.ID, event.Label, "onLabelCreated") - labelConflictResolver := s.labelConflictManager.NewConflictResolver(maps.Values(s.connectors)) + labelConflictResolver := s.labelConflictManager.NewUserConflictResolver(maps.Values(s.connectors)) conflictUpdatesGenerator, err := labelConflictResolver.ResolveConflict(ctx, event.Label, make(map[string]bool)) if err != nil { return updates, err @@ -150,7 +150,7 @@ func onLabelUpdated(ctx context.Context, s *Service, event proton.LabelEvent) ([ wr.SetLabel(apiLabel.ID, apiLabel, "onLabelUpdatedApiID") // Resolve potential conflicts - labelConflictResolver := s.labelConflictManager.NewConflictResolver(maps.Values(s.connectors)) + labelConflictResolver := s.labelConflictManager.NewUserConflictResolver(maps.Values(s.connectors)) conflictUpdatesGenerator, err := labelConflictResolver.ResolveConflict(ctx, apiLabel, make(map[string]bool)) if err != nil { return updates, err diff --git a/internal/services/imapservice/sync_update_applier.go b/internal/services/imapservice/sync_update_applier.go index 2d1611bf..3b5932aa 100644 --- a/internal/services/imapservice/sync_update_applier.go +++ b/internal/services/imapservice/sync_update_applier.go @@ -133,11 +133,21 @@ func (s *SyncUpdateApplier) SyncLabels(ctx context.Context, labels map[string]pr func syncLabels(ctx context.Context, labels map[string]proton.Label, connectors []*Connector, labelConflictManager *LabelConflictManager) ([]imap.Update, error) { var updates []imap.Update - labelConflictResolver := labelConflictManager.NewConflictResolver(connectors) + userLabelConflictResolver := labelConflictManager.NewUserConflictResolver(connectors) + internalLabelConflictResolver := labelConflictManager.NewInternalLabelConflictResolver(connectors) // Create placeholder Folders/Labels mailboxes with the \Noselect attribute. for _, prefix := range []string{folderPrefix, labelPrefix} { + conflictUpdateGenerator, err := internalLabelConflictResolver.ResolveConflict(ctx) + if err != nil { + return updates, err + } + for _, updateCh := range connectors { + conflictUpdates := conflictUpdateGenerator() + updateCh.publishUpdate(ctx, conflictUpdates...) + updates = append(updates, conflictUpdates...) + update := newPlaceHolderMailboxCreatedUpdate(prefix) updateCh.publishUpdate(ctx, update) updates = append(updates, update) @@ -159,7 +169,7 @@ func syncLabels(ctx context.Context, labels map[string]proton.Label, connectors } case proton.LabelTypeFolder, proton.LabelTypeLabel: - conflictUpdatesGenerator, err := labelConflictResolver.ResolveConflict(ctx, label, make(map[string]bool)) + conflictUpdatesGenerator, err := userLabelConflictResolver.ResolveConflict(ctx, label, make(map[string]bool)) if err != nil { return updates, err } diff --git a/internal/services/syncservice/handler.go b/internal/services/syncservice/handler.go index 494ddaea..e009170b 100644 --- a/internal/services/syncservice/handler.go +++ b/internal/services/syncservice/handler.go @@ -19,10 +19,12 @@ package syncservice import ( "context" + "errors" "fmt" "time" "github.com/ProtonMail/gluon/async" + "github.com/ProtonMail/gluon/db" "github.com/ProtonMail/gluon/reporter" "github.com/ProtonMail/go-proton-api" "github.com/ProtonMail/proton-bridge/v3/internal/network" @@ -34,6 +36,10 @@ const NumSyncStages = 4 type LabelMap = map[string]proton.Label +type labelConflictChecker interface { + CheckAndReportConflicts(ctx context.Context, labels map[string]proton.Label) error +} + // Handler is the interface from which we control the syncing of the IMAP data. One instance should be created for each // user and used for every subsequent sync request. type Handler struct { @@ -95,12 +101,17 @@ func (t *Handler) Execute( updateApplier UpdateApplier, messageBuilder MessageBuilder, coolDown time.Duration, + labelConflictChecker labelConflictChecker, ) { t.log.Info("Sync triggered") t.group.Once(func(ctx context.Context) { start := time.Now() t.log.WithField("start", start).Info("Beginning user sync") + if err := labelConflictChecker.CheckAndReportConflicts(ctx, labels); err != nil { + t.log.WithError(err).Error("Failed to check and report label conflicts") + } + syncReporter.OnStart(ctx) var err error for { @@ -108,11 +119,18 @@ func (t *Handler) Execute( t.log.WithError(err).Error("Sync aborted") break } else if err = t.run(ctx, syncReporter, labels, updateApplier, messageBuilder); err != nil { - if sentryErr := t.sentryReporter.ReportMessageWithContext("Failed to sync, will retry later", reporter.Context{ - "err": err.Error(), - "user_id": t.userID, - }); sentryErr != nil { - t.log.WithError(sentryErr).Error("Failed to report sentry message") + if db.IsUniqueLabelConstraintError(err) { + if sentryErr := t.sentryReporter.ReportMessageWithContext("Failed to sync due to label unique constraint conflict", + reporter.Context{"err": err}); sentryErr != nil { + t.log.WithError(sentryErr).Error("Failed to report label unique constraint conflict error to Sentry") + } + } else if !(errors.Is(err, context.Canceled)) { + if sentryErr := t.sentryReporter.ReportMessageWithContext("Failed to sync, will retry later", reporter.Context{ + "err": err.Error(), + "user_id": t.userID, + }); sentryErr != nil { + t.log.WithError(sentryErr).Error("Failed to report sentry message") + } } t.log.WithError(err).Error("Failed to sync, will retry later") diff --git a/internal/services/syncservice/handler_test.go b/internal/services/syncservice/handler_test.go index 72def32e..83edb880 100644 --- a/internal/services/syncservice/handler_test.go +++ b/internal/services/syncservice/handler_test.go @@ -209,6 +209,13 @@ func TestTask_StateHasSyncedState(t *testing.T) { require.NoError(t, err) } +type mockLabelConflictChecker struct { +} + +func (m *mockLabelConflictChecker) CheckAndReportConflicts(_ context.Context, _ map[string]proton.Label) error { + return nil +} + func TestTask_RepeatsOnSyncFailure(t *testing.T) { const MessageTotal int64 = 50 const MessageID string = "foo" @@ -272,7 +279,7 @@ func TestTask_RepeatsOnSyncFailure(t *testing.T) { tt.syncReporter.EXPECT().OnFinished(gomock.Any()) tt.syncReporter.EXPECT().OnProgress(gomock.Any(), gomock.Eq(MessageDelta)) - tt.task.Execute(tt.syncReporter, labels, tt.updateApplier, tt.messageBuilder, time.Microsecond) + tt.task.Execute(tt.syncReporter, labels, tt.updateApplier, tt.messageBuilder, time.Microsecond, &mockLabelConflictChecker{}) require.NoError(t, <-tt.task.OnSyncFinishedCH()) } diff --git a/internal/unleash/service.go b/internal/unleash/service.go index e4fb2339..622ff5dc 100644 --- a/internal/unleash/service.go +++ b/internal/unleash/service.go @@ -43,6 +43,7 @@ const ( UpdateUseNewVersionFileStructureDisabled = "InboxBridgeUpdateWithOsFilterDisabled" LabelConflictResolverDisabled = "InboxBridgeLabelConflictResolverDisabled" SMTPSubmissionRequestSentryReportDisabled = "InboxBridgeSmtpSubmissionRequestSentryReportDisabled" + InternalLabelConflictResolverDisabled = "InboxBridgeUnexpectedFoldersLabelsStartupFixupDisabled" ) type FeatureFlagValueProvider interface {