Support external Flood (Redis, etc.)

+++ b/src/Flood/PhpRedis.php
@@ -10,7 +10,7 @@ use Symfony\Component\HttpFoundation\RequestStack;
  * Defines the database flood backend. This is the default Drupal backend.
  */
-class PhpRedis implements FloodInterface {
+class PhpRedis implements VisualizableFloodInterface {
 
   use RedisPrefixTrait;
 
@@ -92,4 +92,49 @@ class PhpRedis implements FloodInterface {

you need to upload the redis patch to the redis issue queue.

The problem with this approach is that we can not add a hard dependency on flood_unblock but that's what you are doing with this. Instead, it would need to be a subclass of PhpRedis that lives somewhere, and you can optionally use that by overriding a service/factory/setting.

I'd also be open to adding a module exists check in the factory class to pick that class automatically or a service provider alter.

I will try to test this patch ASAP. Did you test this patch with a non-redis install as well? Does everything still work?

I'm working on a new flood project and i faced the same issue.

The (real) problem (from what i found so far): the storage of the flood records dictates the way flood_unblock deals with them (the @flood passed to the FloodUnblockManager). I believe this has to change for the current issue to work correctly.

I think instead of overriding the flood storage + having all these contrib modules adopt a part of the flood_unblock code (and maintain it), the classes should remain in A. a separate flood storage module or B. be added in flood_unblock (think redis, memcache, etc, yada) - and - (more important): the storage and display should be separated, so flood_unblock can react to whatever is configured.

I believe the flood_unblock module should adopt the choice we made in our flood project: act on the @flood that's passed to the handler - and not use it. Load the corresponding 'flood_unblock process class' from what's passed via the @flood, instead of overloading the thing that needs to run fast with additional code only required for 'processing' the records, like it would do with the current patch (+ all the related other complexities).

Saves a lot of work and this also works without module dependencies. Just make sure the class that core flood uses works, if that's broken, too bad. (then just return a message: unable to load handler or something, currently it returns the empty row message only, but that's details) (also no need for a module exists, but can't hurt in the manager init)

So i believe when Drupal+Redis is installed, and configured, flood_unblock should load the corresponding 'flood unblock process class' for it (or whatever you want to call it) and not override + use the flood storage class itself passed via @flood, but could be wrong ;)

[edit: render>process, fits better to what it actually does]

This looks related. #2908307: Implement flood control with memcache

After reviewing some more bits n pieces, other modules, rethink it all again, i wonder if we can not just implement a factory for retrieving the 'flood_unblock method class' bases on whatever is loaded in the flood service (if those a separated like i proposed previously).

(Where the 'flood_unblock method class' corresponds to whatever is loaded in the flood service, so a class for each in flood_unblock.)

Then the manager can just be amended with the result from that factory.

To answer my own question:

No we can't use a factory if we want to inject the connection etc. Did some testing with some concept code and have some more ideas. Also attempted submodules for storage backends and more.

I propose to separate the flood storage & the flood unblock storage.

Make flood_unblock use the database by default and let sites override that behaviour in their site's services file (like to override the core flood service) (cause really, what would break this: a site with the flood service set to (for example) Redis - and - the database schema for the flood table not installed, e.g. a super custom setup, that table is always created by default). It will render an empy table, but that's the worse that could happen on a default install. + We could test for the class and report on the reports page if the service for flood does not match the configured service for flood_unblock. (this is what i actually do in my flood project now, seems to work ok).

Then we can create storage classes for each we want flood_unblock to support and make the flood unblock manager accept that instance as a param. So we can just call: manager->listBlockedIps() (etc) on the form/etc.

So yes, (soft) dependencies, but i think way better maintainable then adding classes to each contrib project (redis/memcache/couchbase/etc - list continues to grow).

What do you guys think about this last proposal? ^^

Looks the service provider is better approach because when flood backend is overriden (it happens once of container build) that's exactly where flood_unblock should check "real" (current) service class for flood and try to find compatible class to query (list) and unblock (unset) methods

Could this be handled by using a service provider to inspect the flood backend and add an appropriate manager dynamically rather than defining statically in flood_unblock.services.yml? Flood unblock would have to provide the managers itself (or other modules could ship with the manager and themselves add it via a service provider). That gets round hard dependencies in either direction.

Edit; Just realised I didn't read the most recent comment properly, sorry!

This is a really rough implementation of a swappable backend with redis implementation. Issues include:

• No tests
• Not overly happy with having to actually load the flood service to check what backend to use...
• Could maybe do with some namespace changes to better structure the managers - all just in src at the moment.
• There's quite a lot of copy/pasted code. It might be better to either use a base class or a decorator for things like user details, location etc.

Perhaps not entirely surprisingly, loading the service caused errors on install... Here's switching to sniffing from the definition. Perhaps #3174906: Consolidate flood backend might make it cleaner!

Hi, wondering why is this patch on 3.0.x version and not on 3.1.x and also I tried getting this to apply on 3.0.x but it doesnt work. I am bit confused why dont we follow branch release name with tags, is this a bug or proper way to use this versioning.

Since this module is being discontinued/merged with flood_control, moving this issue over there. Will need a re-roll for file name changes (at least).

Untested re-roll.

Couple of missed used statements.

There are more issues here than just a re-roll.

The API for flood_unblock module that this patch is written against uses floodUnblockClearEvent($event, $identifier). This would work fine for Redis (although I'm not sure which branch it was written against).

The API for flood_control uses floodUnblockClearEvent($event, $identifier) - Redis doesn't store an fid.

Some extra changes in this patch to record progress, but @todos:

1. Is it OK to change the API of FloodUnblockManagerInterface here (i.e. do we need a new method to clear events and leave the old one intact or similar).

2. The admin form still needs updating.

OK a bit more work.

Implemented ::getEntries() for FloodUnblockManagerDatabase

Simplified the form so that it can rely on the ::getEntries() method.

I've only tested this locally with the database backend but posting progress. Will do some manual testing with redis tomorrow hopefully.

More progress.

Redis backend now works.

::getEvents() doesn't belong on FloodUnblockManagerInterface - it's just a mapping for the UI. Have copied this to the form class but other references not converted yet.

Removed the smart IP mapping for now - this should IMO be brought back in a different issue that only attempts to check IP location for actual IP addresses (probably depending on the flood event) - right now it's looking up IP location for arbitrary strings.

Some other bits like getIdentifiers() that I think should probably be factored away rather than implemented with Redis.

Just doing a little tag tidying. Nice work everyone!!

Hi, thanks everybody! I will try this probably tomorrow on our project.

Ok for me too! Works on our projects.

Used the patch from the MR.

i can confirm it works (#36) on our project.

Rebased the PR, and adding a new patch.

@catch, thanks for all the good work. I've just checked the merge request, and everything seems fine. But what I notice is that the filters, tablesorting and such has been removed (https://git.drupalcode.org/project/flood_control/-/merge_requests/16/dif...) in your patch.

Did you do this with a specific reason?

@fabianderijk alternative backends can't really support tablesort and filters, or not in an efficient way. It might be possible to add them back just for the database backend.

Can it also support SMS Framework?

Notice: Undefined index: sms.verify_phone_number in Drupal\flood_control\Form\FloodUnblockAdminForm->buildForm() (line 137 of /app/web/modules/contrib/flood_control/src/Form/FloodUnblockAdminForm.php)

This no longer applies.

Via composer:

- Installing drupal/flood_control (2.2.3): Extracting archive
- Applying patches for drupal/flood_control
https://www.drupal.org/files/issues/2021-09-14/2928007-38.patch (Support external Flood)
Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2021-09-14/2928007-38.patch

Struggling to get the MR branch working, so here's a rerolled patch for someone else to figure it out with :)

Hi! I've tested Drupal 9.3.6 + 2.2.3 + patch #45 and it works with Redis!

Needs reroll for 2.2.4

Reroll for 2.2.4. Doesn't include implementation of new method needed for Drush commands.

Hello !

New patch with redis and databases seems working good.

Based from last stable release branch 2.2.4

Sorry I try to use issue fork system but i don't succeed to use it.

Rerolled flood_control-support_external_flood-2928007-52.patch for 2.3.0

Oops! Previous patch had a misnamed file. Fixed version attached.

New patch with a new branch from fork: flood_control-2928007

The patch is well applied with this link: https://git.drupalcode.org/project/flood_control/-/merge_requests/36.diff

i implement fetchIdentifiers function for database and redis class.

I completed this issue to have a more readable user name.

I have not tested the patch with the database project.

Thanks for this module. But...

I maintain a site that uses redis. The Flood Control module v2.3.0, with the diff file patch applied from #57, works correctly displaying flood control info and enabling deleting these from redis.

Updating to Flood Control v2.3.2, this patch can no longer be applied. Composer results in this error:
Could not apply patch! Skipping. The error was: Cannot apply patch https://git.drupalcode.org/project/flood_control/-/merge_requests/36.diff

I also tried the patch from #54, with the same fail result.

I would try to offer a suggested fix, but I don't see where the fail is.

Will this patch be updated, and/or merged with a future version of Flood Control?

We have produced a new patch that works for us with Redis on Flood control 2.3.2.
See attached flood_control_2.3.2.patch.

Fixed some errors.

Added fix for misleading "blocked status" on custom events.

With Drupal 10.0.9, and Flood control 2.3.2
with this patch (62) flood_control-support_external_flood-2928007-62.patch
I'm getting this following error:
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "flood_control.flood_unblock_manager". in Drupal\Component\DependencyInjection\Container->get() (line 157 of core/lib/Drupal/Component/DependencyInjection/Container.php).
Drupal\flood_control\Form\FloodUnblockAdminForm::create(Object) (Line: 28)
Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition('\Drupal\flood_control\Form\FloodUnblockAdminForm') (Line: 48)
Drupal\Core\Controller\HtmlFormController->getFormObject(Object, '\Drupal\flood_control\Form\FloodUnblockAdminForm') (Line: 58)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 580)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 163)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 74)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 686)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

