bug #22034 [Security] json auth listener should not produce a 500 response on bad request format (ogizanagi)
This PR was merged into the 3.3-dev branch.
Discussion
----------
[Security] json auth listener should not produce a 500 response on bad request format
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | N/A
| License | MIT
| Doc PR | N/A
To me, it looks wrong to simply throw a `BadCredentialsException` in the wild, which produces a 500 (unless an entrypoint handles it, which you probably don't have on a json login firewall). There isn't any server error, the client request originated the error due to a wrong format.
Instead, the listener should give a chance to the failure handler to resolve it, and return a proper 4XX response. (BTW, the `UsernamePasswordFormAuthenticationListener` also throws a similar `BadCredentialsException` on a too long submitted username, which is caught and forwarded to the failure handler)
Better diff: https://github.com/symfony/symfony/pull/22034/files?w=1
BTW, should we have another exception type like `BadCredentialsFormatException` or whatever in order to distinct a proper `BadCredentialsException` from a format issue in a failure listener?
Commits
-------
cb175a41c3
[Security] json auth listener should not produce a 500 response on bad request format
This commit is contained in:
commit
9761b44aa4
@ -70,35 +70,35 @@ class UsernamePasswordJsonAuthenticationListener implements ListenerInterface
|
|||||||
$request = $event->getRequest();
|
$request = $event->getRequest();
|
||||||
$data = json_decode($request->getContent());
|
$data = json_decode($request->getContent());
|
||||||
|
|
||||||
if (!$data instanceof \stdClass) {
|
|
||||||
throw new BadCredentialsException('Invalid JSON.');
|
|
||||||
}
|
|
||||||
|
|
||||||
try {
|
try {
|
||||||
$username = $this->propertyAccessor->getValue($data, $this->options['username_path']);
|
if (!$data instanceof \stdClass) {
|
||||||
} catch (AccessException $e) {
|
throw new BadCredentialsException('Invalid JSON.');
|
||||||
throw new BadCredentialsException(sprintf('The key "%s" must be provided.', $this->options['username_path']));
|
}
|
||||||
}
|
|
||||||
|
|
||||||
try {
|
try {
|
||||||
$password = $this->propertyAccessor->getValue($data, $this->options['password_path']);
|
$username = $this->propertyAccessor->getValue($data, $this->options['username_path']);
|
||||||
} catch (AccessException $e) {
|
} catch (AccessException $e) {
|
||||||
throw new BadCredentialsException(sprintf('The key "%s" must be provided.', $this->options['password_path']));
|
throw new BadCredentialsException(sprintf('The key "%s" must be provided.', $this->options['username_path']));
|
||||||
}
|
}
|
||||||
|
|
||||||
if (!is_string($username)) {
|
try {
|
||||||
throw new BadCredentialsException(sprintf('The key "%s" must be a string.', $this->options['username_path']));
|
$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']));
|
||||||
|
}
|
||||||
|
|
||||||
if (strlen($username) > Security::MAX_USERNAME_LENGTH) {
|
if (!is_string($username)) {
|
||||||
throw new BadCredentialsException('Invalid username.');
|
throw new BadCredentialsException(sprintf('The key "%s" must be a string.', $this->options['username_path']));
|
||||||
}
|
}
|
||||||
|
|
||||||
if (!is_string($password)) {
|
if (strlen($username) > Security::MAX_USERNAME_LENGTH) {
|
||||||
throw new BadCredentialsException(sprintf('The key "%s" must be a string.', $this->options['password_path']));
|
throw new BadCredentialsException('Invalid username.');
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (!is_string($password)) {
|
||||||
|
throw new BadCredentialsException(sprintf('The key "%s" must be a string.', $this->options['password_path']));
|
||||||
|
}
|
||||||
|
|
||||||
try {
|
|
||||||
$token = new UsernamePasswordToken($username, $password, $this->providerKey);
|
$token = new UsernamePasswordToken($username, $password, $this->providerKey);
|
||||||
|
|
||||||
$authenticatedToken = $this->authenticationManager->authenticate($token);
|
$authenticatedToken = $this->authenticationManager->authenticate($token);
|
||||||
|
@ -86,9 +86,6 @@ class UsernamePasswordJsonAuthenticationListenerTest extends TestCase
|
|||||||
$this->assertEquals('ok', $event->getResponse()->getContent());
|
$this->assertEquals('ok', $event->getResponse()->getContent());
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* @expectedException \Symfony\Component\Security\Core\Exception\BadCredentialsException
|
|
||||||
*/
|
|
||||||
public function testAttemptAuthenticationNoUsername()
|
public function testAttemptAuthenticationNoUsername()
|
||||||
{
|
{
|
||||||
$this->createListener();
|
$this->createListener();
|
||||||
@ -96,11 +93,9 @@ class UsernamePasswordJsonAuthenticationListenerTest extends TestCase
|
|||||||
$event = new GetResponseEvent($this->getMockBuilder(KernelInterface::class)->getMock(), $request, KernelInterface::MASTER_REQUEST);
|
$event = new GetResponseEvent($this->getMockBuilder(KernelInterface::class)->getMock(), $request, KernelInterface::MASTER_REQUEST);
|
||||||
|
|
||||||
$this->listener->handle($event);
|
$this->listener->handle($event);
|
||||||
|
$this->assertSame('ko', $event->getResponse()->getContent());
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* @expectedException \Symfony\Component\Security\Core\Exception\BadCredentialsException
|
|
||||||
*/
|
|
||||||
public function testAttemptAuthenticationNoPassword()
|
public function testAttemptAuthenticationNoPassword()
|
||||||
{
|
{
|
||||||
$this->createListener();
|
$this->createListener();
|
||||||
@ -108,11 +103,9 @@ class UsernamePasswordJsonAuthenticationListenerTest extends TestCase
|
|||||||
$event = new GetResponseEvent($this->getMockBuilder(KernelInterface::class)->getMock(), $request, KernelInterface::MASTER_REQUEST);
|
$event = new GetResponseEvent($this->getMockBuilder(KernelInterface::class)->getMock(), $request, KernelInterface::MASTER_REQUEST);
|
||||||
|
|
||||||
$this->listener->handle($event);
|
$this->listener->handle($event);
|
||||||
|
$this->assertSame('ko', $event->getResponse()->getContent());
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* @expectedException \Symfony\Component\Security\Core\Exception\BadCredentialsException
|
|
||||||
*/
|
|
||||||
public function testAttemptAuthenticationUsernameNotAString()
|
public function testAttemptAuthenticationUsernameNotAString()
|
||||||
{
|
{
|
||||||
$this->createListener();
|
$this->createListener();
|
||||||
@ -120,11 +113,9 @@ class UsernamePasswordJsonAuthenticationListenerTest extends TestCase
|
|||||||
$event = new GetResponseEvent($this->getMockBuilder(KernelInterface::class)->getMock(), $request, KernelInterface::MASTER_REQUEST);
|
$event = new GetResponseEvent($this->getMockBuilder(KernelInterface::class)->getMock(), $request, KernelInterface::MASTER_REQUEST);
|
||||||
|
|
||||||
$this->listener->handle($event);
|
$this->listener->handle($event);
|
||||||
|
$this->assertSame('ko', $event->getResponse()->getContent());
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* @expectedException \Symfony\Component\Security\Core\Exception\BadCredentialsException
|
|
||||||
*/
|
|
||||||
public function testAttemptAuthenticationPasswordNotAString()
|
public function testAttemptAuthenticationPasswordNotAString()
|
||||||
{
|
{
|
||||||
$this->createListener();
|
$this->createListener();
|
||||||
@ -132,11 +123,9 @@ class UsernamePasswordJsonAuthenticationListenerTest extends TestCase
|
|||||||
$event = new GetResponseEvent($this->getMockBuilder(KernelInterface::class)->getMock(), $request, KernelInterface::MASTER_REQUEST);
|
$event = new GetResponseEvent($this->getMockBuilder(KernelInterface::class)->getMock(), $request, KernelInterface::MASTER_REQUEST);
|
||||||
|
|
||||||
$this->listener->handle($event);
|
$this->listener->handle($event);
|
||||||
|
$this->assertSame('ko', $event->getResponse()->getContent());
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* @expectedException \Symfony\Component\Security\Core\Exception\BadCredentialsException
|
|
||||||
*/
|
|
||||||
public function testAttemptAuthenticationUsernameTooLong()
|
public function testAttemptAuthenticationUsernameTooLong()
|
||||||
{
|
{
|
||||||
$this->createListener();
|
$this->createListener();
|
||||||
@ -145,5 +134,6 @@ class UsernamePasswordJsonAuthenticationListenerTest extends TestCase
|
|||||||
$event = new GetResponseEvent($this->getMockBuilder(KernelInterface::class)->getMock(), $request, KernelInterface::MASTER_REQUEST);
|
$event = new GetResponseEvent($this->getMockBuilder(KernelInterface::class)->getMock(), $request, KernelInterface::MASTER_REQUEST);
|
||||||
|
|
||||||
$this->listener->handle($event);
|
$this->listener->handle($event);
|
||||||
|
$this->assertSame('ko', $event->getResponse()->getContent());
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
Reference in New Issue
Block a user