From 301fed1b43a4cf76ee07e6a954e72c2938c3c5c2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 30 May 2011 14:59:17 -0700 Subject: [PATCH] Revise administrative workflow for user creation Summary: - When an administrator creates a user, provide an option to send a welcome email. Right now this workflow kind of dead-ends. - Prevent administrators from changing the "System Agent" flag. If they can change it, they can grab another user's certificate and then act as them. This is a vaguely weaker security policy than is exhibited elsewhere in the application. Instead, make user accounts immutably normal users or system agents at creation time. - Prevent administrators from changing email addresses after account creation. Same deal as conduit certs. The 'bin/accountadmin' script can still do this if a user has a real problem. - Prevent administrators from resetting passwords. There's no need for this anymore with welcome emails plus email login and it raises the same issues. Test Plan: - Created a new account, selected "send welcome email", got a welcome email, logged in with the link inside it. - Created a new system agent. - Reset an account's password. Reviewed By: aran Reviewers: tuomaspelkonen, jungejason, aran CC: anjali, aran, epriestley Differential Revision: 379 --- .../email/PhabricatorEmailLoginController.php | 19 +- .../auth/controller/email/__init__.php | 1 - .../edit/PhabricatorPeopleEditController.php | 185 +++++++++--------- .../people/controller/edit/__init__.php | 3 + .../people/storage/user/PhabricatorUser.php | 9 +- .../people/storage/user/__init__.php | 1 + 6 files changed, 118 insertions(+), 100 deletions(-) diff --git a/src/applications/auth/controller/email/PhabricatorEmailLoginController.php b/src/applications/auth/controller/email/PhabricatorEmailLoginController.php index 2fe6d8aa38..4c32a5a4f7 100644 --- a/src/applications/auth/controller/email/PhabricatorEmailLoginController.php +++ b/src/applications/auth/controller/email/PhabricatorEmailLoginController.php @@ -64,7 +64,20 @@ class PhabricatorEmailLoginController extends PhabricatorAuthController { } if (!$errors) { - $etoken = $target_user->generateEmailToken(); + $uri = $target_user->getEmailLoginURI(); + $body = <<setSubject('[Phabricator] Password Reset'); @@ -73,9 +86,7 @@ class PhabricatorEmailLoginController extends PhabricatorAuthController { array( $target_user->getPHID(), )); - $mail->setBody( - "here is your link ". - PhabricatorEnv::getURI('/login/etoken/'.$etoken.'/').'?email='.phutil_escape_uri($target_user->getEmail())); + $mail->setBody($body); $mail->saveAndSend(); $view = new AphrontRequestFailureView(); diff --git a/src/applications/auth/controller/email/__init__.php b/src/applications/auth/controller/email/__init__.php index fbec05c968..1022165205 100644 --- a/src/applications/auth/controller/email/__init__.php +++ b/src/applications/auth/controller/email/__init__.php @@ -19,7 +19,6 @@ phutil_require_module('phabricator', 'view/form/error'); phutil_require_module('phabricator', 'view/layout/panel'); phutil_require_module('phabricator', 'view/page/failure'); -phutil_require_module('phutil', 'markup'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/people/controller/edit/PhabricatorPeopleEditController.php b/src/applications/people/controller/edit/PhabricatorPeopleEditController.php index f2b2b68f01..a98563d129 100644 --- a/src/applications/people/controller/edit/PhabricatorPeopleEditController.php +++ b/src/applications/people/controller/edit/PhabricatorPeopleEditController.php @@ -46,7 +46,6 @@ class PhabricatorPeopleEditController extends PhabricatorPeopleController { $views = array( 'basic' => 'Basic Information', - 'password' => 'Reset Password', 'role' => 'Edit Role', 'cert' => 'Conduit Certificate', ); @@ -73,9 +72,6 @@ class PhabricatorPeopleEditController extends PhabricatorPeopleController { case 'basic': $response = $this->processBasicRequest($user); break; - case 'password': - $response = $this->processPasswordRequest($user); - break; case 'role': $response = $this->processRoleRequest($user); break; @@ -124,13 +120,21 @@ class PhabricatorPeopleEditController extends PhabricatorPeopleController { $e_email = true; $errors = array(); + $welcome_checked = true; + $request = $this->getRequest(); if ($request->isFormPost()) { + $welcome_checked = $request->getInt('welcome'); + if (!$user->getID()) { $user->setUsername($request->getStr('username')); + $user->setEmail($request->getStr('email')); + + if ($request->getStr('role') == 'agent') { + $user->setIsSystemAgent(true); + } } $user->setRealName($request->getStr('realname')); - $user->setEmail($request->getStr('email')); if (!strlen($user->getUsername())) { $errors[] = "Username is required."; @@ -158,13 +162,52 @@ class PhabricatorPeopleEditController extends PhabricatorPeopleController { if (!$errors) { try { + $is_new = !$user->getID(); + $user->save(); - $log = PhabricatorUserLog::newLog( - $admin, - $user, - PhabricatorUserLog::ACTION_CREATE); - $log->save(); + if ($is_new) { + $log = PhabricatorUserLog::newLog( + $admin, + $user, + PhabricatorUserLog::ACTION_CREATE); + $log->save(); + + if ($welcome_checked) { + $admin_username = $admin->getUserName(); + $admin_realname = $admin->getRealName(); + $user_username = $user->getUserName(); + + $base_uri = PhabricatorEnv::getProductionURI('/'); + + $uri = $user->getEmailLoginURI(); + $body = <<addTos(array($user->getPHID())) + ->setSubject('[Phabricator] Welcome to Phabricator') + ->setBody($body) + ->setFrom($admin->getPHID()) + ->saveAndSend(); + } + } $response = id(new AphrontRedirectResponse()) ->setURI('/people/edit/'.$user->getID().'/?saved=true'); @@ -228,12 +271,46 @@ class PhabricatorPeopleEditController extends PhabricatorPeopleController { id(new AphrontFormTextControl()) ->setLabel('Email') ->setName('email') + ->setDisabled($is_immutable) ->setValue($user->getEmail()) - ->setError($e_email)) + ->setError($e_email)); + + if (!$user->getID()) { + $form + ->appendChild( + id(new AphrontFormSelectControl()) + ->setLabel('Role') + ->setName('role') + ->setValue('user') + ->setOptions( + array( + 'user' => 'Normal User', + 'agent' => 'System Agent', + )) + ->setCaption( + 'You can create a "system agent" account for bots, scripts, '. + 'etc.')) + ->appendChild( + id(new AphrontFormCheckboxControl()) + ->addCheckbox( + 'welcome', + 1, + 'Send "Welcome to Phabricator" email.', + $welcome_checked)); + } else { + $form->appendChild( + id(new AphrontFormStaticControl()) + ->setLabel('Role') + ->setValue( + $user->getIsSystemAgent() + ? 'System Agent' + : 'Normal User')); + } + + $form ->appendChild( id(new AphrontFormSubmitControl()) - ->setValue('Save') - ->addCancelButton('/people/')); + ->setValue('Save')); $panel = new AphrontPanelView(); if ($user->getID()) { @@ -248,69 +325,6 @@ class PhabricatorPeopleEditController extends PhabricatorPeopleController { return array($error_view, $panel); } - private function processPasswordRequest(PhabricatorUser $user) { - $request = $this->getRequest(); - $admin = $request->getUser(); - - $e_password = true; - $errors = array(); - - if ($request->isFormPost()) { - if (strlen($request->getStr('password'))) { - $user->setPassword($request->getStr('password')); - $e_password = null; - } else { - $errors[] = 'Password is required.'; - $e_password = 'Required'; - } - - if (!$errors) { - $user->save(); - - $log = PhabricatorUserLog::newLog( - $admin, - $user, - PhabricatorUserLog::ACTION_RESET_PASSWORD); - $log->save(); - - return id(new AphrontRedirectResponse()) - ->setURI($request->getRequestURI()->alter('saved', 'true')); - } - } - - $error_view = null; - if ($errors) { - $error_view = id(new AphrontErrorView()) - ->setTitle('Form Errors') - ->setErrors($errors); - } - - - $form = id(new AphrontFormView()) - ->setUser($admin) - ->setAction($request->getRequestURI()->alter('saved', null)) - ->appendChild( - '

