From 596b7edc4d77fffdfb9731df47f2148c95fd2f3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFck=20Piera?= Date: Thu, 12 Oct 2017 14:42:17 +0200 Subject: [PATCH 1/3] Fix local test issues caused by simple-phpunit --- CONTRIBUTING.md | 2 +- composer.json | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 86d4261..e7f646e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -48,7 +48,7 @@ changes, improvements or alternatives may be given). Run the tests using the following script: ```shell -vendor/bin/phpunit +composer test ``` ## Standard code diff --git a/composer.json b/composer.json index a8687ed..5ceadc4 100644 --- a/composer.json +++ b/composer.json @@ -47,5 +47,10 @@ "branch-alias": { "dev-master": "0.x-dev" } + }, + "scripts": { + "test": [ + "SYMFONY_PHPUNIT_REMOVE=\"symfony/yaml\" vendor/bin/simple-phpunit" + ] } } From 8e87c8f4edc3756761451e25a880dbbea6d4fed3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFck=20Piera?= Date: Thu, 12 Oct 2017 14:52:40 +0200 Subject: [PATCH 2/3] Fix and add functionnal tests for old blacklisters --- .../Symfony/Resources/config/services.yml | 2 +- .../app/Resources/views/admin.html.twig | 9 +++ .../Fixtures/symfony/app/config/config.yml | 9 ++- .../Fixtures/symfony/app/config/routing.yml | 9 ++- ...{ErrorController.php => AppController.php} | 7 ++- tests/Functional/SymfonyTest.php | 55 +++++++++++++------ 6 files changed, 67 insertions(+), 24 deletions(-) create mode 100644 tests/Functional/Fixtures/symfony/app/Resources/views/admin.html.twig rename tests/Functional/Fixtures/symfony/src/Controller/{ErrorController.php => AppController.php} (76%) diff --git a/src/Bridge/Symfony/Resources/config/services.yml b/src/Bridge/Symfony/Resources/config/services.yml index 8ae88f1..ab1e84b 100644 --- a/src/Bridge/Symfony/Resources/config/services.yml +++ b/src/Bridge/Symfony/Resources/config/services.yml @@ -74,6 +74,6 @@ services: class: Joli\SeoOverride\Bridge\Symfony\Blacklister\PathBlacklister public: false arguments: - - '' + - '' # Argument configured in the DIC extension tags: - { name: seo_override.blacklister, alias: path, required_options: 'pattern' } diff --git a/tests/Functional/Fixtures/symfony/app/Resources/views/admin.html.twig b/tests/Functional/Fixtures/symfony/app/Resources/views/admin.html.twig new file mode 100644 index 0000000..c903773 --- /dev/null +++ b/tests/Functional/Fixtures/symfony/app/Resources/views/admin.html.twig @@ -0,0 +1,9 @@ + + + old title for admin + + + +

Hello admin

