jeirmeister/Cockatrice
1
0
Fork 0
mirror of https://github.com/Cockatrice/Cockatrice.git synced 2026-09-21 09:05:10 -07:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

[Server/Client/Protocol] Address developer role review feedback

Browse source 
Address ZeizaZach's review of the developer staff role:

- Nudge the developer log query to exclude private chat and sender IPs
  (the ModeratorCommand path still sees everything).
- Deduplicate Command_GetLogHistory into Command_ViewLogHistory, which now
  extends both ModeratorCommand (ext) and DeveloperCommand (dev_ext); the
  client picks the DeveloperCommand-scoped extension by extendee, and the
  server reads it via the extension number.
- Pull the uptime snapshot SQL into Servatrice_DatabaseInterface as
  getLatestUptimeSnapshot() and widen the reported counters to 64-bit.
- Document the admin bitfield (1 admin, 2 moderator, 4 judge, 8 developer)
  and add a server-side test for the developer command path.
This commit is contained in:
Lukas Brübach 2026-08-30 21:06:31 +02:00
parent 1ff7ffcaa4
commit 1dc20176b3
14 changed files with 251 additions and 76 deletions
Download patch file Download diff file Expand all files Collapse all files

2
cockatrice/src/interface/widgets/tabs/tab_developer.cpp
View file