Submitting this form will '. - 'change this user\'s password. They will no longer be able to login '. - 'with their old password.

') - ->appendChild( - id(new AphrontFormTextControl()) - ->setLabel('New Password') - ->setName('password') - ->setError($e_password)) - ->appendChild( - id(new AphrontFormSubmitControl()) - ->setValue('Reset Password')); - - - $panel = new AphrontPanelView(); - $panel->setHeader('Reset Password'); - $panel->setWidth(AphrontPanelView::WIDTH_FORM); - $panel->appendChild($form); - - return array($error_view, $panel); - } - private function processRoleRequest(PhabricatorUser $user) { $request = $this->getRequest(); $admin = $request->getUser(); @@ -352,13 +366,6 @@ class PhabricatorPeopleEditController extends PhabricatorPeopleController { $user->setIsDisabled($new_disabled); $logs[] = $log; } - - $new_agent = (bool)$request->getBool('is_agent'); - $old_agent = (bool)$user->getIsSystemAgent(); - if ($new_agent != $old_agent) { - // TODO: Get rid of this, move it to the create flow. - $user->setIsSystemAgent($new_agent); - } } if (!$errors) { @@ -405,14 +412,6 @@ class PhabricatorPeopleEditController extends PhabricatorPeopleController { 1, 'Disabled: can not login.', $user->getIsDisabled()) - ->setDisabled($is_self)) - ->appendChild( - id(new AphrontFormCheckboxControl()) - ->addCheckbox( - 'is_agent', - 1, - 'Agent: system agent (robot).', - $user->getIsSystemAgent()) ->setDisabled($is_self)); if (!$is_self) { @@ -459,9 +458,7 @@ class PhabricatorPeopleEditController extends PhabricatorPeopleController { id(new AphrontFormStaticControl()) ->setLabel('Certificate') ->setValue( - 'You may only view the certificates for System Agents. Mark '. - 'this account as a "system agent" in the "Edit Role" tab to '. - 'view the corresponding certificate.')); + 'You may only view the certificates of System Agents.')); } $panel = new AphrontPanelView(); diff --git a/src/applications/people/controller/edit/__init__.php b/src/applications/people/controller/edit/__init__.php index 5c631230d4..b64ccccd5c 100644 --- a/src/applications/people/controller/edit/__init__.php +++ b/src/applications/people/controller/edit/__init__.php @@ -8,11 +8,14 @@ phutil_require_module('phabricator', 'aphront/response/404'); phutil_require_module('phabricator', 'aphront/response/redirect'); +phutil_require_module('phabricator', 'applications/metamta/storage/mail'); phutil_require_module('phabricator', 'applications/people/controller/base'); phutil_require_module('phabricator', 'applications/people/storage/log'); phutil_require_module('phabricator', 'applications/people/storage/user'); +phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'view/form/base'); phutil_require_module('phabricator', 'view/form/control/checkbox'); +phutil_require_module('phabricator', 'view/form/control/select'); phutil_require_module('phabricator', 'view/form/control/static'); phutil_require_module('phabricator', 'view/form/control/submit'); phutil_require_module('phabricator', 'view/form/control/text'); diff --git a/src/applications/people/storage/user/PhabricatorUser.php b/src/applications/people/storage/user/PhabricatorUser.php index a1f9ef916b..ab88cdc35e 100644 --- a/src/applications/people/storage/user/PhabricatorUser.php +++ b/src/applications/people/storage/user/PhabricatorUser.php @@ -248,7 +248,7 @@ class PhabricatorUser extends PhabricatorUserDAO { return $session_key; } - public function generateEmailToken($offset = 0) { + private function generateEmailToken($offset = 0) { return $this->generateToken( time() + ($offset * self::EMAIL_CYCLE_FREQUENCY), self::EMAIL_CYCLE_FREQUENCY, @@ -266,6 +266,13 @@ class PhabricatorUser extends PhabricatorUserDAO { return false; } + public function getEmailLoginURI() { + $token = $this->generateEmailToken(); + $uri = PhabricatorEnv::getProductionURI('/login/etoken/'.$token.'/'); + $uri = new PhutilURI($uri); + return $uri->alter('email', $this->getEmail()); + } + public function loadPreferences() { if ($this->preferences) { return $this->preferences; diff --git a/src/applications/people/storage/user/__init__.php b/src/applications/people/storage/user/__init__.php index e3a2672545..c75b225dbb 100644 --- a/src/applications/people/storage/user/__init__.php +++ b/src/applications/people/storage/user/__init__.php @@ -15,6 +15,7 @@ phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'storage/queryfx'); phutil_require_module('phutil', 'filesystem'); +phutil_require_module('phutil', 'parser/uri'); phutil_require_module('phutil', 'utils');