Merge pull request #7698 from open-sausages/pulls/4/not-auth-ful-middleware

API Shift basic auth checking into middleware

_config/security.yml

@@ -26,9 +26,13 @@ SilverStripe\Core\Injector\Injector:
     properties:
       Middlewares:
         AuthenticationMiddleware: '%$SilverStripe\Security\AuthenticationMiddleware'
+        BasicAuthMiddleware: '%$SilverStripe\Security\BasicAuthMiddleware'
   SilverStripe\Security\AuthenticationMiddleware:
     properties:
       AuthenticationHandler: '%$SilverStripe\Security\AuthenticationHandler'
+  SilverStripe\Security\BasicAuthMiddleware:
+    properties:
+      URLPatterns: []
   SilverStripe\Security\Security:
     properties:
       Authenticators:

src/Control/Controller.php

@@ -4,8 +4,10 @@ namespace SilverStripe\Control;
 
 use SilverStripe\Core\ClassInfo;
 use SilverStripe\Dev\Debug;
+use SilverStripe\Dev\Deprecation;
 use SilverStripe\ORM\FieldType\DBHTMLText;
 use SilverStripe\Security\BasicAuth;
+use SilverStripe\Security\BasicAuthMiddleware;
 use SilverStripe\Security\Member;
 use SilverStripe\Security\Security;
 use SilverStripe\View\SSViewer;
@@ -61,6 +63,8 @@ class Controller extends RequestHandler implements TemplateGlobalProvider
     protected $templates = [];
 
     /**
+     * @deprecated 4.1.0...5.0.0 Add this controller's url to
+     * SilverStripe\Security\BasicAuthMiddleware.URLPatterns injected property instead of setting false
      * @var bool
      */
     protected $basicAuthEnabled = true;
