From 299e893ca90d17e5eb4f8a77c0d05ff4dc00090b Mon Sep 17 00:00:00 2001 From: Hugo Sales Date: Thu, 21 Oct 2021 14:45:18 +0100 Subject: [PATCH] [TOOLS][PHPStan][DocCheck] Fix errors found by PHPStan and Doc Check --- plugins/Cover/Controller/Cover.php | 1 + src/Controller/Security.php | 72 +++++++++---------- src/Core/Router/RouteLoader.php | 6 +- src/Core/Router/Router.php | 2 +- .../Compiler/SchemaDefDriver.php | 4 +- src/Entity/LocalUser.php | 45 ++++++------ src/Security/Authenticator.php | 37 +++++++--- src/Util/Form/ActorArrayTransformer.php | 5 +- src/Util/Form/ArrayTransformer.php | 12 ++++ tests/Entity/ActorTest.php | 6 -- tests/Entity/LocalUserTest.php | 7 +- tests/Util/NicknameTest.php | 2 - 12 files changed, 110 insertions(+), 89 deletions(-) diff --git a/plugins/Cover/Controller/Cover.php b/plugins/Cover/Controller/Cover.php index 764eccaca8..92d284bd85 100644 --- a/plugins/Cover/Controller/Cover.php +++ b/plugins/Cover/Controller/Cover.php @@ -152,5 +152,6 @@ class Cover // return new Response('Cover File not found',Response::HTTP_NOT_FOUND); // } // return GSFile::sendFile($cover->getFilePath(), $file->getMimetype(), $file->getTitle()); + return new Response('Cover File not found', Response::HTTP_NOT_FOUND); } } diff --git a/src/Controller/Security.php b/src/Controller/Security.php index eed9869179..c9d5cc6d0d 100644 --- a/src/Controller/Security.php +++ b/src/Controller/Security.php @@ -1,6 +1,6 @@ getLastUsername(); return [ - '_template' => 'security/login.html.twig', + '_template' => 'security/login.html.twig', 'last_login_id' => $last_login_id, - 'error' => $error, - 'notes_fn' => fn() => Note::getAllNotes(VisibilityScope::$instance_scope), + 'error' => $error, + 'notes_fn' => fn () => Note::getAllNotes(VisibilityScope::$instance_scope), ]; } @@ -76,50 +77,45 @@ class Security extends Controller * Register a user, making sure the nickname is not reserved and * possibly sending a confirmation email * - * @param Request $request - * @param GuardAuthenticatorHandler $guard_handler - * @param Authenticator $authenticator - * - * @return null|array|Response - * @throws EmailTakenException - * @throws NicknameTakenException - * @throws ServerException * @throws DuplicateFoundException * @throws EmailTakenException + * @throws EmailTakenException * @throws NicknameEmptyException + * @throws NicknameException * @throws NicknameInvalidException * @throws NicknameNotAllowedException * @throws NicknameTakenException * @throws NicknameTooLongException * @throws ServerException - * @throws NicknameException */ - public function register(Request $request, - GuardAuthenticatorHandler $guard_handler, - Authenticator $authenticator): array|Response|null + public function register( + Request $request, + GuardAuthenticatorHandler $guard_handler, + Authenticator $authenticator, + ): array|Response|null { $form = Form::create([ ['nickname', TextType::class, [ - 'label' => _m('Nickname'), - 'help' => _m('Your desired nickname (e.g., j0hnD03)'), + 'label' => _m('Nickname'), + 'help' => _m('Your desired nickname (e.g., j0hnD03)'), 'constraints' => [ new NotBlank(['message' => _m('Please enter a nickname')]), new Length([ - 'min' => 1, + 'min' => 1, 'minMessage' => _m(['Your nickname must be at least # characters long'], ['count' => 1]), - 'max' => Nickname::MAX_LEN, - 'maxMessage' => _m(['Your nickname must be at most # characters long'], ['count' => Nickname::MAX_LEN]),]), + 'max' => Nickname::MAX_LEN, + 'maxMessage' => _m(['Your nickname must be at most # characters long'], ['count' => Nickname::MAX_LEN]), ]), ], - 'block_name' => 'nickname', - 'label_attr' => ['class' => 'section-form-label'], + 'block_name' => 'nickname', + 'label_attr' => ['class' => 'section-form-label'], 'invalid_message' => _m('Nickname not valid. Please provide a valid nickname.'), ]], ['email', EmailType::class, [ - 'label' => _m('Email'), - 'help' => _m('Desired email for this account (e.g., john@provider.com)'), - 'constraints' => [new NotBlank(['message' => _m('Please enter an email')])], - 'block_name' => 'email', - 'label_attr' => ['class' => 'section-form-label'], + 'label' => _m('Email'), + 'help' => _m('Desired email for this account (e.g., john@provider.com)'), + 'constraints' => [new NotBlank(['message' => _m('Please enter an email')])], + 'block_name' => 'email', + 'label_attr' => ['class' => 'section-form-label'], 'invalid_message' => _m('Email not valid. Please provide a valid email.'), ]], FormFields::repeated_password(), @@ -129,7 +125,7 @@ class Security extends Controller $form->handleRequest($request); if ($form->isSubmitted() && $form->isValid()) { - $data = $form->getData(); + $data = $form->getData(); $data['password'] = $form->get('password')->getData(); // TODO: ensure there's no user with this email registered already @@ -140,17 +136,17 @@ class Security extends Controller try { // This already checks if the nickname is being used $actor = Actor::create(['nickname' => $sanitized_nickname]); - $user = LocalUser::create([ - 'nickname' => $sanitized_nickname, + $user = LocalUser::create([ + 'nickname' => $sanitized_nickname, 'outgoing_email' => $data['email'], 'incoming_email' => $data['email'], - 'password' => LocalUser::hashPassword($data['password']), + 'password' => LocalUser::hashPassword($data['password']), ]); DB::persistWithSameId( $actor, $user, // Self follow - fn(int $id) => DB::persist(Follow::create(['follower' => $id, 'followed' => $id])), + fn (int $id) => DB::persist(Follow::create(['follower' => $id, 'followed' => $id])), ); Event::handle('SuccessfulLocalUserRegistration', [$actor, $user]); @@ -169,7 +165,7 @@ class Security extends Controller if ($_ENV['APP_ENV'] !== 'dev' && Common::config('site', 'use_email')) { // @codeCoverageIgnoreStart EmailVerifier::sendEmailConfirmation($user); - // @codeCoverageIgnoreEnd + // @codeCoverageIgnoreEnd } else { $user->setIsEmailVerified(true); } @@ -183,9 +179,9 @@ class Security extends Controller } return [ - '_template' => 'security/register.html.twig', + '_template' => 'security/register.html.twig', 'registration_form' => $form->createView(), - 'notes_fn' => fn() => Note::getAllNotes(VisibilityScope::$instance_scope), + 'notes_fn' => fn () => Note::getAllNotes(VisibilityScope::$instance_scope), ]; } } diff --git a/src/Core/Router/RouteLoader.php b/src/Core/Router/RouteLoader.php index 48868dcbaf..584356df4c 100644 --- a/src/Core/Router/RouteLoader.php +++ b/src/Core/Router/RouteLoader.php @@ -103,12 +103,12 @@ class RouteLoader extends Loader // XXX: This hack can definitely be optimised by actually intersecting the arrays, // maybe this helps: https://backbeat.tech/blog/symfony-routing-tricks-part-1 // see: https://symfony.com/index.php/doc/3.1/components/http_foundation.html#accessing-accept-headers-data + $accept_header_condition = ''; if (isset($options['accept'])) { - $accept_header_condition = ''; foreach ($options['accept'] as $accept) { - $accept_header_condition .= "('$accept' in request.getAcceptableContentTypes()) ||"; + $accept_header_condition .= "('{$accept}' in request.getAcceptableContentTypes()) ||"; } - $accept_header_condition = substr($accept_header_condition, 0, -3); + $accept_header_condition = mb_substr($accept_header_condition, 0, -3); } $this->rc->add( diff --git a/src/Core/Router/Router.php b/src/Core/Router/Router.php index 133af1ffaa..b8a473fb74 100644 --- a/src/Core/Router/Router.php +++ b/src/Core/Router/Router.php @@ -82,7 +82,7 @@ abstract class Router * placeholder route values. Extra params are added as query * string to the URL */ - public static function url(string $id, array $args, int $type = self::ABSOLUTE_PATH): string + public static function url(string $id, array $args = [], int $type = self::ABSOLUTE_PATH): string { return self::$url_gen->generate($id, $args, $type); } diff --git a/src/DependencyInjection/Compiler/SchemaDefDriver.php b/src/DependencyInjection/Compiler/SchemaDefDriver.php index e27b192aa6..dfeaa4a601 100644 --- a/src/DependencyInjection/Compiler/SchemaDefDriver.php +++ b/src/DependencyInjection/Compiler/SchemaDefDriver.php @@ -92,7 +92,9 @@ class SchemaDefDriver extends StaticPHPDriver implements CompilerPassInterface /** * Fill in the database $metadata for $class_name - * @param ClassMetadataInfo $metadata ClassMetadataInfo is the real type, but we need to override the method + * + * @param class-string $class_name + * @param ClassMetadataInfo $metadata ClassMetadataInfo is the real type, but we need to override the method */ public function loadMetadataForClass($class_name, ClassMetadata $metadata) { diff --git a/src/Entity/LocalUser.php b/src/Entity/LocalUser.php index 3f71acfb0d..b27f8f9212 100644 --- a/src/Entity/LocalUser.php +++ b/src/Entity/LocalUser.php @@ -1,5 +1,7 @@ DB::findOneBy('local_user', ['nickname' => $nickname])); + } + /** - * Is the nickname or email already in use locally? - * - * @return self Returns self if nickname or email found + * @return self Returns self if email found */ public static function getByEmail(string $email): ?self { - $users = DB::findBy('local_user', ['or' => ['outgoing_email' => $email, 'incoming_email' => $email]]); - switch (count($users)) { - case 0: - return null; - case 1: - return $users[0]; - default: - // @codeCoverageIgnoreStart - throw new DuplicateFoundException('Multiple values in table local_user match the requested criteria'); - // @codeCoverageIgnoreEnd - } + return Cache::get("user-email-{$email}", fn () => DB::findOneBy('local_user', ['or' => ['outgoing_email' => $email, 'incoming_email' => $email]])); } /** @@ -332,9 +327,11 @@ class LocalUser extends Entity implements UserInterface // Timing safe password verification if (password_verify($password_plain_text, $this->password)) { // Update old formats - if (password_needs_rehash($this->password, - self::algoNameToConstant(Common::config('security', 'algorithm')), - Common::config('security', 'options')) + if (password_needs_rehash( + $this->password, + self::algoNameToConstant(Common::config('security', 'algorithm')), + Common::config('security', 'options'), + ) ) { // @codeCoverageIgnoreStart $this->changePassword(null, $password_plain_text, override: true); @@ -373,9 +370,9 @@ class LocalUser extends Entity implements UserInterface case 'argon2i': case 'argon2d': case 'argon2id': - $c = 'PASSWORD_' . strtoupper($algo); - if (defined($c)) { - return constant($c); + $c = 'PASSWORD_' . mb_strtoupper($algo); + if (\defined($c)) { + return \constant($c); } // fallthrough // no break diff --git a/src/Security/Authenticator.php b/src/Security/Authenticator.php index 4180435d53..fb713029cb 100644 --- a/src/Security/Authenticator.php +++ b/src/Security/Authenticator.php @@ -21,14 +21,14 @@ declare(strict_types = 1); namespace App\Security; -use App\Core\DB\DB; -use App\Util\Exception\NoSuchActorException; -use App\Util\Exception\NotFoundException; use function App\Core\I18n\_m; +use App\Core\Router\Router; use App\Entity\LocalUser; use App\Entity\User; +use App\Util\Exception\NoSuchActorException; use App\Util\Nickname; use Exception; +use Stringable; use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\Routing\Generator\UrlGeneratorInterface; @@ -36,6 +36,7 @@ use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Core\Exception\CustomUserMessageAuthenticationException; use Symfony\Component\Security\Core\Exception\InvalidCsrfTokenException; use Symfony\Component\Security\Core\Security; +use Symfony\Component\Security\Core\User\UserInterface; use Symfony\Component\Security\Core\User\UserProviderInterface; use Symfony\Component\Security\Csrf\CsrfToken; use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface; @@ -75,12 +76,15 @@ class Authenticator extends AbstractFormLoginAuthenticator public function getCredentials(Request $request) { return [ - 'nickname_or_email' => $request->request->get('nickname_or_email'), - 'password' => $request->request->get('password'), - 'csrf_token' => $request->request->get('_csrf_token'), + 'nickname_or_email' => $request->request->get('nickname_or_email'), + 'password' => $request->request->get('password'), + 'csrf_token' => $request->request->get('_csrf_token'), ]; } + /** + * Get a user given credentials and a CSRF token + */ public function getUser($credentials, UserProviderInterface $userProvider) { $token = new CsrfToken('authenticate', $credentials['csrf_token']); @@ -89,10 +93,10 @@ class Authenticator extends AbstractFormLoginAuthenticator } try { - if (filter_var($credentials['nickname_or_email'], FILTER_VALIDATE_EMAIL) !== false) { + if (filter_var($credentials['nickname_or_email'], \FILTER_VALIDATE_EMAIL) !== false) { $user = LocalUser::getByEmail($credentials['nickname_or_email']); } else { - $user = LocalUser::getWithPK(['nickname' => Nickname::normalize($credentials['nickname_or_email'], check_already_used: false, which: Nickname::CHECK_LOCAL_USER, check_is_allowed: false)]); + $user = LocalUser::getByNickname(Nickname::normalize($credentials['nickname_or_email'], check_already_used: false, which: Nickname::CHECK_LOCAL_USER, check_is_allowed: false)); } if ($user === null) { throw new NoSuchActorException('No such local user.'); @@ -119,21 +123,32 @@ class Authenticator extends AbstractFormLoginAuthenticator } } + /** + * After a successful login, redirect user to the path saved in their session or to the root of the website + */ public function onAuthenticationSuccess(Request $request, TokenInterface $token, $providerKey) { + $nickname = $token->getUser(); + if ($nickname instanceof Stringable) { + $nickname = (string) $nickname; + } elseif ($nickname instanceof UserInterface) { + $nickname = $nickname->getUsername(); + } + $request->getSession()->set( Security::LAST_USERNAME, - $token->getUser()->getNickname() + $nickname, ); + if ($targetPath = $this->getTargetPath($request->getSession(), $providerKey)) { return new RedirectResponse($targetPath); } - return new RedirectResponse($this->urlGenerator->generate('main_all')); + return new RedirectResponse(Router::url('/')); } protected function getLoginUrl() { - return $this->urlGenerator->generate(self::LOGIN_ROUTE); + return Router::url(self::LOGIN_ROUTE); } } diff --git a/src/Util/Form/ActorArrayTransformer.php b/src/Util/Form/ActorArrayTransformer.php index d7cea71e7b..2c6391ee6b 100644 --- a/src/Util/Form/ActorArrayTransformer.php +++ b/src/Util/Form/ActorArrayTransformer.php @@ -34,7 +34,7 @@ declare(strict_types = 1); namespace App\Util\Form; -use App\Entity\Actor; +use App\Entity\LocalUser; class ActorArrayTransformer extends ArrayTransformer { @@ -56,8 +56,9 @@ class ActorArrayTransformer extends ArrayTransformer */ public function reverseTransform($s) { + // TODO support full mentions return array_map( - fn ($nickmame) => Actor::getFromNickname($nickmame), + fn ($nickmame) => LocalUser::getByNickname($nickmame), parent::reverseTransform($s), ); } diff --git a/src/Util/Form/ArrayTransformer.php b/src/Util/Form/ArrayTransformer.php index 97b99c4250..58c04d6684 100644 --- a/src/Util/Form/ArrayTransformer.php +++ b/src/Util/Form/ArrayTransformer.php @@ -40,6 +40,11 @@ class ArrayTransformer implements DataTransformerInterface { // Can't use type annotations, to conform to interface + /** + * @param array $a + * + * @return string + */ public function transform($a) { if (!\is_array($a)) { @@ -48,6 +53,13 @@ class ArrayTransformer implements DataTransformerInterface return Formatting::toString($a, Formatting::SPLIT_BY_SPACE); } + /** + * Transform a string to an array + * + * @param string $s + * + * @return array + */ public function reverseTransform($s) { if (empty($s)) { diff --git a/tests/Entity/ActorTest.php b/tests/Entity/ActorTest.php index 946508f402..3670a0f955 100644 --- a/tests/Entity/ActorTest.php +++ b/tests/Entity/ActorTest.php @@ -22,7 +22,6 @@ declare(strict_types = 1); namespace App\Tests\Entity; use App\Core\DB\DB; -use App\Entity\Actor; use App\Entity\ActorTag; use App\Util\GNUsocialTestCase; use Functional as F; @@ -38,11 +37,6 @@ class ActorTest extends GNUsocialTestCase static::assertSame("/{$actor->getId()}/avatar", $actor->getAvatarUrl()); } - public function testGetFromNickname() - { - static::assertNotNull(Actor::getFromNickname('taken_user')); - } - public function testSelfTags() { $actor = DB::findOneBy('actor', ['nickname' => 'taken_user']); diff --git a/tests/Entity/LocalUserTest.php b/tests/Entity/LocalUserTest.php index 67d5248949..560f3b90d3 100644 --- a/tests/Entity/LocalUserTest.php +++ b/tests/Entity/LocalUserTest.php @@ -50,9 +50,14 @@ class LocalUserTest extends GNUsocialTestCase public function testChangePassword() { parent::bootKernel(); - $user = LocalUser::findByNicknameOrEmail('form_personal_info_test_user', 'some@email'); + $user = LocalUser::getByNickname('form_personal_info_test_user'); static::assertTrue($user->changePassword(old_password_plain_text: '', new_password_plain_text: 'password', override: true)); static::assertTrue($user->changePassword(old_password_plain_text: 'password', new_password_plain_text: 'new_password', override: false)); static::assertFalse($user->changePassword(old_password_plain_text: 'wrong_password', new_password_plain_text: 'new_password', override: false)); } + + public function testGetByNickname() + { + static::assertNotNull(LocalUser::getByNickname('taken_user')); + } } diff --git a/tests/Util/NicknameTest.php b/tests/Util/NicknameTest.php index 831f570d9d..a20348722e 100644 --- a/tests/Util/NicknameTest.php +++ b/tests/Util/NicknameTest.php @@ -27,7 +27,6 @@ use App\Util\Exception\NicknameInvalidException; use App\Util\Exception\NicknameNotAllowedException; use App\Util\Exception\NicknameTakenException; use App\Util\Exception\NicknameTooLongException; -use App\Util\Exception\NicknameTooShortException; use App\Util\GNUsocialTestCase; use App\Util\Nickname; use Jchook\AssertThrows\AssertThrows; @@ -52,7 +51,6 @@ class NicknameTest extends GNUsocialTestCase static::assertSame('foobar', Nickname::normalize(' foobar ', check_already_used: false)); // static::assertSame('foobar', Nickname::normalize('foo_bar', check_already_used: false)); // static::assertSame('foobar', Nickname::normalize('FooBar', check_already_used: false)); - static::assertThrows(NicknameTooShortException::class, fn () => Nickname::normalize('foo', check_already_used: false)); static::assertThrows(NicknameEmptyException::class, fn () => Nickname::normalize('', check_already_used: false)); // static::assertThrows(NicknameInvalidException::class, fn () => Nickname::normalize('FóóBár', check_already_used: false)); static::assertThrows(NicknameNotAllowedException::class, fn () => Nickname::normalize('this_nickname_is_reserved', check_already_used: false));