API HTTPRequestBuilder::createFromEnvironment() now cleans up live gl…

…obals BUG Fix issue with SSViewer Fix Security / View tests
open-sausages · Jun 21, 2017 · 5d235e6 · 5d235e6
1 parent d88d4ed
commit 5d235e6
Show file tree

Hide file tree

Showing 30 changed files with 228 additions and 147 deletions.
diff --git a/src/Control/CLIRequestBuilder.php b/src/Control/CLIRequestBuilder.php
@@ -7,7 +7,7 @@
  */
 class CLIRequestBuilder extends HTTPRequestBuilder
 {
-    protected static function cleanEnvironment(array $variables)
+    public static function cleanEnvironment(array $variables)
     {
         // Create all blank vars
         foreach (['_REQUEST', '_GET', '_POST', '_SESSION', '_SERVER', '_COOKIE', '_ENV', '_FILES'] as $key) {

diff --git a/src/Control/Director.php b/src/Control/Director.php
@@ -315,6 +315,9 @@ public static function mockRequest(
         $newVars['_SERVER']['REQUEST_URI'] = Director::baseURL() . $url;
         $newVars['_REQUEST'] = array_merge($newVars['_GET'], $newVars['_POST']);
 
+        // Normalise vars
+        $newVars = HTTPRequestBuilder::cleanEnvironment($newVars);
+
         // Create new request
         $request = HTTPRequestBuilder::createFromVariables($newVars, $body);
         if ($headers) {

diff --git a/src/Control/HTTPRequestBuilder.php b/src/Control/HTTPRequestBuilder.php
@@ -15,8 +15,12 @@ class HTTPRequestBuilder
      */
     public static function createFromEnvironment()
     {
+        // Clean and update live global variables
+        $variables = static::cleanEnvironment(Environment::getVariables());
+        Environment::setVariables($variables); // Currently necessary for SSViewer, etc to work
+
         // Health-check prior to creating environment
-        return static::createFromVariables(Environment::getVariables(), @file_get_contents('php://input'));
+        return static::createFromVariables($variables, @file_get_contents('php://input'));
     }
 
     /**
@@ -28,8 +32,6 @@ public static function createFromEnvironment()
      */
     public static function createFromVariables(array $variables, $input)
     {
-        $variables = static::cleanEnvironment($variables);
-
         // Strip `url` out of querystring
         $url = $variables['_GET']['url'];
         unset($variables['_GET']['url']);
@@ -92,7 +94,7 @@ protected static function extractRequestHeaders(array $server)
      * @param array $variables
      * @return array Cleaned variables
      */
-    protected static function cleanEnvironment(array $variables)
+    public static function cleanEnvironment(array $variables)
     {
         // IIS will sometimes generate this.
         if (!empty($variables['_SERVER']['HTTP_X_ORIGINAL_URL'])) {

diff --git a/src/Core/TempFolder.php b/src/Core/TempFolder.php
@@ -34,7 +34,7 @@ public static function getTempFolder($base)
      *
      * @return string
      */
-    protected static function getTempFolderUsername()
+    public static function getTempFolderUsername()
     {
         $user = getenv('APACHE_RUN_USER');
         if (!$user) {

diff --git a/src/Dev/CSSContentParser.php b/src/Dev/CSSContentParser.php
@@ -69,7 +69,7 @@ public function __construct($content)
      * See {@link getByXpath()} for a more direct selector syntax.
      *
      * @param String $selector
-     * @return SimpleXMLElement
+     * @return SimpleXMLElement[]
      */
     public function getBySelector($selector)
     {
@@ -81,7 +81,7 @@ public function getBySelector($selector)
      * Allows querying the content through XPATH selectors.
      *
      * @param String $xpath SimpleXML compatible XPATH statement
-     * @return SimpleXMLElement|false
+     * @return SimpleXMLElement[]
      */
     public function getByXpath($xpath)
     {

diff --git a/src/Dev/SapphireTest.php b/src/Dev/SapphireTest.php
@@ -6,6 +6,7 @@
 use LogicException;
 use PHPUnit_Framework_TestCase;
 use SilverStripe\CMS\Controllers\RootURLController;
+use SilverStripe\Control\CLIRequestBuilder;
 use SilverStripe\Control\Controller;
 use SilverStripe\Control\Cookie;
 use SilverStripe\Control\Director;
@@ -889,9 +890,8 @@ public static function start()
         static::set_is_running_test(true);
 
         // Mock request
-        $session = new Session(isset($_SESSION) ? $_SESSION : array());
-        $request = new HTTPRequest('GET', '/');
-        $request->setSession($session);
+        $_SERVER['argv'] = ['vendor/bin/phpunit', '/'];
+        $request = CLIRequestBuilder::createFromEnvironment();
 
         // Test application
         $kernel = new TestKernel(BASE_PATH);

diff --git a/src/Forms/Form.php b/src/Forms/Form.php
@@ -2,9 +2,12 @@
 
 namespace SilverStripe\Forms;
 
+use BadMethodCallException;
 use SilverStripe\Control\Controller;
 use SilverStripe\Control\HasRequestHandler;
 use SilverStripe\Control\HTTP;
+use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Control\NullHTTPRequest;
 use SilverStripe\Control\RequestHandler;
 use SilverStripe\Control\Session;
 use SilverStripe\Core\ClassInfo;
@@ -341,16 +344,37 @@ public function clearFormState()
         return $this;
     }
 
+    /**
+     * Helper to get current request for this form
+     *
+     * @return HTTPRequest
+     */
+    protected function getRequest()
+    {
+        // Check if current request handler has a request object
+        $controller = $this->getController();
+        if ($controller && !($controller->getRequest() instanceof NullHTTPRequest)) {
+            return $controller->getRequest();
+        }
+        // Fall back to current controller
+        if (Controller::has_curr() && !(Controller::curr()->getRequest() instanceof NullHTTPRequest)) {
+            return Controller::curr()->getRequest();
+        }
+        return null;
+    }
+
     /**
      * Get session for this form
      *
      * @return Session
      */
     protected function getSession()
     {
-        // Note: Session may not be available if this form doesn't have a request handler
-        $controller = $this->getController() ?: Controller::curr();
-        return $controller->getRequest()->getSession();
+        $request = $this->getRequest();
+        if ($request) {
+            return $request->getSession();
+        }
+        throw new BadMethodCallException("Session not available in the current context");
     }
 
     /**

diff --git a/src/Security/BasicAuth.php b/src/Security/BasicAuth.php
@@ -8,7 +8,7 @@
 use SilverStripe\Control\HTTPResponse;
 use SilverStripe\Control\HTTPResponse_Exception;
 use SilverStripe\Core\Config\Configurable;
-use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Dev\Debug;
 use SilverStripe\Security\MemberAuthenticator\MemberAuthenticator;
 
 /**

diff --git a/src/Security/DefaultAdminService.php b/src/Security/DefaultAdminService.php
@@ -18,9 +18,12 @@ class DefaultAdminService
     use Injectable;
 
     /**
-     * @var bool
+     * Can be set to explicitly true or false, or left null.
+     * If null, hasDefaultAdmin() will be inferred from environment.
+     *
+     * @var bool|null
      */
-    protected static $has_default_admin = false;
+    protected static $has_default_admin = null;
 
     /**
      * @var string
@@ -72,7 +75,7 @@ public static function getDefaultAdminUsername()
                 "No default admin configured. Please call hasDefaultAdmin() before getting default admin username"
             );
         }
-        return static::$default_username;
+        return static::$default_username ?: getenv('SS_DEFAULT_ADMIN_USERNAME');
     }
 
     /**
@@ -86,7 +89,7 @@ public static function getDefaultAdminPassword()
                 "No default admin configured. Please call hasDefaultAdmin() before getting default admin password"
             );
         }
-        return static::$default_password;
+        return static::$default_password ?: getenv('SS_DEFAULT_ADMIN_PASSWORD');
     }
 
     /**
@@ -96,11 +99,16 @@ public static function getDefaultAdminPassword()
      */
     public static function hasDefaultAdmin()
     {
+        // Check environment if not explicitly set
+        if (!isset(static::$has_default_admin)) {
+            return !empty(getenv('SS_DEFAULT_ADMIN_USERNAME'))
+                && !empty(getenv('SS_DEFAULT_ADMIN_PASSWORD'));
+        }
         return static::$has_default_admin;
     }
 
     /**
-     * Flush the default admin credentials
+     * Flush the default admin credentials.
      */
     public static function clearDefaultAdmin()
     {

diff --git a/src/Security/MemberAuthenticator/MemberAuthenticator.php b/src/Security/MemberAuthenticator/MemberAuthenticator.php
@@ -3,17 +3,16 @@
 namespace SilverStripe\Security\MemberAuthenticator;
 
 use InvalidArgumentException;
-use SilverStripe\Control\Controller;
-use SilverStripe\Control\Session;
-use SilverStripe\Core\Extensible;
 use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Core\Extensible;
+use SilverStripe\Dev\Debug;
 use SilverStripe\ORM\ValidationResult;
 use SilverStripe\Security\Authenticator;
+use SilverStripe\Security\DefaultAdminService;
 use SilverStripe\Security\LoginAttempt;
 use SilverStripe\Security\Member;
 use SilverStripe\Security\PasswordEncryptor;
 use SilverStripe\Security\Security;
-use SilverStripe\Security\DefaultAdminService;
 
 /**
  * Authenticator for the default "member" method
@@ -69,15 +68,15 @@ protected function authenticateMember($data, ValidationResult &$result = null, M
             if ($result->isValid()) {
                 // Check if default admin credentials are correct
                 if (DefaultAdminService::isDefaultAdminCredentials($email, $data['Password'])) {
-                return $member;
-            } else {
-                $result->addError(_t(
-                    'SilverStripe\\Security\\Member.ERRORWRONGCRED',
-                    "The provided details don't seem to be correct. Please try again."
-                ));
+                    return $member;
+                } else {
+                    $result->addError(_t(
+                        'SilverStripe\\Security\\Member.ERRORWRONGCRED',
+                        "The provided details don't seem to be correct. Please try again."
+                    ));
+                }
             }
         }
-        }
 
         // Attempt to identify user by email
         if (!$member && $email) {

diff --git a/src/Security/MemberAuthenticator/MemberLoginForm.php b/src/Security/MemberAuthenticator/MemberLoginForm.php
@@ -4,7 +4,6 @@
 
 use SilverStripe\Control\Director;
 use SilverStripe\Control\RequestHandler;
-use SilverStripe\Control\Session;
 use SilverStripe\Forms\CheckboxField;
 use SilverStripe\Forms\FieldList;
 use SilverStripe\Forms\FormAction;
@@ -124,11 +123,11 @@ public function __construct(
      */
     protected function getFormFields()
     {
-        $request = $this->getController()->getRequest();
+        $request = $this->getRequest();
         if ($request->getVar('BackURL')) {
             $backURL = $request->getVar('BackURL');
         } else {
-            $backURL = Session::get('BackURL');
+            $backURL = $request->getSession()->get('BackURL');
         }
 
         $label = Member::singleton()->fieldLabel(Member::config()->get('unique_identifier_field'));
@@ -191,11 +190,13 @@ protected function getFormActions()
         return $actions;
     }
 
+
+
     public function restoreFormState()
     {
         parent::restoreFormState();
 
-        $session = Controller::curr()->getRequest()->getSession();
+        $session = $this->getSession();
         $forceMessage = $session->get('MemberLoginForm.force_message');
         if (($member = Security::getCurrentUser()) && !$forceMessage) {
             $message = _t(

diff --git a/src/Security/Security.php b/src/Security/Security.php
@@ -391,7 +391,7 @@ public static function permissionFailure($controller = null, $messageSet = null)
             }
 
             static::singleton()->setLoginMessage($message, ValidationResult::TYPE_WARNING);
-            $loginResponse = static::singleton()->login();
+            $loginResponse = static::singleton()->login($controller ? $controller->getRequest() : $controller);
             if ($loginResponse instanceof HTTPResponse) {
                 return $loginResponse;
             }
@@ -702,16 +702,20 @@ public static function clearLoginMessage()
      */
     public function login($request = null, $service = Authenticator::LOGIN)
     {
+        if ($request) {
+            $this->setRequest($request);
+        } elseif ($request) {
+            $request = $this->getRequest();
+        } else {
+            throw new HTTPResponse_Exception("No request available", 500);
+        }
+
         // Check pre-login process
         if ($response = $this->preLogin()) {
             return $response;
         }
         $authName = null;
 
-        if (!$request) {
-            $request = $this->getRequest();
-        }
-
         if ($request && $request->param('ID')) {
             $authName = $request->param('ID');
         }

diff --git a/src/View/Requirements_Backend.php b/src/View/Requirements_Backend.php
@@ -1325,9 +1325,12 @@ protected function getCombinedFileURL($combinedFile, $fileList, $type)
         if ($minify && !$this->minifier) {
             throw new Exception(
                 sprintf(
-                    'Cannot minify files without a minification service defined.
-        			Set %s::minifyCombinedFiles to false, or inject a %s service on
-        			%s.properties.minifier',
+                    <<<MESSAGE
+Cannot minify files without a minification service defined.
+Set %s::minifyCombinedFiles to false, or inject a %s service on
+%s.properties.minifier
+MESSAGE
+                    ,
                     __CLASS__,
                     Requirements_Minifier::class,
                     __CLASS__

diff --git a/tests/php/Control/PjaxResponseNegotiatorTest.php b/tests/php/Control/PjaxResponseNegotiatorTest.php
@@ -2,9 +2,10 @@
 
 namespace SilverStripe\Control\Tests;
 
-use SilverStripe\Dev\SapphireTest;
-use SilverStripe\Control\PjaxResponseNegotiator;
 use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Control\PjaxResponseNegotiator;
+use SilverStripe\Control\Session;
+use SilverStripe\Dev\SapphireTest;
 
 class PjaxResponseNegotiatorTest extends SapphireTest
 {
@@ -19,6 +20,7 @@ public function testDefaultCallbacks()
             )
         );
         $request = new HTTPRequest('GET', '/'); // not setting pjax header
+        $request->setSession(new Session([]));
         $response = $negotiator->respond($request);
         $this->assertEquals('default response', $response->getBody());
     }
@@ -36,6 +38,7 @@ public function testSelectsFragmentByHeader()
             )
         );
         $request = new HTTPRequest('GET', '/');
+        $request->setSession(new Session([]));
         $request->addHeader('X-Pjax', 'myfragment');
         $response = $negotiator->respond($request);
         $this->assertEquals('{"myfragment":"myfragment response"}', $response->getBody());
@@ -57,6 +60,7 @@ public function testMultipleFragments()
             )
         );
         $request = new HTTPRequest('GET', '/');
+        $request->setSession(new Session([]));
         $request->addHeader('X-Pjax', 'myfragment,otherfragment');
         $request->addHeader('Accept', 'text/json');
         $response = $negotiator->respond($request);
@@ -81,6 +85,7 @@ public function testFragmentsOverride()
         );
 
         $request = new HTTPRequest('GET', '/');
+        $request->setSession(new Session([]));
         $request->addHeader('X-Pjax', 'alpha');
         $request->addHeader('Accept', 'text/json');