Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cweagans’s picture

Status: Active » Needs review
FileSize
13.43 KB
Lars Toomre’s picture

This is a great idea @cweagans.

Can we enhance the documentation in the new Flood.php class to match D8 documentation standards? In particular, that would mean adding type hinting to all @param and @return directives as well as starting all variable descriptions that have a default with the phrase '(optional)' as well as stating what the default option is. Such care greatly improves how the class appears in the API documentation system. Thanks!

catch’s picture

Status: Needs review » Needs work

Yeah this is really something that should exist as a standalone class. Few thoughts though:

- every static method is taking $name. We can move that into a constructor and remove the static from the methods
- there should be an interface, then a database implementation of the interface to allow for alternative implementations. Ideally we could inject the database connection into the class as well, although not sure there's any existing examples of that in core yet.
- register the flood system to the DIC similar to #1764474: Make Cache interface and backends use the DIC. I'm a bit concerned about this though since that registration is going to be more expensive than just loading the procedural code would have been, and this is rarely used. So maybe we don't want to register this to the DIC for now, not sure.
- add a garbageCollection method to the flood class, and call that from system_cron() instead of the direct database query that's there now.
- not in this patch but in a follow-up, it'd be great if the flood API could take care of it's own storage similar to #1167144: Make cache backends responsible for their own storage.

cweagans’s picture

- every static method is taking $name. We can move that into a constructor and remove the static from the methods
- there should be an interface, then a database implementation of the interface to allow for alternative implementations. Ideally we could inject the database connection into the class as well, although not sure there's any existing examples of that in core yet.

I'm not sure I agree with this. That feels way over-engineered for what this code is used for. Plus, having the methods static makes a lot of sense when you look at how the methods are used: simple one-line checks, rather than having to instantiate the class first. If this is really a hard requirement to moving cruft out of common.inc, I guess I'll do it, but it doesn't sound like a good idea to me.

@Lars, I won't have time to get back to this in the next couple days, but feel free to reroll the patch with the changes that you'd like.

catch’s picture

The difference with the non-static methods is you can then inject the flood control into unit tests and/or swap out the storage. I agree there's not much reason to do it otherwise but those two are enough.

Lars Toomre’s picture

I am working on this issue. It is my first experience wiring objects with interfaces into Drupal's procedural code.

I have created FloodInterface which the new FloodDatabase implements. However, I am unsure how to use it.

For instance, the patch in #1 did this in contact.pages.inc:

+use Drupal\Core\Utility\Flood;

-  flood_register_event('contact', config('contact.settings')->get('flood.interval'));
+  Flood::registerEvent('contact', config('contact.settings')->get('flood.interval'));

With $name now in the constructor of FloodDatabase, I think we want to do something like:

+use Drupal\Core\Utility\FloodDatabase;

-  flood_register_event('contact', config('contact.settings')->get('flood.interval'));
+  $flood = new Drupal\Core\Utility\FloodDatabase('contact');
+  $flood->registerEvent(config('contact.settings')->get('flood.interval'));

Is this correct? The $flood assignment confuses me.

I am wondering too whether the database implementation should be called FloodDatabase or just Flood. Thoughts??

Lars Toomre’s picture

FileSize
32 KB

Attached is the first draft of a patch that addresses all of the points that @catch raised in #3 (including the last one).

I expect this patch to fail however, as I am unsure how we want to initiate the class so that it swapable. Hence, I left the rest of the static version's implementation as it was from #1.

It also is missing the new file flood.setting.yml file because I do not know what directory to put that function in.

All comments are welcome on how to proceed further.

cweagans’s picture

Assigned: cweagans » Unassigned
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, flood-1795186-7.patch, failed testing.

Lars Toomre’s picture

For now, #1 is the good patch in this issue. #7 is incomplete. I need some feedback before I can advance it further.

cweagans’s picture

@catch, with the patch in #1, everything works exactly as it did before, but the code doesn't live in common.inc anymore. That's the goal of this issue.

Can we keep the re-engineering for a separate issue? Pretty please?

Lars Toomre’s picture

I like the idsea of committing #1 and then moving #3 through #7 to a follow-up issue.

chx’s picture

yes to the issue but totally no to Flood:: . You wanted drupal_container()->get('flood')->....

chx’s picture

And/or a plugin. Yes. I didn't write a D8 plugin yet...

Lars Toomre’s picture

@chx: Off topic, I would enjoy your thoughts and/or input on turmimg actions into a plug-in. #1788104: Convert actions to plugin sub-system

