banPlayer doesn't kick all players using same IP

[FileEX]

Describe the bug
If a player has two computers and gets banned on one, it can no longer be banned on the other with the IP ban option. banPlayer returns false

To reproduce
You must have two computers

1. Join from computer A
2. Join from computer B
3. Ban player from computer A with ban ip
4. Try ban player from computer B with ban ip
   -->

Expected behaviour
banPlayer should automatically check if an IP ban already exists. If so, it should ban only the serial or the same IP, but with a different (next) serial.

Screenshots

Version
1.5.8

Additional context

[botder]

I don't understand the bug description.

A single person plays on the same server with two computers. That means you have two different serials, but the same IP is present.

You ban one player (one of his computers running the client) with the IP option. At this point I would expect both players to be banned one after another (almost simultanenous).

Are you saying the second player remains on the server and you can't ban him with the IP, because the IP was already banned?

[PlatinMTA]

I guess he wants to add two bans with the same IP but different serial.

I wouldn't ever trust an IP ban to actually do anything tho, it's to simple to change an IP, specially if your ISP has a good DHCP and your ip is changed every router restart or 12/24 hours.

[LosFaul]

in general since many people have dynamic ips
ip bans longer than 48 hours are not worth it

also basicly all ip v4 addresses are used up, so it's not unusual that end user devices can have the same public ip

[FileEX]

I guess he wants to add two bans with the same IP but different serial.

I wouldn't ever trust an IP ban to actually do anything tho, it's to simple to change an IP, specially if your ISP has a good DHCP and your ip is changed every router restart or 12/24 hours.

Yes, i want add two bans with different serial but the same IP address

[PlatinMTA]

Yes, i want add two bans with different serial but the same IP address

Yeah, it would be nice if that could be possible

[jlillis]

... then just ban the different serial? The IP is already banned.

[Lpsd]

Deleted previous response and re-opened, issue wasn't really clear but after reading again I understand.

Player A and B are connected to same server, and both players have same IP
Ban player A (only player A gets kicked, doesn't matter if player B has the same IP)
Ban player B, but there's already a ban with that IP so it returns false

There's some important information missed in your initial issue.

What arguments do you supply to banPlayer when banning player A? Do you supply different arguments when trying to ban player B?

If using default arguments for both, and the issue still occurs, the solution here would be to check if anyone else in the server has that IP address and kick them too.

Your original proposed solution of forcing the serial to be used in this case only introduces unexpected behaviour.

Please confirm the above information and/or include a test resource so we can clarify the exact issue you're having.

[PlatinMTA]

I tried using addBan() to ban two different serials with same IP, they both get banned but the second ban doesn't add the IP in the ban.xml

Apart from that, i think i had the same issue with banPlayer() before (some of my staff said that they tried to ban an user and the ban just didn't work for some reason). Also having a test resource would requiere at least two players with the same IP to test, or a VM and the AC disabled.

banPlayer() should automatically kick every player that matches the IP, at least imo. Scripters shouldn't have to worry about that, also addBan already does that.

[Lpsd]

I tried using addBan() to ban too different serials with same IP, they both get banned but the second ban doesn't add the IP in the ban.xml

What else are you expecting to happen here? You try to ban the same IP twice, and two unique serials. Therefore you end up with a single entry for the IP, and two entries for the serial. There wouldn't be any sense in having two entries for the same IP in the ban list?

To clarify, the actual issue here is that when a player is IP banned, if other players in the server have the same IP they won't be kicked too? That does sound like an issue, if it's true.

[PlatinMTA]

To clarify, the actual issue here is that when a player is IP banned, if other players in the server have the same IP they won't be kicked too? That does sound like an issue, if it's true.

Pretty sure it doesn't kick them but I should have to test.

Regarding the other issue, it could be good for knowing which IP address are related to the serial, and in the case that you unban the first serial that doesnt mean that the IP is unbanned. However as I said before, IP bans are dumb and they arent that important.

[sbx320]

To clarify, the actual issue here is that when a player is IP banned, if other players in the server have the same IP they won't be kicked too? That does sound like an issue, if it's true.

I've just looked at the responsible code and yes, that is the case. banPlayer will only remove the specified player and no others.

[jlillis]

I think the original intent may have been to have banPlayer ban a single player, while addBan can be used to add a ban with more "detail". I don't agree with that if it was intentional - banPlayer should behave just like addBan.

[Lpsd]

Regarding the other issue, it could be good for knowing which IP address are related to the serial, and in the case that you unban the first serial that doesnt mean that the IP is unbanned. However as I said before, IP bans are dumb and they arent that important.

I'm not sure if we should make changes like that to the ban system, for backwards-compatibility reasons, as the expected behaviour changes. Not 100% sure without looking into the code but it seems that way.

It's very simple to create a script that tracks serials to IPs, but this method is also inconsistent since IPs can obviously be dynamic. For that reason I don't think we should introduce the functionality into MTA.

@sbx320 thanks for checking - yeah we should fix this.

[LosFaul]

i would always recommend to use serial over ip for banning anyway...

[Pirulax]

We really should start using a database for bans instead of XML.

[PlatinMTA]

We really should start using a database for bans instead of XML.

Yeah but it wouldnt be backwards compatible. Maybe we could add an option to have the bans saved on xml or sqlite, or even mysql 🤑

[sbx320]

It can easily be backwards compatible if we simply convert all bans into the database on server startup and then remove the banlist.

[ccw808]

What is the advantage of a database over an XML file?

[AlexTMjugador]

Maybe we could add an option to have the bans saved on xml or sqlite, or even mysql 🤑

<sarcasm>Yet another MySQL database, listening on some kind of socket to allow distributed access that will likely only ever be used locally by the MTA server, for a database that will never grow to a size big enough that storing it in a single file would become a problem? Sounds fantastic!</sarcasm>

Seriously, though, a SQLite database sounds okay, mainly because it allows enforcing some basic integrity constraints that are not intrinsic to XML, it should be faster for bigger ban sets due to the usage of indexes and other optimizations, and in general migrating or processing a relational schema if need be seems more more easy than a XML document. I know that XML files can get pretty structured via XSD, but I have not seen such schemas being used with MTA before. The most important downside I can think of is that this would make it harder for the server administrator to add bans manually, because they would need a specific program to open SQLite DB files.

Anyway, if the issue at hand is that banPlayer doesn't kick all the connected players that have some specific IP, I don't see how changing the ban information storage mechanism would improve that situation.

[Pirulax]

Every time we load/save the bans it needs to process the whole XML file. Also, searching for specific IPs/stuff is linear on the main thread, which isn't particularly efficient.

Changing XML to SQLite or something would help the code to process bans faster.

Anyways. The solution here would be to check whenever a specific IP-serial pair has been banned previously, instead of only checking for IP.

[Lpsd]

The actual issue here is that if multiple players share the same IP, and one of those players is banned by IP using banPlayer, the other players with that IP won't be removed from the server.

Other improvements / problems (such as the ones discussed above) should have their own issue opened.