BotBlocker-Minecraft / BotBlocker

Automatically ban MC bots that log into your server.
GNU General Public License v3.0
8 stars 0 forks source link

Ban method? Customizable? #2

Open ScrapMetalGolem opened 8 months ago

ScrapMetalGolem commented 8 months ago

Summary

I don't know the current method the plugin uses to ban the player because we haven't gotten to ban any users with it yet (I assume vanilla ban). But I would like to use banmanager and set a custom reason/message to redirect any legit user to get unbanned.

Basic example

banmanager:ban username reason (like "Suspicious Activity, visit [discord link] to appeal)

Motivation

I expect that if any legit new player gets caught in a false positive, this will give them an opportunity to 1) let me know about it and 2) get unbanned so they can play.

AitorAstorga commented 8 months ago

Hi, I'm working on it. The plugin currently uses the Bukkit.getBanList(Type.NAME).addBan method. In the Fabric version I do something similar with getUserBanList().add(new BannedPlayerEntry. For your points:

  1. There is a console log that reads "Player was banned for disconnecting within seconds of joining for the first time - suspected bot.", so you will get at least this info when an account is banned.
  2. I will first modify the plugin to allow setting a custom ban text. If this works correctly I will start working for integration with banmanager, I have never used it so I'll have to find about it.

My idea is to add new commands:

ScrapMetalGolem commented 8 months ago

That's good news, thanks for maintaining this tool.

AitorAstorga commented 8 months ago

Please check BotBlocker-1.4.0-SNAPSHOT. Also check the Changelog.

I have added a check so if the BanManager plugin is in BotBlocker will use its API to ban instead of the default Bukkit method. I tested it in my server and could see the banned user in the list correctly and unban it afterwards. If it works for you then I will set it as latest release.

ScrapMetalGolem commented 8 months ago

First attempt with the 1.4.0 version appeared to not work. I followed these steps:

However, I decided to try again. I replaced 1.4.0 with 1.3.0 and tried the same method. The account was banned and everything worked as expected for that version. Then I:

I'm able to correctly unban the account and rejoin the server with it. The only thing I don't fully understand is that the account UUID is not added to the players.yml file anymore, even after playing for a while and leaving the server. The account does seem immune to the plugin now though, so I have to assume that data is being stored elsewhere now (in the banmanager database?)

I will test this further when I can get some people with untainted accounts to join and test for me, maybe I'll get a different result when an account that never joined before tries it.

AitorAstorga commented 8 months ago

Wow, that's some good effort, thank you!

TL;DR: Weird errors are weird. Let's test more. I propose renaming players.yml to whitelist.yml and improving logs.

BanManager seems to be using its own database, probably in local_bans.mv.db, while the vanilla ban adds an entry into banned-players.json. Not that I really cared too much about that before, as I just use the BanManager API methods and they handle that, but this is definitely an issue for compatibility between Vanilla bans and BanManager bans. That is, as previous BotBlocker used vanilla, those bans will remain in banned-players.json.

1.3.0 -> 1.4.0 update error

Your issue when installing the new version confuses me a bit. In the first try you cleaned both players.yml and banned-players.json. Good call. But it didn't work. And then after rolling back to 1.3.0 and getting a ban (so players.yml should be clean) and also cleaning banned-players.json again (so, we should be in the same state as in the beginning), this time it worked! Also strange that there were no errors popping up. Did you see anything in the log file?

Account not added to players.yml

The account not being added to players.yml is unexpected too. Even more if it is working regardless. Some weirdness must be going on over here:

if (timeConnected < timeLimit) {
    String playerName = event.getPlayer().getName();

    // Check if BanManager is installed and enabled
    Plugin banManager = Bukkit.getServer().getPluginManager().getPlugin("BanManager");
    if (banManager != null && banManager.isEnabled()) {
        // Use BanManager to ban the player
        banWithBanManager(event.getPlayer(), banMessage);
    } else {
        // Fallback to Bukkit's default banning system
        Bukkit.getBanList(Type.PROFILE).addBan(playerName, banMessage, null, "BotBlocker");
    }

    getLogger().info("Player '" + playerName + "' was banned for disconnecting within " + timeLimit + " seconds of joining for the first time - suspected bot.");
} else {
    // Add the player to players.yml if it is not banned
    playersCfg.set(playerId.toString(), true);
    savePlayers();
}

What the code above does is simply check if a user must be banned and ban him (either vanilla or BanManager). And if the user is legit, it is added to the players.yml file. I could understand users not getting added here, but the fact that even with that the immunity is working is even more concerning. My guess would be that it is not working at all, but you mentioned that it correctly bans an alt account, so...

I will try to do the same from my side, testing with different accounts. Hopefully I have time for that this weekend.

Regardless, I have some ideas:

  1. I notice that users are added to the players.yml only as a whitelist if they are legit users, but supposed bots are not, they just get banned. Maybe it would make sense to:
    • a) Rename the file to whitelist.yml for clarity. If players.yml is found, rename this file.
    • b) Improve the logging to include messages such as "User x banned." or "User y whitelisted". Also a distinction if using BanManager or vanilla ban.
  2. I should stop using true blankly in the players file. I just did because the method for it is convenient, but I see that it may lead to confusion. Perhaps I should only set it to true but accept that an admin may change it to false without removing the line.
  3. Some config is loaded at the startup. I should either clarify that to avoid problems or make it reload it more often (periodically, after any command issued...).

And of course, if you've read this far, I accept suggestions :)

ScrapMetalGolem commented 8 months ago

Further testing provides these clues:

[14:53:12 WARN]: java.lang.NullPointerException: Cannot invoke "me.confuser.banmanager.common.data.PlayerData.getUUID()" because "this.actor" is null
[14:53:12 WARN]:        at BanManagerBukkit.jar//me.confuser.banmanager.common.data.PlayerBanData.equalsBan(PlayerBanData.java:83)
[14:53:12 WARN]:        at BanManagerBukkit.jar//me.confuser.banmanager.common.runnables.BanSync.newBans(BanSync.java:42)
[14:53:12 WARN]:        at BanManagerBukkit.jar//me.confuser.banmanager.common.runnables.BanSync.run(BanSync.java:26)
[14:53:12 WARN]:        at BanManagerBukkit.jar//me.confuser.banmanager.common.runnables.Runner.run(Runner.java:27)
[14:53:12 WARN]:        at org.bukkit.craftbukkit.v1_20_R3.scheduler.CraftTask.run(CraftTask.java:101)
[14:53:12 WARN]:        at org.bukkit.craftbukkit.v1_20_R3.scheduler.CraftAsyncTask.run(CraftAsyncTask.java:57)
[14:53:12 WARN]:        at com.destroystokyo.paper.ServerSchedulerReportingWrapper.run(ServerSchedulerReportingWrapper.java:22)
[14:53:12 WARN]:        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
[14:53:12 WARN]:        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
[14:53:12 WARN]:        at java.base/java.lang.Thread.run(Thread.java:833)

To clarify a few things: