Allow custom email server for invitations #796

whaught · 2020-10-08T23:04:23Z

Issue #787

Proposed Changes

This PR allows for a server-level override to sending new user invitations via a custom SMTP server
If no secret is present, it falls back to our firebase hack with the password-reset template

In the future we can use this to

Allow per-realm overrides of the email server
Allow per-realm templates for the invitation email itself

This gets around our server-side call to the firebase client API which may face IP rate limiting.
https://cloud.google.com/identity-platform/quotas#email_link_generation_limits_2
Note that generating the link has 10x quota and is intended to be called from the server.

We could someday send other kinds of emails (eg. TOTP for 2nd factor auth if we want to roll our own 2nd factor via this and Twilio), but that sort of thing is not currently planned.

Release Note

Allow for invitations from a custom SMTP server

jeremyfaller

Nothing I've commented on here requires changing. Feel free to address in a followup if necessary.

jeremyfaller · 2020-10-09T16:00:20Z

pkg/controller/user/reset_password.go

@@ -49,7 +49,8 @@ func (c *Controller) ensureFirebaseUserExists(ctx context.Context, user *databas
 	}

 	if created {
-		if err := c.firebaseInternal.SendNewUserInvitation(ctx, user.Email); err != nil {
+		err := c.emailer.SendNewUserInvitation(ctx, user.Email)
+		if err != nil {
 			flash.Error("Could not send new user invitation: %v", err)
 			return true, err


dunno if we want to wrap errors. We're typically trying to.

pkg/email/smtp.go

jeremyfaller · 2020-10-09T16:07:42Z

pkg/email/config.go

+
+	User     string `env:"EMAIL_USER" json:",omitempty"`
+	Password string `env:"EMAIL_PASSWORD" json:",omitempty"`
+	SmtpHost string `env:"EMAIL_SMTP_HOST" json:",omitempty"`


Dunno why these are separate, especially when you just combine them later....

Lines like smtp.PlainAuth("", s.User, s.Password, s.SmtpHost) use the host alone, we add on the port only for the connection

jeremyfaller · 2020-10-09T16:08:32Z

pkg/email/smtp.go

+
+	User     string
+	Password string
+	SmtpHost string


jeremyfaller · 2020-10-09T19:56:45Z

pkg/email/smtp.go

+	// Sending email.
+	err = smtp.SendMail(s.SMTPHost+":"+s.SMTPPort, auth, s.User, []string{toEmail}, []byte(finalMessage))
+	if err != nil {
+		return err


wrap, or don't bother setting err.

jeremyfaller · 2020-10-09T19:56:57Z

pkg/email/smtp.go

+
+	inviteLink, err := s.FirebaseAuth.PasswordResetLink(ctx, toEmail)
+	if err != nil {
+		return err


google-oss-robot · 2020-10-09T20:02:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jeremyfaller, whaught

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jeremyfaller,whaught]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sethvargo · 2020-10-11T13:28:56Z

pkg/email/config.go

+
+	User     string `env:"EMAIL_USER" json:",omitempty"`
+	Password string `env:"EMAIL_PASSWORD" json:",omitempty"`
+	SMTPHost string `env:"EMAIL_SMTP_HOST" json:",omitempty"`


It's probably worth noting that this won't work for most modern Gmail accounts, and won't work for ~~G Suite~~ Google Workplace either.

Wait... but it does. I'm running [email protected] to test this

You don't have MFA enabled then? It won't work with advanced protection accounts either.

https://support.google.com/mail/answer/185833

With MFA you need to create an app password for this to use

App Passwords aren’t recommended and are unnecessary in most cases.

If you don’t have this option, it might be because:

c. Your account is through work, school, or other organization.
d. You turned on Advanced Protection.

c is very likely. d will probably become a default in the future.

72cc99b

Adding in some notes

sethvargo · 2020-10-11T13:30:43Z

pkg/email/config.go

+
+type Provider interface {
+	// SendNewUserInvitation sends an invite to join the server.
+	SendNewUserInvitation(ctx context.Context, email string) error


It would probably be better to abstract the email content from the email sending. I'd expect the email provider to only know how to SendEmail:

type Provider interface { SendEmail(ctx context.Context, subject, to, cc, bcc string, body io.Reader) }

sethvargo · 2020-10-11T13:31:20Z

pkg/email/smtp.go

+func (s *SMTPProvider) SendNewUserInvitation(ctx context.Context, toEmail string) error {
+	// Header
+	header := make(map[string]string)
+	header["From"] = s.User


We should try to make most of these constants. Both the map keys and many of their values are constant.

sethvargo · 2020-10-11T13:31:25Z

pkg/email/smtp.go

+
+	inviteLink, err := s.FirebaseAuth.PasswordResetLink(ctx, toEmail)
+	if err != nil {
+		return err


sethvargo · 2020-10-11T13:32:14Z

pkg/email/smtp.go

+	header["Subject"] = "COVID-19 Verification Server Invitation"
+
+	header["MIME-Version"] = "1.0"
+	header["Content-Type"] = `text/html; charset="utf-8"`


I'm not sure we want to send as text/html. The firebase ones are text/plain.

Allow custom email server for invitations

02454cf

google-oss-robot added the do-not-merge/work-in-progress label Oct 8, 2020

google-oss-robot requested review from jeremyfaller and yegle October 8, 2020 23:04

googlebot added the cla: yes Auto: added by CLA bot when all committers have signed a CLA. label Oct 8, 2020

google-oss-robot added approved labels Oct 8, 2020

jeremyfaller approved these changes Oct 9, 2020

View reviewed changes

google-oss-robot assigned jeremyfaller Oct 9, 2020

google-oss-robot added the lgtm label Oct 9, 2020

strings

2e4e5d2

google-oss-robot removed the lgtm label Oct 9, 2020

fix lint

98d23bb

whaught changed the title ~~[WIP] Allow custom email server for invitations~~ Allow custom email server for invitations Oct 9, 2020

google-oss-robot removed the do-not-merge/work-in-progress label Oct 9, 2020

whaught added 5 commits October 9, 2020 12:01

lint2

b6c467e

fix returns

ac9b8b3

fix message

4f85a77

break

559930a

has creds

665c898

jeremyfaller approved these changes Oct 9, 2020

View reviewed changes

google-oss-robot added the lgtm label Oct 9, 2020

google-oss-robot merged commit 9dc1498 into google:main Oct 9, 2020

whaught deleted the send-email branch October 9, 2020 20:14

github-actions bot locked as resolved and limited conversation to collaborators Oct 10, 2020

sethvargo reviewed Oct 11, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow custom email server for invitations #796

Allow custom email server for invitations #796

whaught commented Oct 8, 2020

jeremyfaller left a comment

jeremyfaller Oct 9, 2020

jeremyfaller Oct 9, 2020

whaught Oct 9, 2020

jeremyfaller Oct 9, 2020

jeremyfaller Oct 9, 2020

jeremyfaller Oct 9, 2020

sethvargo Oct 11, 2020

google-oss-robot commented Oct 9, 2020

sethvargo Oct 11, 2020

whaught Oct 11, 2020

sethvargo Oct 12, 2020

whaught Oct 12, 2020

sethvargo Oct 12, 2020

whaught Oct 12, 2020

sethvargo Oct 12, 2020

sethvargo Oct 11, 2020

sethvargo Oct 11, 2020

sethvargo Oct 11, 2020

sethvargo Oct 11, 2020

Allow custom email server for invitations #796

Allow custom email server for invitations #796

Conversation

whaught commented Oct 8, 2020

Proposed Changes

jeremyfaller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-robot commented Oct 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment