I'd like to propose a non-breaking enhancement which would facilitate testing thresholds for flooding across all users, instead of just individual users.
Drupal/Core/Flood enables events to be logged for specific event name and user identifier combinations, and for this data to be queried to see whether a specific user has exceeded some configured threshold for a given event (within a given time window).
That same data could also be used to see whether a threshold has been exceeded for an event irrespective of the users involved; but at present this type of query is not supported.
The intent is to allow distributed flooding scenarios to be detected using only the existing data (i.e. without the need to log additional events).
(This would not be useful in all circumstances, but there are also cases where enabling a site administrator to configure a global threshold for processing certain kinds of event would be of decided benefit.)
At present the isAllowed() method filters events by both the name of the event and the identifier of the user. If we simply omit the identifier condition from the resulting checks, we will be counting all events of the specified name within the given time window, enabling code to ascertain whether a given event is happening too frequently even when the events are issuing from different IP addresses.
The current interface documents that $identifier must be either a string or NULL (in the latter case the user's IP address is used). I propose that an additional value of FALSE -- being both appropriate to the intent, and also non-conflicting with currently-allowed values -- be allowed to signify that identifiers be ignored for that call.
(This enhancement could also be applied to the clear() method, where it would have the same meaning, therefore deleting all events for the given name irrespective of their registered identifier.)
I further note that these changes could safely be back-ported to Drupal 7. Although D7's flood_is_allowed() does not document that a non-NULL $identifier must be a string, FALSE simply doesn't work as an identifier -- the database column (D7 provides only a database-based implementation) is a varchar (non-NULL, defaulting to an empty string), and experimentally an attempt to register an event with identifier FALSE results in the identifier actually being an empty string, and flood_is_allowed() only recognises those events when called with an (explicit) empty string for $identifier.
Finally, those notes on D7 also apply directly to the DatabaseBackend in D8; and although you could get away with passing a boolean for the MemoryBackend, it would (a) violate the interface, (b) be incompatible with the DatabaseBackend, and (c) those booleans would be coerced to integers in that implementation, which could conflict with other values.
In summary, this would add a useful additional ability to the flood mechanism, providing a new use for the pre-existing flood data, and with no adverse effect on existing applications (in either Drupal 8 or Drupal 7).
* Write D8 patch
* Backport to D7
- None
Non-breaking additions to the isAllowed() and clear() methods of Drupal/Core/Flood, allowing $identifier === FALSE to indicate that the queries should ignore identifiers.
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | drupal-flood-2472941-25-D8.patch | 14.56 KB | jweowu |
Comments
Comment #1
jweowu commentedComment #2
jweowu commentedComment #8
jweowu commentedLet's see what the testbot makes of it.
Comment #11
jweowu commentedI thought that would probably happen. Not very useful, testbot. I'll have to mash it into a single patch for testing...
Comment #15
jweowu commentedComment #17
jweowu commentedComment #19
jweowu commentedComment #23
jweowu commentedComment #24
jweowu commentedComment #25
jweowu commented