From 8bf2d836a6908399bc1209e6b07019ef4b679d3a Mon Sep 17 00:00:00 2001 From: DawnFire42 Date: Tue, 16 Jun 2026 14:36:46 -0400 Subject: [PATCH] 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. --- .../server/remote/game/server_player.cpp | 84 +++++---- .../server/remote/game/server_player.h | 13 ++ tests/CMakeLists.txt | 1 + tests/command_zone_tests/CMakeLists.txt | 33 ++++ .../counter_command_auth_test.cpp | 165 ++++++++++++++++++ .../new_counter_id_test.cpp | 76 ++++++++ 6 files changed, 340 insertions(+), 32 deletions(-) create mode 100644 tests/command_zone_tests/CMakeLists.txt create mode 100644 tests/command_zone_tests/counter_command_auth_test.cpp create mode 100644 tests/command_zone_tests/new_counter_id_test.cpp diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp index 74730f519..8907448f6 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp +++ b/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 (!cmd.active() && c->getCount() != 0) { - return Response::RespContextError; - } - - bool didChange = c->setActive(cmd.active()); - - if (didChange) { + if (c->setActive(cmd.active())) { Event_SetCounterActive event; event.set_counter_id(c->getId()); event.set_active(c->isActive()); diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.h b/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.h index eb4559631..608f85b8b 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.h +++ b/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; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 04ac7fcee..dd7e26ee1 100644 --- a/tests/CMakeLists.txt +++ b/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) diff --git a/tests/command_zone_tests/CMakeLists.txt b/tests/command_zone_tests/CMakeLists.txt new file mode 100644 index 000000000..1bfe04a24 --- /dev/null +++ b/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) diff --git a/tests/command_zone_tests/counter_command_auth_test.cpp b/tests/command_zone_tests/counter_command_auth_test.cpp new file mode 100644 index 000000000..0b93e1ddf --- /dev/null +++ b/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 +#include +#include +#include +#include + +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(); +} diff --git a/tests/command_zone_tests/new_counter_id_test.cpp b/tests/command_zone_tests/new_counter_id_test.cpp new file mode 100644 index 000000000..679ee7062 --- /dev/null +++ b/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 +#include +#include +#include +#include + +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(), 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(); +}