From 8dee93b5239da19849db3c024228eb005920a504 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Fri, 2 Jul 2010 00:13:11 +0000 Subject: [PATCH] BUGFIX Allowing dev/build in "live" mode when Security::database_is_ready() returns FALSE (typically happens when an existing SilverStripe project is upgraded and database columns in Member/Permission/Group have been added) (fixes #4957) MINOR Using Object::create() in DevelopmentAdmin to make objects mockable ENHANCEMENT Added Security::$force_database_is_ready to mock database_is_ready() state ENHANCEMENT Added permission check exception in TaskRunner and DatabaseAdmin if SapphireTest::is_running_test() returns TRUE (necessary for DevelopmentAdminTest) git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/branches/2.4@107415 467b73ca-7a2a-4603-9d3b-597d59a354a9 --- core/model/DatabaseAdmin.php | 4 ++- dev/DebugView.php | 2 +- dev/DevelopmentAdmin.php | 48 +++++++++++++++++++----------------- dev/SapphireTest.php | 12 +++++++++ dev/TaskRunner.php | 8 +++++- security/Security.php | 9 +++++++ 6 files changed, 57 insertions(+), 26 deletions(-) diff --git a/core/model/DatabaseAdmin.php b/core/model/DatabaseAdmin.php index ed991bd90..971c689ee 100644 --- a/core/model/DatabaseAdmin.php +++ b/core/model/DatabaseAdmin.php @@ -32,7 +32,9 @@ class DatabaseAdmin extends Controller { $canAccess = ( Director::isDev() || !Security::database_is_ready() - || Director::is_cli() + // We need to ensure that DevelopmentAdminTest can simulate permission failures when running + // "dev/tests" from CLI. + || (Director::is_cli() && !SapphireTest::is_running_test()) || Permission::check("ADMIN") ); if(!$canAccess) { diff --git a/dev/DebugView.php b/dev/DebugView.php index 10a9fe0ef..1c116ca6c 100644 --- a/dev/DebugView.php +++ b/dev/DebugView.php @@ -11,7 +11,7 @@ * @package sapphire * @subpackage dev */ -class DebugView { +class DebugView extends Object { protected static $error_types = array( E_USER_ERROR => array( diff --git a/dev/DevelopmentAdmin.php b/dev/DevelopmentAdmin.php index b2a67a505..6989b6c3e 100644 --- a/dev/DevelopmentAdmin.php +++ b/dev/DevelopmentAdmin.php @@ -20,17 +20,19 @@ class DevelopmentAdmin extends Controller { function init() { parent::init(); + // Special case for dev/build: Defer permission checks to DatabaseAdmin->init() (see #4957) + $requestedDevBuild = (stripos($this->request->getURL(), 'dev/build') === 0); + // We allow access to this controller regardless of live-status or ADMIN permission only // if on CLI. Access to this controller is always allowed in "dev-mode", or of the user is ADMIN. - $canAccess = (Director::isDev() || Director::is_cli() || Permission::check("ADMIN")); - // Special case for dev/build: Allow unauthenticated building of database, emulate DatabaseAdmin->init() - // permission restrictions (see #4957) - // TODO Decouple sub-controllers like DatabaseAdmin instead of weak URL checking - $requestedDevBuild = (stripos($this->request->getURL(), 'dev/build') === 0 && !Security::database_is_ready()); - - if(!$canAccess && !$requestedDevBuild) { - return Security::permissionFailure($this); - } + $canAccess = ( + $requestedDevBuild + || Director::isDev() + || Director::is_cli() + // Its important that we don't run this check if dev/build was requested + || Permission::check("ADMIN") + ); + if(!$canAccess) return Security::permissionFailure($this); // check for valid url mapping // lacking this information can cause really nasty bugs, @@ -79,7 +81,7 @@ class DevelopmentAdmin extends Controller { // This action is sake-only right now. unset($actions["modules/add"]); - $renderer = new DebugView(); + $renderer = Object::create('DebugView'); $renderer->writeHeader(); $renderer->writeInfo("Sapphire Development Tools", Director::absoluteBaseURL()); $base = Director::baseURL(); @@ -103,33 +105,33 @@ class DevelopmentAdmin extends Controller { } function tests($request) { - return new TestRunner(); + return Object::create('TestRunner'); } function jstests($request) { - return new JSTestRunner(); + return Object::create('JSTestRunner'); } function tasks() { - return new TaskRunner(); + return Object::create('TaskRunner'); } function viewmodel() { - return new ModelViewer(); + return Object::create('ModelViewer'); } - function build() { + function build($request) { if(Director::is_cli()) { - $da = new DatabaseAdmin(); - $da->build(); + $da = Object::create('DatabaseAdmin'); + return $da->handleRequest($request); } else { - $renderer = new DebugView(); + $renderer = Object::create('DebugView'); $renderer->writeHeader(); $renderer->writeInfo("Environment Builder", Director::absoluteBaseURL()); echo "
"; - $da = new DatabaseAdmin(); - $da->build(); + $da = Object::create('DatabaseAdmin'); + return $da->handleRequest($request); echo "
"; $renderer->writeFooter(); @@ -143,10 +145,10 @@ class DevelopmentAdmin extends Controller { * 'build/defaults' => 'buildDefaults', */ function buildDefaults() { - $da = new DatabaseAdmin(); + $da = Object::create('DatabaseAdmin'); if (!Director::is_cli()) { - $renderer = new DebugView(); + $renderer = Object::create('DebugView'); $renderer->writeHeader(); $renderer->writeInfo("Defaults Builder", Director::absoluteBaseURL()); echo "
"; @@ -175,7 +177,7 @@ class DevelopmentAdmin extends Controller { } function viewcode($request) { - return new CodeViewer(); + return Object::create('CodeViewer'); } } ?> \ No newline at end of file diff --git a/dev/SapphireTest.php b/dev/SapphireTest.php index 2f2564ff0..b1284a8aa 100755 --- a/dev/SapphireTest.php +++ b/dev/SapphireTest.php @@ -32,6 +32,9 @@ class SapphireTest extends PHPUnit_Framework_TestCase { protected $mailer; + /** + * @var boolean + */ protected static $is_running_test = false; /** @@ -82,6 +85,15 @@ class SapphireTest extends PHPUnit_Framework_TestCase { */ private $extensionsToReapply = array(), $extensionsToRemove = array(); + /** + * Determines if unit tests are currently run (via {@link TestRunner}). + * This is used as a cheap replacement for fully mockable state + * in certain contiditions (e.g. access checks). + * Caution: When set to FALSE, certain controllers might bypass + * access checks, so this is a very security sensitive setting. + * + * @return boolean + */ public static function is_running_test() { return self::$is_running_test; } diff --git a/dev/TaskRunner.php b/dev/TaskRunner.php index bc6dbe241..f4d9a118b 100644 --- a/dev/TaskRunner.php +++ b/dev/TaskRunner.php @@ -13,7 +13,13 @@ class TaskRunner extends Controller { function init() { parent::init(); - $canAccess = (Director::isDev() || Director::is_cli() || Permission::check("ADMIN")); + $canAccess = ( + Director::isDev() + // We need to ensure that DevelopmentAdminTest can simulate permission failures when running + // "dev/tasks" from CLI. + || (Director::is_cli() && !SapphireTest::is_running_test()) + || Permission::check("ADMIN") + ); if(!$canAccess) return Security::permissionFailure($this); } diff --git a/security/Security.php b/security/Security.php index 72df13db4..ddbd299ff 100644 --- a/security/Security.php +++ b/security/Security.php @@ -98,6 +98,12 @@ class Security extends Controller { */ protected static $login_recording = false; + /** + * @var boolean If set to TRUE or FALSE, {@link database_is_ready()} + * will always return FALSE. Used for unit testing. + */ + static $force_database_is_ready = null; + /** * Set location of word list file * @@ -839,6 +845,9 @@ class Security extends Controller { * @return bool */ public static function database_is_ready() { + // Used for unit tests + if(self::$force_database_is_ready !== NULL) return self::$force_database_is_ready; + $requiredTables = ClassInfo::dataClassesFor('Member'); $requiredTables[] = 'Group'; $requiredTables[] = 'Permission';