Page MenuHomePhabricator

Improve handling of email verification and "activated" accounts
ClosedPublic

Authored by epriestley on Nov 12 2013, 10:10 PM.
Tags
None
Referenced Files
F13097438: D7572.id17091.diff
Thu, Apr 25, 11:46 PM
F13097437: D7572.id.diff
Thu, Apr 25, 11:46 PM
F13097436: D7572.id17093.diff
Thu, Apr 25, 11:46 PM
Unknown Object (File)
Wed, Apr 24, 9:57 PM
Unknown Object (File)
Wed, Apr 17, 10:03 AM
Unknown Object (File)
Thu, Apr 11, 7:34 AM
Unknown Object (File)
Wed, Apr 10, 5:56 AM
Unknown Object (File)
Tue, Apr 9, 11:11 AM
Subscribers

Details

Summary

Small step forward which improves existing stuff or lays groudwork for future stuff:

  • Currently, to check for email verification, we have to single-query the email address on every page. Instead, denoramlize it into the user object.
    • Migrate all the existing users.
    • When the user verifies an email, mark them as isEmailVerified if the email is their primary email.
    • Just make the checks look at the isEmailVerified field.
  • Add a new check, isUserActivated(), to cover email-verified plus disabled. Currently, a non-verified-but-not-disabled user could theoretically use Conduit over SSH, if anyone deployed it. Tighten that up.
  • Add an isApproved flag, which is always true for now. In a future diff, I want to add a default-on admin approval queue for new accounts, to prevent configuration mistakes. The way it will work is:
    • When the queue is enabled, registering users are created with isApproved = false.
    • Admins are sent an email, "[Phabricator] New User Approval (alincoln)", telling them that a new user is waiting for approval.
    • They go to the web UI and approve the user.
    • Manually-created accounts are auto-approved.
    • The email will have instructions for disabling the queue.

I think this queue will be helpful for new installs and give them peace of mind, and when you go to disable it we have a better opportunity to warn you about exactly what that means.

Generally, I want to improve the default safety of registration, since if you just blindly coast through the path of least resistance right now your install ends up pretty open, and realistically few installs are on VPNs.

Test Plan
  • Ran migration, verified isEmailVerified populated correctly.
  • Created a new user, checked DB for verified (not verified).
  • Verified, checked DB (now verified).
  • Used Conduit, People, Diffusion.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

That was pretty clean. :)

The approval queue sounds like a good idea. As an administrator of this wide-open instance, I wish I could more easily tell "who the heck is this person?" I hope said queue thus surfaces some of the useful information that is no longer easily findable, like links to the external account(s) the queued users are registering with.