@ -94,7 +94,7 @@ void TabDeveloper::refreshClicked()
void TabDeveloper::serverStatsResponse(const Response &resp)
{
if (resp.response_code() != Response::RespOk) {
statusLabel->setText(tr("Failed to collect server statistics."));
statusLabel->setText(tr("No server statistics available yet."));
return;
}

22
cockatrice/src/interface/widgets/tabs/tab_logs.cpp
View file

@ -14,7 +14,6 @@
#include <QTabWidget>
#include <QTableWidget>
#include <libcockatrice/network/client/abstract/abstract_client.h>
#include <libcockatrice/protocol/pb/command_get_log_history.pb.h>
#include <libcockatrice/protocol/pb/moderator_commands.pb.h>
#include <libcockatrice/protocol/pb/response_viewlog_history.pb.h>
#include <libcockatrice/protocol/pending_command.h>
@ -82,7 +81,9 @@ void TabLog::getClicked()
if (!mainRoom->isChecked() && !gameRoom->isChecked() && !privateChat->isChecked()) {
mainRoom->setChecked(true);
gameRoom->setChecked(true);
privateChat->setChecked(true);
if (!canUseDeveloperCommands) {
privateChat->setChecked(true);
}
}
if (maximumResults->value() == 0) {
@ -123,18 +124,7 @@ void TabLog::getClicked()
PendingCommand *pend;
if (canUseDeveloperCommands) {
// Developers query logs through the developer command family.
Command_GetLogHistory devCmd;
devCmd.set_user_name(cmd.user_name());
devCmd.set_ip_address(cmd.ip_address());
devCmd.set_game_name(cmd.game_name());
devCmd.set_game_id(cmd.game_id());
devCmd.set_message(cmd.message());
for (int i = 0; i < cmd.log_location_size(); ++i) {
devCmd.add_log_location(cmd.log_location(i));
}
devCmd.set_date_range(cmd.date_range());
devCmd.set_maximum_results(cmd.maximum_results());
pend = client->prepareDeveloperCommand(devCmd);
pend = client->prepareDeveloperCommand(cmd);
} else {
pend = client->prepareModeratorCommand(cmd);
}
@ -192,6 +182,10 @@ void TabLog::createDock()
mainRoom = new QCheckBox(tr("Main Room"));
gameRoom = new QCheckBox(tr("Game Room"));
privateChat = new QCheckBox(tr("Private Chat"));
if (canUseDeveloperCommands) {
// Developers cannot query private conversations.
privateChat->setVisible(false);
}
pastDays = new QRadioButton(tr("Past X Days: "));
today = new QRadioButton(tr("Today"));

8
cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp
View file

@ -844,10 +844,10 @@ void TabSupervisor::actTabLog(bool checked)
void TabSupervisor::openTabLog()
{
// Developers without moderation rights query logs through the developer
// command family, so tell the tab which family to use.
const bool isDeveloper = (userInfo->user_level() & ServerInfo_User::IsDeveloper) != 0;
tabLog = new TabLog(this, client, isDeveloper);
// Developers query logs through the developer command family, so tell the
// tab which family to use.
const bool useDeveloperCommands = (userInfo->user_level() & ServerInfo_User::IsDeveloper) != 0;
tabLog = new TabLog(this, client, useDeveloperCommands);
myAddTab(tabLog, aTabLog);
connect(tabLog, &QObject::destroyed, this, [this] {
tabLog = nullptr;

15
libcockatrice_network/libcockatrice/network/client/abstract/abstract_client.cpp
View file

@ -258,6 +258,19 @@ PendingCommand *AbstractClient::prepareDeveloperCommand(const ::google::protobuf
{
CommandContainer cont;
DeveloperCommand *c = cont.add_developer_command();
c->GetReflection()->MutableMessage(c, cmd.GetDescriptor()->FindExtensionByName("ext"))->CopyFrom(cmd);
// A developer command message may also be usable through other command
// families, so select the extension scoped to DeveloperCommand rather than
// guessing by name.
const ::google::protobuf::Descriptor *cmdDescriptor = cmd.GetDescriptor();
const ::google::protobuf::Descriptor *developerDescriptor = DeveloperCommand::descriptor();
const ::google::protobuf::FieldDescriptor *developerExtension = nullptr;
for (int i = 0; i < cmdDescriptor->extension_count(); ++i) {
if (cmdDescriptor->extension(i)->containing_type() == developerDescriptor) {
developerExtension = cmdDescriptor->extension(i);
break;
}
}
Q_ASSERT(developerExtension != nullptr);
c->GetReflection()->MutableMessage(c, developerExtension)->CopyFrom(cmd);
return new PendingCommand(cont);
}

3
libcockatrice_protocol/libcockatrice/protocol/pb/CMakeLists.txt
View file

@ -22,11 +22,10 @@ set(PROTO_FILES
command_del_counter.proto
command_delete_arrow.proto
command_draw_cards.proto
command_get_log_history.proto
command_get_server_stats.proto
command_dump_zone.proto
command_flip_card.proto
command_game_say.proto
command_get_server_stats.proto
command_inc_card_counter.proto
command_inc_counter.proto
command_kick_from_game.proto

19
libcockatrice_protocol/libcockatrice/protocol/pb/command_get_log_history.proto
View file

@ -1,19 +0,0 @@
syntax = "proto2";
import "developer_commands.proto";
// Developer counterpart of Command_ViewLogHistory: identical query fields, but
// routed through the developer command family so developers never need the
// moderator command container.
message Command_GetLogHistory {
extend DeveloperCommand {
optional Command_GetLogHistory ext = 1001;
}
optional string user_name = 1; // user that created message
optional string ip_address = 2; // ip address of user that created message
optional string game_name = 3; // client id of user that created the message
optional string game_id = 4; // game number the message was sent to
optional string message = 5; // raw message that was sent
repeated string log_location = 6; // destination of message (ex: main room, game room, private chat)
required uint32 date_range = 7; // the length of time (in minutes) to look back for
optional uint32 maximum_results = 8; // the maximum number of query results
}

4
libcockatrice_protocol/libcockatrice/protocol/pb/moderator_commands.proto
View file

@ -1,4 +1,5 @@
syntax = "proto2";
import "developer_commands.proto";
message ModeratorCommand {
enum ModeratorCommandType {
BAN_FROM_SERVER = 1000;
@ -80,6 +81,9 @@ message Command_ViewLogHistory {
extend ModeratorCommand {
optional Command_ViewLogHistory ext = 1005;
}
extend DeveloperCommand {
optional Command_ViewLogHistory dev_ext = 1001;
}
optional string user_name = 1; // user that created message
optional string ip_address = 2; // ip address of user that created message
optional string game_name = 3; // client id of user that created the message

3
servatrice/servatrice.sql
View file

@ -25,6 +25,9 @@ INSERT INTO cockatrice_schema_version VALUES(36);
-- users and user data tables
CREATE TABLE IF NOT EXISTS `cockatrice_users` (
`id` int(7) unsigned zerofill NOT NULL auto_increment,
-- Bitfield of staff levels: 1 = admin (implies moderator), 2 = moderator,
-- 4 = judge, 8 = developer. Operators set these by hand with
-- "UPDATE cockatrice_users SET admin = ...".
`admin` tinyint(1) NOT NULL,
`name` varchar(35) NOT NULL,
`realname` varchar(255) NOT NULL,

32
servatrice/src/servatrice_database_interface.cpp
View file

@ -1487,6 +1487,38 @@ QList<ServerInfo_ModeratorLogin> Servatrice_DatabaseInterface::getModeratorLastL
return results;
}
Servatrice_DatabaseInterface::UptimeSnapshot Servatrice_DatabaseInterface::getLatestUptimeSnapshot(int serverId)
{
UptimeSnapshot snapshot;
if (!checkSql()) {
return snapshot;
}
QSqlQuery *query = prepareQuery("SELECT users_count, mods_count, games_count, tx_bytes, rx_bytes, uptime, "
"UNIX_TIMESTAMP(timest) FROM {prefix}_uptime "
"WHERE id_server = :id_server ORDER BY timest DESC LIMIT 1");
query->bindValue(":id_server", serverId);
if (!execSqlQuery(query)) {
qCWarning(DatabaseInterfaceLog) << "Failed to collect server stats snapshot: SQL Error";
return snapshot;
}
if (query->next()) {
snapshot.valid = true;
snapshot.usersCount = query->value(0).toULongLong();
snapshot.modsCount = query->value(1).toULongLong();
snapshot.gamesCount = query->value(2).toULongLong();
snapshot.txBytes = query->value(3).toULongLong();
snapshot.rxBytes = query->value(4).toULongLong();
snapshot.uptimeSecs = query->value(5).toULongLong();
snapshot.timest = query->value(6).toULongLong();
}
return snapshot;
}
bool Servatrice_DatabaseInterface::removeUserAvatar(const QString &userName)
{
if (!checkSql()) {

15
servatrice/src/servatrice_database_interface.h
View file

@ -140,6 +140,21 @@ public:
QList<ServerInfo_UserSession> getUserSessions(const QString &userName, int limit);
QList<ServerInfo_UserAlt> getUserAlts(const QString &userName);
QList<ServerInfo_ModeratorLogin> getModeratorLastLogins();
// Uptime snapshot as recorded by Servatrice::statusUpdate() into the
// {prefix}_uptime table. valid is false when no snapshot exists yet.
struct UptimeSnapshot
{
bool valid = false;
quint64 usersCount = 0;
quint64 modsCount = 0;
quint64 gamesCount = 0;
quint64 txBytes = 0;
quint64 rxBytes = 0;
quint64 uptimeSecs = 0;
quint64 timest = 0;
};
UptimeSnapshot getLatestUptimeSnapshot(int serverId);
bool removeUserAvatar(const QString &userName);
bool addForgotPassword(const QString &user);
bool removeForgotPassword(const QString &user) override;

57
servatrice/src/serversocketinterface.cpp
View file

@ -287,7 +287,7 @@ Response::ResponseCode AbstractServerSocketInterface::processExtendedModeratorCo
case ModeratorCommand::REPORT_RESOLVE:
return cmdReportResolve(cmd.GetExtension(Command_ReportResolve::ext), rc);
case ModeratorCommand::VIEWLOG_HISTORY:
return cmdGetLogHistory(cmd.GetExtension(Command_ViewLogHistory::ext), rc);
return cmdGetLogHistory(cmd.GetExtension(Command_ViewLogHistory::ext), rc, true);
case ModeratorCommand::GRANT_REPLAY_ACCESS:
return cmdGrantReplayAccess(cmd.GetExtension(Command_GrantReplayAccess::ext), rc);
case ModeratorCommand::REPLAY_DOWNLOAD_BY_GAME_ID:
@ -351,21 +351,9 @@ Response::ResponseCode AbstractServerSocketInterface::processExtendedDeveloperCo
case DeveloperCommand::GET_SERVER_STATS:
return cmdGetServerStats(cmd.GetExtension(Command_GetServerStats::ext), rc);
case DeveloperCommand::VIEWLOG_HISTORY: {
// Same query as the moderator log view, just carried by the
// developer command family.
const Command_GetLogHistory &devCmd = cmd.GetExtension(Command_GetLogHistory::ext);
Command_ViewLogHistory modCmd;
modCmd.set_user_name(devCmd.user_name());
modCmd.set_ip_address(devCmd.ip_address());
modCmd.set_game_name(devCmd.game_name());
modCmd.set_game_id(devCmd.game_id());
modCmd.set_message(devCmd.message());
for (int i = 0; i < devCmd.log_location_size(); ++i) {
modCmd.add_log_location(devCmd.log_location(i));
}
modCmd.set_date_range(devCmd.date_range());
modCmd.set_maximum_results(devCmd.maximum_results());
return cmdGetLogHistory(modCmd, rc);
// Same query as the moderator log view, carried by the developer
// command family, but narrows out private chats and sender IPs.
return cmdGetLogHistory(cmd.GetExtension(Command_ViewLogHistory::dev_ext), rc, false);
}
default:
return Response::RespFunctionNotAllowed;
@ -1055,12 +1043,13 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReplaySubmitCode(const
// MODERATOR FUNCTIONS.
// May be called by admins and moderators. Permission is checked by the calling function.
Response::ResponseCode AbstractServerSocketInterface::cmdGetLogHistory(const Command_ViewLogHistory &cmd,
ResponseContainer &rc)
ResponseContainer &rc,
bool allowPrivateChat)
{
QList<ServerInfo_ChatMessage> messageList;
QString userName = nameFromStdString(cmd.user_name());
QString ipAddress = nameFromStdString(cmd.ip_address());
QString ipAddress = allowPrivateChat ? nameFromStdString(cmd.ip_address()) : QString();
QString gameName = nameFromStdString(cmd.game_name());
QString gameID = nameFromStdString(cmd.game_id());
QString message = textFromStdString(cmd.message());
@ -1075,7 +1064,7 @@ Response::ResponseCode AbstractServerSocketInterface::cmdGetLogHistory(const Com
if (nameFromStdString(cmd.log_location(i)).simplified() == "game") {
gameType = true;
}
if (nameFromStdString(cmd.log_location(i)).simplified() == "chat") {
if (nameFromStdString(cmd.log_location(i)).simplified() == "chat" && allowPrivateChat) {
chatType = true;
}
}
@ -1089,7 +1078,11 @@ Response::ResponseCode AbstractServerSocketInterface::cmdGetLogHistory(const Com
QListIterator<ServerInfo_ChatMessage> messageIterator(sqlInterface->getMessageLogHistory(
userName, ipAddress, gameName, gameID, message, chatType, gameType, roomType, dateRange, maximumResults));
while (messageIterator.hasNext()) {
re->add_log_message()->CopyFrom(messageIterator.next());
ServerInfo_ChatMessage chatMessage = messageIterator.next();
if (!allowPrivateChat) {
chatMessage.clear_sender_ip();
}
re->add_log_message()->CopyFrom(chatMessage);
}
} else {
ServerInfo_ChatMessage chatMessage;
@ -1687,24 +1680,20 @@ Response::ResponseCode AbstractServerSocketInterface::cmdGetServerStats(const Co
// Servatrice::statusUpdate() periodically snapshots server health into the
// uptime table. Serve the freshest snapshot for this server.
QSqlQuery *query = sqlInterface->prepareQuery(
"SELECT users_count, mods_count, games_count, tx_bytes, rx_bytes, uptime, UNIX_TIMESTAMP(timest) "
"FROM {prefix}_uptime WHERE id_server = :id_server ORDER BY timest DESC LIMIT 1");
query->bindValue(":id_server", servatrice->getServerID());
if (!sqlInterface->execSqlQuery(query)) {
const auto snapshot = sqlInterface->getLatestUptimeSnapshot(servatrice->getServerID());
if (!snapshot.valid) {
// No snapshot yet (fresh server, or statusUpdate() has not ticked).
return Response::RespInternalError;
}
auto *re = new Response_GetServerStats;
if (query->next()) {
re->set_users_count(query->value(0).toUInt());
re->set_mods_count(query->value(1).toUInt());
re->set_games_count(query->value(2).toUInt());
re->set_tx_bytes(query->value(3).toUInt());
re->set_rx_bytes(query->value(4).toUInt());
re->set_uptime_secs(query->value(5).toUInt());
re->set_timest(query->value(6).toUInt());
}
re->set_users_count(snapshot.usersCount);
re->set_mods_count(snapshot.modsCount);
re->set_games_count(snapshot.gamesCount);
re->set_tx_bytes(snapshot.txBytes);
re->set_rx_bytes(snapshot.rxBytes);
re->set_uptime_secs(snapshot.uptimeSecs);
re->set_timest(snapshot.timest);
rc.setResponseExtension(re);
return Response::RespOk;

3
servatrice/src/serversocketinterface.h
View file

@ -117,7 +117,8 @@ private:
Response::ResponseCode cmdBanFromServer(const Command_BanFromServer &cmd, ResponseContainer &rc);
Response::ResponseCode cmdReportList(const Command_ReportList &cmd, ResponseContainer &rc);
Response::ResponseCode cmdWarnUser(const Command_WarnUser &cmd, ResponseContainer &rc);
Response::ResponseCode cmdGetLogHistory(const Command_ViewLogHistory &cmd, ResponseContainer &rc);
Response::ResponseCode
cmdGetLogHistory(const Command_ViewLogHistory &cmd, ResponseContainer &rc, bool allowPrivateChat);
Response::ResponseCode cmdGetBanHistory(const Command_GetBanHistory &cmd, ResponseContainer &rc);
Response::ResponseCode cmdGetWarnList(const Command_GetWarnList &cmd, ResponseContainer &rc);
Response::ResponseCode cmdGetWarnHistory(const Command_GetWarnHistory &cmd, ResponseContainer &rc);

7
tests/CMakeLists.txt
View file

@ -11,6 +11,7 @@ add_test(NAME playmat_resolver_test COMMAND playmat_resolver_test)
add_test(NAME server_card_counter_test COMMAND server_card_counter_test)
add_test(NAME server_counter_test COMMAND server_counter_test)
add_test(NAME server_rate_limiter_test COMMAND server_rate_limiter_test)
add_test(NAME server_developer_role_test COMMAND server_developer_role_test)
add_test(NAME warning_categories_test COMMAND warning_categories_test)
add_test(NAME lag_monitor_test COMMAND lag_monitor_test)
add_test(NAME latency_tracker_test COMMAND latency_tracker_test)
@ -30,6 +31,7 @@ add_executable(deck_hash_performance_test deck_hash_performance_test.cpp)
add_executable(server_card_counter_test server_card_counter_test.cpp)
add_executable(server_counter_test server_counter_test.cpp)
add_executable(server_rate_limiter_test server_rate_limiter_test.cpp)
add_executable(server_developer_role_test server_developer_role_test.cpp)
add_executable(warning_categories_test warning_categories_test.cpp)
add_executable(lag_monitor_test ${CMAKE_SOURCE_DIR}/cockatrice/src/client/lag_monitor.cpp lag_monitor_test.cpp)
target_include_directories(lag_monitor_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src)
@ -70,6 +72,7 @@ if(NOT GTEST_FOUND)
add_dependencies(server_card_counter_test gtest)
add_dependencies(server_counter_test gtest)
add_dependencies(server_rate_limiter_test gtest)
add_dependencies(server_developer_role_test gtest)
add_dependencies(warning_categories_test gtest)
add_dependencies(lag_monitor_test gtest)
add_dependencies(latency_tracker_test gtest)
@ -104,6 +107,10 @@ target_link_libraries(
target_link_libraries(
server_rate_limiter_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}
)
target_link_libraries(
server_developer_role_test libcockatrice_network libcockatrice_rng Threads::Threads ${GTEST_BOTH_LIBRARIES}
${TEST_QT_MODULES}
)
target_link_libraries(
warning_categories_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}
)

137
tests/server_developer_role_test.cpp Normal file
View file

@ -0,0 +1,137 @@
/** @file server_developer_role_test.cpp
* @brief Tests for the developer staff role authorization and dispatch.
* @ingroup Tests
*/
#include <gtest/gtest.h>
#include <libcockatrice/network/server/remote/server.h>
#include <libcockatrice/network/server/remote/server_protocolhandler.h>
#include <libcockatrice/protocol/pb/command_get_server_stats.pb.h>
#include <libcockatrice/protocol/pb/commands.pb.h>
#include <libcockatrice/protocol/pb/developer_commands.pb.h>
#include <libcockatrice/protocol/pb/serverinfo_user.pb.h>
#include <libcockatrice/rng/rng_abstract.h>
// The server_remote library references the global RNG, which is normally
// defined by the servatrice/client executable main(). Provide a stub so the
// unit test can link against it.
RNG_Abstract *rng = nullptr;
namespace
{
class TestDeveloperHandler : public Server_ProtocolHandler
{
public:
explicit TestDeveloperHandler(Server *_server) : Server_ProtocolHandler(_server, nullptr)
{
}
QString getAddress() const override
{
return {};
}
QString getConnectionType() const override
{
return {};
}
// Buffer the last response code sent to the client so tests can assert on
// the outcome of processCommandContainer().
Response::ResponseCode lastResponseCode = Response::RespNothing;
int dispatchCount = 0;
protected:
void transmitProtocolItem(const ServerMessage &item) override
{
if (item.message_type() == ServerMessage::RESPONSE) {
lastResponseCode = item.response().response_code();
}
}
Response::ResponseCode
processExtendedDeveloperCommand(int cmdType, const DeveloperCommand &, ResponseContainer &) override
{
++dispatchCount;
// Fail closed for anything not explicitly handled.
if (cmdType != DeveloperCommand::GET_SERVER_STATS) {
return Response::RespFunctionNotAllowed;
}
return Response::RespOk;
}
};
class DeveloperRoleTest : public ::testing::Test
{
protected:
Server server;
TestDeveloperHandler handler{&server};
void setUserLevel(uint32_t level)
{
ServerInfo_User user;
user.set_user_level(level);
handler.setUserInfo(user);
}
};
TEST_F(DeveloperRoleTest, RejectsWhenNotLoggedIn)
{
CommandContainer cont;
cont.add_developer_command();
handler.processCommandContainer(cont);
EXPECT_EQ(handler.lastResponseCode, Response::RespLoginNeeded);
EXPECT_EQ(handler.dispatchCount, 0);
}
TEST_F(DeveloperRoleTest, RejectsPlainUser)
{
setUserLevel(ServerInfo_User::IsUser | ServerInfo_User::IsRegistered);
CommandContainer cont;
cont.add_developer_command();
handler.processCommandContainer(cont);
EXPECT_EQ(handler.lastResponseCode, Response::RespLoginNeeded);
EXPECT_EQ(handler.dispatchCount, 0);
}
TEST_F(DeveloperRoleTest, RejectsModeratorThatIsNotDeveloper)
{
setUserLevel(ServerInfo_User::IsModerator);
CommandContainer cont;
cont.add_developer_command();
handler.processCommandContainer(cont);
EXPECT_EQ(handler.lastResponseCode, Response::RespLoginNeeded);
}
TEST_F(DeveloperRoleTest, DispatchesToDeveloperCommandForDeveloper)
{
setUserLevel(ServerInfo_User::IsDeveloper);
CommandContainer cont;
DeveloperCommand *cmd = cont.add_developer_command();
cmd->MutableExtension(Command_GetServerStats::ext);
handler.processCommandContainer(cont);
EXPECT_EQ(handler.lastResponseCode, Response::RespOk);
EXPECT_EQ(handler.dispatchCount, 1);
}
TEST_F(DeveloperRoleTest, FailClosedForUnknownDeveloperCommand)
{
setUserLevel(ServerInfo_User::IsDeveloper);
CommandContainer cont;
cont.add_developer_command(); // no extension set -> getPbExtension() returns -1
handler.processCommandContainer(cont);
EXPECT_EQ(handler.lastResponseCode, Response::RespFunctionNotAllowed);
EXPECT_EQ(handler.dispatchCount, 1);
}
} // namespace
int main(int argc, char **argv)
{
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
}