Skip to content

Commit d1679b9

Browse files
committed
minor #2193 make sure tests fail without access denied listener (lsmith77)
This PR was merged into the 2.x branch. Discussion ---------- make sure tests fail without access denied listener core already seems to do all the right things when using basic auth and/or json_login. so at least for 3.0, imho we should just remove the AccessDeniedListener and tell people to use `json_login` or adapt whatever custom solution they have. however when using a custom guard authenticator it does not work properly out of the box unless one enables the the AccessDeniedListener and only with the removed `$exception` propagation done in #2109 that being said an alternative and likely cleaner approach in place of the AccessDeniedListener would be to offer a default implementation for a failure handler. so maybe for 3.0 this is what should be done? ie. drop AccessDeniedListener and add a failure handler? then we avoid even going into the exception handling process and directly return the appropriate response. and unrelated issue I noticed is if I enable the `exception_listener` in the tests (ie. set `true` here https://github.com/FriendsOfSymfony/FOSRestBundle/blob/fix-access-denied-tests/Tests/Functional/app/AccessDeniedListener/config.yml#L14), then the explode with a circular dependency in the `AbstractNormalizer`: ``` Symfony\Component\Serializer\Exception\CircularReferenceException: A circular reference has been detected when serializing the object of class "Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException" (configured limit: 1). /FOSRestBundle/vendor/symfony/serializer/Normalizer/AbstractNormalizer.php:330 ... ``` Commits ------- d1a7d7d make sure tests fail without access denied listener
2 parents 65bf5a3 + d1a7d7d commit d1679b9

File tree

7 files changed

+175
-66
lines changed

7 files changed

+175
-66
lines changed

Tests/Functional/AccessDeniedListenerTest.php

Lines changed: 24 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,10 @@ class AccessDeniedListenerTest extends WebTestCase
1919

2020
public static function setUpBeforeClass()
2121
{
22+
if (!interface_exists(ErrorRendererInterface::class)) {
23+
self::markTestSkipped();
24+
}
25+
2226
parent::setUpBeforeClass();
2327
static::$client = static::createClient(['test_case' => 'AccessDeniedListener']);
2428
}
@@ -29,65 +33,39 @@ public static function tearDownAfterClass()
2933
parent::tearDownAfterClass();
3034
}
3135

32-
protected function setUp()
33-
{
34-
if (!interface_exists(ErrorRendererInterface::class)) {
35-
$this->markTestSkipped();
36-
}
37-
}
38-
39-
public function testBundleListenerHandlesExceptionsInRestZonesWithoutLogin()
36+
public function testNoCredentialsGives403()
4037
{
41-
static::$client->request('GET', '/api/comments');
38+
static::$client->request('POST', '/api/login', [], [], ['CONTENT_TYPE' => 'application/json']);
39+
$response = static::$client->getResponse();
4240

43-
$this->assertEquals(401, static::$client->getResponse()->getStatusCode());
44-
$this->assertEquals('application/json', static::$client->getResponse()->headers->get('Content-Type'));
41+
$this->assertEquals(403, $response->getStatusCode());
42+
$this->assertEquals('application/json', $response->headers->get('Content-Type'));
4543
}
4644

47-
public function testBundleListenerHandlesExceptionsInRestZonesWithLogin()
45+
public function testWrongLoginGives401()
4846
{
49-
$credentials = [
50-
'PHP_AUTH_USER' => 'restapi',
51-
'PHP_AUTH_PW' => 'secretpw',
52-
];
47+
static::$client->request('POST', '/api/login', [], [], ['HTTP_X-FOO' => 'BAR', 'CONTENT_TYPE' => 'application/json']);
48+
$response = static::$client->getResponse();
5349

54-
static::$client->request('GET', '/api/comments', [], [], $credentials);
55-
56-
$this->assertEquals(200, static::$client->getResponse()->getStatusCode());
57-
$this->assertEquals('application/json', static::$client->getResponse()->headers->get('Content-Type'));
50+
$this->assertEquals(401, $response->getStatusCode());
51+
$this->assertEquals('application/json', $response->headers->get('Content-Type'));
5852
}
5953

