Add test coverage to the PasswordEngine upgrade workflow and fix a few bugs

Summary:
Ref T13043. When we verify a password and a better hasher is available, we automatically upgrade the stored hash to the stronger hasher.

Add test coverage for this workflow and fix a few bugs and issues, mostly related to shuffling the old hasher name into the transaction.

This doesn't touch anything user-visible yet.

Test Plan: Ran unit tests.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

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

src/__phutil_library_map__.php

@@ -2099,6 +2099,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthPasswordRevoker' => 'applications/auth/revoker/PhabricatorAuthPasswordRevoker.php',
     'PhabricatorAuthPasswordTestCase' => 'applications/auth/__tests__/PhabricatorAuthPasswordTestCase.php',
     'PhabricatorAuthPasswordTransaction' => 'applications/auth/storage/PhabricatorAuthPasswordTransaction.php',
+    'PhabricatorAuthPasswordTransactionQuery' => 'applications/auth/query/PhabricatorAuthPasswordTransactionQuery.php',
     'PhabricatorAuthPasswordTransactionType' => 'applications/auth/xaction/PhabricatorAuthPasswordTransactionType.php',
     'PhabricatorAuthPasswordUpgradeTransaction' => 'applications/auth/xaction/PhabricatorAuthPasswordUpgradeTransaction.php',
     'PhabricatorAuthProvider' => 'applications/auth/provider/PhabricatorAuthProvider.php',
@@ -7393,6 +7394,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthPasswordRevoker' => 'PhabricatorAuthRevoker',
     'PhabricatorAuthPasswordTestCase' => 'PhabricatorTestCase',
     'PhabricatorAuthPasswordTransaction' => 'PhabricatorModularTransaction',
+    'PhabricatorAuthPasswordTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
     'PhabricatorAuthPasswordTransactionType' => 'PhabricatorModularTransactionType',
     'PhabricatorAuthPasswordUpgradeTransaction' => 'PhabricatorAuthPasswordTransactionType',
     'PhabricatorAuthProvider' => 'Phobject',

src/applications/auth/__tests__/PhabricatorAuthPasswordTestCase.php

@@ -99,6 +99,91 @@ final class PhabricatorAuthPasswordTestCase extends PhabricatorTestCase {
     $this->assertTrue($account_engine->isUniquePassword($password2));
   }
 