+ + diff --git a/tests/Functional/Fixtures/symfony/app/config/config.yml b/tests/Functional/Fixtures/symfony/app/config/config.yml index 7f3ba47..e7196f2 100644 --- a/tests/Functional/Fixtures/symfony/app/config/config.yml +++ b/tests/Functional/Fixtures/symfony/app/config/config.yml @@ -1,6 +1,6 @@ services: - app.error_controller: - class: Joli\SeoOverride\Tests\Functional\Fixtures\symfony\src\Controller\ErrorController + app.controller: + class: Joli\SeoOverride\Tests\Functional\Fixtures\symfony\src\Controller\AppController calls: - [setContainer, ["@service_container"]] @@ -40,6 +40,8 @@ seo_override: '': '/error': title: 'new title for error page' + '/admin': + title: 'new title for admin' - type: php include_path: "%kernel.root_dir%/config/seo_overrides.php" @@ -47,3 +49,6 @@ seo_override: domain_doctrine: 'domain_doctrine.com' domain_in_memory: 'domain_in_memory.com' domain_php: 'domain_php.com' + blacklist: + - not_2xx + - { type: path, pattern: '^/admin' } diff --git a/tests/Functional/Fixtures/symfony/app/config/routing.yml b/tests/Functional/Fixtures/symfony/app/config/routing.yml index ad8b1d0..7fd729c 100644 --- a/tests/Functional/Fixtures/symfony/app/config/routing.yml +++ b/tests/Functional/Fixtures/symfony/app/config/routing.yml @@ -7,9 +7,14 @@ index: error: path: /error defaults: - _controller: app.error_controller:errorAction + _controller: app.controller:errorAction download: path: /download defaults: - _controller: app.error_controller:downloadAction + _controller: app.controller:downloadAction + +admin: + path: /admin + defaults: + _controller: app.controller:adminAction diff --git a/tests/Functional/Fixtures/symfony/src/Controller/ErrorController.php b/tests/Functional/Fixtures/symfony/src/Controller/AppController.php similarity index 76% rename from tests/Functional/Fixtures/symfony/src/Controller/ErrorController.php rename to tests/Functional/Fixtures/symfony/src/Controller/AppController.php index 893c7a9..0c32850 100644 --- a/tests/Functional/Fixtures/symfony/src/Controller/ErrorController.php +++ b/tests/Functional/Fixtures/symfony/src/Controller/AppController.php @@ -6,7 +6,7 @@ use Symfony\Component\HttpFoundation\BinaryFileResponse; use Symfony\Component\HttpFoundation\Response; -class ErrorController extends Controller +class AppController extends Controller { public function errorAction() { @@ -19,4 +19,9 @@ public function downloadAction() return new BinaryFileResponse($tmpfname); } + + public function adminAction() + { + return new Response($this->renderView('admin.html.twig'), 200); + } } diff --git a/tests/Functional/SymfonyTest.php b/tests/Functional/SymfonyTest.php index 2811bc7..400f55f 100644 --- a/tests/Functional/SymfonyTest.php +++ b/tests/Functional/SymfonyTest.php @@ -22,6 +22,19 @@ class SymfonyTest extends KernelTestCase { + const NOT_OVERRIDDEN_HOMEPAGE_CONTENT = <<<'HTML' + + + old title for homepage + + + +

Hello world

+ + + +HTML; + public static function setUpBeforeClass() { parent::setUpBeforeClass(); @@ -120,22 +133,17 @@ public function test_it_overrides_seo_handled_with_php_fetcher() public function test_it_does_not_override_seo_when_no_fetcher_matching() { - $expected = <<<'HTML' - - - old title for homepage - - - -

Hello world

- - + $response = $this->call('/', 'domain_unkown.com'); -HTML; + $this->assertSame(self::NOT_OVERRIDDEN_HOMEPAGE_CONTENT, $response->getContent()); + } - $response = $this->call('/', 'domain_unkown.com'); + public function test_it_does_not_override_seo_when_no_content_or_binary_response() + { + $response = $this->call('/download', 'localhost'); - $this->assertSame($expected, $response->getContent()); + $this->assertSame(200, $response->getStatusCode()); + $this->assertSame(false, $response->getContent()); } public function test_it_does_not_override_seo_when_no_2XX_response() @@ -152,19 +160,30 @@ public function test_it_does_not_override_seo_when_no_2XX_response() HTML; - $response = $this->call('/error', 'localhost'); $this->assertSame(400, $response->getStatusCode()); $this->assertSame($expected, $response->getContent()); } - public function test_it_does_not_override_seo_when_no_content_or_binary_response() + public function test_it_does_not_override_seo_when_request_path_does_not_match() { - $response = $this->call('/download', 'localhost'); + $expected = <<<'HTML' + + + old title for admin + + + +

Hello admin

+ + + +HTML; + $response = $this->call('/admin', 'localhost'); $this->assertSame(200, $response->getStatusCode()); - $this->assertSame(false, $response->getContent()); + $this->assertSame($expected, $response->getContent()); } protected static function getKernelClass() @@ -172,7 +191,7 @@ protected static function getKernelClass() return AppKernel::class; } - private function call($uri, $host) + private function call(string $uri, string $host) { $request = Request::create($uri, 'GET', [], [], [], [ 'HTTP_HOST' => $host, From 91c4d35e98fae2f367ddf7c0945a9e9b6dfc0f2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFck=20Piera?= Date: Thu, 12 Oct 2017 14:56:07 +0200 Subject: [PATCH 3/3] Refactore new methods blacklisters and add documentation --- CHANGELOG.md | 1 + doc/symfony/README.md | 2 +- doc/symfony/available-blacklisters.md | 30 +++++++++ ...acklister.php => NotMethodBlacklister.php} | 26 +++++--- .../Blacklister/PostMethodBlacklister.php | 33 ---------- .../Blacklister/XmlHttpBlacklister.php | 9 --- .../Symfony/Resources/config/services.yml | 14 ++--- .../Fixtures/symfony/app/config/config.yml | 2 + tests/Functional/SymfonyTest.php | 26 ++++++-- .../Blacklister/GetMethodBlackListerTest.php | 46 -------------- .../Blacklister/NotMethodBlacklisterTest.php | 62 +++++++++++++++++++ .../Blacklister/PostMethodBlacklisterTest.php | 46 -------------- .../Blacklister/XmlHttpBlacklisterTest.php | 2 +- 13 files changed, 141 insertions(+), 158 deletions(-) rename src/Bridge/Symfony/Blacklister/{GetMethodBlacklister.php => NotMethodBlacklister.php} (57%) delete mode 100644 src/Bridge/Symfony/Blacklister/PostMethodBlacklister.php delete mode 100644 tests/Unit/Bridge/Symfony/Blacklister/GetMethodBlackListerTest.php create mode 100644 tests/Unit/Bridge/Symfony/Blacklister/NotMethodBlacklisterTest.php delete mode 100644 tests/Unit/Bridge/Symfony/Blacklister/PostMethodBlacklisterTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 79b7d58..a77a8bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ * [BC BREAK] Remove SeoManagerInterface * Add blacklist behaviour to avoid useless fetcher runs +* Add blacklisters checking for XHR or HTTP methods ## 0.4.0 (2017-09-12) diff --git a/doc/symfony/README.md b/doc/symfony/README.md index 16e9aba..7cd9251 100644 --- a/doc/symfony/README.md +++ b/doc/symfony/README.md @@ -111,7 +111,7 @@ Note: ### Blacklist You can blacklist some request/response to avoid fetcher to run (f.e. on non -2xx response, on your admin, etc). +2xx response, on your admin, on XHR calls, etc). There is some built-in blacklister that you can enable/configure via the config `blacklist`: diff --git a/doc/symfony/available-blacklisters.md b/doc/symfony/available-blacklisters.md index 2ba2f8d..e5fc4b2 100644 --- a/doc/symfony/available-blacklisters.md +++ b/doc/symfony/available-blacklisters.md @@ -4,6 +4,8 @@ |---|---|---| | not_2xx | Blacklist if the response status code is not 2xx | None | | path | Blacklist if the request path matches the pattern | - "pattern": mandatory, the regex pattern to match | +| not_method | Blacklist if the request HTTP method is in the accepted ones | - "method": mandatory, the HTTP method(s) to accept | +| xml_http | Blacklist if the request is made through XmlHttpRequest | None | ## not_2xx @@ -36,3 +38,31 @@ seo_override: type: path pattern: '^/(admin|account)' ``` + +## not_method + +The `not_method` blacklister refuses request whose method is not in the +configured `method` option. This option can be a single HTTP method or an array +of methods: + +```yaml +seo_override: + blacklist: + - + type: not_method + method: [GET, POST] + # if only one method should be accepted + # method: GET +``` + +## xml_http + +The `xml_http` blacklister refuses request which have been made through Ajax +(XmlHttpRequest). This blacklister does not have any option so the short YAML +syntax can be used: + +```yaml +seo_override: + blacklist: + - xml_http +``` diff --git a/src/Bridge/Symfony/Blacklister/GetMethodBlacklister.php b/src/Bridge/Symfony/Blacklister/NotMethodBlacklister.php similarity index 57% rename from src/Bridge/Symfony/Blacklister/GetMethodBlacklister.php rename to src/Bridge/Symfony/Blacklister/NotMethodBlacklister.php index a1eb4d1..ed7ed6c 100644 --- a/src/Bridge/Symfony/Blacklister/GetMethodBlacklister.php +++ b/src/Bridge/Symfony/Blacklister/NotMethodBlacklister.php @@ -15,19 +15,27 @@ use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; -/** - * Class GetBlacklister. - */ -class GetMethodBlacklister implements Blacklister +class NotMethodBlacklister implements Blacklister { + /** @var string[] */ + private $methods; + /** - * @param Request $request - * @param Response $response - * - * @return bool + * @param string|string[] $method */ + public function __construct($method) + { + $this->methods = (array) $method; + } + public function isBlacklisted(Request $request, Response $response): bool { - return $request->isMethod('get'); + foreach ($this->methods as $method) { + if ($request->isMethod($method)) { + return false; + } + } + + return true; } } diff --git a/src/Bridge/Symfony/Blacklister/PostMethodBlacklister.php b/src/Bridge/Symfony/Blacklister/PostMethodBlacklister.php deleted file mode 100644 index a0583f0..0000000 --- a/src/Bridge/Symfony/Blacklister/PostMethodBlacklister.php +++ /dev/null @@ -1,33 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -namespace Joli\SeoOverride\Bridge\Symfony\Blacklister; - -use Joli\SeoOverride\Bridge\Symfony\Blacklister; -use Symfony\Component\HttpFoundation\Request; -use Symfony\Component\HttpFoundation\Response; - -/** - * Class PostBlacklister. - */ -class PostMethodBlacklister implements Blacklister -{ - /** - * @param Request $request - * @param Response $response - * - * @return bool - */ - public function isBlacklisted(Request $request, Response $response): bool - { - return $request->isMethod('post'); - } -} diff --git a/src/Bridge/Symfony/Blacklister/XmlHttpBlacklister.php b/src/Bridge/Symfony/Blacklister/XmlHttpBlacklister.php index bbc6be7..c808641 100644 --- a/src/Bridge/Symfony/Blacklister/XmlHttpBlacklister.php +++ b/src/Bridge/Symfony/Blacklister/XmlHttpBlacklister.php @@ -15,17 +15,8 @@ use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; -/** - * Class XmlHttpBlacklister. - */ class XmlHttpBlacklister implements Blacklister { - /** - * @param Request $request - * @param Response $response - * - * @return bool - */ public function isBlacklisted(Request $request, Response $response): bool { return $request->isXmlHttpRequest(); diff --git a/src/Bridge/Symfony/Resources/config/services.yml b/src/Bridge/Symfony/Resources/config/services.yml index ab1e84b..b801443 100644 --- a/src/Bridge/Symfony/Resources/config/services.yml +++ b/src/Bridge/Symfony/Resources/config/services.yml @@ -52,17 +52,13 @@ services: tags: - { name: seo_override.blacklister, alias: not_2xx } - seo_override.blacklister.get_method: - class: Joli\SeoOverride\Bridge\Symfony\Blacklister\GetMethodBlacklister - public: false - tags: - - { name: seo_override.blacklister, alias: get_method } - - seo_override.blacklister.post_method: - class: Joli\SeoOverride\Bridge\Symfony\Blacklister\PostMethodBlacklister + seo_override.blacklister.not_method: + class: Joli\SeoOverride\Bridge\Symfony\Blacklister\NotMethodBlacklister public: false + arguments: + - '' # Argument configured in the DIC extension tags: - - { name: seo_override.blacklister, alias: post_method } + - { name: seo_override.blacklister, alias: not_method, required_options: 'method' } seo_override.blacklister.xml_http: class: Joli\SeoOverride\Bridge\Symfony\Blacklister\XmlHttpBlacklister diff --git a/tests/Functional/Fixtures/symfony/app/config/config.yml b/tests/Functional/Fixtures/symfony/app/config/config.yml index e7196f2..86e4c51 100644 --- a/tests/Functional/Fixtures/symfony/app/config/config.yml +++ b/tests/Functional/Fixtures/symfony/app/config/config.yml @@ -52,3 +52,5 @@ seo_override: blacklist: - not_2xx - { type: path, pattern: '^/admin' } + - { type: not_method, method: ['GET', 'POST'] } + - xml_http diff --git a/tests/Functional/SymfonyTest.php b/tests/Functional/SymfonyTest.php index 400f55f..fa96a42 100644 --- a/tests/Functional/SymfonyTest.php +++ b/tests/Functional/SymfonyTest.php @@ -186,16 +186,34 @@ public function test_it_does_not_override_seo_when_request_path_does_not_match() $this->assertSame($expected, $response->getContent()); } + public function test_it_does_not_override_seo_when_request_use_not_allowed_action() + { + $response = $this->call('/', 'localhost', 'PUT'); + + $this->assertSame(200, $response->getStatusCode()); + $this->assertSame(self::NOT_OVERRIDDEN_HOMEPAGE_CONTENT, $response->getContent()); + } + + public function test_it_does_not_override_seo_when_request_is_xhr() + { + $response = $this->call('/', 'localhost', 'GET', [ + 'X-Requested-With' => 'XMLHttpRequest', + ]); + + $this->assertSame(200, $response->getStatusCode()); + $this->assertSame(self::NOT_OVERRIDDEN_HOMEPAGE_CONTENT, $response->getContent()); + } + protected static function getKernelClass() { return AppKernel::class; } - private function call(string $uri, string $host) + private function call(string $uri, string $host, string $method = 'GET', array $server = []) { - $request = Request::create($uri, 'GET', [], [], [], [ - 'HTTP_HOST' => $host, - ]); + $server['HTTP_HOST'] = $host; + + $request = Request::create($uri, $method, [], [], [], $server); return self::$kernel->handle($request); } diff --git a/tests/Unit/Bridge/Symfony/Blacklister/GetMethodBlackListerTest.php b/tests/Unit/Bridge/Symfony/Blacklister/GetMethodBlackListerTest.php deleted file mode 100644 index 8591e3c..0000000 --- a/tests/Unit/Bridge/Symfony/Blacklister/GetMethodBlackListerTest.php +++ /dev/null @@ -1,46 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -namespace Joli\SeoOverride\Tests\Unit\Bridge\Symfony\Blacklister; - -use Joli\SeoOverride\Bridge\Symfony\Blacklister\GetMethodBlacklister; -use PHPUnit\Framework\TestCase; -use Symfony\Component\HttpFoundation\Request; -use Symfony\Component\HttpFoundation\Response; - -class GetMethodBlackListerTest extends TestCase -{ - /** @var GetMethodBlacklister */ - private $blacklister; - - public function setUp() - { - parent::setUp(); - - $this->blacklister = new GetMethodBlacklister(); - } - - public function test_it_blacklists_get_method_requests() - { - $request = new Request(); - $request->setMethod('get'); - - self::assertTrue($this->blacklister->isBlacklisted($request, new Response())); - } - - public function test_it_does_not_blacklist_get_method_requests() - { - $request = new Request(); - $request->setMethod('post'); - - self::assertFalse($this->blacklister->isBlacklisted($request, new Response())); - } -} diff --git a/tests/Unit/Bridge/Symfony/Blacklister/NotMethodBlacklisterTest.php b/tests/Unit/Bridge/Symfony/Blacklister/NotMethodBlacklisterTest.php new file mode 100644 index 0000000..dcc5b46 --- /dev/null +++ b/tests/Unit/Bridge/Symfony/Blacklister/NotMethodBlacklisterTest.php @@ -0,0 +1,62 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Joli\SeoOverride\Tests\Unit\Bridge\Symfony\Blacklister; + +use Joli\SeoOverride\Bridge\Symfony\Blacklister\NotMethodBlacklister; +use PHPUnit\Framework\TestCase; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpFoundation\Response; + +class NotMethodBlacklisterTest extends TestCase +{ + /** @var NotMethodBlacklisterTest */ + private $blacklister; + + public function setUp() + { + parent::setUp(); + } + + public function test_it_blacklists_not_accepted_method_requests() + { + $request = new Request(); + $request->setMethod('put'); + + $blacklister = new NotMethodBlacklister(['GET', 'POST']); + + self::assertTrue($blacklister->isBlacklisted($request, new Response())); + } + + public function test_it_does_not_blacklist_accepted_method_requests() + { + $request = new Request(); + $request->setMethod('get'); + + $blacklister = new NotMethodBlacklister('GET'); + + self::assertFalse($blacklister->isBlacklisted($request, new Response())); + + $request = new Request(); + $request->setMethod('get'); + + $blacklister = new NotMethodBlacklister(['GET', 'POST']); + + self::assertFalse($blacklister->isBlacklisted($request, new Response())); + + $request = new Request(); + $request->setMethod('POST'); + + $blacklister = new NotMethodBlacklister(['GET', 'POST']); + + self::assertFalse($blacklister->isBlacklisted($request, new Response())); + } +} diff --git a/tests/Unit/Bridge/Symfony/Blacklister/PostMethodBlacklisterTest.php b/tests/Unit/Bridge/Symfony/Blacklister/PostMethodBlacklisterTest.php deleted file mode 100644 index 98843f2..0000000 --- a/tests/Unit/Bridge/Symfony/Blacklister/PostMethodBlacklisterTest.php +++ /dev/null @@ -1,46 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -namespace Joli\SeoOverride\Tests\Unit\Bridge\Symfony\Blacklister; - -use Joli\SeoOverride\Bridge\Symfony\Blacklister\PostMethodBlacklister; -use PHPUnit\Framework\TestCase; -use Symfony\Component\HttpFoundation\Request; -use Symfony\Component\HttpFoundation\Response; - -class PostMethodBlacklisterTest extends TestCase -{ - /** @var PostMethodBlacklister */ - private $blacklister; - - public function setUp() - { - parent::setUp(); - - $this->blacklister = new PostMethodBlacklister(); - } - - public function test_it_blacklists_post_method_requests() - { - $request = new Request(); - $request->setMethod('post'); - - self::assertTrue($this->blacklister->isBlacklisted($request, new Response())); - } - - public function test_it_does_not_blacklist_post_method_requests() - { - $request = new Request(); - $request->setMethod('get'); - - self::assertFalse($this->blacklister->isBlacklisted($request, new Response())); - } -} diff --git a/tests/Unit/Bridge/Symfony/Blacklister/XmlHttpBlacklisterTest.php b/tests/Unit/Bridge/Symfony/Blacklister/XmlHttpBlacklisterTest.php index ce7ae4e..9b56d3b 100644 --- a/tests/Unit/Bridge/Symfony/Blacklister/XmlHttpBlacklisterTest.php +++ b/tests/Unit/Bridge/Symfony/Blacklister/XmlHttpBlacklisterTest.php @@ -36,7 +36,7 @@ public function test_it_blacklists_xml_http_requests() self::assertTrue($this->blacklister->isBlacklisted($request, new Response())); } - public function test_it_does_not_blacklist_xml_http_requests() + public function test_it_does_not_blacklist_not_xml_http_requests() { $request = new Request();