From 9ce5973992f7600bfcbfc63c353c1899a113f545 Mon Sep 17 00:00:00 2001 From: Nathaniel Catchpole Date: Thu, 8 May 2014 13:48:51 +0100 Subject: [PATCH] Issue #2207585 by damiankloip, Xano, dawehner: Find a new OO home for drupal_get_hash_salt(). --- core/includes/bootstrap.inc | 11 ++--- .../Drupal/Core/Access/CsrfTokenGenerator.php | 3 +- core/lib/Drupal/Core/Site/Settings.php | 20 +++++++++ .../Core/Access/CsrfTokenGeneratorTest.php | 45 ++++++++++++------- .../Drupal/Tests/Core/Site/SettingsTest.php | 42 ++++++++++++++++- 5 files changed, 96 insertions(+), 25 deletions(-) diff --git a/core/includes/bootstrap.inc b/core/includes/bootstrap.inc index d00f819d72e..f7ff0b69bc2 100644 --- a/core/includes/bootstrap.inc +++ b/core/includes/bootstrap.inc @@ -1499,15 +1499,12 @@ function drupal_get_user_timezone() { * * @return * A salt based on information in settings.php, not in the database. + * + * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0. Use + * \Drupal\Core\Site\Settings::getHashSalt() instead. */ function drupal_get_hash_salt() { - $hash_salt = Settings::get('hash_salt'); - // This should never happen, as it breaks user logins and many other services. - // Therefore, explicitly notify the user (developer) by throwing an exception. - if (empty($hash_salt)) { - throw new \RuntimeException('Missing $settings[\'hash_salt\'] in settings.php.'); - } - return $hash_salt; + return Settings::getHashSalt(); } /** diff --git a/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php index 7d68750714c..4fa30e2efbf 100644 --- a/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php +++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php @@ -10,6 +10,7 @@ namespace Drupal\Core\Access; use Drupal\Component\Utility\Crypt; use Drupal\Core\PrivateKey; use Drupal\Core\Session\AccountInterface; +use Drupal\Core\Site\Settings; /** * Generates and validates CSRF tokens. @@ -95,7 +96,7 @@ class CsrfTokenGenerator { * 'drupal_private_key' configuration variable. */ protected function computeToken($seed, $value = '') { - return Crypt::hmacBase64($value, $seed . $this->privateKey->get() . drupal_get_hash_salt()); + return Crypt::hmacBase64($value, $seed . $this->privateKey->get() . Settings::getHashSalt()); } } diff --git a/core/lib/Drupal/Core/Site/Settings.php b/core/lib/Drupal/Core/Site/Settings.php index fe2260d7a8b..3ac0f03328c 100644 --- a/core/lib/Drupal/Core/Site/Settings.php +++ b/core/lib/Drupal/Core/Site/Settings.php @@ -80,4 +80,24 @@ final class Settings { return self::$instance->storage; } + /** + * Gets a salt useful for hardening against SQL injection. + * + * @return string + * A salt based on information in settings.php, not in the database. + * + * @throws \RuntimeException + */ + public static function getHashSalt() { + $hash_salt = self::$instance->get('hash_salt'); + // This should never happen, as it breaks user logins and many other + // services. Therefore, explicitly notify the user (developer) by throwing + // an exception. + if (empty($hash_salt)) { + throw new \RuntimeException('Missing $settings[\'hash_salt\'] in settings.php.'); + } + + return $hash_salt; + } + } diff --git a/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php b/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php index 81ac83d5202..5fe25708d9c 100644 --- a/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php +++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php @@ -5,8 +5,9 @@ * Contains \Drupal\Tests\Core\Access\CsrfTokenGeneratorTest. */ -namespace Drupal\Tests\Core\Access { +namespace Drupal\Tests\Core\Access; +use Drupal\Core\Site\Settings; use Drupal\Tests\UnitTestCase; use Drupal\Core\Access\CsrfTokenGenerator; use Drupal\Component\Utility\Crypt; @@ -24,6 +25,13 @@ class CsrfTokenGeneratorTest extends UnitTestCase { */ protected $generator; + /** + * The mock private key instance. + * + * @var \Drupal\Core\PrivateKey|\PHPUnit_Framework_MockObject_MockObject + */ + protected $privateKey; + public static function getInfo() { return array( 'name' => 'CsrfTokenGenerator test', @@ -39,16 +47,22 @@ class CsrfTokenGeneratorTest extends UnitTestCase { parent::setUp(); $this->key = Crypt::randomBytesBase64(55); - $private_key = $this->getMockBuilder('Drupal\Core\PrivateKey') + $this->privateKey = $this->getMockBuilder('Drupal\Core\PrivateKey') ->disableOriginalConstructor() ->setMethods(array('get')) ->getMock(); - $private_key->expects($this->any()) + $this->privateKey->expects($this->any()) ->method('get') ->will($this->returnValue($this->key)); - $this->generator = new CsrfTokenGenerator($private_key); + $settings = array( + 'hash_salt' => $this->randomName(), + ); + + new Settings($settings); + + $this->generator = new CsrfTokenGenerator($this->privateKey); } /** @@ -141,17 +155,16 @@ class CsrfTokenGeneratorTest extends UnitTestCase { ); } -} - -} - -/** - * @todo Remove this when https://drupal.org/node/2036259 is resolved. - */ -namespace { - if (!function_exists('drupal_get_hash_salt')) { - function drupal_get_hash_salt() { - return hash('sha256', 'test_hash_salt'); - } + /** + * Tests the exception thrown when no 'hash_salt' is provided in settings. + * + * @expectedException \RuntimeException + */ + public function testGetWithNoHashSalt() { + // Update settings with no hash salt. + new Settings(array()); + $generator = new CsrfTokenGenerator($this->privateKey); + $generator->get(); } + } diff --git a/core/tests/Drupal/Tests/Core/Site/SettingsTest.php b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php index 541cbe8b537..60388990615 100644 --- a/core/tests/Drupal/Tests/Core/Site/SettingsTest.php +++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php @@ -13,6 +13,8 @@ use Drupal\Tests\UnitTestCase; /** * Tests read-only settings. * + * @group Drupal + * * @coversDefaultClass \Drupal\Core\Site\Settings */ class SettingsTest extends UnitTestCase { @@ -49,6 +51,7 @@ class SettingsTest extends UnitTestCase { $this->config = array( 'one' => '1', 'two' => '2', + 'hash_salt' => $this->randomName(), ); $this->settings = new Settings($this->config); } @@ -58,7 +61,7 @@ class SettingsTest extends UnitTestCase { */ public function testGet() { // Test stored settings. - $this->assertEquals($this->config['one'], Settings::get('one'), 'The correect setting was not returned.'); + $this->assertEquals($this->config['one'], Settings::get('one'), 'The correct setting was not returned.'); $this->assertEquals($this->config['two'], Settings::get('two'), 'The correct setting was not returned.'); // Test setting that isn't stored with default. @@ -81,4 +84,41 @@ class SettingsTest extends UnitTestCase { $this->assertEquals($singleton, $this->settings); } + /** + * Tests Settings::getHashSalt(); + * + * @covers ::getHashSalt + */ + public function testGetHashSalt() { + $this->assertSame($this->config['hash_salt'], $this->settings->getHashSalt()); + } + + /** + * Tests Settings::getHashSalt() with no hash salt value. + * + * @covers ::getHashSalt + * + * @dataProvider providerTestGetHashSaltEmpty + * + * @expectedException \RuntimeException + */ + public function testGetHashSaltEmpty(array $config) { + // Re-create settings with no 'hash_salt' key. + $settings = new Settings($config); + $settings->getHashSalt(); + } + + /** + * Data provider for testGetHashSaltEmpty. + * + * @return array + */ + public function providerTestGetHashSaltEmpty() { + return array( + array(array()), + array(array('hash_salt' => '')), + array(array('hash_salt' => NULL)), + ); + } + }