Added protection against 2 users having same mail

src/Models/Entities/UserEntity.php

@@ -66,20 +66,7 @@ final class UserEntity extends AbstractEntity implements IValidatable
 		$otherUser = UserModel::tryGetByName($userName);
 		if ($otherUser !== null and $otherUser->getId() != $this->getId())
 		{
-			if (!$otherUser->getConfirmedEmail()
+			$this->throwDuplicateUserError($otherUser, 'name');
-				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->isStaffConfirmed()
-				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);
@@ -96,7 +83,7 @@ final class UserEntity extends AbstractEntity implements IValidatable
 			throw new SimpleException('User name contains invalid characters');
 	}
 
-	public function validatePassword()
+	private function validatePassword()
 	{
 		if (empty($this->getPasswordHash()))
 			throw new Exception('Trying to save user with no password into database');
@@ -117,7 +104,7 @@ final class UserEntity extends AbstractEntity implements IValidatable
 			throw new SimpleException('Password contains invalid characters');
 	}
 
-	public function validateAccessRank()
+	private function validateAccessRank()
 	{
 		$this->accessRank->validate();
 
@@ -125,7 +112,7 @@ final class UserEntity extends AbstractEntity implements IValidatable
 			throw new Exception('Cannot set special access rank "%s"', $this->accessRank->toString());
 	}
 
-	public function validateEmails()
+	private function validateEmails()
 	{
 		$this->validateEmail($this->getUnconfirmedEmail());
 		$this->validateEmail($this->getConfirmedEmail());
@@ -135,6 +122,38 @@ final class UserEntity extends AbstractEntity implements IValidatable
 	{
 		if (!empty($email) and !TextHelper::isValidEmail($email))
 			throw new SimpleException('E-mail address appears to be invalid');
+
+		$otherUser = UserModel::tryGetByEmail($email);
+		if ($otherUser !== null and $otherUser->getId() != $this->getId())
+		{
+			$this->throwDuplicateUserError($otherUser, 'e-mail');
+		}
+	}
+
+	private function throwDuplicateUserError($otherUser, $reason)
+	{
+		$config = getConfig();
+
+		if (!$otherUser->getConfirmedEmail()
+			and isset($config->registration->needEmailForRegistering)
+			and $config->registration->needEmailForRegistering)
+		{
+			throw new SimpleException(
+				'User with this %s is already registered and awaits e-mail confirmation',
+				$reason);
+		}
+
+		if (!$otherUser->isStaffConfirmed()
+			and $config->registration->staffActivation)
+		{
+			throw new SimpleException(
+				'User with this %s is already registered and awaits staff confirmation',
+				$reason);
+		}
+
+		throw new SimpleException(
+			'User with this %s is already registered',
+			$reason);
 	}
 
 	public function isBanned()

tests/JobTests/ActivateUserEmailJobTest.php

@@ -108,42 +108,4 @@ class ActivateUserEmailJobTest extends AbstractTest
 				]);
 		}, 'This token was already used');
 	}
-
-	public function testTokensTwoUsersSameMail()
-	{
-		getConfig()->registration->needEmailForRegistering = true;
-		Mailer::mockSending();
-
-		$user1 = $this->mockUser();
-		$user2 = $this->mockUser();
-		$user1->setUnconfirmedEmail('godzilla@whitestar.gov');
-		$user2->setUnconfirmedEmail('godzilla@whitestar.gov');
-		UserModel::save($user1);
-		UserModel::save($user2);
-
-		Api::run(
-			new ActivateUserEmailJob(),
-			[
-				JobArgs::ARG_USER_NAME => $user1->getName(),
-			]);
-
-		Api::run(
-			new ActivateUserEmailJob(),
-			[
-				JobArgs::ARG_USER_NAME => $user2->getName(),
-			]);
-
-		$tokens1 = Mailer::getMailsSent()[0]->tokens;
-		$tokens2 = Mailer::getMailsSent()[1]->tokens;
-		$token1text = $tokens1['token'];
-		$token2text = $tokens2['token'];
-		$this->assert->areNotEqual($token1text, $token2text);
-
-		$token1 = TokenModel::getByToken($token1text);
-		$token2 = TokenModel::getByToken($token2text);
-
-		$this->assert->areEqual($user1->getId(), $token1->getUser()->getId());
-		$this->assert->areEqual($user2->getId(), $token2->getUser()->getId());
-		$this->assert->areNotEqual($token1->getUserId(), $token2->getUserId());
-	}
 }

tests/JobTests/AddUserJobTest.php

@@ -282,7 +282,7 @@ class AddUserJobTest extends AbstractTest
 				]);
 		});
 
