Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

email: do not send certain emails to unverified emails#46184

Merged
bobheadxi merged 5 commits intomainfrom
01-05-email_do_not_send_emails_to_unverified_emails
Jan 10, 2023
Merged

email: do not send certain emails to unverified emails#46184
bobheadxi merged 5 commits intomainfrom
01-05-email_do_not_send_emails_to_unverified_emails

Conversation

@bobheadxi
Copy link
Member

@bobheadxi bobheadxi commented Jan 6, 2023 •
edited
Loading

We should seek to avoid damaging email reputations by validating if emails are verified before sending certain types of potentially high-frequency, non-essential emails to user primary emails, in this change:

  • Account update notifications
  • Code monitors

In practice, at the moment only builtin auth signups will create accounts that have unverified primary emails. External service accounts are created with primary emails already verified, and so are those created with the createUser mutation (which I am hoping to change in https://github.com/sourcegraph/sourcegraph/pull/46187).

Today, a user could create a user, which automatically marks the email as verified, and then proceed to set up a code monitor that accidentally sends large numbers of emails that all bounce, which has implications for Cloud's managed SMTP services.

The risk: there was at least one incident recently where a small number of code monitors issued over a thousand emails in a few days due to an unforeseen issue with retry handling (https://github.com/sourcegraph/sourcegraph/pull/45641), and at least one instance of a user creating an account with an invalid email.

Stacked on https://github.com/sourcegraph/sourcegraph/pull/46183

Test plan

image

@cla-bot cla-bot bot added the cla-signed label Jan 6, 2023
@bobheadxi bobheadxi mentioned this pull request Jan 6, 2023
Merged
@bobheadxi
Copy link
Member Author

bobheadxi commented Jan 6, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@bobheadxi bobheadxi force-pushed the 01-05-client_remove_deprecated_authenticatedUser.email branch from e60c2a0 to 6b80bb1 Compare January 6, 2023 00:28
@bobheadxi bobheadxi force-pushed the 01-05-email_do_not_send_emails_to_unverified_emails branch from 01bfde0 to 5b40d8f Compare January 6, 2023 00:28
@bobheadxi bobheadxi mentioned this pull request Jan 6, 2023
Closed
@sg-e2e-regression-test-bob
Copy link

sg-e2e-regression-test-bob commented Jan 6, 2023 •
edited
Loading

Bundle size report 📦

Initial size Total size Async size Modules
0.00% (0.00 kb) 0.00% (+0.18 kb) 0.00% (+0.18 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits 58b69df and 1d331cd or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

@bobheadxi bobheadxi force-pushed the 01-05-email_do_not_send_emails_to_unverified_emails branch from 5b40d8f to ab399f9 Compare January 6, 2023 01:10
@bobheadxi bobheadxi mentioned this pull request Jan 6, 2023
Merged
@bobheadxi bobheadxi force-pushed the 01-05-client_remove_deprecated_authenticatedUser.email branch from 6b80bb1 to fc4644e Compare January 6, 2023 01:49
@bobheadxi bobheadxi force-pushed the 01-05-email_do_not_send_emails_to_unverified_emails branch from ab399f9 to 5a20b9f Compare January 6, 2023 01:49
@bobheadxi bobheadxi force-pushed the 01-05-client_remove_deprecated_authenticatedUser.email branch from fc4644e to ad68c3d Compare January 6, 2023 02:09
@bobheadxi bobheadxi force-pushed the 01-05-email_do_not_send_emails_to_unverified_emails branch from 5a20b9f to 75e7b2c Compare January 6, 2023 02:09
@bobheadxi bobheadxi force-pushed the 01-05-client_remove_deprecated_authenticatedUser.email branch from ad68c3d to fb5a412 Compare January 6, 2023 04:49
@bobheadxi bobheadxi force-pushed the 01-05-email_do_not_send_emails_to_unverified_emails branch from 75e7b2c to d1a6bd6 Compare January 6, 2023 04:49
@bobheadxi bobheadxi force-pushed the 01-05-client_remove_deprecated_authenticatedUser.email branch from fb5a412 to 863f3b3 Compare January 6, 2023 04:57
@bobheadxi bobheadxi force-pushed the 01-05-email_do_not_send_emails_to_unverified_emails branch 2 times, most recently from 1c50b9a to 3ef8164 Compare January 6, 2023 05:07
@bobheadxi bobheadxi force-pushed the 01-05-client_remove_deprecated_authenticatedUser.email branch from 863f3b3 to c9913ac Compare January 6, 2023 07:05
@bobheadxi bobheadxi force-pushed the 01-05-email_do_not_send_emails_to_unverified_emails branch from 3ef8164 to 78a3acc Compare January 6, 2023 07:05
@bobheadxi bobheadxi requested a review from a team January 6, 2023 07:22
@bobheadxi bobheadxi marked this pull request as ready for review January 6, 2023 07:22
@bobheadxi bobheadxi requested review from a team January 6, 2023 07:24
@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Jan 6, 2023 •
edited
Loading

Codenotify: Notifying subscribers in CODENOTIFY files for diff 1d331cd...58b69df.

Notify File(s)
@limitedmage client/web/src/enterprise/code-monitoring/components/actions/EmailAction.tsx
@unknwon enterprise/cmd/frontend/internal/auth/githuboauth/session.go
enterprise/cmd/frontend/internal/auth/httpheader/middleware.go

@bobheadxi bobheadxi changed the title email: do not send emails to unverified emails email: do not send certain emails to unverified emails Jan 6, 2023
unknwon
unknwon approved these changes Jan 6, 2023
View reviewed changes
Copy link
Contributor

@unknwon unknwon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve to unblock, just one minor issue

cmd/frontend/backend/user_emails.go Outdated
Comment on lines 304 to 305
Copy link
Contributor

@unknwon unknwon Jan 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@bobheadxi bobheadxi force-pushed the 01-05-client_remove_deprecated_authenticatedUser.email branch from 470cc9e to 85d7ee6 Compare January 6, 2023 17:42
@bobheadxi bobheadxi force-pushed the 01-05-email_do_not_send_emails_to_unverified_emails branch from 78a3acc to a28f30a Compare January 6, 2023 17:42
limitedmage
limitedmage approved these changes Jan 6, 2023
View reviewed changes
Copy link
Contributor

@limitedmage limitedmage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EmailAction.tsx looks good! Didn't review the Go code.

Base automatically changed from 01-05-client_remove_deprecated_authenticatedUser.email to main January 6, 2023 18:46
@bobheadxi bobheadxi force-pushed the 01-05-email_do_not_send_emails_to_unverified_emails branch from 6611218 to 6cf4633 Compare January 6, 2023 18:48
@bobheadxi bobheadxi enabled auto-merge (squash) January 10, 2023 18:56
@bobheadxi bobheadxi merged commit 7ec296a into main Jan 10, 2023
@bobheadxi bobheadxi deleted the 01-05-email_do_not_send_emails_to_unverified_emails branch January 10, 2023 19:08
bobheadxi referenced this pull request Jan 25, 2023
@bobheadxi @unknwon @kopancek
createUser: mark emails as unverified on creation (#46187)
4ad221c 
Today, users created with `mutation { createUser }` automatically have their emails marked as verified, even if the email is bogus. This could cause us to automatically send potentially large numbers of emails automatically to nonexistent email addresses (see sourcegraph/customer#1790). To mitigate this, this change marks the emails of created users as unverified if SMTP is configured.

This has implications for Cloud instances, where `mutation { createUser }` is frequently used to create initial admin users. After this change, these users may receive a more limited set of emails (i.e. https://github.com/sourcegraph/sourcegraph/pull/46184) and have limited capabilities (i.e. unable to link account via external service) until they verify their email by:

1. using a "set password" or "reset password" link delivered by email (https://github.com/sourcegraph/sourcegraph/pull/46307)
1. using "verify email" in user settings to send an email verification
3. a site admin verifying the user email manually via `setUserEmailVerified` mutation or UI

## Test plan

<!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles -->

I've added new unit tests that cover the old and new set password behaviour on the mutation.

Manual test:

1. `sg start`, configured with SMTP in `dev-private`
2. Site admin -> users -> create `testuser` with email 
3. Site admin -> users -> `testuser` -> emails -> primary email is unverified
4. Log out
5. CLick password reset link in email
6. Log in
7. User -> emails -> primary email is verified

Co-authored-by: Joe Chen <joe@sourcegraph.com>
Co-authored-by: Milan Freml <kopancek@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@limitedmage limitedmage limitedmage approved these changes

@unknwon unknwon unknwon approved these changes

@evict evict evict approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@bobheadxi @sg-e2e-regression-test-bob @sourcegraph-bot @limitedmage @unknwon @evict