Merge pull request #119 from creative-commoners/pulls/3/admin-bypass

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,7 +21,7 @@
     ],
     "require": {
         "php": "^8.1",
-        "silverstripe/framework": "^5",
+        "silverstripe/framework": "^5.3",
         "silverstripe/versioned": "^2",
         "guzzlehttp/guzzle": "^7"
     },
@@ -31,6 +31,9 @@
         "silverstripe/standards": "^1",
         "phpstan/extension-installer": "^1.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,28 +104,44 @@ public function __construct($checkSuiteName, $title)
      */
     public function init($permission = 'ADMIN')
     {
-        // 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')
-            ) {
-                // Fail check with basic auth challenge
-                $response = new HTTPResponse(null, 401);
-                $response->addHeader('WWW-Authenticate', "Basic realm=\"Environment check\"");
-                throw new HTTPResponse_Exception($response);
+        $canAccess = $this->canAccess(null, $permission);
+        // Allow bypassing basic-auth check if the user is logged in with the required permission
+        if ($canAccess) {
+            return;
+        }
+        // If basic auth is not configured, raise a permission failure
+        $basicAuthUsername = Environment::getEnv('ENVCHECK_BASICAUTH_USERNAME');
+        $basicAuthPassword = Environment::getEnv('ENVCHECK_BASICAUTH_PASSWORD');
+        if (!$basicAuthUsername || !$basicAuthPassword) {
+            $response = Security::permissionFailure();
+            throw new HTTPResponse_Exception($response);
+        }
+        // Check basic auth
+        $request = $this->getRequest();
+        $user = $request->getHeader('PHP_AUTH_USER');
+        $pw = $request->getHeader('PHP_AUTH_PW');
+        // Check that submitted basic auth credentials match
+        if ($user !== $basicAuthUsername || $pw !== $basicAuthPassword) {
+            // Fail check with basic auth challenge
+            $response = new HTTPResponse(null, 401);
+            $response->addHeader('WWW-Authenticate', "Basic realm=\"Environment check\"");
+            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.'
+                ));
             }
-        } 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);
+            // Exception is caught by RequestHandler->handleRequest() and will halt further execution
+            $e = new HTTPResponse_Exception(null, 401);
+            $e->setResponse($response);
+            throw $e;
         }
+        // If we get to this point, user has passed basic auth
     }
 
     /**

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,197 @@ 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,
+            ],
+            'logged-out-no-creds' => [
+                'user' => 'logged-out',
+                'validCreds' => null,
+                'expectedSuccess' => false,
+            ],
+            'non-admin-valid-creds' => [
+                'user' => 'non-admin',
+                'validCreds' => true,
+                'expectedSuccess' => true,
+            ],
+            'non-admin-invalid-creds' => [
+                'user' => 'non-admin',
+                'validCreds' => false,
+                'expectedSuccess' => false,
+            ],
+            'non-admin-no-creds' => [
+                'user' => 'non-admin',
+                'validCreds' => null,
+                'expectedSuccess' => false,
+            ],
+            'admin-valid-creds' => [
+                'user' => 'admin',
+                'validCreds' => true,
+                'expectedSuccess' => true,
+            ],
+            'admin-invalid-creds' => [
+                'user' => 'admin',
+                'validCreds' => false,
+                'expectedSuccess' => true,
+            ],
+            'admin-no-creds' => [
+                'user' => 'admin',
+                'validCreds' => null,
+                'expectedSuccess' => true,
+            ],
+        ];
+    }
+
+    /**
+     * @dataProvider provideBasicAuthAdminBypass
+     */
+    public function testBasicAuthAdminBypass(
+        string $user,
+        ?bool $validCreds,
+        bool $expectedSuccess,
+    ): void {
+        $this->setupBasicAuthEnv(true, true);
+        // Log in or out
+        if ($user === 'admin') {
+            $this->logInWithPermission('ADMIN');
+        } elseif ($user === 'non-admin') {
+            $this->logInWithPermission('NOT_AN_ADMIN');
+        } else {
+            $this->logOut();
+        }
+        $request = new HTTPRequest('GET', 'dev/check');
+        if ($validCreds !== null) {
+            // Simulate passing in basic auth creds
+            $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);
+    }
+
+    public static function provideBasicAuthEnvVariables(): array
+    {
+        return [
+            'logged-in-env-vars-set' => [
+                'loggedIn' => true,
+                'setUsernameEnvVar' => true,
+                'setPwEnvVar' => true,
+                'expected' => true,
+            ],
+            'logged-in-env-no-username' => [
+                'loggedIn' => true,
+                'setUsernameEnvVar' => false,
+                'setPwEnvVar' => true,
+                'expected' => true,
+            ],
+            'logged-in-env-no-password' => [
+                'loggedIn' => true,
+                'setUsernameEnvVar' => true,
+                'setPwEnvVar' => false,
+                'expected' => true,
+            ],
+            'logged-in-env-no-username-or-pw' => [
+                'loggedIn' => true,
+                'setUsernameEnvVar' => false,
+                'setPwEnvVar' => false,
+                'expected' => true,
+            ],
+            'logged-out-env-vars-set' => [
+                'loggedIn' => false,
+                'setUsernameEnvVar' => true,
+                'setPwEnvVar' => true,
+                'expected' => true,
+            ],
+            'logged-out-env-no-username' => [
+                'loggedIn' => false,
+                'setUsernameEnvVar' => false,
+                'setPwEnvVar' => true,
+                'expected' => false,
+            ],
+            'logged-out-env-no-password' => [
+                'loggedIn' => false,
+                'setUsernameEnvVar' => true,
+                'setPwEnvVar' => false,
+                'expected' => false,
+            ],
+            'logged-out-env-no-username-or-pw' => [
+                'loggedIn' => false,
+                'setUsernameEnvVar' => false,
+                'setPwEnvVar' => false,
+                'expected' => false,
+            ],
+        ];
+    }
+
+    /**
+     * @dataProvider provideBasicAuthEnvVariables
+     */
+    public function testBasicAuthEnvVariables(
+        bool $loggedIn,
+        bool $setUsernameEnvVar,
+        bool $setPwEnvVar,
+        bool $expected,
+    ): void {
+        $this->setupBasicAuthEnv($setUsernameEnvVar, $setPwEnvVar);
+        if ($loggedIn) {
+            $this->logInWithPermission('ADMIN');
+        } else {
+            $this->logOut();
+        }
+        $request = new HTTPRequest('GET', 'dev/check');
+        $request->addHeader('PHP_AUTH_USER', 'test');
+        $request->addHeader('PHP_AUTH_PW', 'password');
+        // 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(302, $response->getStatusCode());
+        }
+        $this->assertSame($expected, $success);
+    }
+
+    /**
+     * Setup the test environment so that we can use basic auth
+     */
+    private function setupBasicAuthEnv(bool $setUsernameEnvVar, bool $setPwEnvVar): 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');
+        // Set the basic auth env vars
+        Environment::setEnv('ENVCHECK_BASICAUTH_USERNAME', $setUsernameEnvVar ? 'test' : null);
+        Environment::setEnv('ENVCHECK_BASICAUTH_PASSWORD', $setPwEnvVar ? 'password' : null);
+    }
 }