diff --git a/src/slic3r/Utils/OrcaCloudServiceAgent.cpp b/src/slic3r/Utils/OrcaCloudServiceAgent.cpp index 5d129a490e..9d2aa91ef3 100644 --- a/src/slic3r/Utils/OrcaCloudServiceAgent.cpp +++ b/src/slic3r/Utils/OrcaCloudServiceAgent.cpp @@ -818,7 +818,9 @@ int OrcaCloudServiceAgent::user_logout(bool request) } } - clear_session(); + // An explicit logout also wipes the backend the token storage option is not using, so a token + // stranded by switching that option cannot sign the account back in later. + clear_session(/*all_backends=*/request); return BAMBU_NETWORK_SUCCESS; } @@ -1603,7 +1605,9 @@ void OrcaCloudServiceAgent::persist_user_secret(const std::string& secret) } } - (void) stored; + if (stored) { + secret_stored = true; + } } bool OrcaCloudServiceAgent::load_user_secret(std::string& out_secret) @@ -1643,6 +1647,7 @@ bool OrcaCloudServiceAgent::load_user_secret(std::string& out_secret) } if (integrity_ok && aes256gcm_decrypt(encoded_payload, key, plain) && !plain.empty()) { + secret_stored = true; out_secret = plain; // Upgrade legacy payloads to signed format if (payload.rfind("v2:", 0) != 0) { @@ -1660,6 +1665,7 @@ bool OrcaCloudServiceAgent::load_user_secret(std::string& out_secret) if (store.Load(SECRET_STORE_SERVICE, username, secret) && secret.IsOk()) { out_secret.assign(static_cast(secret.GetData()), secret.GetSize()); if (!out_secret.empty()) { + secret_stored = true; return true; } } @@ -1669,11 +1675,20 @@ bool OrcaCloudServiceAgent::load_user_secret(std::string& out_secret) return false; } -void OrcaCloudServiceAgent::clear_user_secret() +void OrcaCloudServiceAgent::clear_user_secret(bool all_backends) { - wxSecretStore store = wxSecretStore::GetDefault(); - if (store.IsOk()) { - store.Delete(SECRET_STORE_SERVICE); + // Nothing this process loaded or saved: leave the store alone. Deleting would only cost a + // keychain round trip (or a hang while the keychain is unresponsive) and could remove a + // login another instance just saved. + if (!secret_stored.exchange(false) && !all_backends) { + return; + } + + if (all_backends || !m_use_encrypted_token_file) { + wxSecretStore store = wxSecretStore::GetDefault(); + if (store.IsOk()) { + store.Delete(SECRET_STORE_SERVICE); + } } compute_fallback_path(); @@ -2022,13 +2037,13 @@ bool OrcaCloudServiceAgent::set_user_session(const json& session_json, bool noti return success; } -void OrcaCloudServiceAgent::clear_session() +void OrcaCloudServiceAgent::clear_session(bool all_backends) { { std::lock_guard lock(session_mutex); session = SessionInfo{}; } - clear_user_secret(); + clear_user_secret(all_backends); } // ============================================================================ diff --git a/src/slic3r/Utils/OrcaCloudServiceAgent.hpp b/src/slic3r/Utils/OrcaCloudServiceAgent.hpp index 3ae86ec27c..6b8be732bf 100644 --- a/src/slic3r/Utils/OrcaCloudServiceAgent.hpp +++ b/src/slic3r/Utils/OrcaCloudServiceAgent.hpp @@ -324,7 +324,7 @@ public: void persist_user_secret(const std::string& secret); bool load_user_secret(std::string& out_secret); - void clear_user_secret(); + void clear_user_secret(bool all_backends = false); // Token refresh helpers bool refresh_if_expiring(std::chrono::seconds skew, const std::string& reason); @@ -342,7 +342,7 @@ public: bool persist = true); // Accepts either nested Orca cloud / GoTrue session JSON or flat WebView token JSON. bool set_user_session(const nlohmann::json& session_json, bool notify_login = true); - void clear_session(); + void clear_session(bool all_backends = false); static std::string generate_uuid_for_setting_id(const std::string& name, const std::string& user_id = ""); @@ -411,6 +411,11 @@ private: // Member variables - auth state PkceBundle pkce_bundle; std::string secret_fallback_path; + // Set once this process has read a secret from the store or written one. Unless the user logs + // out explicitly, clear_user_secret() only touches the store while it is set, so a logged-out + // instance (the GUI polls the login status every 2 s) makes no keychain calls and cannot wipe + // a login another instance saved. + std::atomic_bool secret_stored{false}; SessionHandler session_handler; OnLoginCompleteHandler on_login_complete_handler; SessionInfo session; diff --git a/tests/slic3rutils/CMakeLists.txt b/tests/slic3rutils/CMakeLists.txt index 7f541f3701..54700acab7 100644 --- a/tests/slic3rutils/CMakeLists.txt +++ b/tests/slic3rutils/CMakeLists.txt @@ -10,6 +10,7 @@ add_executable(${_TEST_NAME}_tests test_prebuild_queue.cpp test_staged_build.cpp test_network_versions.cpp + test_orca_cloud_agent.cpp test_action_source.cpp test_plugin_host_api.cpp test_plugin_capability_config.cpp diff --git a/tests/slic3rutils/test_orca_cloud_agent.cpp b/tests/slic3rutils/test_orca_cloud_agent.cpp new file mode 100644 index 0000000000..76f4e8df16 --- /dev/null +++ b/tests/slic3rutils/test_orca_cloud_agent.cpp @@ -0,0 +1,86 @@ +#include + +#include +#include + +#include +#include + +#include "slic3r/Utils/OrcaCloudServiceAgent.hpp" +#include "test_utils.hpp" + +using namespace Slic3r; +namespace fs = boost::filesystem; + +namespace { + +// The encrypted token file is the one secret backend a test can observe without a system +// keychain. Every agent pointed at the same directory shares it, like separate app instances +// share the keychain entry. +std::unique_ptr make_file_backed_agent(const fs::path& dir) +{ + auto agent = std::make_unique(dir.string()); + agent->set_use_encrypted_token_file(true); + agent->set_config_dir(dir.string()); + return agent; +} + +fs::path secret_file(const fs::path& dir) { return dir / secret_constants::USER_SECRET_FILENAME; } + +} // namespace + +TEST_CASE("Logging out removes the secret this instance saved", "[OrcaCloudServiceAgent]") +{ + ScopedTemporaryDir dir("orca-secret"); + auto agent = make_file_backed_agent(dir.path()); + + agent->persist_user_secret("refresh-token"); + REQUIRE(fs::exists(secret_file(dir.path()))); + + agent->user_logout(false); + CHECK_FALSE(fs::exists(secret_file(dir.path()))); +} + +TEST_CASE("Logging out removes a secret this instance loaded from the store", "[OrcaCloudServiceAgent]") +{ + ScopedTemporaryDir dir("orca-secret"); + make_file_backed_agent(dir.path())->persist_user_secret("refresh-token"); + + auto agent = make_file_backed_agent(dir.path()); + std::string secret; + REQUIRE(agent->load_user_secret(secret)); + CHECK(secret == "refresh-token"); + + agent->user_logout(false); + CHECK_FALSE(fs::exists(secret_file(dir.path()))); +} + +TEST_CASE("Logging out leaves a secret this instance never loaded or saved alone", "[OrcaCloudServiceAgent]") +{ + ScopedTemporaryDir dir("orca-secret"); + make_file_backed_agent(dir.path())->persist_user_secret("refresh-token"); + + // A logged-out instance is asked to log out on every login-status poll. + auto other = make_file_backed_agent(dir.path()); + other->user_logout(false); + other->user_logout(false); + CHECK(fs::exists(secret_file(dir.path()))); + + std::string secret; + REQUIRE(make_file_backed_agent(dir.path())->load_user_secret(secret)); + CHECK(secret == "refresh-token"); +} + +TEST_CASE("Logging out leaves a secret this instance could not read alone", "[OrcaCloudServiceAgent]") +{ + ScopedTemporaryDir dir("orca-secret"); + // Written under another encryption key, e.g. by another OS user sharing the data directory. + fs::ofstream(secret_file(dir.path())) << "v2:0000:not-a-payload-this-user-can-decrypt"; + + auto agent = make_file_backed_agent(dir.path()); + std::string secret; + REQUIRE_FALSE(agent->load_user_secret(secret)); + + agent->user_logout(false); + CHECK(fs::exists(secret_file(dir.path()))); +}