From 50a2ae9cc8d56f2014479e88dcb0b25dfea6bb7d Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Mon, 2 Nov 2026 07:01:29 +0100 Subject: [PATCH] [Server/Client] Enforce and complete the forced password change at login (#7680) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit checkUserPassword ignored force_password_change on the challenge-response path, so a reset password stayed valid. Returning PasswordChangeRequired was not enough on its own: the session was dropped and the client told the user to ask the administrator for the temporary password they had just used. The pending session now keeps its identity without registering a user session or announcing a join, may only ping and change its password, and the client completes the change in-session before reconnecting. Co-authored-by: Lukas Brübach --- .../remote_connection_controller.cpp | 56 ++++- .../remote_connection_controller.h | 4 + .../widgets/server/user/user_info_box.cpp | 13 +- .../network/client/abstract/CMakeLists.txt | 2 +- .../client/abstract/abstract_client.cpp | 21 ++ .../network/client/abstract/abstract_client.h | 14 ++ .../network/client/remote/remote_client.cpp | 41 +++ .../network/client/remote/remote_client.h | 11 + .../network/server/remote/server.cpp | 28 ++- .../server/remote/server_protocolhandler.cpp | 28 ++- .../server/remote/server_protocolhandler.h | 6 + .../src/servatrice_database_interface.cpp | 4 + servatrice/src/serversocketinterface.cpp | 3 + tests/CMakeLists.txt | 22 ++ tests/client_login_status_test.cpp | 116 +++++++++ tests/server_forced_password_change_test.cpp | 234 ++++++++++++++++++ 16 files changed, 575 insertions(+), 28 deletions(-) create mode 100644 tests/client_login_status_test.cpp create mode 100644 tests/server_forced_password_change_test.cpp diff --git a/cockatrice/src/client/network/connection_controller/remote_connection_controller.cpp b/cockatrice/src/client/network/connection_controller/remote_connection_controller.cpp index 020e53877..c702f91f1 100644 --- a/cockatrice/src/client/network/connection_controller/remote_connection_controller.cpp +++ b/cockatrice/src/client/network/connection_controller/remote_connection_controller.cpp @@ -3,6 +3,7 @@ #include "../../../interface/pixel_map_generator.h" #include "../../settings/cache_settings.h" #include "../interface/widgets/dialogs/dlg_connect.h" +#include "../interface/widgets/dialogs/dlg_edit_password.h" #include "../interface/widgets/dialogs/dlg_forgot_password_challenge.h" #include "../interface/widgets/dialogs/dlg_forgot_password_request.h" #include "../interface/widgets/dialogs/dlg_forgot_password_reset.h" @@ -99,6 +100,10 @@ void ConnectionController::wireClientSignals() connect(remoteClient, &RemoteClient::sigPasswordVerifierReady, this, &ConnectionController::onPasswordVerifierReady); + + connect(remoteClient, &RemoteClient::sigPasswordChangeSuccess, this, + &ConnectionController::onPasswordChangeSuccess); + connect(remoteClient, &RemoteClient::sigPasswordChangeError, this, &ConnectionController::onPasswordChangeError); } void ConnectionController::connectToServer() @@ -351,10 +356,16 @@ void ConnectionController::onLoginError(int r, } case Response::RespPasswordChangeRequired: { - QMessageBox::information( - dialogParent, tr("Password Change Required"), - tr("An administrator has reset your password. Please contact your server administrator to obtain " - "your temporary password, then log in and change it via Account -> Change Password.")); + // The credentials were right, but the account may not be used until a new password + // is chosen. Let the user complete the change here: sending them off to "ask the + // administrator for your temporary password" sends them in circles, since they just + // logged in with it and the session ends without a way to comply. + DlgEditPassword dlg(dialogParent); + dlg.setWindowTitle(tr("Password Change Required")); + if (dlg.exec()) { + remoteClient->changePasswordToServer(dlg.getOldPassword(), dlg.getNewPassword()); + return; // onPasswordChangeSuccess/onPasswordChangeError decides the next step + } remoteClient->disconnectFromServer(); return; } @@ -517,6 +528,43 @@ void ConnectionController::onNotifyUserAboutUpdate() "To update your client, go to Help -> Check for Updates.")); } +void ConnectionController::onPasswordChangeSuccess() +{ + // The credential saved for this profile is stale now: drop it so the next login asks for + // the new password instead of replaying the old verifier. + if (!pendingSaveName.isEmpty()) { + SettingsCache::instance().servers().setServerPassword(pendingSaveName, ""); + } + + QMessageBox::information(dialogParent, tr("Password Changed"), + tr("Your password has been changed. Please log in again with your new password.")); + remoteClient->disconnectFromServer(); + connectToServer(); +} + +void ConnectionController::onPasswordChangeError(int responseCode) +{ + QString reason; + switch (static_cast(responseCode)) { + case Response::RespWrongPassword: + reason = tr("The current password is incorrect."); + break; + case Response::RespPasswordTooShort: + reason = tr("The new password is too short."); + break; + case Response::RespClientUpdateRequired: + reason = tr("The server does not accept the new password format."); + break; + default: + reason = tr("The password could not be changed (error %1).").arg(responseCode); + break; + } + + QMessageBox::critical(dialogParent, tr("Password Change Failed"), reason); + remoteClient->disconnectFromServer(); + connectToServer(); +} + void ConnectionController::onForgotPasswordSuccess() { QMessageBox::information( diff --git a/cockatrice/src/client/network/connection_controller/remote_connection_controller.h b/cockatrice/src/client/network/connection_controller/remote_connection_controller.h index c8f96a9a9..2987a8d99 100644 --- a/cockatrice/src/client/network/connection_controller/remote_connection_controller.h +++ b/cockatrice/src/client/network/connection_controller/remote_connection_controller.h @@ -94,6 +94,10 @@ private slots: // Persists the derived scrypt verifier after a successful challenge-response login void onPasswordVerifierReady(const QString &verifier); + // Completes the forced password change the server demanded at login + void onPasswordChangeSuccess(); + void onPasswordChangeError(int responseCode); + private: void wireClientSignals(); void updateWindowTitle(); diff --git a/cockatrice/src/interface/widgets/server/user/user_info_box.cpp b/cockatrice/src/interface/widgets/server/user/user_info_box.cpp index 4feb73538..fa7698037 100644 --- a/cockatrice/src/interface/widgets/server/user/user_info_box.cpp +++ b/cockatrice/src/interface/widgets/server/user/user_info_box.cpp @@ -32,7 +32,6 @@ #include #include #include -#include #include #include #include @@ -299,17 +298,7 @@ void UserInfoBox::actPassword() void UserInfoBox::changePassword(const QString &oldPassword, const QString &newPassword) { - Command_AccountPassword cmd; - cmd.set_old_password(oldPassword.toStdString()); - if (client->getServerSupportsChallengeResponse()) { - cmd.set_hashed_new_password(PasswordHasher::generatePasswordVerifier(newPassword).toStdString()); - } else if (client->getServerSupportsPasswordHash()) { - auto passwordSalt = PasswordHasher::generateRandomSalt(); - QString hashedPassword = PasswordHasher::computeHash(newPassword, passwordSalt); - cmd.set_hashed_new_password(hashedPassword.toStdString()); - } else { - cmd.set_new_password(newPassword.toStdString()); - } + const Command_AccountPassword cmd = client->makeAccountPasswordCommand(oldPassword, newPassword); PendingCommand *pend = client->prepareSessionCommand(cmd); connect(pend, &PendingCommand::finished, this, &UserInfoBox::processPasswordResponse); diff --git a/libcockatrice_network/libcockatrice/network/client/abstract/CMakeLists.txt b/libcockatrice_network/libcockatrice/network/client/abstract/CMakeLists.txt index 880cef5a1..791db6348 100644 --- a/libcockatrice_network/libcockatrice/network/client/abstract/CMakeLists.txt +++ b/libcockatrice_network/libcockatrice/network/client/abstract/CMakeLists.txt @@ -15,7 +15,7 @@ target_include_directories(libcockatrice_network_client_abstract PUBLIC .) target_link_libraries( libcockatrice_network_client_abstract PUBLIC ${COCKATRICE_QT_VERSION_NAME}::Network ${COCKATRICE_QT_VERSION_NAME}::WebSockets libcockatrice_protocol - libcockatrice_network_server_remote + libcockatrice_network_server_remote libcockatrice_utility ) target_precompile_headers(libcockatrice_network_client_abstract PRIVATE "${CMAKE_SOURCE_DIR}/cmake/pch/qtcore_pch.h") diff --git a/libcockatrice_network/libcockatrice/network/client/abstract/abstract_client.cpp b/libcockatrice_network/libcockatrice/network/client/abstract/abstract_client.cpp index 0c523d776..04e6beda0 100644 --- a/libcockatrice_network/libcockatrice/network/client/abstract/abstract_client.cpp +++ b/libcockatrice_network/libcockatrice/network/client/abstract/abstract_client.cpp @@ -10,6 +10,7 @@ #include "libcockatrice/protocol/pb/serverinfo_user.pb.h" #include "libcockatrice/protocol/pb/session_commands.pb.h" #include "libcockatrice/protocol/pb/session_event.pb.h" +#include "libcockatrice/utility/passwordhasher.h" #include #include @@ -80,6 +81,26 @@ AbstractClient::~AbstractClient() { } +Command_AccountPassword AbstractClient::makeAccountPasswordCommand(const QString &oldPassword, + const QString &newPassword) const +{ + Command_AccountPassword cmd; + cmd.set_old_password(oldPassword.toStdString()); + if (getServerSupportsChallengeResponse()) { + // A verifier lets the server upgrade the stored credential without ever seeing the + // password, and it is what a challenge-response server expects for a new password. + cmd.set_hashed_new_password(PasswordHasher::generatePasswordVerifier(newPassword).toStdString()); + } else if (getServerSupportsPasswordHash()) { + // The server hashes the password itself with its own salt; a salted hash is the + // strongest form it accepts, so the plaintext never goes on the wire. + const QString passwordSalt = PasswordHasher::generateRandomSalt(); + cmd.set_hashed_new_password(PasswordHasher::computeHash(newPassword, passwordSalt).toStdString()); + } else { + cmd.set_new_password(newPassword.toStdString()); + } + return cmd; +} + void AbstractClient::processProtocolItem(const ServerMessage &item) { switch (item.message_type()) { diff --git a/libcockatrice_network/libcockatrice/network/client/abstract/abstract_client.h b/libcockatrice_network/libcockatrice/network/client/abstract/abstract_client.h index 7efd67972..005587ccc 100644 --- a/libcockatrice_network/libcockatrice/network/client/abstract/abstract_client.h +++ b/libcockatrice_network/libcockatrice/network/client/abstract/abstract_client.h @@ -47,6 +47,7 @@ class Event_UserJoined; class Event_UserLeft; class Event_ServerMessage; class Event_ListRooms; +class Command_AccountPassword; class Event_GameJoined; class Event_UserMessage; class Event_NotifyUser; @@ -67,6 +68,9 @@ enum ClientStatus StatusSubmitForgotPasswordReset, StatusSubmitForgotPasswordChallenge, StatusGettingPasswordSalt, + //! The server accepted the credentials but keeps the session open until the account + //! password is replaced; the connection must survive for `Command_AccountPassword`. + StatusPasswordChangeRequired, }; class AbstractClient : public QObject @@ -171,6 +175,16 @@ public: { return serverSupportsChallengeResponse; } + + /** + * @brief Builds the account-password command in the form this server understands. + * + * Challenge-response servers take a scrypt verifier; servers that only advertise + * SupportsPasswordHash take a salted SHA-512 hash; anything older gets the plaintext, + * which is the only form it understands. Both password-change paths go through here so + * the two cannot drift apart and hand a plaintext password to a server that can hash it. + */ + Command_AccountPassword makeAccountPasswordCommand(const QString &oldPassword, const QString &newPassword) const; const QString &getUserName() const { return userName; diff --git a/libcockatrice_network/libcockatrice/network/client/remote/remote_client.cpp b/libcockatrice_network/libcockatrice/network/client/remote/remote_client.cpp index 7eab58668..faacdc9f7 100644 --- a/libcockatrice_network/libcockatrice/network/client/remote/remote_client.cpp +++ b/libcockatrice_network/libcockatrice/network/client/remote/remote_client.cpp @@ -125,6 +125,7 @@ RemoteClient::RemoteClient(QObject *parent, INetworkSettingsProvider *_networkSe &RemoteClient::doSubmitForgotPasswordResetToServer); connect(this, &RemoteClient::sigSubmitForgotPasswordChallengeToServer, this, &RemoteClient::doSubmitForgotPasswordChallengeToServer); + connect(this, &RemoteClient::sigChangePasswordToServer, this, &RemoteClient::doChangePasswordToServer); } RemoteClient::~RemoteClient() @@ -354,6 +355,29 @@ void RemoteClient::submitPasswordVerifierResponse(const Response &response) } } +void RemoteClient::changePasswordToServer(const QString &oldPassword, const QString &newPassword) +{ + emit sigChangePasswordToServer(oldPassword, newPassword); +} + +void RemoteClient::doChangePasswordToServer(const QString &oldPassword, const QString &newPassword) +{ + const Command_AccountPassword cmd = makeAccountPasswordCommand(oldPassword, newPassword); + + PendingCommand *pend = prepareSessionCommand(cmd); + connect(pend, &PendingCommand::finished, this, &RemoteClient::accountPasswordResponse); + sendCommand(pend); +} + +void RemoteClient::accountPasswordResponse(const Response &response) +{ + if (response.response_code() == Response::RespOk) { + emit sigPasswordChangeSuccess(); + } else { + emit sigPasswordChangeError(response.response_code()); + } +} + void RemoteClient::processConnectionClosedEvent(const Event_ConnectionClosed & /*event*/) { doDisconnectFromServer(); @@ -475,6 +499,23 @@ void RemoteClient::loginResponse(const Response &response) emit notifyUserAboutUpdate(); } + } else if (response.response_code() == Response::RespPasswordChangeRequired) { + // The credentials were accepted: the server keeps this session open and serves + // nothing but ping and Command_AccountPassword until the password is replaced. + // Treating it as a failed login would disconnect before the dialog that collects + // the new password (queued to the UI thread behind this slot) can send anything: + // the command would sit in pendingCommands forever, because the ping timer that + // times it out stops with the connection. Keep the session and let the controller + // report the outcome through accountPasswordResponse(). + QList missingFeatures; + if (resp.missing_features_size() > 0) { + for (int i = 0; i < resp.missing_features_size(); ++i) { + missingFeatures << QString::fromStdString(resp.missing_features(i)); + } + } + setStatus(StatusPasswordChangeRequired); + emit loginError(response.response_code(), QString::fromStdString(resp.denied_reason_str()), + static_cast(resp.denied_end_time()), missingFeatures); } else if (response.response_code() != Response::RespNotConnected) { QList missingFeatures; if (resp.missing_features_size() > 0) { diff --git a/libcockatrice_network/libcockatrice/network/client/remote/remote_client.h b/libcockatrice_network/libcockatrice/network/client/remote/remote_client.h index d0c0a18ce..1cf40491c 100644 --- a/libcockatrice_network/libcockatrice/network/client/remote/remote_client.h +++ b/libcockatrice_network/libcockatrice/network/client/remote/remote_client.h @@ -71,6 +71,11 @@ signals: //! should only store it for the connection it is currently negotiating, so //! the hostname and user name are intentionally not part of the signal. void sigPasswordVerifierReady(const QString &verifier); + //! \brief Outcome of the password change the server demanded at login. + void sigPasswordChangeSuccess(); + void sigPasswordChangeError(int responseCode); + //! \brief Request a password change; queued to the client thread like the other requests. + void sigChangePasswordToServer(const QString &oldPassword, const QString &newPassword); private slots: void slotConnected(); void readData(); @@ -99,6 +104,8 @@ private slots: Command_Login generateCommandLogin(); void doSubmitPasswordVerifier(); void submitPasswordVerifierResponse(const Response &response); + void accountPasswordResponse(const Response &response); + void doChangePasswordToServer(const QString &oldPassword, const QString &newPassword); void doDisconnectFromServer(); void doActivateToServer(const QString &_token); void doRequestForgotPasswordToServer(const QString &hostname, unsigned int port, const QString &_userName); @@ -201,6 +208,10 @@ public: unsigned int port, const QString &_userName, const QString &_email); + //! \brief Change the password of the account that just logged in, following the + //! server's forced-password-change reply. Emits sigPasswordChangeSuccess or + //! sigPasswordChangeError when the server answers. + void changePasswordToServer(const QString &oldPassword, const QString &newPassword); }; #endif diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server.cpp b/libcockatrice_network/libcockatrice/network/server/remote/server.cpp index 63989bbfa..8ea2cd993 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/server.cpp @@ -131,6 +131,16 @@ AuthenticationResult Server::loginUser(Server_ProtocolHandler *session, return authState; } + if (authState == PasswordChangeRequired) { + // The credentials were right, but the account must choose a new password before it may + // use the server. Give the session an identity so it can complete that change, without + // registering a user session or announcing the user as logged in. + ServerInfo_User pendingData = databaseInterface->getUserData(name, true); + pendingData.set_address(session->getAddress().toStdString()); + session->setUserInfo(pendingData); + return authState; + } + ServerInfo_User data = databaseInterface->getUserData(name, true); data.set_address(session->getAddress().toStdString()); name = QString::fromStdString(data.name()); // Compensate for case indifference @@ -296,7 +306,9 @@ void Server::removeClient(Server_ProtocolHandler *client) QWriteLocker locker(&clientsLock); clients.removeAt(clientIndex); ServerInfo_User *data = client->getUserInfo(); - if (data) { + // A session that never completed login (for example one that still owes a password change) + // has no session id: it was never announced as joined, so it must not be announced as leaving. + if (data && data->has_session_id()) { Event_UserLeft event; event.set_name(data->name()); SessionEvent *se = Server_ProtocolHandler::prepareSessionEvent(event); @@ -311,12 +323,10 @@ void Server::removeClient(Server_ProtocolHandler *client) users.remove(QString::fromStdString(data->name())); qDebug() << "Server::removeClient: name=" << QString::fromStdString(data->name()); - if (data->has_session_id()) { - const qint64 sessionId = data->session_id(); - usersBySessionId.remove(sessionId); - emit endSession(sessionId); - qDebug() << "closed session id:" << sessionId; - } + const qint64 sessionId = data->session_id(); + usersBySessionId.remove(sessionId); + emit endSession(sessionId); + qDebug() << "closed session id:" << sessionId; } qDebug() << "Server::removeClient: removed" << (void *)client << ";" << clients.size() << "clients; " << users.size() << "users left"; @@ -330,7 +340,9 @@ QList Server::getOnlineModeratorList() const ServerInfo_User *data = client->getUserInfo(); //! \todo This line should be updated in the event there is any type of new user level created. - if (data && + // A session that still owes a forced password change has user info but no session id: + // it never joined, so it must not be reported as an online moderator either. + if (data && data->has_session_id() && (data->user_level() & ServerInfo_User::IsModerator || data->user_level() & ServerInfo_User::IsAdmin)) { results << QString::fromStdString(data->name()).simplified(); } diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp index dd074a517..39c1eb3e6 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp @@ -64,8 +64,9 @@ Server_ProtocolHandler::Server_ProtocolHandler(Server *_server, Server_DatabaseInterface *_databaseInterface, QObject *parent) : QObject(parent), Server_AbstractUserInterface(_server), deleted(false), databaseInterface(_databaseInterface), - authState(NotLoggedIn), usingRealPassword(false), acceptsUserListChanges(false), acceptsRoomListChanges(false), - idleClientWarningSent(false), timeRunning(0), lastDataReceived(0), lastActionReceived(0) + authState(NotLoggedIn), usingRealPassword(false), passwordChangePending(false), acceptsUserListChanges(false), + acceptsRoomListChanges(false), idleClientWarningSent(false), timeRunning(0), lastDataReceived(0), + lastActionReceived(0) { connect(server, &Server::pingClockTimeout, this, &Server_ProtocolHandler::pingClockTimeout); @@ -212,7 +213,10 @@ Response::ResponseCode Server_ProtocolHandler::processSessionCommandContainer(co logDebugMessage(getSafeDebugString(sc)); } const auto commandType = static_cast(num); - if (authState == NotLoggedIn && !isPreAuthSessionCommand(commandType)) { + if (passwordChangePending && commandType != SessionCommand::PING && + commandType != SessionCommand::ACCOUNT_PASSWORD) { + resp = Response::RespFunctionNotAllowed; + } else if (authState == NotLoggedIn && !isPreAuthSessionCommand(commandType)) { resp = Response::RespLoginNeeded; } else { switch (commandType) { @@ -489,6 +493,19 @@ void Server_ProtocolHandler::processCommandContainer(const CommandContainer &con lastDataReceived = timeRunning; + // A session that still owes a forced password change may only reach the session family, + // where processSessionCommandContainer narrows it further to ping and the change itself. + // Every other family is rejected here, including a container that also carries a session + // command: the dispatch below prefers game/room over session, so testing only for the + // absence of a session command would let such a container through. + if (passwordChangePending && + (cont.game_command_size() || cont.room_command_size() || cont.moderator_command_size() || + cont.admin_command_size() || cont.developer_command_size())) { + sendResponseContainer(ResponseContainer(cont.has_cmd_id() ? cont.cmd_id() : -1), + Response::RespFunctionNotAllowed); + return; + } + ResponseContainer responseContainer(cont.has_cmd_id() ? cont.cmd_id() : -1); Response::ResponseCode finalResponseCode; @@ -696,6 +713,11 @@ Response::ResponseCode Server_ProtocolHandler::cmdLogin(const Command_Login &cmd server->recordFailedLogin(getAddress(), userName); return Response::RespAccountNotActivated; case PasswordChangeRequired: + // The credentials were right, but the account must choose a new password first. + // Keep the session authenticated just far enough for the client to do that: + // processCommandContainer refuses everything except the change itself. + authState = PasswordRight; + passwordChangePending = true; return Response::RespPasswordChangeRequired; default: server->clearFailedLogins(getAddress(), userName); diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.h b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.h index 266c4b26c..8ce110354 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.h @@ -46,6 +46,12 @@ protected: Server_DatabaseInterface *databaseInterface; AuthenticationResult authState; bool usingRealPassword; + /** + * @brief True when the credentials were right but the account must choose a new password + * before it may use the server. Only ping, the password change and disconnecting + * are served while this is set. + */ + bool passwordChangePending; bool acceptsUserListChanges; bool acceptsRoomListChanges; bool idleClientWarningSent; diff --git a/servatrice/src/servatrice_database_interface.cpp b/servatrice/src/servatrice_database_interface.cpp index 1b49c3a82..45ae9bde8 100644 --- a/servatrice/src/servatrice_database_interface.cpp +++ b/servatrice/src/servatrice_database_interface.cpp @@ -482,6 +482,10 @@ AuthenticationResult Servatrice_DatabaseInterface::checkUserPassword(Server_Prot const QByteArray expected = PasswordHasher::computeResponse(key, nonce); handler->clearAuthNonce(); if (PasswordHasher::constantTimeEquals(expected, response)) { + if (forceChange) { + qCDebug(DatabaseInterfaceLog) << "Login accepted but password change required"; + return PasswordChangeRequired; + } qCDebug(DatabaseInterfaceLog) << "Login accepted: challenge-response password right"; return PasswordRight; } diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index 963b98c9c..f0c0c3536 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -3787,6 +3787,9 @@ Response::ResponseCode AbstractServerSocketInterface::cmdAccountPassword(const C } databaseInterface->setForcePasswordChange(userName, false); + // The forced change is done, but this session never completed login (see + // Server_ProtocolHandler::cmdLogin). It stays restricted to ping and another password + // change; the client reconnects with the new password to actually enter the server. return Response::RespOk; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index c8d3fbd59..00b39193d 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -328,6 +328,28 @@ cockatrice_add_test( LABELS server ) +cockatrice_add_test( + NAME + server_forced_password_change_test + SOURCES + server_forced_password_change_test.cpp + LIBS + libcockatrice_network_server_remote + libcockatrice_rng + LABELS + server +) +cockatrice_add_test( + NAME + client_login_status_test + SOURCES + ${VERSION_STRING_CPP} + client_login_status_test.cpp + LIBS + libcockatrice_network_client_remote + LABELS + ci +) cockatrice_add_test( NAME server_room_channels_test diff --git a/tests/client_login_status_test.cpp b/tests/client_login_status_test.cpp new file mode 100644 index 000000000..d8ea63550 --- /dev/null +++ b/tests/client_login_status_test.cpp @@ -0,0 +1,116 @@ +/** @file client_login_status_test.cpp + * @brief The login response state machine must keep the session a forced password change needs. + * @ingroup Tests + */ + +#include "abstract_client.h" +#include "libcockatrice/interfaces/interface_network_settings_provider.h" +#include "libcockatrice/protocol/pb/response.pb.h" +#include "libcockatrice/protocol/pb/response_login.pb.h" +#include "remote_client.h" + +#include +#include +#include +#include + +namespace +{ + +class StubNetworkSettingsProvider : public INetworkSettingsProvider +{ +public: + QString getClientID() override + { + return {}; + } + [[nodiscard]] int getTimeOut() const override + { + return 1000; + } + [[nodiscard]] int getKeepAlive() const override + { + return 60; + } + [[nodiscard]] bool getNotifyAboutUpdates() const override + { + return false; + } + void setKnownMissingFeatures(const QString &) override + { + } + QString getKnownMissingFeatures() override + { + return {}; + } +}; + +/** + * @brief Feeds a login response to RemoteClient without a socket. + * + * `loginResponse` is the slot the socket thread runs for the login command's answer, so + * invoking it directly exercises the status transition that decides whether `readData()` + * tears the connection down right after it. + */ +class ClientLoginStatusTest : public ::testing::Test +{ +protected: + void SetUp() override + { + client = new RemoteClient(nullptr, &provider); + } + + void TearDown() override + { + delete client; + client = nullptr; + } + + void deliverLoginResponse(Response::ResponseCode code) + { + Response response; + response.set_response_code(code); + response.MutableExtension(Response_Login::ext); + ASSERT_TRUE( + QMetaObject::invokeMethod(client, "loginResponse", Qt::DirectConnection, Q_ARG(Response, response))); + } + + StubNetworkSettingsProvider provider; + RemoteClient *client = nullptr; +}; + +/** + * @brief A required password change is not a failed login. + * + * The server keeps the session open and accepts only ping and Command_AccountPassword until + * the password is replaced. StatusDisconnecting would make readData() close the socket right + * after this slot returns - before the dialog that collects the new password (queued to the + * UI thread) can send anything - and the pending command would never be answered because the + * ping timer that times it out dies with the connection. StatusLoggedIn would start the + * normal post-login flow for a session that cannot serve it. + */ +TEST_F(ClientLoginStatusTest, PasswordChangeRequiredKeepsTheSessionOpen) +{ + deliverLoginResponse(Response::RespPasswordChangeRequired); + + EXPECT_EQ(client->getStatus(), StatusPasswordChangeRequired); + EXPECT_NE(client->getStatus(), StatusDisconnecting); + EXPECT_NE(client->getStatus(), StatusLoggedIn); +} + +/** @brief Every other login error still ends the connection, as it did before. */ +TEST_F(ClientLoginStatusTest, OtherLoginErrorsDisconnect) +{ + deliverLoginResponse(Response::RespWrongPassword); + + EXPECT_EQ(client->getStatus(), StatusDisconnecting); +} + +} // namespace + +int main(int argc, char **argv) +{ + QCoreApplication app(argc, argv); + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/tests/server_forced_password_change_test.cpp b/tests/server_forced_password_change_test.cpp new file mode 100644 index 000000000..9717b610b --- /dev/null +++ b/tests/server_forced_password_change_test.cpp @@ -0,0 +1,234 @@ +/** @file server_forced_password_change_test.cpp + * @brief Tests for the restricted session a forced password change leaves behind at login. + * @ingroup Tests + */ + +#include "libcockatrice/protocol/pb/response.pb.h" +#include "libcockatrice/protocol/pb/server_message.pb.h" +#include "server.h" +#include "server_database_interface.h" +#include "server_protocolhandler.h" + +#include +#include +#include +#include +#include +#include +#include + +RNG_Abstract *rng = nullptr; // referenced by the server_remote library + +namespace +{ + +class MockDatabaseInterface : public Server_DatabaseInterface +{ +public: + AuthenticationResult checkUserPassword(Server_ProtocolHandler *, + const QString &, + const QString &, + const QString &, + QString &, + int &, + bool) override + { + return PasswordChangeRequired; + } + ServerInfo_User getUserData(const QString &name, bool withId) override + { + ServerInfo_User user; + user.set_name(name.toStdString()); + user.set_user_level(ServerInfo_User::IsUser | ServerInfo_User::IsRegistered | ServerInfo_User::IsModerator); + if (withId) { + user.set_id(1); + } + return user; + } + int getNextReplayId() override + { + return 1; + } + int getNextGameId() override + { + return 1; + } + int getActiveUserCount(QString) override + { + return 0; + } +}; + +class FakeServer : public Server +{ +public: + FakeServer() + { + setDatabaseInterface(new MockDatabaseInterface()); + } +}; + +class TestHandler : public Server_ProtocolHandler +{ +public: + TestHandler(Server *_server, Server_DatabaseInterface *_db) : Server_ProtocolHandler(_server, _db) + { + } + + QString getAddress() const override + { + return {}; + } + QString getConnectionType() const override + { + return {}; + } + + Response::ResponseCode lastResponseCode = Response::RespNothing; + int lastExtendedSessionCommand = -1; + +protected: + Response::ResponseCode + processExtendedSessionCommand(int cmdType, const SessionCommand & /*cmd*/, ResponseContainer & /*rc*/) override + { + lastExtendedSessionCommand = cmdType; + return Response::RespOk; + } + + void transmitProtocolItem(const ServerMessage &item) override + { + if (item.message_type() == ServerMessage::RESPONSE) { + lastResponseCode = item.response().response_code(); + } + } +}; + +class ForcedPasswordChangeTest : public ::testing::Test +{ +protected: + FakeServer server; + TestHandler handler{&server, server.getDatabaseInterface()}; + + void login() + { + CommandContainer cont; + cont.set_cmd_id(1); + Command_Login cmd; + cmd.set_user_name("mod"); + cont.add_session_command()->MutableExtension(Command_Login::ext)->CopyFrom(cmd); + handler.processCommandContainer(cont); + } + + Response::ResponseCode sendPing() + { + CommandContainer cont; + cont.set_cmd_id(2); + cont.add_session_command()->MutableExtension(Command_Ping::ext); + handler.processCommandContainer(cont); + return handler.lastResponseCode; + } + + Response::ResponseCode sendSessionCommand() + { + CommandContainer cont; + cont.set_cmd_id(3); + cont.add_session_command()->MutableExtension(Command_ListRooms::ext); + handler.processCommandContainer(cont); + return handler.lastResponseCode; + } + + Response::ResponseCode sendRoomCommand() + { + CommandContainer cont; + cont.set_cmd_id(4); + cont.add_room_command()->MutableExtension(Command_RoomSay::ext)->set_message("hello"); + handler.processCommandContainer(cont); + return handler.lastResponseCode; + } + + Response::ResponseCode sendMixedSessionAndRoomCommand() + { + CommandContainer cont; + cont.set_cmd_id(5); + cont.add_session_command()->MutableExtension(Command_Ping::ext); + cont.add_room_command()->MutableExtension(Command_RoomSay::ext)->set_message("hello"); + handler.processCommandContainer(cont); + return handler.lastResponseCode; + } + + Response::ResponseCode sendAccountPassword() + { + CommandContainer cont; + cont.set_cmd_id(6); + Command_AccountPassword cmd; + cmd.set_old_password("temporary-password"); + cmd.set_new_password("replacement-password"); + cont.add_session_command()->MutableExtension(Command_AccountPassword::ext)->CopyFrom(cmd); + handler.processCommandContainer(cont); + return handler.lastResponseCode; + } +}; + +TEST_F(ForcedPasswordChangeTest, LoginReportsTheRequiredChange) +{ + login(); + + EXPECT_EQ(Response::RespPasswordChangeRequired, handler.lastResponseCode); + ASSERT_NE(nullptr, handler.getUserInfo()); + EXPECT_EQ("mod", QString::fromStdString(handler.getUserInfo()->name())); +} + +TEST_F(ForcedPasswordChangeTest, PendingSessionIsNotRegisteredAsAUser) +{ + login(); + // The session may change its password, but it must not take a user slot or appear in + // the session table before the client reconnects with the new password. + EXPECT_FALSE(handler.getUserInfo()->has_session_id()); + EXPECT_EQ(0, server.getUsersCount()); +} + +TEST_F(ForcedPasswordChangeTest, PendingModeratorIsNotReportedOnline) +{ + login(); + server.addClient(&handler); + + EXPECT_TRUE(server.getOnlineModeratorList().isEmpty()); +} + +TEST_F(ForcedPasswordChangeTest, OnlyPingAndPasswordChangeAreServed) +{ + login(); + + EXPECT_EQ(Response::RespOk, sendPing()); + EXPECT_EQ(Response::RespFunctionNotAllowed, sendSessionCommand()); + EXPECT_EQ(Response::RespFunctionNotAllowed, sendRoomCommand()); + // Dispatch prefers game/room over session, so a container carrying both must not slip past + // the restriction by carrying a session command. + EXPECT_EQ(Response::RespFunctionNotAllowed, sendMixedSessionAndRoomCommand()); +} + +TEST_F(ForcedPasswordChangeTest, AccountPasswordReachesTheCommand) +{ + login(); + // The forced change is useless if the gate refuses the command it needs. ACCOUNT_PASSWORD is + // served by the extended-command hook, so it must arrive there instead of being filtered. + EXPECT_EQ(Response::RespOk, sendAccountPassword()); + EXPECT_EQ(SessionCommand::ACCOUNT_PASSWORD, handler.lastExtendedSessionCommand); +} + +TEST_F(ForcedPasswordChangeTest, CompletedLoginIsStillReportedOnline) +{ + TestHandler loggedIn{&server, server.getDatabaseInterface()}; + ServerInfo_User user; + user.set_name("online-mod"); + user.set_user_level(ServerInfo_User::IsUser | ServerInfo_User::IsRegistered | ServerInfo_User::IsModerator); + user.set_session_id(7); + loggedIn.setUserInfo(user); + server.addClient(&loggedIn); + + const QList moderators = server.getOnlineModeratorList(); + ASSERT_EQ(1, moderators.size()); + EXPECT_EQ("online-mod", moderators.first()); +} + +} // namespace