Berdir’s picture

As suggested in #1801962: [meta] Convert custom non-volatile cache-alike things to k/v store, maybe we can use the upcoming expirable key value store for this?

amontero’s picture

If flood has to provide some config UI, http://drupal.org/project/flood_control might prove useful.

marcingy’s picture

Assigned: Unassigned » marcingy

I'll take this, this should be a simple to make it plugable if we base it off file managed conversion.

* Create interface
* Convert the current code into a class
* Add a flood function to instanate the class

Key is lets convert and then do other changes afterwards.

paranojik’s picture

Assigned: marcingy » Unassigned
Status: Needs work » Needs review
FileSize
11.98 KB

This is my take on this patch. It's basically as simple as #1, but implements some suggestions from #3 (and #13).
Pluggability can still be addressed in a follow-up issue.

Status: Needs review » Needs work

The last submitted patch, 1795186-move_flood_control_to_class-18.patch, failed testing.

paranojik’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1795186-move_flood_control_to_class-18.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
15.54 KB

Simple port of what we have now making the storage plugable.

Status: Needs review » Needs work

The last submitted patch, 1795186-23.patch, failed testing.

marcingy’s picture

Title: Move flood control functions to a class » Make flood storage plugable
Status: Needs work » Needs review
FileSize
15.54 KB

Status: Needs review » Needs work

The last submitted patch, 1795186-25.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
15.56 KB

Ok there was issue in test but beyond that not sure why enabling the module fails

Lars Toomre’s picture

When is the appropriate point to do a documentation oriented review of this issue? The patch in #27 has a number of issues that should be addressed. However, that patch appears to be still under development.

marcingy’s picture

@lars feel free to make docs reviews, it is passing locally so at the moment it is a fight with the bot, and also feel more than free to fix it up in a patch of your own.

cweagans’s picture

