Merge pull request #28 from jolicode/feature/new-blacklisters-tweaks

Blacklisters tweaks

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)
 

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

composer.json

@@ -47,5 +47,10 @@
         "branch-alias": {
             "dev-master": "0.x-dev"
         }
+    },
+    "scripts": {
+        "test": [
+            "SYMFONY_PHPUNIT_REMOVE=\"symfony/yaml\" vendor/bin/simple-phpunit"
+        ]
     }
 }

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`:

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
+```

src/Bridge/Symfony/Blacklister/GetMethodBlacklister.php → 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;
     }
 }

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();

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
@@ -74,6 +70,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' }

tests/Functional/Fixtures/symfony/app/Resources/views/admin.html.twig

@@ -0,0 +1,9 @@
+<html>
+    <head>
+        <!--SEO_TITLE--><title>old title for admin</title><!--/SEO_TITLE-->
+        <meta name="description" content="description for admin" />
+    </head>
+    <body>
+        <h1>Hello admin</h1>
+    </body>
+</html>

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,10 +40,17 @@ 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"
     domains:
         domain_doctrine: 'domain_doctrine.com'
         domain_in_memory: 'domain_in_memory.com'
         domain_php: 'domain_php.com'
+    blacklist:
+        - not_2xx
+        - { type: path, pattern: '^/admin' }
+        - { type: not_method, method: ['GET', 'POST'] }
+        - xml_http

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

tests/Functional/Fixtures/symfony/src/Controller/ErrorController.php → 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);
+    }
 }

tests/Functional/SymfonyTest.php

@@ -22,6 +22,19 @@
 
 class SymfonyTest extends KernelTestCase
 {
+    const NOT_OVERRIDDEN_HOMEPAGE_CONTENT = <<<'HTML'
+<html>
+    <head>
+        <title>old title for homepage</title>
+        <meta name="description" content="description for homepage" />
+    </head>
+    <body>
+        <h1>Hello world</h1>
+    </body>
+</html>
+
+HTML;
+
     public static function setUpBeforeClass()
     {
         parent::setUpBeforeClass();
@@ -119,64 +132,88 @@ public function test_it_overrides_seo_handled_with_php_fetcher()
     }
 
     public function test_it_does_not_override_seo_when_no_fetcher_matching()
+    {
+        $response = $this->call('/', 'domain_unkown.com');
+
+        $this->assertSame(self::NOT_OVERRIDDEN_HOMEPAGE_CONTENT, $response->getContent());
+    }
+
+    public function test_it_does_not_override_seo_when_no_content_or_binary_response()
+    {
+        $response = $this->call('/download', 'localhost');
+
+        $this->assertSame(200, $response->getStatusCode());
+        $this->assertSame(false, $response->getContent());
+    }
+
+    public function test_it_does_not_override_seo_when_no_2XX_response()
     {
         $expected = <<<'HTML'
 <html>
     <head>
-        <title>old title for homepage</title>
-        <meta name="description" content="description for homepage" />
+        <title>old title for error page</title>
+        <meta name="description" content="description for error page" />
     </head>
     <body>
-        <h1>Hello world</h1>
+        <h1>Hello error</h1>
     </body>
 </html>
 
 HTML;
+        $response = $this->call('/error', 'localhost');
 
-        $response = $this->call('/', 'domain_unkown.com');
-
+        $this->assertSame(400, $response->getStatusCode());
         $this->assertSame($expected, $response->getContent());
     }
 
-    public function test_it_does_not_override_seo_when_no_2XX_response()
+    public function test_it_does_not_override_seo_when_request_path_does_not_match()
     {
         $expected = <<<'HTML'
 <html>
     <head>
-        <title>old title for error page</title>
-        <meta name="description" content="description for error page" />
+        <title>old title for admin</title>
+        <meta name="description" content="description for admin" />
     </head>
     <body>
-        <h1>Hello error</h1>
+        <h1>Hello admin</h1>
     </body>
 </html>
 
 HTML;
+        $response = $this->call('/admin', 'localhost');
 
-        $response = $this->call('/error', 'localhost');
-
-        $this->assertSame(400, $response->getStatusCode());
+        $this->assertSame(200, $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_use_not_allowed_action()
     {
-        $response = $this->call('/download', 'localhost');
+        $response = $this->call('/', 'localhost', 'PUT');
 
         $this->assertSame(200, $response->getStatusCode());
-        $this->assertSame(false, $response->getContent());
+        $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($uri, $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);
     }