To add a bit to my previous comment, I've been getting the same error using
flood_control_2.3.2.patch (see #59) but the patch worked on Drupal 9.
Something is going on with dependency injection but I haven't been able to figure
out what the problem is. I hope you can figure it out for
patch (62) flood_control-support_external_flood-2928007-62.patch so it can work
on Drupal 10.

I have the same issue on D10
In the FloodControlServiceProvider::alter method ... instead of setting a new service definition i'm updating the existing one... which seems to work:

$container->getDefinition('flood_control.flood_unblock_manager')
  ->setClass(FloodUnblockManagerRedis::class)
  ->setArguments( [
        new Reference('redis.factory'),
        new Reference('flood'),
        new Reference('config.factory'),
        new Reference('entity_type.manager'),
        new Reference('messenger'),
      ]);

not sure why

sonneworks - I'm eager to try your new fix but so far have not been able to get
it to work. Could you either:
post your entire new FloodControlServiceProvider::alter method
or
if you have produced a new patch based on this fix could you post that? thanks will_frank

@sonneworks - is it possible that you might have changed other parts of the code
besides the FloodControlServiceProvider::alter method to get it to work?

One idea is that if you don't have a full patch file ready, perhaps you could
just zip up your entire flood_control module (all the files) and email it to me
at fwilliams7070@gmail.com and I'll try it - and if it works I could produce a patch.

@sonneworks
We have succeeded in creating a new patch based on your idea. Many thanks.
This works on Drupal 10. Likely this patch only will work with sites that are using
redis.

I took the patch #69 and aligned the method signature of the FloodUnblockManager::fetchIdentifiers with the interface method.

Edit: removed Patch beacuse this class is not used anymore. I recommend to remove the FloodUnblockManager.php file, in order to get rid
of legacy code.

@will_frank sorry, was on holiday, didn't have access to codebase :)

Hi @will_frank , flood_control_2.3.2_d10.patch not working for flood_control 2.3.3.

version: 2.3.3

@KalaiyarasiThangavel yes you're right. I've been working trying to produce
a new patch for flood_control 2.3.3 but so far without success. If anyone has
a solution I would love to see it.

I have produced a new patch that works for us on Drupal core 10.1.4
and Flood control 2.3.3.
see
flood_control_2.3.3_d10.patch (30.44 KB)

Thank You @will_frank . The patch works fine.

Hi, I am getting the same error even after applying the #75 patch.

The website encountered an unexpected error. Please try again later.

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "flood_control.flood_unblock_manager". in Drupal\Component\DependencyInjection\Container->get() (line 157 of core/lib/Drupal/Component/DependencyInjection/Container.php).
Drupal\flood_control\Form\FloodUnblockAdminForm::create(Object) (Line: 28)
Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition('\Drupal\flood_control\Form\FloodUnblockAdminForm') (Line: 48)
Drupal\Core\Controller\HtmlFormController->getFormObject(Object, '\Drupal\flood_control\Form\FloodUnblockAdminForm') (Line: 58)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 592)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 53)
Asm89\Stack\Cors->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 704)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Drupal version: 10.5.1
Flood_control: 2.3.3

