diff --git a/scripts/user/account_admin.php b/scripts/user/account_admin.php index 2570f0a3ae..2a7d663b71 100755 --- a/scripts/user/account_admin.php +++ b/scripts/user/account_admin.php @@ -121,7 +121,6 @@ $is_admin = $user->getIsAdmin(); $set_admin = phutil_console_confirm( 'Should this user be an administrator?', $default_no = !$is_admin); -$user->setIsAdmin($set_admin); echo "\n\nACCOUNT SUMMARY\n\n"; $tpl = "%12s %-30s %-30s\n"; @@ -149,21 +148,30 @@ if (!phutil_console_confirm("Save these changes?", $default_no = false)) { exit(1); } -$user->save(); -if ($changed_pass !== false) { - // This must happen after saving the user because we use their PHID as a - // component of the password hash. - $user->setPassword($changed_pass); - $user->save(); -} +$user->openTransaction(); -if ($new_email) { - id(new PhabricatorUserEmail()) - ->setUserPHID($user->getPHID()) - ->setAddress($new_email) - ->setIsVerified(1) - ->setIsPrimary(1) - ->save(); -} + $editor = new PhabricatorUserEditor(); + + // TODO: This is wrong, but we have a chicken-and-egg problem when you use + // this script to create the first user. + $editor->setActor($user); + + if ($new_email) { + $email = id(new PhabricatorUserEmail()) + ->setAddress($new_email) + ->setIsVerified(1); + + $editor->createNewUser($user, $email); + } else { + $editor->updateUser($user); + } + + $editor->makeAdminUser($user, $set_admin); + + if ($changed_pass !== false) { + $editor->changePassword($user, $changed_pass); + } + +$user->saveTransaction(); echo "Saved changes.\n"; diff --git a/scripts/user/add_user.php b/scripts/user/add_user.php index 2ab010e29c..9b18a673a9 100755 --- a/scripts/user/add_user.php +++ b/scripts/user/add_user.php @@ -61,14 +61,14 @@ if ($existing_email) { $user = new PhabricatorUser(); $user->setUsername($username); $user->setRealname($realname); -$user->save(); $email_object = id(new PhabricatorUserEmail()) - ->setUserPHID($user->getPHID()) ->setAddress($email) - ->setIsVerified(1) - ->setIsPrimary(1) - ->save(); + ->setIsVerified(1); + +id(new PhabricatorUserEditor()) + ->setActor($admin) + ->createNewUser($user, $email_object); $user->sendWelcomeEmail($admin); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 22072a763d..c3a00d6f07 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -956,6 +956,7 @@ phutil_register_library_map(array( 'PhabricatorUserAccountSettingsPanelController' => 'applications/people/controller/settings/panels/account', 'PhabricatorUserConduitSettingsPanelController' => 'applications/people/controller/settings/panels/conduit', 'PhabricatorUserDAO' => 'applications/people/storage/base', + 'PhabricatorUserEditor' => 'applications/people/editor', 'PhabricatorUserEmail' => 'applications/people/storage/email', 'PhabricatorUserEmailPreferenceSettingsPanelController' => 'applications/people/controller/settings/panels/emailpref', 'PhabricatorUserEmailSettingsPanelController' => 'applications/people/controller/settings/panels/email', diff --git a/src/applications/auth/controller/oauthregistration/default/PhabricatorOAuthDefaultRegistrationController.php b/src/applications/auth/controller/oauthregistration/default/PhabricatorOAuthDefaultRegistrationController.php index 8bc0333ce5..5a52d8fdaf 100644 --- a/src/applications/auth/controller/oauthregistration/default/PhabricatorOAuthDefaultRegistrationController.php +++ b/src/applications/auth/controller/oauthregistration/default/PhabricatorOAuthDefaultRegistrationController.php @@ -83,7 +83,6 @@ final class PhabricatorOAuthDefaultRegistrationController } try { - $user->save(); // NOTE: We don't verify OAuth email addresses by default because // OAuth providers might associate email addresses with accounts that @@ -92,12 +91,14 @@ final class PhabricatorOAuthDefaultRegistrationController // verifying an email address are high because having a corporate // address at a company is sometimes the key to the castle. - $new_email = id(new PhabricatorUserEmail()) - ->setUserPHID($user->getPHID()) + + $email_obj = id(new PhabricatorUserEmail()) ->setAddress($new_email) - ->setIsPrimary(1) - ->setIsVerified(0) - ->save(); + ->setIsVerified(0); + + id(new PhabricatorUserEditor()) + ->setActor($user) + ->createNewUser($user, $email_obj); $oauth_info->setUserID($user->getID()); $oauth_info->save(); @@ -106,7 +107,7 @@ final class PhabricatorOAuthDefaultRegistrationController $request->setCookie('phusr', $user->getUsername()); $request->setCookie('phsid', $session_key); - $new_email->sendVerificationEmail($user); + $email_obj->sendVerificationEmail($user); return id(new AphrontRedirectResponse())->setURI('/'); } catch (AphrontQueryDuplicateKeyException $exception) { diff --git a/src/applications/auth/controller/oauthregistration/default/__init__.php b/src/applications/auth/controller/oauthregistration/default/__init__.php index 5062b237bb..fdb9a3fcb2 100644 --- a/src/applications/auth/controller/oauthregistration/default/__init__.php +++ b/src/applications/auth/controller/oauthregistration/default/__init__.php @@ -9,6 +9,7 @@ phutil_require_module('phabricator', 'aphront/response/redirect'); phutil_require_module('phabricator', 'applications/auth/controller/oauthregistration/base'); phutil_require_module('phabricator', 'applications/files/storage/file'); +phutil_require_module('phabricator', 'applications/people/editor'); phutil_require_module('phabricator', 'applications/people/storage/email'); phutil_require_module('phabricator', 'applications/people/storage/user'); phutil_require_module('phabricator', 'view/form/base'); diff --git a/src/applications/conduit/method/user/disable/ConduitAPI_user_disable_Method.php b/src/applications/conduit/method/user/disable/ConduitAPI_user_disable_Method.php index 29c32016ec..7aa5503ccb 100644 --- a/src/applications/conduit/method/user/disable/ConduitAPI_user_disable_Method.php +++ b/src/applications/conduit/method/user/disable/ConduitAPI_user_disable_Method.php @@ -44,7 +44,8 @@ final class ConduitAPI_user_disable_Method } protected function execute(ConduitAPIRequest $request) { - if (!$request->getUser()->getIsAdmin()) { + $actor = $request->getUser(); + if (!$actor->getIsAdmin()) { throw new ConduitException('ERR-PERMISSIONS'); } @@ -59,8 +60,9 @@ final class ConduitAPI_user_disable_Method } foreach ($users as $user) { - $user->setIsDisabled(true); - $user->save(); + id(new PhabricatorUserEditor()) + ->setActor($actor) + ->disableUser($user, true); } } diff --git a/src/applications/conduit/method/user/disable/__init__.php b/src/applications/conduit/method/user/disable/__init__.php index c8554888c6..c6a13b9a8a 100644 --- a/src/applications/conduit/method/user/disable/__init__.php +++ b/src/applications/conduit/method/user/disable/__init__.php @@ -8,6 +8,7 @@ phutil_require_module('phabricator', 'applications/conduit/method/user/base'); phutil_require_module('phabricator', 'applications/conduit/protocol/exception'); +phutil_require_module('phabricator', 'applications/people/editor'); phutil_require_module('phabricator', 'applications/people/storage/user'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/people/controller/edit/PhabricatorPeopleEditController.php b/src/applications/people/controller/edit/PhabricatorPeopleEditController.php index 5644d6c7a8..944021e290 100644 --- a/src/applications/people/controller/edit/PhabricatorPeopleEditController.php +++ b/src/applications/people/controller/edit/PhabricatorPeopleEditController.php @@ -165,22 +165,18 @@ final class PhabricatorPeopleEditController try { $is_new = !$user->getID(); - $user->save(); - - if ($is_new) { - + if (!$is_new) { + id(new PhabricatorUserEditor()) + ->setActor($admin) + ->updateUser($user); + } else { $email = id(new PhabricatorUserEmail()) - ->setUserPHID($user->getPHID()) ->setAddress($new_email) - ->setIsPrimary(1) - ->setIsVerified(0) - ->save(); + ->setIsVerified(0); - $log = PhabricatorUserLog::newLog( - $admin, - $user, - PhabricatorUserLog::ACTION_CREATE); - $log->save(); + id(new PhabricatorUserEditor()) + ->setActor($admin) + ->createNewUser($user, $email); if ($welcome_checked) { $user->sendWelcomeEmail($admin); @@ -255,13 +251,17 @@ final class PhabricatorPeopleEditController ->setValue($new_email) ->setError($e_email)); } else { + $email = $user->loadPrimaryEmail(); + if ($email) { + $status = $email->getIsVerified() ? 'Verified' : 'Unverified'; + } else { + $status = 'No Email Address'; + } + $form->appendChild( id(new AphrontFormStaticControl()) ->setLabel('Email') - ->setValue( - $user->loadPrimaryEmail()->getIsVerified() - ? 'Verified' - : 'Unverified')); + ->setValue($status)); } $form->appendChild($this->getRoleInstructions()); @@ -354,31 +354,21 @@ final class PhabricatorPeopleEditController $new_admin = (bool)$request->getBool('is_admin'); $old_admin = (bool)$user->getIsAdmin(); if ($new_admin != $old_admin) { - $log = clone $log_template; - $log->setAction(PhabricatorUserLog::ACTION_ADMIN); - $log->setOldValue($old_admin); - $log->setNewValue($new_admin); - $user->setIsAdmin($new_admin); - $logs[] = $log; + id(new PhabricatorUserEditor()) + ->setActor($admin) + ->makeAdminUser($user, $new_admin); } $new_disabled = (bool)$request->getBool('is_disabled'); $old_disabled = (bool)$user->getIsDisabled(); if ($new_disabled != $old_disabled) { - $log = clone $log_template; - $log->setAction(PhabricatorUserLog::ACTION_DISABLE); - $log->setOldValue($old_disabled); - $log->setNewValue($new_disabled); - $user->setIsDisabled($new_disabled); - $logs[] = $log; + id(new PhabricatorUserEditor()) + ->setActor($admin) + ->disableUser($user, $new_disabled); } } if (!$errors) { - $user->save(); - foreach ($logs as $log) { - $log->save(); - } return id(new AphrontRedirectResponse()) ->setURI($request->getRequestURI()->alter('saved', 'true')); } diff --git a/src/applications/people/controller/edit/__init__.php b/src/applications/people/controller/edit/__init__.php index 9ee6aaffb0..cd75f2473c 100644 --- a/src/applications/people/controller/edit/__init__.php +++ b/src/applications/people/controller/edit/__init__.php @@ -9,6 +9,7 @@ phutil_require_module('phabricator', 'aphront/response/404'); phutil_require_module('phabricator', 'aphront/response/redirect'); phutil_require_module('phabricator', 'applications/people/controller/base'); +phutil_require_module('phabricator', 'applications/people/editor'); phutil_require_module('phabricator', 'applications/people/storage/email'); phutil_require_module('phabricator', 'applications/people/storage/log'); phutil_require_module('phabricator', 'applications/people/storage/user'); diff --git a/src/applications/people/controller/settings/panels/email/PhabricatorUserEmailSettingsPanelController.php b/src/applications/people/controller/settings/panels/email/PhabricatorUserEmailSettingsPanelController.php index f50a1e968b..362363b1d6 100644 --- a/src/applications/people/controller/settings/panels/email/PhabricatorUserEmailSettingsPanelController.php +++ b/src/applications/people/controller/settings/panels/email/PhabricatorUserEmailSettingsPanelController.php @@ -175,13 +175,14 @@ final class PhabricatorUserEmailSettingsPanelController if (!$errors) { $object = id(new PhabricatorUserEmail()) - ->setUserPHID($user->getPHID()) ->setAddress($email) - ->setIsVerified(0) - ->setIsPrimary(0); + ->setIsVerified(0); try { - $object->save(); + + id(new PhabricatorUserEditor()) + ->setActor($user) + ->addEmail($user, $object); $object->sendVerificationEmail($user); @@ -245,7 +246,11 @@ final class PhabricatorUserEmailSettingsPanelController } if ($request->isFormPost()) { - $email->delete(); + + id(new PhabricatorUserEditor()) + ->setActor($user) + ->removeEmail($user, $email); + return id(new AphrontRedirectResponse())->setURI($uri); } @@ -314,21 +319,9 @@ final class PhabricatorUserEmailSettingsPanelController if ($request->isFormPost()) { - // TODO: Transactions! - - $email->setIsPrimary(1); - - $old_primary = $user->loadPrimaryEmail(); - if ($old_primary) { - $old_primary->setIsPrimary(0); - $old_primary->save(); - } - $email->save(); - - if ($old_primary) { - $old_primary->sendOldPrimaryEmail($user, $email); - } - $email->sendNewPrimaryEmail($user); + id(new PhabricatorUserEditor()) + ->setActor($user) + ->changePrimaryEmail($user, $email); return id(new AphrontRedirectResponse())->setURI($uri); } diff --git a/src/applications/people/controller/settings/panels/email/__init__.php b/src/applications/people/controller/settings/panels/email/__init__.php index 86aed32284..7dcb8c2a16 100644 --- a/src/applications/people/controller/settings/panels/email/__init__.php +++ b/src/applications/people/controller/settings/panels/email/__init__.php @@ -11,6 +11,7 @@ phutil_require_module('phabricator', 'aphront/response/dialog'); phutil_require_module('phabricator', 'aphront/response/redirect'); phutil_require_module('phabricator', 'aphront/response/reload'); phutil_require_module('phabricator', 'applications/people/controller/settings/panels/base'); +phutil_require_module('phabricator', 'applications/people/editor'); phutil_require_module('phabricator', 'applications/people/storage/email'); phutil_require_module('phabricator', 'infrastructure/javelin/markup'); phutil_require_module('phabricator', 'view/control/table'); diff --git a/src/applications/people/controller/settings/panels/password/PhabricatorUserPasswordSettingsPanelController.php b/src/applications/people/controller/settings/panels/password/PhabricatorUserPasswordSettingsPanelController.php index 9479aeb03f..3f2ca1021a 100644 --- a/src/applications/people/controller/settings/panels/password/PhabricatorUserPasswordSettingsPanelController.php +++ b/src/applications/people/controller/settings/panels/password/PhabricatorUserPasswordSettingsPanelController.php @@ -79,13 +79,16 @@ final class PhabricatorUserPasswordSettingsPanelController } if (!$errors) { - $user->setPassword($pass); // This write is unguarded because the CSRF token has already // been checked in the call to $request->isFormPost() and // the CSRF token depends on the password hash, so when it // is changed here the CSRF token check will fail. $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $user->save(); + + id(new PhabricatorUserEditor()) + ->setActor($user) + ->changePassword($user, $pass); + unset($unguarded); if ($valid_token) { diff --git a/src/applications/people/controller/settings/panels/password/__init__.php b/src/applications/people/controller/settings/panels/password/__init__.php index d87675fd12..b26c580ea6 100644 --- a/src/applications/people/controller/settings/panels/password/__init__.php +++ b/src/applications/people/controller/settings/panels/password/__init__.php @@ -10,6 +10,7 @@ phutil_require_module('phabricator', 'aphront/response/400'); phutil_require_module('phabricator', 'aphront/response/redirect'); phutil_require_module('phabricator', 'aphront/writeguard'); phutil_require_module('phabricator', 'applications/people/controller/settings/panels/base'); +phutil_require_module('phabricator', 'applications/people/editor'); phutil_require_module('phabricator', 'applications/people/storage/email'); phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'view/form/base'); diff --git a/src/applications/people/editor/PhabricatorUserEditor.php b/src/applications/people/editor/PhabricatorUserEditor.php new file mode 100644 index 0000000000..67db0eaa18 --- /dev/null +++ b/src/applications/people/editor/PhabricatorUserEditor.php @@ -0,0 +1,393 @@ +actor = $actor; + return $this; + } + + +/* -( Creating and Editing Users )----------------------------------------- */ + + + /** + * @task edit + */ + public function createNewUser( + PhabricatorUser $user, + PhabricatorUserEmail $email) { + + if ($user->getID()) { + throw new Exception("User has already been created!"); + } + + if ($email->getID()) { + throw new Exception("Email has already been created!"); + } + + // Always set a new user's email address to primary. + $email->setIsPrimary(1); + + $user->openTransaction(); + $user->save(); + + $email->setUserPHID($user->getPHID()); + + try { + $email->save(); + } catch (AphrontQueryDuplicateKeyException $ex) { + $user->killTransaction(); + throw $ex; + } + + $log = PhabricatorUserLog::newLog( + $this->actor, + $user, + PhabricatorUserLog::ACTION_CREATE); + $log->setNewValue($email->getAddress()); + $log->save(); + + $user->saveTransaction(); + + return $this; + } + + + /** + * @task edit + */ + public function updateUser(PhabricatorUser $user) { + if (!$user->getID()) { + throw new Exception("User has not been created yet!"); + } + + $actor = $this->requireActor(); + $user->openTransaction(); + $user->save(); + + $log = PhabricatorUserLog::newLog( + $actor, + $user, + PhabricatorUserLog::ACTION_EDIT); + $log->save(); + + $user->saveTransaction(); + + return $this; + } + + + /** + * @task edit + */ + public function changePassword(PhabricatorUser $user, $password) { + if (!$user->getID()) { + throw new Exception("User has not been created yet!"); + } + + $user->openTransaction(); + $user->reload(); + + $user->setPassword($password); + $user->save(); + + $log = PhabricatorUserLog::newLog( + $this->actor, + $user, + PhabricatorUserLog::ACTION_CHANGE_PASSWORD); + $log->save(); + + $user->saveTransaction(); + } + + +/* -( Editing Roles )------------------------------------------------------ */ + + + /** + * @task role + */ + public function makeAdminUser(PhabricatorUser $user, $admin) { + $actor = $this->requireActor(); + + if (!$user->getID()) { + throw new Exception("User has not been created yet!"); + } + + $user->openTransaction(); + $user->beginWriteLocking(); + + $user->reload(); + if ($user->getIsAdmin() == $admin) { + $user->endWriteLocking(); + $user->killTransaction(); + return $this; + } + + $log = PhabricatorUserLog::newLog( + $actor, + $user, + PhabricatorUserLog::ACTION_ADMIN); + $log->setOldValue($user->getIsAdmin()); + $log->setNewValue($admin); + + $user->setIsAdmin($admin); + $user->save(); + + $log->save(); + + $user->endWriteLocking(); + $user->saveTransaction(); + + return $this; + } + + /** + * @task role + */ + public function disableUser(PhabricatorUser $user, $disable) { + $actor = $this->requireActor(); + + if (!$user->getID()) { + throw new Exception("User has not been created yet!"); + } + + $user->openTransaction(); + $user->beginWriteLocking(); + + $user->reload(); + if ($user->getIsDisabled() == $disable) { + $user->endWriteLocking(); + $user->killTransaction(); + return $this; + } + + $log = PhabricatorUserLog::newLog( + $actor, + $user, + PhabricatorUserLog::ACTION_DISABLE); + $log->setOldValue($user->getIsDisabled()); + $log->setNewValue($disable); + + $user->setIsDisabled($disable); + $user->save(); + + $log->save(); + + $user->endWriteLocking(); + $user->saveTransaction(); + + return $this; + } + + +/* -( Adding, Removing and Changing Email )-------------------------------- */ + + + /** + * @task email + */ + public function addEmail( + PhabricatorUser $user, + PhabricatorUserEmail $email) { + + $actor = $this->requireActor(); + + if (!$user->getID()) { + throw new Exception("User has not been created yet!"); + } + if ($email->getID()) { + throw new Exception("Email has already been created!"); + } + + // Use changePrimaryEmail() to change primary email. + $email->setIsPrimary(0); + $email->setUserPHID($user->getPHID()); + + $user->openTransaction(); + $user->beginWriteLocking(); + + $user->reload(); + + try { + $email->save(); + } catch (AphrontQueryDuplicateKeyException $ex) { + $user->endWriteLocking(); + $user->killTransaction(); + + throw $ex; + } + + $log = PhabricatorUserLog::newLog( + $this->actor, + $user, + PhabricatorUserLog::ACTION_EMAIL_ADD); + $log->setNewValue($email->getAddress()); + $log->save(); + + $user->endWriteLocking(); + $user->saveTransaction(); + + return $this; + } + + + /** + * @task email + */ + public function removeEmail( + PhabricatorUser $user, + PhabricatorUserEmail $email) { + + $actor = $this->requireActor(); + + if (!$user->getID()) { + throw new Exception("User has not been created yet!"); + } + if (!$email->getID()) { + throw new Exception("Email has not been created yet!"); + } + + $user->openTransaction(); + $user->beginWriteLocking(); + + $user->reload(); + $email->reload(); + + if ($email->getIsPrimary()) { + throw new Exception("Can't remove primary email!"); + } + if ($email->getUserPHID() != $user->getPHID()) { + throw new Exception("Email not owned by user!"); + } + + $email->delete(); + + $log = PhabricatorUserLog::newLog( + $this->actor, + $user, + PhabricatorUserLog::ACTION_EMAIL_REMOVE); + $log->setOldValue($email->getAddress()); + $log->save(); + + $user->endWriteLocking(); + $user->saveTransaction(); + + return $this; + } + + + /** + * @task email + */ + public function changePrimaryEmail( + PhabricatorUser $user, + PhabricatorUserEmail $email) { + $actor = $this->requireActor(); + + if (!$user->getID()) { + throw new Exception("User has not been created yet!"); + } + if (!$email->getID()) { + throw new Exception("Email has not been created yet!"); + } + + $user->openTransaction(); + $user->beginWriteLocking(); + + $user->reload(); + $email->reload(); + + if ($email->getUserPHID() != $user->getPHID()) { + throw new Exception("User does not own email!"); + } + + if ($email->getIsPrimary()) { + throw new Exception("Email is already primary!"); + } + + if (!$email->getIsVerified()) { + throw new Exception("Email is not verified!"); + } + + $old_primary = $user->loadPrimaryEmail(); + if ($old_primary) { + $old_primary->setIsPrimary(0); + $old_primary->save(); + } + + $email->setIsPrimary(1); + $email->save(); + + $log = PhabricatorUserLog::newLog( + $actor, + $user, + PhabricatorUserLog::ACTION_EMAIL_PRIMARY); + $log->setOldValue($old_primary ? $old_primary->getAddress() : null); + $log->setNewValue($email->getAddress()); + + $log->save(); + + $user->endWriteLocking(); + $user->saveTransaction(); + + if ($old_primary) { + $old_primary->sendOldPrimaryEmail($user, $email); + } + $email->sendNewPrimaryEmail($user); + + return $this; + } + + +/* -( Internals )---------------------------------------------------------- */ + + + /** + * @task internal + */ + private function requireActor() { + if (!$this->actor) { + throw new Exception("User edit requires actor!"); + } + return $this->actor; + } + +} diff --git a/src/applications/people/editor/__init__.php b/src/applications/people/editor/__init__.php new file mode 100644 index 0000000000..3896d621ca --- /dev/null +++ b/src/applications/people/editor/__init__.php @@ -0,0 +1,12 @@ +remoteAddr) { - $this->remoteAddr = idx($_SERVER, 'REMOTE_ADDR'); + $this->remoteAddr = idx($_SERVER, 'REMOTE_ADDR', ''); } if (!$this->session) { $this->setSession(idx($_COOKIE, 'phsid'));