Convert ban_boot() to an event listener

You will not duplicate the discussion of #1479466: Convert IP address blocking to config system here. Thanks. ;)

On the patch:

+++ b/core/modules/ban/ban.admin.inc
@@ -6,11 +6,13 @@
+ * @see ban_menu()

This is a bogus addition and should be removed.

+++ b/core/modules/ban/ban.admin.inc
@@ -41,9 +43,8 @@ function ban_admin_page($default_ip = '') {
- * @see ban_ip_form_validate()
  * @see ban_ip_form_submit()
- *
+ * @see ban_ip_form_validate()

_validate() comes before _submit() and should thus be referenced first.

+++ b/core/modules/ban/ban.admin.inc
@@ -109,7 +110,7 @@ function ban_ip_delete_form($form, &$form_state, array $ban_ip) {
-    t('Are you sure you want to delete %ip?', array('%ip' => $ban_ip['ip'])),
+    t('Are you sure you want to remove the ban on %ip?', array('%ip' => $ban_ip['ip'])),

Let's replace this with "unblock" instead?

+++ b/core/modules/ban/ban.module
@@ -2,7 +2,7 @@
  * @file
- * Enables banning of IP addresses.
+ * Provides capability to ban individual IP addresses.

"Allow to..."

+++ b/core/modules/ban/ban.module
@@ -13,11 +13,11 @@ function ban_help($path, $arg) {
-      $output .= '<p>' . t('The Ban module allows administrators to ban visits to their site from given IP addresses.') . '</p>';
+      $output .= '<p>' . t('The Ban module allows administrators to ban visits to their site from individual IP addresses.') . '</p>';
...
-      $output .= '<dd>' . t('Administrators can enter IP addresses to ban on the <a href="@bans">IP address bans</a> page.', array('@bans' => url('admin/config/people/ban'))) . '</dd>';
+      $output .= '<dd>' . t('Administrators can enter individual IP addresses to ban on the <a href="@bans">IP address bans</a> page.', array('@bans' => url('admin/config/people/ban'))) . '</dd>';

@@ -67,10 +67,10 @@ function ban_boot() {
- * Returns whether an IP address is blocked.
+ * Returns whether an individual IP address is blocked.

I don't think that the "individual" helps in any way to clarify the module's functionality. It only seems to add visual/textual clutter/noise. I'd suggest to revert these changes.

On the test failure:

The feature of blocking visitors through Statistics module's top visitor report page actually belongs to Statistics module, not Ban module. Therefore, it makes sense to keep the test in Statistics module. The test just needs to be updated for the changed UI strings.

hook_boot() is not where you would integrate this. hook_boot() should be deprecated in favor of the request listener, honestly.

What you'd want is to add a BanBundle class that registers a subscriber. That subscriber then listens to the kernel.request event, and itself depends on the database connection (or the state system, or whatever you're going to back it with). The listener then checks the database to see if the IP address of the $request is on the block list, and if so calls $event->setResponse(new Response(403)); (or whatever it is). If it's not, it does nothing.

There would be no functions at all in the main pipeline.

Honestly I'd do that first before you get into tidying up comments.

I was responding to this:

Based upon the above it appears, we want to register this service in the boot_hook of this module. Correct?

Not correct. :-) You'd create a class Drupal\ban\BanBundle (in modules/ban/lib/Drupal/ban/BanBundle.php) and have it register a subscriber. There should be examples in core already. You don't use hook_boot() for that.

I also would argue that change is more important than tidying up grammar on the comments, since the comments will no doubt change as a result of the code changes described above.

#9: ban-followup-1794754-9.patch queued for re-testing.

This patch

removes ban's hook_boot implementation and does what Crell suggested
removes ban_block_denied()
merges Lars doc fixes.

I am not sure if i should also remove ban_is_denied() as well and just query the database directly from subscriber since i

+use Drupal\Core\Database\Database;

But i am not sure so leaving it for discussion

And removing copy-paste failures:/

Yes, you should pass $container->get('database') (which is a Database\Connection) as an argument to your class and then execute the query directly on that ($this->connection->query()). you can move ban_is_denied() to a isDenied() method, if it's not used anywhere else.

thanks for the hints Berdir!!

I left variable_get('ban_ips'); as is, since there is another issue for this..#1479466: Convert IP address blocking to config system

Looks nice. Wondering if the ip_address() should be passed in to the constructor as well. That would theoretically allow to write unit tests for this as it would be possible to create it with a different ip address that would be blocked.

Sure, yes you are right.
Added $ip = '' as parameter of __construct.
ip_adress() will be used if empty.

+++ b/core/modules/ban/lib/Drupal/ban/BanBundle.php
@@ -0,0 +1,28 @@
+  public function build(ContainerBuilder $container) {
+    $dispatcher = $container->get('dispatcher');
+    $connection = $container->get('database');
+    $dispatcher->addSubscriber(new \Drupal\ban\EventSubscriber\BanSubscriber($connection));
+  }

This forces the DB to be instantiated here. That's too early. Instead, do this:

$container->register('subscriber.ban')->addArgument(new Reference('database'));
$dispatcher->addSubscriberService('subscriber.ban', '\Drupal\ban\EventSubscriber\BanSubscriber');

That lets the subscriber class not get instantiated yet, and is closer to what the syntax will be after the container gets compiled.

+++ b/core/modules/ban/lib/Drupal/ban/EventSubscriber/BanSubscriber.php
@@ -0,0 +1,99 @@
+  public function __construct(Connection $connection, $ip = '') {
+    $this->connection = $connection;
+    $this->ip = !empty($ip) ? $ip : ip_address();
+  }

Don't use ip_address(). That's a hard dependency, and in fact you don't need it at all. See below.

+++ b/core/modules/ban/lib/Drupal/ban/EventSubscriber/BanSubscriber.php
@@ -0,0 +1,99 @@
+  public function onKernelRequestBannedIpCheck(GetResponseEvent $event) {
+    if ($this->isDenied()) {
+      $response = new Response('Sorry, ' . check_plain($this->ip) . ' has been banned.', 403);
+      $event->setResponse($response);
+    }
+  }

Here, call $request = $event->getRequest(); That gives you the request object. You can then get the IP address off of that. (I forget what the method is off hand.)

Then, pass that IP address to $this->isDenied($ip).

Here, call $request = $event->getRequest(); That gives you the request object. You can then get the IP address off of that. (I forget what the method is off hand.)

thats what i found
$ip = $event->getRequest()->getClientIp();

$dispatcher->addSubscriberService('subscriber.ban', '\Drupal\ban\EventSubscriber\BanSubscriber');

the moment i uncomment this line, i get a white page :( no error or anything:/ HTTP status 200 but nothing outputs

First point: Yep, that looks about right.

Second point: Uh, hm. Turn on display_errors in php.ini or your htaccess file and see if it says something more useful? (I may have gotten the syntax wrong.)

yeah this is my dev enviorment and thats what i find weird..
I have

display_errors = On
display_startup_errors = On
error_reporting = E_ALL

But i had to check apache error logs to find this.Browser just outputs white screen:

[Sat Oct 20 23:15:43 2012] [error] [client 127.0.0.1] Exception thrown when handling an exception (Symfony\\Component\\DependencyInjection\\Exception\\ServiceCircularReferenceException: Circular reference detected for service "subscriber.ban", path: "subscriber.ban".), referer: http://localhost/d8/

Hm. What's your full build method look like? It sounds like the identifier subscriber.ban may simply be used already. :-)

thats the full class

/**
 * @file
 * Definition of Drupal\ban\BanBundle.
 */

namespace Drupal\Ban;

use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Reference;
use Symfony\Component\HttpKernel\Bundle\Bundle;

use Drupal\Core\Database\Database;

/**
 * Ban dependency injection container.
 */
class BanBundle extends Bundle {

  /**
   * Overrides Symfony\Component\HttpKernel\Bundle\Bundle::build().
   */
  public function build(ContainerBuilder $container) {
    $dispatcher = $container->get('dispatcher');
    $container->register('subscriber.ban')->addArgument(new Reference('database'));
    $dispatcher->addSubscriberService('subscriber.ban', '\Drupal\ban\EventSubscriber\BanSubscriber');
    $dispatcher->addSubscriber(new \Drupal\ban\EventSubscriber\BanSubscriber());
  }
}

Oh, duh the register() line is wrong. It doesn't specify what class to use. :-) That's what I get for writing code samples in Dreditor...

Try this:

  public function build(ContainerBuilder $container) {
    $dispatcher = $container->get('dispatcher');
    $container->register('subscriber.ban', '\Drupal\ban\EventSubscriber\BanSubscriber')->addArgument(new Reference('database'));
    $dispatcher->addSubscriberService('subscriber.ban', '\Drupal\ban\EventSubscriber\BanSubscriber');
  }

I suppose for bonus points you can make the table name a parameter to the listener, too, or factor the isDenied logic out into a separate IP blocking managing class and make the listener depend on that, instead. I don't know if you want to go that far in this patch but that would be the ideal, and most testable.

Thanks for the guidance Crell and Berdir.
Ok here is so far the work, i am willing to do what Crell suggested, but i guess i will have time in the weekend.

So if anyone wants to finish this off before that here is the working patch with Crell's corrections.

Setting it to NR just to check with bot

Ok, i couldn't let this go.. i was passing class names wrongly. it doesnt need the starting \.

But when i fixed this, event wouldnt fire...i cant make addSubscriberService work and best docs i could find describe what i am already doing:/

The BanSubscriber::getSubscribedEvents() fires, it is being registered into listenerIds..but thats it..i lose track and onKernelRequestBannedIpCheck() method never fires..and the only error i get its in dblog when i clear cache, where i get a WSOD :

Symfony\Component\HttpKernel\Exception\NotFoundHttpException: Unable to find the controller for path "/admin/config/development/performance". Maybe you forgot to add the matching route in your routing configuration? in Symfony\Component\HttpKernel\HttpKernel->handleRaw() (line 118 of /var/www/d8/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php).

Nothing else in dblog or apache error logs..

thats it..till next time

Sorry, but I think that @Crell's direction to use Request::getClientIp() is not correct, at least for this issue. The issue/task that intends to do that has to convert all functionality and behavior as well as tests of ip_address() to Request::getClientIp() (which is another case of settings.php variables/config being passed to service, though the service is Request this time). Doing that will also have to ensure that getClientIp() actually detects all forms of reverse-proxies we're able to detect currently, which may or may not require upstream patches. IMHO, that's completely out of scope for this issue.

Aside from that, needs review for latest patch.

sun: Request does handle reverse proxies, you just have to set a flag on the class rather than setting a variable_get(). If there's another issue to eliminate ip_address() in favor of Request::getClientIP(), I'm OK with leaving it as is for now if there's a @todo with a link to that issue. (If not, then we need such an issue created.)

The linked issue was closed as won't fix, so we should try to get rid of the variable_get() here I think.

Not that this can't really be config() as the point of its existence is to avoid a db query.

I see two options:

a) This is is an optional module now, if you don't want the functionality, just disable it. So, just remove that feature.
b) As documented, this is supposed to be set in settings.php, so we could just use global $conf, like we're doing elsewhere currently to avoid a variable_get() dependency.

At the moment, I'd tend towards a). Opinions?

Quoting myself from #1479466-71: Convert IP address blocking to config system:

1. There's room for turning ban_boot() into an event subscriber, let's do that.
2. We can and probably should re-introduce sane support for IP address wildcards (not funky/strange access domain rules) as well as IPv6, let's do that.
3. There might even be room for making the storage pluggable for high-performance sites — but then again, the entire feature has loads of comments all over the place, which say that high-performance sites won't use it in the first place. Ban is a module now, which isn't enabled by default and which you don't need to enable, so who fucking cares? Let's completely remove that high-performance stuff entirely.

Let's cover the 80% use-case and let's ensure it works.

Thus, my concrete recommendation is to remove the $conf/variable stuff entirely. There's a single database table that is queried, and only that.

Everyone who thinks that this is slow can install better_ban.module from contrib, or even better, be informed that iptables and other OS-level tools are the proper answer if you really care about performance.

Totally +1 to that :)

