From 7f11e8d7401e83c8f6bbc1e8c98dafae26b1a711 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 12 Nov 2013 14:37:04 -0800 Subject: [PATCH] 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 --- .../patches/20131112.userverified.1.col.sql | 10 ++ .../patches/20131112.userverified.2.mig.php | 33 ++++++ scripts/ssh/ssh-exec.php | 4 +- src/__celerity_resource_map__.php | 110 +++++++++--------- ...PhabricatorEmailVerificationController.php | 16 ++- .../PhabricatorMustVerifyEmailController.php | 45 +++---- .../base/controller/PhabricatorController.php | 7 +- .../PhabricatorAccessControlTestCase.php | 2 +- .../PhabricatorConduitAPIController.php | 19 +-- .../controller/DiffusionServeController.php | 10 +- .../herald/query/HeraldRuleQuery.php | 2 +- .../query/PhabricatorMetaMTAActorQuery.php | 5 + .../receiver/PhabricatorMailReceiver.php | 7 +- .../people/conduit/ConduitAPI_user_Method.php | 8 ++ .../PhabricatorPeopleEditController.php | 4 +- .../PhabricatorPeopleListController.php | 4 + .../customfield/PhabricatorUserRolesField.php | 3 + .../people/editor/PhabricatorUserEditor.php | 6 + ...habricatorPeopleHovercardEventListener.php | 2 + .../phid/PhabricatorPeoplePHIDTypeUser.php | 2 +- .../PhabricatorRemarkupRuleMention.php | 2 +- .../search/PhabricatorUserSearchIndexer.php | 2 +- .../people/storage/PhabricatorUser.php | 32 +++++ .../patch/PhabricatorBuiltinPatchList.php | 8 ++ .../testing/PhabricatorTestCase.php | 5 + src/view/AphrontDialogView.php | 10 ++ webroot/rsrc/css/aphront/dialog-view.css | 4 + 27 files changed, 238 insertions(+), 124 deletions(-) create mode 100644 resources/sql/patches/20131112.userverified.1.col.sql create mode 100644 resources/sql/patches/20131112.userverified.2.mig.php diff --git a/resources/sql/patches/20131112.userverified.1.col.sql b/resources/sql/patches/20131112.userverified.1.col.sql new file mode 100644 index 0000000000..0612e79825 --- /dev/null +++ b/resources/sql/patches/20131112.userverified.1.col.sql @@ -0,0 +1,10 @@ +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; diff --git a/resources/sql/patches/20131112.userverified.2.mig.php b/resources/sql/patches/20131112.userverified.2.mig.php new file mode 100644 index 0000000000..381fc966ab --- /dev/null +++ b/resources/sql/patches/20131112.userverified.2.mig.php @@ -0,0 +1,33 @@ +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"; diff --git a/scripts/ssh/ssh-exec.php b/scripts/ssh/ssh-exec.php index 5b518e01b0..6d8763b901 100755 --- a/scripts/ssh/ssh-exec.php +++ b/scripts/ssh/ssh-exec.php @@ -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')) { diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 0746bd8bb5..012818dfc6 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -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', ), )); diff --git a/src/applications/auth/controller/PhabricatorEmailVerificationController.php b/src/applications/auth/controller/PhabricatorEmailVerificationController.php index 1836e10153..091575309e 100644 --- a/src/applications/auth/controller/PhabricatorEmailVerificationController.php +++ b/src/applications/auth/controller/PhabricatorEmailVerificationController.php @@ -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'); diff --git a/src/applications/auth/controller/PhabricatorMustVerifyEmailController.php b/src/applications/auth/controller/PhabricatorMustVerifyEmailController.php index 7b580a1ef7..beae85a3a9 100644 --- a/src/applications/auth/controller/PhabricatorMustVerifyEmailController.php +++ b/src/applications/auth/controller/PhabricatorMustVerifyEmailController.php @@ -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( - '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'))))); + $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.'); + + $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'), diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php index d4cbd60de7..6d7a6f0ae5 100644 --- a/src/applications/base/controller/PhabricatorController.php +++ b/src/applications/base/controller/PhabricatorController.php @@ -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); diff --git a/src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php b/src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php index 1d6be0e793..28e7ca618e 100644 --- a/src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php +++ b/src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php @@ -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') diff --git a/src/applications/conduit/controller/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/PhabricatorConduitAPIController.php index ce6b6a02c9..b08873f51c 100644 --- a/src/applications/conduit/controller/PhabricatorConduitAPIController.php +++ b/src/applications/conduit/controller/PhabricatorConduitAPIController.php @@ -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); diff --git a/src/applications/diffusion/controller/DiffusionServeController.php b/src/applications/diffusion/controller/DiffusionServeController.php index 3a7312c39a..c009fbe83e 100644 --- a/src/applications/diffusion/controller/DiffusionServeController.php +++ b/src/applications/diffusion/controller/DiffusionServeController.php @@ -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; } diff --git a/src/applications/herald/query/HeraldRuleQuery.php b/src/applications/herald/query/HeraldRuleQuery.php index e9d3d05d5f..2b29248745 100644 --- a/src/applications/herald/query/HeraldRuleQuery.php +++ b/src/applications/herald/query/HeraldRuleQuery.php @@ -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; } diff --git a/src/applications/metamta/query/PhabricatorMetaMTAActorQuery.php b/src/applications/metamta/query/PhabricatorMetaMTAActorQuery.php index 2e893dc93f..ad94b9ff31 100644 --- a/src/applications/metamta/query/PhabricatorMetaMTAActorQuery.php +++ b/src/applications/metamta/query/PhabricatorMetaMTAActorQuery.php @@ -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); diff --git a/src/applications/metamta/receiver/PhabricatorMailReceiver.php b/src/applications/metamta/receiver/PhabricatorMailReceiver.php index d621497ec1..8899b182e7 100644 --- a/src/applications/metamta/receiver/PhabricatorMailReceiver.php +++ b/src/applications/metamta/receiver/PhabricatorMailReceiver.php @@ -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; } /** diff --git a/src/applications/people/conduit/ConduitAPI_user_Method.php b/src/applications/people/conduit/ConduitAPI_user_Method.php index a8207f4174..8f9e2f899b 100644 --- a/src/applications/people/conduit/ConduitAPI_user_Method.php +++ b/src/applications/people/conduit/ConduitAPI_user_Method.php @@ -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(), diff --git a/src/applications/people/controller/PhabricatorPeopleEditController.php b/src/applications/people/controller/PhabricatorPeopleEditController.php index ab466dabf7..904402dfb6 100644 --- a/src/applications/people/controller/PhabricatorPeopleEditController.php +++ b/src/applications/people/controller/PhabricatorPeopleEditController.php @@ -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'); } diff --git a/src/applications/people/controller/PhabricatorPeopleListController.php b/src/applications/people/controller/PhabricatorPeopleListController.php index 878ca311ca..b084de30bb 100644 --- a/src/applications/people/controller/PhabricatorPeopleListController.php +++ b/src/applications/people/controller/PhabricatorPeopleListController.php @@ -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')); } diff --git a/src/applications/people/customfield/PhabricatorUserRolesField.php b/src/applications/people/customfield/PhabricatorUserRolesField.php index dca0b3aaf3..add52385a2 100644 --- a/src/applications/people/customfield/PhabricatorUserRolesField.php +++ b/src/applications/people/customfield/PhabricatorUserRolesField.php @@ -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'); } diff --git a/src/applications/people/editor/PhabricatorUserEditor.php b/src/applications/people/editor/PhabricatorUserEditor.php index f8fb6811a6..e0835cb789 100644 --- a/src/applications/people/editor/PhabricatorUserEditor.php +++ b/src/applications/people/editor/PhabricatorUserEditor.php @@ -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(); diff --git a/src/applications/people/event/PhabricatorPeopleHovercardEventListener.php b/src/applications/people/event/PhabricatorPeopleHovercardEventListener.php index 31ebf778c2..380e446b64 100644 --- a/src/applications/people/event/PhabricatorPeopleHovercardEventListener.php +++ b/src/applications/people/event/PhabricatorPeopleHovercardEventListener.php @@ -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())); diff --git a/src/applications/people/phid/PhabricatorPeoplePHIDTypeUser.php b/src/applications/people/phid/PhabricatorPeoplePHIDTypeUser.php index a0bce6ea47..a6df8adb15 100644 --- a/src/applications/people/phid/PhabricatorPeoplePHIDTypeUser.php +++ b/src/applications/people/phid/PhabricatorPeoplePHIDTypeUser.php @@ -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()); diff --git a/src/applications/people/remarkup/PhabricatorRemarkupRuleMention.php b/src/applications/people/remarkup/PhabricatorRemarkupRuleMention.php index 31d2f4a2cf..1ca9e0a231 100644 --- a/src/applications/people/remarkup/PhabricatorRemarkupRuleMention.php +++ b/src/applications/people/remarkup/PhabricatorRemarkupRuleMention.php @@ -107,7 +107,7 @@ final class PhabricatorRemarkupRuleMention ->setName('@'.$user->getUserName()) ->setHref('/p/'.$user->getUserName().'/'); - if ($user->getIsDisabled()) { + if (!$user->isUserActivated()) { $tag->setDotColor(PhabricatorTagView::COLOR_GREY); } else { $status = idx($user_statuses, $user->getPHID()); diff --git a/src/applications/people/search/PhabricatorUserSearchIndexer.php b/src/applications/people/search/PhabricatorUserSearchIndexer.php index 8e7cd6984d..9a8c069610 100644 --- a/src/applications/people/search/PhabricatorUserSearchIndexer.php +++ b/src/applications/people/search/PhabricatorUserSearchIndexer.php @@ -20,7 +20,7 @@ final class PhabricatorUserSearchIndexer // TODO: Index the blurbs from their profile or something? Probably not // actually useful... - if (!$user->getIsDisabled()) { + if ($user->isUserActivated()) { $doc->addRelationship( PhabricatorSearchRelationship::RELATIONSHIP_OPEN, $user->getPHID(), diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index a0e0552635..5034072715 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -29,6 +29,8 @@ final class PhabricatorUser protected $isSystemAgent = 0; protected $isAdmin = 0; protected $isDisabled = 0; + protected $isEmailVerified = 0; + protected $isApproved = 1; private $profileImage = null; private $profile = null; @@ -51,11 +53,41 @@ final class PhabricatorUser return (bool)$this->isDisabled; case 'isSystemAgent': return (bool)$this->isSystemAgent; + case 'isEmailVerified': + return (bool)$this->isEmailVerified; + case 'isApproved': + return (bool)$this->isApproved; default: return parent::readField($field); } } + + /** + * Is this a live account which has passed required approvals? Returns true + * if this is an enabled, verified (if required), approved (if required) + * account, and false otherwise. + * + * @return bool True if this is a standard, usable account. + */ + public function isUserActivated() { + if ($this->getIsDisabled()) { + return false; + } + + if (!$this->getIsApproved()) { + return false; + } + + if (PhabricatorUserEmail::isEmailVerificationRequired()) { + if (!$this->getIsEmailVerified()) { + return false; + } + } + + return true; + } + public function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index 08a4c21349..940a78e427 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1748,6 +1748,14 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('20131107.buildlog.sql'), ), + '20131112.userverified.1.col.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('20131112.userverified.1.col.sql'), + ), + '20131112.userverified.2.mig.php' => array( + 'type' => 'php', + 'name' => $this->getPatchPath('20131112.userverified.2.mig.php'), + ), ); } } diff --git a/src/infrastructure/testing/PhabricatorTestCase.php b/src/infrastructure/testing/PhabricatorTestCase.php index 18c0ed7dcf..85627c0bfb 100644 --- a/src/infrastructure/testing/PhabricatorTestCase.php +++ b/src/infrastructure/testing/PhabricatorTestCase.php @@ -105,6 +105,11 @@ abstract class PhabricatorTestCase extends ArcanistPhutilTestCase { 'phabricator.show-beta-applications', true); + // Reset application settings to defaults, particularly policies. + $this->env->overrideEnvConfig( + 'phabricator.application-settings', + array()); + // TODO: Remove this when we remove "releeph.installed". $this->env->overrideEnvConfig('releeph.installed', true); } diff --git a/src/view/AphrontDialogView.php b/src/view/AphrontDialogView.php index 1a590c4b6c..4076106c95 100644 --- a/src/view/AphrontDialogView.php +++ b/src/view/AphrontDialogView.php @@ -111,6 +111,16 @@ final class AphrontDialogView extends AphrontView { return $this; } + public function appendParagraph($paragraph) { + return $this->appendChild( + phutil_tag( + 'p', + array( + 'class' => 'aphront-dialog-view-paragraph', + ), + $paragraph)); + } + final public function render() { require_celerity_resource('aphront-dialog-view-css'); diff --git a/webroot/rsrc/css/aphront/dialog-view.css b/webroot/rsrc/css/aphront/dialog-view.css index 0462c83017..ea77b9852c 100644 --- a/webroot/rsrc/css/aphront/dialog-view.css +++ b/webroot/rsrc/css/aphront/dialog-view.css @@ -124,3 +124,7 @@ .aphront-capability-details { margin: 20px 0 4px; } + +.aphront-dialog-view-paragraph + .aphront-dialog-view-paragraph { + margin-top: 16px; +}