60-
public function testBundleListenerHandlesExceptionsInRestZonesWrongLogin()
54+
public function testSuccessfulLogin()
6155
{
62-
$credentials = [
63-
'PHP_AUTH_USER' => 'admin',
64-
'PHP_AUTH_PW' => 'secretpw',
65-
];
66-
67-
static::$client->request('GET', '/api/comments', [], [], $credentials);
68-
69-
$this->assertEquals(403, static::$client->getResponse()->getStatusCode());
70-
$this->assertEquals('application/json', static::$client->getResponse()->headers->get('Content-Type'));
71-
}
72-
73-
public function testBundleListenerHandlesExceptionsInRestZonesWithIncorrectLogin()
74-
{
75-
$credentials = [
76-
'PHP_AUTH_USER' => 'restapi',
77-
'PHP_AUTH_PW' => 'foobar',
78-
];
79-
80-
static::$client->request('GET', '/api/comments', [], [], $credentials);
56+
static::$client->request('POST', '/api/login', [], [], ['HTTP_X-FOO' => 'FOOBAR', 'CONTENT_TYPE' => 'application/json']);
57+
$response = static::$client->getResponse();
8158

82-
$this->assertEquals(401, static::$client->getResponse()->getStatusCode());
83-
$this->assertEquals('application/json', static::$client->getResponse()->headers->get('Content-Type'));
59+
$this->assertEquals(200, $response->getStatusCode());
60+
$this->assertEquals('application/json', $response->headers->get('Content-Type'));
8461
}
8562

86-
public function testSymfonyListenerHandlesExceptionsOutsideRestZones()
63+
public function testAccessDeniedExceptionGives403()
8764
{
88-
static::$client->request('GET', '/admin/comments');
65+
static::$client->request('GET', '/api/comments', [], [], ['CONTENT_TYPE' => 'application/json']);
66+
$response = static::$client->getResponse();
8967

90-
$this->assertEquals(302, static::$client->getResponse()->getStatusCode());
91-
$this->assertEquals('text/html; charset=UTF-8', static::$client->getResponse()->headers->get('Content-Type'));
68+
$this->assertEquals(403, $response->getStatusCode());
69+
$this->assertEquals('application/json', $response->headers->get('Content-Type'));
9270
}
9371
}

Tests/Functional/Bundle/TestBundle/Controller/Api/CommentController.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,11 @@
1515

