From 87faa5a1f76514b901ba79622144c5f41b71fcd9 Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Mon, 27 Jun 2016 23:34:55 -0700 Subject: [PATCH 1/2] *: depricate --email-from flag and move to email config files --- Documentation/email-configuration.md | 4 +++- cmd/dex-worker/main.go | 6 ++++- email/interface.go | 32 +++++++++++++++++++++------ email/mailgun.go | 21 +++++++++++++----- email/mailgun_test.go | 33 +++++++++++++++++++++++----- email/smtp.go | 18 ++++++++++++--- email/template.go | 14 +++++++----- email/template_test.go | 10 +++------ server/config.go | 5 ++--- server/password_test.go | 13 +++++------ server/testutil_test.go | 4 ++-- static/fixtures/emailer.json.sample | 5 +++-- user/email/email.go | 9 +++----- user/email/email_test.go | 21 +++++------------- 14 files changed, 123 insertions(+), 72 deletions(-) diff --git a/Documentation/email-configuration.md b/Documentation/email-configuration.md index 2a5d4b04..82008096 100644 --- a/Documentation/email-configuration.md +++ b/Documentation/email-configuration.md @@ -20,6 +20,7 @@ set `auth` to `plain` and specify your username and password. "host": "smtp.example.org", "port": 587, "auth": "plain", + "from": "postmaster@example.com", "username": "postmaster@example.org", "password": "foo" } @@ -33,6 +34,7 @@ If using Mailgun the `type` field **must** be set to `mailgun`. Additionally ``` { "type": "mailgun", + "from": "noreply@example.com", "privateAPIKey": "key-XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX", "publicAPIKey": "YYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYY", "domain": "sandboxZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ.mailgun.org" @@ -49,4 +51,4 @@ emailer the `type` field **must** be set to `fake`. { "type": "fake" } -``` \ No newline at end of file +``` diff --git a/cmd/dex-worker/main.go b/cmd/dex-worker/main.go index 234c96f6..2b4f2c67 100644 --- a/cmd/dex-worker/main.go +++ b/cmd/dex-worker/main.go @@ -43,7 +43,7 @@ func main() { emailTemplateDirs := flagutil.StringSliceFlag{"./static/email"} fs.Var(&emailTemplateDirs, "email-templates", "comma separated list of directories of email template files") - emailFrom := fs.String("email-from", "", "emails sent from dex will come from this address") + emailFrom := fs.String("email-from", "", `DEPRICATED: use "from" field in email config.`) emailConfig := fs.String("email-cfg", "./static/fixtures/emailer.json", "configures emailer.") enableRegistration := fs.Bool("enable-registration", false, "Allows users to self-register. This flag cannot be used in combination with --enable-automatic-registration.") @@ -132,6 +132,10 @@ func main() { log.Fatalf("Only 'http' and 'https' schemes are supported") } + if *emailFrom != "" { + log.Errorf(`--email-from flag is depricated. Use "from" field in email config.`) + } + scfg := server.ServerConfig{ IssuerURL: *issuer, TemplateDir: *templates, diff --git a/email/interface.go b/email/interface.go index 05b9ed92..9e4ec30e 100644 --- a/email/interface.go +++ b/email/interface.go @@ -28,14 +28,20 @@ type Emailer interface { // SendMail queues an email to be sent to 1 or more recipients. // At least one of "text" or "html" must not be blank. If text is blank, but // html is not, then an html-only email should be sent and vice-versal. - SendMail(from, subject, text, html string, to ...string) error + SendMail(subject, text, html string, to ...string) error } //go:generate genconfig -o config.go email Emailer type EmailerConfig interface { EmailerID() string EmailerType() string - Emailer() (Emailer, error) + + // Because emailers can either be configured through command line flags or through + // JSON configs, we need a way to set the fromAddr when initializing the emailer. + // + // Values passed in the JSON config should override this value. Supplying neither + // should result in an error. + Emailer(fromAddress string) (Emailer, error) } func newEmailerConfigFromReader(r io.Reader) (EmailerConfig, error) { @@ -65,6 +71,7 @@ func NewEmailerConfigFromFile(loc string) (EmailerConfig, error) { } type FakeEmailerConfig struct { + FromAddr string `json:"from"` } func (cfg FakeEmailerConfig) EmailerType() string { @@ -75,15 +82,26 @@ func (cfg FakeEmailerConfig) EmailerID() string { return FakeEmailerType } -func (cfg FakeEmailerConfig) Emailer() (Emailer, error) { - return FakeEmailer{}, nil +func (cfg FakeEmailerConfig) Emailer(fromAddr string) (Emailer, error) { + from := cfg.FromAddr + if from == "" { + from = fromAddr + } + if from == "" { + // Since the emailer just prints to stdout, the actual value doesn't matter. + from = "noreply@example.com" + } + + return FakeEmailer{from}, nil } // FakeEmailer is an Emailer that writes emails to stdout. Should only be used in development. -type FakeEmailer struct{} +type FakeEmailer struct { + from string +} -func (f FakeEmailer) SendMail(from, subject, text, html string, to ...string) error { - fmt.Printf("From: %v\n", from) +func (f FakeEmailer) SendMail(subject, text, html string, to ...string) error { + fmt.Printf("From: %v\n", f.from) fmt.Printf("Subject: %v\n", subject) fmt.Printf("To: %v\n", strings.Join(to, ",")) fmt.Printf("Body(text): %v\n", text) diff --git a/email/mailgun.go b/email/mailgun.go index 8dd121ce..b7f53be4 100644 --- a/email/mailgun.go +++ b/email/mailgun.go @@ -17,6 +17,7 @@ func init() { } type MailgunEmailerConfig struct { + FromAddr string `json:"from"` PrivateAPIKey string `json:"privateAPIKey"` PublicAPIKey string `json:"publicAPIKey"` Domain string `json:"domain"` @@ -30,10 +31,19 @@ func (cfg MailgunEmailerConfig) EmailerID() string { return MailgunEmailerType } -func (cfg MailgunEmailerConfig) Emailer() (Emailer, error) { +func (cfg MailgunEmailerConfig) Emailer(fromAddr string) (Emailer, error) { + from := cfg.FromAddr + if from == "" { + from = fromAddr + } + + if from == "" { + return nil, errors.New(`missing "from" field in email config`) + } mg := mailgun.NewMailgun(cfg.Domain, cfg.PrivateAPIKey, cfg.PublicAPIKey) return &mailgunEmailer{ - mg: mg, + mg: mg, + from: from, }, nil } @@ -64,11 +74,12 @@ func (cfg *MailgunEmailerConfig) UnmarshalJSON(data []byte) error { } type mailgunEmailer struct { - mg mailgun.Mailgun + mg mailgun.Mailgun + from string } -func (m *mailgunEmailer) SendMail(from, subject, text, html string, to ...string) error { - msg := m.mg.NewMessage(from, subject, text, to...) +func (m *mailgunEmailer) SendMail(subject, text, html string, to ...string) error { + msg := m.mg.NewMessage(m.from, subject, text, to...) if html != "" { msg.SetHtml(html) } diff --git a/email/mailgun_test.go b/email/mailgun_test.go index 3c299b69..9868af52 100644 --- a/email/mailgun_test.go +++ b/email/mailgun_test.go @@ -9,20 +9,22 @@ import ( func TestNewEmailConfigFromReader(t *testing.T) { tests := []struct { - json string - want MailgunEmailerConfig - wantErr bool + json string + want MailgunEmailerConfig + wantErr bool + wantInitErr bool // want error when calling Emailer() with no fromAddr }{ { - json: `{"type":"mailgun","id":"mg","privateAPIKey":"private","publicAPIKey":"public","domain":"example.com"}`, + json: `{"type":"mailgun","id":"mg","privateAPIKey":"private","publicAPIKey":"public","domain":"example.com","from":"admin@example.com"}`, want: MailgunEmailerConfig{ PrivateAPIKey: "private", PublicAPIKey: "public", Domain: "example.com", + FromAddr: "admin@example.com", }, }, { - json: `{"type":"mailgun","id":"mg","publicAPIKey":"public","domain":"example.com"}`, + json: `{"type":"mailgun","id":"mg","publicAPIKey":"public","domain":"example.com",""}`, wantErr: true, }, { @@ -33,6 +35,18 @@ func TestNewEmailConfigFromReader(t *testing.T) { json: `{"type":"mailgun","id":"mg","privateAPIKey":"private","domain":"example.com"}`, wantErr: true, }, + { + json: `{"type":"mailgun","id":"mg","privateAPIKey":"private","publicAPIKey":"public","domain":"example.com"}`, + want: MailgunEmailerConfig{ + PrivateAPIKey: "private", + PublicAPIKey: "public", + Domain: "example.com", + }, + + // No fromAddr email provided. Calling Emailer("") should error since fromAddr needs to be provided + // in the config or as a command line argument. + wantInitErr: true, + }, } for i, tt := range tests { @@ -42,7 +56,6 @@ func TestNewEmailConfigFromReader(t *testing.T) { if err == nil { t.Errorf("case %d: want non-nil err.", i) } - t.Logf("WHAT: %v", err) continue } if err != nil { @@ -52,5 +65,13 @@ func TestNewEmailConfigFromReader(t *testing.T) { if diff := pretty.Compare(tt.want, ec); diff != "" { t.Errorf("case %d: Compare(want, got): %v", i, diff) } + + _, err = ec.Emailer("") + if err != nil && !tt.wantInitErr { + t.Errorf("case %d: failed to initialize emailer: %v", i, err) + } + if err == nil && tt.wantInitErr { + t.Errorf("case %d: expected error initializing emailer", i) + } } } diff --git a/email/smtp.go b/email/smtp.go index 22a8b539..002c44eb 100644 --- a/email/smtp.go +++ b/email/smtp.go @@ -21,6 +21,7 @@ type SmtpEmailerConfig struct { Auth string `json:"auth"` Username string `json:"username"` Password string `json:"password"` + FromAddr string `json:"from"` } func (cfg SmtpEmailerConfig) EmailerType() string { @@ -31,7 +32,16 @@ func (cfg SmtpEmailerConfig) EmailerID() string { return SmtpEmailerType } -func (cfg SmtpEmailerConfig) Emailer() (Emailer, error) { +func (cfg SmtpEmailerConfig) Emailer(fromAddr string) (Emailer, error) { + from := cfg.FromAddr + 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) @@ -43,6 +53,7 @@ func (cfg SmtpEmailerConfig) Emailer() (Emailer, error) { } return &smtpEmailer{ dialer: dialer, + from: from, }, nil } @@ -66,11 +77,12 @@ func (cfg *SmtpEmailerConfig) UnmarshalJSON(data []byte) error { type smtpEmailer struct { dialer *gomail.Dialer + from string } -func (emailer *smtpEmailer) SendMail(from, subject, text, html string, to ...string) error { +func (emailer *smtpEmailer) SendMail(subject, text, html string, to ...string) error { msg := gomail.NewMessage() - msg.SetHeader("From", from) + msg.SetHeader("From", emailer.from) msg.SetHeader("To", to...) msg.SetHeader("Subject", subject) msg.SetBody("text/plain", text) diff --git a/email/template.go b/email/template.go index d992eded..9fefae4e 100644 --- a/email/template.go +++ b/email/template.go @@ -8,7 +8,7 @@ import ( ) // NewTemplatizedEmailerFromGlobs creates a new TemplatizedEmailer, parsing the templates found in the given filepattern globs. -func NewTemplatizedEmailerFromGlobs(textGlob, htmlGlob string, emailer Emailer) (*TemplatizedEmailer, error) { +func NewTemplatizedEmailerFromGlobs(textGlob, htmlGlob string, emailer Emailer, fromAddr string) (*TemplatizedEmailer, error) { textTemplates, err := template.ParseGlob(textGlob) if err != nil { return nil, err @@ -19,15 +19,16 @@ func NewTemplatizedEmailerFromGlobs(textGlob, htmlGlob string, emailer Emailer) return nil, err } - return NewTemplatizedEmailerFromTemplates(textTemplates, htmlTemplates, emailer), nil + return NewTemplatizedEmailerFromTemplates(textTemplates, htmlTemplates, emailer, fromAddr), nil } // NewTemplatizedEmailerFromTemplates creates a new TemplatizedEmailer, given root text and html templates. -func NewTemplatizedEmailerFromTemplates(textTemplates *template.Template, htmlTemplates *htmltemplate.Template, emailer Emailer) *TemplatizedEmailer { +func NewTemplatizedEmailerFromTemplates(textTemplates *template.Template, htmlTemplates *htmltemplate.Template, emailer Emailer, fromAddr string) *TemplatizedEmailer { return &TemplatizedEmailer{ emailer: emailer, textTemplates: textTemplates, htmlTemplates: htmlTemplates, + fromAddr: fromAddr, } } @@ -37,6 +38,7 @@ type TemplatizedEmailer struct { htmlTemplates *htmltemplate.Template emailer Emailer globalCtx map[string]interface{} + fromAddr string } func (t *TemplatizedEmailer) SetGlobalContext(ctx map[string]interface{}) { @@ -48,7 +50,7 @@ func (t *TemplatizedEmailer) SetGlobalContext(ctx map[string]interface{}) { // the template names you want to base the message on instead of the actual // text. "to", "from" and "subject" will be added into the data map regardless // of if they are used. -func (t *TemplatizedEmailer) SendMail(from, subject, tplName string, data map[string]interface{}, to string) error { +func (t *TemplatizedEmailer) SendMail(subject, tplName string, data map[string]interface{}, to string) error { if tplName == "" { return errors.New("Must provide a template name") } @@ -61,7 +63,7 @@ func (t *TemplatizedEmailer) SendMail(from, subject, tplName string, data map[st } data["to"] = to - data["from"] = from + data["from"] = t.fromAddr data["subject"] = subject for k, v := range t.globalCtx { @@ -84,5 +86,5 @@ func (t *TemplatizedEmailer) SendMail(from, subject, tplName string, data map[st } } - return t.emailer.SendMail(from, subject, textBuffer.String(), htmlBuffer.String(), to) + return t.emailer.SendMail(subject, textBuffer.String(), htmlBuffer.String(), to) } diff --git a/email/template_test.go b/email/template_test.go index 59644172..99937a8f 100644 --- a/email/template_test.go +++ b/email/template_test.go @@ -22,8 +22,7 @@ type testEmailer struct { to []string } -func (t *testEmailer) SendMail(from, subject, text, html string, to ...string) error { - t.from = from +func (t *testEmailer) SendMail(subject, text, html string, to ...string) error { t.subject = subject t.text = text t.html = html @@ -118,12 +117,12 @@ func TestTemplatizedEmailSendMail(t *testing.T) { for i, tt := range tests { emailer := &testEmailer{} - templatizer := NewTemplatizedEmailerFromTemplates(textTemplates, htmlTemplates, emailer) + templatizer := NewTemplatizedEmailerFromTemplates(textTemplates, htmlTemplates, emailer, tt.from) if tt.ctx != nil { templatizer.SetGlobalContext(tt.ctx) } - err := templatizer.SendMail(tt.from, tt.subject, tt.tplName, tt.data, tt.to) + err := templatizer.SendMail(tt.subject, tt.tplName, tt.data, tt.to) if tt.wantErr { if err == nil { t.Errorf("case %d: err == nil, want non-nil err", i) @@ -131,9 +130,6 @@ func TestTemplatizedEmailSendMail(t *testing.T) { continue } - if emailer.from != tt.from { - t.Errorf("case %d: want=%q, got=%q", i, tt.from, emailer.from) - } if emailer.subject != tt.subject { t.Errorf("case %d: want=%q, got=%q", i, tt.subject, emailer.subject) } diff --git a/server/config.go b/server/config.go index 8698e253..35e19a96 100644 --- a/server/config.go +++ b/server/config.go @@ -327,7 +327,7 @@ func setEmailer(srv *Server, issuerName, fromAddress, emailerConfigFile string, return err } - emailer, err := cfg.Emailer() + emailer, err := cfg.Emailer(fromAddress) if err != nil { return err } @@ -371,7 +371,7 @@ func setEmailer(srv *Server, issuerName, fromAddress, emailerConfigFile string, return err } } - tMailer := email.NewTemplatizedEmailerFromTemplates(textTemplates, htmlTemplates, emailer) + tMailer := email.NewTemplatizedEmailerFromTemplates(textTemplates, htmlTemplates, emailer, fromAddress) tMailer.SetGlobalContext(map[string]interface{}{ "issuer_name": issuerName, }) @@ -382,7 +382,6 @@ func setEmailer(srv *Server, issuerName, fromAddress, emailerConfigFile string, srv.SessionManager.ValidityWindow, srv.IssuerURL, tMailer, - fromAddress, srv.absURL(httpPathResetPassword), srv.absURL(httpPathEmailVerify), srv.absURL(httpPathAcceptInvitation), diff --git a/server/password_test.go b/server/password_test.go index 41ce6467..d1af8865 100644 --- a/server/password_test.go +++ b/server/password_test.go @@ -101,7 +101,6 @@ func TestSendResetPasswordEmailHandler(t *testing.T) { wantCode: http.StatusOK, wantEmailer: &testEmailer{ to: str("email-1@example.com"), - from: "noreply@example.com", subject: "Reset Your Password", }, wantPRUserID: "ID-1", @@ -137,7 +136,6 @@ func TestSendResetPasswordEmailHandler(t *testing.T) { wantCode: http.StatusOK, wantEmailer: &testEmailer{ to: str("email-1@example.com"), - from: "noreply@example.com", subject: "Reset Your Password", }, wantPRPassword: "password", @@ -261,7 +259,7 @@ func TestSendResetPasswordEmailHandler(t *testing.T) { emailer := &testEmailer{ sent: make(chan struct{}), } - templatizer := email.NewTemplatizedEmailerFromTemplates(textTemplates, htmlTemplates, emailer) + templatizer := email.NewTemplatizedEmailerFromTemplates(textTemplates, htmlTemplates, emailer, "admin@example.com") f.srv.UserEmailer.SetEmailer(templatizer) hdlr := SendResetPasswordEmailHandler{ tpl: f.srv.SendResetPasswordEmailTemplate, @@ -584,13 +582,12 @@ func TestResetPasswordHandler(t *testing.T) { } type testEmailer struct { - from, subject, text, html string - to []string - sent chan struct{} + subject, text, html string + to []string + sent chan struct{} } -func (t *testEmailer) SendMail(from, subject, text, html string, to ...string) error { - t.from = from +func (t *testEmailer) SendMail(subject, text, html string, to ...string) error { t.subject = subject t.text = text t.html = html diff --git a/server/testutil_test.go b/server/testutil_test.go index 212a30ae..0ef7bfa0 100644 --- a/server/testutil_test.go +++ b/server/testutil_test.go @@ -198,7 +198,8 @@ func makeTestFixturesWithOptions(options testFixtureOptions) (*testFixtures, err emailer, err := email.NewTemplatizedEmailerFromGlobs( emailTemplatesLocation+"/*.txt", emailTemplatesLocation+"/*.html", - &email.FakeEmailer{}) + &email.FakeEmailer{}, + "admin@example.com") if err != nil { return nil, err } @@ -266,7 +267,6 @@ func makeTestFixturesWithOptions(options testFixtureOptions) (*testFixtures, err srv.SessionManager.ValidityWindow, srv.IssuerURL, emailer, - "noreply@example.com", srv.absURL(httpPathResetPassword), srv.absURL(httpPathEmailVerify), srv.absURL(httpPathAcceptInvitation), diff --git a/static/fixtures/emailer.json.sample b/static/fixtures/emailer.json.sample index 15a5cd26..8fde81a7 100644 --- a/static/fixtures/emailer.json.sample +++ b/static/fixtures/emailer.json.sample @@ -1,3 +1,4 @@ { - "type": "fake" -} \ No newline at end of file + "type": "fake", + "from": "noreply@example.com" +} diff --git a/user/email/email.go b/user/email/email.go index f9c5a6fb..1329ba10 100644 --- a/user/email/email.go +++ b/user/email/email.go @@ -19,7 +19,6 @@ type UserEmailer struct { tokenValidityWindow time.Duration issuerURL url.URL emailer *email.TemplatizedEmailer - fromAddress string passwordResetURL url.URL verifyEmailURL url.URL @@ -33,7 +32,6 @@ func NewUserEmailer(ur user.UserRepo, tokenValidityWindow time.Duration, issuerURL url.URL, emailer *email.TemplatizedEmailer, - fromAddress string, passwordResetURL url.URL, verifyEmailURL url.URL, invitationURL url.URL, @@ -45,7 +43,6 @@ func NewUserEmailer(ur user.UserRepo, tokenValidityWindow: tokenValidityWindow, issuerURL: issuerURL, emailer: emailer, - fromAddress: fromAddress, passwordResetURL: passwordResetURL, verifyEmailURL: verifyEmailURL, invitationURL: invitationURL, @@ -106,7 +103,7 @@ func (u *UserEmailer) SendResetPasswordEmail(email string, redirectURL url.URL, resetURL.RawQuery = q.Encode() if u.emailer != nil { - err = u.emailer.SendMail(u.fromAddress, "Reset Your Password", "password-reset", + err = u.emailer.SendMail("Reset Your Password", "password-reset", map[string]interface{}{ "email": usr.Email, "link": resetURL.String(), @@ -144,7 +141,7 @@ func (u *UserEmailer) SendInviteEmail(email string, redirectURL url.URL, clientI resetURL.RawQuery = q.Encode() if u.emailer != nil { - err = u.emailer.SendMail(u.fromAddress, "Activate Your Account", "invite", + err = u.emailer.SendMail("Activate Your Account", "invite", map[string]interface{}{ "email": usr.Email, "link": resetURL.String(), @@ -191,7 +188,7 @@ func (u *UserEmailer) SendEmailVerification(userID, clientID string, redirectURL verifyURL.RawQuery = q.Encode() if u.emailer != nil { - err = u.emailer.SendMail(u.fromAddress, "Please verify your email address.", "verify-email", + err = u.emailer.SendMail("Please verify your email address.", "verify-email", map[string]interface{}{ "email": usr.Email, "link": verifyURL.String(), diff --git a/user/email/email_test.go b/user/email/email_test.go index 453025ea..a9ed6868 100644 --- a/user/email/email_test.go +++ b/user/email/email_test.go @@ -29,13 +29,12 @@ var ( ) type testEmailer struct { - from, subject, text, html string - to []string - sent bool + subject, text, html string + to []string + sent bool } -func (t *testEmailer) SendMail(from, subject, text, html string, to ...string) error { - t.from = from +func (t *testEmailer) SendMail(subject, text, html string, to ...string) error { t.subject = subject t.text = text t.html = html @@ -112,9 +111,9 @@ func makeTestFixtures() (*UserEmailer, *testEmailer, *key.PublicKey) { htmlTemplates := htmltemplate.New("html") emailer := &testEmailer{} - tEmailer := email.NewTemplatizedEmailerFromTemplates(textTemplates, htmlTemplates, emailer) + tEmailer := email.NewTemplatizedEmailerFromTemplates(textTemplates, htmlTemplates, emailer, fromAddress) - userEmailer := NewUserEmailer(ur, pwr, signerFn, validityWindow, issuerURL, tEmailer, fromAddress, passwordResetURL, verifyEmailURL, acceptInvitationURL) + userEmailer := NewUserEmailer(ur, pwr, signerFn, validityWindow, issuerURL, tEmailer, passwordResetURL, verifyEmailURL, acceptInvitationURL) return userEmailer, emailer, publicKey } @@ -203,10 +202,6 @@ func TestSendResetPasswordEmail(t *testing.T) { t.Errorf("case %d: want==%v, got==%v", i, tt.email, emailer.to[0]) } - if fromAddress != emailer.from { - t.Errorf("case %d: want==%v, got==%v", i, fromAddress, emailer.from) - } - } else if emailer.sent { t.Errorf("case %d: want !emailer.sent", i) } @@ -306,10 +301,6 @@ func TestSendEmailVerificationEmail(t *testing.T) { t.Errorf("case %d: want==%v, got==%v", i, tt.wantEmailAddress, emailer.to[0]) } - if fromAddress != emailer.from { - t.Errorf("case %d: want==%v, got==%v", i, fromAddress, emailer.from) - } - } else if emailer.sent { t.Errorf("case %d: want !emailer.sent", i) } From 7378414e11ae4526d5629b9269ab162f22d22a6c Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Mon, 11 Jul 2016 09:16:32 -0700 Subject: [PATCH 2/2] 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) + } + } +}