Introduce the ability to use CIDR Notation to ban IP-Ranges

I created advban module as core Ban module fork and add some features: range ban, expire ban. I planned add whitelist and other features also. These changes required the addition of new columns to the table. My features may be included to Drupal's 9 Ban module.

Add range IP ban

Try apply the same patch with absolute paths

Fix error: wrong table name

Fix some errors and change by coding standarts. But don''t understand about unknown ip_end column which added on ban.install

• Fix display message on DeleteForm
• Change query on isBanned() to dynamic mode (maybe Test will be passed)

Change db_add_field to construction like $injected_database->schema()->addField

• Add IP range check on isBanned()
• Change all queries type to dymamic
• Add check exists of the field ip_end
• Change ban_ip alias for queries

Add default arguments to function definitions

Interesting idea. To be honest it feels like ban module could totally actually live outside of core. What are your thoughts about that?

1. +++ b/core/modules/ban/ban.install
   @@ -25,6 +27,13 @@ function ban_schema() {
   +      'ip_end' => [
   +        'description' => 'IP address (end of range if not empty)',
   

   How about naming it ip_range_end, so its a bit more obvious what this field is about?

2. +++ b/core/modules/ban/src/BanIpManager.php
   @@ -29,41 +29,93 @@ class BanIpManager implements BanIpManagerInterface {
       */
   -  public function isBanned($ip) {
   -    return (bool) $this->connection->query("SELECT * FROM {ban_ip} WHERE ip = :ip", [':ip' => $ip])->fetchField();
   +  public function isBanned($ip, $ip_end = '') {
   +    $query = $this->connection->select('ban_ip', 'bip');
   ...
       * {@inheritdoc}
       */
   -  public function banIp($ip) {
   +  public function banIp($ip, $ip_end = '') {
   +    if (empty($ip_end)) {
   +      $fields = ['ip' => $ip];
   

   I'm wondering whether for better BC there should be additional methods instead. We could then also have some state variable to store whether we need any range checking at all. This way runtime level checks could stay basically the same for existing sites.

3. +++ b/core/modules/ban/src/BanIpManager.php
   @@ -29,41 +29,93 @@ class BanIpManager implements BanIpManagerInterface {
   +    if (!$is_banned && $this->connection->schema()->fieldExists('ban_ip', 'ip_end')) {
   

   Why do you have to check for the existence of this field? This would be quite a slow query on every request

Dawehner, thank you for review. I made Advanced ban (advban)

1. Ok, I will change the field name

2. I will fix the query code, after testing

I use second argument for ban functions as array in my advban module. So no need create new functions. For example, I use expire time for banning

3. This code need for passing drupal.org test only. It will be removed on productive.

I will do create new patch, thank you

1. +++ b/core/modules/ban/ban.install
   @@ -33,3 +42,20 @@ function ban_schema() {
   +  $database = \Drupal::database();
   +  $schema = $database->schema();
   

   it could be oneliner
   + $schema = \Drupal::database()->schema();

2. +++ b/core/modules/ban/src/BanIpManager.php
   @@ -29,41 +29,93 @@ class BanIpManager implements BanIpManagerInterface {
   +    if (!$is_banned && $this->connection->schema()->fieldExists('ban_ip', 'ip_end')) {
   

   it looks strange that code expects that field could be missing

3. +++ b/core/modules/ban/src/BanIpManager.php
   @@ -29,41 +29,93 @@ class BanIpManager implements BanIpManagerInterface {
   +      $group_ip = $query->andConditionGroup()
   +        ->condition('bip.ip', $ip_long, '<=')
   +        ->condition('bip.ip_end', $ip_long, '>=');
   +      if (empty($ip_end)) {
   +        $query->condition($group_ip);
   ...
   +      else {
   +        $ip_end_long = ip2long($ip_end);
   

   This code needs comments cos not clear how conditions grouped and how that affects performance because no index on new field and `>=` for strings could have visible effect

andypost, thank you for review

1. I just copy the code from /core/system/system.install. I will fix it

2. Without this table existing checking test do not passed. I created topic which referenced to this one. I will try again to pass the test after removing this code.

3. Range IP stored long values to the ip and ip_end fields. I'm looking for such intervals for which the address is inside, for start IP and for end IP. I will add comments.

1. Change field ip_end name, fix some
2. Change IsBanned() functions
3. Add tests for range ban
4. Not removed $this->connection->schema()->fieldExists('ban_ip', 'ip_end') yet

remove $this->connection->schema()->fieldExists('ban_ip', 'ip_end')

Create patch from 8.7-dev version

It looks like when you run process that will update the database and add a new field, it checks the IP address, which uses a new field that has not yet been created. If this is a feature of the Ban module, the test can be considered as passed.

Or, as was previously done and criticized, add a check for the existence of a new field.

Add new field exists checking

Delete new column exists check and add try/catch block

1. +++ b/core/modules/ban/ban.install
   @@ -5,6 +5,8 @@
   +use Drupal\Core\Database\Database;
   +
   

   This can be removed as it not used.

2. +++ b/core/modules/ban/src/BanIpManagerInterface.php
   @@ -31,16 +33,20 @@ interface BanIpManagerInterface {
   +    * @param string $ip_range_end
   

   Code style issue. Remove 1 space at the beginning of the line.

3. +++ b/core/modules/ban/src/BanIpManagerInterface.php
   @@ -53,4 +59,11 @@ interface BanIpManagerInterface {
   +  public function canRangeCheck();
    }
   

   Code style issue. Add a new line after the last method.

4. +++ b/core/modules/ban/src/Form/BanAdmin.php
   @@ -103,14 +111,55 @@ class BanAdmin extends FormBase {
   +        'ip_range_end' => $ip_range_end,
   +        'range_check' => TRUE,
   +      ])) {
   

   Code style issues. Fix indentation needed.

5. +++ b/core/modules/ban/src/Form/BanAdmin.php
   @@ -119,8 +168,13 @@ class BanAdmin extends FormBase {
   +    $ban_message = empty($ip_range_end) ? $this->t('The IP address %ip has been banned.', ['%ip' => $ip]) :
   +      $this->t('The IP addresses from %ip to %ip_range_end have been banned.', ['%ip' => $ip, '%ip_range_end' => $ip_range_end]);
   

   Make those lines more readable, i.e.

   $expression ?
     $long_line_1 :
     $long_line_2;

@voleger, thanks for your review. I've added test for IP range also.

Fix testing error.

Fix path

There's contrib implementation https://www.drupal.org/project/restrict_ip

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Tagged for needs reroll

Think this would be a nice feature to still have!

Rerolled patch for 10.1.x.

Fixed CCF for #51.

Fixed the phpunit test errors in #51, thanks!

function ban_update_8700() {
This needs to be updated for D10.

Thanks for the review @smustgrave!🙌🏼

Updated the patch as requested.
Thanks!

Thanks for the quick turnaround

On Drupal 10.1. with a standard install
Tested the update hook does pass.
Uploading a screenshot of what the UI now looks like (added IS template while I was at it)
Running the tests without the fix I get

Drupal\Core\Database\DatabaseExceptionWrapper : SQLSTATE[HY000]: General error: 1 no such column: bip.ip_range_end: SELECT "bip"."iid" AS "iid"
FROM
"test70272775"."ban_ip" "bip"
WHERE ("bip"."ip" = :db_condition_placeholder_0) AND ("bip"."ip_range_end" = :db_condition_placeholder_1); Array
(
    [:db_condition_placeholder_0] => 16909316
    [:db_condition_placeholder_1] => 16909319
)

Changes look good

As well as the below comments I also wonder that if instead of arbitrary start and end IPs, we should support CIDR ranges, e.g. 192.168.0.0/16 to ban all of 192.168.0.0 to 192.168.255.255. That would be easier to store and parse.

1. +++ b/core/modules/ban/src/BanIpManager.php
   @@ -29,41 +29,138 @@ public function __construct(Connection $connection) {
   +      $group_ip = $query->andConditionGroup()
   +        ->condition('bip.ip', $ip_long, '<=')
   +        ->condition('bip.ip_range_end', $ip_long, '>=');
   

   Wondering if this works as intended in all cases, given that the ip and ip_range_end columns are defined as strings, but here we seem to be doing a numeric comparison.

2. +++ b/core/modules/ban/src/BanIpManagerInterface.php
   @@ -12,11 +12,13 @@ interface BanIpManagerInterface {
   +   * @param array $options
   +   *   Options array.
   

   What are the possible options? We need to document this.

3. +++ b/core/modules/ban/src/BanIpManagerInterface.php
   @@ -31,16 +33,20 @@ public function findAll();
   +   *   The end of the IP address to ban (optional).
   

   ban -> unban

   Also, what happens with overlapping ranges?

4. +++ b/core/modules/ban/src/Form/BanAdmin.php
   @@ -108,14 +116,55 @@ public function buildForm(array $form, FormStateInterface $form_state, $default_
   +          if (!$ip_long) {
   +            $form_state->setErrorByName('ip', $this->t('Only IPv4 is available for IP range.'));
   +          }
   +          if (!$ip_range_end_long) {
   +            $form_state->setErrorByName('ip_range_end', $this->t('Only IPv4 is available for IP range.'));
   +          }
   

   None of the UI changes in this patch (such as these lines above) have test coverage.

The Ban Module was approved for removal in #1570102: [Policy] Deprecate Ban module.

This remains Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

The deprecation work is in #3482198: [meta] Tasks to deprecate the Ban module and the removal work in #3488827: [meta] Tasks to remove Ban module.

Ban will be moved to a contributed project after the Drupal 12.x branch is open.

Moved to contrib queue

I think it should just allow CIDR notation and use standards, instead of inventing an own notation?
Maybe for adding IPs a textarea could be used?

And let's please use a MR instead of patches.

Using IpUtils here like in #3392147: Add an allowed IP list to the Ban module should help a lot providing IP block support and unify things. Let's do that!
If someone needs more, that should be done in a follow-up imho.

All done, please review!

I feel like this is the better approach even though, we pull the entire database table column in memory.

Thanks @grevil, yes I also think this is a good, standardized and consistent solution now!

Some final points I'd like to add are:

• Documentation about valid syntax where the IPs are entered (and maybe use a multilane textarea in a follow-up?)
• Add tests for the new CIDR notation.

Maybe someone else here can also review or proceed?

@vloeger, thanks for the suggestions, I think the validator should be a simple helper class. I don't think we need to decorate it or inject it at any point. It should simply hold two simple static validation methods, that's it. But of course I am open for discussion 🙂.

Due to the (even if small) fundamental changes and possible performance implications, I'd like to wait for feedback by @mstrelan.

I'd need to have a better look, but I'm not at all comfortable with this query with no condition or limit:

$banned = $this->connection->query('SELECT [ip] FROM {ban_ip}')?->fetchCol();

Particular in light of #3153262: Add pagination to banned IPs view

I think there are some tricks to query this directly. Postgres has built in support for this but Mysql doesn't. I think you can convert the cidr to an int and store that, maybe in a separate column, and then you can do a BETWEEN query.

You might want to see if @berdir has some feedback on this

@mstrelan thanks for the feedback! Yea that might be a problem if we pull 7000+ ip strings in PHP memory... we are currently not really in need of this feature, so if anyone wants to finish it, feel free to!

@berdir any thoughts on this?

Fetching all IP's is definitely a no-go.

I also saw stuff about this in advban, but didn't investigate closely, but even that then adds a second query to every request with complex conditions.

As mentioned in the advban issue, I'm personally also not too interested in having support for this and think it should definitely be optional to keep overhead of this minimal as this is something that runs on every request, even page cache hits. One option for that would be putting this feature into a separate, optional module.

Vague thoughts to consider:
* Store them in a separate table, easier to load all then.
* Cache them, possibly keyed by the first part (I've also thought about this for regular IP lookups, but that might be more complexity than it's worth)
* Implement something like what advban does, but as mentioned, that's then multiple queries per request.
* backend specific implementations if postgresql can do this natively.

Thanks @berdir I agree with that. A further question is, if CIDR syntax is enough or if we need a broader "range" functionality or just a textarea for multiple entries?

I'd vote for the textarea and CIDR only. While the textarea should go into the main module, CIDR should be a submodule IMHO.
The module should allow that.

A single textarea for CIDR ranges sounds sensible. That also fits pretty well with what I know from Upsun: https://fixed.docs.upsun.com/environments/http-access-control.html#filte... (not sure we should support the allow/ban feature, that sounds complicated in combination with single IP's, I think just blocking is fine)

Still wondering how to most efficiently get that information into the super-early ban middleware. We don't want to read config and inject the ConfigFactory there, see my auto_unban issue. it could be put directly in a service parameter in a ServiceProvider class, like language module does. But every change would require a pretty expensive container rebuild. A fast chained bootstrap cache might be acceptable, reading config only on a miss, with a service closure.

Based on the feedback I'd say we should postpone this. Next step would be to move the range / CIDR functionality into a submodule to make it optional and describe the possible negative performance impacts, I'd say?

Based on the valid reasons given above I'll close this won't fix. For anyone who needs a quick solution and is aware of the performance implications, please use https://www.drupal.org/project/advban

I'll add some information to the module page.

Done! @goodboy I also linked your modules on the module page now, would be great if you could do a comparison vice-versa. Blocking IP-ranges is and will be a USP of your module now and in the future :)