From dd8f588ac51bb178fb24aeb8f09ee1c0cd097d68 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 20 Jan 2018 19:22:17 -0800 Subject: [PATCH] Migrate VCS passwords to new shared password infrastructure Summary: Ref T13043. Migrate VCS passwords away from their dedicated table to new the new shared infrastructure. Future changes will migrate account passwords and remove the old table. Test Plan: - Ran migrations. - Cloned with the same password that was configured before the migrations (worked). - Cloned with a different, invalid password (failed). - Changed password. - Cloned with old password (failed). - Cloned with new password (worked). - Deleted password in web UI. - Cloned with old password (failed). - Set password to the same password as it currently is set to (worked, no "unique" collision). - Set password to account password. !!This (incorrectly) works for now until account passwords migrate, since the uniqueness check can't see them yet.!! - Set password to a new unique password. - Cloned (worked). - Revoked the password with `bin/auth revoke`. - Verified web UI shows "no password set". - Verified that pull no longer works. - Verified that I can no longer select the revoked password. - Verified that accounts do not interact: - Tried to set account B to account A's password (worked). - Tried to set account B to a password revoked on account A (worked). - Spot checked the `password` and `passwordtransaction` tables for saniity. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13043 Differential Revision: https://secure.phabricator.com/D18898 --- .../autopatches/20180120.auth.03.vcsdata.sql | 5 ++ .../autopatches/20180120.auth.04.vcsphid.php | 22 ++++++++ .../controller/DiffusionServeController.php | 31 ++++-------- .../DiffusionSetPasswordSettingsPanel.php | 50 ++++++++++++------- 4 files changed, 70 insertions(+), 38 deletions(-) create mode 100644 resources/sql/autopatches/20180120.auth.03.vcsdata.sql create mode 100644 resources/sql/autopatches/20180120.auth.04.vcsphid.php diff --git a/resources/sql/autopatches/20180120.auth.03.vcsdata.sql b/resources/sql/autopatches/20180120.auth.03.vcsdata.sql new file mode 100644 index 0000000000..3d1392bd5c --- /dev/null +++ b/resources/sql/autopatches/20180120.auth.03.vcsdata.sql @@ -0,0 +1,5 @@ +INSERT INTO {$NAMESPACE}_auth.auth_password + (objectPHID, phid, passwordType, passwordHash, isRevoked, + dateCreated, dateModified) + SELECT userPHID, '', 'vcs', passwordHash, 0, dateCreated, dateModified + FROM {$NAMESPACE}_repository.repository_vcspassword; diff --git a/resources/sql/autopatches/20180120.auth.04.vcsphid.php b/resources/sql/autopatches/20180120.auth.04.vcsphid.php new file mode 100644 index 0000000000..40b61a54b9 --- /dev/null +++ b/resources/sql/autopatches/20180120.auth.04.vcsphid.php @@ -0,0 +1,22 @@ +establishConnection('w'); + +foreach (new LiskMigrationIterator($table) as $row) { + if ($row->getPHID()) { + continue; + } + + $new_phid = $row->generatePHID(); + + queryfx( + $conn, + 'UPDATE %T SET phid = %s WHERE id = %d', + $table->getTableName(), + $new_phid, + $row->getID()); +} diff --git a/src/applications/diffusion/controller/DiffusionServeController.php b/src/applications/diffusion/controller/DiffusionServeController.php index 475f688a4c..0496b93bb6 100644 --- a/src/applications/diffusion/controller/DiffusionServeController.php +++ b/src/applications/diffusion/controller/DiffusionServeController.php @@ -715,30 +715,19 @@ final class DiffusionServeController extends DiffusionController { return null; } - $password_entry = id(new PhabricatorRepositoryVCSPassword()) - ->loadOneWhere('userPHID = %s', $user->getPHID()); - if (!$password_entry) { - // User doesn't have a password set. + $request = $this->getRequest(); + $content_source = PhabricatorContentSource::newFromRequest($request); + + $engine = id(new PhabricatorAuthPasswordEngine()) + ->setViewer($user) + ->setContentSource($content_source) + ->setPasswordType(PhabricatorAuthPassword::PASSWORD_TYPE_VCS) + ->setObject($user); + + if (!$engine->isValidPassword($password)) { return null; } - if (!$password_entry->comparePassword($password, $user)) { - // Password doesn't match. - return null; - } - - // If the user's password is stored using a less-than-optimal hash, upgrade - // them to the strongest available hash. - - $hash_envelope = new PhutilOpaqueEnvelope( - $password_entry->getPasswordHash()); - if (PhabricatorPasswordHasher::canUpgradeHash($hash_envelope)) { - $password_entry->setPassword($password, $user); - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $password_entry->save(); - unset($unguarded); - } - return $user; } diff --git a/src/applications/diffusion/panel/DiffusionSetPasswordSettingsPanel.php b/src/applications/diffusion/panel/DiffusionSetPasswordSettingsPanel.php index ccfa0df4a4..2da51e0624 100644 --- a/src/applications/diffusion/panel/DiffusionSetPasswordSettingsPanel.php +++ b/src/applications/diffusion/panel/DiffusionSetPasswordSettingsPanel.php @@ -35,13 +35,20 @@ final class DiffusionSetPasswordSettingsPanel extends PhabricatorSettingsPanel { $request, '/settings/'); - $vcspassword = id(new PhabricatorRepositoryVCSPassword()) - ->loadOneWhere( - 'userPHID = %s', - $user->getPHID()); - if (!$vcspassword) { - $vcspassword = id(new PhabricatorRepositoryVCSPassword()); - $vcspassword->setUserPHID($user->getPHID()); + $vcs_type = PhabricatorAuthPassword::PASSWORD_TYPE_VCS; + + $vcspasswords = id(new PhabricatorAuthPasswordQuery()) + ->setViewer($viewer) + ->withObjectPHIDs(array($user->getPHID())) + ->withPasswordTypes(array($vcs_type)) + ->withIsRevoked(false) + ->execute(); + if ($vcspasswords) { + $vcspassword = head($vcspasswords); + } else { + $vcspassword = PhabricatorAuthPassword::initializeNewPassword( + $user, + $vcs_type); } $panel_uri = $this->getPanelURI('?saved=true'); @@ -77,23 +84,32 @@ final class DiffusionSetPasswordSettingsPanel extends PhabricatorSettingsPanel { if (!$errors) { $envelope = new PhutilOpaqueEnvelope($new_password); + $content_source = PhabricatorContentSource::newFromRequest($request); - try { - // NOTE: This test is against $viewer (not $user), so that the error - // message below makes sense in the case that the two are different, - // and because an admin reusing their own password is bad, while - // system agents generally do not have passwords anyway. + // NOTE: This test is against $viewer (not $user), so that the error + // message below makes sense in the case that the two are different, + // and because an admin reusing their own password is bad, while + // system agents generally do not have passwords anyway. - $same_password = $viewer->comparePassword($envelope); - } catch (PhabricatorPasswordHasherUnavailableException $ex) { - // If we're missing the hasher, just let the user continue. - $same_password = false; - } + $engine = id(new PhabricatorAuthPasswordEngine()) + ->setViewer($viewer) + ->setContentSource($content_source) + ->setObject($viewer) + ->setPasswordType($vcs_type); + + $same_password = !$engine->isUniquePassword($envelope); + $revoked_password = $engine->isRevokedPassword($envelope); if ($new_password !== $confirm) { $e_password = pht('Does Not Match'); $e_confirm = pht('Does Not Match'); $errors[] = pht('Password and confirmation do not match.'); + } else if ($revoked_password) { + $e_password = pht('Revoked'); + $e_confirm = pht('Revoked'); + $errors[] = pht( + 'This password has been revoked. You must choose a new, unique '. + 'password.'); } else if ($same_password) { $e_password = pht('Not Unique'); $e_confirm = pht('Not Unique');