From d23cc4b862aac68caf6fc68835884b2d2dae735b Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Thu, 13 Dec 2018 16:13:07 -0800 Subject: [PATCH] Move user renames to modular transactions Summary: Cleaning up more super-old code from `PhabricatorUserEditor`. Also fix user logging in approve transactions. I'm not sure how it worked at all previously. Test Plan: Created new users, renamed them, checked DB for sanity. Entered invalid names, duplicate names, and empty names, got appropriate error messages. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D19887 --- src/__phutil_library_map__.php | 2 + .../PhabricatorPeopleRenameController.php | 56 +++++------ .../people/editor/PhabricatorUserEditor.php | 47 ---------- .../PhabricatorUserUsernameTransaction.php | 92 +++++++++++++++++++ 4 files changed, 116 insertions(+), 81 deletions(-) create mode 100644 src/applications/people/xaction/PhabricatorUserUsernameTransaction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 420457f525..b0408e127d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4646,6 +4646,7 @@ phutil_register_library_map(array( 'PhabricatorUserTransaction' => 'applications/people/storage/PhabricatorUserTransaction.php', 'PhabricatorUserTransactionEditor' => 'applications/people/editor/PhabricatorUserTransactionEditor.php', 'PhabricatorUserTransactionType' => 'applications/people/xaction/PhabricatorUserTransactionType.php', + 'PhabricatorUserUsernameTransaction' => 'applications/people/xaction/PhabricatorUserUsernameTransaction.php', 'PhabricatorUsersEditField' => 'applications/transactions/editfield/PhabricatorUsersEditField.php', 'PhabricatorUsersPolicyRule' => 'applications/people/policyrule/PhabricatorUsersPolicyRule.php', 'PhabricatorUsersSearchField' => 'applications/people/searchfield/PhabricatorUsersSearchField.php', @@ -10706,6 +10707,7 @@ phutil_register_library_map(array( 'PhabricatorUserTransaction' => 'PhabricatorModularTransaction', 'PhabricatorUserTransactionEditor' => 'PhabricatorApplicationTransactionEditor', 'PhabricatorUserTransactionType' => 'PhabricatorModularTransactionType', + 'PhabricatorUserUsernameTransaction' => 'PhabricatorUserTransactionType', 'PhabricatorUsersEditField' => 'PhabricatorTokenizerEditField', 'PhabricatorUsersPolicyRule' => 'PhabricatorPolicyRule', 'PhabricatorUsersSearchField' => 'PhabricatorSearchTokenizerField', diff --git a/src/applications/people/controller/PhabricatorPeopleRenameController.php b/src/applications/people/controller/PhabricatorPeopleRenameController.php index cd552717e0..42ff2e7988 100644 --- a/src/applications/people/controller/PhabricatorPeopleRenameController.php +++ b/src/applications/people/controller/PhabricatorPeopleRenameController.php @@ -22,36 +22,29 @@ final class PhabricatorPeopleRenameController $request, $done_uri); - $errors = array(); - - $v_username = $user->getUsername(); - $e_username = true; + $validation_exception = null; + $username = $user->getUsername(); if ($request->isFormPost()) { - $v_username = $request->getStr('username'); + $username = $request->getStr('username'); + $xactions = array(); - if (!strlen($v_username)) { - $e_username = pht('Required'); - $errors[] = pht('New username is required.'); - } else if ($v_username == $user->getUsername()) { - $e_username = pht('Invalid'); - $errors[] = pht('New username must be different from old username.'); - } else if (!PhabricatorUser::validateUsername($v_username)) { - $e_username = pht('Invalid'); - $errors[] = PhabricatorUser::describeValidUsername(); + $xactions[] = id(new PhabricatorUserTransaction()) + ->setTransactionType( + PhabricatorUserUsernameTransaction::TRANSACTIONTYPE) + ->setNewValue($username); + + $editor = id(new PhabricatorUserTransactionEditor()) + ->setActor($viewer) + ->setContentSourceFromRequest($request) + ->setContinueOnMissingFields(true); + + try { + $editor->applyTransactions($user, $xactions); + return id(new AphrontRedirectResponse())->setURI($done_uri); + } catch (PhabricatorApplicationTransactionValidationException $ex) { + $validation_exception = $ex; } - if (!$errors) { - try { - id(new PhabricatorUserEditor()) - ->setActor($viewer) - ->changeUsername($user, $v_username); - - return id(new AphrontRedirectResponse())->setURI($done_uri); - } catch (AphrontDuplicateKeyQueryException $ex) { - $e_username = pht('Not Unique'); - $errors[] = pht('Another user already has that username.'); - } - } } $inst1 = pht( @@ -87,18 +80,13 @@ final class PhabricatorPeopleRenameController ->appendChild( id(new AphrontFormTextControl()) ->setLabel(pht('New Username')) - ->setValue($v_username) - ->setName('username') - ->setError($e_username)); - - if ($errors) { - $errors = id(new PHUIInfoView())->setErrors($errors); - } + ->setValue($username) + ->setName('username')); return $this->newDialog() ->setWidth(AphrontDialogView::WIDTH_FORM) ->setTitle(pht('Change Username')) - ->appendChild($errors) + ->setValidationException($validation_exception) ->appendParagraph($inst1) ->appendParagraph($inst2) ->appendParagraph($inst3) diff --git a/src/applications/people/editor/PhabricatorUserEditor.php b/src/applications/people/editor/PhabricatorUserEditor.php index a64c1cc2a9..8092824a0c 100644 --- a/src/applications/people/editor/PhabricatorUserEditor.php +++ b/src/applications/people/editor/PhabricatorUserEditor.php @@ -129,53 +129,6 @@ final class PhabricatorUserEditor extends PhabricatorEditor { } - /** - * @task edit - */ - public function changeUsername(PhabricatorUser $user, $username) { - $actor = $this->requireActor(); - - if (!$user->getID()) { - throw new Exception(pht('User has not been created yet!')); - } - - if (!PhabricatorUser::validateUsername($username)) { - $valid = PhabricatorUser::describeValidUsername(); - throw new Exception(pht('Username is invalid! %s', $valid)); - } - - $old_username = $user->getUsername(); - - $user->openTransaction(); - $user->reload(); - $user->setUsername($username); - - try { - $user->save(); - } catch (AphrontDuplicateKeyQueryException $ex) { - $user->setUsername($old_username); - $user->killTransaction(); - throw $ex; - } - - $log = PhabricatorUserLog::initializeNewLog( - $actor, - $user->getPHID(), - PhabricatorUserLog::ACTION_CHANGE_USERNAME); - $log->setOldValue($old_username); - $log->setNewValue($username); - $log->save(); - - $user->saveTransaction(); - - // The SSH key cache currently includes usernames, so dirty it. See T12554 - // for discussion. - PhabricatorAuthSSHKeyQuery::deleteSSHKeyCache(); - - $user->sendUsernameChangeEmail($actor, $old_username); - } - - /* -( Editing Roles )------------------------------------------------------ */ diff --git a/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php b/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php new file mode 100644 index 0000000000..db134a5c78 --- /dev/null +++ b/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php @@ -0,0 +1,92 @@ +getUsername(); + } + + public function generateNewValue($object, $value) { + return $value; + } + + public function applyInternalEffects($object, $value) { + $object->setUsername($value); + } + + public function applyExternalEffects($object, $value) { + $user = $object; + + $this->newUserLog(PhabricatorUserLog::ACTION_CHANGE_USERNAME) + ->setOldValue($this->getOldValue()) + ->setNewValue($value) + ->save(); + + // The SSH key cache currently includes usernames, so dirty it. See T12554 + // for discussion. + PhabricatorAuthSSHKeyQuery::deleteSSHKeyCache(); + + $user->sendUsernameChangeEmail($this->getActor(), $this->getOldValue()); + } + + public function getTitle() { + return pht( + '%s renamed this user from %s to %s.', + $this->renderAuthor(), + $this->renderOldValue(), + $this->renderNewValue()); + } + + public function validateTransactions($object, array $xactions) { + $actor = $this->getActor(); + $errors = array(); + + foreach ($xactions as $xaction) { + $new = $xaction->getNewValue(); + $old = $xaction->getOldValue(); + + if ($old === $new) { + continue; + } + + if (!$actor->getIsAdmin()) { + $errors[] = $this->newInvalidError( + pht('You must be an administrator to rename users.')); + } + + if (!strlen($new)) { + $errors[] = $this->newRequiredError( + pht('New username is required.'), $xaction); + } else if (!PhabricatorUser::validateUsername($new)) { + $errors[] = $this->newInvalidError( + PhabricatorUser::describeValidUsername(), $xaction); + } + + $user = id(new PhabricatorPeopleQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withUsernames(array($new)) + ->executeOne(); + + if ($user) { + $errors[] = $this->newInvalidError( + pht('Another user already has that username.'), $xaction); + } + + } + + return $errors; + } + + public function getRequiredCapabilities( + $object, + PhabricatorApplicationTransaction $xaction) { + + // Unlike normal user edits, renames require admin permissions, which + // is enforced by validateTransactions(). + + return null; + } +}