diff --git a/src/Api/Jobs/AddUserJob.php b/src/Api/Jobs/AddUserJob.php index e30fdad4..3c0cdf9e 100644 --- a/src/Api/Jobs/AddUserJob.php +++ b/src/Api/Jobs/AddUserJob.php @@ -8,26 +8,26 @@ class AddUserJob extends AbstractJob $user = UserModel::spawn(); $user->joinDate = time(); $user->staffConfirmed = $firstUser; - $user->setName($this->getArgument(EditUserNameJob::NEW_USER_NAME)); UserModel::forgeId($user); $arguments = $this->getArguments(); $arguments[EditUserJob::USER_ENTITY] = $user; - $arguments[EditUserAccessRankJob::NEW_ACCESS_RANK] = $firstUser - ? AccessRank::Admin - : AccessRank::Registered; - Logger::bufferChanges(); - Access::disablePrivilegeChecking(); $job = new EditUserJob(); $job->setContext(self::CONTEXT_BATCH_ADD); Api::run($job, $arguments); - Access::enablePrivilegeChecking(); Logger::setBuffer([]); if ($firstUser) + { + $user->setAccessRank(new AccessRank(AccessRank::Admin)); $user->confirmEmail(); + } + else + { + $user->setAccessRank(new AccessRank(AccessRank::Registered)); + } //save the user to db if everything went okay UserModel::save($user); diff --git a/src/Api/Jobs/EditUserEmailJob.php b/src/Api/Jobs/EditUserEmailJob.php index 7841fa86..dd1cebd7 100644 --- a/src/Api/Jobs/EditUserEmailJob.php +++ b/src/Api/Jobs/EditUserEmailJob.php @@ -11,7 +11,7 @@ class EditUserEmailJob extends AbstractUserJob public function execute() { if (getConfig()->registration->needEmailForRegistering) - if (!$this->hasArguemnt(self::NEW_EMAIL) or empty($this->getArgument(self::NEW_EMAIL))) + if (!$this->hasArgument(self::NEW_EMAIL) or empty($this->getArgument(self::NEW_EMAIL))) throw new SimpleException('E-mail address is required - you will be sent confirmation e-mail.'); $user = $this->user; @@ -48,7 +48,9 @@ class EditUserEmailJob extends AbstractUserJob public function requiresPrivilege() { return new Privilege( - Privilege::ChangeUserEmail, + $this->getContext() == self::CONTEXT_BATCH_ADD + ? Privilege::RegisterAccount + : Privilege::ChangeUserEmail, Access::getIdentity($this->user)); } } diff --git a/src/Api/Jobs/EditUserNameJob.php b/src/Api/Jobs/EditUserNameJob.php index c92d4440..2bcf6eb4 100644 --- a/src/Api/Jobs/EditUserNameJob.php +++ b/src/Api/Jobs/EditUserNameJob.php @@ -18,7 +18,6 @@ class EditUserNameJob extends AbstractUserJob return $user; $user->setName($newName); - UserModel::validateUserName($user); if ($this->getContext() == self::CONTEXT_NORMAL) UserModel::save($user); @@ -34,7 +33,9 @@ class EditUserNameJob extends AbstractUserJob public function requiresPrivilege() { return new Privilege( - Privilege::ChangeUserName, + $this->getContext() == self::CONTEXT_BATCH_ADD + ? Privilege::RegisterAccount + : Privilege::ChangeUserName, Access::getIdentity($this->user)); } } diff --git a/src/Api/Jobs/EditUserPasswordJob.php b/src/Api/Jobs/EditUserPasswordJob.php index 39c62bc5..c66fa27e 100644 --- a/src/Api/Jobs/EditUserPasswordJob.php +++ b/src/Api/Jobs/EditUserPasswordJob.php @@ -11,15 +11,14 @@ class EditUserPasswordJob extends AbstractUserJob public function execute() { $user = $this->user; - $newPassword = UserModel::validatePassword($this->getArgument(self::NEW_PASSWORD)); + $newPassword = $this->getArgument(self::NEW_PASSWORD); - $newPasswordHash = UserModel::hashPassword($newPassword, $user->passSalt); - $oldPasswordHash = $user->passHash; + $oldPasswordHash = $user->getPasswordHash(); + $user->setPassword($newPassword); + $newPasswordHash = $user->getPasswordHash(); if ($oldPasswordHash == $newPasswordHash) return $user; - $user->passHash = $newPasswordHash; - if ($this->getContext() == self::CONTEXT_NORMAL) UserModel::save($user); @@ -33,7 +32,9 @@ class EditUserPasswordJob extends AbstractUserJob public function requiresPrivilege() { return new Privilege( - Privilege::ChangeUserPassword, + $this->getContext() == self::CONTEXT_BATCH_ADD + ? Privilege::RegisterAccount + : Privilege::ChangeUserPassword, Access::getIdentity($this->user)); } } diff --git a/src/Api/Jobs/PasswordResetJob.php b/src/Api/Jobs/PasswordResetJob.php index 88c5f60b..a7a936ae 100644 --- a/src/Api/Jobs/PasswordResetJob.php +++ b/src/Api/Jobs/PasswordResetJob.php @@ -30,7 +30,7 @@ class PasswordResetJob extends AbstractJob }, array_rand($alphabet, 8))); $user = $token->getUser(); - $user->passHash = UserModel::hashPassword($newPassword, $user->passSalt); + $user->setPassword($newPassword); $token->used = true; TokenModel::save($token); UserModel::save($user); diff --git a/src/Assert.php b/src/Assert.php index b74d03b8..58d01589 100644 --- a/src/Assert.php +++ b/src/Assert.php @@ -12,7 +12,10 @@ class Assert catch (Exception $e) { if (stripos($e->getMessage(), $expectedMessage) === false) - $this->fail('Assertion failed. Expected: "' . $expectedMessage . '", got: "' . $e->getMessage() . '"'); + { + $this->fail('Assertion failed. Expected: "' . $expectedMessage . '", got: "' . $e->getMessage() . '"' + . PHP_EOL . $e->getTraceAsString() . PHP_EOL . '---' . PHP_EOL); + } } if ($success) $this->fail('Assertion failed. Expected exception, got nothing'); @@ -26,7 +29,8 @@ class Assert } catch (Exception $e) { - $this->fail('Assertion failed. Expected nothing, got exception: "' . $e->getMessage() . '"'); + $this->fail('Assertion failed. Expected nothing, got exception: "' . $e->getMessage() . '"' + . PHP_EOL . $e->getTraceAsString() . PHP_EOL . '---' . PHP_EOL); } return $ret; } diff --git a/src/Auth.php b/src/Auth.php index 6f4e16ce..4f4af4e9 100644 --- a/src/Auth.php +++ b/src/Auth.php @@ -17,8 +17,8 @@ class Auth if ($user === null) throw new SimpleException('Invalid username'); - $passwordHash = UserModel::hashPassword($password, $user->passSalt); - if ($passwordHash != $user->passHash) + $passwordHash = UserModel::hashPassword($password, $user->getPasswordSalt()); + if ($passwordHash != $user->getPasswordHash()) throw new SimpleException('Invalid password'); if (!$user->staffConfirmed and $config->registration->staffActivation) diff --git a/src/Controllers/UserController.php b/src/Controllers/UserController.php index d0f19f0b..fc2b0c58 100644 --- a/src/Controllers/UserController.php +++ b/src/Controllers/UserController.php @@ -303,8 +303,8 @@ class UserController if (Auth::getCurrentUser()->getId() == $user->getId()) { $suppliedPassword = InputHelper::get('current-password'); - $suppliedPasswordHash = UserModel::hashPassword($suppliedPassword, $user->passSalt); - if ($suppliedPasswordHash != $user->passHash) + $suppliedPasswordHash = UserModel::hashPassword($suppliedPassword, $user->getPasswordSalt()); + if ($suppliedPasswordHash != $user->getPasswordHash()) throw new SimpleException('Must supply valid password'); } } diff --git a/src/Models/Entities/UserEntity.php b/src/Models/Entities/UserEntity.php index 678f5add..2b8296b0 100644 --- a/src/Models/Entities/UserEntity.php +++ b/src/Models/Entities/UserEntity.php @@ -5,8 +5,8 @@ use \Chibi\Database as Database; class UserEntity extends AbstractEntity implements IValidatable { protected $name; - public $passSalt; - public $passHash; + protected $passSalt; + protected $passHash; public $staffConfirmed; public $emailUnconfirmed; public $emailConfirmed; @@ -16,16 +16,93 @@ class UserEntity extends AbstractEntity implements IValidatable public $settings; protected $banned = false; + protected $__passwordChanged = false; + protected $__password; + public function validate() { - //todo: add more validation + $this->validateUserName(); + $this->validatePassword(); + + //todo: validate e-mails + if (empty($this->getAccessRank())) - throw new SimpleException('No access rank detected'); + throw new Exception('No access rank detected'); if ($this->getAccessRank()->toInteger() == AccessRank::Anonymous) throw new Exception('Trying to save anonymous user into database'); } + + protected function validateUserName() + { + $userName = $this->getName(); + $config = getConfig(); + + $otherUser = UserModel::findByName($userName, false); + if ($otherUser !== null and $otherUser->getId() != $this->getId()) + { + if (!$otherUser->emailConfirmed + and isset($config->registration->needEmailForRegistering) + and $config->registration->needEmailForRegistering) + { + throw new SimpleException('User with this name is already registered and awaits e-mail confirmation'); + } + + if (!$otherUser->staffConfirmed + and $config->registration->staffActivation) + { + throw new SimpleException('User with this name is already registered and awaits staff confirmation'); + } + + throw new SimpleException('User with this name is already registered'); + } + + $userNameMinLength = intval($config->registration->userNameMinLength); + $userNameMaxLength = intval($config->registration->userNameMaxLength); + $userNameRegex = $config->registration->userNameRegex; + + if (strlen($userName) < $userNameMinLength) + throw new SimpleException('User name must have at least %d characters', $userNameMinLength); + + if (strlen($userName) > $userNameMaxLength) + throw new SimpleException('User name must have at most %d characters', $userNameMaxLength); + + if (!preg_match($userNameRegex, $userName)) + throw new SimpleException('User name contains invalid characters'); + } + + public function validatePassword() + { + if (empty($this->getPasswordHash())) + throw new Exception('Trying to save user with no password into database'); + + if (!$this->__passwordChanged) + return; + + $config = getConfig(); + $passMinLength = intval($config->registration->passMinLength); + $passRegex = $config->registration->passRegex; + + $password = $this->__password; + + if (strlen($password) < $passMinLength) + throw new SimpleException('Password must have at least %d characters', $passMinLength); + + if (!preg_match($passRegex, $password)) + throw new SimpleException('Password contains invalid characters'); + } + + public static function validateAccessRank(AccessRank $accessRank) + { + $accessRank->validate(); + + if ($accessRank->toInteger() == AccessRank::Nobody) + throw new Exception('Cannot set special access rank "%s"', $accessRank->toString()); + + return $accessRank; + } + public function isBanned() { return $this->banned; @@ -48,7 +125,30 @@ class UserEntity extends AbstractEntity implements IValidatable public function setName($name) { - $this->name = $name; + $this->name = trim($name); + } + + public function getPasswordHash() + { + return $this->passHash; + } + + public function getPasswordSalt() + { + return $this->passSalt; + } + + public function setPasswordSalt($passSalt) + { + $this->passSalt = $passSalt; + $this->passHash = null; + } + + public function setPassword($password) + { + $this->__passwordChanged = true; + $this->__password = $password; + $this->passHash = UserModel::hashPassword($password, $this->passSalt); } public function getAccessRank() @@ -162,7 +262,7 @@ class UserEntity extends AbstractEntity implements IValidatable if (!empty($this->emailConfirmed)) return; - $this->emailConfirmed = $user->emailUnconfirmed; + $this->emailConfirmed = $this->emailUnconfirmed; $this->emailUnconfirmed = null; } diff --git a/src/Models/UserModel.php b/src/Models/UserModel.php index 9822b30b..7adde427 100644 --- a/src/Models/UserModel.php +++ b/src/Models/UserModel.php @@ -9,6 +9,7 @@ class UserModel extends AbstractCrudModel const SETTING_POST_TAG_TITLES = 3; const SETTING_HIDE_DISLIKED_POSTS = 4; + public static function getTableName() { return 'user'; @@ -28,7 +29,7 @@ class UserModel extends AbstractCrudModel { $user = new UserEntity(); $user->setAccessRank(new AccessRank(AccessRank::Anonymous)); - $user->passSalt = md5(mt_rand() . uniqid()); + $user->setPasswordSalt(md5(mt_rand() . uniqid())); return $user; } @@ -42,8 +43,8 @@ class UserModel extends AbstractCrudModel $bindings = [ 'name' => $user->getName(), - 'pass_salt' => $user->passSalt, - 'pass_hash' => $user->passHash, + 'pass_salt' => $user->getPasswordSalt(), + 'pass_hash' => $user->getPasswordHash(), 'staff_confirmed' => $user->staffConfirmed, 'email_unconfirmed' => $user->emailUnconfirmed, 'email_confirmed' => $user->emailConfirmed, @@ -187,56 +188,6 @@ class UserModel extends AbstractCrudModel }); } - - - public static function validateUserName(UserEntity $user) - { - $userName = trim($user->getName()); - $config = getConfig(); - - $otherUser = self::findByName($userName, false); - if ($otherUser !== null and $otherUser->getId() != $user->getId()) - { - if (!$otherUser->emailConfirmed and $config->registration->needEmailForRegistering) - throw new SimpleException('User with this name is already registered and awaits e-mail confirmation'); - - if (!$otherUser->staffConfirmed and $config->registration->staffActivation) - throw new SimpleException('User with this name is already registered and awaits staff confirmation'); - - throw new SimpleException('User with this name is already registered'); - } - - $userNameMinLength = intval($config->registration->userNameMinLength); - $userNameMaxLength = intval($config->registration->userNameMaxLength); - $userNameRegex = $config->registration->userNameRegex; - - if (strlen($userName) < $userNameMinLength) - throw new SimpleException('User name must have at least %d characters', $userNameMinLength); - - if (strlen($userName) > $userNameMaxLength) - throw new SimpleException('User name must have at most %d characters', $userNameMaxLength); - - if (!preg_match($userNameRegex, $userName)) - throw new SimpleException('User name contains invalid characters'); - - return $userName; - } - - public static function validatePassword($password) - { - $config = getConfig(); - $passMinLength = intval($config->registration->passMinLength); - $passRegex = $config->registration->passRegex; - - if (strlen($password) < $passMinLength) - throw new SimpleException('Password must have at least %d characters', $passMinLength); - - if (!preg_match($passRegex, $password)) - throw new SimpleException('Password contains invalid characters'); - - return $password; - } - public static function validateEmail($email) { $email = trim($email); @@ -247,17 +198,6 @@ class UserModel extends AbstractCrudModel return $email; } - public static function validateAccessRank(AccessRank $accessRank) - { - $accessRank->validate(); - - if ($accessRank->toInteger() == AccessRank::Nobody) - throw new SimpleException('Cannot set special access rank "%s"', $accessRank->toString()); - - return $accessRank; - } - - public static function getAnonymousName() { diff --git a/tests/AbstractTest.php b/tests/AbstractTest.php index 88843990..63f486a3 100644 --- a/tests/AbstractTest.php +++ b/tests/AbstractTest.php @@ -20,8 +20,8 @@ class AbstractTest { $user = UserModel::spawn(); $user->setAccessRank(new AccessRank(AccessRank::Registered)); - $user->setName('dummy'); - $user->passHash = UserModel::hashPassword('ble', $user->passSalt); + $user->setName('dummy'.uniqid()); + $user->setPassword('sekai'); return UserModel::save($user); } diff --git a/tests/Api/ApiPrivilegeTest.php b/tests/Api/ApiPrivilegeTest.php index 04756274..3e5f2c1b 100644 --- a/tests/Api/ApiPrivilegeTest.php +++ b/tests/Api/ApiPrivilegeTest.php @@ -140,8 +140,8 @@ class ApiPrivilegeTest extends AbstractFullApiTest { $ownUser = Auth::getCurrentUser(); - $otherUser = $this->mockUser($this->mockUser()); - $otherUser->setName('somebody-else'); + $otherUser = $this->mockUser(); + $otherUser->setName('dummy' . uniqid()); UserModel::save($otherUser); $this->testedJobs []= $job; diff --git a/tests/JobTests/AddUserJobTest.php b/tests/JobTests/AddUserJobTest.php new file mode 100644 index 00000000..a06b52e1 --- /dev/null +++ b/tests/JobTests/AddUserJobTest.php @@ -0,0 +1,119 @@ +grantAccess('registerAccount'); + + $user1 = $this->assert->doesNotThrow(function() + { + return Api::run( + new AddUserJob(), + [ + EditUserNameJob::NEW_USER_NAME => 'dummy', + EditUserPasswordJob::NEW_PASSWORD => 'sekai', + ]); + }); + + $this->assert->areEqual('dummy', $user1->getName()); + $this->assert->areEquivalent(new AccessRank(AccessRank::Admin), $user1->getAccessRank()); + $this->assert->isFalse(empty($user1->getPasswordSalt())); + $this->assert->isFalse(empty($user1->getPasswordHash())); + + $user2 = $this->assert->doesNotThrow(function() + { + return Api::run( + new AddUserJob(), + [ + EditUserNameJob::NEW_USER_NAME => 'dummy2', + EditUserPasswordJob::NEW_PASSWORD => 'sekai', + ]); + }); + + $this->assert->areEquivalent(new AccessRank(AccessRank::Registered), $user2->getAccessRank()); + } + + public function testTooShortPassword() + { + $this->grantAccess('registerAccount'); + + $this->assert->throws(function() + { + Api::run( + new AddUserJob(), + [ + EditUserNameJob::NEW_USER_NAME => 'dummy', + EditUserPasswordJob::NEW_PASSWORD => str_repeat('s', getConfig()->registration->passMinLength - 1), + ]); + }, 'Password must have at least'); + } + + public function testVeryLongPassword() + { + $this->grantAccess('registerAccount'); + + $pass = str_repeat('s', 10000); + $user = $this->assert->doesNotThrow(function() use ($pass) + { + return Api::run( + new AddUserJob(), + [ + EditUserNameJob::NEW_USER_NAME => 'dummy', + EditUserPasswordJob::NEW_PASSWORD => $pass, + ]); + }); + + $this->assert->isTrue(strlen($user->getPasswordHash()) < 100); + + getConfig()->registration->needEmailForRegistering = false; + $this->assert->doesNotThrow(function() use ($pass) + { + Auth::login('dummy', $pass, false); + }); + $this->assert->throws(function() use ($pass) + { + Auth::login('dummy', $pass . '!', false); + }, 'Invalid password'); + } + + public function testDuplicateNames() + { + $this->grantAccess('registerAccount'); + + $this->assert->doesNotThrow(function() + { + Api::run( + new AddUserJob(), + [ + EditUserNameJob::NEW_USER_NAME => 'dummy', + EditUserPasswordJob::NEW_PASSWORD => 'sekai', + ]); + }); + + $this->assert->throws(function() + { + Api::run( + new AddUserJob(), + [ + EditUserNameJob::NEW_USER_NAME => 'dummy', + EditUserPasswordJob::NEW_PASSWORD => 'sekai', + ]); + }, 'User with'); + } + + public function testAccessRankDenial() + { + $this->grantAccess('registerAccount'); + + $this->assert->throws(function() + { + Api::run( + new AddUserJob(), + [ + EditUserNameJob::NEW_USER_NAME => 'dummy', + EditUserPasswordJob::NEW_PASSWORD => 'sekai', + EditUserAccessRankJob::NEW_ACCESS_RANK => 'power-user', + ]); + }, 'Insufficient privileges'); + } +} diff --git a/tests/MiscTests/AuthTest.php b/tests/MiscTests/AuthTest.php index f301507a..25f8677f 100644 --- a/tests/MiscTests/AuthTest.php +++ b/tests/MiscTests/AuthTest.php @@ -11,7 +11,7 @@ class AuthTest extends AbstractTest $this->assert->doesNotThrow(function() { - Auth::login('existing', 'ble', false); + Auth::login('existing', 'bleee', false); }); } @@ -35,7 +35,7 @@ class AuthTest extends AbstractTest public function testInvalidPassword() { $user = $this->prepareValidUser(); - $user->passHash = UserModel::hashPassword('ble2', $user->passSalt); + $user->setPassword('blee2'); UserModel::save($user); $this->assert->throws(function() @@ -55,7 +55,7 @@ class AuthTest extends AbstractTest $this->assert->throws(function() { - Auth::login('existing', 'ble', false); + Auth::login('existing', 'bleee', false); }, 'You are banned'); } @@ -70,7 +70,7 @@ class AuthTest extends AbstractTest $this->assert->throws(function() { - Auth::login('existing', 'ble', false); + Auth::login('existing', 'bleee', false); }, 'staff hasn\'t confirmed'); } @@ -85,7 +85,7 @@ class AuthTest extends AbstractTest $this->assert->doesNotThrow(function() { - Auth::login('existing', 'ble', false); + Auth::login('existing', 'bleee', false); }); } @@ -100,7 +100,7 @@ class AuthTest extends AbstractTest $this->assert->throws(function() { - Auth::login('existing', 'ble', false); + Auth::login('existing', 'bleee', false); }, 'need e-mail address confirmation'); } @@ -116,7 +116,7 @@ class AuthTest extends AbstractTest $this->assert->throws(function() { - Auth::login('existing', 'ble', false); + Auth::login('existing', 'bleee', false); }, 'need e-mail address confirmation'); } @@ -132,7 +132,7 @@ class AuthTest extends AbstractTest $this->assert->doesNotThrow(function() { - Auth::login('existing', 'ble', false); + Auth::login('existing', 'bleee', false); }); } @@ -143,7 +143,7 @@ class AuthTest extends AbstractTest $user = UserModel::spawn(); $user->setAccessRank(new AccessRank(AccessRank::Registered)); $user->setName('existing'); - $user->passHash = UserModel::hashPassword('ble', $user->passSalt); + $user->setPassword('bleee'); return $user; } }