I really appreciate your support

The issue forks for this are pretty wildly broken. I tried to see if I could rescue at least one of them, but in reality it looks like they should all be closed off and deleted.

Here's a re-roll of the patch from the current HEAD which also includes an update - the patch in #75 remove it?

Returning to Needs Work as there are a number of changes included recently in HEAD which have not been included in the change.

Work in progress.

Should work for redis OK - just doesn't have creation/expire times showing at present. No ability to filter.

Database back end is completely untested. Interface needs to be updated to pass in filtering options so no filtering atm.

Hey @WilliBautista, Your issue seems to be deeper than just the patch as the server which is present with or without this patch is what your site is complaining about. Flush caches? Patch from HEAD?

I ran into the same problem as comments #64 & #77, finally settling on the solution in #66 with some additional guards since I had an out of order xServiceProvider occurrence where the definition did not exist before trying alter it. The issue only manifest itself after restarting my cli session and prevented the site from working until I disabled the alter completely, flushed redis completely, restarted apache and php-fpm, and then flushed caches. Discovered the FloodUnblockManagerUnknown class needed some more method stubs.

No other significant changes.

I have updated the patch for the last dev version of module.

Hello Sergei,
Thanks for your work!
#83 created an error in `FloodUnblockManagerDatabase` re "not in GROUP".
Please check the patch including the fixes attached.