Status: Needs review » Needs work
+++ b/core/includes/common.incundefined
@@ -1160,82 +1160,13 @@ function valid_number_step($value, $step, $offset = 0.0) {
+function flood() {
+  return drupal_container()->get('flood');

I don't think we need this. It's handy, but it's extra cruft we don't need in common.inc

Also, can you turn on your editor's "Trim trailing whitespace" option and go through and resave all those files?

paranojik’s picture

Status: Needs work » Needs review
FileSize
17.42 KB

Continuing the work from #27:
-renamed DatabaseFloodBackend to FloodDatabaseBackend - seems more appropriate,
-added the garbageCollection() method and removed the query from system_cron,
-also removed $table parameter of FloodDatabaseBackend as I don't see how this could be changed anyway,
-removed flood() from common.inc

Status: Needs review » Needs work

The last submitted patch, 1795186-make_flood_control_pluggable-31.patch, failed testing.

paranojik’s picture

Status: Needs work » Needs review
FileSize
17.4 KB

Looks like the testbot doesn't like me...

Status: Needs review » Needs work

The last submitted patch, 1795186-make_flood_control_pluggable-33.patch, failed testing.

paranojik’s picture

Status: Needs work » Needs review
FileSize
17.4 KB

...never blame the computer :)

marcingy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

sun’s picture

Status: Reviewed & tested by the community » Needs review

Awesome patch, folks!

I have a couple of questions:

+++ b/core/lib/Drupal/Core/CoreBundle.php
@@ -121,6 +121,8 @@ public function build(ContainerBuilder $container) {
+    $container->register('flood', 'Drupal\Core\Utility\FloodDatabaseBackend')

Hm. I think we need an alternate MemoryBackend implementation for the testing framework, so as to prevent tests from talking to the database when they should not. The MemoryBackend should be part of this patch, IMO.

+++ b/core/lib/Drupal/Core/Utility/FloodDatabaseBackend.php
@@ -0,0 +1,91 @@
+ * Definition of Drupal\Core\Utility\FloodDatabaseBackend.

Is there any reason for why we're not putting this into a proper Flood component instead of mixing it into Utility?

+++ b/core/lib/Drupal/Core/Utility/FloodDatabaseBackend.php
@@ -0,0 +1,91 @@
+  public function garbageCollection() {
+    return $this->connection->delete('flood')
+            ->condition('expiration', REQUEST_TIME, '<')
+            ->execute();

+++ b/core/lib/Drupal/Core/Utility/FloodInterface.php
@@ -0,0 +1,68 @@
+  /**
+   * Cleans up the flood.
+   */
+  public function garbageCollection();

+++ b/core/modules/system/system.module
@@ -3466,9 +3466,7 @@ function system_get_module_admin_tasks($module, $info) {
 function system_cron() {
...
-  db_delete('flood')
-    ->condition('expiration', REQUEST_TIME, '<')
-    ->execute();
+  drupal_container()->get('flood')->garbageCollection();

Hm. It would be good if the phpDoc would clarify who is responsible for calling it (Am I?), and when that would be appropriate to do, and what it really does.

The db implementation seems to be delete all records from the flood table, disregarding event expirations... I wonder whether this is correct?

+++ b/core/lib/Drupal/Core/Utility/FloodInterface.php
@@ -0,0 +1,68 @@
+   * @param String $name
...
+   * @param Integer $window

Minor: The data types in @param should be lowercase.

+++ b/core/lib/Drupal/Core/Utility/FloodInterface.php
@@ -0,0 +1,68 @@
+  public function registerEvent($name, $window = 3600, $identifier = NULL);
...
+  public function clearEvent($name, $identifier = NULL);
...
+  public function isAllowed($name, $threshold, $window = 3600, $identifier = NULL);

Is it only me or are the method names unnecessarily verbose? :)

Why don't we just go with:

register()
clear()
allowed()

KISS? :)

+++ b/core/modules/contact/contact.pages.inc
@@ -166,7 +166,7 @@ function contact_site_form_submit($form, &$form_state) {
+  drupal_container()->get('flood')->registerEvent('contact', config('contact.settings')->get('flood.interval'));

+++ b/core/modules/user/user.module
@@ -1691,10 +1691,10 @@ function user_login_final_validate($form, &$form_state) {
+    drupal_container()->get('flood')->registerEvent('failed_login_attempt_ip', $flood_config->get('ip_window'));

@@ -1714,7 +1714,7 @@ function user_login_final_validate($form, &$form_state) {
+    drupal_container()->get('flood')->clearEvent('failed_login_attempt_user', $form_state['flood_control_user_identifier']);

What always bugged me in the registered flood event are the uncontrolled and random event identifiers — there's a huge chance for a unintended name clashes throughout contrib with these identifiers, since everyone can register any event name.

Couldn't we address that while we're already touching those lines?

The most simple way would obviously be to always put the module name first in the event identifiers, and ideally also use a dot to separate the owning module from the remaining event name; e.g.:

contact
user.login_failed_ip
user.login_failed_user

The more advanced option would be to add a $module or $owner argument to actually enforce a proper event "namespace", but that might be overkill; clear guidelines can be sufficient, too.

Berdir’s picture

- Not exactly sure what we need a memory backend for?

- Also, it's @param int and not @param integer according to http://drupal.org/coding-standards/docs. Note: We have quite a few incorrect examples in core that use @param integer, most in symfony code, though. And symfony also is mixing int and integer, but has more integer than int.

- The database implementation for garbageCollection() includes the expiration condition, just like the old query? Consider me confused...

- Cleaning up the identifiers sounds good to me, there aren't that many that we need a follow-up for this.

paranojik’s picture

Thanks for the reviews, guys!

I tried to address as many issues as you mentioned:
- moved from Utility to Flood component
- updated phpDocs
- renamed FloodInterface methods except for isAllowed() as I've seen this kind of naming elsewhere in core (e.g. CacheBackendInterface::isEmpty())
- cleaned up event names as suggested in #37

I don't know how to properly implement the MemoryBackend, so I've left that out...

Berdir’s picture

Status: Needs review » Needs work

Thanks for the re-roll.

Suggestion: Including an interdiff makes it easier to review the actual changes. You can create those for example with the interdiff tool ( but it often fails with more complex patches) or by using branches, see http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-.... Personally, I use a feature branch for each issue that I'm rebasing on 8.x so creating an interdiff is as easy as diffing against the previous commit.

+++ b/core/lib/Drupal/Core/Flood/DatabaseBackend.phpundefined
@@ -0,0 +1,91 @@
+   * @var Drupal\Core\Database\Connection

Per our new coding standards, references to classes should always start with a \, e.g. \Drupal\...

+++ b/core/lib/Drupal/Core/Flood/DatabaseBackend.phpundefined
@@ -0,0 +1,91 @@
+    return $this->connection->delete('flood')
+            ->condition('expiration', REQUEST_TIME, '<')

Indendation looks off, should be two spaces of the previous line like the previous queries.

paranojik’s picture

Status: Needs work » Needs review
FileSize
9.15 KB
1.12 KB
17.63 KB

Thanks for the tip! The interdiff between #35 and #39 is non-representative, because I moved the files from Utilty to Flood and also made other changes. I'm attaching a simple diff between the two.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Flood/FloodInterface.phpundefined
@@ -0,0 +1,76 @@
+   * @param string $name
+   *   The name of an event. To prevent unintended name clashes, always use
+   *   the module name first in the event name, and ideally also use a dot to
+   *   separate the owning module from the remaining event name.

Hm, instead of ideally, maybe something like this?

", optionally followed by a dot and the actual event name." + maybe an example.

Or just make that part recommended, like we're afaik doing for the state system and leave out the optionally.

Because otherwise, you first just use the module name and when you add a second flood event, you have to rename the existing one?

Other than that, this looks good to me. Maybe sun can explain why a memory backend is required. It wasn't until now, obviously? Most other such pluggable services do have one now too (cache, queue, state, keyvalue, ...), is that the argument?

sun’s picture

In #1774388: Unit Tests Remastered™, I had to override the service definition for the Lock backend class with its NullBackend in order to make DrupalUnitTestBase work (so as to prevent it from querying the database where no database table is).

Flood is used less often, but still by Contact and User, but also system_cron(). In particular System and User modules are very likely candidates for modules to get enabled for DrupalUnitTestBase without getting installed, so we will want to override the service definition with a MemoryBackend/NullBackend. (I'm not sure whether NullBackends are actually correct for DrupalUnitTestBase, but I'm absolutely confident that MemoryBackend implementations are.)

However, the interface here is simple enough, so I think I can perfectly add the MemoryBackend myself over there when it is needed. Of course, I'd appreciate it if you'd simply add it here, but I certainly don't want to block this issue on that. :)

Berdir’s picture

Ok, I see. I've thought about doing similar garbage collection than we're doing in the expirable key value store (trigger flag on set/delete that does garbage collection on destruct) but I don't think this makes sense here because we're doing much more set and delete than there.

A different backend that can for example rely on actual expiration of items can just leave the method empty, so that's not a problem either.

Here's how a memory implementation could work: Have a single array property and add added events in there like this:
$this->events[$name][$identifier][] = array('timestamp' => $timestamp, 'expiration' => $timestamp + $window);

clear() is a straight-forward unset(), isAllowed() would then need to loop over the events of the given name and identifier (if any) and count the events that have not yet expired with a similar check like the one in the query.

I guess we'd also need tests. Having unit tests for both the database an the memory implementation would be nice. expiration can't be simulated in the current implementation anyway, so you just need to add a few events with different identifiers and check that it allows when it should and doesn't when not. then do a clear() and it should work again for that event/identifier, but not for others. Then execute them against both backends. See the KeyValue tests for ideas.

Do you think you can implement that? If you run into blockers with the memory backend, report them and we'll figure it out or postpone it until it's required.

sun’s picture

Yup! #44 could only be simplified by doing:

$this->events[$name][$identifier][$timestamp + $window] = $timestamp;

isAllowed() should translate into this direct equivalent with that:

$limit = REQUEST_TIME - $window;
$number = count(array_filter($this->events[$name][$identifier], function ($timestamp) use ($limit) {
  return $timestamp > $limit;
}));
return $number < $threshold;

(give or take some PHP array internals I might have overlooked)

Berdir’s picture

$this->events[$name][$identifier][$timestamp + $window] = $timestamp;

This will only work if $timestamp is guaranteed to be unique, so we would be required to use microtime() instead of REQUEST_TIME.

paranojik’s picture

Does this look like something you had in mind? The garbageCollection() method looks a bit convoluted...

Berdir’s picture

+++ b/core/lib/Drupal/Core/Flood/MemoryBackend.phpundefined
@@ -0,0 +1,75 @@
+    if (!isset($identifier)) {
+      $identifier = ip_address();

Haven't noticed before. In #1794754: Convert ban_boot() to an event listener, there was quite the discussion about using ip_address() in services and the result was that we shouldn't. So we should probably update it here and in the database backend as well. Suggestion: Make it a constructor argument (default_identifier) and pass it in when registering the service).

Forget that, looks like this was changed again, so let's not change this. On the other side, we won't have an $event to work with here, so I'm not sure. Anyway, follow-up material IMHO

+++ b/core/lib/Drupal/Core/Flood/MemoryBackend.phpundefined
@@ -0,0 +1,75 @@
+    $limit = REQUEST_TIME - $window;

This should then use microtime() as well, because otherwise it might result in unexpected results when working with small windows.

+++ b/core/lib/Drupal/Core/Flood/MemoryBackend.phpundefined
@@ -0,0 +1,75 @@
+      foreach ($this->events[$name] as $identifier => $timestamps) {
+        $expired = array_filter(array_keys($timestamps), function ($expiration) {
+          return $expiration < REQUEST_TIME;
+        });
+        foreach($expired as $expiration) {
+          unset($this->events[$name][$identifier][$expiration]);
+        }

It might be possible to reverse the condition and then do something like $this->events[$name][$identifier] = array_filter()?

Also, as long as we have something that works correctly, it doesn't matter much, this isn't something that we're actually going to use in a real-world scenario.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/FloodTest.phpundefined
@@ -45,4 +45,30 @@ function testCleanUp() {
+
+  /**
+   * Test flood control memory backend.
+   */
+  function testMemoryBackend() {

Tests look good enough to me. Maybe we could open a follow-up to refactor them to unit tests and have a base class that is executed against both backends, similar to the key value tests.

paranojik’s picture

Redone the garbageCollection() as you suggested, and the microtime() bit...
I'll see what I can do about the tests.

Thanks for the review!

Berdir’s picture

Ah, I see the problem, that's a bit black magic :) Not sure which approach is better.

Re tests, as I said, these look fine for me to get this in, we can look into improving it in a follow-up issue.

marcingy’s picture

Status: Needs review » Needs work

The memory back end tests do not do asserts against anything.

eg

// Verify event is not allowed.
$flood->isAllowed($name, $threshold);

vs

$this->assertFalse($flood->isAllowed($name, $threshold));
paranojik’s picture

Rerolled and fixed the memory backend tests. Thanks @marcingy!

Berdir’s picture

Status: Needs work » Needs review
cweagans’s picture

Status: Needs review » Reviewed & tested by the community

This looks good, and all the feedback has been captured, so I say back to RTBC!

catch’s picture

Title: Make flood storage plugable » Make flood storage pluggable
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

This looks great. It'd be good to factor out the ip_address() calls but that's not a problem created by this patch, I opened #1847768: Remove ip_address() and committed/pushed this one. Thanks folks!

This will need a change notice.

plach’s picture

Title: Make flood storage pluggable » Change notice: Make flood storage pluggable
YesCT’s picture

Assigned: Unassigned » YesCT

I'll do the change notice for this.

YesCT’s picture

Title: Change notice: Make flood storage pluggable » Make flood storage pluggable
Assigned: YesCT » Unassigned
Status: Active » Fixed

http://drupal.org/node/1848332 is the change notice.

sun’s picture

Status: Fixed » Needs work

Thanks!

Unfortunately, we're missing upgrade docs for module developers there. Essentially, the change notice needs to clarify that developers need to perform these changes when interacting with the Flood API:

-  if (!flood_is_allowed('contact', $limit, $interval) && !user_access('administer contact forms')) {
+  if (!drupal_container()->get('flood')->isAllowed('contact', $limit, $interval) && !user_access('administer contact forms')) {

-  flood_register_event('contact', config('contact.settings')->get('flood.interval'));
+  drupal_container()->get('flood')->register('contact', config('contact.settings')->get('flood.interval'));

   // Cleanup the flood.
-  db_delete('flood')
-    ->condition('expiration', REQUEST_TIME, '<')
-    ->execute();
+  drupal_container()->get('flood')->garbageCollection();

     // Clear past failures for this user so as not to block a user who might
     // log in and out more than once in an hour.
-    flood_clear_event('failed_login_attempt_user', $form_state['flood_control_user_identifier']);
+    drupal_container()->get('flood')->clear('user.failed_login_user', $form_state['flood_control_user_identifier']);
Berdir’s picture

Assigned: Unassigned » Berdir

Started working on that.

Berdir’s picture

Assigned: Berdir » Unassigned
Status: Needs work » Needs review

Ok, updated the change notice, contains some more detailed description, removed irrelevant parts and added before/after code snippets.

http://drupal.org/node/1848332

paranojik’s picture

Status: Needs review » Reviewed & tested by the community

Nice write-up. I think it's accurate enough.

chx’s picture

Status: Reviewed & tested by the community » Fixed
Tor Arne Thune’s picture

Priority: Critical » Normal

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.