From 7378414e11ae4526d5629b9269ab162f22d22a6c Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Mon, 11 Jul 2016 09:16:32 -0700 Subject: [PATCH] smtp: make "auth" and "port" config fields optional Use "plain" auth if and only if username and password are provided. Parse port from "host" field if it's provided. --- Documentation/email-configuration.md | 6 +- email/smtp.go | 50 +++++++--- email/smtp_test.go | 144 +++++++++++++++++++++++++++ 3 files changed, 182 insertions(+), 18 deletions(-) create mode 100644 email/smtp_test.go diff --git a/Documentation/email-configuration.md b/Documentation/email-configuration.md index 82008096..2d86f213 100644 --- a/Documentation/email-configuration.md +++ b/Documentation/email-configuration.md @@ -12,14 +12,12 @@ specific fields. If using SMTP the `type` field **must** be set to `smtp`. Additionally both `host` and `port` are required. If you wish to use SMTP plain auth, then -set `auth` to `plain` and specify your username and password. +specify your username and password. ``` { "type": "smtp", - "host": "smtp.example.org", - "port": 587, - "auth": "plain", + "host": "smtp.example.org:587", "from": "postmaster@example.com", "username": "postmaster@example.org", "password": "foo" diff --git a/email/smtp.go b/email/smtp.go index 002c44eb..b3226ba4 100644 --- a/email/smtp.go +++ b/email/smtp.go @@ -3,6 +3,9 @@ package email import ( "encoding/json" "errors" + "fmt" + "net" + "strconv" "gopkg.in/gomail.v2" ) @@ -17,11 +20,16 @@ func init() { type SmtpEmailerConfig struct { Host string `json:"host"` - Port int `json:"port"` - Auth string `json:"auth"` Username string `json:"username"` Password string `json:"password"` FromAddr string `json:"from"` + + // OPTIONAL: If empty and host is of form "host:port" just use that. For backward + // compatibility do not change this. + Port int `json:"port"` + + // DEPRICATED: If "username" and "password" are provided, use them. + Auth string `json:"auth"` } func (cfg SmtpEmailerConfig) EmailerType() string { @@ -37,24 +45,38 @@ func (cfg SmtpEmailerConfig) Emailer(fromAddr string) (Emailer, error) { if from == "" { from = fromAddr } - if from == "" { return nil, errors.New(`missing "from" field in email config`) } - var dialer *gomail.Dialer - if cfg.Auth == "plain" { - dialer = gomail.NewPlainDialer(cfg.Host, cfg.Port, cfg.Username, cfg.Password) - } else { - dialer = &gomail.Dialer{ - Host: cfg.Host, - Port: cfg.Port, + host, port := cfg.Host, cfg.Port + + // If port hasn't been supplied, check the "host" field. + if port == 0 { + hostStr, portStr, err := net.SplitHostPort(cfg.Host) + if err != nil { + return nil, fmt.Errorf(`"host" must be in format of "host:port" %v`, err) + } + host = hostStr + if port, err = strconv.Atoi(portStr); err != nil { + return nil, fmt.Errorf(`failed to parse %q as "host:port" %v`, cfg.Host, err) } } - return &smtpEmailer{ - dialer: dialer, - from: from, - }, nil + + if (cfg.Username == "") != (cfg.Password == "") { + return nil, errors.New(`must provide both "username" and "password"`) + } + + var dialer *gomail.Dialer + if cfg.Username == "" { + // NOTE(ericchiang): Guess SSL using the same logic as gomail. We should + // eventually allow this to be set explicitly. + dialer = &gomail.Dialer{Host: host, Port: port, SSL: port == 465} + } else { + dialer = gomail.NewPlainDialer(host, port, cfg.Username, cfg.Password) + } + + return &smtpEmailer{dialer: dialer, from: from}, nil } type smtpEmailerConfig SmtpEmailerConfig diff --git a/email/smtp_test.go b/email/smtp_test.go new file mode 100644 index 00000000..0331fa8c --- /dev/null +++ b/email/smtp_test.go @@ -0,0 +1,144 @@ +package email + +import ( + "encoding/json" + "strconv" + "testing" + + "github.com/kylelemons/godebug/pretty" + + "gopkg.in/gomail.v2" +) + +func TestNewSmtpEmailer(t *testing.T) { + // If (and only if) this port is provided, gomail assumes SSL. + gomailSSLPort := 465 + + tests := []struct { + config SmtpEmailerConfig + + // formAddr set by the dex-worker flag + fromAddrFlag string + + wantEmailer Emailer + wantErr bool + }{ + { + config: SmtpEmailerConfig{ + Host: "example.com:" + strconv.Itoa(gomailSSLPort), + FromAddr: "foo@example.com", + }, + wantEmailer: &smtpEmailer{ + from: "foo@example.com", + dialer: &gomail.Dialer{ + Host: "example.com", + Port: gomailSSLPort, + SSL: true, + }, + }, + }, + { + config: SmtpEmailerConfig{ + Host: "example.com", + Port: gomailSSLPort, + FromAddr: "foo@example.com", + }, + wantEmailer: &smtpEmailer{ + from: "foo@example.com", + dialer: &gomail.Dialer{ + Host: "example.com", + Port: gomailSSLPort, + SSL: true, + }, + }, + }, + { + config: SmtpEmailerConfig{ + Host: "example.com", + Port: 80, + FromAddr: "foo@example.com", + }, + wantEmailer: &smtpEmailer{ + from: "foo@example.com", + dialer: &gomail.Dialer{ + Host: "example.com", + Port: 80, + }, + }, + }, + { + // No port provided. + config: SmtpEmailerConfig{ + Host: "example.com", + FromAddr: "foo@example.com", + }, + wantErr: true, + }, + { + config: SmtpEmailerConfig{ + Host: "example.com", + Port: 80, + FromAddr: "foo@example.com", + }, + fromAddrFlag: "bar@example.com", + wantEmailer: &smtpEmailer{ + from: "foo@example.com", // config should override flag. + dialer: &gomail.Dialer{ + Host: "example.com", + Port: 80, + }, + }, + }, + { + // No fromAddr provided as a flag or in config. + config: SmtpEmailerConfig{Host: "example.com"}, + wantErr: true, + }, + { + config: SmtpEmailerConfig{ + Host: "example.com", + Port: 80, + Username: "foo", + Password: "bar", + FromAddr: "foo@example.com", + }, + wantEmailer: &smtpEmailer{ + from: "foo@example.com", // config should override flag. + dialer: gomail.NewPlainDialer("example.com", 80, "foo", "bar"), + }, + }, + { + // Password provided without username. + config: SmtpEmailerConfig{ + Host: "example.com", + Port: 80, + Password: "bar", + FromAddr: "foo@example.com", + }, + wantErr: true, + }, + } + + for i, tt := range tests { + testCase, err := json.MarshalIndent(tt.config, "", " ") + if err != nil { + t.Fatal(err) + } + + emailer, err := tt.config.Emailer(tt.fromAddrFlag) + if err != nil { + if !tt.wantErr { + t.Errorf("case %d %s.Emailer(): %v", i, testCase, err) + } + continue + } + if tt.wantErr { + t.Errorf("case %d %s.Emailer(): expected error creating emailer", i, testCase) + continue + } + + if diff := pretty.Compare(emailer, tt.wantEmailer); diff != "" { + t.Errorf("case %d: unexpected emailer %s", i, diff) + } + } +}