Patch from #85 reports as "corrupted".

Confirming that patch #86 works as expected on a D10.1.7 and D10.2.3.

Hi, thanks everybody! I received some error and warning when testing patch #86 on D10.1.6 native database backend:
InvalidArgumentException: The timestamp must be numeric. in Drupal\Component\Datetime\DateTimePlus::createFromTimestamp() (line 201 of core/lib/Drupal/Component/Datetime/DateTimePlus.php).

Warning: Undefined array key "value" in Drupal\flood_control\Form\FloodUnblockAdminForm->buildForm() (line 189 of modules/contrib/flood_control/src/Form/FloodUnblockAdminForm.php).
Drupal\flood_control\Form\FloodUnblockAdminForm->buildForm(Array, Object)

Used $results[$key]["value"] instead of $results[$identifier]["value"] in FloodUnblockManagerDatabase::fetchIdentifiers.

A a maintainer, I have been monitoring this issue for a while.

I think that there are still major problems with this patch:
- Code has been copied in the past from FloodUnblockManager to new classes (FloodUnblockManagerBase & FloodUnblockManagerDatabase). After that moment FloodUnblockManagerBase was changed and improved, but those changes were not transferred to the code in FloodUnblockManagerBase & FloodUnblockManagerDatabase, resulting in the code in those files to miss certain features and improvements. The same happened with code that was copied from FloodUnblockAdminForm.php
- Having a default manager that is 'unknown' is confusing to me. We better assume that we have a database connection and make the original Manager the
default again. In that case we don't need FloodUnblockManagerUnknown.php

I propose to introduce the External Flood Support in two steps, using 2 different issues:
- 1. Add the generic FloodUnblockManager. This will be achieved by adding the generic FloodUnblockManagerBase and the database specific FloodUnblockManagerDatabase. This should be done while ensuring that everything keeps functioning as it does in the latest release.
#3437860: Make FloodUnblockManager generic
- 2. Add the Redis FloodUnblockManager. This will be achieve by adding the FloodUnblockManagerRedis
#3437875: Add Redis support

Oh man, I just ran into this and was searching for an hour, why IPs are blocked, but the list at /admin/people/flood-unblock and the flood database is empty -.-

The reason is: The project uses redis as caching backend (#3192286: Flood unblock page is always empty with redis module)

Just wanted to point out here, that this is really tricky currently and not easy to find, if you didn't set up the project and are not aware of this special case.
I totally agree with @batigolix in #89 but perhaps we should first make this more visible, until it is solved?
We could also treat this as bug, while I understand that Redis etc. are seen as bonus.

I hope what happened to me doesn't happen to others. For now I'll exclude flood from Redis. Documented here: https://www.drupal.org/project/flood_control/issues/3192286#comment-1396...

Needs work seems like a better status than needs more info here - the problem is known, the question is how best to implement the fix for it.

Updated the last patch to support the new filter "Only blocked"
I did this the same way it is done in "dev" if I'm right.
(This means it's display filter only, not a request filter)

The code in the patches in this issue is missing a lot of the improvements that were made in the last years (among them the new filter "Only blocked").

I explained this in #89

I created a new patch that includes all improvements (including the new filter "Only blocked") plus the code that makes the flood unblock generic. See #3437860: Make FloodUnblockManager generic

There is an open issue for adding Redis support #3437875: Add Redis support

Please do no update the patches in because they are outdated.

I put this issue on hold until #3437860: Make FloodUnblockManager generic is fixed.

blocker is fixed

Sorry, I was not clear. #3437875: Add Redis support also needs to be fixed.

#3437875: Add Redis support can be reviewed. We just pushed a couple of commits that allow the use of Redis.
This needs thorough testing. Any help with testing this, would be greatly appreciated.

The issue for adding Redis support is #3437875: Add Redis support