diff --git a/.gitignore b/.gitignore index 44d3e68..dd752af 100644 --- a/.gitignore +++ b/.gitignore @@ -112,6 +112,7 @@ logo.jpg # ============================================================ docs/* !docs/server-setup-guide.html +!docs/LUCKPERMS_COMPATIBILITY_AUDIT.md # ============================================================ # BlueJ diff --git a/CHANGELOG.md b/CHANGELOG.md index 320c142..a55b7c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,26 @@ Versioning: [Semantic Versioning](https://semver.org/) --- +## [Unreleased] + +LuckPerms compatibility audit. + +### Fixed + +- The LuckPerms `UserDataRecalculateEvent` subscription is now closed on server stop and re-created on the next server start, instead of leaking across server lifecycles in the same JVM (stale `MinecraftServer` reference, broken live resync after an embedded restart). +- Command-tree resyncs triggered by LuckPerms recalculation events are now coalesced per player, so a burst of recalculations (login, group inheritance, sync) sends a single command-tree packet instead of one per event. +- Pending resync state is now isolated per server lifecycle, preventing an event racing with shutdown from suppressing resyncs after an embedded restart. + +### Changed + +- The per-event LuckPerms recalculation log line is now DEBUG instead of INFO. + +### Added + +- `docs/LUCKPERMS_COMPATIBILITY_AUDIT.md` — full audit of every LuckPerms touchpoint, including documented-but-unchanged findings (permanent degradation policy, vanilla-command policy with LP installed, version-gate behaviour). + +--- + ## [1.0.3] - 2026-06-10 Backend policy update. diff --git a/docs/LUCKPERMS_COMPATIBILITY_AUDIT.md b/docs/LUCKPERMS_COMPATIBILITY_AUDIT.md new file mode 100644 index 0000000..c4bfa3c --- /dev/null +++ b/docs/LUCKPERMS_COMPATIBILITY_AUDIT.md @@ -0,0 +1,123 @@ +# LuckPerms Compatibility Audit — CustomPerm 1.0.3 + +Date: 2026-06-10 +Scope: full review of every LuckPerms touchpoint in the mod — backend selection, +runtime permission checks, event integration, version gating, metadata, tests and docs. + +Severity scale: **HIGH** = broken behaviour in a realistic scenario · **MEDIUM** = degraded +behaviour or misleading outcome · **LOW** = inconsistency / documentation / hardening note. + +--- + +## Fixed in this PR + +### F1 — HIGH · LP event subscription leaks across server lifecycles + +`LuckPermsService.initServerHooks()` subscribed to `UserDataRecalculateEvent` without +keeping the returned `EventSubscription`, guarded by a one-way `hooksReady` flag. + +Consequences when a server stops and another starts in the same JVM (integrated server +opening a second world, or any embedded restart): + +1. The old subscription is never closed — its lambda retains the dead `MinecraftServer` + (memory leak: the full server object graph stays reachable from LP's event bus). +2. `hooksReady` stays `true`, so the new server never re-subscribes — live command-tree + resync silently stops working until the JVM restarts. + +**Fix:** the subscription is stored, closed on `ServerStoppedEvent` +(`LuckPermsService.closeServerHooks()`, wired in `CustomPerm.onServerStopped`), and +re-created on the next `ServerStartedEvent`. Pending resync state is isolated in a +per-server lifecycle coordinator; closing a lifecycle invalidates already queued work, +so an event racing with shutdown cannot target the stopped server or suppress the same +player's resync after restart. + +### F2 — MEDIUM · Resync storm on `UserDataRecalculateEvent` + +LuckPerms fires `UserDataRecalculateEvent` aggressively — typically several times in a +burst for the same user (login, group inheritance recalculation, messaging-service sync, +web-editor apply). Each event caused one full `ClientboundCommandsPacket` re-send plus +one INFO log line. + +**Fix:** resyncs are coalesced per player (`pendingResyncs` set — one packet per burst), +and the per-event log is downgraded from INFO to DEBUG. The one-time subscribe/unsubscribe +logs remain at INFO. + +--- + +## Findings documented, intentionally not changed + +### A1 — MEDIUM · Permanent degradation on the first LP exception + +`LuckPermsService.hasPermission()` flips a one-way `degraded` flag on **any** exception, +including `IllegalStateException` from `LuckPermsProvider.get()`. This is the documented +policy (AC1–AC3, INVARIANT-301), but note the consequence: a single transient hiccup +(e.g. a permission check racing LP's startup, or one storage timeout surfacing through +the cached-data layer) permanently disables the LuckPerms backend until server restart, +with `deny` as the default fallback. In practice predicates are only evaluated after +`ServerStartedEvent` (player login), when LP is registered, so the startup race is +unlikely — but a recovery path (e.g. re-probe on the next `/customperm reload`) would be +a worthwhile follow-up. + +### A2 — MEDIUM · With LuckPerms installed, vanilla commands cannot be granted at all + +The 1.0.3 policy disables direct command exposure whenever LP is present, deferring +"normal command permissions" to LuckPerms. On NeoForge, however, **LuckPerms does not +gate vanilla Brigadier commands** — it serves the PermissionAPI for mods that opt in; +vanilla `/gamemode`, `/give`, etc. remain op-level-gated. So with LP installed, the only +way to give a non-op access to a vanilla command is a CustomPerm alias (which elevates to +op-4). The README's "Configure the server's normal /gamemode permission directly in +LuckPerms" workflow does not work for vanilla commands on NeoForge. Operators should be +aware that installing LP *removes* the `customperm.command.*` feature without LP providing +an equivalent. + +### A3 — LOW · Version gate rejects qualified LP versions + +`VersionUtils.isVersionAtLeast` returns `false` for any version with more than three +`[.\-]`-separated parts, so `5.4.150-SNAPSHOT`, `5.5.0-beta`, or build-metadata-qualified +strings are rejected even when numerically above the minimum. This is documented as a +deliberate safety stance (README: "prerelease-style versions are rejected"); release LP +builds use plain `x.y.z` so impact is limited to dev/snapshot builds of LP. + +### A4 — LOW · `neoforge.mods.toml` range looser than the code gate + +The optional dependency declares `versionRange="[5.4,)"` while the code enforces +`5.4.150+`. LP `5.4.0–5.4.149` passes the loader check and is then gracefully rejected by +the code gate (deny/internal per `settings.json`) — fine. LP `< 5.4` instead fails the +loader range and produces a hard NeoForge dependency error rather than the graceful +fallback. Tightening the range to `[5.4.150,)` would convert *all* old-LP cases into hard +loader errors, which is worse; the current split is acceptable but worth knowing. + +### A5 — LOW · OP short-circuit asymmetry between backends + +`InternalPermService` returns `true` for any op-level-2 source before consulting grades; +`LuckPermsService` queries LP verbatim (no op short-circuit — call sites add +`src.hasPermission(2) ||` themselves). Net effect on `/customperm test`: with the LP +backend, an op player can show `DENIED` for a node LP doesn't grant, while the internal +backend would show `GRANTED`. This matches LP semantics (LP itself doesn't special-case +server ops) but can confuse diagnostics. + +### A6 — LOW · `getUser()` returning null is treated as deny, never loaded + +For an online player LP's user object is loaded during login, so `getUser(uuid)` should +not be null in practice; if it ever is (plugin-managed unload, very early check), the +check silently denies. No lookup/load fallback is attempted — acceptable for a hot path, +noted for completeness. + +### A7 — LOW · Group-level changes rely on LP's cascade + +Only `UserDataRecalculateEvent` is subscribed. Group edits (`/lp group vip permission set +…`) only trigger a client resync because LP invalidates and recalculates affected users' +cached data, which fires the user event per affected online user. This holds with current +LP versions; if LP ever changes that cascade, `NodeMutateEvent`/`GroupDataRecalculateEvent` +would need explicit handling. + +--- + +## Verification + +- `./gradlew test` — pure-Java unit suite (resolver, configs, version gate). +- `ResyncCoordinatorTest` covers burst coalescing, queued-work invalidation on close, + and reuse of the same player UUID in a new server lifecycle. +- `./gradlew runGameTestServer` — GameTests pass without LP at runtime (LP is + `compileOnly`); LP-specific behaviour (F1/F2) requires a dev server with the LP jar in + `run/mods/` as per README's manual validation section. diff --git a/src/main/java/com/arcadia/customperm/CustomPerm.java b/src/main/java/com/arcadia/customperm/CustomPerm.java index 51d17fb..54a8004 100644 --- a/src/main/java/com/arcadia/customperm/CustomPerm.java +++ b/src/main/java/com/arcadia/customperm/CustomPerm.java @@ -15,6 +15,7 @@ import net.neoforged.fml.common.Mod; import net.neoforged.neoforge.common.NeoForge; import net.neoforged.neoforge.event.server.ServerStartedEvent; +import net.neoforged.neoforge.event.server.ServerStoppedEvent; import org.slf4j.Logger; @Mod(CustomPerm.MODID) @@ -77,6 +78,7 @@ public CustomPerm(IEventBus modBus, ModContainer container) { NeoForge.EVENT_BUS.register(CommandTreeRewriter.class); NeoForge.EVENT_BUS.addListener(CustomPerm::onServerStarted); + NeoForge.EVENT_BUS.addListener(CustomPerm::onServerStopped); } private static PermissionService unavailableLuckPermsBackend(InternalPermService internalBackend, String reason) { @@ -104,6 +106,12 @@ private static void onServerStarted(ServerStartedEvent event) { backend, wrapped, exposed, aliases, grades); } + private static void onServerStopped(ServerStoppedEvent event) { + if (permissions instanceof LuckPermsService lps) { + lps.closeServerHooks(); + } + } + public static String backendLabel() { if (permissions instanceof LuckPermsService lps) { if (lps.isDegraded()) { diff --git a/src/main/java/com/arcadia/customperm/perm/LuckPermsService.java b/src/main/java/com/arcadia/customperm/perm/LuckPermsService.java index f5d34b3..cf5eb66 100644 --- a/src/main/java/com/arcadia/customperm/perm/LuckPermsService.java +++ b/src/main/java/com/arcadia/customperm/perm/LuckPermsService.java @@ -3,6 +3,7 @@ import com.arcadia.customperm.CustomPerm; import net.luckperms.api.LuckPerms; import net.luckperms.api.LuckPermsProvider; +import net.luckperms.api.event.EventSubscription; import net.luckperms.api.event.user.UserDataRecalculateEvent; import net.luckperms.api.model.user.User; import net.minecraft.commands.CommandSourceStack; @@ -25,7 +26,14 @@ public class LuckPermsService implements PermissionService { */ private final AtomicBoolean degraded = new AtomicBoolean(false); - private volatile boolean hooksReady = false; + /** + * Abonnement LP actif — conservé pour pouvoir le fermer proprement à l'arrêt du serveur. + * Sans close(), l'abonnement survit au cycle de vie du serveur : la lambda garde une + * référence vers l'ancien MinecraftServer (fuite mémoire) et, au démarrage suivant dans + * la même JVM, le resync pointe vers un serveur mort. + */ + private final Object hooksLock = new Object(); + private volatile ServerHooks serverHooks; public LuckPermsService(InternalPermService fallback) { // P7 : fail-fast si fallback null — NPE tardif lors d'une vraie défaillance LP serait bien pire. @@ -98,24 +106,60 @@ private boolean safeCallFallback(CommandSourceStack source, String node) { * until the player reconnects or the server reloads. */ public void initServerHooks(MinecraftServer server) { - if (hooksReady) return; - try { - LuckPerms api = LuckPermsProvider.get(); - api.getEventBus().subscribe(UserDataRecalculateEvent.class, event -> { - UUID uuid = event.getUser().getUniqueId(); - // LP events fire off-thread; schedule the resync on the server thread. - server.execute(() -> { - ServerPlayer player = server.getPlayerList().getPlayer(uuid); - if (player != null) { - CustomPerm.LOGGER.info("[CustomPerm] LP user data recalculated for {}, resending command tree.", player.getGameProfile().getName()); - server.getCommands().sendCommands(player); - } - }); - }); - hooksReady = true; - CustomPerm.LOGGER.info("[CustomPerm] Subscribed to LuckPerms UserDataRecalculateEvent for live command tree resync."); - } catch (Throwable t) { - CustomPerm.LOGGER.warn("[CustomPerm] Could not subscribe LP events; permission checks may still work but live command tree resync is disabled.", t); + synchronized (hooksLock) { + if (serverHooks != null) return; + try { + LuckPerms api = LuckPermsProvider.get(); + ResyncCoordinator coordinator = new ResyncCoordinator(); + EventSubscription subscription = + api.getEventBus().subscribe(UserDataRecalculateEvent.class, event -> { + UUID uuid = event.getUser().getUniqueId(); + if (!coordinator.schedule(uuid)) return; + // LP events fire off-thread; schedule the resync on the server thread. + server.execute(() -> { + if (!coordinator.complete(uuid)) return; + ServerPlayer player = server.getPlayerList().getPlayer(uuid); + if (player != null) { + CustomPerm.LOGGER.debug("[CustomPerm] LP user data recalculated for {}, resending command tree.", player.getGameProfile().getName()); + server.getCommands().sendCommands(player); + } + }); + }); + serverHooks = new ServerHooks(subscription, coordinator); + CustomPerm.LOGGER.info("[CustomPerm] Subscribed to LuckPerms UserDataRecalculateEvent for live command tree resync."); + } catch (Throwable t) { + CustomPerm.LOGGER.warn("[CustomPerm] Could not subscribe LP events; permission checks may still work but live command tree resync is disabled.", t); + } } } + + /** + * Ferme l'abonnement LP au moment où le serveur s'arrête, pour que : + * 1) la lambda ne retienne pas l'ancien MinecraftServer (fuite mémoire), + * 2) un redémarrage de serveur dans la même JVM ré-abonne avec le bon serveur. + */ + public void closeServerHooks() { + ServerHooks hooks; + synchronized (hooksLock) { + hooks = serverHooks; + serverHooks = null; + if (hooks != null) { + hooks.coordinator().close(); + } + } + + if (hooks != null) { + try { + hooks.subscription().close(); + CustomPerm.LOGGER.info("[CustomPerm] Unsubscribed from LuckPerms UserDataRecalculateEvent (server stopped)."); + } catch (Throwable t) { + CustomPerm.LOGGER.warn("[CustomPerm] Failed to close LuckPerms event subscription.", t); + } + } + } + + private record ServerHooks( + EventSubscription subscription, + ResyncCoordinator coordinator) { + } } diff --git a/src/main/java/com/arcadia/customperm/perm/ResyncCoordinator.java b/src/main/java/com/arcadia/customperm/perm/ResyncCoordinator.java new file mode 100644 index 0000000..845adc6 --- /dev/null +++ b/src/main/java/com/arcadia/customperm/perm/ResyncCoordinator.java @@ -0,0 +1,31 @@ +package com.arcadia.customperm.perm; + +import java.util.HashSet; +import java.util.Set; +import java.util.UUID; + +/** + * Tracks command-tree resyncs for one server lifecycle. + * + *

