Merge pull request #3246 from tractorcow/pulls/3.1/remove-basicauth-isdev

BUG using isDev query string no longer triggers basic auth

control/Director.php

@@ -979,29 +979,21 @@ class Director implements TemplateGlobalProvider {
 	/*
 	 * This function will return true if the site is in a live environment.
 	 * For information about environment types, see {@link Director::set_environment_type()}.
-	 *
-	 * @param $skipDatabase Skips database checks for current login permissions if set to TRUE,
-	 * which is useful for checks happening before the database is functional.
 	 */
-	public static function isLive($skipDatabase = false) {
+	public static function isLive() {
-		return !(Director::isDev($skipDatabase) || Director::isTest($skipDatabase));
+		return !(Director::isDev() || Director::isTest());
 	}
 	
 	/**
 	 * This function will return true if the site is in a development environment.
 	 * For information about environment types, see {@link Director::set_environment_type()}.
-	 * 
-	 * @param $skipDatabase Skips database checks for current login permissions if set to TRUE,
-	 * which is useful for checks happening before the database is functional.
 	 */
-	public static function isDev($skipDatabase = false) {
+	public static function isDev() {
-		// This variable is used to supress repetitions of the isDev security message below.
+		// Check session
-		static $firstTimeCheckingGetVar = true;
+		if($env = self::session_environment()) return $env === 'dev';
-
+		
-		$result = false;
+		// Check config
-
+		if(Config::inst()->get('Director', 'environment_type') === 'dev') return true;
-		if(isset($_SESSION['isDev']) && $_SESSION['isDev']) $result = true;
-		if(Config::inst()->get('Director', 'environment_type') == 'dev') $result = true;
 
 		// Check if we are running on one of the test servers
 		$devServers = (array)Config::inst()->get('Director', 'dev_servers');
@@ -1009,54 +1001,22 @@ class Director implements TemplateGlobalProvider {
 			return true;
 		}
 
-		// Use ?isDev=1 to get development access on the live server
+		return false;
-		if(!$skipDatabase && !$result && isset($_GET['isDev'])) {
-			if(Security::database_is_ready()) {
-				if($firstTimeCheckingGetVar && !Permission::check('ADMIN')){
-					BasicAuth::requireLogin("SilverStripe developer access. Use your CMS login", "ADMIN");
-				}
-				$_SESSION['isDev'] = $_GET['isDev'];
-				$firstTimeCheckingGetVar = false;
-				$result = $_GET['isDev'];
-			} else {
-				if($firstTimeCheckingGetVar && DB::connection_attempted()) {
-					echo "<p style=\"padding: 3px; margin: 3px; background-color: orange; 
-						color: white; font-weight: bold\">Sorry, you can't use ?isDev=1 until your
-						Member and Group tables database are available.  Perhaps your database
-						connection is failing?</p>";
-					$firstTimeCheckingGetVar = false;
-				}
-			}
-		}
-
-		return $result;
 	}
 	
 	/**
 	 * This function will return true if the site is in a test environment.
 	 * For information about environment types, see {@link Director::set_environment_type()}.
-	 *
-	 * @param $skipDatabase Skips database checks for current login permissions if set to TRUE,
-	 * which is useful for checks happening before the database is functional.
 	 */
-	public static function isTest($skipDatabase = false) {
+	public static function isTest() {
-		// Use ?isTest=1 to get test access on the live server, or explicitly set your environment
+		// In case of isDev and isTest both being set, dev has higher priority
-		if(!$skipDatabase && isset($_GET['isTest'])) {
+		if(self::isDev()) return false;
-			if(Security::database_is_ready()) {
-				BasicAuth::requireLogin("SilverStripe developer access. Use your CMS login", "ADMIN");
-				$_SESSION['isTest'] = $_GET['isTest'];
-			} else {
-				return true;
-			}
-		}
 		
-		if(self::isDev($skipDatabase)) {
+		// Check saved session
-			return false;
+		if($env = self::session_environment()) return $env === 'test';
-		}
 		
-		if(Config::inst()->get('Director', 'environment_type')) {
+		// Check config
-			return Config::inst()->get('Director', 'environment_type') == 'test';
+		if(Config::inst()->get('Director', 'environment_type') === 'test') return true;
-		}
 		
 		// Check if we are running on one of the test servers
 		$testServers = (array)Config::inst()->get('Director', 'test_servers');
@@ -1066,6 +1026,36 @@ class Director implements TemplateGlobalProvider {
 		
 		return false;
 	}
+	
+	/**
+	 * Check or update any temporary environment specified in the session
+	 * 
+	 * @return string 'test', 'dev', or null
+	 */
+	protected static function session_environment() {
+		// Set session from querystring
+		if(isset($_GET['isDev'])) {
+			if(isset($_SESSION)) {
+				unset($_SESSION['isTest']); // In case we are changing from test mode
+				$_SESSION['isDev'] = $_GET['isDev'];
+			}
+			return 'dev';
+		} elseif(isset($_GET['isTest'])) {
+			if(isset($_SESSION)) {
+				unset($_SESSION['isDev']); // In case we are changing from dev mode
+				$_SESSION['isTest'] = $_GET['isTest'];
+			}
+			return 'test';
+		}
+		// Check session
+		if(isset($_SESSION['isDev']) && $_SESSION['isDev']) {
+			return 'dev';
+		} elseif(isset($_SESSION['isTest']) && $_SESSION['isTest']) {
+			return 'test';
+		} else {
+			return null;
+		}
+	}
 
 	/**
 	 * @return array Returns an array of strings of the method names of methods on the call that should be exposed

core/startup/ParameterConfirmationToken.php

@@ -54,14 +54,49 @@ class ParameterConfirmationToken {
 		// If a token was provided, but isn't valid, ignore it
 		if ($this->token && (!$this->checkToken($this->token))) $this->token = null;
 	}
+	
+	/**
+	 * Get the name of this token
+	 * 
+	 * @return string
+	 */
+	public function getName() {
+		return $this->parameterName;
+	}
 
+	/**
+	 * Is the parameter requested?
+	 * 
+	 * @return bool
+	 */
 	public function parameterProvided() {
 		return $this->parameter !== null;
 	}
 
+	/**
+	 * Is the necessary token provided for this parameter?
+	 * 
+	 * @return bool
+	 */
 	public function tokenProvided() {
 		return $this->token !== null;
 	}
+	
+	/**
+	 * Is this parameter requested without a valid token?
+	 * 
+	 * @return bool True if the parameter is given without a valid token
+	 */
+	public function reloadRequired() {
+		return $this->parameterProvided() && !$this->tokenProvided();
+	}
+	
+	/**
+	 * Suppress the current parameter by unsetting it from $_GET
+	 */
+	public function suppress() {
+		unset($_GET[$this->parameterName]);
+	}
 
 	public function params() {
 		return array(
@@ -139,4 +174,24 @@ You are being redirected. If you are not redirected soon, <a href='$location'>cl
 		else header('location: '.$location, true, 302);
 		die;
 	}
+	
+	/**
+	 * Given a list of token names, suppress all tokens that have not been validated, and 
+	 * return the non-validated token with the highest priority
+	 * 
+	 * @param type $keys List of token keys in ascending priority (low to high)
+	 * @return ParameterConfirmationToken The token container for the unvalidated $key given with the highest priority
+	 */
+	public static function prepare_tokens($keys) {
+		$target = null;
+		foreach($keys as $key) {
+			$token = new ParameterConfirmationToken($key);
+			// Validate this token
+			if($token->reloadRequired()) {
+				$token->suppress();
+				$target = $token;
+			}
+		}
+		return $target;
+	}
 }

docs/en/changelogs/3.1.6.md

@@ -0,0 +1,9 @@
+# 3.1.6
+
+## Upgrading
+
+ * The use of the isDev or isTest query string parameter is now restricted to those logged in as admin,
+   and requires users to login via the front end form rather than using basic authentication. This
+   follows the same process as the use of the 'flush' query string parameter, and will redirect
+   requests containing this argument with a token in the querystring.
+   Director::isDev, Director::isTest, and Director::isLive no longer have any parameters.

main.php

@@ -110,18 +110,13 @@ if (substr(strtolower($url), 0, strlen(BASE_URL)) == strtolower(BASE_URL)) $url
 require_once('core/startup/ErrorControlChain.php');
 require_once('core/startup/ParameterConfirmationToken.php');
 
+// Prepare tokens and execute chain
+$reloadToken = ParameterConfirmationToken::prepare_tokens(array('isTest', 'isDev', 'flush'));
 $chain = new ErrorControlChain();
-$token = new ParameterConfirmationToken('flush');
-
 $chain
-	->then(function($chain) use ($token){
+	->then(function($chain) use ($reloadToken) {
-		// First, if $_GET['flush'] was set, but no valid token, suppress the flush
+		// If no redirection is necessary then we can disable error supression
-		if (isset($_GET['flush']) && !$token->tokenProvided()) {
+		if (!$reloadToken) $chain->setSuppression(false);
-			unset($_GET['flush']);
-		}
-		else {
-			$chain->setSuppression(false);
-		}
 
 		// Load in core
 		require_once('core/Core.php');
@@ -130,38 +125,30 @@ $chain
 		require_once('model/DB.php');
 		global $databaseConfig;
 		if ($databaseConfig) DB::connect($databaseConfig);
-
+		
-		// Then if a flush was requested, redirect to it
+		// Check if a token is requesting a redirect
-		if ($token->parameterProvided() && !$token->tokenProvided()) {
+		if (!$reloadToken) return;
-			// First, check if we're in dev mode, or the database doesn't have any security data
+		
-			$canFlush = Director::isDev(true) || !Security::database_is_ready();
+		// Otherwise, we start up the session if needed
-
+		if(!isset($_SESSION) && Session::request_contains_session_id()) {
-			// Otherwise, we start up the session if needed, then check for admin
+			Session::start();
-			if (!$canFlush) {
-				if(!isset($_SESSION) && Session::request_contains_session_id()) {
-					Session::start();
-				}
-
-				if (Permission::check('ADMIN')) {
-					$canFlush = true;
-				}
-				else {
-					$loginPage = Director::absoluteURL(Config::inst()->get('Security', 'login_url'));
-					$loginPage .= "?BackURL=" . urlencode($_SERVER['REQUEST_URI']);
-
-					header('location: '.$loginPage, true, 302);
-					die;
-				}
-			}
-
-			// And if we can flush, reload with an authority token
-			if ($canFlush) $token->reloadWithToken();
 		}
+		
+		// Next, check if we're in dev mode, or the database doesn't have any security data, or we are admin
+		if (Director::isDev() || !Security::database_is_ready() || Permission::check('ADMIN')) {
+			return $reloadToken->reloadWithToken();
+		}
+
+		// Fail and redirect the user to the login page
+		$loginPage = Director::absoluteURL(Config::inst()->get('Security', 'login_url'));
+		$loginPage .= "?BackURL=" . urlencode($_SERVER['REQUEST_URI']);
+		header('location: '.$loginPage, true, 302);
+		die;
 	})
-	// Finally if a flush was requested but there was an error while figuring out if it's allowed, do it anyway
+	// Finally if a token was requested but there was an error while figuring out if it's allowed, do it anyway
-	->thenIfErrored(function() use ($token){
+	->thenIfErrored(function() use ($reloadToken){
-		if ($token->parameterProvided() && !$token->tokenProvided()) {
+		if ($reloadToken) {
-			$token->reloadWithToken();
+			$reloadToken->reloadWithToken();
 		}
 	})
 	->execute();

tests/control/DirectorTest.php

@@ -10,6 +10,10 @@ class DirectorTest extends SapphireTest {
 	protected static $originalRequestURI;
 
 	protected $originalProtocolHeaders = array();
+	
+	protected $originalGet = array();
+	
+	protected $originalSession = array();
 
 	public function setUp() {
 		parent::setUp();
@@ -22,6 +26,9 @@ class DirectorTest extends SapphireTest {
 			self::$originalRequestURI = $_SERVER['REQUEST_URI'];
 		}
 		
+		$this->originalGet = $_GET;
+		$this->originalSession = $_SESSION;
+		
 		Config::inst()->update('Director', 'rules', array(
 			'DirectorTestRule/$Action/$ID/$OtherID' => 'DirectorTestRequest_Controller',
 			'en-nz/$Action/$ID/$OtherID' => array(
@@ -47,6 +54,9 @@ class DirectorTest extends SapphireTest {
 		// Remove base URL override (setting to false reverts to default behaviour)
 		Config::inst()->update('Director', 'alternate_base_url', false);
 		
+		$_GET = $this->originalGet;
+		$_SESSION = $this->originalSession;
+		
 		// Reinstate the original REQUEST_URI after it was modified by some tests
 		$_SERVER['REQUEST_URI'] = self::$originalRequestURI;
 
@@ -221,6 +231,41 @@ class DirectorTest extends SapphireTest {
 		$this->assertFalse(Director::is_site_url("//test.com?url=" . Director::absoluteBaseURL()));
 	}
 	
+	/**
+	 * Tests isDev, isTest, isLive set from querystring
+	 */
+	public function testQueryIsEnvironment() {
+		// Reset
+		unset($_SESSION['isDev']);
+		unset($_SESSION['isLive']);
+		unset($_GET['isTest']);
+		unset($_GET['isDev']);
+		
+		// Test isDev=1
+		$_GET['isDev'] = '1';
+		$this->assertTrue(Director::isDev());
+		$this->assertFalse(Director::isTest());
+		$this->assertFalse(Director::isLive());
+		
+		// Test persistence
+		unset($_GET['isDev']);
+		$this->assertTrue(Director::isDev());
+		$this->assertFalse(Director::isTest());
+		$this->assertFalse(Director::isLive());
+		
+		// Test change to isTest
+		$_GET['isTest'] = '1';
+		$this->assertFalse(Director::isDev());
+		$this->assertTrue(Director::isTest());
+		$this->assertFalse(Director::isLive());
+		
+		// Test persistence
+		unset($_GET['isTest']);
+		$this->assertFalse(Director::isDev());
+		$this->assertTrue(Director::isTest());
+		$this->assertFalse(Director::isLive());
+	}
+	
 	public function testResetGlobalsAfterTestRequest() {
 		$_GET = array('somekey' => 'getvalue');
 		$_POST = array('somekey' => 'postvalue');

tests/core/startup/ParameterConfirmationTokenTest.php

@@ -1,11 +1,24 @@
 <?php
 
-class ParameterConfirmationTokenTest_Token extends ParameterConfirmationToken {
+/**
-
+ * Dummy parameter token
+ */
+class ParameterConfirmationTokenTest_Token extends ParameterConfirmationToken implements TestOnly {
+	
 	public function currentAbsoluteURL() {
 		return parent::currentAbsoluteURL();
 	}
+}
 
+
+/**
+ * A token that always validates a given token
+ */
+class ParameterConfirmationTokenTest_ValidToken extends ParameterConfirmationTokenTest_Token {
+
+	protected function checkToken($token) {
+		return true;
+	}
 }
 
 class ParameterConfirmationTokenTest extends SapphireTest {
@@ -18,6 +31,66 @@ class ParameterConfirmationTokenTest extends SapphireTest {
 
 		return array($answer, $slash);
 	}
+	
+	public function setUp() {
+		parent::setUp();
+		$_GET['parameterconfirmationtokentest_notoken'] = 'value';
+		$_GET['parameterconfirmationtokentest_empty'] = '';
+		$_GET['parameterconfirmationtokentest_withtoken'] = '1';
+		$_GET['parameterconfirmationtokentest_withtokentoken'] = 'dummy';
+	}
+	
+	public function tearDown() {
+		foreach($_GET as $param) {
+			if(stripos($param, 'parameterconfirmationtokentest_') === 0) unset($_GET[$param]);
+		}
+		parent::tearDown();
+	}
+	
+	public function testParameterDetectsParameters() {
+		$withoutToken = new ParameterConfirmationTokenTest_Token('parameterconfirmationtokentest_notoken');
+		$emptyParameter = new ParameterConfirmationTokenTest_Token('parameterconfirmationtokentest_empty');
+		$withToken = new ParameterConfirmationTokenTest_ValidToken('parameterconfirmationtokentest_withtoken');
+		$withoutParameter = new ParameterConfirmationTokenTest_Token('parameterconfirmationtokentest_noparam');
+		
+		// Check parameter
+		$this->assertTrue($withoutToken->parameterProvided());
+		$this->assertTrue($emptyParameter->parameterProvided());  // even if empty, it's still provided
+		$this->assertTrue($withToken->parameterProvided());
+		$this->assertFalse($withoutParameter->parameterProvided());
+		
+		// Check token
+		$this->assertFalse($withoutToken->tokenProvided());
+		$this->assertFalse($emptyParameter->tokenProvided());
+		$this->assertTrue($withToken->tokenProvided());
+		$this->assertFalse($withoutParameter->tokenProvided());
+		
+		// Check if reload is required
+		$this->assertTrue($withoutToken->reloadRequired());
+		$this->assertTrue($emptyParameter->reloadRequired());
+		$this->assertFalse($withToken->reloadRequired());
+		$this->assertFalse($withoutParameter->reloadRequired());
+		
+		// Check suppression
+		$this->assertTrue(isset($_GET['parameterconfirmationtokentest_notoken']));
+		$withoutToken->suppress();
+		$this->assertFalse(isset($_GET['parameterconfirmationtokentest_notoken']));
+	}
+	
+	public function testPrepareTokens() {
+		// Test priority ordering
+		$token = ParameterConfirmationToken::prepare_tokens(array(
+			'parameterconfirmationtokentest_notoken',
+			'parameterconfirmationtokentest_empty',
+			'parameterconfirmationtokentest_noparam'
+		));
+		// Test no invalid tokens
+		$this->assertEquals('parameterconfirmationtokentest_empty', $token->getName());
+		$token = ParameterConfirmationToken::prepare_tokens(array(
+			'parameterconfirmationtokentest_noparam'
+		));
+		$this->assertEmpty($token);
+	}
 
 	/**
 	 * currentAbsoluteURL needs to handle base or url being missing, or any combination of slashes.
@@ -25,7 +98,7 @@ class ParameterConfirmationTokenTest extends SapphireTest {
 	 * There should always be exactly one slash between each part in the result, and any trailing slash
 	 * should be preserved.
 	 */
-	function testCurrentAbsoluteURLHandlesSlashes() {
+	public function testCurrentAbsoluteURLHandlesSlashes() {
 		global $url;
 
 		$token = new ParameterConfirmationTokenTest_Token('parameterconfirmationtokentest_parameter');