Commit 7f11e8d7 authored by epriestley's avatar epriestley
Browse files

Improve handling of email verification and "activated" accounts

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.

Reviewers: btrahan

Reviewed By: btrahan

CC: chad, aran

Differential Revision: https://secure.phabricator.com/D7572
parent cd73fe78
ALTER TABLE {$NAMESPACE}_user.user
ADD isEmailVerified INT UNSIGNED NOT NULL;
ALTER TABLE {$NAMESPACE}_user.user
ADD isApproved INT UNSIGNED NOT NULL;
ALTER TABLE {$NAMESPACE}_user.user
ADD KEY `key_approved` (isApproved);
UPDATE {$NAMESPACE}_user.user SET isApproved = 1;
<?php
$table = new PhabricatorUser();
$conn_w = $table->establishConnection('w');
foreach (new LiskMigrationIterator($table) as $user) {
$username = $user->getUsername();
echo "Migrating {$username}...\n";
if ($user->getIsEmailVerified()) {
// Email already verified.
continue;
}
$primary = $user->loadPrimaryEmail();
if (!$primary) {
// No primary email.
continue;
}
if (!$primary->getIsVerified()) {
// Primary email not verified.
continue;
}
// Primary email is verified, so mark the account as verified.
queryfx(
$conn_w,
'UPDATE %T SET isEmailVerified = 1 WHERE id = %d',
$table->getTableName(),
$user->getID());
}
echo "Done.\n";
......@@ -38,8 +38,8 @@ try {
throw new Exception("Invalid username.");
}
if ($user->getIsDisabled()) {
throw new Exception("You have been exiled.");
if (!$user->isUserActivated()) {
throw new Exception(pht("Your account is not activated."));
}
if ($args->getArg('ssh-command')) {
......
......@@ -857,7 +857,7 @@ celerity_register_resource_map(array(
),
'aphront-dialog-view-css' =>
array(
'uri' => '/res/830fa2de/rsrc/css/aphront/dialog-view.css',
'uri' => '/res/6b6a41c6/rsrc/css/aphront/dialog-view.css',
'type' => 'css',
'requires' =>
array(
......@@ -1889,7 +1889,7 @@ celerity_register_resource_map(array(
),
'javelin-behavior-maniphest-batch-selector' =>
array(
'uri' => '/res/423c5f1b/rsrc/js/application/maniphest/behavior-batch-selector.js',
'uri' => '/res/a82658b3/rsrc/js/application/maniphest/behavior-batch-selector.js',
'type' => 'js',
'requires' =>
array(
......@@ -1917,7 +1917,7 @@ celerity_register_resource_map(array(
),
'javelin-behavior-maniphest-subpriority-editor' =>
array(
'uri' => '/res/1fa4961f/rsrc/js/application/maniphest/behavior-subpriorityeditor.js',
'uri' => '/res/95f3d4a6/rsrc/js/application/maniphest/behavior-subpriorityeditor.js',
'type' => 'js',
'requires' =>
array(
......@@ -4328,7 +4328,7 @@ celerity_register_resource_map(array(
), array(
'packages' =>
array(
'f0d63822' =>
'd831cac3' =>
array(
'name' => 'core.pkg.css',
'symbols' =>
......@@ -4377,7 +4377,7 @@ celerity_register_resource_map(array(
41 => 'phabricator-tag-view-css',
42 => 'phui-list-view-css',
),
'uri' => '/res/pkg/f0d63822/core.pkg.css',
'uri' => '/res/pkg/d831cac3/core.pkg.css',
'type' => 'css',
),
'2c1dba03' =>
......@@ -4552,7 +4552,7 @@ celerity_register_resource_map(array(
'uri' => '/res/pkg/49898640/maniphest.pkg.css',
'type' => 'css',
),
'0a694954' =>
'0474f45c' =>
array(
'name' => 'maniphest.pkg.js',
'symbols' =>
......@@ -4563,21 +4563,21 @@ celerity_register_resource_map(array(
3 => 'javelin-behavior-maniphest-transaction-expand',
4 => 'javelin-behavior-maniphest-subpriority-editor',
),
'uri' => '/res/pkg/0a694954/maniphest.pkg.js',
'uri' => '/res/pkg/0474f45c/maniphest.pkg.js',
'type' => 'js',
),
),
'reverse' =>
array(
'aphront-dialog-view-css' => 'f0d63822',
'aphront-error-view-css' => 'f0d63822',
'aphront-list-filter-view-css' => 'f0d63822',
'aphront-pager-view-css' => 'f0d63822',
'aphront-panel-view-css' => 'f0d63822',
'aphront-table-view-css' => 'f0d63822',
'aphront-tokenizer-control-css' => 'f0d63822',
'aphront-tooltip-css' => 'f0d63822',
'aphront-typeahead-control-css' => 'f0d63822',
'aphront-dialog-view-css' => 'd831cac3',
'aphront-error-view-css' => 'd831cac3',
'aphront-list-filter-view-css' => 'd831cac3',
'aphront-pager-view-css' => 'd831cac3',
'aphront-panel-view-css' => 'd831cac3',
'aphront-table-view-css' => 'd831cac3',
'aphront-tokenizer-control-css' => 'd831cac3',
'aphront-tooltip-css' => 'd831cac3',
'aphront-typeahead-control-css' => 'd831cac3',
'differential-changeset-view-css' => '1084b12b',
'differential-core-view-css' => '1084b12b',
'differential-inline-comment-editor' => '5e9e5c4e',
......@@ -4591,7 +4591,7 @@ celerity_register_resource_map(array(
'differential-table-of-contents-css' => '1084b12b',
'diffusion-commit-view-css' => '7aa115b4',
'diffusion-icons-css' => '7aa115b4',
'global-drag-and-drop-css' => 'f0d63822',
'global-drag-and-drop-css' => 'd831cac3',
'inline-comment-summary-css' => '1084b12b',
'javelin-aphlict' => '2c1dba03',
'javelin-behavior' => '3e3be199',
......@@ -4623,11 +4623,11 @@ celerity_register_resource_map(array(
'javelin-behavior-konami' => '2c1dba03',
'javelin-behavior-lightbox-attachments' => '2c1dba03',
'javelin-behavior-load-blame' => '5e9e5c4e',
'javelin-behavior-maniphest-batch-selector' => '0a694954',
'javelin-behavior-maniphest-subpriority-editor' => '0a694954',
'javelin-behavior-maniphest-transaction-controls' => '0a694954',
'javelin-behavior-maniphest-transaction-expand' => '0a694954',
'javelin-behavior-maniphest-transaction-preview' => '0a694954',
'javelin-behavior-maniphest-batch-selector' => '0474f45c',
'javelin-behavior-maniphest-subpriority-editor' => '0474f45c',
'javelin-behavior-maniphest-transaction-controls' => '0474f45c',
'javelin-behavior-maniphest-transaction-expand' => '0474f45c',
'javelin-behavior-maniphest-transaction-preview' => '0474f45c',
'javelin-behavior-phabricator-active-nav' => '2c1dba03',
'javelin-behavior-phabricator-autofocus' => '2c1dba03',
'javelin-behavior-phabricator-gesture' => '2c1dba03',
......@@ -4666,56 +4666,56 @@ celerity_register_resource_map(array(
'javelin-util' => '3e3be199',
'javelin-vector' => '3e3be199',
'javelin-workflow' => '3e3be199',
'lightbox-attachment-css' => 'f0d63822',
'lightbox-attachment-css' => 'd831cac3',
'maniphest-task-summary-css' => '49898640',
'phabricator-action-list-view-css' => 'f0d63822',
'phabricator-application-launch-view-css' => 'f0d63822',
'phabricator-action-list-view-css' => 'd831cac3',
'phabricator-application-launch-view-css' => 'd831cac3',
'phabricator-busy' => '2c1dba03',
'phabricator-content-source-view-css' => '1084b12b',
'phabricator-core-css' => 'f0d63822',
'phabricator-crumbs-view-css' => 'f0d63822',
'phabricator-core-css' => 'd831cac3',
'phabricator-crumbs-view-css' => 'd831cac3',
'phabricator-drag-and-drop-file-upload' => '5e9e5c4e',
'phabricator-dropdown-menu' => '2c1dba03',
'phabricator-file-upload' => '2c1dba03',
'phabricator-filetree-view-css' => 'f0d63822',
'phabricator-flag-css' => 'f0d63822',
'phabricator-filetree-view-css' => 'd831cac3',
'phabricator-flag-css' => 'd831cac3',
'phabricator-hovercard' => '2c1dba03',
'phabricator-jump-nav' => 'f0d63822',
'phabricator-jump-nav' => 'd831cac3',
'phabricator-keyboard-shortcut' => '2c1dba03',
'phabricator-keyboard-shortcut-manager' => '2c1dba03',
'phabricator-main-menu-view' => 'f0d63822',
'phabricator-main-menu-view' => 'd831cac3',
'phabricator-menu-item' => '2c1dba03',
'phabricator-nav-view-css' => 'f0d63822',
'phabricator-nav-view-css' => 'd831cac3',
'phabricator-notification' => '2c1dba03',
'phabricator-notification-css' => 'f0d63822',
'phabricator-notification-menu-css' => 'f0d63822',
'phabricator-notification-css' => 'd831cac3',
'phabricator-notification-menu-css' => 'd831cac3',
'phabricator-object-selector-css' => '1084b12b',
'phabricator-phtize' => '2c1dba03',
'phabricator-prefab' => '2c1dba03',
'phabricator-project-tag-css' => '49898640',
'phabricator-remarkup-css' => 'f0d63822',
'phabricator-remarkup-css' => 'd831cac3',
'phabricator-shaped-request' => '5e9e5c4e',
'phabricator-side-menu-view-css' => 'f0d63822',
'phabricator-standard-page-view' => 'f0d63822',
'phabricator-tag-view-css' => 'f0d63822',
'phabricator-side-menu-view-css' => 'd831cac3',
'phabricator-standard-page-view' => 'd831cac3',
'phabricator-tag-view-css' => 'd831cac3',
'phabricator-textareautils' => '2c1dba03',
'phabricator-tooltip' => '2c1dba03',
'phabricator-transaction-view-css' => 'f0d63822',
'phabricator-zindex-css' => 'f0d63822',
'phui-button-css' => 'f0d63822',
'phui-form-css' => 'f0d63822',
'phui-form-view-css' => 'f0d63822',
'phui-header-view-css' => 'f0d63822',
'phui-icon-view-css' => 'f0d63822',
'phui-list-view-css' => 'f0d63822',
'phui-object-item-list-view-css' => 'f0d63822',
'phui-property-list-view-css' => 'f0d63822',
'phui-spacing-css' => 'f0d63822',
'sprite-apps-large-css' => 'f0d63822',
'sprite-gradient-css' => 'f0d63822',
'sprite-icons-css' => 'f0d63822',
'sprite-menu-css' => 'f0d63822',
'sprite-status-css' => 'f0d63822',
'syntax-highlighting-css' => 'f0d63822',
'phabricator-transaction-view-css' => 'd831cac3',
'phabricator-zindex-css' => 'd831cac3',
'phui-button-css' => 'd831cac3',
'phui-form-css' => 'd831cac3',
'phui-form-view-css' => 'd831cac3',
'phui-header-view-css' => 'd831cac3',
'phui-icon-view-css' => 'd831cac3',
'phui-list-view-css' => 'd831cac3',
'phui-object-item-list-view-css' => 'd831cac3',
'phui-property-list-view-css' => 'd831cac3',
'phui-spacing-css' => 'd831cac3',
'sprite-apps-large-css' => 'd831cac3',
'sprite-gradient-css' => 'd831cac3',
'sprite-icons-css' => 'd831cac3',
'sprite-menu-css' => 'd831cac3',
'sprite-status-css' => 'd831cac3',
'syntax-highlighting-css' => 'd831cac3',
),
));
......@@ -39,8 +39,20 @@ final class PhabricatorEmailVerificationController
$continue = pht('Continue to Phabricator');
} else {
$guard = AphrontWriteGuard::beginScopedUnguardedWrites();
$email->setIsVerified(1);
$email->save();
$email->openTransaction();
$email->setIsVerified(1);
$email->save();
// If the user just verified their primary email address, mark their
// account as email verified.
$user_primary = $user->loadPrimaryEmail();
if ($user_primary->getID() == $email->getID()) {
$user->setIsEmailVerified(1);
$user->save();
}
$email->saveTransaction();
unset($guard);
$title = pht('Address Verified');
......
......@@ -31,42 +31,33 @@ final class PhabricatorMustVerifyEmailController
$sent = new AphrontErrorView();
$sent->setSeverity(AphrontErrorView::SEVERITY_NOTICE);
$sent->setTitle(pht('Email Sent'));
$sent->appendChild(phutil_tag(
'p',
array(),
$sent->appendChild(
pht(
'Another verification email was sent to %s.',
phutil_tag('strong', array(), $email_address))));
phutil_tag('strong', array(), $email_address)));
}
$error_view = new AphrontRequestFailureView();
$error_view->setHeader(pht('Check Your Email'));
$error_view->appendChild(phutil_tag('p', array(), pht(
'You must verify your email address to login. You should have a new '.
'email message from Phabricator with verification instructions in your '.
'inbox (%s).', phutil_tag('strong', array(), $email_address))));
$error_view->appendChild(phutil_tag('p', array(), pht(
$must_verify = pht(
'You must verify your email address to login. You should have a '.
'new email message from Phabricator with verification instructions '.
'in your inbox (%s).',
phutil_tag('strong', array(), $email_address));
$send_again = pht(
'If you did not receive an email, you can click the button below '.
'to try sending another one.')));
$error_view->appendChild(phutil_tag_div(
'aphront-failure-continue',
phabricator_form(
$user,
array(
'action' => '/login/mustverify/',
'method' => 'POST',
),
phutil_tag(
'button',
array(
),
pht('Send Another Email')))));
'to try sending another one.');
$dialog = id(new AphrontDialogView())
->setUser($user)
->setTitle(pht('Check Your Email'))
->appendParagraph($must_verify)
->appendParagraph($send_again)
->addSubmitButton(pht('Send Another Email'));
return $this->buildApplicationPage(
array(
$sent,
$error_view,
$dialog,
),
array(
'title' => pht('Must Verify Email'),
......
......@@ -114,12 +114,7 @@ abstract class PhabricatorController extends AphrontController {
if ($user->isLoggedIn()) {
if ($this->shouldRequireEmailVerification()) {
$email = $user->loadPrimaryEmail();
if (!$email) {
throw new Exception(
"No primary email address associated with this account!");
}
if (!$email->getIsVerified()) {
if (!$user->getIsEmailVerified()) {
$controller = new PhabricatorMustVerifyEmailController($request);
$this->setCurrentApplication($auth_application);
return $this->delegateToController($controller);
......
......@@ -31,7 +31,7 @@ final class PhabricatorAccessControlTestCase
$u_unverified = $this->generateNewTestUser()
->setUsername('unverified')
->save();
$u_unverified->loadPrimaryEmail()->setIsVerified(0)->save();
$u_unverified->setIsEmailVerified(0)->save();
$u_normal = $this->generateNewTestUser()
->setUsername('normal')
......
......@@ -313,24 +313,11 @@ final class PhabricatorConduitAPIController
ConduitAPIRequest $request,
PhabricatorUser $user) {
if ($user->getIsDisabled()) {
if (!$user->isUserActivated()) {
return array(
'ERR-USER-DISABLED',
'User is disabled.');
}
if (PhabricatorUserEmail::isEmailVerificationRequired()) {
$email = $user->loadPrimaryEmail();
if (!$email) {
return array(
'ERR-USER-NOEMAIL',
'User has no primary email address.');
}
if (!$email->getIsVerified()) {
return array(
'ERR-USER-UNVERIFIED',
'User has unverified email address.');
}
pht('User account is not activated.'),
);
}
$request->setUser($user);
......
......@@ -382,6 +382,11 @@ final class DiffusionServeController extends DiffusionController {
return null;
}
if (!$user->isUserActivated()) {
// User is not activated.
return null;
}
$password_entry = id(new PhabricatorRepositoryVCSPassword())
->loadOneWhere('userPHID = %s', $user->getPHID());
if (!$password_entry) {
......@@ -394,11 +399,6 @@ final class DiffusionServeController extends DiffusionController {
return null;
}
if ($user->getIsDisabled()) {
// User is disabled.
return null;
}
return $user;
}
......
......@@ -219,7 +219,7 @@ final class HeraldRuleQuery
$rule->attachValidAuthor(false);
continue;
}
if ($users[$author_phid]->getIsDisabled()) {
if (!$users[$author_phid]->isUserActivated()) {
$rule->attachValidAuthor(false);
continue;
}
......
......@@ -82,6 +82,11 @@ final class PhabricatorMetaMTAActorQuery extends PhabricatorQuery {
$actor->setUndeliverable(
pht('This user is a bot; bot accounts do not receive mail.'));
}
// NOTE: We do send email to unapproved users, and to unverified users,
// because it would otherwise be impossible to get them to verify their
// email addresses. Possibly we should white-list this kind of mail and
// deny all other types of mail.
}
$email = idx($emails, $phid);
......
......@@ -19,16 +19,13 @@ abstract class PhabricatorMailReceiver {
PhabricatorMetaMTAReceivedMail $mail,
PhabricatorUser $sender) {
if ($sender->getIsDisabled()) {
if (!$sender->isUserActivated()) {
throw new PhabricatorMetaMTAReceivedMailProcessingException(
MetaMTAReceivedMailStatus::STATUS_DISABLED_SENDER,
pht(
"Sender '%s' has a disabled user account.",
"Sender '%s' does not have an activated user account.",
$sender->getUsername()));
}
return;
}
/**
......
......@@ -32,6 +32,14 @@ abstract class ConduitAPI_user_Method extends ConduitAPIMethod {
$roles[] = 'unverified';
}
if ($user->getIsApproved()) {
$roles[] = 'approved';
}
if ($user->isUserActivated()) {
$roles[] = 'activated';
}
$return = array(
'phid' => $user->getPHID(),
'userName' => $user->getUserName(),
......
......@@ -325,7 +325,9 @@ final class PhabricatorPeopleEditController
if ($user->getIsDisabled()) {
$roles[] = pht('Disabled');
}
if (!$user->getIsApproved()) {
$roles[] = pht('Not Approved');
}
if (!$roles) {
$roles[] = pht('Normal User');
}
......
......@@ -61,6 +61,10 @@ final class PhabricatorPeopleListController extends PhabricatorPeopleController
$item->addIcon('disable', pht('Disabled'));
}
if (!$user->getIsApproved()) {
$item->addIcon('raise-priority', pht('Not Approved'));
}
if ($user->getIsAdmin()) {
$item->addIcon('highlight', pht('Admin'));
}
......
......@@ -31,6 +31,9 @@ final class PhabricatorUserRolesField
if ($user->getIsDisabled()) {
$roles[] = pht('Disabled');
}
if (!$user->getIsApproved()) {
$roles[] = pht('Not Approved');
}
if ($user->getIsSystemAgent()) {
$roles[] = pht('Bot');
}
......
......@@ -41,6 +41,12 @@ final class PhabricatorUserEditor extends PhabricatorEditor {
// Always set a new user's email address to primary.
$email->setIsPrimary(1);
// If the primary address is already verified, also set the verified flag
// on the user themselves.
if ($email->getIsVerified()) {
$user->setIsEmailVerified(1);
}
$this->willAddEmail($email);
$user->openTransaction();
......
......@@ -35,6 +35,8 @@ final class PhabricatorPeopleHovercardEventListener
if ($user->getIsDisabled()) {
$hovercard->addField(pht('Account'), pht('Disabled'));
} else if (!$user->isUserActivated()) {
$hovercard->addField(pht('Account'), pht('Not Activated'));
} else {
$statuses = id(new PhabricatorUserStatus())->loadCurrentStatuses(
array($user->getPHID()));
......
......@@ -38,7 +38,7 @@ final class PhabricatorPeoplePHIDTypeUser extends PhabricatorPHIDType {
$handle->setFullName(
$user->getUsername().' ('.$user->getRealName().')');
$handle->setImageURI($user->loadProfileImageURI());
$handle->setDisabled($user->getIsDisabled());
$handle->setDisabled(!$user->isUserActivated());
if ($user->hasStatus()) {
$status = $user->getStatus();
$handle->setStatus($status->getTextStatus());
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment