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
2024-11-18 21:02:41 +01:00 · 2011-05-30 14:59:17 -07:00 · 2011-05-30 14:59:17 -07:00 · 301fed1b43
commit 301fed1b43
parent 729d2f9c93
6 changed files with 118 additions and 100 deletions
--- 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 = <<<EOBODY
+Condolences on forgetting your password. You can use this link to reset it:
+
+  {$uri}
+
+After you set a new password, consider writing it down on a sticky note and
+attaching it to your monitor so you don't forget again! Choosing a very short,
+easy-to-remember password like "cat" or "1234" might also help.
+
+Best Wishes,
+Phabricator
+
+EOBODY;

          $mail = new PhabricatorMetaMTAMail();
          $mail->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();
--- 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');


--- 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 = <<<EOBODY
+Welcome to Phabricator!
+
+{$admin_username} ({$admin_realname}) has created an account for you.
+
+  Username: {$user_username}
+
+To login to Phabricator, follow this link and set a password:
+
+  {$uri}
+
+After you have set a password, you can login in the future by going here:
+
+  {$base_uri}
+
+Love,
+Phabricator
+
+EOBODY;
+              $mail = id(new PhabricatorMetaMTAMail())
+                ->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(
-        '<p class="aphront-form-instructions">Submitting this form will '.
-        'change this user\'s password. They will no longer be able to login '.
-        'with their old password.</p>')
-      ->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();
--- 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');
--- 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;
--- 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');