[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

This commit is contained in:
Hugo Sales 2021-11-14 23:15:24 +00:00
parent bf5ffe7d3d
commit eab6de3609
Signed by untrusted user: someonewithpc
GPG Key ID: 7D0C7EAFC9D835A0
7 changed files with 62 additions and 43 deletions

View File

@ -26,6 +26,7 @@ use App\Util\Exception\NicknameInvalidException;
use App\Util\Exception\NicknameNotAllowedException; use App\Util\Exception\NicknameNotAllowedException;
use App\Util\Exception\NicknameTakenException; use App\Util\Exception\NicknameTakenException;
use App\Util\Exception\NicknameTooLongException; use App\Util\Exception\NicknameTooLongException;
use App\Util\Exception\NotFoundException;
use App\Util\Exception\ServerException; use App\Util\Exception\ServerException;
use App\Util\Form\FormFields; use App\Util\Form\FormFields;
use App\Util\Nickname; use App\Util\Nickname;
@ -126,10 +127,24 @@ class Security extends Controller
$data = $form->getData(); $data = $form->getData();
$data['password'] = $form->get('password')->getData(); $data['password'] = $form->get('password')->getData();
// TODO: ensure there's no user with this email registered already
// Already used is checked below // 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 { try {
// This already checks if the nickname is being used // This already checks if the nickname is being used
@ -153,9 +168,9 @@ class Security extends Controller
// @codeCoverageIgnoreStart // @codeCoverageIgnoreStart
} catch (UniqueConstraintViolationException $e) { } catch (UniqueConstraintViolationException $e) {
// _something_ was duplicated, but since we already check if nickname is in use, we can't tell what went wrong // _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'; $m = 'An error occurred while trying to register';
Log::critical($e . " with nickname: '{$sanitized_nickname}' and email '{$data['email']}'"); Log::critical($m . " with nickname: '{$sanitized_nickname}' and email '{$data['email']}'");
throw new ServerException(_m($e)); throw new ServerException(_m($m), previous: $e);
} }
// @codeCoverageIgnoreEnd // @codeCoverageIgnoreEnd

View File

@ -362,7 +362,8 @@ class LocalUser extends Entity implements UserInterface
*/ */
public static function getByEmail(string $email): ?self 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 public static function schemaDef(): array
@ -371,22 +372,22 @@ class LocalUser extends Entity implements UserInterface
'name' => 'local_user', 'name' => 'local_user',
'description' => 'local users, bots, etc', 'description' => 'local users, bots, etc',
'fields' => [ 'fields' => [
'id' => ['type' => 'int', 'foreign key' => true, 'target' => 'Actor.id', 'multiplicity' => 'one to one', 'not null' => true, 'description' => 'foreign key to actor table'], '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'], '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'], '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.'], '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'], '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'], 'is_email_verified' => ['type' => 'bool', 'default' => false, 'description' => 'Whether the user opened the comfirmation email'],
'timezone' => ['type' => 'varchar', 'length' => 50, 'description' => 'timezone'], 'timezone' => ['type' => 'varchar', 'length' => 50, 'description' => 'timezone'],
'phone_number' => ['type' => 'phone_number', 'description' => 'phone number'], '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_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)'], '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'], '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'], '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'], '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'], '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'], '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'], 'modified' => ['type' => 'timestamp', 'not null' => true, 'default' => 'CURRENT_TIMESTAMP', 'description' => 'date this record was modified'],
], ],
'primary key' => ['id'], 'primary key' => ['id'],
'unique keys' => [ 'unique keys' => [

View File

@ -25,8 +25,10 @@ use function App\Core\I18n\_m;
use App\Core\Router\Router; use App\Core\Router\Router;
use App\Entity\LocalUser; use App\Entity\LocalUser;
use App\Entity\User; use App\Entity\User;
use App\Util\Common;
use App\Util\Exception\NoSuchActorException; use App\Util\Exception\NoSuchActorException;
use App\Util\Exception\NotFoundException; use App\Util\Exception\NotFoundException;
use App\Util\Nickname;
use Stringable; use Stringable;
use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\RedirectResponse;
use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Request;
@ -99,9 +101,9 @@ class Authenticator extends AbstractFormLoginAuthenticator
} }
$user = null; $user = null;
try { 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']); $user = LocalUser::getByEmail($credentials['nickname_or_email']);
} else { } elseif (Nickname::isValid($credentials['nickname_or_email'])) {
$user = LocalUser::getByNickname($credentials['nickname_or_email']); $user = LocalUser::getByNickname($credentials['nickname_or_email']);
} }
if ($user === null) { if ($user === null) {

View File

@ -39,6 +39,8 @@ use App\Core\Security;
use App\Entity\Actor; use App\Entity\Actor;
use App\Entity\LocalUser; use App\Entity\LocalUser;
use App\Util\Exception\NoLoggedInUser; use App\Util\Exception\NoLoggedInUser;
use Egulias\EmailValidator\EmailValidator;
use Egulias\EmailValidator\Validation\RFCValidation as RFCEmailValidation;
use Functional as F; use Functional as F;
use Symfony\Component\DependencyInjection\ParameterBag\ContainerBagInterface; use Symfony\Component\DependencyInjection\ParameterBag\ContainerBagInterface;
use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Request;
@ -293,6 +295,11 @@ abstract class Common
&& preg_match($regex, parse_url($url, \PHP_URL_SCHEME)); && 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 * Flatten an array of ['note' => note, 'replies' => [notes]] to an array of notes
*/ */

View File

@ -39,11 +39,11 @@ use App\Entity\Actor;
class EmailTakenException extends EmailException 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); parent::__construct($msg, $code);
} }

