Repository: emersion/go-smtp
Pull request #64 (2019-09-21 23:23:37)

client: Return SMTPError for server errors
Was briefly mentioned in #36 but left unimplemented.

codecov[bot]: # [Codecov](https://codecov.io/gh/emersion/go-smtp/pull/64?src=pr&el=h1) Report
> Merging [#64](https://codecov.io/gh/emersion/go-smtp/pull/64?src=pr&el=desc) into [master](https://codecov.io/gh/emersion/go-smtp/commit/04984480e0a5c0f8646a33942ac02b999882a58c?src=pr&el=desc) will **decrease** coverage by `0.3%`.
> The diff coverage is `71.42%`.

[![Impacted file tree graph](https://codecov.io/gh/emersion/go-smtp/pull/64/graphs/tree.svg?width=650&token=vj7HO8ABwC&height=150&src=pr)](https://codecov.io/gh/emersion/go-smtp/pull/64?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
- Coverage   63.36%   63.06%   -0.31%     
==========================================
  Files           8        7       -1     
  Lines         767      777      +10     
==========================================
+ Hits          486      490       +4     
- Misses        205      210       +5     
- Partials       76       77       +1
```


| [Impacted Files](https://codecov.io/gh/emersion/go-smtp/pull/64?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [data.go](https://codecov.io/gh/emersion/go-smtp/pull/64/diff?src=pr&el=tree#diff-ZGF0YS5nbw==) | `100% <ø> (+10.52%)` | :arrow_up: |
| [client.go](https://codecov.io/gh/emersion/go-smtp/pull/64/diff?src=pr&el=tree#diff-Y2xpZW50Lmdv) | `67.27% <71.42%> (+0.32%)` | :arrow_up: |
| [server.go](https://codecov.io/gh/emersion/go-smtp/pull/64/diff?src=pr&el=tree#diff-c2VydmVyLmdv) | `52.32% <0%> (-2.12%)` | :arrow_down: |
| [conn.go](https://codecov.io/gh/emersion/go-smtp/pull/64/diff?src=pr&el=tree#diff-Y29ubi5nbw==) | `59.87% <0%> (-1.21%)` | :arrow_down: |
| [lengthlimit\_reader.go](https://codecov.io/gh/emersion/go-smtp/pull/64/diff?src=pr&el=tree#diff-bGVuZ3RobGltaXRfcmVhZGVyLmdv) | | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/emersion/go-smtp/pull/64?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/emersion/go-smtp/pull/64?src=pr&el=footer). Last update [0498448...fd9d45c](https://codecov.io/gh/emersion/go-smtp/pull/64?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/client.go b/client.go
index afaf608..43a9e01 100644
--- a/client.go
+++ b/client.go
@@ -12,6 +12,7 @@ import (
 	"io"
 	"net"
 	"net/textproto"
+	"strconv"
 	"strings"
 
 	"github.com/emersion/go-sasl"
@@ -68,6 +69,9 @@ func NewClient(conn net.Conn, host string) (*Client, error) {
 	_, _, err := text.ReadResponse(220)
 	if err != nil {
 		text.Close()
+		if protoErr, ok := err.(*textproto.Error); ok {
+			return nil, toSMTPErr(protoErr)
+		}
 		return nil, err
 	}
 	_, isTLS := conn.(*tls.Conn)
@@ -108,6 +112,8 @@ func (c *Client) hello() error {
 // over the host name used. The client will introduce itself as "localhost"
 // automatically otherwise. If Hello is called, it must be called before
 // any of the other methods.
+//
+// If server returns an error, it will be of type *SMTPError.
 func (c *Client) Hello(localName string) error {
 	if err := validateLine(localName); err != nil {
 		return err
@@ -120,6 +126,7 @@ func (c *Client) Hello(localName string) error {
 }
 
 // cmd is a convenience function that sends a command and returns the response
+// textproto.Error returned by c.Text.ReadResponse is converted into SMTPError.
 func (c *Client) cmd(expectCode int, format string, args ...interface{}) (int, string, error) {
 	id, err := c.Text.Cmd(format, args...)
 	if err != nil {
@@ -128,7 +135,14 @@ func (c *Client) cmd(expectCode int, format string, args ...interface{}) (int, s
 	c.Text.StartResponse(id)
 	defer c.Text.EndResponse(id)
 	code, msg, err := c.Text.ReadResponse(expectCode)
-	return code, msg, err
+	if err != nil {
+		if protoErr, ok := err.(*textproto.Error); ok {
+			smtpErr := toSMTPErr(protoErr)
+			return code, smtpErr.Message, smtpErr
+		}
+		return code, msg, err
+	}
+	return code, msg, nil
 }
 
 // helo sends the HELO greeting to the server. It should be used only when the
@@ -172,6 +186,8 @@ func (c *Client) ehlo() error {
 
 // StartTLS sends the STARTTLS command and encrypts all further communication.
 // Only servers that advertise the STARTTLS extension support this function.
+//
+// If server returns an error, it will be of type *SMTPError.
 func (c *Client) StartTLS(config *tls.Config) error {
 	if err := c.hello(); err != nil {
 		return err
@@ -212,6 +228,8 @@ func (c *Client) TLSConnectionState() (state tls.ConnectionState, ok bool) {
 // If Verify returns nil, the address is valid. A non-nil return
 // does not necessarily indicate an invalid address. Many servers
 // will not verify addresses for security reasons.
+//
+// If server returns an error, it will be of type *SMTPError.
 func (c *Client) Verify(addr string) error {
 	if err := validateLine(addr); err != nil {
 		return err
@@ -226,6 +244,8 @@ func (c *Client) Verify(addr string) error {
 // Auth authenticates a client using the provided authentication mechanism.
 // A failed authentication closes the connection.
 // Only servers that advertise the AUTH extension support this function.
+//
+// If server returns an error, it will be of type *SMTPError.
 func (c *Client) Auth(a sasl.Client) error {
 	if err := c.hello(); err != nil {
 		return err
@@ -248,7 +268,7 @@ func (c *Client) Auth(a sasl.Client) error {
 			// the last message isn't base64 because it isn't a challenge
 			msg = []byte(msg64)
 		default:
-			err = &textproto.Error{Code: code, Msg: msg64}
+			err = toSMTPErr(&textproto.Error{Code: code, Msg: msg64})
 		}
 		if err == nil {
 			if code == 334 {
@@ -277,6 +297,8 @@ func (c *Client) Auth(a sasl.Client) error {
 // If the server supports the 8BITMIME extension, Mail adds the BODY=8BITMIME
 // parameter.
 // This initiates a mail transaction and is followed by one or more Rcpt calls.
+//
+// If server returns an error, it will be of type *SMTPError.
 func (c *Client) Mail(from string) error {
 	if err := validateLine(from); err != nil {
 		return err
@@ -297,6 +319,8 @@ func (c *Client) Mail(from string) error {
 // Rcpt issues a RCPT command to the server using the provided email address.
 // A call to Rcpt must be preceded by a call to Mail and may be followed by
 // a Data call or another Rcpt call.
+//
+// If server returns an error, it will be of type *SMTPError.
 func (c *Client) Rcpt(to string) error {
 	if err := validateLine(to); err != nil {
 		return err
@@ -318,6 +342,9 @@ func (d *dataCloser) Close() error {
 	if d.c.lmtp {
 		for d.c.rcptToCount > 0 {
 			if _, _, err := d.c.Text.ReadResponse(250); err != nil {
+				if protoErr, ok := err.(*textproto.Error); ok {
+					return toSMTPErr(protoErr)
+				}
 				return err
 			}
 			d.c.rcptToCount--
@@ -325,7 +352,13 @@ func (d *dataCloser) Close() error {
 		return nil
 	} else {
 		_, _, err := d.c.Text.ReadResponse(250)
-		return err
+		if err != nil {
+			if protoErr, ok := err.(*textproto.Error); ok {
+				return toSMTPErr(protoErr)
+			}
+			return err
+		}
+		return nil
 	}
 }
 
@@ -333,6 +366,8 @@ func (d *dataCloser) Close() error {
 // can be used to write the mail headers and body. The caller should
 // close the writer before calling any more methods on c. A call to
 // Data must be preceded by one or more calls to Rcpt.
+//
+// If server returns an error, it will be of type *SMTPError.
 func (c *Client) Data() (io.WriteCloser, error) {
 	_, _, err := c.cmd(354, "DATA")
 	if err != nil {
@@ -466,3 +501,46 @@ func (c *Client) Quit() error {
 	}
 	return c.Text.Close()
 }
+
+func parseEnhancedCode(s string) (EnhancedCode, error) {
+	parts := strings.Split(s, ".")
+	if len(parts) != 3 {
+		return EnhancedCode{}, fmt.Errorf("wrong amount of enhanced code parts")
+	}
+
+	code := EnhancedCode{}
+	for i, part := range parts {
+		num, err := strconv.Atoi(part)
+		if err != nil {
+			return code, err
+		}
+		code[i] = num
+	}
+	return code, nil
+}
+
+// toSMTPErr converts textproto.Error into SMTPError, parsing
+// enhanced status code if it is present.
+func toSMTPErr(protoErr *textproto.Error) *SMTPError {
+	if protoErr == nil {
+		return nil
+	}
+	smtpErr := &SMTPError{
+		Code:    protoErr.Code,
+		Message: protoErr.Msg,
+	}
+
+	parts := strings.SplitN(protoErr.Msg, " ", 2)
+	if len(parts) != 2 {
+		return smtpErr
+	}
+
+	enchCode, err := parseEnhancedCode(parts[0])
+	if err != nil {
+		return smtpErr
+	}
+
+	smtpErr.EnhancedCode = enchCode
+	smtpErr.Message = parts[1]
+	return smtpErr
+}
diff --git a/data.go b/data.go
index d72be0c..27f4887 100644
--- a/data.go
+++ b/data.go
@@ -6,7 +6,8 @@ import (
 
 type EnhancedCode [3]int
 
-// SMTPError specifies the error code and message that needs to be returned to the client
+// SMTPError specifies the error code, enhanced error code (if any) and
+// message returned by the server.
 type SMTPError struct {
 	Code         int
 	EnhancedCode EnhancedCode


Test patch:
diff --git a/client_test.go b/client_test.go
index 8425c1d..27f493c 100644
--- a/client_test.go
+++ b/client_test.go
@@ -161,6 +161,66 @@ Goodbye.`
 	}
 }
 
+func TestBasic_SMTPError(t *testing.T) {
+	faultyServer := `220 mx.google.com at your service
+250-mx.google.com at your service
+250 ENHANCEDSTATUSCODES
+500 5.0.0 Failing with enhanced code
+500 Failing without enhanced code`
+	// RFC 2034 says that enhanced codes *SHOULD* be included in errors,
+	// this means it can be violated hence we need to handle last
+	// case properly.
+
+	faultyServer = strings.Join(strings.Split(faultyServer, "\n"), "\r\n")
+
+	var wrote bytes.Buffer
+	var fake faker
+	fake.ReadWriter = struct {
+		io.Reader
+		io.Writer
+	}{
+		strings.NewReader(faultyServer),
+		&wrote,
+	}
+	c, err := NewClient(fake, "fake.host")
+	if err != nil {
+		t.Fatalf("NewClient failed: %v", err)
+	}
+
+	err = c.Mail("whatever")
+	if err == nil {
+		t.Fatal("MAIL succeded")
+	}
+	smtpErr, ok := err.(*SMTPError)
+	if !ok {
+		t.Fatal("Returned error is not SMTPError")
+	}
+	if smtpErr.Code != 500 {
+		t.Fatalf("Wrong status code, got %d, want %d", smtpErr.Code, 500)
+	}
+	if smtpErr.EnhancedCode != (EnhancedCode{5, 0, 0}) {
+		t.Fatalf("Wrong enhanced code, got %v, want %v", smtpErr.EnhancedCode, EnhancedCode{5, 0, 0})
+	}
+	if smtpErr.Message != "Failing with enhanced code" {
+		t.Fatalf("Wrong message, got %s, want %s", smtpErr.Message, "Failing with enhanced code")
+	}
+
+	err = c.Mail("whatever")
+	if err == nil {
+		t.Fatal("MAIL succeded")
+	}
+	smtpErr, ok = err.(*SMTPError)
+	if !ok {
+		t.Fatal("Returned error is not SMTPError")
+	}
+	if smtpErr.Code != 500 {
+		t.Fatalf("Wrong status code, got %d, want %d", smtpErr.Code, 500)
+	}
+	if smtpErr.Message != "Failing without enhanced code" {
+		t.Fatalf("Wrong message, got %s, want %s", smtpErr.Message, "Failing without enhanced code")
+	}
+}
+
 var basicServer = `250 mx.google.com at your service
 502 Unrecognized command.
 250-mx.google.com at your service
@@ -315,7 +375,7 @@ func TestHello(t *testing.T) {
 			err = c.Hello("customhost")
 		case 1:
 			err = c.StartTLS(nil)
-			if err.Error() == "502 Not implemented" {
+			if err.Error() == "Not implemented" {
 				err = nil
 			}
 		case 2:
@@ -561,8 +621,8 @@ func TestAuthFailed(t *testing.T) {
 
 	if err == nil {
 		t.Error("Auth: expected error; got none")
-	} else if err.Error() != "535 Invalid credentials\nplease see www.example.com" {
-		t.Errorf("Auth: got error: %v, want: %s", err, "535 Invalid credentials\nplease see www.example.com")
+	} else if err.Error() != "Invalid credentials\nplease see www.example.com" {
+		t.Errorf("Auth: got error: %v, want: %s", err, "Invalid credentials\nplease see www.example.com")
 	}
 
 	bcmdbuf.Flush()