All operations are synchronized so closing a lifecycle cannot race with + * an event adding work after the pending set has been cleared.

+ */ +final class ResyncCoordinator { + + private final Set pending = new HashSet<>(); + private boolean active = true; + + synchronized boolean schedule(UUID uuid) { + return active && pending.add(uuid); + } + + synchronized boolean complete(UUID uuid) { + pending.remove(uuid); + return active; + } + + synchronized void close() { + active = false; + pending.clear(); + } +} diff --git a/src/test/java/com/arcadia/customperm/perm/ResyncCoordinatorTest.java b/src/test/java/com/arcadia/customperm/perm/ResyncCoordinatorTest.java new file mode 100644 index 0000000..c21195b --- /dev/null +++ b/src/test/java/com/arcadia/customperm/perm/ResyncCoordinatorTest.java @@ -0,0 +1,46 @@ +package com.arcadia.customperm.perm; + +import org.junit.jupiter.api.Test; + +import java.util.UUID; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class ResyncCoordinatorTest { + + @Test + void coalescesEventsUntilScheduledWorkCompletes() { + ResyncCoordinator coordinator = new ResyncCoordinator(); + UUID uuid = UUID.randomUUID(); + + assertTrue(coordinator.schedule(uuid)); + assertFalse(coordinator.schedule(uuid)); + assertTrue(coordinator.complete(uuid)); + assertTrue(coordinator.schedule(uuid)); + } + + @Test + void closingLifecycleInvalidatesQueuedWork() { + ResyncCoordinator coordinator = new ResyncCoordinator(); + UUID uuid = UUID.randomUUID(); + + assertTrue(coordinator.schedule(uuid)); + coordinator.close(); + + assertFalse(coordinator.complete(uuid)); + assertFalse(coordinator.schedule(uuid)); + } + + @Test + void oldLifecycleCannotBlockSamePlayerOnNewLifecycle() { + UUID uuid = UUID.randomUUID(); + ResyncCoordinator oldCoordinator = new ResyncCoordinator(); + + assertTrue(oldCoordinator.schedule(uuid)); + oldCoordinator.close(); + + ResyncCoordinator newCoordinator = new ResyncCoordinator(); + assertTrue(newCoordinator.schedule(uuid)); + } +}