createUser: mark emails as unverified on creation by bobheadxi · Pull Request #46187 · sourcegraph/sourcegraph-public-snapshot

bobheadxi · 2023-01-06T01:10:53Z

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 https://github.com/sourcegraph/customer/issues/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:

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

Test plan

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

Manual test:

sg start, configured with SMTP in dev-private
Site admin -> users -> create testuser with email
Site admin -> users -> testuser -> emails -> primary email is unverified
Log out
CLick password reset link in email
Log in
User -> emails -> primary email is verified

bobheadxi · 2023-01-06T01:11:03Z

Current dependencies on/for this PR:

main
- PR client: remove deprecated authenticatedUser.email #46183
  - PR email: do not send certain emails to unverified emails #46184
    - PR createUser: mark emails as unverified on creation #46187 👈

This comment was auto-generated by Graphite.

sg-e2e-regression-test-bob · 2023-01-06T01:18:03Z

Bundle size report 📦

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

Look at the Statoscope report for a full comparison between the commits c94542c and 40ab277 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

unknwon · 2023-01-06T08:21:32Z

We could send a verification email on user creation, but these aren't needed if an admin expects a user to sign in via an external service, and would send an extra email on top of the password reset link if needed. This is also awkward because you must sign in before you click the verification.

IIRC, if an account is pre-created with an unverified and the user is expected to sign in via an external service, a brand new account is always created because we only consider account linking by verified emails. Not saying we can't change the current behavior, just want to call out the implication.

We could bundle verification into the password reset, but it's unclear how worthwhile this investment is at the moment - I could implement this in a follow-up PR if required.

For a best possible UX, I believe we should. I can't recall another product that wants me to do both when I get an account... If we trust the password reset link being sent to an email, there seems no reason we can't in turn trust the email itself, right?

unknwon

Minor thinking: should we keep the current behavior if SMTP is not configured?

bobheadxi · 2023-01-06T17:37:23Z

Minor thinking: should we keep the current behavior if SMTP is not configured?

SMTP configuration is not static - you might not set up SMTP until some time after an instance is stood up, for example. Also, whether the email is verified or not doesn't impact instances that don't have SMTP set up, since we cannot send emails anyway

Update: actually, I think you may be right - a lot of other functionality (login, external accounts) key on validated emails, so retaining the old behaviour is probably best. Instances that care should set up SMTP before onboarding users

IIRC, if an account is pre-created with an unverified and the user is expected to sign in via an external service, a brand new account is always created because we only consider account linking by verified emails. Not saying we can't change the current behavior, just want to call out the implication.

Ah, you are right. I think I'll look harder look at having the password reset also handle email verification before making this change then

Update: see https://github.com/sourcegraph/sourcegraph/pull/46307

…email_do_not_send_emails_to_unverified_emails_split

bobheadxi · 2023-01-20T04:10:11Z

@kopancek thanks for the first round of reviews - I've addressed your comments and updated the behaviour, would appreciate a review when you get the chance!

mrnugget

Do we also need to update docs?

…email_do_not_send_emails_to_unverified_emails_split

bobheadxi · 2023-01-20T17:46:40Z

@mrnugget I've created some docs around email verification here: https://github.com/sourcegraph/sourcegraph/pull/46187/commits/00d5137881812fc280f7b7265df34aadcb2e28d3

sourcegraph-bot · 2023-01-20T17:48:21Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 40ab277...c94542c.

Notify	File(s)
@sourcegraph/delivery	doc/admin/auth/index.md doc/admin/config/email.md

mrnugget · 2023-01-23T10:29:12Z

Thanks!

kopancek

Looks good to me. Added small suggestions that should not block PR merge.