@@ -98,6 +102,7 @@ class Controller extends RequestHandler implements TemplateGlobalProvider
      */
     protected function init()
     {
+        // @todo This will be removed in 5.0 and will be controlled by middleware instead
         if ($this->basicAuthEnabled) {
             BasicAuth::protect_site_if_necessary();
         }
@@ -528,9 +533,16 @@ class Controller extends RequestHandler implements TemplateGlobalProvider
      * Call this to disable site-wide basic authentication for a specific controller. This must be
      * called before Controller::init(). That is, you must call it in your controller's init method
      * before it calls parent::init().
+     *
+     * @deprecated 4.1.0...5.0.0 Add this controller's url to
+     * SilverStripe\Security\BasicAuthMiddleware.URLPatterns injected property instead of setting false
      */
     public function disableBasicAuth()
     {
+        Deprecation::notice(
+            '5.0',
+            'Add this controller\'s url to ' . BasicAuthMiddleware::class . '.URLPatterns injected property instead'
+        );
         $this->basicAuthEnabled = false;
     }
 

src/Security/BasicAuth.php

@@ -9,6 +9,7 @@ use SilverStripe\Control\HTTPResponse;
 use SilverStripe\Control\HTTPResponse_Exception;
 use SilverStripe\Core\Config\Configurable;
 use SilverStripe\Core\Environment;
+use SilverStripe\Core\Injector\Injector;
 use SilverStripe\ORM\Connect\DatabaseException;
 use SilverStripe\Security\MemberAuthenticator\MemberAuthenticator;
 
@@ -200,11 +201,13 @@ class BasicAuth
      *
      * If you want to enabled protection (rather than enforcing it),
      * please use {@link protect_entire_site()}.
+     *
+     * @param HTTPRequest|null $request
+     * @throws HTTPResponse_Exception
      */
-    public static function protect_site_if_necessary()
+    public static function protect_site_if_necessary(HTTPRequest $request = null)
     {
         $config = static::config();
-        $request = Controller::curr()->getRequest();
 
         // Check if site is protected
         if ($config->get('entire_site_protected')) {
@@ -220,6 +223,11 @@ class BasicAuth
             return;
         }
 
+        // Get request
+        if (!$request && Injector::inst()->has(HTTPRequest::class)) {
+            $request = Injector::inst()->get(HTTPRequest::class);
+        }
+
         // Require login
         static::requireLogin(
             $request,

src/Security/BasicAuthMiddleware.php

@@ -0,0 +1,118 @@
+<?php
+
+namespace SilverStripe\Security;
+
+use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Control\HTTPResponse;
+use SilverStripe\Control\HTTPResponse_Exception;
+use SilverStripe\Control\Middleware\HTTPMiddleware;
+use SilverStripe\Control\Middleware\CanonicalURLMiddleware;
+
+class BasicAuthMiddleware implements HTTPMiddleware
+{
+    /**
+     * URL Patterns for basic auth. Keys are the Regexp string to match, and the key can
+     * be one of the below:
+     *  - true (bool) - Enabled for this url
+     *  - false (bool) - Disabled for this url
+     *  - Any string / array - Enabled for this url, and require the given string as a permission code
+     *  - null (default) - Calls BasicAuth::protect_site_if_necessary(), which falls back to config setting
+     *
+     * E.g.
+     * [
+     *   '#^home#i' => false,
+     *   '#^secure#i' => true,
+     *   '#^secure/admin#i' => 'ADMIN',
+     * ]
+     *
+     * @see CanonicalURLMiddleware
+     * @var array
+     */
+    protected $urlPatterns = [];
+
+    /**
+     * Generate response for the given request
+     *
+     * @param HTTPRequest $request
+     * @param callable $delegate
+     * @return HTTPResponse
+     */
+    public function process(HTTPRequest $request, callable $delegate)
+    {
+        // Check if url matches any patterns
+        $match = $this->checkMatchingURL($request);
+
+        // Check middleware unless specifically opting out
+        if ($match !== false) {
+            try {
+                // Determine method to check
+                if ($match) {
+                    // Truthy values are explicit, check with optional permission code
+                    $permission = $match === true ? null : $match;
+                    BasicAuth::requireLogin(
+                        $request,
+                        BasicAuth::config()->get('entire_site_protected_message'),
+                        $permission,
+                        false
+                    );
+                } elseif ($match === null) {
+                    // Null implies fall back to default behaviour
+                    BasicAuth::protect_site_if_necessary($request);
+                }
+            } catch (HTTPResponse_Exception $ex) {
+                return $ex->getResponse();
+            }
+        }
+
+        // Pass on to other middlewares
+        return $delegate($request);
+    }
+
+    /**
+     * Get list of url patterns
+     *
+     * @return array
+     */
+    public function getURLPatterns()
+    {
+        return $this->urlPatterns ?: [];
+    }
+
+    /**
+     * @param array $urlPatterns
+     * @return $this
+     */
+    public function setURLPatterns(array $urlPatterns)
+    {
+        $this->urlPatterns = $urlPatterns;
+        return $this;
+    }
+
+    /**
+     * Check if global basic auth is enabled for the given request
+     *
+     * @param HTTPRequest $request
+     * @return bool|string|array|null boolean value if enabled/disabled explicitly for this request,
+     * or null if should fall back to config value. Can also provide an explicit string / array of permission
+     * codes to require for this requset.
+     */
+    protected function checkMatchingURL(HTTPRequest $request)
+    {
+        // Null if no permissions enabled
+        $patterns = $this->getURLPatterns();
+        if (!$patterns) {
+            return null;
+        }
+
+        // Filter redirect based on url
+        $relativeURL = $request->getURL(true);
+        foreach ($patterns as $pattern => $result) {
+            if (preg_match($pattern, $relativeURL)) {
+                return $result;
+            }
+        }
+
+        // No patterns match
+        return null;
+    }
+}

tests/php/Security/BasicAuthTest.php

@@ -2,11 +2,14 @@
 
 namespace SilverStripe\Security\Tests;
 
+use SilverStripe\Core\Injector\Injector;
 use SilverStripe\Security\BasicAuth;
+use SilverStripe\Security\BasicAuthMiddleware;
 use SilverStripe\Security\Member;
 use SilverStripe\Security\Security;
 use SilverStripe\Dev\FunctionalTest;
 use SilverStripe\Control\Director;
+use SilverStripe\Security\Tests\BasicAuthTest\ControllerNotSecured;
 use SilverStripe\Security\Tests\BasicAuthTest\ControllerSecuredWithoutPermission;
 use SilverStripe\Security\Tests\BasicAuthTest\ControllerSecuredWithPermission;
 
@@ -15,14 +18,12 @@ use SilverStripe\Security\Tests\BasicAuthTest\ControllerSecuredWithPermission;
  */
 class BasicAuthTest extends FunctionalTest
 {
-
-    protected static $original_unique_identifier_field;
-
     protected static $fixture_file = 'BasicAuthTest.yml';
 
     protected static $extra_controllers = [
         ControllerSecuredWithPermission::class,
         ControllerSecuredWithoutPermission::class,
+        ControllerNotSecured::class,
     ];
 
     protected function setUp()
@@ -37,9 +38,13 @@ class BasicAuthTest extends FunctionalTest
         // Temp disable is_cli() exemption for tests
         BasicAuth::config()->set('ignore_cli', false);
 
-        // Reset statics
+        // Set route-specific permissions
-        BasicAuthTest\ControllerSecuredWithPermission::$index_called = false;
+        /** @var BasicAuthMiddleware $middleware */
-        BasicAuthTest\ControllerSecuredWithPermission::$post_init_called = false;
+        $middleware = Injector::inst()->get(BasicAuthMiddleware::class);
+        $middleware->setURLPatterns([
+            '/^BasicAuthTest_ControllerSecuredWithPermission$/' => 'MYCODE',
+            '/^BasicAuthTest_ControllerSecuredWithoutPermission$/' => true,
+        ]);
     }
 
     public function testBasicAuthEnabledWithoutLogin()
@@ -48,21 +53,6 @@ class BasicAuthTest extends FunctionalTest
         $this->assertEquals(401, $response->getStatusCode());
     }
 
-    public function testBasicAuthDoesntCallActionOrFurtherInitOnAuthFailure()
-    {
-        Director::test('BasicAuthTest_ControllerSecuredWithPermission');
-        $this->assertFalse(BasicAuthTest\ControllerSecuredWithPermission::$index_called);
-        $this->assertFalse(BasicAuthTest\ControllerSecuredWithPermission::$post_init_called);
-
-        $headers = [
-            'PHP_AUTH_USER' => 'user-in-mygroup@test.com',
-            'PHP_AUTH_PW' => 'test',
-        ];
-        Director::test('BasicAuthTest_ControllerSecuredWithPermission', [], [], null, null, $headers);
-        $this->assertTrue(BasicAuthTest\ControllerSecuredWithPermission::$index_called);
-        $this->assertTrue(BasicAuthTest\ControllerSecuredWithPermission::$post_init_called);
-    }
-
     public function testBasicAuthEnabledWithPermission()
     {
         $headers = [
@@ -138,4 +128,26 @@ class BasicAuthTest extends FunctionalTest
         $check = Member::get()->filter('Email', 'failedlogin@test.com')->first();
         $this->assertEquals(0, $check->FailedLoginCount);
     }
+
+    public function testProtectEntireSite()
+    {
+        // Unsecured controller allows access
+        $response = Director::test('BasicAuthTest_ControllerNotSecured');
+        $this->assertEquals(200, $response->getStatusCode());
+
+        // Globally enable basic auth
+        BasicAuth::protect_entire_site();
+        $response = Director::test('BasicAuthTest_ControllerNotSecured');
+        $this->assertEquals(401, $response->getStatusCode());
+        $this->assertNotEmpty($response->getHeader('WWW-Authenticate'));
+
+        // Can be excluded via rule
+        /** @var BasicAuthMiddleware $middleware */
+        $middleware = Injector::inst()->get(BasicAuthMiddleware::class);
+        $middleware->setURLPatterns([
+            '/^BasicAuthTest_ControllerNotSecured$/' => false,
+        ]);
+        $response = Director::test('BasicAuthTest_ControllerNotSecured');
+        $this->assertEquals(200, $response->getStatusCode());
+    }
 }

tests/php/Security/BasicAuthTest/ControllerNotSecured.php

@@ -0,0 +1,27 @@
+<?php
+
+namespace SilverStripe\Security\Tests\BasicAuthTest;
+
+use SilverStripe\Control\Controller;
+use SilverStripe\Dev\TestOnly;
+
+/**
+ * @skipUpgrade
+ */
+class ControllerNotSecured extends Controller implements TestOnly
+{
+    protected $template = 'BlankPage';
+
+    /**
+     * Disable legacy global-enable
+     *
+     * @deprecated 4.0..5.0
+     * @var bool
+     */
+    protected $basicAuthEnabled = false;
+
+    public function Link($action = null)
+    {
+        return Controller::join_links('BasicAuthTest_ControllerNotSecured', $action, '/');
+    }
+}

tests/php/Security/BasicAuthTest/ControllerSecuredWithPermission.php

@@ -4,33 +4,16 @@ namespace SilverStripe\Security\Tests\BasicAuthTest;
 
 use SilverStripe\Control\Controller;
 use SilverStripe\Dev\TestOnly;
-use SilverStripe\Security\BasicAuth;
 
 /**
  * @skipUpgrade
  */
 class ControllerSecuredWithPermission extends Controller implements TestOnly
 {
-    public static $post_init_called = false;
-
-    public static $index_called = false;
-
     protected $template = 'BlankPage';
 
-    protected function init()
-    {
-        self::$post_init_called = false;
-        self::$index_called = false;
-
-        BasicAuth::protect_entire_site(true, 'MYCODE');
-        parent::init();
-
-        self::$post_init_called = true;
-    }
-
     public function index()
     {
-        self::$index_called = true;
         return "index";
     }
 

tests/php/Security/BasicAuthTest/ControllerSecuredWithoutPermission.php

@@ -4,22 +4,14 @@ namespace SilverStripe\Security\Tests\BasicAuthTest;
 
 use SilverStripe\Control\Controller;
 use SilverStripe\Dev\TestOnly;
-use SilverStripe\Security\BasicAuth;
 
 /**
  * @skipUpgrade
  */
 class ControllerSecuredWithoutPermission extends Controller implements TestOnly
 {
-
     protected $template = 'BlankPage';
 
-    protected function init()
-    {
-        BasicAuth::protect_entire_site(true, null);
-        parent::init();
-    }
-
     public function Link($action = null)
     {
         return Controller::join_links('BasicAuthTest_ControllerSecuredWithoutPermission', $action, '/');