From c938dc00aa07db4ab92792892b59c68cf59f56ac Mon Sep 17 00:00:00 2001 From: Nathaniel Catchpole Date: Mon, 8 Feb 2016 21:21:14 +0900 Subject: [PATCH] Issue #2662990 by alexpott: Flood's database backend is a core service but it depends on system install --- .../lib/Drupal/Core/Flood/DatabaseBackend.php | 168 ++++++++++++++++-- .../contact/src/Tests/ContactPersonalTest.php | 5 - .../contact/src/Tests/ContactSitewideTest.php | 4 - .../KeyValueStore/GarbageCollectionTest.php | 2 +- core/modules/system/system.install | 42 ----- 5 files changed, 153 insertions(+), 68 deletions(-) diff --git a/core/lib/Drupal/Core/Flood/DatabaseBackend.php b/core/lib/Drupal/Core/Flood/DatabaseBackend.php index 4fd4b2b3e5ae..922ff974328d 100644 --- a/core/lib/Drupal/Core/Flood/DatabaseBackend.php +++ b/core/lib/Drupal/Core/Flood/DatabaseBackend.php @@ -7,6 +7,7 @@ namespace Drupal\Core\Flood; +use Drupal\Core\Database\SchemaObjectExistsException; use Symfony\Component\HttpFoundation\RequestStack; use Drupal\Core\Database\Connection; @@ -15,6 +16,11 @@ use Drupal\Core\Database\Connection; */ class DatabaseBackend implements FloodInterface { + /** + * The database table name. + */ + const TABLE_NAME = 'flood'; + /** * The database connection used to store flood event information. * @@ -50,7 +56,35 @@ class DatabaseBackend implements FloodInterface { if (!isset($identifier)) { $identifier = $this->requestStack->getCurrentRequest()->getClientIp(); } - $this->connection->insert('flood') + $try_again = FALSE; + try { + $this->doInsert($name, $window, $identifier); + } + catch (\Exception $e) { + $try_again = $this->ensureTableExists(); + if (!$try_again) { + throw $e; + } + } + if ($try_again) { + $this->doInsert($name, $window, $identifier); + } + } + + /** + * Inserts an event into the flood table + * + * @param string $name + * The name of an event. + * @param int $window + * Number of seconds before this event expires. + * @param string $identifier + * Unique identifier of the current user. + * + * @see \Drupal\Core\Flood\DatabaseBackend::register + */ + protected function doInsert($name, $window, $identifier) { + $this->connection->insert(static::TABLE_NAME) ->fields(array( 'event' => $name, 'identifier' => $identifier, @@ -67,10 +101,15 @@ class DatabaseBackend implements FloodInterface { if (!isset($identifier)) { $identifier = $this->requestStack->getCurrentRequest()->getClientIp(); } - $this->connection->delete('flood') - ->condition('event', $name) - ->condition('identifier', $identifier) - ->execute(); + try { + $this->connection->delete(static::TABLE_NAME) + ->condition('event', $name) + ->condition('identifier', $identifier) + ->execute(); + } + catch (\Exception $e) { + $this->catchException($e); + } } /** @@ -80,23 +119,120 @@ class DatabaseBackend implements FloodInterface { if (!isset($identifier)) { $identifier = $this->requestStack->getCurrentRequest()->getClientIp(); } - $number = $this->connection->select('flood', 'f') - ->condition('event', $name) - ->condition('identifier', $identifier) - ->condition('timestamp', REQUEST_TIME - $window, '>') - ->countQuery() - ->execute() - ->fetchField(); - return ($number < $threshold); + try { + $number = $this->connection->select(static::TABLE_NAME, 'f') + ->condition('event', $name) + ->condition('identifier', $identifier) + ->condition('timestamp', REQUEST_TIME - $window, '>') + ->countQuery() + ->execute() + ->fetchField(); + return ($number < $threshold); + } + catch (\Exception $e) { + $this->catchException($e); + return TRUE; + } } /** * {@inheritdoc} */ public function garbageCollection() { - return $this->connection->delete('flood') - ->condition('expiration', REQUEST_TIME, '<') - ->execute(); + try { + $return = $this->connection->delete(static::TABLE_NAME) + ->condition('expiration', REQUEST_TIME, '<') + ->execute(); + } + catch (\Exception $e) { + $this->catchException($e); + } + } + + /** + * Check if the flood table exists and create it if not. + */ + protected function ensureTableExists() { + try { + $database_schema = $this->connection->schema(); + if (!$database_schema->tableExists(static::TABLE_NAME)) { + $schema_definition = $this->schemaDefinition(); + $database_schema->createTable(static::TABLE_NAME, $schema_definition); + return TRUE; + } + } + // If another process has already created the table, attempting to create + // it will throw an exception. In this case just catch the exception and do + // nothing. + catch (SchemaObjectExistsException $e) { + return TRUE; + } + return FALSE; + } + + /** + * Act on an exception when flood might be stale. + * + * If the table does not yet exist, that's fine, but if the table exists and + * yet the query failed, then the flood is stale and the exception needs to + * propagate. + * + * @param $e + * The exception. + * + * @throws \Exception + */ + protected function catchException(\Exception $e) { + if ($this->connection->schema()->tableExists(static::TABLE_NAME)) { + throw $e; + } + } + + /** + * Defines the schema for the flood table. + */ + public function schemaDefinition() { + return [ + 'description' => 'Flood controls the threshold of events, such as the number of contact attempts.', + 'fields' => [ + 'fid' => [ + 'description' => 'Unique flood event ID.', + 'type' => 'serial', + 'not null' => TRUE, + ], + 'event' => [ + 'description' => 'Name of event (e.g. contact).', + 'type' => 'varchar_ascii', + 'length' => 64, + 'not null' => TRUE, + 'default' => '', + ], + 'identifier' => [ + 'description' => 'Identifier of the visitor, such as an IP address or hostname.', + 'type' => 'varchar_ascii', + 'length' => 128, + 'not null' => TRUE, + 'default' => '', + ], + 'timestamp' => [ + 'description' => 'Timestamp of the event.', + 'type' => 'int', + 'not null' => TRUE, + 'default' => 0, + ], + 'expiration' => [ + 'description' => 'Expiration timestamp. Expired events are purged on cron run.', + 'type' => 'int', + 'not null' => TRUE, + 'default' => 0, + ], + ], + 'primary key' => ['fid'], + 'indexes' => [ + 'allow' => ['event', 'identifier', 'timestamp'], + 'purge' => ['expiration'], + ], + ]; } } diff --git a/core/modules/contact/src/Tests/ContactPersonalTest.php b/core/modules/contact/src/Tests/ContactPersonalTest.php index d13324006d70..8b1a7a128c4c 100644 --- a/core/modules/contact/src/Tests/ContactPersonalTest.php +++ b/core/modules/contact/src/Tests/ContactPersonalTest.php @@ -232,11 +232,6 @@ class ContactPersonalTest extends WebTestBase { $flood_limit = 3; $this->config('contact.settings')->set('flood.limit', $flood_limit)->save(); - // Clear flood table in preparation for flood test and allow other checks to complete. - db_delete('flood')->execute(); - $num_records_flood = db_query("SELECT COUNT(*) FROM {flood}")->fetchField(); - $this->assertIdentical($num_records_flood, '0', 'Flood table emptied.'); - $this->drupalLogin($this->webUser); // Submit contact form with correct values and check flood interval. diff --git a/core/modules/contact/src/Tests/ContactSitewideTest.php b/core/modules/contact/src/Tests/ContactSitewideTest.php index 39cd34a21f39..a29a45b403e5 100644 --- a/core/modules/contact/src/Tests/ContactSitewideTest.php +++ b/core/modules/contact/src/Tests/ContactSitewideTest.php @@ -185,10 +185,6 @@ class ContactSitewideTest extends WebTestBase { $this->assertNoRaw(t('Contact form %label has been added.', array('%label' => $label))); $this->assertRaw(t('The machine-readable name is already in use. It must be unique.')); - // Clear flood table in preparation for flood test and allow other checks to complete. - db_delete('flood')->execute(); - $num_records_after = db_query("SELECT COUNT(*) FROM {flood}")->fetchField(); - $this->assertIdentical($num_records_after, '0', 'Flood table emptied.'); $this->drupalLogout(); // Check to see that anonymous user cannot see contact page without permission. diff --git a/core/modules/system/src/Tests/KeyValueStore/GarbageCollectionTest.php b/core/modules/system/src/Tests/KeyValueStore/GarbageCollectionTest.php index 7afcda719cdb..a1d4467fa3b0 100644 --- a/core/modules/system/src/Tests/KeyValueStore/GarbageCollectionTest.php +++ b/core/modules/system/src/Tests/KeyValueStore/GarbageCollectionTest.php @@ -30,7 +30,7 @@ class GarbageCollectionTest extends KernelTestBase { parent::setUp(); // These additional tables are necessary due to the call to system_cron(). - $this->installSchema('system', array('key_value_expire', 'flood', 'queue')); + $this->installSchema('system', array('key_value_expire', 'queue')); } /** diff --git a/core/modules/system/system.install b/core/modules/system/system.install index 169921e8736a..49b5ff35141e 100644 --- a/core/modules/system/system.install +++ b/core/modules/system/system.install @@ -889,48 +889,6 @@ function system_schema() { ), ); - $schema['flood'] = array( - 'description' => 'Flood controls the threshold of events, such as the number of contact attempts.', - 'fields' => array( - 'fid' => array( - 'description' => 'Unique flood event ID.', - 'type' => 'serial', - 'not null' => TRUE, - ), - 'event' => array( - 'description' => 'Name of event (e.g. contact).', - 'type' => 'varchar_ascii', - 'length' => 64, - 'not null' => TRUE, - 'default' => '', - ), - 'identifier' => array( - 'description' => 'Identifier of the visitor, such as an IP address or hostname.', - 'type' => 'varchar_ascii', - 'length' => 128, - 'not null' => TRUE, - 'default' => '', - ), - 'timestamp' => array( - 'description' => 'Timestamp of the event.', - 'type' => 'int', - 'not null' => TRUE, - 'default' => 0, - ), - 'expiration' => array( - 'description' => 'Expiration timestamp. Expired events are purged on cron run.', - 'type' => 'int', - 'not null' => TRUE, - 'default' => 0, - ), - ), - 'primary key' => array('fid'), - 'indexes' => array( - 'allow' => array('event', 'identifier', 'timestamp'), - 'purge' => array('expiration'), - ), - ); - $schema['key_value'] = array( 'description' => 'Generic key-value storage table. See the state system for an example.', 'fields' => array(