From 87faa5a1f76514b901ba79622144c5f41b71fcd9 Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Mon, 27 Jun 2016 23:34:55 -0700 Subject: [PATCH] *: 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) }