Avoid Username/Email is passed as clear GET Parameter to CheckEmailAc…

…tion on password reset process (#1695)

Co-authored-by: BA-JBI <[email protected]>

src/Action/CheckEmailAction.php

@@ -15,32 +15,98 @@
 
 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;
 
 final class CheckEmailAction
 {
+    private Pool $adminPool;
+    private TemplateRegistryInterface $templateRegistry;
+    private int $tokenTtl;
+
+    /**
+     * NEXT_MAJOR: Remove `$tokenTtlDeprecated` argument and only allow first types of arguments.
+     */
     public function __construct(
         private Environment $twig,
-        private UrlGeneratorInterface $urlGenerator,
-        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,
+                    $templateRegistry::class
+                ));
+            }
+            $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, int given.',
+                    __METHOD__,
+                    TemplateRegistryInterface::class,
+                ));
+            }
+            $this->templateRegistry = $tokenTtl;
+
+            if (!\is_int($tokenTtlDeprecated)) {
+                throw new \TypeError(\sprintf(
+                    'Argument 5 passed to %s() must be type of %s, %s given.',
+                    __METHOD__,
+                    'integer',
+                    \gettype($tokenTtlDeprecated)
+                ));
+            }
+            $this->tokenTtl = $tokenTtlDeprecated;
 
-        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,
+                    $templateRegistry::class
+                ));
+            }
+            $this->templateRegistry = $templateRegistry;
+
+            if (!\is_int($tokenTtl)) {
+                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) {
+                throw new \TypeError(\sprintf(
+                    'Argument 5 passed to %s() must be %s, %s given.',
+                    __METHOD__,
+                    'NULL',
+                    \gettype($tokenTtlDeprecated)
+                ));
+            }
         }
+    }
 
+    public function __invoke(): Response
+    {
         return new Response($this->twig->render('@SonataUser/Admin/Security/Resetting/checkEmail.html.twig', [
             'base_template' => $this->templateRegistry->getTemplate('layout'),
             'admin_pool' => $this->adminPool,

src/Action/RequestAction.php

@@ -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', [

src/DependencyInjection/SonataUserExtension.php

@@ -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']);

src/Resources/config/actions_resetting.php

@@ -39,7 +39,6 @@
         ->public()
         ->args([
             service('twig'),
-            service('router'),
             service('sonata.admin.pool'),
             service('sonata.admin.global_template_registry'),
             abstract_arg('token ttl'),

tests/Action/CheckEmailActionTest.php

@@ -19,9 +19,6 @@
 use Sonata\AdminBundle\Templating\TemplateRegistryInterface;
 use Sonata\UserBundle\Action\CheckEmailAction;
 use Symfony\Component\DependencyInjection\Container;
-use Symfony\Component\HttpFoundation\RedirectResponse;
-use Symfony\Component\HttpFoundation\Request;
-use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
 use Twig\Environment;
 
 final class CheckEmailActionTest extends TestCase
@@ -31,11 +28,6 @@ final class CheckEmailActionTest extends TestCase
      */
     private MockObject $templating;
 
-    /**
-     * @var MockObject&UrlGeneratorInterface
-     */
-    private MockObject $urlGenerator;
-
     private Pool $pool;
 
     /**
@@ -48,32 +40,13 @@ final class CheckEmailActionTest extends TestCase
     protected function setUp(): void
     {
         $this->templating = $this->createMock(Environment::class);
-        $this->urlGenerator = $this->createMock(UrlGeneratorInterface::class);
         $this->pool = new Pool(new Container());
         $this->templateRegistry = $this->createMock(TemplateRegistryInterface::class);
         $this->resetTtl = 60;
     }
 
-    public function testWithoutUsername(): void
-    {
-        $request = new Request();
-
-        $this->urlGenerator->expects(static::once())
-            ->method('generate')
-            ->with('sonata_user_admin_resetting_request')
-            ->willReturn('/foo');
-
-        $action = $this->getAction();
-        $result = $action($request);
-
-        static::assertInstanceOf(RedirectResponse::class, $result);
-        static::assertSame('/foo', $result->getTargetUrl());
-    }
-
     public function testWithUsername(): void
     {
-        $request = new Request(['username' => 'bar']);
-
         $parameters = [
             'base_template' => 'base.html.twig',
             'admin_pool' => $this->pool,
@@ -91,13 +64,13 @@ public function testWithUsername(): void
             ->willReturn('base.html.twig');
 
         $action = $this->getAction();
-        $result = $action($request);
+        $result = $action();
 
         static::assertSame('template content', $result->getContent());
     }
 
     private function getAction(): CheckEmailAction
     {
-        return new CheckEmailAction($this->templating, $this->urlGenerator, $this->pool, $this->templateRegistry, $this->resetTtl);
+        return new CheckEmailAction($this->templating, $this->pool, $this->templateRegistry, $this->resetTtl);
     }
 }

tests/Action/RequestActionTest.php

@@ -238,7 +238,7 @@ public function testEmailSent(): void
 
         $this->urlGenerator->method('generate')->with(
             'sonata_user_admin_resetting_check_email',
-            ['username' => 'bar'],
+            [],
         )->willReturn('/check-email');
 
         $action = $this->getAction();