Lack of interest [Developer Tool] Allow custom IpMatch entries

[DragonByte Tech]

The IpMatch system has the potential to be used by 3rd party add-ons to automatically ban IP addresses for various infractions, but currently using the XF:Banning :: banIp method requires that a valid user record is either passed as an argument, or exists in \XF::visitor().

This is problematic because of things like automatically updating IP block lists, where even if the Cron job is executed by an user, that user should not be recorded as the banner of the IP address.

Therefore I suggest removing the 'required' => true part of the IpMatch entity, and either change the existing banIp method to allow for passing "no user, and don't default to visitor", or creating a new method like banIpAutomatic which doesn't take a user argument.

Use case: Our Security addon has an option to block TOR Exit Nodes, and a cron job that periodically updates the list of exit nodes for the server IP in question. While it's technically possible to insert records into the xf_ip_match table manually and rebuild the banned IPs cache manually, for compatibility with other 3rd party mods I would prefer if I was able to use the repository method for banning IP addresses.

---------------
EDIT: This is no longer needed thanks to the instructions from Mike

The IpMatch system has the potential to be used by 3rd party add-ons to create their own filtering lists separate from Discouraged and Banned, but unfortunately the match_type column in the xf_ip_match table is an ENUM with fixed value options, rather than a VARCHAR.

The Banning repository is already dynamic enough that addons wishing to add their own types of bans can do so without any further coding changes.

Use case: Our Security addon has an option to block TOR Exit Nodes, and a cron job that periodically updates the list of exit nodes for the server IP in question. While it's technically possible to insert these IPs as ban entries, the problem is that toggling that setting via the AdminCP would necessitate pruning those IPs (approx. 850 entries) and re-adding them whenever the setting is toggled.

If I had the ability to add a new match type, I could add the "dbtech_security_tornode" match type and only rebuild that cache whenever the IP list is updated, and the setting I mentioned above would only toggle whether this ban was in effect.


Fillip

 

[Mike]

Just to point out, the enum isn't inherently a blocker, at least in the manner it was in XF1. The schema manager has the ability to add and remove values from an enum without affecting the full list, so if other add-ons/a new version changes the list, your changes won't interfere with them.

As an example, from the 2.0 upgrade:

$table->changeColumn('user_state')->addValues(['rejected', 'disabled']);

 

[DragonByte Tech]

The schema manager has the ability to add and remove values from an enum without affecting the full list, so if other add-ons/a new version changes the list, your changes won't interfere with them.

Interesting, thanks for that

It is still a bit more trouble than necessary because I'd also have to hook into the entity to update the allowedValues but it's nice to know it isn't a total blocker.


Fillip

 

[Chris D]

It is still a bit more trouble than necessary because I'd also have to hook into the entity to update the allowedValues but it's nice to know it isn't a total blocker.

To be fair, even if it was already a VARCHAR, we would probably still use allowedValues in the entity to constrain it to known types. So you'd likely need to do that either way

 

[DragonByte Tech]

To be fair, even if it was already a VARCHAR, we would probably still use allowedValues in the entity to constrain it to known types. So you'd likely need to do that either way

Fair enough

The only other thing I'd suggest is removing the "required" flag from the create_user_id column, since auto added bans via things like a cron job or automatically configured actions won't have a valid and/or correct user record.

The use case for this is again our Security mod; we have various "Watchers" that can trigger on certain actions, e.g. "5 failed AdminCP login attempts from the same IP address in the past 1 hour(s)", and admins will be able to configure the automatic banning of that IP address.

I've updated the OP with a revised suggestion


Fillip

 

[Mike]

In terms of the creation user ID, required in the context of an integer generally just means that you have to try to set a value for it. 0 would be valid (and would likely make sense in the context of automated actions).

 

[DragonByte Tech]

In terms of the creation user ID, required in the context of an integer generally just means that you have to try to set a value for it. 0 would be valid (and would likely make sense in the context of automated actions).

Fair enough, although the second half of the suggestion still applies; the existing banIp method forces a user record, so a new method in the Banning repository would still be appropriate IMO.


Fillip