From eab6de3609f6867029683e67e8e0164fad163348 Mon Sep 17 00:00:00 2001 From: Hugo Sales Date: Sun, 14 Nov 2021 23:15:24 +0000 Subject: [PATCH] [TESTS][Security] Fix SecurityTest. Remove nickname normalization on register (a plugin can handle that). Move from filter_var(FILTER_VALIDATE_EMAIL) as it does not support dotless domains --- src/Controller/Security.php | 27 ++++++++++---- src/Entity/LocalUser.php | 35 ++++++++++--------- src/Security/Authenticator.php | 6 ++-- src/Util/Common.php | 7 ++++ src/Util/Exception/EmailTakenException.php | 6 ++-- src/Util/Exception/NicknameTakenException.php | 6 ++-- tests/Controller/SecurityTest.php | 18 ++++------ 7 files changed, 62 insertions(+), 43 deletions(-) diff --git a/src/Controller/Security.php b/src/Controller/Security.php index 0384e12fef..9ab8cff257 100644 --- a/src/Controller/Security.php +++ b/src/Controller/Security.php @@ -26,6 +26,7 @@ use App\Util\Exception\NicknameInvalidException; use App\Util\Exception\NicknameNotAllowedException; use App\Util\Exception\NicknameTakenException; use App\Util\Exception\NicknameTooLongException; +use App\Util\Exception\NotFoundException; use App\Util\Exception\ServerException; use App\Util\Form\FormFields; use App\Util\Nickname; @@ -126,10 +127,24 @@ class Security extends Controller $data = $form->getData(); $data['password'] = $form->get('password')->getData(); - // TODO: ensure there's no user with this email registered already - // Already used is checked below - $sanitized_nickname = Nickname::normalize($data['nickname'], check_already_used: false, which: Nickname::CHECK_LOCAL_USER, check_is_allowed: false); + $sanitized_nickname = null; + if (Event::handle('SanitizeNickname', [$data['nickname'], &$sanitized_nickname]) != Event::stop) { + $sanitized_nickname = $data['nickname']; + // $sanitized_nickname = Nickname::normalize($data['nickname'], check_already_used: false, which: Nickname::CHECK_LOCAL_USER, check_is_allowed: false); + } + + try { + $found_user = DB::findOneBy('local_user', ['or' => ['nickname' => $sanitized_nickname, 'outgoing_email' => $data['email']]]); + if ($found_user->getNickname() === $sanitized_nickname) { + throw new NicknameTakenException($found_user->getActor()); + } elseif ($found_user->getOutgoingEmail() === $data['email']) { + throw new EmailTakenException($found_user->getActor()); + } + unset($found_user); + } catch (NotFoundException) { + // continue + } try { // This already checks if the nickname is being used @@ -153,9 +168,9 @@ class Security extends Controller // @codeCoverageIgnoreStart } catch (UniqueConstraintViolationException $e) { // _something_ was duplicated, but since we already check if nickname is in use, we can't tell what went wrong - $e = 'An error occurred while trying to register'; - Log::critical($e . " with nickname: '{$sanitized_nickname}' and email '{$data['email']}'"); - throw new ServerException(_m($e)); + $m = 'An error occurred while trying to register'; + Log::critical($m . " with nickname: '{$sanitized_nickname}' and email '{$data['email']}'"); + throw new ServerException(_m($m), previous: $e); } // @codeCoverageIgnoreEnd diff --git a/src/Entity/LocalUser.php b/src/Entity/LocalUser.php index 8ca851a0bf..f208eabb0e 100644 --- a/src/Entity/LocalUser.php +++ b/src/Entity/LocalUser.php @@ -362,7 +362,8 @@ class LocalUser extends Entity implements UserInterface */ public static function getByEmail(string $email): ?self { - return Cache::get("user-email-{$email}", fn () => DB::findOneBy('local_user', ['or' => ['outgoing_email' => $email, 'incoming_email' => $email]])); + $key = str_replace('@', '-', $email); + return Cache::get("user-email-{$key}", fn () => DB::findOneBy('local_user', ['or' => ['outgoing_email' => $email, 'incoming_email' => $email]])); } public static function schemaDef(): array @@ -371,22 +372,22 @@ class LocalUser extends Entity implements UserInterface 'name' => 'local_user', 'description' => 'local users, bots, etc', 'fields' => [ - 'id' => ['type' => 'int', 'foreign key' => true, 'target' => 'Actor.id', 'multiplicity' => 'one to one', 'not null' => true, 'description' => 'foreign key to actor table'], - 'nickname' => ['type' => 'varchar', 'not null' => true, 'length' => 64, 'description' => 'nickname or username, foreign key to actor'], - 'password' => ['type' => 'varchar', 'length' => 191, 'description' => 'salted password, can be null for users with federated authentication'], - 'outgoing_email' => ['type' => 'varchar', 'length' => 191, 'description' => 'email address for password recovery, notifications, etc.'], - 'incoming_email' => ['type' => 'varchar', 'length' => 191, 'description' => 'email address for post-by-email'], - 'is_email_verified' => ['type' => 'bool', 'default' => false, 'description' => 'Whether the user opened the comfirmation email'], - 'timezone' => ['type' => 'varchar', 'length' => 50, 'description' => 'timezone'], - 'phone_number' => ['type' => 'phone_number', 'description' => 'phone number'], - 'sms_carrier' => ['type' => 'int', 'foreign key' => true, 'target' => 'SmsCarrier.id', 'multiplicity' => 'one to one', 'description' => 'foreign key to sms_carrier'], - 'sms_email' => ['type' => 'varchar', 'length' => 191, 'description' => 'built from sms and carrier (see sms_carrier)'], - 'uri' => ['type' => 'varchar', 'length' => 191, 'description' => 'universally unique identifier, usually a tag URI'], - 'auto_subscribe_back' => ['type' => 'bool', 'default' => false, 'description' => 'automatically subscribe to users who subscribed us'], - 'subscription_policy' => ['type' => 'int', 'size' => 'tiny', 'default' => 0, 'description' => '0 = anybody can subscribe; 1 = require approval'], - 'is_stream_private' => ['type' => 'bool', 'default' => false, 'description' => 'whether to limit all notices to subscribers only'], - 'created' => ['type' => 'datetime', 'not null' => true, 'default' => 'CURRENT_TIMESTAMP', 'description' => 'date this record was created'], - 'modified' => ['type' => 'timestamp', 'not null' => true, 'default' => 'CURRENT_TIMESTAMP', 'description' => 'date this record was modified'], + 'id' => ['type' => 'int', 'foreign key' => true, 'target' => 'Actor.id', 'multiplicity' => 'one to one', 'not null' => true, 'description' => 'foreign key to actor table'], + 'nickname' => ['type' => 'varchar', 'not null' => true, 'length' => 64, 'description' => 'nickname or username, foreign key to actor'], + 'password' => ['type' => 'varchar', 'length' => 191, 'description' => 'salted password, can be null for users with federated authentication'], + 'outgoing_email' => ['type' => 'varchar', 'length' => 191, 'description' => 'email address for password recovery, notifications, etc.'], + 'incoming_email' => ['type' => 'varchar', 'length' => 191, 'description' => 'email address for post-by-email'], + 'is_email_verified' => ['type' => 'bool', 'default' => false, 'description' => 'Whether the user opened the comfirmation email'], + 'timezone' => ['type' => 'varchar', 'length' => 50, 'description' => 'timezone'], + 'phone_number' => ['type' => 'phone_number', 'description' => 'phone number'], + 'sms_carrier' => ['type' => 'int', 'foreign key' => true, 'target' => 'SmsCarrier.id', 'multiplicity' => 'one to one', 'description' => 'foreign key to sms_carrier'], + 'sms_email' => ['type' => 'varchar', 'length' => 191, 'description' => 'built from sms and carrier (see sms_carrier)'], + 'uri' => ['type' => 'varchar', 'length' => 191, 'description' => 'universally unique identifier, usually a tag URI'], + 'auto_subscribe_back' => ['type' => 'bool', 'default' => false, 'description' => 'automatically subscribe to users who subscribed us'], + 'subscription_policy' => ['type' => 'int', 'size' => 'tiny', 'default' => 0, 'description' => '0 = anybody can subscribe; 1 = require approval'], + 'is_stream_private' => ['type' => 'bool', 'default' => false, 'description' => 'whether to limit all notices to subscribers only'], + 'created' => ['type' => 'datetime', 'not null' => true, 'default' => 'CURRENT_TIMESTAMP', 'description' => 'date this record was created'], + 'modified' => ['type' => 'timestamp', 'not null' => true, 'default' => 'CURRENT_TIMESTAMP', 'description' => 'date this record was modified'], ], 'primary key' => ['id'], 'unique keys' => [ diff --git a/src/Security/Authenticator.php b/src/Security/Authenticator.php index 701c28da06..45ae780f4c 100644 --- a/src/Security/Authenticator.php +++ b/src/Security/Authenticator.php @@ -25,8 +25,10 @@ use function App\Core\I18n\_m; use App\Core\Router\Router; use App\Entity\LocalUser; use App\Entity\User; +use App\Util\Common; use App\Util\Exception\NoSuchActorException; use App\Util\Exception\NotFoundException; +use App\Util\Nickname; use Stringable; use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\Request; @@ -99,9 +101,9 @@ class Authenticator extends AbstractFormLoginAuthenticator } $user = null; try { - if (filter_var($credentials['nickname_or_email'], \FILTER_VALIDATE_EMAIL) !== false) { + if (Common::isValidEmail($credentials['nickname_or_email'])) { $user = LocalUser::getByEmail($credentials['nickname_or_email']); - } else { + } elseif (Nickname::isValid($credentials['nickname_or_email'])) { $user = LocalUser::getByNickname($credentials['nickname_or_email']); } if ($user === null) { diff --git a/src/Util/Common.php b/src/Util/Common.php index 92344f3f3d..1f1bd41afd 100644 --- a/src/Util/Common.php +++ b/src/Util/Common.php @@ -39,6 +39,8 @@ use App\Core\Security; use App\Entity\Actor; use App\Entity\LocalUser; use App\Util\Exception\NoLoggedInUser; +use Egulias\EmailValidator\EmailValidator; +use Egulias\EmailValidator\Validation\RFCValidation as RFCEmailValidation; use Functional as F; use Symfony\Component\DependencyInjection\ParameterBag\ContainerBagInterface; use Symfony\Component\HttpFoundation\Request; @@ -293,6 +295,11 @@ abstract class Common && preg_match($regex, parse_url($url, \PHP_URL_SCHEME)); } + public static function isValidEmail(string $email): bool + { + return (new EmailValidator())->isValid($email, new RFCEmailValidation()); + } + /** * Flatten an array of ['note' => note, 'replies' => [notes]] to an array of notes */ diff --git a/src/Util/Exception/EmailTakenException.php b/src/Util/Exception/EmailTakenException.php index 7d64fe4250..864aab306a 100644 --- a/src/Util/Exception/EmailTakenException.php +++ b/src/Util/Exception/EmailTakenException.php @@ -39,11 +39,11 @@ use App\Entity\Actor; class EmailTakenException extends EmailException { - public ?Actor $profile = null; // the Actor which occupies the email + public ?Actor $actor = null; // the Actor which occupies the email - public function __construct(?Actor $profile = null, ?string $msg = null, int $code = 400) + public function __construct(?Actor $actor = null, ?string $msg = null, int $code = 400) { - $this->profile = $profile; + $this->profile = $actor; parent::__construct($msg, $code); } diff --git a/src/Util/Exception/NicknameTakenException.php b/src/Util/Exception/NicknameTakenException.php index f62ec55817..c45c51dcdd 100644 --- a/src/Util/Exception/NicknameTakenException.php +++ b/src/Util/Exception/NicknameTakenException.php @@ -48,11 +48,11 @@ use App\Entity\Actor; class NicknameTakenException extends NicknameException { - public ?Actor $profile = null; // the Actor which occupies the nickname + public ?Actor $actor = null; // the Actor which occupies the nickname - public function __construct(?Actor $profile = null, ?string $msg = null, int $code = 400) + public function __construct(?Actor $actor = null, ?string $msg = null, int $code = 400) { - $this->profile = $profile; + $this->profile = $actor; parent::__construct($msg, $code); } diff --git a/tests/Controller/SecurityTest.php b/tests/Controller/SecurityTest.php index b5744037a9..d48dbdb069 100644 --- a/tests/Controller/SecurityTest.php +++ b/tests/Controller/SecurityTest.php @@ -66,9 +66,8 @@ class SecurityTest extends GNUsocialTestCase { self::testLogin('taken_user', 'wrong password'); $this->assertResponseIsSuccessful(); - // TODO(eliseu) Login page doesn't have this error - // $this->assertSelectorTextContains('.alert', 'Invalid login credentials'); - $this->assertRouteSame('login'); + $this->assertSelectorTextContains('.alert', 'Invalid login credentials'); + $this->assertRouteSame('security_login'); } public function testLoginEmail() @@ -120,7 +119,7 @@ class SecurityTest extends GNUsocialTestCase ]); $this->assertSelectorTextContains('form[name=register] ul li', 'The password fields must match'); $this->assertResponseStatusCodeSame(200); - $this->assertRouteSame('register'); + $this->assertRouteSame('security_register'); } private function testRegisterPasswordLength(string $password, string $error) @@ -128,7 +127,7 @@ class SecurityTest extends GNUsocialTestCase self::testRegister('new_nickname', 'email@provider', $password); $this->assertResponseIsSuccessful(); $this->assertSelectorTextContains('.help-block > ul > li', $error); - $this->assertRouteSame('register'); + $this->assertRouteSame('security_register'); } public function testRegisterPasswordEmpty() @@ -151,7 +150,7 @@ class SecurityTest extends GNUsocialTestCase self::testRegister('new_nickname', '', 'foobar'); $this->assertResponseIsSuccessful(); $this->assertSelectorTextContains('.help-block > ul > li', 'Please enter an email'); - $this->assertRouteSame('register'); + $this->assertRouteSame('security_register'); } private function testRegisterNicknameLength(string $nickname, string $error) @@ -159,7 +158,7 @@ class SecurityTest extends GNUsocialTestCase self::testRegister($nickname, 'email@provider', 'foobar'); $this->assertResponseIsSuccessful(); $this->assertSelectorTextContains('.help-block > ul > li', $error); - $this->assertRouteSame('register'); + $this->assertRouteSame('security_register'); } public function testRegisterNicknameEmpty() @@ -167,11 +166,6 @@ class SecurityTest extends GNUsocialTestCase self::testRegisterNicknameLength('', error: 'Please enter a nickname'); } - public function testRegisterNicknameShort() - { - self::testRegisterNicknameLength('f', error: 'Your nickname must be at least'); - } - public function testRegisterNicknameLong() { self::testRegisterNicknameLength(str_repeat('f', 128), error: 'Your nickname must be at most');