From 866832b665dadc1c95ecce063ead23b6eb32a833 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 30 Sep 2021 09:39:17 -0500 Subject: [PATCH 1/2] This seemingly works the way I'd expect, going to merge into the warning dialog and check --- src/cascadia/TerminalApp/TerminalPage.cpp | 3 - .../ApplicationState.cpp | 72 +++++++++++++------ .../TerminalSettingsModel/ApplicationState.h | 2 + 3 files changed, 53 insertions(+), 24 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 25140362d..b80aa54d3 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -298,10 +298,7 @@ namespace winrt::TerminalApp::implementation // - true if the ApplicationState should be used. bool TerminalPage::ShouldUsePersistedLayout(CascadiaSettings& settings) const { - // GH#5000 Until there is a separate state file for elevated sessions we should just not - // save at all while in an elevated window. return Feature_PersistedWindowLayout::IsEnabled() && - !IsElevated() && settings.GlobalSettings().FirstWindowPreference() == FirstWindowPreference::PersistedWindowLayout; } diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index 1874dd24a..f1c31297c 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -13,7 +13,7 @@ static constexpr std::wstring_view stateFileName{ L"state.json" }; static constexpr std::wstring_view elevatedStateFileName{ L"elevated-state.json" }; -static constexpr std::wstring_view unelevatedStateFileName{ L"user-state.json" }; +// static constexpr std::wstring_view unelevatedStateFileName{ L"user-state.json" }; static constexpr std::string_view TabLayoutKey{ "tabLayout" }; static constexpr std::string_view InitialPositionKey{ "initialPosition" }; @@ -91,7 +91,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation ApplicationState::ApplicationState(const std::filesystem::path& stateRoot) noexcept : _sharedPath{ stateRoot / stateFileName }, - _userPath{ stateRoot / unelevatedStateFileName }, + // _userPath{ stateRoot / unelevatedStateFileName }, _elevatedPath{ stateRoot / elevatedStateFileName }, _throttler{ std::chrono::seconds(1), [this]() { _write(); } } { @@ -116,8 +116,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { static const auto sharedPath{ _sharedPath.filename() }; static const auto elevatedPath{ _elevatedPath.filename() }; - static const auto userPath{ _userPath.filename() }; - return filename == sharedPath || filename == elevatedPath || filename == userPath; + // static const auto userPath{ _userPath.filename() }; + return filename == sharedPath || filename == elevatedPath /* || filename == userPath*/; } // Deserializes the state.json and user-state (or elevated-state if @@ -140,19 +140,26 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); } - FromJson(root, FileSource::Shared); - } - - // Then, try and get anything in user-state/elevated-state - if (const auto localData{ _readLocalContents().value_or(std::string{}) }; !localData.empty()) - { - Json::Value root; - if (!reader->parse(localData.data(), localData.data() + localData.size(), &root, &errs)) + if (::Microsoft::Console::Utils::IsElevated()) { - throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); + // Only load shared properties if we're elevated + FromJson(root, FileSource::Shared); + // Then, try and get anything in user-state/elevated-state + if (const auto localData{ _readLocalContents().value_or(std::string{}) }; !localData.empty()) + { + Json::Value root; + if (!reader->parse(localData.data(), localData.data() + localData.size(), &root, &errs)) + { + throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); + } + FromJson(root, FileSource::Local); + } + } + else + { + // If we're unelevated, then load everything. + FromJson(root, FileSource::Shared | FileSource::Local); } - - FromJson(root, FileSource::Local); } } CATCH_LOG() @@ -165,9 +172,28 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation try { Json::StreamWriterBuilder wbuilder; - - _writeSharedContents(Json::writeString(wbuilder, ToJson(FileSource::Shared))); - _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local))); + if (::Microsoft::Console::Utils::IsElevated()) + { + std::string errs; + std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; + Json::Value root; + const auto sharedData = _readSharedContents().value_or(std::string{}); + if (!sharedData.empty()) + { + if (!reader->parse(sharedData.data(), sharedData.data() + sharedData.size(), &root, &errs)) + { + throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); + } + } + _writeSharedContents(Json::writeString(wbuilder, _toJsonWithBlob(root, FileSource::Shared))); + _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local))); + } + else + { + _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local | FileSource::Shared))); + } + // _writeSharedContents(Json::writeString(wbuilder, ToJson(FileSource::Shared))); + // _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local))); } CATCH_LOG() @@ -193,7 +219,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // At the time of writing the former version skips missing fields in the json, // but we want to explicitly clear state fields that were removed from state.json. #define MTSM_APPLICATION_STATE_GEN(source, type, name, key, ...) \ - if (parseSource == source) \ + if (WI_IsFlagSet(parseSource, source)) \ state->name = JsonUtils::GetValueForKey>(root, key); MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) @@ -203,7 +229,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Json::Value ApplicationState::ToJson(FileSource parseSource) const noexcept { Json::Value root{ Json::objectValue }; + return _toJsonWithBlob(root, parseSource); + } + Json::Value ApplicationState::_toJsonWithBlob(Json::Value& root, FileSource parseSource) const noexcept + { { auto state = _state.lock_shared(); #define MTSM_APPLICATION_STATE_GEN(source, type, name, key, ...) \ @@ -257,7 +287,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { return ::Microsoft::Console::Utils::IsElevated() ? ReadUTF8FileIfExists(_elevatedPath, true) : - ReadUTF8FileIfExists(_userPath, false); + ReadUTF8FileIfExists(_sharedPath, false); } // Method Description: @@ -291,7 +321,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation } else { - WriteUTF8FileAtomic(_userPath, content); + WriteUTF8FileAtomic(_sharedPath, content); } } diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h index ed6db33fe..80bde4cf1 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.h +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h @@ -91,6 +91,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void _write() const noexcept; void _read() const noexcept; + Json::Value _toJsonWithBlob(Json::Value& root, FileSource parseSource) const noexcept; + std::optional _readSharedContents() const; void _writeSharedContents(const std::string_view content) const; std::optional _readLocalContents() const; From b4e0496effa0a6dea5efb842b4784e377b62b017 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 30 Sep 2021 10:27:57 -0500 Subject: [PATCH 2/2] cleanup --- .../ApplicationState.cpp | 50 +++++++++++++++---- 1 file changed, 41 insertions(+), 9 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index f1c31297c..88c36697e 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -13,7 +13,6 @@ static constexpr std::wstring_view stateFileName{ L"state.json" }; static constexpr std::wstring_view elevatedStateFileName{ L"elevated-state.json" }; -// static constexpr std::wstring_view unelevatedStateFileName{ L"user-state.json" }; static constexpr std::string_view TabLayoutKey{ "tabLayout" }; static constexpr std::string_view InitialPositionKey{ "initialPosition" }; @@ -91,7 +90,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation ApplicationState::ApplicationState(const std::filesystem::path& stateRoot) noexcept : _sharedPath{ stateRoot / stateFileName }, - // _userPath{ stateRoot / unelevatedStateFileName }, _elevatedPath{ stateRoot / elevatedStateFileName }, _throttler{ std::chrono::seconds(1), [this]() { _write(); } } { @@ -116,8 +114,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { static const auto sharedPath{ _sharedPath.filename() }; static const auto elevatedPath{ _elevatedPath.filename() }; - // static const auto userPath{ _userPath.filename() }; - return filename == sharedPath || filename == elevatedPath /* || filename == userPath*/; + return filename == sharedPath || filename == elevatedPath; } // Deserializes the state.json and user-state (or elevated-state if @@ -130,7 +127,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::string errs; std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; - // First get shared state out of `state.json` into us + // First get shared state out of `state.json`. const auto sharedData = _readSharedContents().value_or(std::string{}); if (!sharedData.empty()) { @@ -140,11 +137,16 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); } + // - If we're elevated, we want to only load the Shared properties + // from state.json. We'll then load the Local props from + // `elevated-state.json` + // - If we're unelevated, then load _everything_ from state.json. if (::Microsoft::Console::Utils::IsElevated()) { // Only load shared properties if we're elevated FromJson(root, FileSource::Shared); - // Then, try and get anything in user-state/elevated-state + + // Then, try and get anything in elevated-state if (const auto localData{ _readLocalContents().value_or(std::string{}) }; !localData.empty()) { Json::Value root; @@ -172,11 +174,30 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation try { Json::StreamWriterBuilder wbuilder; + + // When we're elevated, we've got to be tricky. We don't want to write + // our window state, allowed commandlines, and other Local properties + // into the shared `state.json`. But, if we only serialize the Shared + // properties to a json blob, then we'll omit windowState entirely, + // _removing_ the window state of the unelevated instance. Oh no! + // + // So, to be tricky, we'll first _load_ the shared state to a json blob. + // We'll then serialize our view of the shared properties on top of that + // blob. Then we'll write that blob back to the file. This will + // round-trip the Local properties for the unelevated instances + // untouched in state.json + // + // After that's done, we'll write our Local properties into + // elevated-state.json. if (::Microsoft::Console::Utils::IsElevated()) { std::string errs; std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; Json::Value root; + + // First load the contents of state.json into a json blob. This will + // contain the Shared properties and teh unelevated instance's Local + // properties. const auto sharedData = _readSharedContents().value_or(std::string{}); if (!sharedData.empty()) { @@ -185,15 +206,18 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); } } + // Layer our shared properties on top of the blob from state.json, + // and write it back out. _writeSharedContents(Json::writeString(wbuilder, _toJsonWithBlob(root, FileSource::Shared))); + + // Finally, write our Local properties back to elevated-state.json _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local))); } else { + // We're unelevated, this is easy. Just write everything back out. _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local | FileSource::Shared))); } - // _writeSharedContents(Json::writeString(wbuilder, ToJson(FileSource::Shared))); - // _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local))); } CATCH_LOG() @@ -218,8 +242,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // * return std::optional by value // At the time of writing the former version skips missing fields in the json, // but we want to explicitly clear state fields that were removed from state.json. + // + // GH#11222: We only load properties that are of the same type (Local or + // Shared) which we requested. If we didn't want to load this type of + // property, just skip it. #define MTSM_APPLICATION_STATE_GEN(source, type, name, key, ...) \ - if (WI_IsFlagSet(parseSource, source)) \ + if (WI_IsFlagSet(parseSource, source)) \ state->name = JsonUtils::GetValueForKey>(root, key); MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) @@ -236,6 +264,10 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { { auto state = _state.lock_shared(); + + // GH#11222: We only write properties that are of the same type (Local + // or Shared) which we requested. If we didn't want to serialize this + // type of property, just skip it. #define MTSM_APPLICATION_STATE_GEN(source, type, name, key, ...) \ if (WI_IsFlagSet(parseSource, source)) \ JsonUtils::SetValueForKey(root, key, state->name);