From 185f820e21fa089516e45717ee56de47f779217d Mon Sep 17 00:00:00 2001 From: Marcin Kurczewski Date: Sun, 5 Oct 2014 22:26:56 +0200 Subject: [PATCH] Added support for legacy passwords --- .../EntityConverters/UserEntityConverter.php | 2 ++ src/Entities/User.php | 11 +++++++ src/Services/AuthService.php | 8 +++-- src/Services/PasswordService.php | 20 +++++++++++-- src/Services/UserService.php | 5 ++-- src/Upgrades/Upgrade17.php | 13 ++++++++ src/di.php | 1 + tests/Services/AuthServiceTest.php | 8 +++-- tests/Services/PasswordServiceTest.php | 30 +++++++++++++++++-- tests/Services/UserServiceTest.php | 6 ++-- 10 files changed, 91 insertions(+), 13 deletions(-) create mode 100644 src/Upgrades/Upgrade17.php diff --git a/src/Dao/EntityConverters/UserEntityConverter.php b/src/Dao/EntityConverters/UserEntityConverter.php index e192082d..be474290 100644 --- a/src/Dao/EntityConverters/UserEntityConverter.php +++ b/src/Dao/EntityConverters/UserEntityConverter.php @@ -12,6 +12,7 @@ class UserEntityConverter extends AbstractEntityConverter implements IEntityConv 'email' => $entity->getEmail(), 'emailUnconfirmed' => $entity->getEmailUnconfirmed(), 'passwordHash' => $entity->getPasswordHash(), + 'passwordSalt' => $entity->getPasswordSalt(), 'accessRank' => $entity->getAccessRank(), 'registrationTime' => $this->entityTimeToDbTime($entity->getRegistrationTime()), 'lastLoginTime' => $this->entityTimeToDbTime($entity->getLastLoginTime()), @@ -29,6 +30,7 @@ class UserEntityConverter extends AbstractEntityConverter implements IEntityConv $entity->setEmail($array['email']); $entity->setEmailUnconfirmed($array['emailUnconfirmed']); $entity->setPasswordHash($array['passwordHash']); + $entity->setPasswordSalt($array['passwordSalt']); $entity->setAccessRank(intval($array['accessRank'])); $entity->setRegistrationTime($this->dbTimeToEntityTime($array['registrationTime'])); $entity->setLastLoginTime($this->dbTimeToEntityTime($array['lastLoginTime'])); diff --git a/src/Entities/User.php b/src/Entities/User.php index c8f70c85..035a88f0 100644 --- a/src/Entities/User.php +++ b/src/Entities/User.php @@ -20,6 +20,7 @@ final class User extends Entity protected $email; protected $emailUnconfirmed; protected $passwordHash; + protected $passwordSalt; protected $accessRank; protected $registrationTime; protected $lastLoginTime; @@ -88,6 +89,16 @@ final class User extends Entity $this->passwordHash = $passwordHash; } + public function getPasswordSalt() + { + return $this->passwordSalt; + } + + public function setPasswordSalt($passwordSalt) + { + $this->passwordSalt = $passwordSalt; + } + public function getAccessRank() { return $this->accessRank; diff --git a/src/Services/AuthService.php b/src/Services/AuthService.php index e745c402..047fcb3f 100644 --- a/src/Services/AuthService.php +++ b/src/Services/AuthService.php @@ -48,8 +48,12 @@ class AuthService $user = $this->userService->getByNameOrEmail($formData->userNameOrEmail); $this->doFinalChecksOnUser($user); - $passwordHash = $this->passwordService->getHash($formData->password); - if ($user->getPasswordHash() !== $passwordHash) + $hashValid = $this->passwordService->isHashValid( + $formData->password, + $user->getPasswordSalt(), + $user->getPasswordHash()); + + if (!$hashValid) throw new \InvalidArgumentException('Specified password is invalid.'); $this->loginToken = $this->createAndSaveLoginToken($user); diff --git a/src/Services/PasswordService.php b/src/Services/PasswordService.php index 6ebc775e..aaf02e86 100644 --- a/src/Services/PasswordService.php +++ b/src/Services/PasswordService.php @@ -19,9 +19,25 @@ class PasswordService $this->pattern = str_split('cvcvnncvcv'); } - public function getHash($password) + public function getLegacyHash($password, $salt) { - return hash('sha256', $this->config->security->secret . '/' . $password); + //hash used by old szurubooru version + return sha1('1A2/$_4xVa' . $salt . $password); + } + + public function getHash($password, $salt) + { + return hash('sha256', $this->config->security->secret . $salt . $password); + } + + public function isHashValid($password, $salt, $expectedPasswordHash) + { + $hashes = + [ + $this->getLegacyHash($password, $salt), + $this->getHash($password, $salt), + ]; + return in_array($expectedPasswordHash, $hashes); } public function getRandomPassword() diff --git a/src/Services/UserService.php b/src/Services/UserService.php index d98b2fcb..35b24b95 100644 --- a/src/Services/UserService.php +++ b/src/Services/UserService.php @@ -100,6 +100,7 @@ class UserService $user->setAccessRank($this->userDao->hasAnyUsers() ? \Szurubooru\Entities\User::ACCESS_RANK_REGULAR_USER : \Szurubooru\Entities\User::ACCESS_RANK_ADMINISTRATOR); + $user->setPasswordSalt($this->passwordService->getRandomPassword()); $this->updateUserName($user, $formData->userName); $this->updateUserPassword($user, $formData->password); @@ -173,7 +174,7 @@ class UserService $user = $this->getByName($token->getAdditionalData()); $newPassword = $this->passwordService->getRandomPassword(); - $user->setPasswordHash($this->passwordService->getHash($newPassword)); + $this->updateUserPassword($user, $newPassword); $this->userDao->save($user); $this->tokenService->invalidateByName($token->getName()); return $newPassword; @@ -231,7 +232,7 @@ class UserService private function updateUserPassword(\Szurubooru\Entities\User $user, $newPassword) { - $user->setPasswordHash($this->passwordService->getHash($newPassword)); + $user->setPasswordHash($this->passwordService->getHash($newPassword, $user->getPasswordSalt())); } private function updateUserEmail(\Szurubooru\Entities\User $user, $newEmail) diff --git a/src/Upgrades/Upgrade17.php b/src/Upgrades/Upgrade17.php new file mode 100644 index 00000000..28098471 --- /dev/null +++ b/src/Upgrades/Upgrade17.php @@ -0,0 +1,13 @@ +getPDO(); + + $pdo->exec('ALTER TABLE users ADD COLUMN passwordSalt VARCHAR(32)'); + $pdo->exec('UPDATE users SET passwordSalt = "/"'); + } +} diff --git a/src/di.php b/src/di.php index 0dcfddaa..20989485 100644 --- a/src/di.php +++ b/src/di.php @@ -32,6 +32,7 @@ return [ $container->get(\Szurubooru\Upgrades\Upgrade14::class), $container->get(\Szurubooru\Upgrades\Upgrade15::class), $container->get(\Szurubooru\Upgrades\Upgrade16::class), + $container->get(\Szurubooru\Upgrades\Upgrade17::class), ]; }), diff --git a/tests/Services/AuthServiceTest.php b/tests/Services/AuthServiceTest.php index 7654bdd6..23729cf6 100644 --- a/tests/Services/AuthServiceTest.php +++ b/tests/Services/AuthServiceTest.php @@ -22,11 +22,12 @@ class AuthServiceTest extends \Szurubooru\Tests\AbstractTestCase public function testInvalidPassword() { $this->configMock->set('security/needEmailActivationToRegister', false); - $this->passwordServiceMock->expects($this->once())->method('getHash')->willReturn('unmatchingHash'); + $this->passwordServiceMock->expects($this->once())->method('isHashValid')->with('godzilla', 'salt', 'hash')->willReturn(false); $testUser = new \Szurubooru\Entities\User(); $testUser->setName('dummy'); $testUser->setPasswordHash('hash'); + $testUser->setPasswordSalt('salt'); $this->userServiceMock->expects($this->once())->method('getByNameOrEmail')->willReturn($testUser); $this->setExpectedException(\Exception::class, 'Specified password is invalid'); @@ -40,11 +41,12 @@ class AuthServiceTest extends \Szurubooru\Tests\AbstractTestCase public function testValidCredentials() { $this->configMock->set('security/needEmailActivationToRegister', false); - $this->passwordServiceMock->expects($this->once())->method('getHash')->willReturn('hash'); + $this->passwordServiceMock->expects($this->once())->method('isHashValid')->with('godzilla', 'salt', 'hash')->willReturn(true); $testUser = new \Szurubooru\Entities\User('an unusual database identifier'); $testUser->setName('dummy'); $testUser->setPasswordHash('hash'); + $testUser->setPasswordSalt('salt'); $this->userServiceMock->expects($this->once())->method('getByNameOrEmail')->willReturn($testUser); $testToken = new \Szurubooru\Entities\Token(); @@ -68,7 +70,7 @@ class AuthServiceTest extends \Szurubooru\Tests\AbstractTestCase public function testValidCredentialsUnconfirmedEmail() { $this->configMock->set('security/needEmailActivationToRegister', true); - $this->passwordServiceMock->expects($this->never())->method('getHash')->willReturn('hash'); + $this->passwordServiceMock->expects($this->never())->method('isHashValid')->willReturn('hash'); $testUser = new \Szurubooru\Entities\User(); $testUser->setName('dummy'); diff --git a/tests/Services/PasswordServiceTest.php b/tests/Services/PasswordServiceTest.php index 0cc9a70e..5923f21d 100644 --- a/tests/Services/PasswordServiceTest.php +++ b/tests/Services/PasswordServiceTest.php @@ -3,10 +3,31 @@ namespace Szurubooru\Tests\Service; class PasswordServiceTest extends \Szurubooru\Tests\AbstractTestCase { + private $configMock; + + public function setUp() + { + parent::setUp(); + $this->configMock = $this->mockConfig(); + } + + public function testLegacyPasswordValidation() + { + $passwordService = $this->getPasswordService(); + $this->configMock->set('security/secret', 'doesnt matter'); + $this->assertTrue($passwordService->isHashValid('testt', 'ac63e0bcdf20b82db509d123166c4592', '2602572e077d48b35af39d1cff84bfcaa5363116')); + } + + public function testPasswordValidation() + { + $passwordService = $this->getPasswordService(); + $this->configMock->set('security/secret', 'change'); + $this->assertTrue($passwordService->isHashValid('testt', '/', '4f4f8b836cd65f3f1d0b7751fc442f79595c23439cd8a928af15e10807bf08cc')); + } + public function testGeneratingPasswords() { - $configMock = $this->mockConfig(); - $passwordService = new \Szurubooru\Services\PasswordService($configMock); + $passwordService = $this->getPasswordService(); $sampleCount = 10000; $distribution = []; @@ -44,4 +65,9 @@ class PasswordServiceTest extends \Szurubooru\Tests\AbstractTestCase $mean = array_sum($sample) / count($sample); return 100 * $this->getStandardDeviation($sample) / $mean; } + + private function getPasswordService() + { + return new \Szurubooru\Services\PasswordService($this->configMock); + } } diff --git a/tests/Services/UserServiceTest.php b/tests/Services/UserServiceTest.php index 4fbb26b0..847db2b9 100644 --- a/tests/Services/UserServiceTest.php +++ b/tests/Services/UserServiceTest.php @@ -73,7 +73,8 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase $formData->email = 'human@people.gov'; $this->configMock->set('security/needEmailActivationToRegister', false); - $this->passwordServiceMock->expects($this->once())->method('getHash')->willReturn('hash'); + $this->passwordServiceMock->expects($this->once())->method('getRandomPassword')->willReturn('salt'); + $this->passwordServiceMock->expects($this->once())->method('getHash')->with('password', 'salt')->willReturn('hash'); $this->timeServiceMock->expects($this->once())->method('getCurrentTime')->willReturn('now'); $this->userDaoMock->expects($this->once())->method('hasAnyUsers')->willReturn(true); $this->userDaoMock->expects($this->once())->method('save')->will($this->returnArgument(0)); @@ -99,7 +100,8 @@ final class UserServiceTest extends \Szurubooru\Tests\AbstractTestCase $formData->email = 'human@people.gov'; $this->configMock->set('security/needEmailActivationToRegister', true); - $this->passwordServiceMock->expects($this->once())->method('getHash')->willReturn('hash'); + $this->passwordServiceMock->expects($this->once())->method('getRandomPassword')->willReturn('salt'); + $this->passwordServiceMock->expects($this->once())->method('getHash')->with('password', 'salt')->willReturn('hash'); $this->timeServiceMock->expects($this->once())->method('getCurrentTime')->willReturn('now'); $this->userDaoMock->expects($this->once())->method('hasAnyUsers')->willReturn(true); $this->userDaoMock->expects($this->once())->method('save')->will($this->returnArgument(0));