kopancek · 2023-01-24T08:43:43Z

 The [`builtin` auth provider](../config/site_config.md#builtin-password-authentication) manages user accounts internally in its own database. It supports user signup, login, and password reset (via email if configured, or else via a site admin).

-Password reset links expire after 4 hours.
+Password reset links expire after 4 hours by default - this can be configured in site configuration with the `auth.passwordResetLinkExpiry` field.


I think it would be better to have a link that points to the site config schema definition here. Not sure how feasible that is though 🤔

Added a link!

Trick seems to be to view the site_config page in browser, and get the anchor link through there

Co-authored-by: Milan Freml <kopancek@users.noreply.github.com> Co-authored-by: Joe Chen <joe@sourcegraph.com>

cla-bot Bot added the cla-signed label Jan 6, 2023

This was referenced Jan 6, 2023

client: remove deprecated authenticatedUser.email #46183

Merged

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

Merged

bobheadxi commented Jan 6, 2023

View reviewed changes

Comment thread cmd/frontend/graphqlbackend/users_create.go Outdated

bobheadxi mentioned this pull request Jan 6, 2023

emails: send code monitors, account updates to verified emails only #46167

Closed

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 force-pushed the 01-05-email_do_not_send_emails_to_unverified_emails_split branch from 5b40d8f to 780de03 Compare January 6, 2023 01:49

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 force-pushed the 01-05-email_do_not_send_emails_to_unverified_emails_split branch from 780de03 to 61aa4ed Compare January 6, 2023 02:09

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 force-pushed the 01-05-email_do_not_send_emails_to_unverified_emails_split branch from 61aa4ed to 804ed36 Compare January 6, 2023 04:49

bobheadxi force-pushed the 01-05-email_do_not_send_emails_to_unverified_emails branch from d1a6bd6 to 1c50b9a Compare January 6, 2023 04:57

bobheadxi force-pushed the 01-05-email_do_not_send_emails_to_unverified_emails_split branch from 804ed36 to adf98a1 Compare January 6, 2023 04:57

bobheadxi force-pushed the 01-05-email_do_not_send_emails_to_unverified_emails branch 2 times, most recently from 3ef8164 to 78a3acc Compare January 6, 2023 07:05

bobheadxi force-pushed the 01-05-email_do_not_send_emails_to_unverified_emails_split branch from adf98a1 to acdc689 Compare January 6, 2023 07:05

bobheadxi marked this pull request as ready for review January 6, 2023 07:26

bobheadxi requested review from a team January 6, 2023 07:28

unknwon reviewed Jan 6, 2023

View reviewed changes

Comment thread client/web/src/site-admin/SiteAdminCreateUserPage.tsx Outdated

bobheadxi marked this pull request as draft January 6, 2023 17:37

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

bobheadxi force-pushed the 01-05-email_do_not_send_emails_to_unverified_emails_split branch 2 times, most recently from 467afd9 to 8bf46d9 Compare January 6, 2023 17:43

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 added 8 commits January 12, 2023 08:47

add more logging

ce4e4fc

simplify optional email text

3d15cd5

fixup

ce03976

add wording based on emailEnabled

3cdf1ec

Merge branch 'main' of github.com:sourcegraph/sourcegraph into 01-05-…

de610b8

…email_do_not_send_emails_to_unverified_emails_split

add changelog entry

3c4d116

fixup

ea164a1

add tests

b2b4d09

bobheadxi requested a review from kopancek January 12, 2023 17:39

remove url mock

2c8ebd2

evict approved these changes Jan 19, 2023

View reviewed changes

bobheadxi added 3 commits January 19, 2023 19:52

Merge branch 'main' of github.com:sourcegraph/sourcegraph into 01-05-…

14b5351

…email_do_not_send_emails_to_unverified_emails_split

createuser: only set verifiedEmail: false in UI user creation

1471f62

update changelog

f93d946

bobheadxi added 2 commits January 19, 2023 20:13

also check if password resets are enabled

1821615

improve docs

174c057

mrnugget reviewed Jan 20, 2023

View reviewed changes

bobheadxi added 2 commits January 20, 2023 09:45

add docs

00d5137

Merge branch 'main' of github.com:sourcegraph/sourcegraph into 01-05-…

2d32cf9

…email_do_not_send_emails_to_unverified_emails_split

add link to email page, rename email page

cf9fa77

unknwon approved these changes Jan 24, 2023

View reviewed changes

Comment thread cmd/frontend/graphqlbackend/users_create.go Outdated

Comment thread cmd/frontend/internal/auth/userpasswd/set_password.go Outdated

kopancek approved these changes Jan 24, 2023

View reviewed changes

bobheadxi and others added 2 commits January 24, 2023 16:09

Apply suggestions from code review

eea3bbe

Co-authored-by: Milan Freml <kopancek@users.noreply.github.com> Co-authored-by: Joe Chen <joe@sourcegraph.com>

add anchor link to passwordResetLinkExpiry

c94542c

bobheadxi merged commit 4ad221c into main Jan 25, 2023

bobheadxi deleted the 01-05-email_do_not_send_emails_to_unverified_emails_split branch January 25, 2023 00:33

Conversation

bobheadxi commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

bobheadxi commented Jan 6, 2023

Uh oh!

Uh oh!

sg-e2e-regression-test-bob commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle size report 📦

Uh oh!

unknwon commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

unknwon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bobheadxi commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobheadxi commented Jan 20, 2023

Uh oh!

mrnugget left a comment

Choose a reason for hiding this comment

Uh oh!

bobheadxi commented Jan 20, 2023

Uh oh!

sourcegraph-bot commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrnugget commented Jan 23, 2023

Uh oh!

Uh oh!

Uh oh!

kopancek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kopancek Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

bobheadxi Jan 25, 2023

Choose a reason for hiding this comment

Uh oh!

bobheadxi Jan 25, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bobheadxi commented Jan 6, 2023 •

edited

Loading

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

edited

Loading

unknwon commented Jan 6, 2023 •

edited

Loading

bobheadxi commented Jan 6, 2023 •

edited

Loading

sourcegraph-bot commented Jan 20, 2023 •

edited

Loading

kopancek left a comment •

edited

Loading