From 522892440628652d0968d01daec3bdb9bea90b53 Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Thu, 15 Oct 2026 14:43:41 +0000 Subject: [PATCH 1/3] Replace /tfmc promote|demote with /tfmc promote|demote The step now follows the player's highest track: the last one in tfmc.yml whose promote or demote permission they hold. Adds the staff track (staff.promote / staff.demote). Track changes no longer need a restart because the command tree no longer depends on track names. Co-Authored-By: Claude Opus 5.5 --- .../tfmccore/tfmc/TfmcCommand.java | 54 ++++++++------ src/main/resources/tfmc.yml | 11 +-- .../tfmccore/tfmc/TfmcCommandTest.java | 70 +++++++++++++------ 3 files changed, 90 insertions(+), 45 deletions(-) diff --git a/src/main/java/net/tfminecraft/tfmccore/tfmc/TfmcCommand.java b/src/main/java/net/tfminecraft/tfmccore/tfmc/TfmcCommand.java index 29415cb..41e5d57 100644 --- a/src/main/java/net/tfminecraft/tfmccore/tfmc/TfmcCommand.java +++ b/src/main/java/net/tfminecraft/tfmccore/tfmc/TfmcCommand.java @@ -99,18 +99,12 @@ public LiteralCommandNode build() { .then(Commands.argument("reason", StringArgumentType.greedyString()) .executes(context -> ban(context, StringArgumentType.getString(context, "reason")))))); - // The track names shape the tree, so they are fixed until restart; their permissions - // are read on every check so /tcore reload tfmc applies them - for (String name : config.tracks().keySet()) { - Predicate promote = permission(() -> trackPermission(name, true)); - Predicate demote = permission(() -> trackPermission(name, false)); - root.then(Commands.literal(name) - .requires(promote.or(demote)) - .then(Commands.literal("promote").requires(promote) - .executes(player(p -> step(p, name, true)))) - .then(Commands.literal("demote").requires(demote) - .executes(player(p -> step(p, name, false))))); - } + // Promote and demote act on the player's highest track. Permissions are read on every + // check, so /tcore reload tfmc applies track changes + root.then(Commands.literal("promote").requires(source -> stepTrack(source.getSender(), true).isPresent()) + .executes(player(p -> step(p, true)))); + root.then(Commands.literal("demote").requires(source -> stepTrack(source.getSender(), false).isPresent()) + .executes(player(p -> step(p, false)))); // Content still being built on dev; each section stays hidden until tfmc.yml enables it root.then(Commands.literal("tutorial").requires(enabled("tutorials")) @@ -265,6 +259,11 @@ private int ban(CommandContext context, String rawReason) { return Command.SINGLE_SUCCESS; } + // The node's requires check has already found the track + private void step(Player player, boolean promote) { + stepTrack(player, promote).ifPresent(track -> step(player, track, promote)); + } + private void step(Player player, String track, boolean promote) { actions.stepTrack(player, track, promote).whenCompleteAsync((step, error) -> { if (error != null) { @@ -398,12 +397,24 @@ private Command player(Consumer handler) { }; } - private String trackPermission(String track, boolean promote) { - TfmcConfig.TrackSteps steps = config.tracks().get(track); - if (steps == null) { - return null; + /** + * The track a promote or demote moves the sender along: their highest track, the last in + * tfmc.yml whose promote or demote permission they hold, if they may take that step on it. + */ + private Optional stepTrack(CommandSender sender, boolean promote) { + Map.Entry highest = null; + for (Map.Entry track : config.tracks().entrySet()) { + TfmcConfig.TrackSteps steps = track.getValue(); + if (holds(sender, steps.promotePermission()) || holds(sender, steps.demotePermission())) { + highest = track; + } + } + if (highest == null) { + return Optional.empty(); } - return promote ? steps.promotePermission() : steps.demotePermission(); + TfmcConfig.TrackSteps steps = highest.getValue(); + return holds(sender, promote ? steps.promotePermission() : steps.demotePermission()) + ? Optional.of(highest.getKey()) : Optional.empty(); } private Predicate enabled(String section) { @@ -420,10 +431,11 @@ private Predicate anyPermission(String path) { // Restricted nodes fail closed: a missing or blank permission hides the command from everyone private static Predicate permission(Supplier node) { - return source -> { - String permission = node.get(); - return permission != null && !permission.isBlank() && source.getSender().hasPermission(permission); - }; + return source -> holds(source.getSender(), node.get()); + } + + private static boolean holds(CommandSender sender, String permission) { + return permission != null && !permission.isBlank() && sender.hasPermission(permission); } private static SuggestionProvider onlinePlayers() { diff --git a/src/main/resources/tfmc.yml b/src/main/resources/tfmc.yml index c51fcbc..3909108 100644 --- a/src/main/resources/tfmc.yml +++ b/src/main/resources/tfmc.yml @@ -1,8 +1,7 @@ # /tfmc player commands. Messages use MiniMessage (https://docs.advntr.dev/minimessage/format). # A message may be a single string or a list of lines. # Commands are run as written, with replaced by the player's name. -# Changes apply after /tcore reload tfmc. Adding or removing a helper track needs a restart; -# changing a track's permissions does not. +# Changes apply after /tcore reload tfmc. messages: players-only: "Only players can use this command." @@ -149,7 +148,8 @@ ban: - "tempban 8h " message: "Tempbanned for 8 hours." -# /tfmc promote|demote moves the player one step along a LuckPerms track. +# /tfmc promote|demote moves the player one step along their highest LuckPerms track: the last +# one listed here whose promote or demote permission they hold. List tracks lowest first. helper-tracks: helper: promote-permission: helper.promote @@ -157,11 +157,14 @@ helper-tracks: helper+: promote-permission: helper+.promote demote-permission: helper+.demote + staff: + promote-permission: staff.promote + demote-permission: staff.demote track-messages: promoted: "You are now ." demoted: "You are now ." removed: "You are no longer on the track." - failed: "Your helper status could not be changed." + failed: "Your status could not be changed." # /tfmc poster runs the command as the player. posters: diff --git a/src/test/java/net/tfminecraft/tfmccore/tfmc/TfmcCommandTest.java b/src/test/java/net/tfminecraft/tfmccore/tfmc/TfmcCommandTest.java index 35895ac..da76962 100644 --- a/src/test/java/net/tfminecraft/tfmccore/tfmc/TfmcCommandTest.java +++ b/src/test/java/net/tfminecraft/tfmccore/tfmc/TfmcCommandTest.java @@ -95,17 +95,35 @@ void playersSeeOnlyPublicSubcommands() { } @Test - void helperTrackMembersSeeTheStepTheyCanTake() { + void trackMembersSeeTheStepTheyCanTake() { TestPlayer offDuty = player("Helper", "helper.promote"); - assertTrue(visible(offDuty).contains("helper")); - assertFalse(visible(offDuty).contains("helper+")); - assertEquals(List.of("promote"), visible(offDuty, "helper")); + assertTrue(visible(offDuty).contains("promote")); + assertFalse(visible(offDuty).contains("demote")); TestPlayer onDuty = player("Helper", "helper.promote", "helper.demote", "tfmc.helper"); - assertEquals(List.of("promote", "demote"), visible(onDuty, "helper")); - assertTrue(visible(onDuty).contains("ban")); + assertTrue(visible(onDuty).containsAll(List.of("promote", "demote", "ban"))); - assertEquals(List.of("demote"), visible(player("Senior", "helper+.demote"), "helper+")); + assertEquals(List.of("demote"), visible(player("Senior", "helper+.demote")).stream() + .filter(name -> name.equals("promote") || name.equals("demote")).toList()); + } + + @Test + void stepsFollowTheHighestTrackThePlayerIsOn() throws Exception { + TestPlayer helper = player("Helper", "helper.promote"); + run(helper, "tfmc promote"); + TestPlayer senior = player("Senior", "helper.promote", "helper+.demote"); + run(senior, "tfmc demote"); + TestPlayer staff = player("Staff", "helper+.promote", "helper+.demote", "staff.promote", "staff.demote"); + run(staff, "tfmc demote"); + run(staff, "tfmc promote"); + assertEquals(List.of("Helper helper promote", "Senior helper+ demote", "Staff staff demote", "Staff staff promote"), + trackSteps); + + // The highest track decides, even when a lower track would allow the step + TestPlayer newStaff = player("NewStaff", "helper+.demote", "staff.promote"); + assertFalse(visible(newStaff).contains("demote")); + assertThrows(CommandSyntaxException.class, () -> run(newStaff, "tfmc demote")); + assertEquals(4, trackSteps.size()); } @Test @@ -117,8 +135,10 @@ void reloadAppliesPermissionChangesAndBlankPermissionsHideCommands() throws Exce assertFalse(visible(player("Helper", "tfmc.helper")).contains("ban")); assertTrue(visible(player("Mod", "tfmc.moderator")).contains("ban")); - assertEquals(List.of("demote"), visible(player("Helper", "helper.promote", "helper.demote"), "helper")); - assertFalse(visible(player("Anyone")).contains("helper")); + TestPlayer helper = player("Helper", "helper.promote", "helper.demote"); + assertFalse(visible(helper).contains("promote")); + assertTrue(visible(helper).contains("demote")); + assertFalse(visible(player("Anyone", "helper.promote")).contains("promote")); } @Test @@ -126,19 +146,19 @@ void trackStepsReportRemovalAndFailure() throws Exception { TestPlayer helper = player("Helper", "helper.demote"); nextStep = new TfmcActions.TrackStep(true, Optional.empty()); - run(helper, "tfmc helper demote"); + run(helper, "tfmc demote"); assertEquals(List.of("You are no longer on the helper track."), helper.messages); nextStep = TfmcActions.TrackStep.unchanged(); - run(helper, "tfmc helper demote"); - assertEquals(List.of("Your helper status could not be changed."), helper.messages); + run(helper, "tfmc demote"); + assertEquals(List.of("Your status could not be changed."), helper.messages); } @Test void luckPermsResultsAreReportedOnTheMainThread() throws Exception { TestPlayer helper = player("Helper", "helper.demote"); helper.messages.clear(); - dispatcher.execute("tfmc helper demote", source(helper.player)); + dispatcher.execute("tfmc demote", source(helper.player)); assertTrue(helper.messages.isEmpty()); runMainThread(); @@ -149,11 +169,21 @@ void luckPermsResultsAreReportedOnTheMainThread() throws Exception { void trackStepsGoThroughLuckPerms() throws Exception { TestPlayer helper = player("Helper", "helper.demote"); - run(helper, "tfmc helper demote"); + run(helper, "tfmc demote"); assertEquals(List.of("Helper helper demote"), trackSteps); assertEquals(List.of("You are now helper_player."), helper.messages); - assertThrows(CommandSyntaxException.class, () -> run(helper, "tfmc helper promote")); + assertThrows(CommandSyntaxException.class, () -> run(helper, "tfmc promote")); + } + + @Test + void consoleCannotStepATrack() throws Exception { + ConsoleCommandSender console = mock(ConsoleCommandSender.class); + when(console.hasPermission(anyString())).thenReturn(true); + List messages = capture(console); + dispatcher.execute("tfmc demote", source(console)); + assertEquals(List.of("Only players can use this command."), messages); + assertTrue(trackSteps.isEmpty()); } @Test @@ -445,10 +475,10 @@ void failedPermissionAndTrackUpdatesReportFailureOnTheMainThread() throws Except trackFailure = new IllegalStateException("Track storage unavailable"); for (String step : List.of("promote", "demote")) { helper.messages.clear(); - dispatcher.execute("tfmc helper " + step, source(helper.player)); + dispatcher.execute("tfmc " + step, source(helper.player)); assertTrue(helper.messages.isEmpty()); runMainThread(); - assertEquals(List.of("Your helper status could not be changed."), helper.messages); + assertEquals(List.of("Your status could not be changed."), helper.messages); } } @@ -475,10 +505,10 @@ void unknownPostersAndRemovedHelperTracksFailWithoutSideEffects() throws Excepti run(helper, "tfmc poster missing"); assertEquals(List.of("There is no poster called missing."), helper.messages); assertTrue(playerCommands.isEmpty()); - assertTrue(visible(helper).contains("helper")); + assertTrue(visible(helper).contains("promote")); config.loadFromString("helper-tracks: {}"); - assertFalse(visible(helper).contains("helper")); - assertThrows(CommandSyntaxException.class, () -> run(helper, "tfmc helper promote")); + assertFalse(visible(helper).contains("promote")); + assertThrows(CommandSyntaxException.class, () -> run(helper, "tfmc promote")); assertTrue(trackSteps.isEmpty()); } From c9462f14f1f5d5b45e64f99bf6b61effbb6dda23 Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Thu, 15 Oct 2026 14:48:24 +0000 Subject: [PATCH 2/3] Choose the promote/demote track by group membership Active helper+ and staff groups hold *, which matches every track's permissions, so the highest track is now the last one holding one of the player's own global groups. A step never adds a player to a track they are not on. Co-Authored-By: Claude Opus 5.5 --- .../tfmccore/tfmc/BukkitTfmcActions.java | 32 ++++++++++ .../tfmccore/tfmc/TfmcActions.java | 8 ++- .../tfmccore/tfmc/TfmcCommand.java | 19 +++--- src/main/resources/tfmc.yml | 2 +- .../tfmccore/tfmc/BukkitTfmcActionsTest.java | 58 +++++++++++++++++++ .../tfmccore/tfmc/TfmcCommandTest.java | 51 ++++++++++------ 6 files changed, 139 insertions(+), 31 deletions(-) diff --git a/src/main/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActions.java b/src/main/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActions.java index b677312..b062f08 100644 --- a/src/main/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActions.java +++ b/src/main/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActions.java @@ -2,6 +2,7 @@ import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; @@ -25,6 +26,7 @@ import net.luckperms.api.context.ImmutableContextSet; import net.luckperms.api.model.user.User; import net.luckperms.api.node.NodeType; +import net.luckperms.api.node.types.InheritanceNode; import net.luckperms.api.track.Track; import net.luckperms.api.track.TrackManager; import net.tfminecraft.tfmccore.TFMCCore; @@ -98,6 +100,23 @@ public CompletableFuture setPermission(Player player, String permission, b }); } + @Override + public Optional highestTrack(Player player, List tracks) { + LuckPerms api = luckPerms.get(); + // The cached user is fine for reading; stepTrack reloads it before changing anything + User user = api == null ? null : api.getUserManager().getUser(player.getUniqueId()); + if (user == null) { + return Optional.empty(); + } + for (String name : tracks.reversed()) { + Track track = api.getTrackManager().getTrack(name); + if (track != null && onTrack(user, track)) { + return Optional.of(name); + } + } + return Optional.empty(); + } + @Override public CompletableFuture stepTrack(Player player, String trackName, boolean promote) { LuckPerms api = luckPerms.get(); @@ -112,6 +131,11 @@ public CompletableFuture stepTrack(Player player, String trackName, b // Global context, like "lp user promote|demote " ImmutableContextSet global = api.getContextManager().getContextSetFactory().immutableEmpty(); return withFreshUser(api, player, user -> { + // Promoting someone off the track would add them to its first group + if (!onTrack(user, track)) { + LOGGER.warning(player.getName() + " is not on " + trackName + "; not changing their groups"); + return CompletableFuture.completedFuture(TrackStep.unchanged()); + } TrackStep step; String status; if (promote) { @@ -187,6 +211,14 @@ private CompletableFuture withFreshUser(LuckPerms api, Player player, Fun return result; } + /** Whether the user's own global groups, the ones track steps act on, include one on the track. */ + private static boolean onTrack(User user, Track track) { + return user.getNodes(NodeType.INHERITANCE).stream() + .filter(node -> node.getValue() && node.getContexts().isEmpty()) + .map(InheritanceNode::getGroupName) + .anyMatch(track::containsGroup); + } + private static CompletableFuture save(LuckPerms api, User user) { // The lp command pushes changes to the other servers; the API leaves that to us return api.getUserManager().saveUser(user) diff --git a/src/main/java/net/tfminecraft/tfmccore/tfmc/TfmcActions.java b/src/main/java/net/tfminecraft/tfmccore/tfmc/TfmcActions.java index 9f38e11..3b084a2 100644 --- a/src/main/java/net/tfminecraft/tfmccore/tfmc/TfmcActions.java +++ b/src/main/java/net/tfminecraft/tfmccore/tfmc/TfmcActions.java @@ -39,7 +39,13 @@ public static TrackStep unchanged() { /** Sets a LuckPerms permission to true or false for the player in this server's context. */ CompletableFuture setPermission(Player player, String permission, boolean granted); - /** Moves the player one step along a LuckPerms track. */ + /** + * The last of {@code tracks} that holds one of the player's own global groups. Group + * permissions cannot answer this, because the {@code *} on some groups matches every track. + */ + Optional highestTrack(Player player, List tracks); + + /** Moves the player one step along a LuckPerms track they are already on. */ CompletableFuture stepTrack(Player player, String track, boolean promote); /** Lets the player fly at {@code speed} and returns their previous flight settings. */ diff --git a/src/main/java/net/tfminecraft/tfmccore/tfmc/TfmcCommand.java b/src/main/java/net/tfminecraft/tfmccore/tfmc/TfmcCommand.java index 41e5d57..6ba1e16 100644 --- a/src/main/java/net/tfminecraft/tfmccore/tfmc/TfmcCommand.java +++ b/src/main/java/net/tfminecraft/tfmccore/tfmc/TfmcCommand.java @@ -399,22 +399,17 @@ private Command player(Consumer handler) { /** * The track a promote or demote moves the sender along: their highest track, the last in - * tfmc.yml whose promote or demote permission they hold, if they may take that step on it. + * tfmc.yml holding one of their groups, if they hold that step's permission on it. */ private Optional stepTrack(CommandSender sender, boolean promote) { - Map.Entry highest = null; - for (Map.Entry track : config.tracks().entrySet()) { - TfmcConfig.TrackSteps steps = track.getValue(); - if (holds(sender, steps.promotePermission()) || holds(sender, steps.demotePermission())) { - highest = track; - } - } - if (highest == null) { + if (!(sender instanceof Player player)) { return Optional.empty(); } - TfmcConfig.TrackSteps steps = highest.getValue(); - return holds(sender, promote ? steps.promotePermission() : steps.demotePermission()) - ? Optional.of(highest.getKey()) : Optional.empty(); + Map tracks = config.tracks(); + return actions.highestTrack(player, List.copyOf(tracks.keySet())).filter(track -> { + TfmcConfig.TrackSteps steps = tracks.get(track); + return holds(player, promote ? steps.promotePermission() : steps.demotePermission()); + }); } private Predicate enabled(String section) { diff --git a/src/main/resources/tfmc.yml b/src/main/resources/tfmc.yml index 3909108..ec6082c 100644 --- a/src/main/resources/tfmc.yml +++ b/src/main/resources/tfmc.yml @@ -149,7 +149,7 @@ ban: message: "Tempbanned for 8 hours." # /tfmc promote|demote moves the player one step along their highest LuckPerms track: the last -# one listed here whose promote or demote permission they hold. List tracks lowest first. +# one listed here that holds one of their global groups. List tracks lowest first. helper-tracks: helper: promote-permission: helper.promote diff --git a/src/test/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActionsTest.java b/src/test/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActionsTest.java index f99b879..ce9e190 100644 --- a/src/test/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActionsTest.java +++ b/src/test/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActionsTest.java @@ -32,6 +32,8 @@ import net.luckperms.api.model.user.User; import net.luckperms.api.model.user.UserManager; import net.luckperms.api.node.NodeBuilderRegistry; +import net.luckperms.api.node.NodeType; +import net.luckperms.api.node.types.InheritanceNode; import net.luckperms.api.node.types.PermissionNode; import net.luckperms.api.track.DemotionResult; import net.luckperms.api.track.PromotionResult; @@ -66,6 +68,8 @@ void setUp() { when(api.getNodeBuilderRegistry()).thenReturn(nodes); when(nodes.forPermission()).thenReturn(mock(PermissionNode.Builder.class, RETURNS_SELF)); when(tracks.getTrack("helper+")).thenReturn(track); + when(track.containsGroup("helper+_player")).thenReturn(true); + groups(group("helper+_player", true, true)); when(users.getUser(ID)).thenReturn(user); when(users.saveUser(user)).thenReturn(CompletableFuture.completedFuture(null)); when(user.data()).thenReturn(mock(NodeMap.class)); @@ -188,6 +192,60 @@ void callersCannotReleaseTheNextChangeEarly() { verify(users, times(2)).loadUser(ID); } + @Test + void theHighestTrackIsTheLastHoldingOneOfTheUsersOwnGlobalGroups() { + TrackManager tracks = api.getTrackManager(); + Track staff = mock(Track.class); + when(staff.containsGroup("staff_player")).thenReturn(true); + when(tracks.getTrack("staff")).thenReturn(staff); + List order = List.of("helper", "helper+", "staff"); + + assertEquals(Optional.of("helper+"), actions.highestTrack(player, order)); + + // Server-only, negated and absent tracks do not count + groups(group("helper+_player", true, true), group("staff_player", true, false), group("staff_player", false, true)); + assertEquals(Optional.of("helper+"), actions.highestTrack(player, order)); + + groups(group("helper+_player", true, true), group("staff_player", true, true)); + assertEquals(Optional.of("staff"), actions.highestTrack(player, order)); + assertEquals(Optional.of("helper+"), actions.highestTrack(player, List.of("helper+", "helper"))); + + groups(); + assertEquals(Optional.empty(), actions.highestTrack(player, order)); + } + + @Test + void noTrackWithoutLuckPermsOrALoadedUser() { + when(users.getUser(ID)).thenReturn(null); + assertEquals(Optional.empty(), actions.highestTrack(player, List.of("helper+"))); + assertEquals(Optional.empty(), new BukkitTfmcActions(null, () -> null).highestTrack(player, List.of("helper+"))); + } + + @Test + void playersOffTheTrackAreNeverAddedToIt() { + when(users.loadUser(ID)).thenReturn(CompletableFuture.completedFuture(user)); + groups(); + promoteTo("helper+_player"); + + assertFalse(actions.stepTrack(player, "helper+", true).join().changed()); + verify(track, never()).promote(any(User.class), any(ContextSet.class)); + verify(users, never()).saveUser(user); + } + + private void groups(InheritanceNode... nodes) { + when(user.getNodes(NodeType.INHERITANCE)).thenReturn(List.of(nodes)); + } + + private static InheritanceNode group(String name, boolean value, boolean global) { + InheritanceNode node = mock(InheritanceNode.class); + ImmutableContextSet contexts = mock(ImmutableContextSet.class); + when(contexts.isEmpty()).thenReturn(global); + when(node.getGroupName()).thenReturn(name); + when(node.getValue()).thenReturn(value); + when(node.getContexts()).thenReturn(contexts); + return node; + } + private void promoteTo(String group) { PromotionResult promoted = mock(PromotionResult.class); when(promoted.wasSuccessful()).thenReturn(true); diff --git a/src/test/java/net/tfminecraft/tfmccore/tfmc/TfmcCommandTest.java b/src/test/java/net/tfminecraft/tfmccore/tfmc/TfmcCommandTest.java index da76962..6f11899 100644 --- a/src/test/java/net/tfminecraft/tfmccore/tfmc/TfmcCommandTest.java +++ b/src/test/java/net/tfminecraft/tfmccore/tfmc/TfmcCommandTest.java @@ -17,7 +17,9 @@ import java.time.Instant; import java.time.ZoneOffset; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.UUID; @@ -55,6 +57,8 @@ class TfmcCommandTest { private final List scheduledTicks = new ArrayList<>(); private final List scheduled = new ArrayList<>(); private int cancelled; + // Player name to the highest track holding one of their groups + private final Map highestTracks = new HashMap<>(Map.of("Helper", "helper", "Senior", "helper+")); private TfmcActions.TrackStep nextStep; private RuntimeException permissionFailure; private RuntimeException trackFailure; @@ -103,27 +107,31 @@ void trackMembersSeeTheStepTheyCanTake() { TestPlayer onDuty = player("Helper", "helper.promote", "helper.demote", "tfmc.helper"); assertTrue(visible(onDuty).containsAll(List.of("promote", "demote", "ban"))); - assertEquals(List.of("demote"), visible(player("Senior", "helper+.demote")).stream() - .filter(name -> name.equals("promote") || name.equals("demote")).toList()); + TestPlayer senior = player("Senior", "helper+.demote"); + assertTrue(visible(senior).contains("demote")); + assertFalse(visible(senior).contains("promote")); } @Test - void stepsFollowTheHighestTrackThePlayerIsOn() throws Exception { - TestPlayer helper = player("Helper", "helper.promote"); - run(helper, "tfmc promote"); - TestPlayer senior = player("Senior", "helper.promote", "helper+.demote"); - run(senior, "tfmc demote"); - TestPlayer staff = player("Staff", "helper+.promote", "helper+.demote", "staff.promote", "staff.demote"); - run(staff, "tfmc demote"); - run(staff, "tfmc promote"); - assertEquals(List.of("Helper helper promote", "Senior helper+ demote", "Staff staff demote", "Staff staff promote"), - trackSteps); + void stepsFollowTheHighestTrackThePlayerIsIn() throws Exception { + // Active helper+ and staff groups hold *, which matches every track's permissions + highestTracks.put("Wildcard", "helper+"); + run(everything("Wildcard"), "tfmc demote"); + highestTracks.put("Staff", "staff"); + run(everything("Staff"), "tfmc demote"); + run(player("Helper", "helper.promote"), "tfmc promote"); + assertEquals(List.of("Wildcard helper+ demote", "Staff staff demote", "Helper helper promote"), trackSteps); // The highest track decides, even when a lower track would allow the step + highestTracks.put("NewStaff", "staff"); TestPlayer newStaff = player("NewStaff", "helper+.demote", "staff.promote"); assertFalse(visible(newStaff).contains("demote")); assertThrows(CommandSyntaxException.class, () -> run(newStaff, "tfmc demote")); - assertEquals(4, trackSteps.size()); + + TestPlayer offTrack = everything("Admin"); + assertFalse(visible(offTrack).contains("promote")); + assertThrows(CommandSyntaxException.class, () -> run(offTrack, "tfmc promote")); + assertEquals(3, trackSteps.size()); } @Test @@ -138,7 +146,7 @@ void reloadAppliesPermissionChangesAndBlankPermissionsHideCommands() throws Exce TestPlayer helper = player("Helper", "helper.promote", "helper.demote"); assertFalse(visible(helper).contains("promote")); assertTrue(visible(helper).contains("demote")); - assertFalse(visible(player("Anyone", "helper.promote")).contains("promote")); + assertFalse(visible(player("Anyone", "helper.demote")).contains("demote")); } @Test @@ -180,9 +188,7 @@ void trackStepsGoThroughLuckPerms() throws Exception { void consoleCannotStepATrack() throws Exception { ConsoleCommandSender console = mock(ConsoleCommandSender.class); when(console.hasPermission(anyString())).thenReturn(true); - List messages = capture(console); - dispatcher.execute("tfmc demote", source(console)); - assertEquals(List.of("Only players can use this command."), messages); + assertThrows(CommandSyntaxException.class, () -> dispatcher.execute("tfmc demote", source(console))); assertTrue(trackSteps.isEmpty()); } @@ -558,6 +564,12 @@ private static TestPlayer player(String name, String... permissions) { return new TestPlayer(player, capture(player)); } + private static TestPlayer everything(String name) { + TestPlayer player = player(name); + when(player.player.hasPermission(anyString())).thenReturn(true); + return player; + } + private static List capture(CommandSender sender) { List messages = new ArrayList<>(); doAnswer(call -> messages.add(plain(call.getArgument(0)))) @@ -622,6 +634,11 @@ public Runnable later(long ticks, Runnable task) { return () -> cancelled++; } + @Override + public Optional highestTrack(Player player, List tracks) { + return Optional.ofNullable(highestTracks.get(player.getName())).filter(tracks::contains); + } + @Override public CompletableFuture stepTrack(Player player, String track, boolean promote) { trackSteps.add(player.getName() + " " + track + " " + (promote ? "promote" : "demote")); From 77a12e7a1b1337f7a639787e8c26cc502635826d Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Thu, 15 Oct 2026 14:59:33 +0000 Subject: [PATCH 3/3] Read track membership from stored groups only Track steps act on a user's normal data, so a transient group no longer counts: stepping it would save it as permanent. Co-Authored-By: Claude Opus 5.5 --- .../tfmccore/tfmc/BukkitTfmcActions.java | 9 +++++++-- .../tfmc/BukkitTfmcActionsCoverageTest.java | 2 +- .../tfmccore/tfmc/BukkitTfmcActionsTest.java | 16 +++++++++++----- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/main/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActions.java b/src/main/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActions.java index b062f08..997a7f2 100644 --- a/src/main/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActions.java +++ b/src/main/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActions.java @@ -211,9 +211,14 @@ private CompletableFuture withFreshUser(LuckPerms api, Player player, Fun return result; } - /** Whether the user's own global groups, the ones track steps act on, include one on the track. */ + /** + * Whether the user's own stored global groups, the ones track steps act on, include one on + * the track. Transient groups are left out: a step would save them as permanent. + */ private static boolean onTrack(User user, Track track) { - return user.getNodes(NodeType.INHERITANCE).stream() + return user.data().toCollection().stream() + .filter(NodeType.INHERITANCE::matches) + .map(NodeType.INHERITANCE::cast) .filter(node -> node.getValue() && node.getContexts().isEmpty()) .map(InheritanceNode::getGroupName) .anyMatch(track::containsGroup); diff --git a/src/test/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActionsCoverageTest.java b/src/test/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActionsCoverageTest.java index 18ba382..8ebaf25 100644 --- a/src/test/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActionsCoverageTest.java +++ b/src/test/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActionsCoverageTest.java @@ -250,7 +250,7 @@ void globalPermissionChangesRemoveOnlyTheMatchingPermissionAndPushTheSavedUpdate @Test void aFailedUserLoadReleasesTheQueueAndAnUnsuccessfulDemotionDoesNotSave() { LuckPerms api = mock(LuckPerms.class, RETURNS_DEEP_STUBS); - User user = mock(User.class); + User user = mock(User.class, RETURNS_DEEP_STUBS); Player player = mock(Player.class); UUID id = UUID.randomUUID(); when(player.getUniqueId()).thenReturn(id); diff --git a/src/test/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActionsTest.java b/src/test/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActionsTest.java index ce9e190..8e15498 100644 --- a/src/test/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActionsTest.java +++ b/src/test/java/net/tfminecraft/tfmccore/tfmc/BukkitTfmcActionsTest.java @@ -31,6 +31,7 @@ import net.luckperms.api.model.data.NodeMap; import net.luckperms.api.model.user.User; import net.luckperms.api.model.user.UserManager; +import net.luckperms.api.node.Node; import net.luckperms.api.node.NodeBuilderRegistry; import net.luckperms.api.node.NodeType; import net.luckperms.api.node.types.InheritanceNode; @@ -48,6 +49,7 @@ class BukkitTfmcActionsTest { private final UserManager users = mock(UserManager.class); private final Track track = mock(Track.class); private final User user = mock(User.class); + private final NodeMap data = mock(NodeMap.class); private final Player player = mock(Player.class); private BukkitTfmcActions actions; @@ -72,7 +74,7 @@ void setUp() { groups(group("helper+_player", true, true)); when(users.getUser(ID)).thenReturn(user); when(users.saveUser(user)).thenReturn(CompletableFuture.completedFuture(null)); - when(user.data()).thenReturn(mock(NodeMap.class)); + when(user.data()).thenReturn(data); when(player.getUniqueId()).thenReturn(ID); when(player.getName()).thenReturn("RiverBonnie"); actions = new BukkitTfmcActions(null, () -> api); @@ -202,8 +204,11 @@ void theHighestTrackIsTheLastHoldingOneOfTheUsersOwnGlobalGroups() { assertEquals(Optional.of("helper+"), actions.highestTrack(player, order)); - // Server-only, negated and absent tracks do not count - groups(group("helper+_player", true, true), group("staff_player", true, false), group("staff_player", false, true)); + // Server-only, negated and non-group nodes and absent tracks do not count + PermissionNode permission = mock(PermissionNode.class); + when(permission.getType()).thenReturn(NodeType.PERMISSION); + groups(group("helper+_player", true, true), group("staff_player", true, false), group("staff_player", false, true), + permission); assertEquals(Optional.of("helper+"), actions.highestTrack(player, order)); groups(group("helper+_player", true, true), group("staff_player", true, true)); @@ -232,8 +237,8 @@ void playersOffTheTrackAreNeverAddedToIt() { verify(users, never()).saveUser(user); } - private void groups(InheritanceNode... nodes) { - when(user.getNodes(NodeType.INHERITANCE)).thenReturn(List.of(nodes)); + private void groups(Node... nodes) { + when(data.toCollection()).thenReturn(List.of(nodes)); } private static InheritanceNode group(String name, boolean value, boolean global) { @@ -243,6 +248,7 @@ private static InheritanceNode group(String name, boolean value, boolean global) when(node.getGroupName()).thenReturn(name); when(node.getValue()).thenReturn(value); when(node.getContexts()).thenReturn(contexts); + when(node.getType()).thenReturn(NodeType.INHERITANCE); return node; }