From 8b39ea4acb1857265820a42817a27c3bcdfaad4f Mon Sep 17 00:00:00 2001 From: Jakub Date: Thu, 28 Jul 2022 10:52:40 +0200 Subject: [PATCH] GODT-1740: Opt-out All Mail visibility in settings file. --- internal/bridge/bridge.go | 35 ++++++++++++++++------- internal/config/settings/settings.go | 3 ++ internal/imap/bridge.go | 1 + internal/imap/user.go | 4 +++ test/bridge_actions_test.go | 12 ++++++++ test/fakeapi/labels.go | 10 ++++++- test/features/imap/mailbox/create.feature | 23 +++++++++++++++ test/features/imap/mailbox/list.feature | 23 +++++++++++++++ test/imap_checks_test.go | 2 +- 9 files changed, 100 insertions(+), 13 deletions(-) diff --git a/internal/bridge/bridge.go b/internal/bridge/bridge.go index af1c3a67..4c0cd1af 100644 --- a/internal/bridge/bridge.go +++ b/internal/bridge/bridge.go @@ -57,8 +57,9 @@ type Bridge struct { // Bridge's global errors list. errors []error - isFirstStart bool - lastVersion string + isAllMailVisible bool + isFirstStart bool + lastVersion string } func New( @@ -92,15 +93,16 @@ func New( ) b := &Bridge{ - Users: u, - locations: locations, - settings: setting, - clientManager: clientManager, - updater: updater, - versioner: versioner, - cacheProvider: cacheProvider, - autostart: autostart, - isFirstStart: false, + Users: u, + locations: locations, + settings: setting, + clientManager: clientManager, + updater: updater, + versioner: versioner, + cacheProvider: cacheProvider, + autostart: autostart, + isFirstStart: false, + isAllMailVisible: setting.GetBool(settings.IsAllMailVisible), } if setting.GetBool(settings.FirstStartKey) { @@ -302,3 +304,14 @@ func (b *Bridge) GetLastVersion() string { func (b *Bridge) IsFirstStart() bool { return b.isFirstStart } + +// IsAllMailVisible can be called extensively by IMAP. Therefore, it is better +// to cache the value instead of reading from settings file. +func (b *Bridge) IsAllMailVisible() bool { + return b.isAllMailVisible +} + +func (b *Bridge) SetIsAllMailVisible(isVisible bool) { + b.settings.SetBool(settings.IsAllMailVisible, isVisible) + b.isAllMailVisible = isVisible +} diff --git a/internal/config/settings/settings.go b/internal/config/settings/settings.go index 00e1aabb..e92e0ddf 100644 --- a/internal/config/settings/settings.go +++ b/internal/config/settings/settings.go @@ -55,6 +55,7 @@ const ( AttachmentWorkers = "attachment_workers" ColorScheme = "color_scheme" RebrandingMigrationKey = "rebranding_migrated" + IsAllMailVisible = "is_all_mail_visible" ) type Settings struct { @@ -110,4 +111,6 @@ func (s *Settings) setDefaultValues() { // By default, stick to STARTTLS. If the user uses catalina+applemail they'll have to change to SSL. s.setDefault(SMTPSSLKey, "false") + + s.setDefault(IsAllMailVisible, "true") } diff --git a/internal/imap/bridge.go b/internal/imap/bridge.go index c6e76f50..3183002a 100644 --- a/internal/imap/bridge.go +++ b/internal/imap/bridge.go @@ -31,6 +31,7 @@ type cacheProvider interface { type bridger interface { GetUser(query string) (bridgeUser, error) HasError(err error) bool + IsAllMailVisible() bool } type bridgeUser interface { diff --git a/internal/imap/user.go b/internal/imap/user.go index b4fe4d96..6e87e79e 100644 --- a/internal/imap/user.go +++ b/internal/imap/user.go @@ -128,6 +128,10 @@ func (iu *imapUser) ListMailboxes(showOnlySubcribed bool) ([]goIMAPBackend.Mailb mailboxes := []goIMAPBackend.Mailbox{} for _, storeMailbox := range iu.storeAddress.ListMailboxes() { + if storeMailbox.LabelID() == pmapi.AllMailLabel && !iu.backend.bridge.IsAllMailVisible() { + continue + } + if showOnlySubcribed && !iu.isSubscribed(storeMailbox.LabelID()) { continue } diff --git a/test/bridge_actions_test.go b/test/bridge_actions_test.go index 0cea6bac..ba0adb8d 100644 --- a/test/bridge_actions_test.go +++ b/test/bridge_actions_test.go @@ -24,6 +24,8 @@ import ( func BridgeActionsFeatureContext(s *godog.ScenarioContext) { s.Step(`^bridge starts$`, bridgeStarts) s.Step(`^bridge syncs "([^"]*)"$`, bridgeSyncsUser) + s.Step(`^All mail mailbox is hidden$`, allMailMailboxIsHidden) + s.Step(`^All mail mailbox is visible$`, allMailMailboxIsVisible) } func bridgeStarts() error { @@ -42,3 +44,13 @@ func bridgeSyncsUser(bddUserID string) error { ctx.SetLastError(ctx.GetTestingError()) return nil } + +func allMailMailboxIsHidden() error { + ctx.GetBridge().SetIsAllMailVisible(false) + return nil +} + +func allMailMailboxIsVisible() error { + ctx.GetBridge().SetIsAllMailVisible(true) + return nil +} diff --git a/test/fakeapi/labels.go b/test/fakeapi/labels.go index dbec519f..c06ae001 100644 --- a/test/fakeapi/labels.go +++ b/test/fakeapi/labels.go @@ -20,6 +20,7 @@ package fakeapi import ( "context" "fmt" + "strings" "github.com/ProtonMail/proton-bridge/v2/pkg/pmapi" ) @@ -94,9 +95,16 @@ func (api *FakePMAPI) createLabel(_ context.Context, label *pmapi.Label, route s if err := api.checkAndRecordCall(POST, route, &pmapi.LabelReq{Label: label}); err != nil { return nil, err } + + // API blocks certain names + switch strings.ToLower(label.Name) { + case "inbox", "drafts", "trash", "spam", "starred": + return nil, fmt.Errorf("Invalid name") //nolint:stylecheck + } + for _, existingLabel := range api.labels { if existingLabel.Name == label.Name { - return nil, fmt.Errorf("folder or label %s already exists", label.Name) + return nil, fmt.Errorf("A label or folder with this name already exists") //nolint:stylecheck } } prefix := "label" diff --git a/test/features/imap/mailbox/create.feature b/test/features/imap/mailbox/create.feature index 7c37210c..77e84960 100644 --- a/test/features/imap/mailbox/create.feature +++ b/test/features/imap/mailbox/create.feature @@ -17,11 +17,34 @@ Feature: IMAP create mailbox And "user" does not have mailbox "Folders/mbox" And "user" has mailbox "Labels/mbox" + Scenario: Creating label with existing name is not possible + Given there is "user" with mailbox "Folders/mbox" + When IMAP client creates mailbox "Labels/mbox" + Then IMAP response is "IMAP error: NO A label or folder with this name already exists" + And "user" has mailbox "Folders/mbox" + And "user" does not have mailbox "Labels/mbox" + + Scenario: Creating folder with existing name is not possible + Given there is "user" with mailbox "Labels/mbox" + When IMAP client creates mailbox "Folders/mbox" + Then IMAP response is "IMAP error: NO A label or folder with this name already exists" + And "user" has mailbox "Labels/mbox" + And "user" does not have mailbox "Folders/mbox" + Scenario: Creating system mailbox is not possible When IMAP client creates mailbox "INBOX" Then IMAP response is "IMAP error: NO mailbox INBOX already exists" + When IMAP client creates mailbox "Folders/INBOX" + Then IMAP response is "IMAP error: NO Invalid name" + # API allows you to create custom folder with naem `All Mail` + #When IMAP client creates mailbox "Folders/All mail" + #Then IMAP response is "IMAP error: NO mailbox All Mail already exists" Scenario: Creating mailbox without prefix is not possible When IMAP client creates mailbox "mbox" Then IMAP response is "OK" And "user" does not have mailbox "mbox" + When All mail mailbox is hidden + And IMAP client creates mailbox "All mail" + Then IMAP response is "OK" + And "user" does not have mailbox "All mail" diff --git a/test/features/imap/mailbox/list.feature b/test/features/imap/mailbox/list.feature index e8848f1f..99a4591e 100644 --- a/test/features/imap/mailbox/list.feature +++ b/test/features/imap/mailbox/list.feature @@ -15,6 +15,29 @@ Feature: IMAP list mailboxes Then IMAP response contains "Folders/mbox1" Then IMAP response contains "Labels/mbox2" + Scenario: List mailboxes without All Mail + Given there is IMAP client logged in as "user" + When IMAP client lists mailboxes + Then IMAP response contains "INBOX" + Then IMAP response contains "Sent" + Then IMAP response contains "Archive" + Then IMAP response contains "Trash" + Then IMAP response contains "All Mail" + When All mail mailbox is hidden + And IMAP client lists mailboxes + Then IMAP response contains "INBOX" + Then IMAP response contains "Sent" + Then IMAP response contains "Archive" + Then IMAP response contains "Trash" + Then IMAP response doesn't contain "All Mail" + When All mail mailbox is visible + And IMAP client lists mailboxes + Then IMAP response contains "INBOX" + Then IMAP response contains "Sent" + Then IMAP response contains "Archive" + Then IMAP response contains "Trash" + Then IMAP response contains "All Mail" + @ignore-live Scenario: List mailboxes with subfolders # Escaped slash in the name contains slash in the name. diff --git a/test/imap_checks_test.go b/test/imap_checks_test.go index f899161c..5425d1fc 100644 --- a/test/imap_checks_test.go +++ b/test/imap_checks_test.go @@ -33,7 +33,7 @@ func IMAPChecksFeatureContext(s *godog.ScenarioContext) { s.Step(`^IMAP response to "([^"]*)" contains "([^"]*)"$`, imapResponseNamedContains) s.Step(`^IMAP response has (\d+) message(?:s)?$`, imapResponseHasNumberOfMessages) s.Step(`^IMAP response to "([^"]*)" has (\d+) message(?:s)?$`, imapResponseNamedHasNumberOfMessages) - s.Step(`^IMAP response does not contain "([^"]*)"$`, imapResponseDoesNotContain) + s.Step(`^IMAP response does(?: not|n't) contain "([^"]*)"$`, imapResponseDoesNotContain) s.Step(`^IMAP response to "([^"]*)" does not contain "([^"]*)"$`, imapResponseNamedDoesNotContain) s.Step(`^IMAP client receives update marking message seq "([^"]*)" as read within (\d+) seconds$`, imapClientReceivesUpdateMarkingMessageSeqAsReadWithin) s.Step(`^IMAP client "([^"]*)" receives update marking message seq "([^"]*)" as read within (\d+) seconds$`, imapClientNamedReceivesUpdateMarkingMessageSeqAsReadWithin)