diff --git a/app/V1Module/presenters/RegistrationPresenter.php b/app/V1Module/presenters/RegistrationPresenter.php index eebe4a408..2fb44a964 100644 --- a/app/V1Module/presenters/RegistrationPresenter.php +++ b/app/V1Module/presenters/RegistrationPresenter.php @@ -3,6 +3,7 @@ namespace App\V1Module\Presenters; use App\Helpers\MetaFormats\Attributes\Post; +use App\Helpers\MetaFormats\Validators\VBool; use App\Helpers\MetaFormats\Validators\VEmail; use App\Helpers\MetaFormats\Validators\VMixed; use App\Helpers\MetaFormats\Validators\VString; @@ -162,6 +163,12 @@ public function checkCreateAccount() #[Post("instanceId", new VString(1), "Identifier of the instance to register in")] #[Post("titlesBeforeName", new VString(1), "Titles which is placed before user name", required: false)] #[Post("titlesAfterName", new VString(1), "Titles which is placed after user name", required: false)] + #[Post( + "ignoreNameCollision", + new VBool(), + "If a use with the same name exists, this needs to be set to true.", + required: false + )] public function actionCreateAccount() { $req = $this->getRequest(); @@ -169,15 +176,23 @@ public function actionCreateAccount() // check if the email is free $email = trim($req->getPost("email")); // username is name of column which holds login identifier represented by email - if ($this->logins->getByUsername($email) !== null) { - throw new BadRequestException("This email address is already taken."); + if ($this->logins->getByUsername($email) !== null || $this->users->getByEmail($email) !== null) { + throw new BadRequestException( + "This email address is already taken.", + FrontendErrorMappings::E400_110__USER_EMAIL_ALREADY_EXISTS + ); } $instanceId = $req->getPost("instanceId"); $instance = $this->getInstance($instanceId); - $titlesBeforeName = $req->getPost("titlesBeforeName") === null ? "" : $req->getPost("titlesBeforeName"); - $titlesAfterName = $req->getPost("titlesAfterName") === null ? "" : $req->getPost("titlesAfterName"); + $titlesBeforeName = trim($req->getPost("titlesBeforeName") ?? ""); + $titlesAfterName = trim($req->getPost("titlesAfterName") ?? ""); + $firstName = trim($req->getPost("firstName") ?? ""); + $lastName = trim($req->getPost("lastName") ?? ""); + if (!$firstName || !$lastName) { + throw new BadRequestException("The user's full name must be filled in."); + } // check given passwords $password = $req->getPost("password"); @@ -189,10 +204,23 @@ public function actionCreateAccount() ); } + // Check for name collisions, unless the request explicitly says to ignore them. + if (!$req->getPost("ignoreNameCollision")) { + $sameName = $this->users->findByName($instance, $firstName, $lastName); + if ($sameName) { + // let's report the colliding users + $this->sendSuccessResponse([ + "user" => null, + "usersWithSameName" => $this->userViewFactory->getUsers($sameName), + ]); + return; + } + } + $user = new User( $email, - $req->getPost("firstName"), - $req->getPost("lastName"), + $firstName, + $lastName, $titlesBeforeName, $titlesAfterName, Roles::STUDENT_ROLE, @@ -272,31 +300,49 @@ public function actionCreateInvitation() // check if the email is free $email = trim($format->email); // username is name of column which holds login identifier represented by email - if ($this->logins->getByUsername($email) !== null) { - throw new BadRequestException("This email address is already taken."); + if ($this->logins->getByUsername($email) !== null || $this->users->getByEmail($email) !== null) { + throw new BadRequestException( + "This email address is already taken.", + FrontendErrorMappings::E400_110__USER_EMAIL_ALREADY_EXISTS + ); } $groupsIds = $format->groups ?? []; foreach ($groupsIds as $id) { $group = $this->groups->get($id); if (!$group || $group->isOrganizational() || !$this->groupAcl->canInviteStudents($group)) { - throw new BadRequestException("Current user cannot invite people in group '$id'"); + throw new ForbiddenRequestException("Current user cannot invite people in group '$id'"); } } // gather data $instanceId = $format->instanceId; - $instance = $this->getInstance($instanceId); - $titlesBeforeName = $format->titlesBeforeName === null ? "" : $format->titlesBeforeName; - $titlesAfterName = $format->titlesAfterName === null ? "" : $format->titlesAfterName; + $instance = $this->getInstance($instanceId); // we don't need it, just to check it exists + $titlesBeforeName = $format->titlesBeforeName === null ? "" : trim($format->titlesBeforeName); + $titlesAfterName = $format->titlesAfterName === null ? "" : trim($format->titlesAfterName); + $firstName = trim($format->firstName); + $lastName = trim($format->lastName); + if (!$firstName || !$lastName) { + throw new BadRequestException("The user's full name must be filled in."); + } + + // Check for name collisions, unless the request explicitly says to ignore them. + if (!$format->ignoreNameCollision) { + $sameName = $this->users->findByName($instance, $firstName, $lastName); + if ($sameName) { + // let's report the colliding users + $this->sendSuccessResponse($this->userViewFactory->getUsers($sameName)); + return; + } + } // create the token and send it via email try { $this->invitationHelper->invite( $instanceId, $email, - $format->firstName, - $format->lastName, + $firstName, + $lastName, $titlesBeforeName, $titlesAfterName, $groupsIds, diff --git a/app/exceptions/FrontendErrorMappings.php b/app/exceptions/FrontendErrorMappings.php index 8e3449f1b..b5cd246dd 100644 --- a/app/exceptions/FrontendErrorMappings.php +++ b/app/exceptions/FrontendErrorMappings.php @@ -34,6 +34,8 @@ class FrontendErrorMappings public const E400_104__EXTERNAL_AUTH_FAILED_USER_NOT_FOUND = "400-104"; /** External authentication failed - unable to register new user because no role was provided. */ public const E400_105__EXTERNAL_AUTH_FAILED_MISSING_ROLE = "400-105"; + /** User email already exists, new user cannot be created/invited/registered/.... */ + public const E400_110__USER_EMAIL_ALREADY_EXISTS = "400-110"; /** General job config error */ public const E400_200__JOB_CONFIG = "400-200"; diff --git a/app/helpers/MetaFormats/FormatDefinitions/UserFormat.php b/app/helpers/MetaFormats/FormatDefinitions/UserFormat.php index d400a9097..a77d61f58 100644 --- a/app/helpers/MetaFormats/FormatDefinitions/UserFormat.php +++ b/app/helpers/MetaFormats/FormatDefinitions/UserFormat.php @@ -4,10 +4,9 @@ use App\Helpers\MetaFormats\Attributes\Format; use App\Helpers\MetaFormats\MetaFormat; -use App\Helpers\MetaFormats\Attributes\FormatParameterAttribute; use App\Helpers\MetaFormats\Attributes\FPost; -use App\Helpers\MetaFormats\Type; use App\Helpers\MetaFormats\Validators\VArray; +use App\Helpers\MetaFormats\Validators\VBool; use App\Helpers\MetaFormats\Validators\VEmail; use App\Helpers\MetaFormats\Validators\VString; use App\Helpers\MetaFormats\Validators\VUuid; @@ -41,4 +40,7 @@ class UserFormat extends MetaFormat #[FPost(new VString(2, 2), "Language used in the invitation email (en by default).", required: false)] public ?string $locale; + + #[FPost(new VBool(), "If a use with the same name exists, this needs to be set to true.", required: false)] + public ?bool $ignoreNameCollision; } diff --git a/app/model/repository/Users.php b/app/model/repository/Users.php index 5ddb27e36..ec483b70d 100644 --- a/app/model/repository/Users.php +++ b/app/model/repository/Users.php @@ -5,6 +5,7 @@ use App\Helpers\Pagination; use App\Model\Helpers\PaginationDbHelper; use App\Model\Entity\User; +use App\Model\Entity\Instance; use DateTime; use Doctrine\ORM\EntityManagerInterface; @@ -55,7 +56,7 @@ public function getPaginated(Pagination $pagination, &$totalCount): array 'email' => ['u.email'], 'createdAt' => ['u.createdAt'], ], - ['firstName', 'lastName'] // search column names + ['firstName', 'lastName', 'email'] // search column names ); $paginationDbHelper->apply($qb, $pagination); @@ -75,7 +76,7 @@ public function getPaginated(Pagination $pagination, &$totalCount): array /** - * Search users first names and surnames based on given string. + * Search users first names and surnames based on given (sub)string. * @param string|null $search * @return User[] */ @@ -93,6 +94,21 @@ public function findByRoles(string ...$roles): array return $this->findBy(["role" => $roles]); } + /** + * Find users by exact match of the whole name. + * @param Instance $instance + * @param string $firstName + * @param string $lastName + * @return User[] + */ + public function findByName(Instance $instance, string $firstName, string $lastName): array + { + $users = $this->findBy(["firstName" => $firstName, "lastName" => $lastName]); + return array_filter($users, function (User $user) use ($instance) { + return $user->belongsTo($instance); + }); + } + /** * Find all users who have not authenticated to the system for some time. * @param DateTime|null $before Only users with last activity before given date diff --git a/tests/Presenters/RegistrationPresenter.phpt b/tests/Presenters/RegistrationPresenter.phpt index 2d93c9526..469f08ac3 100644 --- a/tests/Presenters/RegistrationPresenter.phpt +++ b/tests/Presenters/RegistrationPresenter.phpt @@ -79,8 +79,8 @@ class TestRegistrationPresenter extends Tester\TestCase $this->presenter = PresenterTestHelper::createPresenter($this->container, RegistrationPresenter::class); $this->presenter->registrationConfig = new RegistrationConfig( [ - 'enabled' => true, - 'implicitGroupsIds' => [] + 'enabled' => true, + 'implicitGroupsIds' => [] ] ); @@ -118,14 +118,14 @@ class TestRegistrationPresenter extends Tester\TestCase 'POST', ['action' => 'createAccount'], [ - 'email' => $email, - 'firstName' => $firstName, - 'lastName' => $lastName, - 'password' => $password, - 'passwordConfirm' => $password, - 'instanceId' => $instanceId, - 'titlesBeforeName' => $titlesBeforeName, - 'titlesAfterName' => $titlesAfterName + 'email' => $email, + 'firstName' => $firstName, + 'lastName' => $lastName, + 'password' => $password, + 'passwordConfirm' => $password, + 'instanceId' => $instanceId, + 'titlesBeforeName' => $titlesBeforeName, + 'titlesAfterName' => $titlesAfterName ] ); $response = $this->presenter->run($request); @@ -170,20 +170,20 @@ class TestRegistrationPresenter extends Tester\TestCase 'POST', ['action' => 'createAccount'], [ - 'email' => $email, - 'firstName' => $firstName, - 'lastName' => $lastName, - 'password' => $password, - 'passwordConfirm' => $password, - 'instanceId' => $instanceId, - 'titlesBeforeName' => $titlesBeforeName, - 'titlesAfterName' => $titlesAfterName + 'email' => $email, + 'firstName' => $firstName, + 'lastName' => $lastName, + 'password' => $password, + 'passwordConfirm' => $password, + 'instanceId' => $instanceId, + 'titlesBeforeName' => $titlesBeforeName, + 'titlesAfterName' => $titlesAfterName ] ); $this->presenter->registrationConfig = new RegistrationConfig( [ - 'enabled' => true, - 'implicitGroupsIds' => [$groupId] + 'enabled' => true, + 'implicitGroupsIds' => [$groupId] ] ); $response = $this->presenter->run($request); @@ -226,19 +226,19 @@ class TestRegistrationPresenter extends Tester\TestCase 'POST', ['action' => 'createAccount'], [ - 'email' => $email, - 'firstName' => $firstName, - 'lastName' => $lastName, - 'password' => $password, - 'passwordConfirm' => $password, - 'instanceId' => $instanceId, - 'titlesBeforeName' => $titlesBeforeName, - 'titlesAfterName' => $titlesAfterName + 'email' => $email, + 'firstName' => $firstName, + 'lastName' => $lastName, + 'password' => $password, + 'passwordConfirm' => $password, + 'instanceId' => $instanceId, + 'titlesBeforeName' => $titlesBeforeName, + 'titlesAfterName' => $titlesAfterName ] ); $this->presenter->registrationConfig = new RegistrationConfig( [ - 'enabled' => false, + 'enabled' => false, ] ); @@ -265,14 +265,14 @@ class TestRegistrationPresenter extends Tester\TestCase 'POST', ['action' => 'createAccount'], [ - 'email' => $email, - 'firstName' => $firstName, - 'lastName' => $lastName, - 'password' => $password, - 'passwordConfirm' => $password, - 'instanceId' => $instanceId, - 'titlesBeforeName' => $titlesBeforeName, - 'titlesAfterName' => $titlesAfterName + 'email' => $email, + 'firstName' => $firstName, + 'lastName' => $lastName, + 'password' => $password, + 'passwordConfirm' => $password, + 'instanceId' => $instanceId, + 'titlesBeforeName' => $titlesBeforeName, + 'titlesAfterName' => $titlesAfterName ] ); @@ -302,14 +302,14 @@ class TestRegistrationPresenter extends Tester\TestCase 'POST', ['action' => 'createAccount'], [ - 'email' => $email, - 'firstName' => $firstName, - 'lastName' => $lastName, - 'password' => $password, - 'passwordConfirm' => $passwordConfirm, - 'instanceId' => $instanceId, - 'titlesBeforeName' => $titlesBeforeName, - 'titlesAfterName' => $titlesAfterName + 'email' => $email, + 'firstName' => $firstName, + 'lastName' => $lastName, + 'password' => $password, + 'passwordConfirm' => $passwordConfirm, + 'instanceId' => $instanceId, + 'titlesBeforeName' => $titlesBeforeName, + 'titlesAfterName' => $titlesAfterName ] ); @@ -336,14 +336,14 @@ class TestRegistrationPresenter extends Tester\TestCase 'POST', ['action' => 'createAccount'], [ - 'email' => $email, - 'firstName' => $firstName, - 'lastName' => $lastName, - 'password' => $password, - 'passwordConfirm' => $password, - 'instanceId' => $instanceId, - 'titlesBeforeName' => $titlesBeforeName, - 'titlesAfterName' => $titlesAfterName + 'email' => $email, + 'firstName' => $firstName, + 'lastName' => $lastName, + 'password' => $password, + 'passwordConfirm' => $password, + 'instanceId' => $instanceId, + 'titlesBeforeName' => $titlesBeforeName, + 'titlesAfterName' => $titlesAfterName ] ); @@ -356,6 +356,93 @@ class TestRegistrationPresenter extends Tester\TestCase ); } + public function testCreateAccountSameName() + { + $student = $this->presenter->users->getByEmail(PresenterTestHelper::STUDENT_GROUP_MEMBER_LOGIN); + Assert::notNull($student); + + $email = "email@email.email"; + $firstName = $student->getFirstName(); + $lastName = $student->getLastName(); + $password = "password"; + $instances = $this->instances->findAll(); + $instanceId = array_pop($instances)->getId(); + $titlesBeforeName = "titlesBeforeName"; + $titlesAfterName = "titlesAfterName"; + + $payload = PresenterTestHelper::performPresenterRequest( + $this->presenter, + $this->presenterPath, + 'POST', + ['action' => 'createAccount'], + [ + 'email' => $email, + 'firstName' => $firstName, + 'lastName' => $lastName, + 'password' => $password, + 'passwordConfirm' => $password, + 'instanceId' => $instanceId, + 'titlesBeforeName' => $titlesBeforeName, + 'titlesAfterName' => $titlesAfterName + ] + ); + + Assert::equal(null, $payload["user"]); + Assert::count(1, $payload["usersWithSameName"]); + $user = current($payload["usersWithSameName"]); + Assert::equal($student->getId(), $user["id"]); + } + + public function testCreateAccountSameNameEnabled() + { + $student = $this->presenter->users->getByEmail(PresenterTestHelper::STUDENT_GROUP_MEMBER_LOGIN); + Assert::notNull($student); + + $email = "email@email.email"; + $firstName = $student->getFirstName(); + $lastName = $student->getLastName(); + $password = "password"; + $instances = $this->instances->findAll(); + $instanceId = array_pop($instances)->getId(); + $titlesBeforeName = "titlesBeforeName"; + $titlesAfterName = "titlesAfterName"; + + $request = new Nette\Application\Request( + $this->presenterPath, + 'POST', + ['action' => 'createAccount'], + [ + 'email' => $email, + 'firstName' => $firstName, + 'lastName' => $lastName, + 'password' => $password, + 'passwordConfirm' => $password, + 'instanceId' => $instanceId, + 'titlesBeforeName' => $titlesBeforeName, + 'titlesAfterName' => $titlesAfterName, + 'ignoreNameCollision' => true, + ] + ); + $response = $this->presenter->run($request); + Assert::type(Nette\Application\Responses\JsonResponse::class, $response); + + $result = $response->getPayload(); + Assert::equal(201, $result['code']); + Assert::equal(2, count($result['payload'])); + Assert::true(array_key_exists("accessToken", $result["payload"])); + Assert::true(array_key_exists("user", $result["payload"])); + + // check created user + $user = $result["payload"]["user"]; + Assert::equal("$titlesBeforeName $firstName $lastName $titlesAfterName", $user["fullName"]); + Assert::equal($email, $user["privateData"]["email"]); + + // check created login + $login = $this->logins->findByUserId($user["id"]); + Assert::equal($user["id"], $login->getUser()->getId()); + Assert::true($login->passwordsMatchOrEmpty($password, $this->presenter->passwordsService)); + } + public function testValidateRegistrationData() { $request = new Nette\Application\Request( @@ -363,8 +450,8 @@ class TestRegistrationPresenter extends Tester\TestCase 'POST', ['action' => 'validateRegistrationData'], [ - 'email' => "totallyFreeEmail@EmailFreeTotally.freeEmailTotally", - 'password' => "totallySecurePasswordWhichIsNot123456" + 'email' => "totallyFreeEmail@EmailFreeTotally.freeEmailTotally", + 'password' => "totallySecurePasswordWhichIsNot123456" ] ); $response = $this->presenter->run($request); @@ -511,10 +598,101 @@ class TestRegistrationPresenter extends Tester\TestCase ] ); }, - BadRequestException::class + ForbiddenRequestException::class ); } + public function testCreateInvitationWithSameName() + { + $student = $this->presenter->users->getByEmail(PresenterTestHelper::STUDENT_GROUP_MEMBER_LOGIN); + Assert::notNull($student); + + $email = "newguy@recodex.com"; + $firstName = $student->getFirstName(); + $lastName = $student->getLastName(); + $instances = $this->instances->findAll(); + $instanceId = array_pop($instances)->getId(); + $titlesBeforeName = "titlesBeforeName"; + $titlesAfterName = "titlesAfterName"; + + $groups = []; + foreach ($this->presenter->groups->findAll() as $group) { + if (!$group->isArchived() && !$group->isOrganizational()) { + $groups[] = $group->getId(); + } + } + Assert::truthy($groups); + + PresenterTestHelper::loginDefaultAdmin($this->container); + $payload = PresenterTestHelper::performPresenterRequest( + $this->presenter, + $this->presenterPath, + 'POST', + ['action' => 'createInvitation'], + [ + 'email' => $email, + 'firstName' => $firstName, + 'lastName' => $lastName, + 'titlesBeforeName' => $titlesBeforeName, + 'titlesAfterName' => $titlesAfterName, + 'instanceId' => $instanceId, + 'groups' => $groups, + 'locale' => 'en', + ] + ); + + Assert::count(1, $payload); + $user = current($payload); + Assert::equal($student->getId(), $user["id"]); + } + + public function testCreateInvitationWithSameNameEnabled() + { + $student = $this->presenter->users->getByEmail(PresenterTestHelper::STUDENT_GROUP_MEMBER_LOGIN); + Assert::notNull($student); + + $email = "newguy@recodex.com"; + $firstName = $student->getFirstName(); + $lastName = $student->getLastName(); + $instances = $this->instances->findAll(); + $instanceId = array_pop($instances)->getId(); + $titlesBeforeName = "titlesBeforeName"; + $titlesAfterName = "titlesAfterName"; + + $groups = []; + foreach ($this->presenter->groups->findAll() as $group) { + if (!$group->isArchived() && !$group->isOrganizational()) { + $groups[] = $group->getId(); + } + } + Assert::truthy($groups); + + $this->emailHelperMock->shouldReceive("send") + ->with("noreply@recodex", [$email], "en", 'User Admin Admin has invited you in ReCodEx!', Mockery::any()) + ->once()->andReturn(true); + + PresenterTestHelper::loginDefaultAdmin($this->container); + $payload = PresenterTestHelper::performPresenterRequest( + $this->presenter, + $this->presenterPath, + 'POST', + ['action' => 'createInvitation'], + [ + 'email' => $email, + 'firstName' => $firstName, + 'lastName' => $lastName, + 'titlesBeforeName' => $titlesBeforeName, + 'titlesAfterName' => $titlesAfterName, + 'instanceId' => $instanceId, + 'groups' => $groups, + 'locale' => 'en', + 'ignoreNameCollision' => true, + ] + ); + + Assert::equal("OK", $payload); + } + public function testAcceptInvitation() { $email = "newguy@recodex.com";