From 5224d659cfc8b58ae21846e72e7b9d39be74cd0d Mon Sep 17 00:00:00 2001
From: Steve Boyd <emteknetnz@gmail.com>
Date: Wed, 15 Jan 2025 16:29:35 +1300
Subject: [PATCH] FIX Allow logged in user with permissions to bypass basic
 auth

---
 composer.json                    |  2 +-
 src/EnvironmentChecker.php       | 12 ++---
 tests/EnvironmentCheckerTest.php | 83 ++++++++++++++++++++++++++++++++
 3 files changed, 89 insertions(+), 8 deletions(-)

diff --git a/composer.json b/composer.json
index d3d793e..3882f61 100644
--- a/composer.json
+++ b/composer.json
@@ -21,7 +21,7 @@
     ],
     "require": {
         "php": "^8.1",
-        "silverstripe/framework": "^5",
+        "silverstripe/framework": "^5.3",
         "silverstripe/versioned": "^2",
         "guzzlehttp/guzzle": "^7"
     },
diff --git a/src/EnvironmentChecker.php b/src/EnvironmentChecker.php
index 4005d81..b8b1bff 100644
--- a/src/EnvironmentChecker.php
+++ b/src/EnvironmentChecker.php
@@ -103,6 +103,11 @@ 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')
@@ -117,13 +122,6 @@ public function init($permission = 'ADMIN')
                 $response->addHeader('WWW-Authenticate', "Basic realm=\"Environment check\"");
                 throw new HTTPResponse_Exception($response);
             }
-        } 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);
         }
     }
 
diff --git a/tests/EnvironmentCheckerTest.php b/tests/EnvironmentCheckerTest.php
index f8e2c02..cb1c5f8 100644
--- a/tests/EnvironmentCheckerTest.php
+++ b/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,9 @@
 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;
 
 /**
  * Class EnvironmentCheckerTest
@@ -94,4 +98,83 @@ 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
+        $_SERVER['PHP_AUTH_USER'] = 'test';
+        $_SERVER['PHP_AUTH_PW'] = $validCreds ? 'password' : 'invalid';
+        // Run test
+        $checker = new EnvironmentChecker('test suite', 'test');
+        $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);
+    }
 }