Aug 30, 2024 · Aug 30, 2024 · Aug 30, 2024 · Aug 30, 2024 · Aug 30, 2024 · Aug 30, 2024
diff --git a/coderd/notifications/dispatch/smtp.go b/coderd/notifications/dispatch/smtp.go
 if err != nil {
 return true, xerrors.Errorf("message transmission: %w", err)
 }
 defer message.Close()

 // Create message headers.
 msg := &bytes.Buffer{}
 return false, xerrors.Errorf("write body buffer: %w", err)
 }

 if err = message.Close(); err != nil {
 return true, xerrors.Errorf("delivery failure: %w", err)
 }

 // Returning false, nil indicates successful send (i.e. non-retryable non-error)
 return false, nil
 }
diff --git a/coderd/notifications/dispatch/smtp_test.go b/coderd/notifications/dispatch/smtp_test.go
 expectedErr      string
 retryable        bool
 useTLS           bool
 failOnDataFn     func() error
 }{
 /**
 * LOGIN auth mechanism
 toAddrs:          []string{to},
 expectedAuthMeth: sasl.Plain,
 },
 /**
 * Other errors
 */
 {
 name: "Rejected on DATA",
 cfg: codersdk.NotificationsEmailConfig{
 Hello: hello,
 From:  from,
 },
 failOnDataFn: func() error {
 return &smtp.SMTPError{Code: 501, EnhancedCode: smtp.EnhancedCode{5, 5, 4}, Message: "Rejected!"}
 },
 expectedErr: "SMTP error 501: Rejected!",
 retryable:   true,
 },
 }

 // nolint:paralleltest // Reinitialization is not required as of Go v1.22.
 AcceptedIdentity: tc.cfg.Auth.Identity.String(),
 AcceptedUsername: username,
 AcceptedPassword: password,

 FailOnDataFn: tc.failOnDataFn,
 })

 // Create a mock SMTP server which conditionally listens for plain or TLS connections.
diff --git a/coderd/notifications/dispatch/smtp_util_test.go b/coderd/notifications/dispatch/smtp_util_test.go
 type Config struct {
 AuthMechanisms                                       []string
 AcceptedIdentity, AcceptedUsername, AcceptedPassword string
 FailOnDataFn                                         func() error
 }

 type Message struct {
 return err
 }

 if s.backend.cfg.FailOnDataFn != nil {
 return s.backend.cfg.FailOnDataFn()
 }

 s.backend.lastMsg.Contents = string(b)

 return nil
Original file line number	Diff line number	Diff line change
Expand Up		@@ -183,7 +183,6 @@ func (s *SMTPHandler) dispatch(subject, htmlBody, plainBody, to string) Delivery
		if err != nil {
		return true, xerrors.Errorf("message transmission: %w", err)
		}
		defer message.Close()

		// Create message headers.
		msg := &bytes.Buffer{}
Expand DownExpand Up		@@ -251,6 +250,10 @@ func (s *SMTPHandler) dispatch(subject, htmlBody, plainBody, to string) Delivery
		return false, xerrors.Errorf("write body buffer: %w", err)
		}

		if err = message.Close(); err != nil {
		return true, xerrors.Errorf("delivery failure: %w", err)
		}

		// Returning false, nil indicates successful send (i.e. non-retryable non-error)
		return false, nil
		}
Expand Down
Original file line number	Diff line number	Diff line change
Expand Up		@@ -62,6 +62,7 @@ func TestSMTP(t *testing.T) {
		expectedErr string
		retryable bool
		useTLS bool
		failOnDataFn func() error
		}{
		/**
		* LOGIN auth mechanism
Expand DownExpand Up		@@ -381,6 +382,21 @@ func TestSMTP(t *testing.T) {
		toAddrs: []string{to},
		expectedAuthMeth: sasl.Plain,
		},
		/**
		* Other errors
		*/
		{
		name: "Rejected on DATA",
		cfg: codersdk.NotificationsEmailConfig{
		Hello: hello,
		From: from,
		},
		failOnDataFn: func() error {
		return &smtp.SMTPError{Code: 501, EnhancedCode: smtp.EnhancedCode{5, 5, 4}, Message: "Rejected!"}
		},
		expectedErr: "SMTP error 501: Rejected!",
		retryable: true,
Copy link Member johnstcnAug 30, 2024 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. Isn't this error non-retryable? Copy link ContributorAuthor dannykoppingAug 30, 2024 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. Not apriori; it could be a temporary failure.
		},
		}

		// nolint:paralleltest // Reinitialization is not required as of Go v1.22.
Expand All		@@ -398,6 +414,8 @@ func TestSMTP(t *testing.T) {
		AcceptedIdentity: tc.cfg.Auth.Identity.String(),
		AcceptedUsername: username,
		AcceptedPassword: password,

		FailOnDataFn: tc.failOnDataFn,
		})

		// Create a mock SMTP server which conditionally listens for plain or TLS connections.
Expand Down
Original file line number	Diff line number	Diff line change
Expand Up		@@ -24,6 +24,7 @@ var (
		type Config struct {
		AuthMechanisms []string
		AcceptedIdentity, AcceptedUsername, AcceptedPassword string
		FailOnDataFn func() error
		}

		type Message struct {
Expand DownExpand Up		@@ -147,6 +148,10 @@ func (s *Session) Data(r io.Reader) error {
		return err
		}

		if s.backend.cfg.FailOnDataFn != nil {
		return s.backend.cfg.FailOnDataFn()
		}
Comment on lines +151 to +153 Copy link Member johnstcnAug 30, 2024 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. At some point in the future, would it make sense to test with something like`mocktools/go-smtp-mock` instead? Copy link ContributorAuthor dannykoppingAug 30, 2024 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. I evaluated it, but it does not support authentication.

		s.backend.lastMsg.Contents = string(b)

		return nil
Expand Down