Validate logins, and simplify email password resets

Summary:
  - There are some recent reports of login issues, see T755 and T754. I'm not
really sure what's going on, but this is an attempt at getting some more
information.
  - When we login a user by setting 'phusr' and 'phsid', send them to
/login/validate/ to validate that the cookies actually got set.
  - Do email password resets in two steps: first, log the user in. Redirect them
through validate, then give them the option to reset their password.
  - Don't CSRF logged-out users. It technically sort of works most of the time
right now, but is silly. If we need logged-out CSRF we should generate it in
some more reliable way.

Test Plan:
  - Logged in with username/password.
  - Logged in with OAuth.
  - Logged in with email password reset.
  - Sent bad values to /login/validate/, got appropriate errors.
  - Reset password.
  - Verified next_uri still works.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan, j3kuntz

Maniphest Tasks: T754, T755

Differential Revision: https://secure.phabricator.com/D1353

src/__phutil_library_map__.php

@@ -499,6 +499,7 @@ phutil_register_library_map(array(
     'PhabricatorLocalDiskFileStorageEngine' => 'applications/files/engine/localdisk',
     'PhabricatorLocalTimeTestCase' => 'view/utils/__tests__',
     'PhabricatorLoginController' => 'applications/auth/controller/login',
+    'PhabricatorLoginValidateController' => 'applications/auth/controller/validate',
     'PhabricatorLogoutController' => 'applications/auth/controller/logout',
     'PhabricatorMailImplementationAdapter' => 'applications/metamta/adapter/base',
     'PhabricatorMailImplementationAmazonSESAdapter' => 'applications/metamta/adapter/amazonses',
@@ -640,6 +641,7 @@ phutil_register_library_map(array(
     'PhabricatorRepositorySymbol' => 'applications/repository/storage/symbol',
     'PhabricatorRepositoryTestCase' => 'applications/repository/storage/repository/__tests__',
     'PhabricatorRepositoryType' => 'applications/repository/constants/repositorytype',
+    'PhabricatorResetPasswordController' => 'applications/auth/controller/resetpassword',
     'PhabricatorS3FileStorageEngine' => 'applications/files/engine/s3',
     'PhabricatorSQLPatchList' => 'infrastructure/setup/sql',
     'PhabricatorSearchAbstractDocument' => 'applications/search/index/abstractdocument',
@@ -1165,6 +1167,7 @@ phutil_register_library_map(array(
     'PhabricatorLocalDiskFileStorageEngine' => 'PhabricatorFileStorageEngine',
     'PhabricatorLocalTimeTestCase' => 'PhabricatorTestCase',
     'PhabricatorLoginController' => 'PhabricatorAuthController',
+    'PhabricatorLoginValidateController' => 'PhabricatorAuthController',
     'PhabricatorLogoutController' => 'PhabricatorAuthController',
     'PhabricatorMailImplementationAmazonSESAdapter' => 'PhabricatorMailImplementationPHPMailerLiteAdapter',
     'PhabricatorMailImplementationPHPMailerLiteAdapter' => 'PhabricatorMailImplementationAdapter',
@@ -1287,6 +1290,7 @@ phutil_register_library_map(array(
     'PhabricatorRepositorySvnCommitMessageParserWorker' => 'PhabricatorRepositoryCommitMessageParserWorker',
     'PhabricatorRepositorySymbol' => 'PhabricatorRepositoryDAO',
     'PhabricatorRepositoryTestCase' => 'PhabricatorTestCase',
+    'PhabricatorResetPasswordController' => 'PhabricatorAuthController',
     'PhabricatorS3FileStorageEngine' => 'PhabricatorFileStorageEngine',
     'PhabricatorSearchAttachController' => 'PhabricatorSearchController',
     'PhabricatorSearchBaseController' => 'PhabricatorController',

src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -134,6 +134,8 @@ class AphrontDefaultApplicationConfiguration
         'email/$' => 'PhabricatorEmailLoginController',
         'etoken/(?P<token>\w+)/$' => 'PhabricatorEmailTokenController',
         'refresh/$' => 'PhabricatorRefreshCSRFController',
+        'reset/$' => 'PhabricatorResetPasswordController',
+        'validate/$' => 'PhabricatorLoginValidateController',
       ),
 
       '/logout/$' => 'PhabricatorLogoutController',

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

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -35,6 +35,22 @@ class PhabricatorEmailTokenController extends PhabricatorAuthController {
       return new Aphront400Response();
     }
 
+    if ($request->getUser()->getPHID()) {
+      $view = new AphrontRequestFailureView();
+      $view->setHeader('Already Logged In');
+      $view->appendChild(
+        '<p>You are already logged in.</p>');
+      $view->appendChild(
+        '<div class="aphront-failure-continue">'.
+          '<a class="button" href="/">Return Home</a>'.
+        '</div>');
+      return $this->buildStandardPageResponse(
+        $view,
+        array(
+          'title' => 'Already Logged In',
+        ));
+    }
+
     $token = $this->token;
     $email = $request->getStr('email');
 
@@ -61,81 +77,18 @@ class PhabricatorEmailTokenController extends PhabricatorAuthController {
         ));
     }
 
-    if ($request->getUser()->getPHID() != $target_user->getPHID()) {
-      $session_key = $target_user->establishSession('web');
-      $request->setCookie('phusr', $target_user->getUsername());
-      $request->setCookie('phsid', $session_key);
-    }
+    $session_key = $target_user->establishSession('web');
+    $request->setCookie('phusr', $target_user->getUsername());
+    $request->setCookie('phsid', $session_key);
 
-    $errors = array();
-
-    $e_pass = true;
-    $e_confirm = true;
-
-    if ($request->isFormPost()) {
-      $e_pass = 'Error';
-      $e_confirm = 'Error';
-
-      $pass = $request->getStr('password');
-      $confirm = $request->getStr('confirm');
-
-      if (strlen($pass) < 3) {
-        $errors[] = 'That password is ridiculously short.';
-      }
-
-      if ($pass !== $confirm) {
-        $errors[] = "Passwords do not match.";
-      }
-
-      if (!$errors) {
-        $target_user->setPassword($pass);
-        $target_user->save();
-        return id(new AphrontRedirectResponse())
-          ->setURI('/');
-      }
-    }
-
-    if ($errors) {
-      $error_view = new AphrontErrorView();
-      $error_view->setTitle('Password Reset Failed');
-      $error_view->setErrors($errors);
-    } else {
-      $error_view = null;
-    }
-
-    $form = new AphrontFormView();
-    $form
-      ->setUser($target_user)
-      ->setAction('/login/etoken/'.$token.'/')
-      ->addHiddenInput('email', $email)
-      ->appendChild(
-        id(new AphrontFormPasswordControl())
-          ->setLabel('New Password')
-          ->setName('password')
-          ->setError($e_pass))
-      ->appendChild(
-        id(new AphrontFormPasswordControl())
-          ->setLabel('Confirm Password')
-          ->setName('confirm')
-          ->setError($e_confirm))
-      ->appendChild(
-        id(new AphrontFormSubmitControl())
-          ->setValue('Reset Password')
-          ->addCancelButton('/', 'Skip'));
-
-    $panel = new AphrontPanelView();
-    $panel->setWidth(AphrontPanelView::WIDTH_FORM);
-    $panel->setHeader('Reset Password');
-    $panel->appendChild($form);
-
-    return $this->buildStandardPageResponse(
+    $uri = new PhutilURI('/login/validate/');
+    $uri->setQueryParams(
       array(
-        $error_view,
-        $panel,
-      ),
-      array(
-        'title' => 'Create New Account',
+        'phusr' => $target_user->getUsername(),
+        'next'  => '/login/reset/',
       ));
-  }
 
+    return id(new AphrontRedirectResponse())
+      ->setURI((string)$uri);
+  }
 }

src/applications/auth/controller/emailtoken/__init__.php

@@ -11,13 +11,9 @@ phutil_require_module('phabricator', 'aphront/response/redirect');
 phutil_require_module('phabricator', 'applications/auth/controller/base');
 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/password');
-phutil_require_module('phabricator', 'view/form/control/submit');
-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', 'parser/uri');
 phutil_require_module('phutil', 'utils');
 
 

src/applications/auth/controller/login/PhabricatorLoginController.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -31,16 +31,13 @@ class PhabricatorLoginController extends PhabricatorAuthController {
     }
 
     $next_uri = $this->getRequest()->getPath();
-    $request->setCookie('next_uri', $next_uri);
-    if ($next_uri == '/login/' && !$request->isFormPost()) {
-      // The user went straight to /login/, so presumably they want to go
-      // to the dashboard upon logging in. Because, you know, that's logical.
-      // And people are logical. Sometimes... Fine, no they're not.
-      // We check for POST here because getPath() would get reset to /login/.
-       $request->setCookie('next_uri', '/');
+    if ($next_uri == '/login/') {
+      $next_uri = '/';
     }
 
-    // Always use $request->getCookie('next_uri', '/') after the above.
+    if (!$request->isFormPost()) {
+      $request->setCookie('next_uri', $next_uri);
+    }
 
     $password_auth = PhabricatorEnv::getEnvConfig('auth.password-auth-enabled');
 
@@ -72,8 +69,14 @@ class PhabricatorLoginController extends PhabricatorAuthController {
             $request->setCookie('phusr', $user->getUsername());
             $request->setCookie('phsid', $session_key);
 
+            $uri = new PhutilURI('/login/validate/');
+            $uri->setQueryParams(
+              array(
+                'phusr' => $user->getUsername(),
+              ));
+
             return id(new AphrontRedirectResponse())
-              ->setURI($request->getCookie('next_uri', '/'));
+              ->setURI((string)$uri);
           } else {
             $log = PhabricatorUserLog::newLog(
               null,

src/applications/auth/controller/login/__init__.php

@@ -19,6 +19,7 @@ phutil_require_module('phabricator', 'view/form/control/text');
 phutil_require_module('phabricator', 'view/form/error');
 phutil_require_module('phabricator', 'view/layout/panel');
 
+phutil_require_module('phutil', 'parser/uri');
 phutil_require_module('phutil', 'utils');
 
 

src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -134,8 +134,6 @@ class PhabricatorOAuthLoginController extends PhabricatorAuthController {
         ->setURI('/settings/page/'.$provider_key.'/');
     }
 
-    $next_uri = $request->getCookie('next_uri', '/');
-
     // Login with known auth.
 
     if ($oauth_info->getID()) {
@@ -154,9 +152,14 @@ class PhabricatorOAuthLoginController extends PhabricatorAuthController {
 
       $request->setCookie('phusr', $known_user->getUsername());
       $request->setCookie('phsid', $session_key);
-      $request->clearCookie('next_uri');
-      return id(new AphrontRedirectResponse())
-        ->setURI($next_uri);
+
+      $uri = new PhutilURI('/login/validate/');
+      $uri->setQueryParams(
+        array(
+          'phusr' => $known_user->getUsername(),
+        ));
+
+      return id(new AphrontRedirectResponse())->setURI((string)$uri);
     }
 
     $oauth_email = $provider->retrieveUserEmail();

src/applications/auth/controller/resetpassword/PhabricatorResetPasswordController.php

@@ -0,0 +1,108 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+class PhabricatorResetPasswordController extends PhabricatorAuthController {
+
+  public function processRequest() {
+    $request = $this->getRequest();
+    $user = $request->getUser();
+
+    if (!PhabricatorEnv::getEnvConfig('auth.password-auth-enabled')) {
+      return new Aphront400Response();
+    }
+
+    $errors = array();
+
+    $e_pass = true;
+    $e_confirm = true;
+
+    if ($request->isFormPost()) {
+      $e_pass = 'Error';
+      $e_confirm = 'Error';
+
+      $pass = $request->getStr('password');
+      $confirm = $request->getStr('confirm');
+
+      if (strlen($pass) < 3) {
+        $errors[] = 'That password is ridiculously short.';
+      }
+
+      if ($pass !== $confirm) {
+        $errors[] = "Passwords do not match.";
+      }
+
+      if (!$errors) {
+
+        // The CSRF token depends on the user's password hash. When we change
+        // it, we cause the CSRF check to fail. We don't need to do a CSRF
+        // check here because we've already performed one in the isFormPost()
+        // call earlier.
+
+        $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+          $user->setPassword($pass);
+          $user->save();
+        unset($unguarded);
+
+        return id(new AphrontRedirectResponse())
+          ->setURI('/');
+      }
+    }
+
+    if ($errors) {
+      $error_view = new AphrontErrorView();
+      $error_view->setTitle('Password Reset Failed');
+      $error_view->setErrors($errors);
+    } else {
+      $error_view = null;
+    }
+
+    $form = new AphrontFormView();
+    $form
+      ->setUser($user)
+      ->setAction('/login/reset/')
+      ->appendChild(
+        id(new AphrontFormPasswordControl())
+          ->setLabel('New Password')
+          ->setName('password')
+          ->setError($e_pass))
+      ->appendChild(
+        id(new AphrontFormPasswordControl())
+          ->setLabel('Confirm Password')
+          ->setName('confirm')
+          ->setError($e_confirm))
+      ->appendChild(
+        id(new AphrontFormSubmitControl())
+          ->setValue('Reset Password')
+          ->addCancelButton('/', 'Skip'));
+
+    $panel = new AphrontPanelView();
+    $panel->setWidth(AphrontPanelView::WIDTH_FORM);
+    $panel->setHeader('Reset Password');
+    $panel->appendChild($form);
+
+    return $this->buildStandardPageResponse(
+      array(
+        $error_view,
+        $panel,
+      ),
+      array(
+        'title' => 'Reset Password',
+      ));
+  }
+
+}

src/applications/auth/controller/resetpassword/__init__.php

@@ -0,0 +1,23 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+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/auth/controller/base');
+phutil_require_module('phabricator', 'infrastructure/env');
+phutil_require_module('phabricator', 'view/form/base');
+phutil_require_module('phabricator', 'view/form/control/password');
+phutil_require_module('phabricator', 'view/form/control/submit');
+phutil_require_module('phabricator', 'view/form/error');
+phutil_require_module('phabricator', 'view/layout/panel');
+
+phutil_require_module('phutil', 'utils');
+
+
+phutil_require_source('PhabricatorResetPasswordController.php');

src/applications/auth/controller/validate/PhabricatorLoginValidateController.php

@@ -0,0 +1,95 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+class PhabricatorLoginValidateController extends PhabricatorAuthController {
+
+  public function shouldRequireLogin() {
+    return false;
+  }
+
+  public function processRequest() {
+    $request = $this->getRequest();
+
+    $failures = array();
+
+    if (!$request->getStr('phusr')) {
+      throw new Exception(
+        "Login validation is missing expected parameters!");
+    }
+
+    $expect_phusr = $request->getStr('phusr');
+    $actual_phusr = $request->getCookie('phusr');
+    if ($actual_phusr != $expect_phusr) {
+
+      if ($actual_phusr) {
+        $cookie_info = "sent back a cookie with the value '{$actual_phusr}'.";
+      } else {
+        $cookie_info = "did not accept the cookie.";
+      }
+
+      $failures[] =
+        "Attempted to set 'phusr' cookie to '{$expect_phusr}', but your ".
+        "browser {$cookie_info}";
+    }
+
+    if (!$failures) {
+      if (!$request->getUser()->getPHID()) {
+        $failures[] = "Cookies were set correctly, but your session ".
+                      "isn't valid.";
+      }
+    }
+
+    if ($failures) {
+
+      $list = array();
+      foreach ($failures as $failure) {
+        $list[] = '<li>'.phutil_escape_html($failure).'</li>';
+      }
+      $list = '<ul>'.implode("\n", $list).'</ul>';
+
+      $view = new AphrontRequestFailureView();
+      $view->setHeader('Login Failed');
+      $view->appendChild(
+        '<p>Login failed:</p>'.
+        $list.
+        '<p><strong>Clear your cookies</strong> and try again.</p>');
+      $view->appendChild(
+        '<div class="aphront-failure-continue">'.
+          '<a class="button" href="/login/">Try Again</a>'.
+        '</div>');
+      return $this->buildStandardPageResponse(
+        $view,
+        array(
+          'title' => 'Login Failed',
+        ));
+    }
+
+    $next = nonempty(
+      $request->getStr('next'),
+      $request->getCookie('next_uri'),
+      '/');
+     $request->clearCookie('next_uri');
+
+    if (strpos($next, '/') !== 0) {
+      $next = '/';
+    }
+
+    return id(new AphrontRedirectResponse())->setURI($next);
+  }
+
+}

src/applications/auth/controller/validate/__init__.php

@@ -0,0 +1,17 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'aphront/response/redirect');
+phutil_require_module('phabricator', 'applications/auth/controller/base');
+phutil_require_module('phabricator', 'view/page/failure');
+
+phutil_require_module('phutil', 'markup');
+phutil_require_module('phutil', 'utils');
+
+
+phutil_require_source('PhabricatorLoginValidateController.php');

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

@@ -152,6 +152,10 @@ class PhabricatorUser extends PhabricatorUserDAO {
 
   public function validateCSRFToken($token) {
 
+    if (!$this->getPHID()) {
+      return true;
+    }
+
     // When the user posts a form, we check that it contains a valid CSRF token.
     // Tokens cycle each hour (every CSRF_CYLCE_FREQUENCY seconds) and we accept
     // either the current token, the next token (users can submit a "future"
@@ -185,6 +189,7 @@ class PhabricatorUser extends PhabricatorUserDAO {
         return true;
       }
     }
+
     return false;
   }