feat(GODT-3185): report cases which leads to wrong address key used

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:

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

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

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

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

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

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

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.

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

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]"
+

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

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":

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

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
+
+

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]"
 

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 <pm.bridge.qa@gmail.com>
+
+      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 <pm.bridge.qa@gmail.com>
+
+      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 <pm.bridge.qa@gmail.com>
+
+      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 <pm.bridge.qa@gmail.com>
+
+      hello
+
+      """
+    Then it fails

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)