bug #22569 [Security] Handle bad request format in json auth listener…

… (ogizanagi)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[Security] Handle bad request format in json auth listener

| Q             | A
| ------------- | ---
| Branch?       | master (3.3)
| Bug fix?      | yesish
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | N/A
| License       | MIT
| Doc PR        | N/A

In #22034, I wondered myself if we shouldn't throw a dedicated exception to handle bad formatted requests and give more inputs to the client by returning a 400 response with an explicit message.

~~Here is a suggestion, introducing a new `BadRequestFormatException` and using it in `UsernamePasswordJsonAuthenticationListener` whenever there is no custom failure handler set (but someone using its own handler should be able to treat the failure properly too).~~

As discussed with @chalasr , it seems better to directly throw a `BadRequestHttpException` as it's actually out of the whole security process. PR updated.

Commits
-------

93a8cb9 [Security] Handle bad request format in json auth listener

src/Symfony/Component/Security/Http/Firewall/UsernamePasswordJsonAuthenticationListener.php

@@ -17,6 +17,7 @@
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpKernel\Event\GetResponseEvent;
+use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
 use Symfony\Component\PropertyAccess\Exception\AccessException;
 use Symfony\Component\PropertyAccess\PropertyAccess;
 use Symfony\Component\PropertyAccess\PropertyAccessorInterface;
@@ -83,31 +84,31 @@ public function handle(GetResponseEvent $event)
 
         try {
             if (!$data instanceof \stdClass) {
-                throw new BadCredentialsException('Invalid JSON.');
+                throw new BadRequestHttpException('Invalid JSON.');
             }
 
             try {
                 $username = $this->propertyAccessor->getValue($data, $this->options['username_path']);
             } catch (AccessException $e) {
-                throw new BadCredentialsException(sprintf('The key "%s" must be provided.', $this->options['username_path']));
+                throw new BadRequestHttpException(sprintf('The key "%s" must be provided.', $this->options['username_path']), $e);
             }
 
             try {
                 $password = $this->propertyAccessor->getValue($data, $this->options['password_path']);
             } catch (AccessException $e) {
-                throw new BadCredentialsException(sprintf('The key "%s" must be provided.', $this->options['password_path']));
+                throw new BadRequestHttpException(sprintf('The key "%s" must be provided.', $this->options['password_path']), $e);
             }
 
             if (!is_string($username)) {
-                throw new BadCredentialsException(sprintf('The key "%s" must be a string.', $this->options['username_path']));
+                throw new BadRequestHttpException(sprintf('The key "%s" must be a string.', $this->options['username_path']));
             }
 
             if (strlen($username) > Security::MAX_USERNAME_LENGTH) {
                 throw new BadCredentialsException('Invalid username.');
             }
 
             if (!is_string($password)) {
-                throw new BadCredentialsException(sprintf('The key "%s" must be a string.', $this->options['password_path']));
+                throw new BadRequestHttpException(sprintf('The key "%s" must be a string.', $this->options['password_path']));
             }
 
             $token = new UsernamePasswordToken($username, $password, $this->providerKey);

src/Symfony/Component/Security/Http/Tests/Firewall/UsernamePasswordJsonAuthenticationListenerTest.php

@@ -93,44 +93,69 @@ public function testUsePath()
         $this->assertEquals('ok', $event->getResponse()->getContent());
     }
 
+    /**
+     * @expectedException \Symfony\Component\HttpKernel\Exception\BadRequestHttpException
+     * @expectedExceptionMessage Invalid JSON
+     */
+    public function testAttemptAuthenticationNoJson()
+    {
+        $this->createListener();
+        $request = new Request();
+        $event = new GetResponseEvent($this->getMockBuilder(KernelInterface::class)->getMock(), $request, KernelInterface::MASTER_REQUEST);
+
+        $this->listener->handle($event);
+    }
+
+    /**
+     * @expectedException \Symfony\Component\HttpKernel\Exception\BadRequestHttpException
+     * @expectedExceptionMessage The key "username" must be provided
+     */
     public function testAttemptAuthenticationNoUsername()
     {
         $this->createListener();
         $request = new Request(array(), array(), array(), array(), array(), array(), '{"usr": "dunglas", "password": "foo"}');
         $event = new GetResponseEvent($this->getMockBuilder(KernelInterface::class)->getMock(), $request, KernelInterface::MASTER_REQUEST);
 
         $this->listener->handle($event);
-        $this->assertSame('ko', $event->getResponse()->getContent());
     }
 
+    /**
+     * @expectedException \Symfony\Component\HttpKernel\Exception\BadRequestHttpException
+     * @expectedExceptionMessage The key "password" must be provided
+     */
     public function testAttemptAuthenticationNoPassword()
     {
         $this->createListener();
         $request = new Request(array(), array(), array(), array(), array(), array(), '{"username": "dunglas", "pass": "foo"}');
         $event = new GetResponseEvent($this->getMockBuilder(KernelInterface::class)->getMock(), $request, KernelInterface::MASTER_REQUEST);
 
         $this->listener->handle($event);
-        $this->assertSame('ko', $event->getResponse()->getContent());
     }
 
+    /**
+     * @expectedException \Symfony\Component\HttpKernel\Exception\BadRequestHttpException
+     * @expectedExceptionMessage The key "username" must be a string.
+     */
     public function testAttemptAuthenticationUsernameNotAString()
     {
         $this->createListener();
         $request = new Request(array(), array(), array(), array(), array(), array(), '{"username": 1, "password": "foo"}');
         $event = new GetResponseEvent($this->getMockBuilder(KernelInterface::class)->getMock(), $request, KernelInterface::MASTER_REQUEST);
 
         $this->listener->handle($event);
-        $this->assertSame('ko', $event->getResponse()->getContent());
     }
 
+    /**
+     * @expectedException \Symfony\Component\HttpKernel\Exception\BadRequestHttpException
+     * @expectedExceptionMessage The key "password" must be a string.
+     */
     public function testAttemptAuthenticationPasswordNotAString()
     {
         $this->createListener();
         $request = new Request(array(), array(), array(), array(), array(), array(), '{"username": "dunglas", "password": 1}');
         $event = new GetResponseEvent($this->getMockBuilder(KernelInterface::class)->getMock(), $request, KernelInterface::MASTER_REQUEST);
 
         $this->listener->handle($event);
-        $this->assertSame('ko', $event->getResponse()->getContent());
     }
 
     public function testAttemptAuthenticationUsernameTooLong()