From 27f1d30515579758a94807495375b9bf88f13bc7 Mon Sep 17 00:00:00 2001 From: Drefvelin <54400154+Drefvelin@users.noreply.github.com> Date: Mon, 19 Oct 2026 08:01:04 +0000 Subject: [PATCH 1/2] fix: stop resending bans after a restart and share bans across servers banned-players.json keeps ban times to the second, so a ban read back after a restart lost its milliseconds and was posted as a new ban. Ban times are now compared in whole seconds, including state files written before this change. Ban posts now carry the ban's start and end times so the API can tell when Main and Lobby report the same ban. With ban-mirror.sync (off by default), each server also applies bans and unbans made on the other servers that share the API. Co-Authored-By: Claude Opus 5.5 --- .../java/net/tfminecraft/tfmcweb/Cache.java | 2 + .../tfmcweb/api/ProvinceSystemClient.java | 170 +++++++++++++++++- .../tfmcweb/loaders/ConfigLoader.java | 1 + .../tfmcweb/managers/BanMirrorPoller.java | 132 +++++++++++++- src/main/resources/config.yml | 7 +- .../tfmcweb/api/ProvinceSystemClientTest.java | 29 ++- .../tfmcweb/managers/BanMirrorPollerTest.java | 130 +++++++++++++- 7 files changed, 445 insertions(+), 26 deletions(-) diff --git a/src/main/java/net/tfminecraft/tfmcweb/Cache.java b/src/main/java/net/tfminecraft/tfmcweb/Cache.java index 5c32f5f..3e1a25e 100644 --- a/src/main/java/net/tfminecraft/tfmcweb/Cache.java +++ b/src/main/java/net/tfminecraft/tfmcweb/Cache.java @@ -75,6 +75,8 @@ public final class Cache { /** Mirror this server's ban list (bans, unbans, expiries) to the Discord bot. */ public static boolean banMirrorEnabled = true; public static int banMirrorPollSeconds = 30; + /** Apply bans and unbans made on the other servers that share this API. */ + public static boolean banMirrorSync = false; /** Publish LuckPerms snapshots to this server's site. {@link #luckPermsBridgeApply} implies it. */ public static boolean luckPermsBridgePublish = false; diff --git a/src/main/java/net/tfminecraft/tfmcweb/api/ProvinceSystemClient.java b/src/main/java/net/tfminecraft/tfmcweb/api/ProvinceSystemClient.java index b46223c..d0dbc8f 100644 --- a/src/main/java/net/tfminecraft/tfmcweb/api/ProvinceSystemClient.java +++ b/src/main/java/net/tfminecraft/tfmcweb/api/ProvinceSystemClient.java @@ -10,10 +10,15 @@ import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Locale; +import java.util.Map; +import java.util.UUID; import com.google.gson.Gson; +import com.google.gson.JsonArray; import com.google.gson.JsonElement; +import com.google.gson.JsonObject; import com.google.gson.JsonParser; import net.tfminecraft.tfmcweb.Cache; @@ -120,20 +125,129 @@ public static CosmeticMintStatus fail(String error) { public static final class MirrorResult { public final boolean ok; public final boolean mirrored; + /** The API already had this ban or unban from another server. */ + public final boolean duplicate; public final String error; - private MirrorResult(boolean ok, boolean mirrored, String error) { + private MirrorResult(boolean ok, boolean mirrored, boolean duplicate, String error) { this.ok = ok; this.mirrored = mirrored; + this.duplicate = duplicate; this.error = error; } public static MirrorResult success(boolean mirrored) { - return new MirrorResult(true, mirrored, null); + return new MirrorResult(true, mirrored, false, null); + } + + public static MirrorResult duplicate() { + return new MirrorResult(true, false, true, null); } public static MirrorResult fail(String error) { - return new MirrorResult(false, false, error); + return new MirrorResult(false, false, false, error); + } + } + + /** One active ban from GET /skins/moderation/bans; expiresAt is 0 for permanent. */ + public static final class SyncedBan { + public final UUID id; + public final String name; + public final String reason; + public final String staffName; + public final long bannedAt; + public final long expiresAt; + + public SyncedBan(UUID id, String name, String reason, String staffName, long bannedAt, long expiresAt) { + this.id = id; + this.name = name; + this.reason = reason; + this.staffName = staffName; + this.bannedAt = bannedAt; + this.expiresAt = expiresAt; + } + } + + /** Active bans and recent unbans (player → unban time) recorded from every server. */ + public static final class BanSync { + public final boolean ok; + public final List bans; + public final Map unbans; + public final String error; + + private BanSync(boolean ok, List bans, Map unbans, String error) { + this.ok = ok; + this.bans = bans; + this.unbans = unbans; + this.error = error; + } + + public static BanSync success(List bans, Map unbans) { + return new BanSync(true, bans, unbans, null); + } + + public static BanSync fail(String error) { + return new BanSync(false, Collections.emptyList(), Collections.emptyMap(), error); + } + + static BanSync fromJson(String json) { + List bans = new ArrayList<>(); + Map unbans = new HashMap<>(); + try { + JsonObject root = JsonParser.parseString(json).getAsJsonObject(); + for (JsonElement el : arrayOrEmpty(root, "bans")) { + JsonObject ban = el.getAsJsonObject(); + UUID id = uuidOrNull(ban, "player_uuid"); + if (id == null) { + continue; + } + bans.add(new SyncedBan( + id, + stringOrNull(ban, "minecraft_name"), + stringOrNull(ban, "reason"), + stringOrNull(ban, "staff_name"), + longOrZero(ban, "banned_at_ms"), + longOrZero(ban, "expires_at_ms") + )); + } + for (JsonElement el : arrayOrEmpty(root, "unbans")) { + JsonObject unban = el.getAsJsonObject(); + UUID id = uuidOrNull(unban, "player_uuid"); + if (id != null) { + unbans.put(id, longOrZero(unban, "unbanned_at_ms")); + } + } + } catch (RuntimeException e) { + return fail("Bad ban list from API: " + e.getMessage()); + } + return success(bans, unbans); + } + + private static JsonArray arrayOrEmpty(JsonObject obj, String key) { + JsonElement el = obj.get(key); + return el != null && el.isJsonArray() ? el.getAsJsonArray() : new JsonArray(); + } + + private static String stringOrNull(JsonObject obj, String key) { + JsonElement el = obj.get(key); + return el == null || el.isJsonNull() ? null : el.getAsString(); + } + + private static long longOrZero(JsonObject obj, String key) { + JsonElement el = obj.get(key); + return el == null || el.isJsonNull() ? 0L : el.getAsLong(); + } + + private static UUID uuidOrNull(JsonObject obj, String key) { + String raw = stringOrNull(obj, key); + if (raw == null) { + return null; + } + try { + return UUID.fromString(raw.trim()); + } catch (IllegalArgumentException e) { + return null; + } } } @@ -412,7 +526,9 @@ public static MirrorResult postBanEvent( String minecraftName, String reason, String duration, - String staffName + String staffName, + long bannedAtMs, + long expiresAtMs ) { String etype = event == null ? "" : event.trim().toLowerCase(Locale.ROOT); if (!"ban".equals(etype) && !"unban".equals(etype)) { @@ -426,10 +542,53 @@ public static MirrorResult postBanEvent( appendOptionalJson(sb, "reason", reason); appendOptionalJson(sb, "duration", duration); appendOptionalJson(sb, "staff_name", staffName); + if (bannedAtMs > 0L) { + sb.append(",\"banned_at_ms\":").append(bannedAtMs); + sb.append(",\"expires_at_ms\":").append(Math.max(0L, expiresAtMs)); + } sb.append('}'); return postMirror("/skins/moderation/ban-events", sb.toString()); } + /** Bans and unbans recorded from every server, for {@code ban-mirror.sync}. */ + public static BanSync getBanSync() { + String base = Cache.apiBaseUrl; + String key = Cache.pluginKey; + if (base == null || base.isEmpty() || key == null || key.isEmpty()) { + return BanSync.fail(notConfiguredMessage()); + } + + HttpURLConnection connection = null; + try { + @SuppressWarnings("deprecation") + URL url = new URL(base + "/skins/moderation/bans"); + connection = (HttpURLConnection) url.openConnection(); + connection.setRequestMethod("GET"); + connection.setConnectTimeout(TIMEOUT_MS); + connection.setReadTimeout(TIMEOUT_MS); + connection.setRequestProperty("X-Plugin-Key", key); + connection.setRequestProperty("Accept", "application/json"); + + int status = connection.getResponseCode(); + String response = readBody( + status >= 200 && status < 300 + ? connection.getInputStream() + : connection.getErrorStream() + ); + + if (status == 200) { + return BanSync.fromJson(response); + } + return BanSync.fail(detailOrHttp(response, status)); + } catch (Exception e) { + return BanSync.fail("Could not reach API: " + e.getMessage()); + } finally { + if (connection != null) { + connection.disconnect(); + } + } + } + public static MirrorResult postBirdMail( String playerUuid, String discordUserId, @@ -499,6 +658,9 @@ private static MirrorResult postMirror(String path, String jsonBody) { : connection.getErrorStream() ); if (status == 200) { + if ("true".equalsIgnoreCase(jsonString(response, "duplicate"))) { + return MirrorResult.duplicate(); + } boolean mirrored = "true".equalsIgnoreCase(jsonString(response, "mirrored")); return MirrorResult.success(mirrored); } diff --git a/src/main/java/net/tfminecraft/tfmcweb/loaders/ConfigLoader.java b/src/main/java/net/tfminecraft/tfmcweb/loaders/ConfigLoader.java index d52f641..527ed82 100644 --- a/src/main/java/net/tfminecraft/tfmcweb/loaders/ConfigLoader.java +++ b/src/main/java/net/tfminecraft/tfmcweb/loaders/ConfigLoader.java @@ -44,6 +44,7 @@ public void load(File configFile) { config.getInt("ban-mirror.poll-seconds", 30), 30 ); + Cache.banMirrorSync = config.getBoolean("ban-mirror.sync", false); loadLuckPermsBridge(config); } diff --git a/src/main/java/net/tfminecraft/tfmcweb/managers/BanMirrorPoller.java b/src/main/java/net/tfminecraft/tfmcweb/managers/BanMirrorPoller.java index cb09afa..941af37 100644 --- a/src/main/java/net/tfminecraft/tfmcweb/managers/BanMirrorPoller.java +++ b/src/main/java/net/tfminecraft/tfmcweb/managers/BanMirrorPoller.java @@ -7,6 +7,7 @@ import java.util.ArrayList; import java.util.Date; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -20,6 +21,7 @@ import org.bukkit.ban.ProfileBanList; import org.bukkit.configuration.ConfigurationSection; import org.bukkit.configuration.file.YamlConfiguration; +import org.bukkit.entity.Player; import org.bukkit.plugin.java.JavaPlugin; import org.bukkit.scheduler.BukkitTask; @@ -28,20 +30,26 @@ import io.papermc.paper.ban.BanListType; import net.tfminecraft.tfmcweb.Cache; import net.tfminecraft.tfmcweb.api.ProvinceSystemClient; +import net.tfminecraft.tfmcweb.api.ProvinceSystemClient.BanSync; import net.tfminecraft.tfmcweb.api.ProvinceSystemClient.IdentityStatus; import net.tfminecraft.tfmcweb.api.ProvinceSystemClient.MirrorResult; +import net.tfminecraft.tfmcweb.api.ProvinceSystemClient.SyncedBan; import net.tfminecraft.tfmcweb.cache.LinkCache; /** * Mirrors the server's player ban list to the ProvinceSystem moderation outbox. * EssentialsX has no ban event and timed bans expire silently, so the list is * diffed on a timer: new entries post a ban, removed or expired entries post an - * unban (which clears the Discord Banned role). + * unban (which clears the Discord Banned role). With ban-mirror.sync, bans and + * unbans the other servers reported are then applied here too. */ public final class BanMirrorPoller { static final String STATE_FILE = "ban-mirror.yml"; static final String EXPIRED_STAFF = "Ban expired"; + static final String SYNC_SOURCE = "TFMC ban sync"; + /** Bans whose end times are this close are the same ban (matches the API). */ + static final long SAME_BAN_MS = 10L * 60_000L; private static final int REASON_MAX = 500; private static final int STAFF_NAME_MAX = 32; @@ -54,6 +62,7 @@ public final class BanMirrorPoller { private final File stateFile; private final AtomicBoolean busy = new AtomicBoolean(); private final Set warned = ConcurrentHashMap.newKeySet(); + private final AtomicBoolean syncWarned = new AtomicBoolean(); /** Bans already mirrored; null until the first list has been adopted. */ private Map known; private BukkitTask task; @@ -108,21 +117,115 @@ void tick() { return; } List changes = diff(known, current, now); - if (changes.isEmpty()) { + boolean sync = Cache.banMirrorSync; + if (changes.isEmpty() && !sync) { busy.set(false); return; } Bukkit.getScheduler().runTaskAsynchronously(plugin, () -> { + boolean applying = false; try { for (Change change : changes) { post(change); } + // Local changes are posted first, so the list fetched next already has them. + if (sync) { + BanSync remote = fetchSync(); + if (remote != null) { + Bukkit.getScheduler().runTask(plugin, () -> applySync(remote)); + applying = true; + } + } } finally { - saveAndRelease(); + if (!applying) { + saveAndRelease(); + } } }); } + private BanSync fetchSync() { + BanSync remote = ProvinceSystemClient.getBanSync(); + if (!remote.ok) { + if (syncWarned.compareAndSet(false, true)) { + plugin.getLogger().warning( + "[ban-sync] could not read bans from the API: " + remote.error + " (retrying quietly)" + ); + } + return null; + } + syncWarned.set(false); + return remote; + } + + /** Main thread: apply bans and unbans another server reported, then save and release. */ + void applySync(BanSync remote) { + try { + long now = System.currentTimeMillis(); + ProfileBanList list = Bukkit.getBanList(BanListType.PROFILE); + Map current = readBanList(now); + Set banned = new HashSet<>(); + for (SyncedBan ban : remote.bans) { + if (shouldApply(ban, known.get(ban.id), current.get(ban.id), now)) { + applyBan(list, ban); + banned.add(ban.id); + } + } + for (Map.Entry unban : remote.unbans.entrySet()) { + UUID id = unban.getKey(); + BanRecord local = current.get(id); + BanRecord was = known.get(id); + // Only lift a ban this server already mirrored and that is older than the unban. + if (local == null || was == null || was.created != local.created + || local.created <= 0L || local.created > unban.getValue()) { + continue; + } + list.pardon(Bukkit.createProfile(id, local.name)); + known.remove(id); + plugin.getLogger().info("[ban-sync] lifted the ban on " + local.name + " (unbanned on another server)"); + } + if (!banned.isEmpty()) { + Map fresh = readBanList(now); + for (UUID id : banned) { + BanRecord record = fresh.get(id); + if (record != null) { + known.put(id, record); + } + } + } + } catch (RuntimeException e) { + plugin.getLogger().warning("[ban-sync] could not apply bans from the API: " + e); + } + Bukkit.getScheduler().runTaskAsynchronously(plugin, this::saveAndRelease); + } + + static boolean shouldApply(SyncedBan ban, BanRecord was, BanRecord local, long now) { + if (ban.expiresAt > 0L && ban.expiresAt <= now) { + return false; + } + if (local == null) { + // Mirrored here but gone from the list: a local unban the API has not had yet. + return was == null; + } + // A newer ban with a different length replaces this server's copy. + return ban.bannedAt > local.created && Math.abs(ban.expiresAt - local.expires) > SAME_BAN_MS; + } + + private void applyBan(ProfileBanList list, SyncedBan ban) { + Date expires = ban.expiresAt > 0L ? new Date(ban.expiresAt) : null; + String source = ban.staffName != null && !ban.staffName.isBlank() ? ban.staffName : SYNC_SOURCE; + Player online = Bukkit.getPlayer(ban.id); + if (online != null) { + online.ban(ban.reason, expires, source, true); + } else { + list.addBan(Bukkit.createProfile(ban.id, ban.name), ban.reason, expires, source); + } + plugin.getLogger().info( + "[ban-sync] applied the ban on " + ban.name + " from another server (" + + formatDuration(ban.expiresAt, ban.bannedAt) + ")" + ); + } + private void saveAndRelease() { try { saveState(known); @@ -140,13 +243,13 @@ static Map readBanList(long now) { if (id == null) { continue; } - long expires = millis(entry.getExpiration()); + long expires = seconds(millis(entry.getExpiration())); if (expires > 0L && expires <= now) { continue; } bans.put(id, new BanRecord( profile.getName(), - millis(entry.getCreated()), + seconds(millis(entry.getCreated())), expires, entry.getReason(), entry.getSource() @@ -187,7 +290,9 @@ private void post(Change change) { name, change.banned ? cleanReason(ban.reason) : null, change.banned ? formatDuration(ban.expires, banStart(ban)) : null, - cleanDisplay(change.staff, STAFF_NAME_MAX) + cleanDisplay(change.staff, STAFF_NAME_MAX), + ban.created, + ban.expires ); if (!result.ok) { if (warned.add(change.id)) { @@ -206,7 +311,8 @@ private void post(Change change) { } plugin.getLogger().info( "[ban-mirror] " + eventType + " for " + name - + (result.mirrored ? " sent to Discord" : " (no Discord link)") + + (result.duplicate ? " already sent from another server" + : result.mirrored ? " sent to Discord" : " (no Discord link)") ); } @@ -242,8 +348,8 @@ private Map loadState() { } state.put(id, new BanRecord( bans.getString(key + ".name"), - bans.getLong(key + ".created"), - bans.getLong(key + ".expires"), + seconds(bans.getLong(key + ".created")), + seconds(bans.getLong(key + ".expires")), null, null )); @@ -271,6 +377,14 @@ private static long millis(Date date) { return date == null ? 0L : date.getTime(); } + /** + * Whole seconds. banned-players.json keeps ban times to the second, so a ban + * read back after a restart loses the milliseconds it had when it was made. + */ + static long seconds(long millis) { + return millis / 1000L * 1000L; + } + /** Full ban length, not time left: a 7 day ban seen a minute late is still "7 days". */ private static long banStart(BanRecord ban) { return ban.created > 0L ? ban.created : System.currentTimeMillis(); diff --git a/src/main/resources/config.yml b/src/main/resources/config.yml index 74d1673..934183e 100644 --- a/src/main/resources/config.yml +++ b/src/main/resources/config.yml @@ -158,10 +158,15 @@ patreon: # Mirror this server's ban list to Discord (DM + Banned role on ban, role # cleared on unban or when a timed ban expires). Linked players only. The list -# is checked every poll-seconds; changes apply on restart. +# is checked every poll-seconds; changes apply on restart. The same ban +# reported by several servers sends one Discord message. +# sync: also apply bans and unbans made on the other servers that use this +# API, so one /ban covers them all. Set true on each server that should share +# bans (TFMCMain and TFMCLobby). ban-mirror: enabled: true poll-seconds: 30 + sync: false # Staff-panel LuckPerms bridge. LuckPerms storage is shared across servers, so # set apply true on exactly one server. publish only sends data to this server's site. diff --git a/src/test/java/net/tfminecraft/tfmcweb/api/ProvinceSystemClientTest.java b/src/test/java/net/tfminecraft/tfmcweb/api/ProvinceSystemClientTest.java index c3d247c..f5a0334 100644 --- a/src/test/java/net/tfminecraft/tfmcweb/api/ProvinceSystemClientTest.java +++ b/src/test/java/net/tfminecraft/tfmcweb/api/ProvinceSystemClientTest.java @@ -70,7 +70,7 @@ class ProvinceSystemClientTest { assertFalse(ProvinceSystemClient.resetCosmeticMintCooldowns(null, null).ok); assertFalse(ProvinceSystemClient.postWarning(null, "reason", null, null, null, null).ok); assertFalse(ProvinceSystemClient.postWarning("id", null, null, null, null, null).ok); - assertFalse(ProvinceSystemClient.postBanEvent(null, null, null, null, null, null, null).ok); + assertFalse(ProvinceSystemClient.postBanEvent(null, null, null, null, null, null, null, 0L, 0L).ok); assertTrue(ProvinceSystemClient.postBirdMail(null, null, null, null, null).ok); assertFalse(ProvinceSystemClient.postBirdMail("id", "discord", null, null, null).ok); http.reply(204, ""); assertTrue(ProvinceSystemClient.unlinkDiscord(" id ").ok); @@ -80,8 +80,15 @@ class ProvinceSystemClientTest { http.reply(200, "{\"mirrored\":true}"); assertTrue(ProvinceSystemClient.postWarning("id", " reason ", "staff", "Ada", "disc", "mc").mirrored); assertEquals("reason", JsonParser.parseString(http.request().body()).getAsJsonObject().get("reason").getAsString()); - http.reply(200, "{}"); assertFalse(ProvinceSystemClient.postBanEvent(" BAN ", null, " ", "mc", "why", "1d", "staff").mirrored); - assertEquals("/skins/moderation/ban-events", http.request().path()); + http.reply(200, "{}"); assertFalse(ProvinceSystemClient.postBanEvent(" BAN ", null, " ", "mc", "why", "1d", "staff", 0L, 0L).mirrored); + var banRequest = http.request(); + assertEquals("/skins/moderation/ban-events", banRequest.path()); + assertFalse(banRequest.body().contains("banned_at_ms")); + http.reply(200, "{}"); ProvinceSystemClient.postBanEvent("ban", "id", null, null, null, null, null, 1000L, -5L); + var timed = JsonParser.parseString(http.request().body()).getAsJsonObject(); + http.reply(200, "{\"mirrored\":false,\"duplicate\":true}"); + assertTrue(ProvinceSystemClient.postBanEvent("unban", "id", null, null, null, null, null, 0L, 0L).duplicate); http.request(); + assertEquals(1000L, timed.get("banned_at_ms").getAsLong()); assertEquals(0L, timed.get("expires_at_ms").getAsLong()); http.reply(200, "{\"mirrored\":true}"); assertTrue(ProvinceSystemClient.postBirdMail(" id ", "disc", " Char ", "sender", "letter").ok); assertEquals("/skins/moderation/bird-mail", http.request().path()); @@ -106,6 +113,21 @@ class ProvinceSystemClientTest { http.reply(200, "{}"); assertTrue(ProvinceSystemClient.ping().ok); http.request(); http.reply(503, ""); assertEquals("HTTP 503", ProvinceSystemClient.ping().error); http.request(); } + @Test void banSyncParsesBansAndUnbans() throws Exception { + UUID ada = UUID.randomUUID(), bob = UUID.randomUUID(); + http.reply(200, "{\"bans\":[{\"player_uuid\":\"bad\"},{},{\"player_uuid\":\"" + ada + "\",\"minecraft_name\":\"Ada\"," + + "\"reason\":null,\"staff_name\":\"Mod\",\"banned_at_ms\":5,\"expires_at_ms\":0}]," + + "\"unbans\":[{\"player_uuid\":\"" + bob + "\",\"unbanned_at_ms\":7}]}"); + var sync = ProvinceSystemClient.getBanSync(); + assertEquals("/skins/moderation/bans", http.request().path()); + assertTrue(sync.ok); assertEquals(1, sync.bans.size()); + var ban = sync.bans.getFirst(); + assertEquals(ada, ban.id); assertEquals("Ada", ban.name); assertNull(ban.reason); assertEquals("Mod", ban.staffName); + assertEquals(5L, ban.bannedAt); assertEquals(0L, ban.expiresAt); + assertEquals(Map.of(bob, 7L), sync.unbans); + assertTrue(ProvinceSystemClient.BanSync.fromJson("{}").bans.isEmpty()); + assertTrue(ProvinceSystemClient.BanSync.fromJson("[]").error.startsWith("Bad ban list")); + } @Test void allTransportFamiliesReportHttpFailuresConfigurationAndExceptions() throws Exception { List> operations = List.of( () -> ProvinceSystemClient.startDiscordLink("id", "mc").error, @@ -113,6 +135,7 @@ class ProvinceSystemClientTest { () -> ProvinceSystemClient.getCosmeticMintStatus("id").error, () -> ProvinceSystemClient.getIdentityStatus("id").error, () -> ProvinceSystemClient.listPluginNotices().error, + () -> ProvinceSystemClient.getBanSync().error, () -> ProvinceSystemClient.unlinkDiscord("id").error, () -> ProvinceSystemClient.postWarning("id", "reason", null, null, null, null).error); String base = Cache.apiBaseUrl; diff --git a/src/test/java/net/tfminecraft/tfmcweb/managers/BanMirrorPollerTest.java b/src/test/java/net/tfminecraft/tfmcweb/managers/BanMirrorPollerTest.java index 9c1caec..d77d53b 100644 --- a/src/test/java/net/tfminecraft/tfmcweb/managers/BanMirrorPollerTest.java +++ b/src/test/java/net/tfminecraft/tfmcweb/managers/BanMirrorPollerTest.java @@ -16,6 +16,7 @@ import org.bukkit.BanEntry; import org.bukkit.Bukkit; import org.bukkit.ban.ProfileBanList; +import org.bukkit.entity.Player; import org.bukkit.plugin.java.JavaPlugin; import org.bukkit.scheduler.BukkitScheduler; import org.bukkit.scheduler.BukkitTask; @@ -49,11 +50,11 @@ class BanMirrorPollerTest { postReturns(MirrorResult.success(true)); cache = new LinkCache(); } - @AfterEach void cleanup() throws Exception { api.close(); bukkit.close(); state.close(); } + @AfterEach void cleanup() throws Exception { Cache.banMirrorSync = false; api.close(); bukkit.close(); state.close(); } void postReturns(MirrorResult result) { api.when(() -> ProvinceSystemClient.postBanEvent(anyString(), anyString(), nullable(String.class), nullable(String.class), - nullable(String.class), nullable(String.class), nullable(String.class))).thenReturn(result); + nullable(String.class), nullable(String.class), nullable(String.class), anyLong(), anyLong())).thenReturn(result); } @SuppressWarnings("unchecked") BanEntry ban(UUID id, String name, long created, Long expires, String reason, String source) { @@ -69,11 +70,11 @@ BanEntry ban(UUID id, String name, long created, Long expires, St BanMirrorPoller started() { BanMirrorPoller poller = new BanMirrorPoller(plugin, cache); poller.start(); return poller; } void verifyPost(String type, UUID id, String discord, String name, String reason, String duration, String staff) { String uuid = id.toString(); - api.verify(() -> ProvinceSystemClient.postBanEvent(type, uuid, discord, name, reason, duration, staff)); + api.verify(() -> ProvinceSystemClient.postBanEvent(eq(type), eq(uuid), eq(discord), eq(name), eq(reason), eq(duration), eq(staff), anyLong(), anyLong())); } void verifyNoPosts() { api.verify(() -> ProvinceSystemClient.postBanEvent(anyString(), anyString(), nullable(String.class), nullable(String.class), - nullable(String.class), nullable(String.class), nullable(String.class)), never()); + nullable(String.class), nullable(String.class), nullable(String.class), anyLong(), anyLong()), never()); } @Test void startHonoursConfigAndStopsOnce() { @@ -109,6 +110,11 @@ void verifyNoPosts() { tick.run(); verifyPost("unban", ADA, null, "Ada", null, null, null); verify(logger).info(contains("unban for Ada (no Discord link)")); + ban(ADA, "Ada", now, null, "Xray", "Mod"); postReturns(MirrorResult.duplicate()); + tick.run(); + verify(logger).info(contains("ban for Ada already sent from another server")); + entries.removeIf(e -> e.getBanTarget().getId().equals(ADA)); postReturns(MirrorResult.success(false)); + tick.run(); // Bob's timed ban lapses; the entry lingers in the list but counts as expired. postReturns(MirrorResult.success(true)); @@ -117,11 +123,11 @@ void verifyNoPosts() { verifyPost("unban", BOB, "42", "Bob", null, null, "Ban expired"); tick.run(); api.verify(() -> ProvinceSystemClient.postBanEvent(anyString(), anyString(), nullable(String.class), nullable(String.class), - nullable(String.class), nullable(String.class), nullable(String.class)), times(3)); + nullable(String.class), nullable(String.class), nullable(String.class), anyLong(), anyLong()), times(5)); } @Test void rebansFailuresAndIdentityLookups() { - long now = System.currentTimeMillis(); + long now = System.currentTimeMillis() / 1000 * 1000; started(); tick.run(); var first = ban(ADA, "Ada", now, now + 2 * HOUR, "first", "Mod"); ban(BOB, "Bob", now, now + 26 * HOUR, "long", "Mod"); @@ -132,13 +138,13 @@ void verifyNoPosts() { verifyPost("ban", BOB, null, "Bob", "long", "1 day", "Mod"); assertEquals("fetched", cache.get(ADA).discordUserId); - entries.remove(first); ban(ADA, "Ada", now + 1, now + 3 * HOUR, "second", "Mod"); + entries.remove(first); ban(ADA, "Ada", now + 1000, now + 3 * HOUR, "second", "Mod"); postReturns(MirrorResult.fail("down")); tick.run(); tick.run(); verify(logger, times(1)).warning(contains("API failed for ban Ada: down")); postReturns(MirrorResult.success(true)); tick.run(); - api.verify(() -> ProvinceSystemClient.postBanEvent("ban", ADA.toString(), "fetched", "Ada", "second", "2h", "Mod"), times(3)); + api.verify(() -> ProvinceSystemClient.postBanEvent(eq("ban"), eq(ADA.toString()), eq("fetched"), eq("Ada"), eq("second"), eq("2h"), eq("Mod"), anyLong(), anyLong()), times(3)); } @Test void skipsOverlappingTicksAndSurvivesBanListFailures() { @@ -172,7 +178,7 @@ void verifyNoPosts() { Files.writeString(temp.resolve(BanMirrorPoller.STATE_FILE), "bans:\n not-a-uuid:\n name: x\n " + ADA + ":\n name: Ada\n created: 1\n expires: 0\n"); started(); tick.run(); - api.verify(() -> ProvinceSystemClient.postBanEvent(eq("ban"), anyString(), any(), any(), any(), any(), any()), times(1)); + api.verify(() -> ProvinceSystemClient.postBanEvent(eq("ban"), anyString(), any(), any(), any(), any(), any(), anyLong(), anyLong()), times(1)); Files.delete(temp.resolve(BanMirrorPoller.STATE_FILE)); Files.createDirectory(temp.resolve(BanMirrorPoller.STATE_FILE)); @@ -180,6 +186,112 @@ void verifyNoPosts() { verify(logger).warning(contains("could not save")); } + @Test void banTimesAreKeptToTheSecondSoARestartDoesNotResend() throws Exception { + long now = System.currentTimeMillis() / 1000 * 1000 + 977; + started(); tick.run(); + var live = ban(ADA, "Ada", now, now + 24 * HOUR, "traps", "Mod"); + tick.run(); + api.verify(() -> ProvinceSystemClient.postBanEvent("ban", ADA.toString(), null, "Ada", "traps", "1 day", "Mod", now - 977, now + 24 * HOUR - 977)); + + // Restart: banned-players.json gives the times back without milliseconds. + entries.remove(live); ban(ADA, "Ada", now - 977, now + 24 * HOUR - 977, "traps", "Mod"); + started(); tick.run(); + api.verify(() -> ProvinceSystemClient.postBanEvent(anyString(), anyString(), nullable(String.class), nullable(String.class), + nullable(String.class), nullable(String.class), nullable(String.class), anyLong(), anyLong()), times(1)); + + // A state file written before this fix still holds milliseconds. + Files.writeString(temp.resolve(BanMirrorPoller.STATE_FILE), "bans:\n " + ADA + ":\n name: Ada\n created: " + now + + "\n expires: " + (now + 24 * HOUR) + "\n"); + started(); tick.run(); + api.verify(() -> ProvinceSystemClient.postBanEvent(anyString(), anyString(), nullable(String.class), nullable(String.class), + nullable(String.class), nullable(String.class), nullable(String.class), anyLong(), anyLong()), times(1)); + } + + void syncReturns(BanSync sync) { api.when(ProvinceSystemClient::getBanSync).thenReturn(sync); } + PlayerProfile profileFor(UUID id) { + PlayerProfile profile = mock(PlayerProfile.class); + bukkit.when(() -> Bukkit.createProfile(eq(id), nullable(String.class))).thenReturn(profile); + return profile; + } + + @Test void syncAppliesBansFromOtherServersWithoutResending() { + Cache.banMirrorSync = true; + long now = System.currentTimeMillis() / 1000 * 1000; + doAnswer(c -> { ((Runnable) c.getArgument(1)).run(); return null; }).when(scheduler).runTask(eq(plugin), any(Runnable.class)); + syncReturns(BanSync.success(List.of(), Map.of())); + started(); tick.run(); + + PlayerProfile adaProfile = profileFor(ADA); + Player bob = mock(Player.class); + bukkit.when(() -> Bukkit.getPlayer(BOB)).thenReturn(bob); + when(banList.addBan(eq(adaProfile), eq("traps"), any(Date.class), eq("Mod"))).thenAnswer(c -> ban(ADA, "Ada", now, now + 24 * HOUR, "traps", "Mod")); + when(bob.ban(isNull(), isNull(Date.class), eq(BanMirrorPoller.SYNC_SOURCE), eq(true))).thenAnswer(c -> ban(BOB, "Bob", now, null, null, BanMirrorPoller.SYNC_SOURCE)); + syncReturns(BanSync.success(List.of( + new SyncedBan(ADA, "Ada", "traps", "Mod", now - 9_000, now + 24 * HOUR), + new SyncedBan(BOB, "Bob", null, null, now, 0L), + new SyncedBan(UUID.randomUUID(), "Gone", null, null, now - 2 * HOUR, now - HOUR)), Map.of())); + tick.run(); + verify(banList).addBan(eq(adaProfile), eq("traps"), eq(new Date(now + 24 * HOUR)), eq("Mod")); + verify(bob).ban(null, (Date) null, BanMirrorPoller.SYNC_SOURCE, true); + verify(logger).info(contains("applied the ban on Ada from another server (1 day)")); + verify(banList, times(1)).addBan(any(PlayerProfile.class), nullable(String.class), nullable(Date.class), nullable(String.class)); + + // Next tick: the applied bans are already known, so nothing is posted or applied again. + tick.run(); + verifyNoPosts(); + verify(banList, times(1)).addBan(any(PlayerProfile.class), nullable(String.class), nullable(Date.class), nullable(String.class)); + + // Unbanned elsewhere: lifted here, still without a post from this server. + syncReturns(BanSync.success(List.of(), Map.of(ADA, now + 1_000, BOB, now - 60_000))); + tick.run(); + verify(banList).pardon(adaProfile); + verify(banList, times(1)).pardon(any(PlayerProfile.class)); + verify(logger).info(contains("lifted the ban on Ada")); + verifyNoPosts(); + } + + @Test void syncLeavesLocalChangesAloneAndWarnsOnceWhenTheApiFails() { + Cache.banMirrorSync = true; + long now = System.currentTimeMillis() / 1000 * 1000; + doAnswer(c -> { ((Runnable) c.getArgument(1)).run(); return null; }).when(scheduler).runTask(eq(plugin), any(Runnable.class)); + syncReturns(BanSync.fail("down")); + var ada = ban(ADA, "Ada", now, null, "r", "Mod"); + started(); tick.run(); tick.run(); + verify(logger, times(1)).warning(contains("could not read bans from the API: down")); + + // Unbanned here but the unban post fails: the API still lists the ban, which must not come back. + entries.remove(ada); postReturns(MirrorResult.fail("down")); + syncReturns(BanSync.success(List.of(new SyncedBan(ADA, "Ada", "r", "Mod", now, 0L)), Map.of())); + tick.run(); + verify(banList, never()).addBan(any(PlayerProfile.class), nullable(String.class), nullable(Date.class), nullable(String.class)); + + // A different, newer ban from another server replaces this server's copy. + syncReturns(BanSync.success(List.of(), Map.of())); + var local = ban(BOB, "Bob", now - HOUR, now + HOUR, "r", "Mod"); + postReturns(MirrorResult.success(true)); + tick.run(); + PlayerProfile bobProfile = profileFor(BOB); + syncReturns(BanSync.success(List.of(new SyncedBan(BOB, "Bob", "worse", "Admin", now, 0L)), Map.of())); + when(banList.addBan(eq(bobProfile), eq("worse"), isNull(Date.class), eq("Admin"))).thenAnswer(c -> { entries.remove(local); return ban(BOB, "Bob", now, null, "worse", "Admin"); }); + tick.run(); + verify(banList).addBan(bobProfile, "worse", (Date) null, "Admin"); + + bukkit.when(() -> Bukkit.getBanList(BanListType.PROFILE)).thenReturn(banList).thenReturn(banList).thenThrow(new IllegalStateException("boom")); + tick.run(); + verify(logger).warning(contains("could not apply bans from the API")); + } + + @Test void shouldApplyOnlyMissingOrChangedBans() { + long now = 10 * HOUR; + var rec = new BanMirrorPoller.BanRecord("Ada", now - HOUR, now + HOUR, null, null); + assertTrue(BanMirrorPoller.shouldApply(new SyncedBan(ADA, "Ada", null, null, now, 0L), null, null, now)); + assertFalse(BanMirrorPoller.shouldApply(new SyncedBan(ADA, "Ada", null, null, now, 0L), rec, null, now)); + assertFalse(BanMirrorPoller.shouldApply(new SyncedBan(ADA, "Ada", null, null, now - 2 * HOUR, now - 1), null, null, now)); + assertFalse(BanMirrorPoller.shouldApply(new SyncedBan(ADA, "Ada", null, null, now, now + HOUR + 9_000), rec, rec, now)); + assertFalse(BanMirrorPoller.shouldApply(new SyncedBan(ADA, "Ada", null, null, now - 2 * HOUR, 0L), rec, rec, now)); + assertTrue(BanMirrorPoller.shouldApply(new SyncedBan(ADA, "Ada", null, null, now, 0L), rec, rec, now)); + } + @Test void formatsDurations() { assertEquals("Permanent", BanMirrorPoller.formatDuration(0L, 5L)); assertEquals("2 days", BanMirrorPoller.formatDuration(49 * HOUR, 0L)); From 8efc5cbf8743bdde69ae921e55a59c4a56b21102 Mon Sep 17 00:00:00 2001 From: Drefvelin <54400154+Drefvelin@users.noreply.github.com> Date: Mon, 19 Oct 2026 08:06:30 +0000 Subject: [PATCH 2/2] fix: keep a ban the API still lists when it also lists an unban Co-Authored-By: Claude Opus 5.5 --- .../net/tfminecraft/tfmcweb/managers/BanMirrorPoller.java | 5 +++++ .../tfminecraft/tfmcweb/managers/BanMirrorPollerTest.java | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/src/main/java/net/tfminecraft/tfmcweb/managers/BanMirrorPoller.java b/src/main/java/net/tfminecraft/tfmcweb/managers/BanMirrorPoller.java index 941af37..0171101 100644 --- a/src/main/java/net/tfminecraft/tfmcweb/managers/BanMirrorPoller.java +++ b/src/main/java/net/tfminecraft/tfmcweb/managers/BanMirrorPoller.java @@ -165,7 +165,9 @@ void applySync(BanSync remote) { ProfileBanList list = Bukkit.getBanList(BanListType.PROFILE); Map current = readBanList(now); Set banned = new HashSet<>(); + Set remoteBanned = new HashSet<>(); for (SyncedBan ban : remote.bans) { + remoteBanned.add(ban.id); if (shouldApply(ban, known.get(ban.id), current.get(ban.id), now)) { applyBan(list, ban); banned.add(ban.id); @@ -173,6 +175,9 @@ void applySync(BanSync remote) { } for (Map.Entry unban : remote.unbans.entrySet()) { UUID id = unban.getKey(); + if (remoteBanned.contains(id)) { + continue; + } BanRecord local = current.get(id); BanRecord was = known.get(id); // Only lift a ban this server already mirrored and that is older than the unban. diff --git a/src/test/java/net/tfminecraft/tfmcweb/managers/BanMirrorPollerTest.java b/src/test/java/net/tfminecraft/tfmcweb/managers/BanMirrorPollerTest.java index d77d53b..de95916 100644 --- a/src/test/java/net/tfminecraft/tfmcweb/managers/BanMirrorPollerTest.java +++ b/src/test/java/net/tfminecraft/tfmcweb/managers/BanMirrorPollerTest.java @@ -241,6 +241,11 @@ PlayerProfile profileFor(UUID id) { verifyNoPosts(); verify(banList, times(1)).addBan(any(PlayerProfile.class), nullable(String.class), nullable(Date.class), nullable(String.class)); + // A player listed as both banned and unbanned keeps the ban. + syncReturns(BanSync.success(List.of(new SyncedBan(ADA, "Ada", "traps", "Mod", now, now + 24 * HOUR)), Map.of(ADA, now + 1_000))); + tick.run(); + verify(banList, never()).pardon(any(PlayerProfile.class)); + // Unbanned elsewhere: lifted here, still without a post from this server. syncReturns(BanSync.success(List.of(), Map.of(ADA, now + 1_000, BOB, now - 60_000))); tick.run();