FIX Allow logged in user with permissions to bypass basic auth

README.md

@@ -112,7 +112,7 @@ SilverStripe\EnvironmentCheck\EnvironmentChecker:
 ## Authentication
 
 By default, accessing the `dev/check` URL will not require authentication on CLI and dev environments, but if you're
-trying to access it on a live or test environment, it will respond with a 403 HTTP status unless you're logged in as
+trying to access it on a live or test environment, it will respond with a 401 HTTP status unless you're logged in as
 an administrator on the site.
 
 You may wish to have an automated service check `dev/check` periodically, but not want to open it up for public access.

_config/routes.yml

@@ -1,5 +1,6 @@
 ---
 Name: environmentcheckroutes
+Before: coreroutes
 ---
 SilverStripe\Control\Director:
   rules:

composer.json

@@ -21,14 +21,17 @@
     ],
     "require": {
         "php": "^8.1",
-        "silverstripe/framework": "^5",
+        "silverstripe/framework": "^5.3",
         "silverstripe/versioned": "^2",
         "guzzlehttp/guzzle": "^7"
     },
     "require-dev": {
         "phpunit/phpunit": "^9.6",
         "squizlabs/php_codesniffer": "^3"
     },
+    "conflict": {
+        "cwp/cwp-core": "<3.1"
+    },
     "extra": [],
     "autoload": {
         "psr-4": {

src/EnvironmentChecker.php

@@ -14,6 +14,7 @@
 use SilverStripe\Security\Member;
 use SilverStripe\Security\Permission;
 use SilverStripe\Security\Security;
+use SilverStripe\Security\BasicAuth;
 
 /**
  * Provides an interface for checking the given EnvironmentCheckSuite.
@@ -103,27 +104,42 @@ public function __construct($checkSuiteName, $title)
      */
     public function init($permission = 'ADMIN')
     {
+        $canAccess = $this->canAccess(null, $permission);
+        // Allow bypassing basic-auth check if the user is logged in with the required permission
+        if ($canAccess) {
+            return;
+        }
         // if the environment supports it, provide a basic auth challenge and see if it matches configured credentials
         if (Environment::getEnv('ENVCHECK_BASICAUTH_USERNAME')
             && Environment::getEnv('ENVCHECK_BASICAUTH_PASSWORD')
         ) {
-            // Check that details are both provided, and match
-            if (empty($_SERVER['PHP_AUTH_USER']) || empty($_SERVER['PHP_AUTH_PW'])
-                || $_SERVER['PHP_AUTH_USER'] != Environment::getEnv('ENVCHECK_BASICAUTH_USERNAME')
-                || $_SERVER['PHP_AUTH_PW'] != Environment::getEnv('ENVCHECK_BASICAUTH_PASSWORD')
+            $request = $this->getRequest();
+            $user = $request->getHeader('PHP_AUTH_USER');
+            $pw = $request->getHeader('PHP_AUTH_PW');
+            // Check that details match
+            if (empty($user) || empty($pw)
+                || $user !== Environment::getEnv('ENVCHECK_BASICAUTH_USERNAME')
+                || $pw !== Environment::getEnv('ENVCHECK_BASICAUTH_PASSWORD')
             ) {
                 // Fail check with basic auth challenge
                 $response = new HTTPResponse(null, 401);
                 $response->addHeader('WWW-Authenticate', "Basic realm=\"Environment check\"");
-                throw new HTTPResponse_Exception($response);
+                if ($user) {
+                    $response->setBody(_t(
+                        BasicAuth::class . '.ERRORNOTREC',
+                        "That username / password isn't recognised"
+                    ));
+                } else {
+                    $response->setBody(_t(
+                        BasicAuth::class . '.ENTERINFO',
+                        'Please enter a username and password.'
+                    ));
+                }
+                // Exception is caught by RequestHandler->handleRequest() and will halt further execution
+                $e = new HTTPResponse_Exception(null, 401);
+                $e->setResponse($response);
+                throw $e;
             }
-        } elseif (!$this->canAccess(null, $permission)) {
-            // Fail check with silverstripe login challenge
-            $result = Security::permissionFailure(
-                $this,
-                "You must have the {$permission} permission to access this check"
-            );
-            throw new HTTPResponse_Exception($result);
         }
     }
 

tests/EnvironmentCheckerTest.php

@@ -2,6 +2,7 @@
 
 namespace SilverStripe\EnvironmentCheck\Tests;
 
+use ReflectionProperty;
 use Monolog\Logger;
 use Psr\Log\LoggerInterface;
 use Psr\Log\LogLevel;
@@ -10,6 +11,10 @@
 use SilverStripe\Dev\SapphireTest;
 use SilverStripe\EnvironmentCheck\EnvironmentChecker;
 use SilverStripe\EnvironmentCheck\EnvironmentCheckSuite;
+use SilverStripe\Core\Environment;
+use SilverStripe\Core\Kernel;
+use SilverStripe\Control\HTTPResponse_Exception;
+use SilverStripe\Control\HTTPRequest;
 
 /**
  * Class EnvironmentCheckerTest
@@ -94,4 +99,85 @@ public function testLogsWithErrors()
 
         (new EnvironmentChecker('test suite', 'test'))->index();
     }
+
+    public static function provideBasicAuthAdminBypass(): array
+    {
+        return [
+            'logged-out-valid-creds' => [
+                'user' => 'logged-out',
+                'validCreds' => true,
+                'expectedSuccess' => true,
+            ],
+            'logged-out-invalid-creds' => [
+                'user' => 'logged-out',
+                'validCreds' => false,
+                'expectedSuccess' => false,
+            ],
+            'non-admin-valid-creds' => [
+                'user' => 'non-admin',
+                'validCreds' => true,
+                'expectedSuccess' => true,
+            ],
+            'non-admin-invalid-creds' => [
+                'user' => 'non-admin',
+                'validCreds' => false,
+                'expectedSuccess' => false,
+            ],
+            'admin-valid-creds' => [
+                'user' => 'admin',
+                'validCreds' => true,
+                'expectedSuccess' => true,
+            ],
+            'admin-invalid-creds' => [
+                'user' => 'admin',
+                'validCreds' => false,
+                'expectedSuccess' => true,
+            ],
+        ];
+    }
+
+    /**
+     * @dataProvider provideBasicAuthAdminBypass
+     */
+    public function testBasicAuthAdminBypass(
+        string $user,
+        bool $validCreds,
+        bool $expectedSuccess,
+    ): void {
+        // Pretend we're not using CLI which will bypass basic auth,
+        $reflectionCli = new ReflectionProperty(Environment::class, 'isCliOverride');
+        $reflectionCli->setAccessible(true);
+        $reflectionCli->setValue(false);
+        // Change from dev to test mode as dev mode will bypass basic auth
+        $kernel = Injector::inst()->get(Kernel::class);
+        $kernel->setEnvironment('test');
+        // Setup basic auth env variables
+        Environment::setEnv('ENVCHECK_BASICAUTH_USERNAME', 'test');
+        Environment::setEnv('ENVCHECK_BASICAUTH_PASSWORD', 'password');
+        // Log in or out
+        if ($user === 'admin') {
+            $this->logInWithPermission('ADMIN');
+        } elseif ($user === 'non-admin') {
+            $this->logInWithPermission('NOT_AN_ADMIN');
+        } else {
+            $this->logOut();
+        }
+        // Simulate passing in basic auth creds
+        $request = new HTTPRequest('GET', 'dev/check');
+        $request->addHeader('PHP_AUTH_USER', 'test');
+        $request->addHeader('PHP_AUTH_PW', $validCreds ? 'password' : 'invalid');
+        // Run test
+        $checker = new EnvironmentChecker('test suite', 'test');
+        $checker->setRequest($request);
+        $success = null;
+        try {
+            $checker->init();
+            $success = true;
+        } catch (HTTPResponse_Exception $e) {
+            $success = false;
+            $response = $e->getResponse();
+            $this->assertEquals(401, $response->getStatusCode());
+        }
+        $this->assertSame($expectedSuccess, $success);
+    }
 }