Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid Username/Email is passed as clear GET Parameter to CheckEmailAction on password reset process #1693

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 88 additions & 13 deletions src/Action/CheckEmailAction.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,32 +15,107 @@

use Sonata\AdminBundle\Admin\Pool;
use Sonata\AdminBundle\Templating\TemplateRegistryInterface;
use Symfony\Component\HttpFoundation\RedirectResponse;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
use Twig\Environment;
use TypeError;

final class CheckEmailAction
{

private Pool $adminPool;
private TemplateRegistryInterface $templateRegistry;
private int $tokenTtl;

/**
* NEXT_MAJOR: Remove `$tokenTtlDeprecated` argument and only allow first types of arguments.
*
* @param Environment $twig
* @param Pool $adminPool
* @param TemplateRegistryInterface $templateRegistry
* @param int $tokenTtl
* @param null $tokenTtlDeprecated
*/
public function __construct(
private Environment $twig,
private UrlGeneratorInterface $urlGenerator,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a BC break.

If someone does new CheckEmailAction($twig, $urlGenerator, $adminPool, ...) it will now crash.

We have to keep a BC signature. You can get inspiration from https://github.com/sonata-project/SonataUserBundle/blob/4.x/src/GoogleAuthenticator/RequestListener.php#L58-L88

public function __construct(
    private Environment $twig,
    Pool|UrlGeneratorInterface $adminPool,
    TemplateRegistryInterface|Pool $templateRegistry,
    int|TemplateRegistryInterface $tokenTtl,
    ?int $tokenTtl = null,
) {
    if ($adminPool instanceof UrlGeneratorInterface) {
            if (!$templateRegistry instanceof Pool) {
                throw new TypeError(...);
            }
            $this->adminPool = $templateRegistry;

            @trigger_error(sprintf(
                'Passing an instance of %s as argument 2 to "%s()" is deprecated since sonata-project/user-bundle 5.x and will only accept an instance of %s in version 6.0.',
                UrlGeneratorInterface::class,
                __METHOD__,
                Pool::class
            ), \E_USER_DEPRECATED);
        } else {
            $this->adminPool = $adminPool;
        }
        
        // And so on
}

private Pool $adminPool,
private TemplateRegistryInterface $templateRegistry,
private int $tokenTtl
Pool|UrlGeneratorInterface $adminPool,
TemplateRegistryInterface|Pool $templateRegistry,
int|TemplateRegistryInterface $tokenTtl,
?int $tokenTtlDeprecated = null,
) {
}
// NEXT_MAJOR: Remove all checks and use constructor property promotion instead
if ($adminPool instanceof UrlGeneratorInterface) {
if (!$templateRegistry instanceof Pool) {
throw new \TypeError(sprintf(
'Argument 3 passed to %s() must be an instance of %s, %s given.',
__METHOD__,
Pool::class,
\get_class($templateRegistry)
));
}
$this->adminPool = $templateRegistry;

public function __invoke(Request $request): Response
{
$username = $request->query->get('username');
if (!$tokenTtl instanceof TemplateRegistryInterface) {
throw new \TypeError(sprintf(
'Argument 4 passed to %s() must be an instance of %s, %s given.',
__METHOD__,
TemplateRegistryInterface::class,
\get_class($tokenTtl)

Check failure on line 63 in src/Action/CheckEmailAction.php

View workflow job for this annotation

GitHub Actions / PHPStan

Parameter #1 $object of function get_class expects object, int given.

Check failure on line 63 in src/Action/CheckEmailAction.php

View workflow job for this annotation

GitHub Actions / Psalm

InvalidArgument

src/Action/CheckEmailAction.php:63:32: InvalidArgument: Argument 1 of get_class expects object, but int provided (see https://psalm.dev/004)
));
}
$this->templateRegistry = $tokenTtl;

if (!is_int($tokenTtlDeprecated)) {

Check failure on line 68 in src/Action/CheckEmailAction.php

View workflow job for this annotation

GitHub Actions / Psalm

RedundantCondition

src/Action/CheckEmailAction.php:68:18: RedundantCondition: Type null for $tokenTtlDeprecated is never int (see https://psalm.dev/122)
throw new \TypeError(sprintf(
'Argument 5 passed to %s() must be type of %s, %s given.',
__METHOD__,
'integer',
\gettype($tokenTtlDeprecated)
));
}
$this->tokenTtl = $tokenTtlDeprecated;

Check failure on line 76 in src/Action/CheckEmailAction.php

View workflow job for this annotation

GitHub Actions / Psalm

NoValue

src/Action/CheckEmailAction.php:76:13: NoValue: All possible types for this assignment were invalidated - This may be dead code (see https://psalm.dev/179)

if (null === $username) {
// the user does not come from the sendEmail action
return new RedirectResponse($this->urlGenerator->generate('sonata_user_admin_resetting_request'));
@trigger_error(sprintf(
'Passing an instance of %s as argument 2 to "%s()" is deprecated since sonata-project/user-bundle 5.x and will only accept an instance of %s in version 6.0.',
UrlGeneratorInterface::class,
__METHOD__,
Pool::class
), \E_USER_DEPRECATED);
} else {
$this->adminPool = $adminPool;
if (!$templateRegistry instanceof TemplateRegistryInterface) {
throw new \TypeError(sprintf(
'Argument 3 passed to %s() must be an instance of %s, %s given.',
__METHOD__,
TemplateRegistryInterface::class,
\get_class($templateRegistry)
));
}
$this->templateRegistry = $templateRegistry;

if (!is_int($tokenTtl)) {

Check failure on line 96 in src/Action/CheckEmailAction.php

View workflow job for this annotation

GitHub Actions / Psalm

TypeDoesNotContainType

src/Action/CheckEmailAction.php:96:18: TypeDoesNotContainType: Type int for $tokenTtl is always !int (see https://psalm.dev/056)
throw new \TypeError(sprintf(
'Argument 4 passed to %s() must be type of %s, %s given.',
__METHOD__,
'integer',
\gettype($tokenTtl)
));
}
$this->tokenTtl = $tokenTtl;

if (null !== $tokenTtlDeprecated) {

Check failure on line 106 in src/Action/CheckEmailAction.php

View workflow job for this annotation

GitHub Actions / Psalm

TypeDoesNotContainType

src/Action/CheckEmailAction.php:106:17: TypeDoesNotContainType: Type null for $tokenTtlDeprecated is always !null (see https://psalm.dev/056)
throw new \TypeError(sprintf(
'Argument 5 passed to %s() must be %s, %s given.',
__METHOD__,
'NULL',
\gettype($tokenTtlDeprecated)
));
}
}
}

public function __invoke(): Response
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For BC, the Request $request param probably needs to be optional.
(with Deprecate Warning)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I don't think so.

There won't be any error if you pass an extra param

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, i misunderstand the errors there,

it was only Psalm/PHPStan complaining, not a real PHP error.

{
return new Response($this->twig->render('@SonataUser/Admin/Security/Resetting/checkEmail.html.twig', [
'base_template' => $this->templateRegistry->getTemplate('layout'),
'admin_pool' => $this->adminPool,
Expand Down
4 changes: 1 addition & 3 deletions src/Action/RequestAction.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,7 @@ public function __invoke(Request $request): Response
$this->userManager->save($user);
}

return new RedirectResponse($this->urlGenerator->generate('sonata_user_admin_resetting_check_email', [
'username' => $username,
]));
return new RedirectResponse($this->urlGenerator->generate('sonata_user_admin_resetting_check_email'));
}

return new Response($this->twig->render('@SonataUser/Admin/Security/Resetting/request.html.twig', [
Expand Down
2 changes: 1 addition & 1 deletion src/DependencyInjection/SonataUserExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ private function configureResetting(array $config, ContainerBuilder $container):
->replaceArgument(9, $config['retry_ttl']);

$container->getDefinition('sonata.user.action.check_email')
->replaceArgument(4, $config['token_ttl']);
->replaceArgument(3, $config['token_ttl']);

$container->getDefinition('sonata.user.action.reset')
->replaceArgument(8, $config['token_ttl']);
Expand Down
1 change: 0 additions & 1 deletion src/Resources/config/actions_resetting.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
->public()
->args([
service('twig'),
service('router'),
service('sonata.admin.pool'),
service('sonata.admin.global_template_registry'),
abstract_arg('token ttl'),
Expand Down
Loading