-		$user2 = $this->assert->doesNotThrow(function()
+		$user2 = $this->assert->throws(function()
 		{
 			return Api::run(
 				new AddUserJob(),
@@ -291,18 +291,7 @@ class AddUserJobTest extends AbstractTest
 					JobArgs::ARG_NEW_PASSWORD => 'sekai',
 					JobArgs::ARG_NEW_EMAIL => 'godzilla@whitestar.gov',
 				]);
-		});
+		}, 'User with this e-mail is already registered');
-
-		$this->assert->areEqual(2, Mailer::getMailCounter());
-		$token1text = Mailer::getMailsSent()[0]->tokens['token'];
-		$token2text = Mailer::getMailsSent()[1]->tokens['token'];
-		$this->assert->areNotEqual($token1text, $token2text);
-
-		$token1 = TokenModel::getByToken($token1text);
-		$token2 = TokenModel::getByToken($token2text);
-
-		$this->assert->areEqual($user1->getId(), $token1->getUser()->getId());
-		$this->assert->areEqual($user2->getId(), $token2->getUser()->getId());
 	}
 
 	public function testLogBuffering()

tests/ModelTests/UserModelTest.php

@@ -0,0 +1,120 @@
+<?php
+class UserModelTest extends AbstractTest
+{
+	public function testSavingTwoUsersSameNameNoActivation()
+	{
+		getConfig()->registration->needEmailForRegistering = false;
+		list ($user1, $user2) = $this->prepareTwoUsersWithSameName();
+		$this->assert->throws(function() use ($user2)
+		{
+			UserModel::save($user2);
+		}, 'User with this name is already registered');
+	}
+
+	public function testSavingTwoUsersSameNameEmailActivation()
+	{
+		getConfig()->registration->needEmailForRegistering = true;
+		list ($user1, $user2) = $this->prepareTwoUsersWithSameName();
+		$this->assert->throws(function() use ($user2)
+		{
+			UserModel::save($user2);
+		}, 'User with this name is already registered and awaits e-mail');
+	}
+
+	public function testSavingTwoUsersSameNameStaffActivation()
+	{
+		getConfig()->registration->staffActivation = true;
+		list ($user1, $user2) = $this->prepareTwoUsersWithSameName();
+		$this->assert->throws(function() use ($user2)
+		{
+			UserModel::save($user2);
+		}, 'User with this name is already registered and awaits staff');
+	}
+
+	public function testSavingTwoUsersSameEmailNoActivation()
+	{
+		getConfig()->registration->needEmailForRegistering = false;
+		list ($user1, $user2) = $this->prepareTwoUsersWithSameEmail(false, false);
+		$this->assert->throws(function() use ($user2)
+		{
+			UserModel::save($user2);
+		}, 'User with this e-mail is already registered');
+	}
+
+	public function testSavingTwoUsersSameEmailEmailActivation()
+	{
+		getConfig()->registration->needEmailForRegistering = true;
+		list ($user1, $user2) = $this->prepareTwoUsersWithSameEmail(false, false);
+		$this->assert->throws(function() use ($user2)
+		{
+			UserModel::save($user2);
+		}, 'User with this e-mail is already registered and awaits e-mail');
+	}
+
+	public function testSavingTwoUsersSameEmailStaffActivation()
+	{
+		getConfig()->registration->staffActivation = true;
+		list ($user1, $user2) = $this->prepareTwoUsersWithSameEmail(false, false);
+		$this->assert->throws(function() use ($user2)
+		{
+			UserModel::save($user2);
+		}, 'User with this e-mail is already registered and awaits staff');
+	}
+
+	public function testSavingTwoUsersSameEmailDifferConfirm1()
+	{
+		list ($user1, $user2) = $this->prepareTwoUsersWithSameEmail(true, false);
+		$this->assert->throws(function() use ($user2)
+		{
+			UserModel::save($user2);
+		}, 'User with this e-mail is already registered');
+	}
+
+	public function testSavingTwoUsersSameEmailDifferConfirm2()
+	{
+		list ($user1, $user2) = $this->prepareTwoUsersWithSameEmail(false, true);
+		$this->assert->throws(function() use ($user2)
+		{
+			UserModel::save($user2);
+		}, 'User with this e-mail is already registered');
+	}
+
+	public function testSavingTwoUsersSameEmailDifferConfirm3()
+	{
+		list ($user1, $user2) = $this->prepareTwoUsersWithSameEmail(true, true);
+		$this->assert->throws(function() use ($user2)
+		{
+			UserModel::save($user2);
+		}, 'User with this e-mail is already registered');
+	}
+
+	private function prepareTwoUsersWithSameName()
+	{
+		$user1 = $this->mockUser();
+		$user2 = $this->mockUser();
+		$user1->setName('pikachu');
+		$user2->setName('pikachu');
+		UserModel::save($user1);
+		return [$user1, $user2];
+	}
+
+	private function prepareTwoUsersWithSameEmail($confirmFirst, $confirmSecond)
+	{
+		$user1 = $this->mockUser();
+		$user2 = $this->mockUser();
+		$mail = 'godzilla@whitestar.gov';
+
+		if ($confirmFirst)
+			$user1->setConfirmedEmail($mail);
+		else
+			$user1->setUnconfirmedEmail($mail);
+
+		if ($confirmFirst)
+			$user2->setConfirmedEmail($mail);
+		else
+			$user2->setUnconfirmedEmail($mail);
+
+		UserModel::save($user1);
+		return [$user1, $user2];
+	}
+}