Use flood service

Initial patch

+++ b/src/EventSubscriber/PerimeterSubscriber.php
@@ -58,12 +73,15 @@ class PerimeterSubscriber implements EventSubscriberInterface {
-  public function __construct(LoggerChannelFactoryInterface $logger_factory, ConfigFactoryInterface $config_factory, AccountProxyInterface $current_user, BanIpManagerInterface $ban_manager) {
+  public function __construct(LoggerChannelFactoryInterface $logger_factory, ConfigFactoryInterface $config_factory, AccountProxyInterface $current_user, BanIpManagerInterface $ban_manager, FloodInterface $flood) {
     $this->loggerFactory = $logger_factory;
     $this->configFactory = $config_factory;
     $this->currentUser = $current_user;
     $this->banManager = $ban_manager;
+    $this->flood = $flood;
   }

to avoid early bootstrap errors and problems with subclasses and so on, I'd suggest making the argument optional with a fallback to \Drupal::service().

Thanks for the review. Something like that?

NID for patch name is wrong. No change in content.

Tested this patch on a live site, and got myself banned multiple times. This patch works.

I created a MR for this patch, and added two tests for the new configurations.

LGTM in general! But needs a rebase.

I rebased. There is one more thing that @Berdir noticed: perimeter_honeypot_reject() needs to use the service instead of the class.

Left a comment. As written, my point point about perimeter_honeypot_reject() is that it needs to use the same flood behaviour.

Added that. Wasn't sure if it should use the same flood key or not, but then it should probably just be perimeter.

Just took a look and LGTM! I'll ask @Grevil to review it finally. Thanks @Berdir, great work as always!

Let's make this major as it would be very useful to the modules functionality. Especially being able to not block at first wrong access.

Seems that ✨ Make perimeter compatible with fast404 module and use DI for ban.ip_manager Fixed causes handleBannedUrls() being called twice and therefore messing up the threshold logic.

Alright, going to finish this on Monday, just added a few comments for myself.

Note, the method being called twice is fixed in 🐛 "handleBannedUrls()" is executed twice on 404 pages Active .

I have no idea, why the tests fail...

After some debugging, I find that "onKernelException" is only executed on the first "drupalGet()" inside the tests for some reason.

I can not replicate this issue locally...

Maybe @berdir has any idea?

drupalGet() doesn't expect have a leading /, try without that. Tests there run in a subfolder, that might end up outside of Drupal.

The other thing is caching. It should be consistent in a test, but I'd expect that multiple requests to the same URL would end up in page cache. I'd suggest using different unique URL's.

Thanks, @berdir!

I'll check that tomorrow!

I removed the leading slahes, let's see what happens, so you'll have a result tomorrow!

Reviewed and verified the test fails are due to caching.

All tests are green now! Setting back to "Needs Review" for reviewing final changes.

Alright, that's it! Thanks all! 🙂👍

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

Howdy,

Awesome module!

Is this fix included in the latest release (3.0.4/28 August 2024)?

Indeed! The MR title was incorrect, so it doesn't properly show up in the change notes.