diff --git a/cockatrice/src/game/player/player_actions.cpp b/cockatrice/src/game/player/player_actions.cpp index 5cc2f242d..70458ab46 100644 --- a/cockatrice/src/game/player/player_actions.cpp +++ b/cockatrice/src/game/player/player_actions.cpp @@ -1663,23 +1663,21 @@ void PlayerActions::playSelectedCardsImpl(QList selectedCards, void PlayerActions::actPlayAndIncreaseTax(QList selectedCards) { - playSelectedCardsImpl(selectedCards, false, [this](CardItem * /*card*/, const QString &originalZone) { - if (originalZone == ZoneNames::COMMAND) { - CounterState *state = player->getCounters().value(CounterIds::CommanderTax, nullptr); - if (state && state->isActive()) { - sendIncCounter(CounterIds::CommanderTax, 1); - } - } - }); + playAndIncreaseTax(selectedCards, CounterIds::CommanderTax); } void PlayerActions::actPlayAndIncreasePartnerTax(QList selectedCards) { - playSelectedCardsImpl(selectedCards, false, [this](CardItem * /*card*/, const QString &originalZone) { + playAndIncreaseTax(selectedCards, CounterIds::PartnerTax); +} + +void PlayerActions::playAndIncreaseTax(QList selectedCards, int counterId) +{ + playSelectedCardsImpl(selectedCards, false, [this, counterId](CardItem * /*card*/, const QString &originalZone) { if (originalZone == ZoneNames::COMMAND) { - CounterState *state = player->getCounters().value(CounterIds::PartnerTax, nullptr); + CounterState *state = player->getCounters().value(counterId, nullptr); if (state && state->isActive()) { - sendIncCounter(CounterIds::PartnerTax, 1); + sendIncCounter(counterId, 1); } } }); diff --git a/cockatrice/src/game/player/player_actions.h b/cockatrice/src/game/player/player_actions.h index 75ad0483e..0ca9ba76b 100644 --- a/cockatrice/src/game/player/player_actions.h +++ b/cockatrice/src/game/player/player_actions.h @@ -267,6 +267,14 @@ private: bool faceDown, const std::function &postPlayCallback = nullptr); + /** + * @brief Plays the selected cards and, for each that came from the command zone, + * increments the given (active) tax counter by one. + * @param selectedCards Cards to play + * @param counterId The tax counter to increment (CounterIds::CommanderTax or PartnerTax) + */ + void playAndIncreaseTax(QList selectedCards, int counterId); + void cmdSetTopCard(Command_MoveCard &cmd); void cmdSetBottomCard(Command_MoveCard &cmd); diff --git a/cockatrice/src/game/player/player_logic.h b/cockatrice/src/game/player/player_logic.h index 0394bbb74..345ecdd7e 100644 --- a/cockatrice/src/game/player/player_logic.h +++ b/cockatrice/src/game/player/player_logic.h @@ -59,7 +59,6 @@ class ServerInfo_Counter; class ServerInfo_Player; class ServerInfo_User; class TabGame; -class AbstractCounter; const int MAX_TOKENS_PER_DIALOG = 99; diff --git a/cockatrice/src/game_graphics/board/abstract_counter.cpp b/cockatrice/src/game_graphics/board/abstract_counter.cpp index 46eb7cba3..79f385090 100644 --- a/cockatrice/src/game_graphics/board/abstract_counter.cpp +++ b/cockatrice/src/game_graphics/board/abstract_counter.cpp @@ -28,10 +28,9 @@ AbstractCounter::AbstractCounter(CounterState *state, { setAcceptHoverEvents(true); - connect(state, &CounterState::valueChanged, this, [this](int, int newValue) { - value = newValue; - update(); - }); + // Route through the (possibly overridden) virtual setValue so subclasses such as + // CommanderTaxCounter can clamp and refresh their tooltip on every value change. + connect(state, &CounterState::valueChanged, this, [this](int, int newValue) { setValue(newValue); }); connect(state, &CounterState::activeChanged, this, [this](bool newActive) { setActive(newActive); diff --git a/cockatrice/src/game_graphics/player/menu/move_menu.cpp b/cockatrice/src/game_graphics/player/menu/move_menu.cpp index becbc3625..d5f8ff226 100644 --- a/cockatrice/src/game_graphics/player/menu/move_menu.cpp +++ b/cockatrice/src/game_graphics/player/menu/move_menu.cpp @@ -39,6 +39,7 @@ MoveMenu::MoveMenu(PlayerGraphicsItem *player) : QMenu(tr("Move to")) connect(aMoveToHand, &QAction::triggered, actions, invoke(cmMoveToHand)); connect(aMoveToGraveyard, &QAction::triggered, actions, invoke(cmMoveToGraveyard)); connect(aMoveToExile, &QAction::triggered, actions, invoke(cmMoveToExile)); + connect(aMoveToCommandZone, &QAction::triggered, actions, invoke(cmMoveToCommandZone)); addAction(aMoveToTopLibrary); addAction(aMoveToXfromTopOfLibrary); diff --git a/cockatrice/src/game_graphics/player/player_graphics_item.cpp b/cockatrice/src/game_graphics/player/player_graphics_item.cpp index 274815be8..e88c7f64b 100644 --- a/cockatrice/src/game_graphics/player/player_graphics_item.cpp +++ b/cockatrice/src/game_graphics/player/player_graphics_item.cpp @@ -18,6 +18,10 @@ #include #include +// Minimum height the stack zone keeps when the command zone shares its vertical space, +// so the stack remains usable. Owned by this layout code, the sole consumer. +static constexpr qreal MINIMUM_STACKING_HEIGHT = 50.0; + PlayerGraphicsItem::PlayerGraphicsItem(PlayerLogic *_player) : player(_player) { connect(&SettingsCache::instance(), &SettingsCache::horizontalHandChanged, this, @@ -278,8 +282,8 @@ void PlayerGraphicsItem::rearrangeZones() qreal stackHeight = tableHeight; if (commandZoneVisible) { stackHeight = tableHeight - totalCommandZoneHeight(); - if (stackHeight < CommandZone::MINIMUM_STACKING_HEIGHT) { - stackHeight = CommandZone::MINIMUM_STACKING_HEIGHT; + if (stackHeight < MINIMUM_STACKING_HEIGHT) { + stackHeight = MINIMUM_STACKING_HEIGHT; } } stackZoneGraphicsItem->setHeight(stackHeight); diff --git a/cockatrice/src/game_graphics/zones/command_zone.h b/cockatrice/src/game_graphics/zones/command_zone.h index 12d604b17..4595e7445 100644 --- a/cockatrice/src/game_graphics/zones/command_zone.h +++ b/cockatrice/src/game_graphics/zones/command_zone.h @@ -47,9 +47,6 @@ constexpr qreal COMMAND_ZONE_WIDTH = CardDimensions::WIDTH_F * 1.5; class CommandZone : public SelectZone { Q_OBJECT -public: - static constexpr qreal MINIMUM_STACKING_HEIGHT = 50.0; - private: static constexpr double MINIMIZED_HEIGHT_RATIO = 0.25; int zoneHeight; ///< Full height in pixels when expanded 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 b3f1245b6..78d9204a8 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp @@ -441,10 +441,33 @@ Server_Player::cmdUndoDraw(const Command_UndoDraw & /*cmd*/, ResponseContainer & return retVal; } -bool Server_Player::isCommandZoneCounterBlocked(int counterId) const +Response::ResponseCode Server_Player::evaluateModifyCounter(bool gameStarted, + bool playerConceded, + bool commandZoneEnabled, + int counterId, + const Server_Counter *counter) { - return (counterId == CounterIds::CommanderTax || counterId == CounterIds::PartnerTax) && - !game->getEnableCommandZone(); + if (!gameStarted) { + return Response::RespGameNotStarted; + } + if (playerConceded) { + return Response::RespContextError; + } + if (CounterIds::isTaxCounter(counterId)) { + // Tax counters are server-managed: they only exist in Commander games, and an + // inactive (hidden) tax counter must stay at zero. Block modification in either + // case so the value can never diverge from what players can see. + if (!commandZoneEnabled) { + return Response::RespContextError; + } + if (counter && !counter->isActive()) { + return Response::RespContextError; + } + } + if (!counter) { + return Response::RespNameNotFound; + } + return Response::RespOk; } Response::ResponseCode @@ -458,14 +481,12 @@ Server_Player::cmdIncCounter(const Command_IncCounter &cmd, ResponseContainer & } const int counterId = cmd.counter_id(); - - if (isCommandZoneCounterBlocked(counterId)) { - return Response::RespContextError; - } - Server_Counter *c = counters.value(counterId, nullptr); - if (!c) { - return Response::RespNameNotFound; + + const Response::ResponseCode authResult = + evaluateModifyCounter(game->getGameStarted(), conceded, game->getEnableCommandZone(), counterId, c); + if (authResult != Response::RespOk) { + return authResult; } bool didChange = c->incrementCount(cmd.delta()); @@ -489,8 +510,15 @@ Server_Player::cmdCreateCounter(const Command_CreateCounter &cmd, ResponseContai return Response::RespContextError; } - auto *c = new Server_Counter(newCounterId(), nameFromStdString(cmd.counter_name()), cmd.counter_color(), - cmd.radius(), cmd.value()); + const QString counterName = nameFromStdString(cmd.counter_name()); + // Reserved system counter names (commander/partner tax) are how clients identify + // server-managed tax counters for rendering and logging; a client must not be able + // to spoof one via a user-created counter. + if (CounterNames::isTaxCounter(counterName)) { + return Response::RespFunctionNotAllowed; + } + + auto *c = new Server_Counter(newCounterId(), counterName, cmd.counter_color(), cmd.radius(), cmd.value()); addCounter(c); Event_CreateCounter event; @@ -516,14 +544,12 @@ Server_Player::cmdSetCounter(const Command_SetCounter &cmd, ResponseContainer & } const int counterId = cmd.counter_id(); - - if (isCommandZoneCounterBlocked(counterId)) { - return Response::RespContextError; - } - Server_Counter *c = counters.value(counterId, nullptr); - if (!c) { - return Response::RespNameNotFound; + + const Response::ResponseCode authResult = + evaluateModifyCounter(game->getGameStarted(), conceded, game->getEnableCommandZone(), counterId, c); + if (authResult != Response::RespOk) { + return authResult; } bool didChange = c->setCount(cmd.value()); 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 608f85b8b..b9d1daa24 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.h @@ -9,7 +9,6 @@ class Server_Player : public Server_AbstractPlayer private: QMap counters; QList lastDrawList; - bool isCommandZoneCounterBlocked(int counterId) const; public: Server_Player(Server_Game *_game, @@ -37,6 +36,14 @@ public: int counterId, const Server_Counter *counter, bool requestedActive); + // Authorization shared by cmdIncCounter and cmdSetCounter. Reserved tax counters + // may only be modified inside a Commander game and only while active, so that an + // inactive (hidden) tax counter can never accumulate a value behind the scenes. + static Response::ResponseCode evaluateModifyCounter(bool gameStarted, + bool playerConceded, + bool commandZoneEnabled, + int counterId, + const Server_Counter *counter); void setupZones() override; void clearZones() override; diff --git a/tests/command_zone_tests/counter_command_auth_test.cpp b/tests/command_zone_tests/counter_command_auth_test.cpp index 0b93e1ddf..9dba0763b 100644 --- a/tests/command_zone_tests/counter_command_auth_test.cpp +++ b/tests/command_zone_tests/counter_command_auth_test.cpp @@ -1,7 +1,8 @@ // 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. +// handlers. Server_Player::evaluateDelCounter(), evaluateSetCounterActive() and +// evaluateModifyCounter() 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" @@ -158,6 +159,89 @@ TEST(EvaluateSetCounterActive, AllowsDisablingWhenCounterIsZero) Response::RespOk); } +// PartnerTax is the second reserved tax counter and must flow through the same auth +// path as CommanderTax: enabling is always permitted... +TEST(EvaluateSetCounterActive, AllowsEnablingPartnerTax) +{ + Server_Counter counter = makeCounter(CounterIds::PartnerTax, 0); + EXPECT_EQ(Server_Player::evaluateSetCounterActive(true, false, true, CounterIds::PartnerTax, &counter, + /*requestedActive=*/true), + Response::RespOk); +} + +// ...and disabling is rejected while it still holds accumulated tax. +TEST(EvaluateSetCounterActive, RejectsDisablingPartnerTaxWhenAccumulated) +{ + Server_Counter counter = makeCounter(CounterIds::PartnerTax, 2); + EXPECT_EQ(Server_Player::evaluateSetCounterActive(true, false, true, CounterIds::PartnerTax, &counter, + /*requestedActive=*/false), + Response::RespContextError); +} + +// --------------------------------------------------------------------------- +// evaluateModifyCounter (shared by cmdIncCounter / cmdSetCounter) +// --------------------------------------------------------------------------- + +// No counter may be modified before the game starts. +TEST(EvaluateModifyCounter, RejectsWhenGameNotStarted) +{ + Server_Counter counter = makeCounter(UserCounterId, 0); + EXPECT_EQ(Server_Player::evaluateModifyCounter(/*gameStarted=*/false, false, /*commandZoneEnabled=*/true, + UserCounterId, &counter), + Response::RespGameNotStarted); +} + +// A conceded player may no longer modify counters. +TEST(EvaluateModifyCounter, RejectsWhenPlayerConceded) +{ + Server_Counter counter = makeCounter(UserCounterId, 0); + EXPECT_EQ(Server_Player::evaluateModifyCounter(true, /*playerConceded=*/true, true, UserCounterId, &counter), + Response::RespContextError); +} + +// An ordinary user counter can be modified regardless of command-zone state. +TEST(EvaluateModifyCounter, AllowsUserCounter) +{ + Server_Counter counter = makeCounter(UserCounterId, 0); + EXPECT_EQ(Server_Player::evaluateModifyCounter(true, false, /*commandZoneEnabled=*/false, UserCounterId, &counter), + Response::RespOk); +} + +// A non-existent counter is reported as not found. +TEST(EvaluateModifyCounter, RejectsMissingCounter) +{ + EXPECT_EQ(Server_Player::evaluateModifyCounter(true, false, true, UserCounterId, nullptr), + Response::RespNameNotFound); +} + +// A tax counter may not be modified outside a Commander game (command zone disabled). +TEST(EvaluateModifyCounter, RejectsTaxCounterWhenCommandZoneDisabled) +{ + Server_Counter counter = makeCounter(CounterIds::CommanderTax, 0); + EXPECT_EQ(Server_Player::evaluateModifyCounter(true, false, /*commandZoneEnabled=*/false, CounterIds::CommanderTax, + &counter), + Response::RespContextError); +} + +// An inactive (hidden) tax counter must not accumulate value behind the scenes. +TEST(EvaluateModifyCounter, RejectsInactiveTaxCounter) +{ + Server_Counter counter = makeCounter(CounterIds::PartnerTax, 0); + (void)counter.setActive(false); + EXPECT_EQ(Server_Player::evaluateModifyCounter(true, false, /*commandZoneEnabled=*/true, CounterIds::PartnerTax, + &counter), + Response::RespContextError); +} + +// An active tax counter in a Commander game may be modified. +TEST(EvaluateModifyCounter, AllowsActiveTaxCounter) +{ + Server_Counter counter = makeCounter(CounterIds::CommanderTax, 0); + EXPECT_EQ(Server_Player::evaluateModifyCounter(true, false, /*commandZoneEnabled=*/true, CounterIds::CommanderTax, + &counter), + Response::RespOk); +} + int main(int argc, char **argv) { ::testing::InitGoogleTest(&argc, argv); diff --git a/tests/command_zone_tests/new_counter_id_test.cpp b/tests/command_zone_tests/new_counter_id_test.cpp index 679ee7062..210cf5e39 100644 --- a/tests/command_zone_tests/new_counter_id_test.cpp +++ b/tests/command_zone_tests/new_counter_id_test.cpp @@ -41,13 +41,15 @@ TEST(NewCounterId, ReturnsFirstUserIdWhenNoCounters) 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. +// Reserved ids must not drag a new user counter down into the reserved range. The +// ids here (3 and 5) are chosen so a naive "highest id + 1" implementation would +// return 6 (inside the reserved range) and fail this test; only the FirstUserId +// floor yields the correct 10, so this pins the floor against regression. 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)); + f.player.addCounter(new Server_Counter(5, "h", color(), 20, 0)); EXPECT_EQ(f.player.newCounterId(), CounterIds::FirstUserId); }