From 6fadbde4a648a93262a176567c220796b5d6e463 Mon Sep 17 00:00:00 2001 From: Jakub Cuth Date: Wed, 13 Mar 2024 07:49:25 +0000 Subject: [PATCH] feat(GODT-3185): report cases which leads to wrong address key used --- ci/test.yml | 6 + internal/bridge/send_test.go | 2 +- internal/services/imapservice/connector.go | 116 +++++++++++++++++- .../services/imapservice/connector_test.go | 32 +++++ internal/services/imapservice/service.go | 2 + .../imapservice/service_address_events.go | 1 + internal/services/smtp/errors.go | 10 +- internal/services/smtp/smtp.go | 4 +- tests/environment_test.go | 27 ++++ tests/features/imap/addressmode.feature | 42 +++++++ tests/features/imap/auth.feature | 7 -- tests/features/imap/message/create.feature | 1 + tests/features/imap/message/drafts.feature | 1 + tests/features/smtp/addressmode.feature | 45 +++++++ .../smtp/send/failures_disabled.feature | 2 +- tests/features/smtp/send/plain.feature | 2 +- tests/features/smtp/send/sender_key.feature | 79 ++++++++++++ tests/steps_test.go | 2 + 18 files changed, 358 insertions(+), 23 deletions(-) create mode 100644 tests/features/imap/addressmode.feature create mode 100644 tests/features/smtp/addressmode.feature create mode 100644 tests/features/smtp/send/sender_key.feature diff --git a/ci/test.yml b/ci/test.yml index 91160848..f93cde0f 100644 --- a/ci/test.yml +++ b/ci/test.yml @@ -70,6 +70,9 @@ test-integration: - test-linux script: - make test-integration | tee -a integration-job.log + after_script: + - | + grep "Error: " integration-job.log artifacts: when: always paths: @@ -95,6 +98,9 @@ test-integration-nightly: - test-integration script: - make test-integration-nightly | tee -a nightly-job.log + after_script: + - | + grep "Error: " nightly-job.log artifacts: when: always paths: diff --git a/internal/bridge/send_test.go b/internal/bridge/send_test.go index fc1c4904..7914090e 100644 --- a/internal/bridge/send_test.go +++ b/internal/bridge/send_test.go @@ -750,7 +750,7 @@ func TestBridge_SendAddressDisabled(t *testing.T) { strings.NewReader("Subject: Test 1\r\n\r\nHello world!"), ) - smtpErr := smtpservice.NewErrCanNotSendOnAddress(senderInfo.Addresses[0]) + smtpErr := smtpservice.NewErrCannotSendFromAddress(senderInfo.Addresses[0]) require.Equal(t, fmt.Sprintf("Error: %v", smtpErr.Error()), err.Error()) }) }) diff --git a/internal/services/imapservice/connector.go b/internal/services/imapservice/connector.go index 0c1621a0..3a2fa0f8 100644 --- a/internal/services/imapservice/connector.go +++ b/internal/services/imapservice/connector.go @@ -23,12 +23,15 @@ import ( "errors" "fmt" "net/mail" + "strings" "sync/atomic" "time" "github.com/ProtonMail/gluon/async" "github.com/ProtonMail/gluon/connector" "github.com/ProtonMail/gluon/imap" + "github.com/ProtonMail/gluon/reporter" + "github.com/ProtonMail/gluon/rfc5322" "github.com/ProtonMail/gluon/rfc822" "github.com/ProtonMail/go-proton-api" "github.com/ProtonMail/gopenpgp/v2/crypto" @@ -54,6 +57,7 @@ type Connector struct { identityState sharedIdentity client APIClient telemetry Telemetry + reporter reporter.Reporter panicHandler async.PanicHandler sendRecorder *sendrecorder.SendRecorder @@ -75,6 +79,7 @@ func NewConnector( sendRecorder *sendrecorder.SendRecorder, panicHandler async.PanicHandler, telemetry Telemetry, + reporter reporter.Reporter, showAllMail bool, syncState *SyncState, ) *Connector { @@ -90,6 +95,7 @@ func NewConnector( client: apiClient, telemetry: telemetry, + reporter: reporter, panicHandler: panicHandler, sendRecorder: sendRecorder, @@ -279,7 +285,7 @@ func (s *Connector) CreateMessage(ctx context.Context, _ connector.IMAPStateWrit if messageID, ok, err := s.sendRecorder.HasEntryWait(ctx, hash, time.Now().Add(90*time.Second), toList); err != nil { return imap.Message{}, nil, fmt.Errorf("failed to check send hash: %w", err) } else if ok { - s.log.WithField("messageID", messageID).Warn("Message already sent") + s.log.WithField("messageID", messageID).Warn("Message already in sent mailbox") // Query the server-side message. full, err := s.client.GetFullMessage(ctx, messageID, usertypes.NewProtonAPIScheduler(s.panicHandler), proton.NewDefaultAttachmentAllocator()) @@ -671,11 +677,21 @@ func (s *Connector) importMessage( ) (imap.Message, []byte, error) { var full proton.FullMessage + // addr is primary for combined mode or active for split mode addr, ok := s.identityState.GetAddress(s.addrID) if !ok { return imap.Message{}, nil, fmt.Errorf("could not find address") } + p, err2 := parser.New(bytes.NewReader(literal)) + if err2 != nil { + return imap.Message{}, nil, fmt.Errorf("failed to parse literal: %w", err2) + } + + isDraft := slices.Contains(labelIDs, proton.DraftsLabel) + + s.reportGODT3185(isDraft, addr.Email, p, s.addressMode == usertypes.AddressModeCombined) + if err := s.identityState.WithAddrKR(s.addrID, func(_, addrKR *crypto.KeyRing) error { primaryKey, errKey := addrKR.FirstKey() if errKey != nil { @@ -683,11 +699,8 @@ func (s *Connector) importMessage( } var messageID string - p, err2 := parser.New(bytes.NewReader(literal)) - if err2 != nil { - return fmt.Errorf("failed to parse literal: %w", err2) - } - if slices.Contains(labelIDs, proton.DraftsLabel) { + + if isDraft { msg, err := s.createDraftWithParser(ctx, p, primaryKey, addr) if err != nil { return fmt.Errorf("failed to create draft: %w", err) @@ -850,3 +863,94 @@ func defaultMailboxPermanentFlags() imap.FlagSet { func defaultMailboxAttributes() imap.FlagSet { return imap.NewFlagSet() } + +func stripPlusAlias(a string) string { + iPlus := strings.Index(a, "+") + iAt := strings.Index(a, "@") + if iPlus <= 0 || iAt <= 0 || iPlus >= iAt { + return a + } + + return a[:iPlus] + a[iAt:] +} + +func equalAddresses(a, b string) bool { + return strings.EqualFold(stripPlusAlias(a), stripPlusAlias(b)) +} + +func (s *Connector) reportGODT3185(isDraft bool, defaultAddr string, p *parser.Parser, isCombinedMode bool) { + reportAction := "draft" + if !isDraft { + reportAction = "import" + } + + reportMode := "combined" + if isCombinedMode { + reportMode = "split" + } + + senderAddr := "" + if p != nil && p.Root() != nil && p.Root().Header.Len() != 0 { + addrField := p.Root().Header.Get("From") + if addrField == "" { + addrField = p.Root().Header.Get("Sender") + } + if addrField != "" { + sender, err := rfc5322.ParseAddressList(addrField) + if err == nil && len(sender) > 0 { + senderAddr = sender[0].Address + } else { + s.log.WithError(err).Warn("Invalid sender address in reporter") + } + } + } + + if equalAddresses(defaultAddr, senderAddr) { + return + } + + isDisabled := false + isUserAddress := false + for _, a := range s.identityState.GetAddresses() { + if !equalAddresses(a.Email, senderAddr) { + continue + } + + isUserAddress = true + isDisabled = !bool(a.Send) || (a.Status != proton.AddressStatusEnabled) + break + } + + if !isUserAddress && senderAddr != "" { + return + } + + reportResult := "using sender address" + + if !isCombinedMode { + reportResult = "error address not match" + } + + reportAddress := "" + if senderAddr == "" { + reportAddress = " invalid" + reportResult = "error import/draft" + } + + if isDisabled { + reportAddress = " disabled" + if isDraft { + reportResult = "error draft" + } + } + + report := fmt.Sprintf( + "GODT-3185: %s with non-default%s address in %s mode: %s", + reportAction, reportAddress, reportMode, reportResult, + ) + + s.log.Warn(report) + if s.reporter != nil { + _ = s.reporter.ReportMessage(report) + } +} diff --git a/internal/services/imapservice/connector_test.go b/internal/services/imapservice/connector_test.go index 6cecd473..8133267f 100644 --- a/internal/services/imapservice/connector_test.go +++ b/internal/services/imapservice/connector_test.go @@ -203,3 +203,35 @@ func TestFixGODT3003Labels_Noop(t *testing.T) { require.NoError(t, err) require.False(t, applied) } + +func TestStripPlusAlias(t *testing.T) { + cases := map[string]string{ + "one@three.com": "one@three.com", + "one+two@three.com": "one@three.com", + "one@three+two.com": "one@three+two.com", + "+one@three.com": "+one@three.com", + "@three.com": "@three.com", + } + + for given, want := range cases { + require.Equal(t, want, stripPlusAlias(given), "input was %q", given) + } +} + +func TestEqualAddresse(t *testing.T) { + cases := []struct { + a, b string + want bool + }{ + {"one@three.com", "one@three.com", true}, + {"one@three.com", "one+two@three.com", true}, + {"OnE@thReE.com", "One@THree.com", true}, + {"one@three.com", "two@three.com", false}, + {"one+two@three.com", "two@three.com", false}, + {"one@three.com", "one@three+two.com", false}, + } + + for _, c := range cases { + require.Equal(t, c.want, equalAddresses(c.a, c.b), "input was %q and %q", c.a, c.b) + } +} diff --git a/internal/services/imapservice/service.go b/internal/services/imapservice/service.go index 7be496e7..0650e89b 100644 --- a/internal/services/imapservice/service.go +++ b/internal/services/imapservice/service.go @@ -508,6 +508,7 @@ func (s *Service) buildConnectors() (map[string]*Connector, error) { s.sendRecorder, s.panicHandler, s.telemetry, + s.reporter, s.showAllMail, s.syncStateProvider, ) @@ -525,6 +526,7 @@ func (s *Service) buildConnectors() (map[string]*Connector, error) { s.sendRecorder, s.panicHandler, s.telemetry, + s.reporter, s.showAllMail, s.syncStateProvider, ) diff --git a/internal/services/imapservice/service_address_events.go b/internal/services/imapservice/service_address_events.go index 898f16dc..6dfa8b94 100644 --- a/internal/services/imapservice/service_address_events.go +++ b/internal/services/imapservice/service_address_events.go @@ -155,6 +155,7 @@ func addNewAddressSplitMode(ctx context.Context, s *Service, addrID string) erro s.sendRecorder, s.panicHandler, s.telemetry, + s.reporter, s.showAllMail, s.syncStateProvider, ) diff --git a/internal/services/smtp/errors.go b/internal/services/smtp/errors.go index b60b6fe4..ccd3fd19 100644 --- a/internal/services/smtp/errors.go +++ b/internal/services/smtp/errors.go @@ -27,14 +27,14 @@ var ErrInvalidReturnPath = errors.New("invalid return path") var ErrNoSuchUser = errors.New("no such user") var ErrTooManyErrors = errors.New("too many failed requests, please try again later") -type ErrCanNotSendOnAddress struct { +type ErrCannotSendFromAddress struct { address string } -func NewErrCanNotSendOnAddress(address string) *ErrCanNotSendOnAddress { - return &ErrCanNotSendOnAddress{address: address} +func NewErrCannotSendFromAddress(address string) *ErrCannotSendFromAddress { + return &ErrCannotSendFromAddress{address: address} } -func (e ErrCanNotSendOnAddress) Error() string { - return fmt.Sprintf("can't send on address: %v", e.address) +func (e ErrCannotSendFromAddress) Error() string { + return fmt.Sprintf("cannot send from address: %v", e.address) } diff --git a/internal/services/smtp/smtp.go b/internal/services/smtp/smtp.go index 4610e238..34ebfa96 100644 --- a/internal/services/smtp/smtp.go +++ b/internal/services/smtp/smtp.go @@ -103,8 +103,8 @@ func (s *Service) smtpSendMail(ctx context.Context, authID string, from string, } if !fromAddr.Send || fromAddr.Status != proton.AddressStatusEnabled { - s.log.Errorf("Can't send emails on address: %v", fromAddr.Email) - return &ErrCanNotSendOnAddress{address: fromAddr.Email} + s.log.Errorf("Cannot send emails from address: %v", fromAddr.Email) + return &ErrCannotSendFromAddress{address: fromAddr.Email} } // Load the user's mail settings. diff --git a/tests/environment_test.go b/tests/environment_test.go index d8e21032..17184a5b 100644 --- a/tests/environment_test.go +++ b/tests/environment_test.go @@ -205,3 +205,30 @@ func (s *scenario) theBodyInTheResponseToIs(method, path string, value *godog.Do return nil } + +func (s *scenario) theMessageUsedKeyForSending(address string) error { + addrID := s.t.getUserByAddress(address).getAddrID(address) + + call, err := s.t.getLastCallExcludingHTTPOverride("POST", "/mail/v4/messages") + if err != nil { + return err + } + + var body, want map[string]any + + if err := json.Unmarshal(call.ResponseBody, &body); err != nil { + return err + } + + want = map[string]any{ + "Message": map[string]any{ + "AddressID": addrID, + }, + } + + if !IsSub(body, want) { + return fmt.Errorf("have body %v, want %v", body, want) + } + + return nil +} diff --git a/tests/features/imap/addressmode.feature b/tests/features/imap/addressmode.feature new file mode 100644 index 00000000..8fc2180d --- /dev/null +++ b/tests/features/imap/addressmode.feature @@ -0,0 +1,42 @@ +Feature: IMAP client authentication with address modes + Background: + Given there exists an account with username "[user:user]" and password "password" + And the account "[user:user]" has additional address "[alias:alias]@[domain]" + And it succeeds + + Scenario: IMAP client can authenticate successfully with secondary address in combine mode + Given bridge starts + And the user logs in with username "[user:user]" and password "password" + And user "[user:user]" finishes syncing + Then user "[user:user]" connects and authenticates IMAP client "1" with address "[alias:alias]@[domain]" + + Scenario: IMAP client can authenticate successfully with secondary address in split mode + Given bridge starts + And the user logs in with username "[user:user]" and password "password" + And the user sets the address mode of user "[user:user]" to "split" + And user "[user:user]" finishes syncing + Then user "[user:user]" connects and authenticates IMAP client "1" with address "[alias:alias]@[domain]" + + # Need to find way to setup disabled address on black + @skip-black + Scenario: IMAP client cannot authenticate successfully with disabled alias in combine mode + Given the account "[user:user]" has additional disabled address "[alias:disabled]@[domain]" + And it succeeds + Given bridge starts + And the user logs in with username "[user:user]" and password "password" + And user "[user:user]" finishes syncing + # GODT-3307 it should succeed + When user "[user:user]" connects and can not authenticate IMAP client "1" with address "[alias:disabled]@[domain]" + + # Need to find way to setup disabled address on black + @skip-black + Scenario: IMAP client cannot authenticate successfully with disabled alias in split mode + Given the account "[user:user]" has additional disabled address "[alias:disabled]@[domain]" + And it succeeds + Given bridge starts + And the user logs in with username "[user:user]" and password "password" + And the user sets the address mode of user "[user:user]" to "split" + And user "[user:user]" finishes syncing + # GODT-3307 it should succeed + When user "[user:user]" connects and can not authenticate IMAP client "1" with address "[alias:disabled]@[domain]" + diff --git a/tests/features/imap/auth.feature b/tests/features/imap/auth.feature index d72b6a31..a4662e38 100644 --- a/tests/features/imap/auth.feature +++ b/tests/features/imap/auth.feature @@ -20,13 +20,6 @@ Feature: A user can authenticate an IMAP client Scenario: IMAP client can authenticate successfully with secondary address Given user "[user:user]" connects and authenticates IMAP client "1" with address "[alias:alias]@[domain]" - # Need to find way to setup disabled address on black - @skip-black - Scenario: IMAP client can not authenticate successfully with disable address - Given the account "[user:user2]" has additional disabled address "[alias:disabled]@[domain]" - And it succeeds - Then user "[user:user2]" connects and can not authenticate IMAP client "1" with address "[alias:disabled]@[domain]" - Scenario: IMAP client can authenticate successfully When user "[user:user]" connects IMAP client "1" Then IMAP client "1" can authenticate diff --git a/tests/features/imap/message/create.feature b/tests/features/imap/message/create.feature index 9b7305c4..3d7c96e0 100644 --- a/tests/features/imap/message/create.feature +++ b/tests/features/imap/message/create.feature @@ -57,6 +57,7 @@ Feature: IMAP create messages And IMAP client "1" eventually sees the following messages in "All Mail": | from | to | subject | body | | [alias:alias]@[domain] | john.doe@email.com | foo | bar | + And bridge reports a message with "GODT-3185: import with non-default address in split mode: using sender address" Scenario: Imports an unrelated message to inbox When IMAP client "1" appends the following messages to "INBOX": diff --git a/tests/features/imap/message/drafts.feature b/tests/features/imap/message/drafts.feature index 12eb19e2..ac7f3329 100644 --- a/tests/features/imap/message/drafts.feature +++ b/tests/features/imap/message/drafts.feature @@ -164,6 +164,7 @@ Feature: IMAP Draft messages And IMAP client "1" eventually sees the following messages in "Drafts": | to | subject | body | | someone@example.com | Draft without From | This is a Draft without From in header | + And bridge reports a message with "GODT-3185: draft with non-default invalid address in split mode: error import/draft" @regression Scenario: Only one draft in Drafts and All Mail after editing it locally multiple times diff --git a/tests/features/smtp/addressmode.feature b/tests/features/smtp/addressmode.feature new file mode 100644 index 00000000..473645bf --- /dev/null +++ b/tests/features/smtp/addressmode.feature @@ -0,0 +1,45 @@ +Feature: SMTP client authentication with address modes + Background: + Given there exists an account with username "[user:user]" and password "password" + And the account "[user:user]" has additional address "[alias:alias]@[domain]" + And it succeeds + + Scenario: SMTP client can authenticate successfully with secondary address in combine mode + Given bridge starts + And the user logs in with username "[user:user]" and password "password" + And user "[user:user]" finishes syncing + When user "[user:user]" connects and authenticates SMTP client "1" with address "[alias:alias]@[domain]" + Then it succeeds + + Scenario: SMTP client can authenticate successfully with secondary address in split mode + Given bridge starts + And the user logs in with username "[user:user]" and password "password" + And the user sets the address mode of user "[user:user]" to "split" + And user "[user:user]" finishes syncing + When user "[user:user]" connects and authenticates SMTP client "1" with address "[alias:alias]@[domain]" + Then it succeeds + + # Need to find way to setup disabled address on black + @skip-black + Scenario: SMTP client can authenticate successfully with disabled alias in combine mode + Given the account "[user:user]" has additional disabled address "[alias:disabled]@[domain]" + And it succeeds + Given bridge starts + And the user logs in with username "[user:user]" and password "password" + And user "[user:user]" finishes syncing + When user "[user:user]" connects and authenticates SMTP client "1" with address "[alias:disabled]@[domain]" + Then it fails + + # Need to find way to setup disabled address on black + @skip-black + Scenario: SMTP client can authenticate successfully with disabled alias in split mode + Given the account "[user:user]" has additional disabled address "[alias:disabled]@[domain]" + And it succeeds + Given bridge starts + And the user logs in with username "[user:user]" and password "password" + And the user sets the address mode of user "[user:user]" to "split" + And user "[user:user]" finishes syncing + When user "[user:user]" connects and authenticates SMTP client "1" with address "[alias:disabled]@[domain]" + Then it fails + + diff --git a/tests/features/smtp/send/failures_disabled.feature b/tests/features/smtp/send/failures_disabled.feature index 3fc2f450..83f468d0 100644 --- a/tests/features/smtp/send/failures_disabled.feature +++ b/tests/features/smtp/send/failures_disabled.feature @@ -20,5 +20,5 @@ Feature: SMTP wrong messages Hello """ - And it fails with error "Error: can't send on address: [user:disabled]@[domain]" + And it fails with error "Error: cannot send from address: [user:disabled]@[domain]" diff --git a/tests/features/smtp/send/plain.feature b/tests/features/smtp/send/plain.feature index a8427333..a26cb02f 100644 --- a/tests/features/smtp/send/plain.feature +++ b/tests/features/smtp/send/plain.feature @@ -331,4 +331,4 @@ Feature: SMTP sending of plain messages } ] } - """ \ No newline at end of file + """ diff --git a/tests/features/smtp/send/sender_key.feature b/tests/features/smtp/send/sender_key.feature new file mode 100644 index 00000000..ce5c49d2 --- /dev/null +++ b/tests/features/smtp/send/sender_key.feature @@ -0,0 +1,79 @@ +Feature: Address key usage during SMTP send + Background: + Given there exists an account with username "[user:user]" and password "password" + And the account "[user:user]" has additional address "[alias:alias]@[domain]" + And it succeeds + + Scenario: Non-active sender in combined mode using non-active key + Given bridge starts + And the user logs in with username "[user:user]" and password "password" + And it succeeds + When user "[user:user]" connects and authenticates SMTP client "1" with address "[user:user]@[domain]" + And SMTP client "1" sends the following message from "[alias:alias]@[domain]" to "pm.bridge.qa@gmail.com": + """ + From: Bridge Test <[alias:alias]@[domain]> + To: External Bridge + + hello + + """ + Then it succeeds + And the message used "[alias:alias]@[domain]" key for sending + + Scenario: Non-active sender in split mode using non-active key + Given bridge starts + And the user logs in with username "[user:user]" and password "password" + And the user sets the address mode of user "[user:user]" to "split" + And user "[user:user]" finishes syncing + And it succeeds + When user "[user:user]" connects and authenticates SMTP client "1" with address "[user:user]@[domain]" + And SMTP client "1" sends the following message from "[alias:alias]@[domain]" to "pm.bridge.qa@gmail.com": + """ + From: Bridge Test <[alias:alias]@[domain]> + To: External Bridge + + hello + + """ + Then it succeeds + And the message used "[alias:alias]@[domain]" key for sending + + # Need to find way to setup disabled address on black + @skip-black + Scenario: Disabled sender in combined mode fails to send + Given the account "[user:user]" has additional disabled address "[user:disabled]@[domain]" + And it succeeds + And bridge starts + And the user logs in with username "[user:user]" and password "password" + And it succeeds + When user "[user:user]" connects and authenticates SMTP client "1" with address "[user:user]@[domain]" + And SMTP client "1" sends the following message from "[alias:disabled]@[domain]" to "pm.bridge.qa@gmail.com": + """ + From: Bridge Test <[alias:disabled]@[domain]> + To: External Bridge + + hello + + """ + Then it fails + + # Need to find way to setup disabled address on black + @skip-black + Scenario: Disabled sender in split mode fails to send + Given the account "[user:user]" has additional disabled address "[alias:disabled]@[domain]" + And it succeeds + And bridge starts + And the user logs in with username "[user:user]" and password "password" + And the user sets the address mode of user "[user:user]" to "split" + And user "[user:user]" finishes syncing + And it succeeds + When user "[user:user]" connects and authenticates SMTP client "1" with address "[alias:alias]@[domain]" + And SMTP client "1" sends the following message from "[alias:disabled]@[domain]" to "pm.bridge.qa@gmail.com": + """ + From: Bridge Test <[alias:disabled]@[domain]> + To: External Bridge + + hello + + """ + Then it fails diff --git a/tests/steps_test.go b/tests/steps_test.go index 1dd39ea6..8004c834 100644 --- a/tests/steps_test.go +++ b/tests/steps_test.go @@ -38,6 +38,8 @@ func (s *scenario) steps(ctx *godog.ScenarioContext) { ctx.Step(`^the network port range (\d+)-(\d+) is busy$`, s.networkPortRangeIsBusy) ctx.Step(`^bridge IMAP port is (\d+)`, s.bridgeIMAPPortIs) ctx.Step(`^bridge SMTP port is (\d+)`, s.bridgeSMTPPortIs) + ctx.Step(`^the message used "([^"]*)" key for sending$`, s.theMessageUsedKeyForSending) + // ==== SETUP ==== ctx.Step(`^there exists an account with username "([^"]*)" and password "([^"]*)"$`, s.thereExistsAnAccountWithUsernameAndPassword) ctx.Step(`^there exists a disabled account with username "([^"]*)" and password "([^"]*)"$`, s.thereExistsAnAccountWithUsernameAndPasswordWithDisablePrimary)