View File

@ -48,11 +48,11 @@ use App\Entity\Actor;
class NicknameTakenException extends NicknameException 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); parent::__construct($msg, $code);
} }

View File

@ -66,9 +66,8 @@ class SecurityTest extends GNUsocialTestCase
{ {
self::testLogin('taken_user', 'wrong password'); self::testLogin('taken_user', 'wrong password');
$this->assertResponseIsSuccessful(); $this->assertResponseIsSuccessful();
// TODO(eliseu) Login page doesn't have this error $this->assertSelectorTextContains('.alert', 'Invalid login credentials');
// $this->assertSelectorTextContains('.alert', 'Invalid login credentials'); $this->assertRouteSame('security_login');
$this->assertRouteSame('login');
} }
public function testLoginEmail() public function testLoginEmail()
@ -120,7 +119,7 @@ class SecurityTest extends GNUsocialTestCase
]); ]);
$this->assertSelectorTextContains('form[name=register] ul li', 'The password fields must match'); $this->assertSelectorTextContains('form[name=register] ul li', 'The password fields must match');
$this->assertResponseStatusCodeSame(200); $this->assertResponseStatusCodeSame(200);
$this->assertRouteSame('register'); $this->assertRouteSame('security_register');
} }
private function testRegisterPasswordLength(string $password, string $error) private function testRegisterPasswordLength(string $password, string $error)
@ -128,7 +127,7 @@ class SecurityTest extends GNUsocialTestCase
self::testRegister('new_nickname', 'email@provider', $password); self::testRegister('new_nickname', 'email@provider', $password);
$this->assertResponseIsSuccessful(); $this->assertResponseIsSuccessful();
$this->assertSelectorTextContains('.help-block > ul > li', $error); $this->assertSelectorTextContains('.help-block > ul > li', $error);
$this->assertRouteSame('register'); $this->assertRouteSame('security_register');
} }
public function testRegisterPasswordEmpty() public function testRegisterPasswordEmpty()
@ -151,7 +150,7 @@ class SecurityTest extends GNUsocialTestCase
self::testRegister('new_nickname', '', 'foobar'); self::testRegister('new_nickname', '', 'foobar');
$this->assertResponseIsSuccessful(); $this->assertResponseIsSuccessful();
$this->assertSelectorTextContains('.help-block > ul > li', 'Please enter an email'); $this->assertSelectorTextContains('.help-block > ul > li', 'Please enter an email');
$this->assertRouteSame('register'); $this->assertRouteSame('security_register');
} }
private function testRegisterNicknameLength(string $nickname, string $error) private function testRegisterNicknameLength(string $nickname, string $error)
@ -159,7 +158,7 @@ class SecurityTest extends GNUsocialTestCase
self::testRegister($nickname, 'email@provider', 'foobar'); self::testRegister($nickname, 'email@provider', 'foobar');
$this->assertResponseIsSuccessful(); $this->assertResponseIsSuccessful();
$this->assertSelectorTextContains('.help-block > ul > li', $error); $this->assertSelectorTextContains('.help-block > ul > li', $error);
$this->assertRouteSame('register'); $this->assertRouteSame('security_register');
} }
public function testRegisterNicknameEmpty() public function testRegisterNicknameEmpty()
@ -167,11 +166,6 @@ class SecurityTest extends GNUsocialTestCase
self::testRegisterNicknameLength('', error: 'Please enter a nickname'); self::testRegisterNicknameLength('', error: 'Please enter a nickname');
} }
public function testRegisterNicknameShort()
{
self::testRegisterNicknameLength('f', error: 'Your nickname must be at least');
}
public function testRegisterNicknameLong() public function testRegisterNicknameLong()
{ {
self::testRegisterNicknameLength(str_repeat('f', 128), error: 'Your nickname must be at most'); self::testRegisterNicknameLength(str_repeat('f', 128), error: 'Your nickname must be at most');