Extract counter command authorization into testable helpers

Pull the pure guard logic out of cmdDelCounter and cmdSetCounterActive
  into static evaluateDelCounter/evaluateSetCounterActive methods, leaving
  the handlers as thin shells over the decision. Add command_zone_tests
  covering every authorization branch of both evaluators.

libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp

@@ -538,28 +538,37 @@ Server_Player::cmdSetCounter(const Command_SetCounter &cmd, ResponseContainer &
 }
 
 Response::ResponseCode
-Server_Player::cmdDelCounter(const Command_DelCounter &cmd, ResponseContainer & /*rc*/, GameEventStorage &ges)
+Server_Player::evaluateDelCounter(bool gameStarted, bool playerConceded, int counterId, const Server_Counter *counter)
 {
-    if (!game->getGameStarted()) {
+    if (!gameStarted) {
         return Response::RespGameNotStarted;
     }
-    if (conceded) {
+    if (playerConceded) {
         return Response::RespContextError;
     }
-
-    const int counterId = cmd.counter_id();
-
     // Reserved tax counters are server-managed system counters and must never be
     // deleted by a client. When the command zone is disabled they don't exist, so
     // a lookup would fail anyway; when it's enabled they must persist for the game.
     if (CounterIds::isTaxCounter(counterId)) {
         return Response::RespFunctionNotAllowed;
     }
-
-    Server_Counter *counter = counters.value(counterId, nullptr);
     if (!counter) {
         return Response::RespNameNotFound;
     }
+    return Response::RespOk;
+}
+
+Response::ResponseCode
+Server_Player::cmdDelCounter(const Command_DelCounter &cmd, ResponseContainer & /*rc*/, GameEventStorage &ges)
+{
+    const int counterId = cmd.counter_id();
+    Server_Counter *counter = counters.value(counterId, nullptr);
+
+    const Response::ResponseCode authResult = evaluateDelCounter(game->getGameStarted(), conceded, counterId, counter);
+    if (authResult != Response::RespOk) {
+        return authResult;
+    }
+
     counters.remove(counterId);
     delete counter;
 
@@ -570,38 +579,49 @@ Server_Player::cmdDelCounter(const Command_DelCounter &cmd, ResponseContainer &
     return Response::RespOk;
 }
 
+Response::ResponseCode Server_Player::evaluateSetCounterActive(bool gameStarted,
+                                                               bool playerConceded,
+                                                               bool commandZoneEnabled,
+                                                               int counterId,
+                                                               const Server_Counter *counter,
+                                                               bool requestedActive)
+{
+    if (!gameStarted) {
+        return Response::RespGameNotStarted;
+    }
+    if (playerConceded) {
+        return Response::RespContextError;
+    }
+    if (!CounterIds::isTaxCounter(counterId)) {
+        return Response::RespFunctionNotAllowed;
+    }
+    if (!commandZoneEnabled) {
+        return Response::RespContextError;
+    }
+    if (!counter) {
+        return Response::RespNameNotFound;
+    }
+    // Prevent disabling a counter with tax accumulated; player must reset to 0 first
+    if (!requestedActive && counter->getCount() != 0) {
+        return Response::RespContextError;
+    }
+    return Response::RespOk;
+}
+
 Response::ResponseCode Server_Player::cmdSetCounterActive(const Command_SetCounterActive &cmd,
                                                           ResponseContainer & /*rc*/,
                                                           GameEventStorage &ges)
 {
-    if (!game->getGameStarted()) {
-        return Response::RespGameNotStarted;
-    }
-    if (conceded) {
-        return Response::RespContextError;
-    }
-
     const int counterId = cmd.counter_id();
-    if (!CounterIds::isTaxCounter(counterId)) {
-        return Response::RespFunctionNotAllowed;
-    }
-    if (!game->getEnableCommandZone()) {
-        return Response::RespContextError;
-    }
-
     Server_Counter *c = counters.value(counterId, nullptr);
-    if (!c) {
+
-        return Response::RespNameNotFound;
+    const Response::ResponseCode authResult = evaluateSetCounterActive(
+        game->getGameStarted(), conceded, game->getEnableCommandZone(), counterId, c, cmd.active());
+    if (authResult != Response::RespOk) {
+        return authResult;
     }
 
-    // Prevent disabling a counter with tax accumulated; player must reset to 0 first
+    if (c->setActive(cmd.active())) {
-    if (!cmd.active() && c->getCount() != 0) {
-        return Response::RespContextError;
-    }
-
-    bool didChange = c->setActive(cmd.active());
-
-    if (didChange) {
         Event_SetCounterActive event;
         event.set_counter_id(c->getId());
         event.set_active(c->isActive());

libcockatrice_network/libcockatrice/network/server/remote/game/server_player.h

@@ -25,6 +25,19 @@ public:
     int newCounterId() const;
     void addCounter(Server_Counter *counter);
 
+    // Pure authorization/decision logic extracted from the corresponding cmd* handlers
+    // so it can be unit-tested in isolation. These take all relevant state as parameters
+    // and touch no instance members, hence static. They return RespOk when the command
+    // is permitted, or the appropriate error response otherwise.
+    static Response::ResponseCode
+    evaluateDelCounter(bool gameStarted, bool playerConceded, int counterId, const Server_Counter *counter);
+    static Response::ResponseCode evaluateSetCounterActive(bool gameStarted,
+                                                           bool playerConceded,
+                                                           bool commandZoneEnabled,
+                                                           int counterId,
+                                                           const Server_Counter *counter,
+                                                           bool requestedActive);
+
     void setupZones() override;
     void clearZones() override;
 

tests/CMakeLists.txt

@@ -78,6 +78,7 @@ target_link_libraries(
 
 add_subdirectory(card_zone_algorithms)
 add_subdirectory(carddatabase)
+add_subdirectory(command_zone_tests)
 add_subdirectory(loading_from_clipboard)
 add_subdirectory(movecard_tests)
 add_subdirectory(oracle)

tests/command_zone_tests/CMakeLists.txt

@@ -0,0 +1,33 @@
+add_executable(new_counter_id_test new_counter_id_test.cpp)
+
+if(NOT GTEST_FOUND)
+  add_dependencies(new_counter_id_test gtest)
+endif()
+
+target_link_libraries(
+  new_counter_id_test
+  PRIVATE libcockatrice_network_server_remote
+  PRIVATE libcockatrice_rng
+  PRIVATE Threads::Threads
+  PRIVATE ${GTEST_BOTH_LIBRARIES}
+  PRIVATE ${TEST_QT_MODULES}
+)
+
+add_test(NAME new_counter_id_test COMMAND new_counter_id_test)
+
+add_executable(counter_command_auth_test counter_command_auth_test.cpp)
+
+if(NOT GTEST_FOUND)
+  add_dependencies(counter_command_auth_test gtest)
+endif()
+
+target_link_libraries(
+  counter_command_auth_test
+  PRIVATE libcockatrice_network_server_remote
+  PRIVATE libcockatrice_rng
+  PRIVATE Threads::Threads
+  PRIVATE ${GTEST_BOTH_LIBRARIES}
+  PRIVATE ${TEST_QT_MODULES}
+)
+
+add_test(NAME counter_command_auth_test COMMAND counter_command_auth_test)

tests/command_zone_tests/counter_command_auth_test.cpp

@@ -0,0 +1,165 @@
+// Unit tests for the pure authorization logic extracted from the counter command
+// handlers. Server_Player::evaluateDelCounter() and evaluateSetCounterActive() are
+// static and depend only on their arguments, so each guard branch can be exercised
+// directly without a started game, network stack, or player fixture.
+
+#include "game/server_counter.h"
+#include "game/server_player.h"
+
+#include <gtest/gtest.h>
+#include <libcockatrice/protocol/pb/color.pb.h>
+#include <libcockatrice/protocol/pb/response.pb.h>
+#include <libcockatrice/rng/rng_abstract.h>
+#include <libcockatrice/utility/counter_ids.h>
+
+RNG_Abstract *rng = nullptr; // required by linked server code
+
+namespace
+{
+// A user-range counter id that is never a reserved tax counter.
+constexpr int UserCounterId = CounterIds::FirstUserId;
+
+// Builds a counter with the given id and current value; the other fields are
+// irrelevant to the authorization decisions under test.
+Server_Counter makeCounter(int id, int count)
+{
+    return Server_Counter(id, "c", color(), 20, count);
+}
+} // namespace
+
+// ---------------------------------------------------------------------------
+// evaluateDelCounter
+// ---------------------------------------------------------------------------
+
+// The game must be started before any counter can be deleted.
+TEST(EvaluateDelCounter, RejectsWhenGameNotStarted)
+{
+    Server_Counter counter = makeCounter(UserCounterId, 0);
+    EXPECT_EQ(
+        Server_Player::evaluateDelCounter(/*gameStarted=*/false, /*playerConceded=*/false, UserCounterId, &counter),
+        Response::RespGameNotStarted);
+}
+
+// A conceded player may no longer mutate counters.
+TEST(EvaluateDelCounter, RejectsWhenPlayerConceded)
+{
+    Server_Counter counter = makeCounter(UserCounterId, 0);
+    EXPECT_EQ(Server_Player::evaluateDelCounter(/*gameStarted=*/true, /*playerConceded=*/true, UserCounterId, &counter),
+              Response::RespContextError);
+}
+
+// Reserved tax counters are server-managed and may never be deleted by a client.
+TEST(EvaluateDelCounter, RejectsTaxCounters)
+{
+    Server_Counter commander = makeCounter(CounterIds::CommanderTax, 0);
+    EXPECT_EQ(Server_Player::evaluateDelCounter(true, false, CounterIds::CommanderTax, &commander),
+              Response::RespFunctionNotAllowed);
+
+    Server_Counter partner = makeCounter(CounterIds::PartnerTax, 0);
+    EXPECT_EQ(Server_Player::evaluateDelCounter(true, false, CounterIds::PartnerTax, &partner),
+              Response::RespFunctionNotAllowed);
+}
+
+// A non-existent counter (null lookup) is reported as not found.
+TEST(EvaluateDelCounter, RejectsMissingCounter)
+{
+    EXPECT_EQ(Server_Player::evaluateDelCounter(true, false, UserCounterId, nullptr), Response::RespNameNotFound);
+}
+
+// A live user counter in a started, non-conceded game may be deleted.
+TEST(EvaluateDelCounter, AllowsDeletingUserCounter)
+{
+    Server_Counter counter = makeCounter(UserCounterId, 7);
+    EXPECT_EQ(Server_Player::evaluateDelCounter(true, false, UserCounterId, &counter), Response::RespOk);
+}
+
+// The game-not-started guard is checked before the tax-counter guard: an attempt
+// to delete a tax counter before the game starts reports RespGameNotStarted.
+TEST(EvaluateDelCounter, GameNotStartedTakesPrecedenceOverTaxGuard)
+{
+    Server_Counter commander = makeCounter(CounterIds::CommanderTax, 0);
+    EXPECT_EQ(Server_Player::evaluateDelCounter(false, false, CounterIds::CommanderTax, &commander),
+              Response::RespGameNotStarted);
+}
+
+// ---------------------------------------------------------------------------
+// evaluateSetCounterActive
+// ---------------------------------------------------------------------------
+
+// The game must be started before a counter's active state can change.
+TEST(EvaluateSetCounterActive, RejectsWhenGameNotStarted)
+{
+    Server_Counter counter = makeCounter(CounterIds::CommanderTax, 0);
+    EXPECT_EQ(Server_Player::evaluateSetCounterActive(/*gameStarted=*/false, /*playerConceded=*/false,
+                                                      /*commandZoneEnabled=*/true, CounterIds::CommanderTax, &counter,
+                                                      /*requestedActive=*/true),
+              Response::RespGameNotStarted);
+}
+
+// A conceded player may no longer toggle counters.
+TEST(EvaluateSetCounterActive, RejectsWhenPlayerConceded)
+{
+    Server_Counter counter = makeCounter(CounterIds::CommanderTax, 0);
+    EXPECT_EQ(Server_Player::evaluateSetCounterActive(true, /*playerConceded=*/true, true, CounterIds::CommanderTax,
+                                                      &counter, true),
+              Response::RespContextError);
+}
+
+// Only the reserved tax counters support an active/inactive toggle; user counters
+// are rejected as a disallowed operation.
+TEST(EvaluateSetCounterActive, RejectsNonTaxCounter)
+{
+    Server_Counter counter = makeCounter(UserCounterId, 0);
+    EXPECT_EQ(Server_Player::evaluateSetCounterActive(true, false, true, UserCounterId, &counter, true),
+              Response::RespFunctionNotAllowed);
+}
+
+// Toggling a tax counter is only meaningful when the command zone is enabled.
+TEST(EvaluateSetCounterActive, RejectsWhenCommandZoneDisabled)
+{
+    Server_Counter counter = makeCounter(CounterIds::CommanderTax, 0);
+    EXPECT_EQ(Server_Player::evaluateSetCounterActive(true, false, /*commandZoneEnabled=*/false,
+                                                      CounterIds::CommanderTax, &counter, true),
+              Response::RespContextError);
+}
+
+// A missing counter is reported as not found.
+TEST(EvaluateSetCounterActive, RejectsMissingCounter)
+{
+    EXPECT_EQ(Server_Player::evaluateSetCounterActive(true, false, true, CounterIds::CommanderTax, nullptr, true),
+              Response::RespNameNotFound);
+}
+
+// A tax counter may not be disabled while it still holds accumulated tax; the
+// player must reset it to zero first.
+TEST(EvaluateSetCounterActive, RejectsDisablingWhenTaxAccumulated)
+{
+    Server_Counter counter = makeCounter(CounterIds::CommanderTax, 3);
+    EXPECT_EQ(Server_Player::evaluateSetCounterActive(true, false, true, CounterIds::CommanderTax, &counter,
+                                                      /*requestedActive=*/false),
+              Response::RespContextError);
+}
+
+// Enabling a tax counter is allowed regardless of its current value.
+TEST(EvaluateSetCounterActive, AllowsEnablingWithAccumulatedTax)
+{
+    Server_Counter counter = makeCounter(CounterIds::CommanderTax, 3);
+    EXPECT_EQ(Server_Player::evaluateSetCounterActive(true, false, true, CounterIds::CommanderTax, &counter,
+                                                      /*requestedActive=*/true),
+              Response::RespOk);
+}
+
+// Disabling a tax counter that is already at zero is permitted.
+TEST(EvaluateSetCounterActive, AllowsDisablingWhenCounterIsZero)
+{
+    Server_Counter counter = makeCounter(CounterIds::CommanderTax, 0);
+    EXPECT_EQ(Server_Player::evaluateSetCounterActive(true, false, true, CounterIds::CommanderTax, &counter,
+                                                      /*requestedActive=*/false),
+              Response::RespOk);
+}
+
+int main(int argc, char **argv)
+{
+    ::testing::InitGoogleTest(&argc, argv);
+    return RUN_ALL_TESTS();
+}

tests/command_zone_tests/new_counter_id_test.cpp

@@ -0,0 +1,76 @@
+#include "../movecard_tests/server_test_helpers.h"
+#include "game/server_counter.h"
+#include "game/server_game.h"
+#include "game/server_player.h"
+#include "server_room.h"
+
+#include <gtest/gtest.h>
+#include <libcockatrice/protocol/pb/color.pb.h>
+#include <libcockatrice/protocol/pb/serverinfo_user.pb.h>
+#include <libcockatrice/rng/rng_abstract.h>
+#include <libcockatrice/utility/counter_ids.h>
+
+RNG_Abstract *rng = nullptr; // required by linked server code
+
+namespace
+{
+// Wires up a Server_Player backed by a minimal fake game so that counters can be
+// added directly via addCounter() to exercise newCounterId() in isolation.
+// newCounterId() depends only on the player's counter map, not on game state,
+// so the game never needs to be started.
+struct PlayerFixture
+{
+    ServerInfo_User user;
+    FakeServer server;
+    Server_Room room{0, 0, "", "", "", "", false, "", {}, &server};
+    Server_Game game{user,  1,     "",    "",    2,  QList<int>(), false, false,
+                     false, false, false, false, 20, false,        false, &room};
+    Server_Player player{&game, 1, user, false, nullptr};
+
+    ~PlayerFixture()
+    {
+        player.clearZones(); // owns and deletes any counters added during the test
+    }
+};
+} // namespace
+
+// With no counters at all, the first allocated id must be FirstUserId, never 0.
+TEST(NewCounterId, ReturnsFirstUserIdWhenNoCounters)
+{
+    PlayerFixture f;
+    EXPECT_EQ(f.player.newCounterId(), CounterIds::FirstUserId);
+}
+
+// Reserved ids 0-9 (standard player counters and the commander tax counters) must
+// not drag a new user counter down into the reserved range.
+TEST(NewCounterId, SkipsReservedRangeWhenOnlyReservedCountersExist)
+{
+    PlayerFixture f;
+    f.player.addCounter(new Server_Counter(3, "g", color(), 20, 0));
+    f.player.addCounter(new Server_Counter(CounterIds::PartnerTax, CounterNames::PartnerTax, color(), 20, 0));
+    EXPECT_EQ(f.player.newCounterId(), CounterIds::FirstUserId);
+}
+
+// Once user counters exist, the next id is one above the highest of them.
+TEST(NewCounterId, ReturnsNextIdAboveHighestUserCounter)
+{
+    PlayerFixture f;
+    f.player.addCounter(new Server_Counter(CounterIds::FirstUserId, "a", color(), 20, 0)); // 10
+    f.player.addCounter(new Server_Counter(15, "b", color(), 20, 0));
+    EXPECT_EQ(f.player.newCounterId(), 16);
+}
+
+// A reserved counter present alongside user counters must not affect the result.
+TEST(NewCounterId, IgnoresReservedCountersWhenUserCountersPresent)
+{
+    PlayerFixture f;
+    f.player.addCounter(new Server_Counter(5, "r", color(), 20, 0));     // reserved range
+    f.player.addCounter(new Server_Counter(11, "user", color(), 20, 0)); // user range
+    EXPECT_EQ(f.player.newCounterId(), 12);
+}
+
+int main(int argc, char **argv)
+{
+    ::testing::InitGoogleTest(&argc, argv);
+    return RUN_ALL_TESTS();
+}