Skip to content

Commit 0e1bbbd

Browse files
author
epriestley
committed
Allow administrators to change usernames
Summary: Give them a big essay about how it's dangerous, but allow them to do it formally. Because the username is part of the password salt, users must change their passwords after a username change. Make password reset links work for already-logged-in-users since there's no reason not to (if you have a reset link, you can log out and use it) and it's much less confusing if you get this email and are already logged in. Depends on: D2651 Test Plan: Changed a user's username to all kinds of crazy things. Clicked reset links in email. Tried to make invalid/nonsense name changes. Reviewers: btrahan, vrana Reviewed By: btrahan CC: aran Maniphest Tasks: T1303 Differential Revision: https://secure.phabricator.com/D2657
1 parent 0a7b459 commit 0e1bbbd

File tree

5 files changed

+201
-45
lines changed

5 files changed

+201
-45
lines changed

‎src/applications/auth/controller/PhabricatorEmailTokenController.php‎

Lines changed: 5 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -36,22 +36,6 @@ public function processRequest() {
3636
return new Aphront400Response();
3737
}
3838

39-
if ($request->getUser()->getPHID()) {
40-
$view = new AphrontRequestFailureView();
41-
$view->setHeader('Already Logged In');
42-
$view->appendChild(
43-
'<p>You are already logged in.</p>');
44-
$view->appendChild(
45-
'<div class="aphront-failure-continue">'.
46-
'<a class="button" href="/">Return Home</a>'.
47-
'</div>');
48-
return $this->buildStandardPageResponse(
49-
$view,
50-
array(
51-
'title' => 'Already Logged In',
52-
));
53-
}
54-
5539
$token = $this->token;
5640
$email = $request->getStr('email');
5741

