Fixed multiple problems with user jobs

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);

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));
 	}
 }

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));
 	}
 }

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->getPasswordHash();
-		$oldPasswordHash = $user->passHash;
+		$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));
 	}
 }

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);

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;
 	}

src/Auth.php

@@ -17,8 +17,8 @@ class Auth
 		if ($user === null)
 			throw new SimpleException('Invalid username');
 
-		$passwordHash = UserModel::hashPassword($password, $user->passSalt);
+		$passwordHash = UserModel::hashPassword($password, $user->getPasswordSalt());
-		if ($passwordHash != $user->passHash)
+		if ($passwordHash != $user->getPasswordHash())
 			throw new SimpleException('Invalid password');
 
 		if (!$user->staffConfirmed and $config->registration->staffActivation)

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);
+			$suppliedPasswordHash = UserModel::hashPassword($suppliedPassword, $user->getPasswordSalt());
-			if ($suppliedPasswordHash != $user->passHash)
+			if ($suppliedPasswordHash != $user->getPasswordHash())
 				throw new SimpleException('Must supply valid password');
 		}
 	}

src/Models/Entities/UserEntity.php

@@ -5,8 +5,8 @@ use \Chibi\Database as Database;
 class UserEntity extends AbstractEntity implements IValidatable
 {
 	protected $name;
-	public $passSalt;
+	protected $passSalt;
-	public $passHash;
+	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;
 	}
 

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_salt' => $user->getPasswordSalt(),
-				'pass_hash' => $user->passHash,
+				'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()
 	{

tests/AbstractTest.php

@@ -20,8 +20,8 @@ class AbstractTest
 	{
 		$user = UserModel::spawn();
 		$user->setAccessRank(new AccessRank(AccessRank::Registered));
-		$user->setName('dummy');
+		$user->setName('dummy'.uniqid());
-		$user->passHash = UserModel::hashPassword('ble', $user->passSalt);
+		$user->setPassword('sekai');
 		return UserModel::save($user);
 	}
 

tests/Api/ApiPrivilegeTest.php

@@ -140,8 +140,8 @@ class ApiPrivilegeTest extends AbstractFullApiTest
 	{
 		$ownUser = Auth::getCurrentUser();
 
-		$otherUser = $this->mockUser($this->mockUser());
+		$otherUser = $this->mockUser();
-		$otherUser->setName('somebody-else');
+		$otherUser->setName('dummy' . uniqid());
 		UserModel::save($otherUser);
 
 		$this->testedJobs []= $job;

tests/JobTests/AddUserJobTest.php

@@ -0,0 +1,119 @@
+<?php
+class AddUserJobTest extends AbstractTest
+{
+	public function testSaving()
+	{
+		$this->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');
+	}
+}

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;
 	}
 }