1616
class CommentController
1717
{
18+
public function loginAction()
19+
{
20+
return new JsonResponse('login');
21+
}
22+
1823
public function getCommentAction($id)
1924
{
2025
return new JsonResponse(array('id' => (int) $id));
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
<?php
2+
3+
namespace FOS\RestBundle\Tests\Functional\Bundle\TestBundle\Entity;
4+
5+
use Symfony\Component\Security\Core\User\UserInterface;
6+
7+
class User implements UserInterface
8+
{
9+
const ROLE_DEFAULT = 'ROLE_USER';
10+
11+
public $username;
12+
public $roles = [];
13+
14+
public function getRoles(): array
15+
{
16+
$roles = $this->roles;
17+
18+
$roles[] = static::ROLE_DEFAULT;
19+
20+
return array_unique($roles);
21+
}
22+
23+
public function getUsername(): string
24+
{
25+
return $this->username;
26+
}
27+
28+
public function getPassword(): string
29+
{
30+
return '';
31+
}
32+
33+
public function getSalt(): ?string
34+
{
35+
return null;
36+
}
37+
38+
public function eraseCredentials(): void
39+
{
40+
}
41+
}
Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
<?php
2+
3+
namespace FOS\RestBundle\Tests\Functional\Bundle\TestBundle\Security;
4+
5+
use FOS\RestBundle\Tests\Functional\Bundle\TestBundle\Entity\User;
6+
use Symfony\Component\HttpFoundation\Request;
7+
use Symfony\Component\HttpFoundation\JsonResponse;
8+
use Symfony\Component\HttpFoundation\Response;
9+
use Symfony\Component\Security\Core\User\UserInterface;
10+
use Symfony\Component\Security\Guard\AbstractGuardAuthenticator;
11+
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
12+
use Symfony\Component\Security\Core\Exception\AuthenticationException;
13+
use Symfony\Component\Security\Core\User\UserProviderInterface;
14+
15+
class ApiTokenAuthenticator extends AbstractGuardAuthenticator
16+
{
17+
protected $headerName = 'x-foo';
18+
protected $tokenValue = 'FOOBAR';
19+
20+
/**
21+
* Called on every request. Return whatever credentials you want,
22+
* or null to stop authentication.
23+
*/
24+
public function getCredentials(Request $request): ?string
25+
{
26+
if (!$request->headers->has($this->headerName)) {
27+
return null;
28+
}
29+
30+
return $request->headers->get($this->headerName);
31+
}
32+
33+
public function getUser($credentials, UserProviderInterface $userProvider): ?UserInterface
34+
{
35+
if (!$credentials || $credentials !== $this->tokenValue) {
36+
return null;
37+
}
38+
39+
$user = new User();
40+
$user->username = 'foo';
41+
$user->roles[] = 'ROLE_API';
42+
43+
return $user;
44+
}
45+
46+
public function checkCredentials($credentials, UserInterface $user): bool
47+
{
48+
return true;
49+
}
50+
51+
public function onAuthenticationSuccess(Request $request, TokenInterface $token, $providerKey): ?Response
52+
{
53+
return null;
54+
}
55+
56+
public function onAuthenticationFailure(Request $request, AuthenticationException $exception): ?Response
57+
{
58+
throw new AuthenticationException('Token not valid');
59+
}
60+
61+
/**
62+
* Called when authentication is needed, but it's not sent.
63+
*/
64+
public function start(Request $request, AuthenticationException $authException = null): Response
65+
{
66+
$data = ['message' => 'Authentication Required'];
67+
68+
return new JsonResponse($data, Response::HTTP_UNAUTHORIZED);
69+
}
70+
71+
public function supportsRememberMe(): bool
72+
{
73+
return false;
74+
}
75+
76+
/**
77+
* Does the authenticator support the given Request?
78+
*
79+
* If this returns false, the authenticator will be skipped.
80+
*/
81+
public function supports(Request $request): bool
82+
{
83+
if (!$request->headers->has($this->headerName)) {
84+
return false;
85+
}
86+
87+
return true;
88+
}
89+
}

Tests/Functional/app/AccessDeniedListener/config.yml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,3 +17,7 @@ fos_rest:
1717
body_listener: false
1818
zone:
1919
- { path: ^/api/* }
20+
21+
services:
22+
api_token_authenticator:
23+
class: FOS\RestBundle\Tests\Functional\Bundle\TestBundle\Security\ApiTokenAuthenticator

Tests/Functional/app/AccessDeniedListener/routing.yml

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,8 @@ api:
44
_controller: FOS\RestBundle\Tests\Functional\Bundle\TestBundle\Controller\Api\CommentController::getComments
55
_format: json
66

7-
admin:
8-
path: /admin/comments
7+
api_login:
8+
path: /api/login
99
defaults:
10-
_controller: FOS\RestBundle\Tests\Functional\Bundle\TestBundle\Controller\CommentController::getComments
11-
_format: html
10+
_controller: FOS\RestBundle\Tests\Functional\Bundle\TestBundle\Controller\Api\CommentController::loginAction
11+
_format: json

Tests/Functional/app/AccessDeniedListener/security.php

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -21,30 +21,22 @@
2121
'encoders' => ['Symfony\Component\Security\Core\User\User' => 'plaintext'],
2222
'providers' => [
2323
'in_memory' => [
24-
'memory' => [
25-
'users' => [
26-
'restapi' => ['password' => 'secretpw', 'roles' => ['ROLE_API']],
27-
'admin' => ['password' => 'secretpw', 'roles' => ['ROLE_ADMIN']],
28-
],
29-
],
24+
'memory' => [],
3025
],
3126
],
3227
'firewalls' => [
33-
'api' => array_merge($defaultFirewall, [
34-
'pattern' => '^/api',
28+
'default' => array_merge($defaultFirewall, [
29+
'provider' => 'in_memory',
30+
'anonymous' => 'lazy',
3531
'stateless' => true,
36-
'http_basic' => ['realm' => 'Demo REST API'],
37-
'json_login' => [
38-
'check_path' => '/api/login',
32+
'guard' => [
33+
'authenticators' => [
34+
'api_token_authenticator',
35+
],
3936
],
4037
]),
41-
'default' => array_merge($defaultFirewall, [
42-
'anonymous' => null,
43-
'form_login' => null,
44-
]),
4538
],
4639
'access_control' => [
47-
['path' => '^/admin', 'roles' => 'ROLE_ADMIN'],
4840
['path' => '^/api', 'roles' => 'ROLE_API'],
4941
],
5042
]);

0 commit comments

Comments
 (0)