@@ -103,10 +87,12 @@ public function processRequest() {
10387
// enough, without requiring users to go through a second round of email
10488
// verification.
10589

106-
$target_email->setIsVerified(1);
107-
$target_email->save();
90+
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
91+
$target_email->setIsVerified(1);
92+
$target_email->save();
93+
$session_key = $target_user->establishSession('web');
94+
unset($unguarded);
10895

109-
$session_key = $target_user->establishSession('web');
11096
$request->setCookie('phusr', $target_user->getUsername());
11197
$request->setCookie('phsid', $session_key);
11298

‎src/applications/people/PhabricatorUserEditor.php‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,49 @@ public function changePassword(PhabricatorUser $user, $password) {
148148
}
149149

150150

151+
/**
152+
* @task edit
153+
*/
154+
public function changeUsername(PhabricatorUser $user, $username) {
155+
$actor = $this->requireActor();
156+
157+
if (!$user->getID()) {
158+
throw new Exception("User has not been created yet!");
159+
}
160+
161+
if (!PhabricatorUser::validateUsername($username)) {
162+
$valid = PhabricatorUser::describeValidUsername();
163+
throw new Exception("Username is invalid! {$valid}");
164+
}
165+
166+
$old_username = $user->getUsername();
167+
168+
$user->openTransaction();
169+
$user->reload();
170+
$user->setUsername($username);
171+
172+
try {
173+
$user->save();
174+
} catch (AphrontQueryDuplicateKeyException $ex) {
175+
$user->setUsername($old_username);
176+
$user->killTransaction();
177+
throw $ex;
178+
}
179+
180+
$log = PhabricatorUserLog::newLog(
181+
$this->actor,
182+
$user,
183+
PhabricatorUserLog::ACTION_CHANGE_USERNAME);
184+
$log->setOldValue($old_username);
185+
$log->setNewValue($username);
186+
$log->save();
187+
188+
$user->saveTransaction();
189+
190+
$user->sendUsernameChangeEmail($actor, $old_username);
191+
}
192+
193+
151194
/* -( Editing Roles )------------------------------------------------------ */
152195

153196

‎src/applications/people/controller/PhabricatorPeopleEditController.php‎

Lines changed: 112 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -41,23 +41,24 @@ public function processRequest() {
4141
if (!$user) {
4242
return new Aphront404Response();
4343
}
44+
$base_uri = '/people/edit/'.$user->getID().'/';
4445
} else {
4546
$user = new PhabricatorUser();
47+
$base_uri = '/people/edit/';
4648
}
4749

48-
$views = array(
49-
'basic' => 'Basic Information',
50-
'role' => 'Edit Roles',
51-
'cert' => 'Conduit Certificate',
52-
);
50+
$nav = new AphrontSideNavFilterView();
51+
$nav->setBaseURI(new PhutilURI($base_uri));
52+
$nav->addFilter('basic', 'Basic Information');
53+
$nav->addFilter('role', 'Edit Roles');
54+
$nav->addFilter('cert', 'Conduit Certificate');
55+
$nav->addSpacer();
56+
$nav->addFilter('rename', 'Change Username');
5357

5458
if (!$user->getID()) {
55-
$view = 'basic';
56-
} else if (isset($views[$this->view])) {
57-
$view = $this->view;
58-
} else {
59-
$view = 'basic';
59+
$this->view = 'basic';
6060
}
61+
$view = $nav->selectFilter($this->view, 'basic');
6162

6263
$content = array();
6364

@@ -79,6 +80,11 @@ public function processRequest() {
7980
case 'cert':
8081
$response = $this->processCertificateRequest($user);
8182
break;
83+
case 'rename':
84+
$response = $this->processRenameRequest($user);
85+
break;
86+
default:
87+
return new Aphront404Response();
8288
}
8389

8490
if ($response instanceof AphrontResponse) {
@@ -88,21 +94,8 @@ public function processRequest() {
8894
$content[] = $response;
8995

9096
if ($user->getID()) {
91-
$side_nav = new AphrontSideNavView();
92-
$side_nav->appendChild($content);
93-
foreach ($views as $key => $name) {
94-
$side_nav->addNavItem(
95-
phutil_render_tag(
96-
'a',
97-
array(
98-
'href' => '/people/edit/'.$user->getID().'/'.$key.'/',
99-
'class' => ($key == $view)
100-
? 'aphront-side-nav-selected'
101-
: null,
102-
),
103-
phutil_escape_html($name)));
104-
}
105-
$content = $side_nav;
97+
$nav->appendChild($content);
98+
$content = $nav;
10699
}
107100

108101
return $this->buildStandardPageResponse(
@@ -444,7 +437,6 @@ private function processCertificateRequest($user) {
444437
$request = $this->getRequest();
445438
$admin = $request->getUser();
446439

447-
448440
$form = new AphrontFormView();
449441
$form
450442
->setUser($admin)
@@ -481,6 +473,100 @@ private function processCertificateRequest($user) {
481473
return array($panel);
482474
}
483475

476+
private function processRenameRequest(PhabricatorUser $user) {
477+
$request = $this->getRequest();
478+
$admin = $request->getUser();
479+
480+
$e_username = true;
481+
$username = $user->getUsername();
482+
483+
$errors = array();
484+
if ($request->isFormPost()) {
485+
486+
$username = $request->getStr('username');
487+
if (!strlen($username)) {
488+
$e_username = 'Required';
489+
$errors[] = 'New username is required.';
490+
} else if ($username == $user->getUsername()) {
491+
$e_username = 'Invalid';
492+
$errors[] = 'New username must be different from old username.';
493+
} else if (!PhabricatorUser::validateUsername($username)) {
494+
$e_username = 'Invalid';
495+
$errors[] = PhabricatorUser::describeValidUsername();
496+
}
497+
498+
if (!$errors) {
499+
try {
500+
501+
id(new PhabricatorUserEditor())
502+
->setActor($admin)
503+
->changeUsername($user, $username);
504+
505+
return id(new AphrontRedirectResponse())
506+
->setURI($request->getRequestURI()->alter('saved', true));
507+
} catch (AphrontQueryDuplicateKeyException $ex) {
508+
$e_username = 'Not Unique';
509+
$errors[] = 'Another user already has that username.';
510+
}
511+
}
512+
}
513+
514+
if ($errors) {
515+
$errors = id(new AphrontErrorView())
516+
->setTitle('Form Errors')
517+
->setErrors($errors);
518+
} else {
519+
$errors = null;
520+
}
521+
522+
$form = new AphrontFormView();
523+
$form
524+
->setUser($admin)
525+
->setAction($request->getRequestURI())
526+
->appendChild(
527+
'<p class="aphront-form-instructions">'.
528+
'<strong>Be careful when renaming users!</strong> '.
529+
'The old username will no longer be tied to the user, so anything '.
530+
'which uses it (like old commit messages) will no longer associate '.
531+
'correctly. And if you give a user a username which some other user '.
532+
'used to have, username lookups will begin returning the wrong '.
533+
'user.'.
534+
'</p>'.
535+
'<p class="aphront-form-instructions">'.
536+
'It is generally safe to rename newly created users (and test users '.
537+
'and so on), but less safe to rename established users and unsafe '.
538+
'to reissue a username.'.
539+
'</p>'.
540+
'<p class="aphront-form-instructions">'.
541+
'Users who rely on password auth will need to reset their password '.
542+
'after their username is changed (their username is part of the '.
543+
'salt in the password hash). They will receive an email with '.
544+
'instructions on how to do this.'.
545+
'</p>')
546+
->appendChild(
547+
id(new AphrontFormStaticControl())
548+
->setLabel('Old Username')
549+
->setValue($user->getUsername()))
550+
->appendChild(
551+
id(new AphrontFormTextControl())
552+
->setLabel('New Username')
553+
->setValue($username)
554+
->setName('username')
555+
->setError($e_username))
556+
->appendChild(
557+
id(new AphrontFormSubmitControl())
558+
->setValue('Change Username'));
559+
560+
$panel = new AphrontPanelView();
561+
$panel->setHeader('Change Username');
562+
$panel->setWidth(AphrontPanelView::WIDTH_FORM);
563+
$panel->appendChild($form);
564+
565+
return array($errors, $panel);
566+
}
567+
568+
569+
484570
private function getRoleInstructions() {
485571
$roles_link = phutil_render_tag(
486572
'a',

‎src/applications/people/storage/PhabricatorUser.php‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -549,6 +549,46 @@ public function sendWelcomeEmail(PhabricatorUser $admin) {
549549
->saveAndSend();
550550
}
551551

552+
public function sendUsernameChangeEmail(
553+
PhabricatorUser $admin,
554+
$old_username) {
555+
556+
$admin_username = $admin->getUserName();
557+
$admin_realname = $admin->getRealName();
558+
$new_username = $this->getUserName();
559+
560+
$password_instructions = null;
561+
if (PhabricatorEnv::getEnvConfig('auth.password-auth-enabled')) {
562+
$uri = $this->getEmailLoginURI();
563+
$password_instructions = <<<EOTXT
564+
If you use a password to login, you'll need to reset it before you can login
565+
again. You can reset your password by following this link:
566+
567+
{$uri}
568+
569+
And, of course, you'll need to use your new username to login from now on. If
570+
you use OAuth to login, nothing should change.
571+
572+
EOTXT;
573+
}
574+
575+
$body = <<<EOBODY
576+
{$admin_username} ({$admin_realname}) has changed your Phabricator username.
577+
578+
Old Username: {$old_username}
579+
New Username: {$new_username}
580+
581+
{$password_instructions}
582+
EOBODY;
583+
584+
$mail = id(new PhabricatorMetaMTAMail())
585+
->addTos(array($this->getPHID()))
586+
->setSubject('[Phabricator] Username Changed')
587+
->setBody($body)
588+
->setFrom($admin->getPHID())
589+
->saveAndSend();
590+
}
591+
552592
public static function describeValidUsername() {
553593
return 'Usernames must contain only numbers, letters, period, underscore '.
554594
'and hyphen, and can not end with a period.';

‎src/applications/people/storage/PhabricatorUserLog.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ final class PhabricatorUserLog extends PhabricatorUserDAO {
3737
const ACTION_EMAIL_ADD = 'email-add';
3838

3939
const ACTION_CHANGE_PASSWORD = 'change-password';
40+
const ACTION_CHANGE_USERNAME = 'change-username';
4041

4142
protected $actorPHID;
4243
protected $userPHID;

0 commit comments

Comments
 (0)