I agree as well:

Here is my work so far..No interdiff since, pretty much a lot changed..
solution to my problem above was doing
->addTag('event_subscriber') in the bundle..addSubscribeService directly was not working:/

1. Removed variable_get call
2. Added a new class to check the ip so listener does not have to do it
3. Switched back to ip_adress() with a @todo note

Killing the variable_get() entirely++

My previous statement regarding a @todo on ip_address() still stands.

ipv6 support and wildcards would be good, but is a separate issue from this one. Retitling so that we can finish up here and then move on.

This patch also removes reference from default.settings.php

Almost done! :)

+++ b/core/modules/ban/ban.admin.inc
@@ -6,11 +6,14 @@
+ *   A renderable array.

I think we call these "render array" everywhere else.

+++ b/core/modules/ban/ban.module
@@ -2,7 +2,7 @@
+ * Allows capability to ban individual IP addresses.

-capability

+++ b/core/modules/ban/lib/Drupal/ban/BanBundle.php
@@ -0,0 +1,31 @@
+/**
+ * Ban dependency injection container.
+ */
+class BanBundle extends Bundle {

Hum... sounds odd. We don't have a guideline for kernel bundle classes yet, so for now, I'd recommend:

"Defines the Ban bundle."

+++ b/core/modules/ban/lib/Drupal/ban/BanBundle.php
@@ -0,0 +1,31 @@
+    $container->register('ban_ip_manager', 'Drupal\ban\BanIpManager')

I think it would be a very wise idea to namespace service IDs of bundles with their corresponding module names; i.e.:

s/ban_ip_manager/ban.ip_manager/

+++ b/core/modules/ban/lib/Drupal/ban/BanIpManager.php
@@ -0,0 +1,58 @@
+   * @param string $ip
+   *   The IP address to check.
+   *   If omitted or empty the IP of the current user will be used instead.
...
+  public function __construct(Connection $connection, $ip = '') {

Since this is registered as a service, the class won't ever be constructed with the optional $id parameter, so I think this optional argument can be dropped without replacement.

+++ b/core/modules/ban/lib/Drupal/ban/BanIpManager.php
@@ -0,0 +1,58 @@
+    // @todo convert this to Request::getClientIP() ?

@todo Convert ip_address() to Request::getClientIP().

+++ b/core/modules/ban/lib/Drupal/ban/EventSubscriber/BanSubscriber.php
@@ -0,0 +1,63 @@
+  static function getSubscribedEvents() {

Another thing we should not learn from Symfony is to put these metadata-methods to the bottom of event subscriber classes. Instead, it's tremendously helpful to see them first in a class, since this definition is crucial to know.

Doesn't necessarily need to be adjusted in this patch, since we need to polish all event subscribers anyway.

Aside from these minor things, this looks good to go for me.

Hum... sounds odd.

Yeah this seems like a copy-past failure as well:/

I think it would be a very wise idea to namespace service IDs of bundles with their corresponding module names; i.e.:

s/ban_ip_manager/ban.ip_manager/

if i got this correctly:
I renamed both services provided:
ban_ip_manager to ban.ip_manager
and
subscriber.ban to ban.subscriber

Since this is registered as a service, the class won't ever be constructed with the optional $id parameter, so I think this optional argument can be dropped without replacement.

Yes, but doesnt it makes it more testable? eg you can provide different ip addresses

Besides the last one, all other requested changes are incorporated into this patch..

+++ b/core/modules/ban/lib/Drupal/ban/BanIpManager.php
@@ -0,0 +1,58 @@
+  public function __construct(Connection $connection, $ip = '') {
+    $this->connection = $connection;
+    // @todo convert this to Request::getClientIP().
+    $this->ip = !empty($ip) ? $ip : ip_address();
+  }

Per comment #22, actually, the constructor shouldn't even have an IP address in it at all, and there should be no IP address property. Instead, isDenied() should be passed an IP address as a string, and it checks that. The ip_address() call (and associated documentation) should be moved to the listener.

+++ b/core/modules/ban/lib/Drupal/ban/EventSubscriber/BanSubscriber.php
@@ -0,0 +1,62 @@
+  /**
+   * Registers the methods in this class that should be listeners.
+   *
+   * @return array
+   *   An array of event listener definitions.
+   */
+  static function getSubscribedEvents() {
+    $events[KernelEvents::REQUEST][] = array('onKernelRequestBannedIpCheck', 40);
+    return $events;
+  }

By convention, the getSubscribedEvents() method should be the last method in the class.

+++ b/core/modules/ban/lib/Drupal/ban/EventSubscriber/BanSubscriber.php
@@ -0,0 +1,62 @@
+  public function onKernelRequestBannedIpCheck(GetResponseEvent $event) {
+    if ($this->manager->isDenied()) {
+      $response = new Response('Sorry, your IP has been banned.', 403);
+      $event->setResponse($response);
+    }
+  }

Per above, this should call ip_address() [old style] / $event->getRequest()->getClientIp() [new style] and pass that value to isDenied($ip).

That way, the ban checking object itself can be reused on any arbitrary IP address at an time to see if something *would* be banned. It's not hard coded to whatever IP address it was instantiated with.

See, @Crell is obviously married to blindly following Symfony's order of class methods in event subscribers, regardless of whether it makes any sense. ;) That's why I mentioned that the adjustment could potentially be left out, as we need to re-evaluate and polish those classes anyway later on. Thus, let's just put it back to the end for now. I'm sorry for the confusion and inconvenience caused.

Thanks for the explanation:)

• Moved getSubscribedEvents to the bottom, since thats how it is being used. Let's change it to another issue, if at all.
• Moved ip_address() call to subscriber and passed it as an argument to isDenied()

Edit: @sun: np:)

You are amazing,i am not very good at about this.
Feel confused and keep studying from you nowinginging.

Thanks! This looks ready to fly for me, in case the testbot will confirm.

LOL. I hadn't actually seen sun's comment about moving it when I posted that. :-) In any case, yes, if the bot approves of this then so do I.

The failure is due to #1825568: Random test failure in Drupal\system\Tests\Entity\EntityFormTest
I think there is no need to re-test this

#46: 1794754-ban_followup-45.patch queued for re-testing.

Now that ban is an optional module I think it's fine to remove the settings.php hack. Everything else here looks fine so I've gone ahead and committed/pushed this.

why +use Drupal\Core\Database\Database; in BanBundle.php ?

Yeap no reason to use database there.
removed the line and played around after clearing cache. everything works.

nice catch yched

We have dozens of those stale use lines in core, we always forget to remove them and almost every file that I open with Netbeans marks some uses as unused. I guess having a coder review check for that would be useful.

This is RTBC.

Committed/pushed the follow-up, thanks!

See #1929506: Banned IP addresses can still access cached pages which (I believe) is an interesting side effect of this issue.

#60 FYI #1833442-74: Remove hook_boot()

Updated issue summary.