+  public function testPasswordUpgrade() {
+    $weak_hasher = new PhabricatorIteratedMD5PasswordHasher();
+
+    // Make sure we have two different hashers, and that the second one is
+    // stronger than iterated MD5. The most common reason this would fail is
+    // if an install does not have bcrypt available.
+    $strong_hasher = PhabricatorPasswordHasher::getBestHasher();
+    if ($strong_hasher->getStrength() <= $weak_hasher->getStrength()) {
+      $this->assertSkipped(
+        pht(
+          'Multiple password hashers of different strengths are not '.
+          'available, so hash upgrading can not be tested.'));
+    }
+
+    $envelope = new PhutilOpaqueEnvelope('lunar1997');
+
+    $user = $this->generateNewTestUser();
+    $type = PhabricatorAuthPassword::PASSWORD_TYPE_TEST;
+    $content_source = $this->newContentSource();
+
+    $engine = id(new PhabricatorAuthPasswordEngine())
+      ->setViewer($user)
+      ->setContentSource($content_source)
+      ->setPasswordType($type)
+      ->setObject($user);
+
+    $password = PhabricatorAuthPassword::initializeNewPassword($user, $type)
+      ->setPasswordWithHasher($envelope, $user, $weak_hasher)
+      ->save();
+
+    $weak_name = $weak_hasher->getHashName();
+    $strong_name = $strong_hasher->getHashName();
+
+    // Since we explicitly used the weak hasher, the password should have
+    // been hashed with it.
+    $actual_hasher = $password->getHasher();
+    $this->assertEqual($weak_name, $actual_hasher->getHashName());
+
+    $is_valid = $engine
+      ->setUpgradeHashers(false)
+      ->isValidPassword($envelope, $user);
+    $password->reload();
+
+    // Since we disabled hasher upgrading, the password should not have been
+    // rehashed.
+    $this->assertTrue($is_valid);
+    $actual_hasher = $password->getHasher();
+    $this->assertEqual($weak_name, $actual_hasher->getHashName());
+
+    $is_valid = $engine
+      ->setUpgradeHashers(true)
+      ->isValidPassword($envelope, $user);
+    $password->reload();
+
+    // Now that we enabled hasher upgrading, the password should have been
+    // automatically rehashed into the stronger format.
+    $this->assertTrue($is_valid);
+    $actual_hasher = $password->getHasher();
+    $this->assertEqual($strong_name, $actual_hasher->getHashName());
+
+    // We should also have an "upgrade" transaction in the transaction record
+    // now which records the two hasher names.
+    $xactions = id(new PhabricatorAuthPasswordTransactionQuery())
+      ->setViewer($user)
+      ->withObjectPHIDs(array($password->getPHID()))
+      ->withTransactionTypes(
+        array(
+          PhabricatorAuthPasswordUpgradeTransaction::TRANSACTIONTYPE,
+        ))
+      ->execute();
+
+    $this->assertEqual(1, count($xactions));
+    $xaction = head($xactions);
+
+    $this->assertEqual($weak_name, $xaction->getOldValue());
+    $this->assertEqual($strong_name, $xaction->getNewValue());
+
+    $is_valid = $engine
+      ->isValidPassword($envelope, $user);
+
+    // Finally, the password should still be valid after all the dust has
+    // settled.
+    $this->assertTrue($is_valid);
+  }
+
   private function revokePassword(
     PhabricatorUser $actor,
     PhabricatorAuthPassword $password) {

src/applications/auth/editor/PhabricatorAuthPasswordEditor.php

@@ -3,6 +3,17 @@
 final class PhabricatorAuthPasswordEditor
   extends PhabricatorApplicationTransactionEditor {
 
+  private $oldHasher;
+
+  public function setOldHasher(PhabricatorPasswordHasher $old_hasher) {
+    $this->oldHasher = $old_hasher;
+    return $this;
+  }
+
+  public function getOldHasher() {
+    return $this->oldHasher;
+  }
+
   public function getEditorApplicationClass() {
     return 'PhabricatorAuthApplication';
   }

src/applications/auth/engine/PhabricatorAuthPasswordEngine.php

@@ -224,7 +224,6 @@ final class PhabricatorAuthPasswordEngine
 
       $xactions[] = $password->getApplicationTransactionTemplate()
         ->setTransactionType($upgrade_type)
-        ->setOldValue($old_hasher->getHashName())
         ->setNewValue($new_hasher->getHashName());
 
       $editor = $password->getApplicationTransactionEditor()
@@ -232,6 +231,7 @@ final class PhabricatorAuthPasswordEngine
         ->setContinueOnNoEffect(true)
         ->setContinueOnMissingFields(true)
         ->setContentSource($content_source)
+        ->setOldHasher($old_hasher)
         ->applyTransactions($password, $xactions);
     }
     unset($unguarded);

src/applications/auth/query/PhabricatorAuthPasswordTransactionQuery.php

@@ -0,0 +1,10 @@
+<?php
+
+final class PhabricatorAuthPasswordTransactionQuery
+  extends PhabricatorApplicationTransactionQuery {
+
+  public function getTemplateApplicationTransaction() {
+    return new PhabricatorAuthPasswordTransaction();
+  }
+
+}

src/applications/auth/xaction/PhabricatorAuthPasswordUpgradeTransaction.php

@@ -6,11 +6,17 @@ final class PhabricatorAuthPasswordUpgradeTransaction
   const TRANSACTIONTYPE = 'password.upgrade';
 
   public function generateOldValue($object) {
-    return $this->getStorage()->getOldValue();
+    $old_hasher = $this->getEditor()->getOldHasher();
+
+    if (!$old_hasher) {
+      throw new PhutilInvalidStateException('setOldHasher');
+    }
+
+    return $old_hasher->getHashName();
   }
 
   public function generateNewValue($object, $value) {
-    return (bool)$value;
+    return $value;
   }
 
   public function getTitle() {