mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-22 23:02:42 +01:00
Consolidate password verification/revocation logic in a new PhabricatorAuthPasswordEngine
Summary: Ref T13043. This provides a new piece of shared infrastructure that VCS passwords and account passwords can use to validate passwords that users enter. This isn't reachable by anything yet. The test coverage of the "upgrade" flow (where we rehash a password to use a stronger hasher) isn't great in this diff, I'll expand that in the next change and then start migrating things. Test Plan: Added a bunch of unit tests. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13043 Differential Revision: https://secure.phabricator.com/D18896
This commit is contained in:
parent
42e2cd9af0
commit
c280c85772
6 changed files with 402 additions and 4 deletions
|
@ -2091,6 +2091,7 @@ phutil_register_library_map(array(
|
|||
'PhabricatorAuthOneTimeLoginTemporaryTokenType' => 'applications/auth/tokentype/PhabricatorAuthOneTimeLoginTemporaryTokenType.php',
|
||||
'PhabricatorAuthPassword' => 'applications/auth/storage/PhabricatorAuthPassword.php',
|
||||
'PhabricatorAuthPasswordEditor' => 'applications/auth/editor/PhabricatorAuthPasswordEditor.php',
|
||||
'PhabricatorAuthPasswordEngine' => 'applications/auth/engine/PhabricatorAuthPasswordEngine.php',
|
||||
'PhabricatorAuthPasswordPHIDType' => 'applications/auth/phid/PhabricatorAuthPasswordPHIDType.php',
|
||||
'PhabricatorAuthPasswordQuery' => 'applications/auth/query/PhabricatorAuthPasswordQuery.php',
|
||||
'PhabricatorAuthPasswordResetTemporaryTokenType' => 'applications/auth/tokentype/PhabricatorAuthPasswordResetTemporaryTokenType.php',
|
||||
|
@ -2099,6 +2100,7 @@ phutil_register_library_map(array(
|
|||
'PhabricatorAuthPasswordTestCase' => 'applications/auth/__tests__/PhabricatorAuthPasswordTestCase.php',
|
||||
'PhabricatorAuthPasswordTransaction' => 'applications/auth/storage/PhabricatorAuthPasswordTransaction.php',
|
||||
'PhabricatorAuthPasswordTransactionType' => 'applications/auth/xaction/PhabricatorAuthPasswordTransactionType.php',
|
||||
'PhabricatorAuthPasswordUpgradeTransaction' => 'applications/auth/xaction/PhabricatorAuthPasswordUpgradeTransaction.php',
|
||||
'PhabricatorAuthProvider' => 'applications/auth/provider/PhabricatorAuthProvider.php',
|
||||
'PhabricatorAuthProviderConfig' => 'applications/auth/storage/PhabricatorAuthProviderConfig.php',
|
||||
'PhabricatorAuthProviderConfigController' => 'applications/auth/controller/config/PhabricatorAuthProviderConfigController.php',
|
||||
|
@ -7383,14 +7385,16 @@ phutil_register_library_map(array(
|
|||
'PhabricatorApplicationTransactionInterface',
|
||||
),
|
||||
'PhabricatorAuthPasswordEditor' => 'PhabricatorApplicationTransactionEditor',
|
||||
'PhabricatorAuthPasswordEngine' => 'Phobject',
|
||||
'PhabricatorAuthPasswordPHIDType' => 'PhabricatorPHIDType',
|
||||
'PhabricatorAuthPasswordQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
|
||||
'PhabricatorAuthPasswordResetTemporaryTokenType' => 'PhabricatorAuthTemporaryTokenType',
|
||||
'PhabricatorAuthPasswordRevokeTransaction' => 'PhabricatorAuthPasswordTransactionType',
|
||||
'PhabricatorAuthPasswordRevoker' => 'PhabricatorAuthRevoker',
|
||||
'PhabricatorAuthPasswordTestCase' => 'PhabricatorTestCase',
|
||||
'PhabricatorAuthPasswordTransaction' => 'PhabricatorApplicationTransaction',
|
||||
'PhabricatorAuthPasswordTransaction' => 'PhabricatorModularTransaction',
|
||||
'PhabricatorAuthPasswordTransactionType' => 'PhabricatorModularTransactionType',
|
||||
'PhabricatorAuthPasswordUpgradeTransaction' => 'PhabricatorAuthPasswordTransactionType',
|
||||
'PhabricatorAuthProvider' => 'Phobject',
|
||||
'PhabricatorAuthProviderConfig' => array(
|
||||
'PhabricatorAuthDAO',
|
||||
|
|
|
@ -28,4 +28,96 @@ final class PhabricatorAuthPasswordTestCase extends PhabricatorTestCase {
|
|||
pht('Bad password should not match.'));
|
||||
}
|
||||
|
||||
public function testPasswordEngine() {
|
||||
$password1 = new PhutilOpaqueEnvelope('the quick');
|
||||
$password2 = new PhutilOpaqueEnvelope('brown fox');
|
||||
|
||||
$user = $this->generateNewTestUser();
|
||||
$test_type = PhabricatorAuthPassword::PASSWORD_TYPE_TEST;
|
||||
$account_type = PhabricatorAuthPassword::PASSWORD_TYPE_ACCOUNT;
|
||||
$content_source = $this->newContentSource();
|
||||
|
||||
$engine = id(new PhabricatorAuthPasswordEngine())
|
||||
->setViewer($user)
|
||||
->setContentSource($content_source)
|
||||
->setPasswordType($test_type)
|
||||
->setObject($user);
|
||||
|
||||
$account_engine = id(new PhabricatorAuthPasswordEngine())
|
||||
->setViewer($user)
|
||||
->setContentSource($content_source)
|
||||
->setPasswordType($account_type)
|
||||
->setObject($user);
|
||||
|
||||
// We haven't set any passwords yet, so both passwords should be
|
||||
// invalid.
|
||||
$this->assertFalse($engine->isValidPassword($password1));
|
||||
$this->assertFalse($engine->isValidPassword($password2));
|
||||
|
||||
$pass = PhabricatorAuthPassword::initializeNewPassword($user, $test_type)
|
||||
->setPassword($password1, $user)
|
||||
->save();
|
||||
|
||||
// The password should now be valid.
|
||||
$this->assertTrue($engine->isValidPassword($password1));
|
||||
$this->assertFalse($engine->isValidPassword($password2));
|
||||
|
||||
// But, since the password is a "test" password, it should not be a valid
|
||||
// "account" password.
|
||||
$this->assertFalse($account_engine->isValidPassword($password1));
|
||||
$this->assertFalse($account_engine->isValidPassword($password2));
|
||||
|
||||
// Both passwords are unique for the "test" engine, since an active
|
||||
// password of a given type doesn't collide with itself.
|
||||
$this->assertTrue($engine->isUniquePassword($password1));
|
||||
$this->assertTrue($engine->isUniquePassword($password2));
|
||||
|
||||
// The "test" password is no longer unique for the "account" engine.
|
||||
$this->assertFalse($account_engine->isUniquePassword($password1));
|
||||
$this->assertTrue($account_engine->isUniquePassword($password2));
|
||||
|
||||
$this->revokePassword($user, $pass);
|
||||
|
||||
// Now that we've revoked the password, it should no longer be valid.
|
||||
$this->assertFalse($engine->isValidPassword($password1));
|
||||
$this->assertFalse($engine->isValidPassword($password2));
|
||||
|
||||
// But it should be a revoked password.
|
||||
$this->assertTrue($engine->isRevokedPassword($password1));
|
||||
$this->assertFalse($engine->isRevokedPassword($password2));
|
||||
|
||||
// It should be revoked for both roles: revoking a "test" password also
|
||||
// prevents you from choosing it as a new "account" password.
|
||||
$this->assertTrue($account_engine->isRevokedPassword($password1));
|
||||
$this->assertFalse($account_engine->isValidPassword($password2));
|
||||
|
||||
// The revoked password makes this password non-unique for all account
|
||||
// types.
|
||||
$this->assertFalse($engine->isUniquePassword($password1));
|
||||
$this->assertTrue($engine->isUniquePassword($password2));
|
||||
$this->assertFalse($account_engine->isUniquePassword($password1));
|
||||
$this->assertTrue($account_engine->isUniquePassword($password2));
|
||||
}
|
||||
|
||||
private function revokePassword(
|
||||
PhabricatorUser $actor,
|
||||
PhabricatorAuthPassword $password) {
|
||||
|
||||
$content_source = $this->newContentSource();
|
||||
$revoke_type = PhabricatorAuthPasswordRevokeTransaction::TRANSACTIONTYPE;
|
||||
|
||||
$xactions = array();
|
||||
|
||||
$xactions[] = $password->getApplicationTransactionTemplate()
|
||||
->setTransactionType($revoke_type)
|
||||
->setNewValue(true);
|
||||
|
||||
$editor = $password->getApplicationTransactionEditor()
|
||||
->setActor($actor)
|
||||
->setContinueOnNoEffect(true)
|
||||
->setContinueOnMissingFields(true)
|
||||
->setContentSource($content_source)
|
||||
->applyTransactions($password, $xactions);
|
||||
}
|
||||
|
||||
}
|
||||
|
|
240
src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
Normal file
240
src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
Normal file
|
@ -0,0 +1,240 @@
|
|||
<?php
|
||||
|
||||
final class PhabricatorAuthPasswordEngine
|
||||
extends Phobject {
|
||||
|
||||
private $viewer;
|
||||
private $contentSource;
|
||||
private $object;
|
||||
private $passwordType;
|
||||
private $upgradeHashers = true;
|
||||
|
||||
public function setViewer(PhabricatorUser $viewer) {
|
||||
$this->viewer = $viewer;
|
||||
return $this;
|
||||
}
|
||||
|
||||
public function getViewer() {
|
||||
return $this->viewer;
|
||||
}
|
||||
|
||||
public function setContentSource(PhabricatorContentSource $content_source) {
|
||||
$this->contentSource = $content_source;
|
||||
return $this;
|
||||
}
|
||||
|
||||
public function getContentSource() {
|
||||
return $this->contentSource;
|
||||
}
|
||||
|
||||
public function setObject($object) {
|
||||
$this->object = $object;
|
||||
return $this;
|
||||
}
|
||||
|
||||
public function getObject() {
|
||||
return $this->object;
|
||||
}
|
||||
|
||||
public function setPasswordType($password_type) {
|
||||
$this->passwordType = $password_type;
|
||||
return $this;
|
||||
}
|
||||
|
||||
public function getPasswordType() {
|
||||
return $this->passwordType;
|
||||
}
|
||||
|
||||
public function setUpgradeHashers($upgrade_hashers) {
|
||||
$this->upgradeHashers = $upgrade_hashers;
|
||||
return $this;
|
||||
}
|
||||
|
||||
public function getUpgradeHashers() {
|
||||
return $this->upgradeHashers;
|
||||
}
|
||||
|
||||
public function isValidPassword(PhutilOpaqueEnvelope $envelope) {
|
||||
$this->requireSetup();
|
||||
|
||||
$password_type = $this->getPasswordType();
|
||||
|
||||
$passwords = $this->newQuery()
|
||||
->withPasswordTypes(array($password_type))
|
||||
->withIsRevoked(false)
|
||||
->execute();
|
||||
|
||||
$matches = $this->getMatches($envelope, $passwords);
|
||||
if (!$matches) {
|
||||
return false;
|
||||
}
|
||||
|
||||
if ($this->shouldUpgradeHashers()) {
|
||||
$this->upgradeHashers($envelope, $matches);
|
||||
}
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
public function isUniquePassword(PhutilOpaqueEnvelope $envelope) {
|
||||
$this->requireSetup();
|
||||
|
||||
$password_type = $this->getPasswordType();
|
||||
|
||||
// To test that the password is unique, we're loading all active and
|
||||
// revoked passwords for all roles for the given user, then throwing out
|
||||
// the active passwords for the current role (so a password can't
|
||||
// collide with itself).
|
||||
|
||||
// Note that two different objects can have the same password (say,
|
||||
// users @alice and @bailey). We're only preventing @alice from using
|
||||
// the same password for everything.
|
||||
|
||||
$passwords = $this->newQuery()
|
||||
->execute();
|
||||
|
||||
foreach ($passwords as $key => $password) {
|
||||
$same_type = ($password->getPasswordType() === $password_type);
|
||||
$is_active = !$password->getIsRevoked();
|
||||
|
||||
if ($same_type && $is_active) {
|
||||
unset($passwords[$key]);
|
||||
}
|
||||
}
|
||||
|
||||
$matches = $this->getMatches($envelope, $passwords);
|
||||
|
||||
return !$matches;
|
||||
}
|
||||
|
||||
public function isRevokedPassword(PhutilOpaqueEnvelope $envelope) {
|
||||
$this->requireSetup();
|
||||
|
||||
// To test if a password is revoked, we're loading all revoked passwords
|
||||
// across all roles for the given user. If a password was revoked in one
|
||||
// role, you can't reuse it in a different role.
|
||||
|
||||
$passwords = $this->newQuery()
|
||||
->withIsRevoked(true)
|
||||
->execute();
|
||||
|
||||
$matches = $this->getMatches($envelope, $passwords);
|
||||
|
||||
return (bool)$matches;
|
||||
}
|
||||
|
||||
private function requireSetup() {
|
||||
if (!$this->getObject()) {
|
||||
throw new PhutilInvalidStateException('setObject');
|
||||
}
|
||||
|
||||
if (!$this->getPasswordType()) {
|
||||
throw new PhutilInvalidStateException('setPasswordType');
|
||||
}
|
||||
|
||||
if (!$this->getViewer()) {
|
||||
throw new PhutilInvalidStateException('setViewer');
|
||||
}
|
||||
|
||||
if ($this->shouldUpgradeHashers()) {
|
||||
if (!$this->getContentSource()) {
|
||||
throw new PhutilInvalidStateException('setContentSource');
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private function shouldUpgradeHashers() {
|
||||
if (!$this->getUpgradeHashers()) {
|
||||
return false;
|
||||
}
|
||||
|
||||
if (PhabricatorEnv::isReadOnly()) {
|
||||
// Don't try to upgrade hashers if we're in read-only mode, since we
|
||||
// won't be able to write the new hash to the database.
|
||||
return false;
|
||||
}
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
private function newQuery() {
|
||||
$viewer = $this->getViewer();
|
||||
$object = $this->getObject();
|
||||
$password_type = $this->getPasswordType();
|
||||
|
||||
return id(new PhabricatorAuthPasswordQuery())
|
||||
->setViewer($viewer)
|
||||
->withObjectPHIDs(array($object->getPHID()));
|
||||
}
|
||||
|
||||
private function getMatches(
|
||||
PhutilOpaqueEnvelope $envelope,
|
||||
array $passwords) {
|
||||
|
||||
$object = $this->getObject();
|
||||
|
||||
$matches = array();
|
||||
foreach ($passwords as $password) {
|
||||
try {
|
||||
$is_match = $password->comparePassword($envelope, $object);
|
||||
} catch (PhabricatorPasswordHasherUnavailableException $ex) {
|
||||
$is_match = false;
|
||||
}
|
||||
|
||||
if ($is_match) {
|
||||
$matches[] = $password;
|
||||
}
|
||||
}
|
||||
|
||||
return $matches;
|
||||
}
|
||||
|
||||
private function upgradeHashers(
|
||||
PhutilOpaqueEnvelope $envelope,
|
||||
array $passwords) {
|
||||
|
||||
assert_instances_of($passwords, 'PhabricatorAuthPassword');
|
||||
|
||||
$need_upgrade = array();
|
||||
foreach ($passwords as $password) {
|
||||
if (!$password->canUpgrade()) {
|
||||
continue;
|
||||
}
|
||||
$need_upgrade[] = $password;
|
||||
}
|
||||
|
||||
if (!$need_upgrade) {
|
||||
return;
|
||||
}
|
||||
|
||||
$upgrade_type = PhabricatorAuthPasswordUpgradeTransaction::TRANSACTIONTYPE;
|
||||
$viewer = $this->getViewer();
|
||||
$content_source = $this->getContentSource();
|
||||
|
||||
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
|
||||
foreach ($need_upgrade as $password) {
|
||||
|
||||
// This does the actual upgrade. We then apply a transaction to make
|
||||
// the upgrade more visible and auditable.
|
||||
$old_hasher = $password->getHasher();
|
||||
$password->upgradePasswordHasher($envelope, $this->getObject());
|
||||
$new_hasher = $password->getHasher();
|
||||
|
||||
$xactions = array();
|
||||
|
||||
$xactions[] = $password->getApplicationTransactionTemplate()
|
||||
->setTransactionType($upgrade_type)
|
||||
->setOldValue($old_hasher->getHashName())
|
||||
->setNewValue($new_hasher->getHashName());
|
||||
|
||||
$editor = $password->getApplicationTransactionEditor()
|
||||
->setActor($viewer)
|
||||
->setContinueOnNoEffect(true)
|
||||
->setContinueOnMissingFields(true)
|
||||
->setContentSource($content_source)
|
||||
->applyTransactions($password, $xactions);
|
||||
}
|
||||
unset($unguarded);
|
||||
}
|
||||
|
||||
}
|
|
@ -58,11 +58,46 @@ final class PhabricatorAuthPassword
|
|||
return $this;
|
||||
}
|
||||
|
||||
public function getHasher() {
|
||||
$hash = $this->newPasswordEnvelope();
|
||||
return PhabricatorPasswordHasher::getHasherForHash($hash);
|
||||
}
|
||||
|
||||
public function canUpgrade() {
|
||||
$hash = $this->newPasswordEnvelope();
|
||||
return PhabricatorPasswordHasher::canUpgradeHash($hash);
|
||||
}
|
||||
|
||||
public function upgradePasswordHasher(
|
||||
PhutilOpaqueEnvelope $envelope,
|
||||
PhabricatorUser $object) {
|
||||
|
||||
// Before we make changes, double check that this is really the correct
|
||||
// password. It could be really bad if we "upgraded" a password and changed
|
||||
// the secret!
|
||||
|
||||
if (!$this->comparePassword($envelope, $object)) {
|
||||
throw new Exception(
|
||||
pht(
|
||||
'Attempting to upgrade password hasher, but the password for the '.
|
||||
'upgrade is not the stored credential!'));
|
||||
}
|
||||
|
||||
return $this->setPassword($envelope, $object);
|
||||
}
|
||||
|
||||
public function setPassword(
|
||||
PhutilOpaqueEnvelope $password,
|
||||
PhabricatorUser $object) {
|
||||
|
||||
$hasher = PhabricatorPasswordHasher::getBestHasher();
|
||||
return $this->setPasswordWithHasher($password, $object, $hasher);
|
||||
}
|
||||
|
||||
public function setPasswordWithHasher(
|
||||
PhutilOpaqueEnvelope $password,
|
||||
PhabricatorUser $object,
|
||||
PhabricatorPasswordHasher $hasher) {
|
||||
|
||||
$digest = $this->digestPassword($password, $object);
|
||||
$hash = $hasher->getPasswordHashForStorage($digest);
|
||||
|
@ -76,12 +111,15 @@ final class PhabricatorAuthPassword
|
|||
PhabricatorUser $object) {
|
||||
|
||||
$digest = $this->digestPassword($password, $object);
|
||||
$raw_hash = $this->getPasswordHash();
|
||||
$hash = new PhutilOpaqueEnvelope($raw_hash);
|
||||
$hash = $this->newPasswordEnvelope();
|
||||
|
||||
return PhabricatorPasswordHasher::comparePassword($digest, $hash);
|
||||
}
|
||||
|
||||
private function newPasswordEnvelope() {
|
||||
return new PhutilOpaqueEnvelope($this->getPasswordHash());
|
||||
}
|
||||
|
||||
private function digestPassword(
|
||||
PhutilOpaqueEnvelope $password,
|
||||
PhabricatorUser $object) {
|
||||
|
|
|
@ -1,7 +1,7 @@
|
|||
<?php
|
||||
|
||||
final class PhabricatorAuthPasswordTransaction
|
||||
extends PhabricatorApplicationTransaction {
|
||||
extends PhabricatorModularTransaction {
|
||||
|
||||
public function getApplicationName() {
|
||||
return 'auth';
|
||||
|
|
|
@ -0,0 +1,24 @@
|
|||
<?php
|
||||
|
||||
final class PhabricatorAuthPasswordUpgradeTransaction
|
||||
extends PhabricatorAuthPasswordTransactionType {
|
||||
|
||||
const TRANSACTIONTYPE = 'password.upgrade';
|
||||
|
||||
public function generateOldValue($object) {
|
||||
return $this->getStorage()->getOldValue();
|
||||
}
|
||||
|
||||
public function generateNewValue($object, $value) {
|
||||
return (bool)$value;
|
||||
}
|
||||
|
||||
public function getTitle() {
|
||||
return pht(
|
||||
'%s upgraded the hash algorithm for this password from "%s" to "%s".',
|
||||
$this->renderAuthor(),
|
||||
$this->renderOldValue(),
|
||||
$this->renderNewValue());
|
||||
}
|
||||
|
||||
}
|
Loading…
Reference in a new issue