From c5c15e86f3094593fff81c38b02edf4c104cb6c9 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 17 Mar 2023 17:01:37 -0500 Subject: [PATCH] Clearly differentiate running elevated vs. drag/drop breaking (#14946) Credit where credit is due - @jboelter did literally all the hard work. I just separated this out to two elements: * Are we running elevated? * Can we drag drop? As we learned in #7754, the builtin administrator _can_ drag drop. But critically, they are also running as admin! The way we had this logic before, we're treat them as unelevated, because we had been overloading the meaning here. This splits these into two separate functions. Comes with the added benefit of re-adding the elevation shield to the Terminal window for users with UAC disabled (which was missing before) (and can _still_ be disabled). Closes #13928 Tested on a Win10 VM with `EnableLua=0` --- src/cascadia/TerminalApp/AppLogic.cpp | 13 ++++- src/cascadia/TerminalApp/AppLogic.h | 4 +- src/cascadia/TerminalApp/AppLogic.idl | 3 +- src/cascadia/TerminalApp/TerminalPage.cpp | 55 +++++++++---------- src/cascadia/TerminalApp/TerminalPage.h | 3 +- src/cascadia/TerminalApp/TerminalWindow.cpp | 29 ---------- src/cascadia/TerminalApp/TerminalWindow.h | 1 - src/cascadia/TerminalApp/TerminalWindow.idl | 2 - .../ApplicationState.cpp | 8 +-- src/types/inc/utils.hpp | 3 +- src/types/utils.cpp | 35 +++++++++++- 11 files changed, 84 insertions(+), 72 deletions(-) diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index c8677eb7f8..9cb4e9de1b 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -132,7 +132,12 @@ namespace winrt::TerminalApp::implementation // cause you to chase down the rabbit hole of "why is App not // registered?" when it definitely is. - _isElevated = ::Microsoft::Console::Utils::IsElevated(); + // The TerminalPage has to be constructed during our construction, to + // make sure that there's a terminal page for callers of + // SetTitleBarContent + + _isElevated = ::Microsoft::Console::Utils::IsRunningElevated(); + _canDragDrop = ::Microsoft::Console::Utils::CanUwpDragDrop(); _reloadSettings = std::make_shared>(winrt::Windows::System::DispatcherQueue::GetForCurrentThread(), std::chrono::milliseconds(100), [weakSelf = get_weak()]() { if (auto self{ weakSelf.get() }) @@ -163,10 +168,14 @@ namespace winrt::TerminalApp::implementation // - - reports internal state // Return Value: // - True if elevated, false otherwise. - bool AppLogic::IsElevated() const noexcept + bool AppLogic::IsRunningElevated() const noexcept { return _isElevated; } + bool AppLogic::CanDragDrop() const noexcept + { + return _canDragDrop; + } // Method Description: // - Called by UWP context invoker to let us know that we may have to change some of our behaviors diff --git a/src/cascadia/TerminalApp/AppLogic.h b/src/cascadia/TerminalApp/AppLogic.h index 821ec0f5f1..c7eb2eaee9 100644 --- a/src/cascadia/TerminalApp/AppLogic.h +++ b/src/cascadia/TerminalApp/AppLogic.h @@ -49,7 +49,8 @@ namespace winrt::TerminalApp::implementation void Create(); bool IsUwp() const noexcept; void RunAsUwp(); - bool IsElevated() const noexcept; + bool IsRunningElevated() const noexcept; + bool CanDragDrop() const noexcept; void ReloadSettings(); bool HasSettingsStartupActions() const noexcept; @@ -72,6 +73,7 @@ namespace winrt::TerminalApp::implementation private: bool _isUwp{ false }; bool _isElevated{ false }; + bool _canDragDrop{ false }; Microsoft::Terminal::Settings::Model::CascadiaSettings _settings{ nullptr }; diff --git a/src/cascadia/TerminalApp/AppLogic.idl b/src/cascadia/TerminalApp/AppLogic.idl index a5db001de6..3091847cf8 100644 --- a/src/cascadia/TerminalApp/AppLogic.idl +++ b/src/cascadia/TerminalApp/AppLogic.idl @@ -25,7 +25,8 @@ namespace TerminalApp Boolean IsUwp(); void RunAsUwp(); - Boolean IsElevated(); + Boolean IsRunningElevated(); + Boolean CanDragDrop(); Boolean HasSettingsStartupActions(); diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 96e7f24087..fa621c0561 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -153,27 +153,26 @@ namespace winrt::TerminalApp::implementation _systemRowsToScroll = _ReadSystemRowsToScroll(); } - bool TerminalPage::IsElevated() const noexcept + bool TerminalPage::IsRunningElevated() const noexcept { - // use C++11 magic statics to make sure we only do this once. - // This won't change over the lifetime of the application - - static const auto isElevated = []() { - // *** THIS IS A SINGLETON *** - auto result = false; - - // GH#2455 - Make sure to try/catch calls to Application::Current, - // because that _won't_ be an instance of TerminalApp::App in the - // LocalTests - try - { - result = ::winrt::Windows::UI::Xaml::Application::Current().as<::winrt::TerminalApp::App>().Logic().IsElevated(); - } - CATCH_LOG(); - return result; - }(); - - return isElevated; + // GH#2455 - Make sure to try/catch calls to Application::Current, + // because that _won't_ be an instance of TerminalApp::App in the + // LocalTests + try + { + return Application::Current().as().Logic().IsRunningElevated(); + } + CATCH_LOG(); + return false; + } + bool TerminalPage::CanDragDrop() const noexcept + { + try + { + return Application::Current().as().Logic().CanDragDrop(); + } + CATCH_LOG(); + return true; } void TerminalPage::Create() @@ -186,11 +185,11 @@ namespace winrt::TerminalApp::implementation _tabView = _tabRow.TabView(); _rearranging = false; - const auto isElevated = IsElevated(); + const auto canDragDrop = CanDragDrop(); _tabRow.PointerMoved({ get_weak(), &TerminalPage::_RestorePointerCursorHandler }); - _tabView.CanReorderTabs(!isElevated); - _tabView.CanDragTabs(!isElevated); + _tabView.CanReorderTabs(canDragDrop); + _tabView.CanDragTabs(canDragDrop); _tabView.TabDragStarting({ get_weak(), &TerminalPage::_TabDragStarted }); _tabView.TabDragCompleted({ get_weak(), &TerminalPage::_TabDragCompleted }); @@ -312,7 +311,7 @@ namespace winrt::TerminalApp::implementation // Setup mouse vanish attributes SystemParametersInfoW(SPI_GETMOUSEVANISH, 0, &_shouldMouseVanish, false); - _tabRow.ShowElevationShield(IsElevated() && _settings.GlobalSettings().ShowAdminShield()); + _tabRow.ShowElevationShield(IsRunningElevated() && _settings.GlobalSettings().ShowAdminShield()); // Store cursor, so we can restore it, e.g., after mouse vanishing // (we'll need to adapt this logic once we make cursor context aware) @@ -339,7 +338,7 @@ namespace winrt::TerminalApp::implementation // GH#12267: Don't forget about defterm handoff here. If we're being // created for embedding, then _yea_, we don't need to handoff to an // elevated window. - if (!_startupActions || IsElevated() || _shouldStartInboundListener || _startupActions.Size() == 0) + if (!_startupActions || IsRunningElevated() || _shouldStartInboundListener || _startupActions.Size() == 0) { // there aren't startup actions, or we're elevated. In that case, go for it. return false; @@ -1128,7 +1127,7 @@ namespace winrt::TerminalApp::implementation WI_IsFlagSet(lAltState, CoreVirtualKeyStates::Down) && WI_IsFlagSet(rAltState, CoreVirtualKeyStates::Down); - const auto dispatchToElevatedWindow = ctrlPressed && !IsElevated(); + const auto dispatchToElevatedWindow = ctrlPressed && !IsRunningElevated(); if ((shiftPressed || dispatchToElevatedWindow) && !debugTap) { @@ -2894,7 +2893,7 @@ namespace winrt::TerminalApp::implementation // want to create an animation. WUX::Media::Animation::Timeline::AllowDependentAnimations(!_settings.GlobalSettings().DisableAnimations()); - _tabRow.ShowElevationShield(IsElevated() && _settings.GlobalSettings().ShowAdminShield()); + _tabRow.ShowElevationShield(IsRunningElevated() && _settings.GlobalSettings().ShowAdminShield()); Media::SolidColorBrush transparent{ Windows::UI::Colors::Transparent() }; _tabView.Background(transparent); @@ -4034,7 +4033,7 @@ namespace winrt::TerminalApp::implementation { // Try to handle auto-elevation const auto requestedElevation = controlSettings.DefaultSettings().Elevate(); - const auto currentlyElevated = IsElevated(); + const auto currentlyElevated = IsRunningElevated(); // We aren't elevated, but we want to be. if (requestedElevation && !currentlyElevated) diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index f141d46025..ef15498132 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -126,7 +126,8 @@ namespace winrt::TerminalApp::implementation TerminalApp::WindowProperties WindowProperties() const noexcept { return _WindowProperties; }; - bool IsElevated() const noexcept; + bool CanDragDrop() const noexcept; + bool IsRunningElevated() const noexcept; void OpenSettingsUI(); void WindowActivated(const bool activated); diff --git a/src/cascadia/TerminalApp/TerminalWindow.cpp b/src/cascadia/TerminalApp/TerminalWindow.cpp index eb0ef7c78b..1041f17630 100644 --- a/src/cascadia/TerminalApp/TerminalWindow.cpp +++ b/src/cascadia/TerminalApp/TerminalWindow.cpp @@ -201,35 +201,6 @@ namespace winrt::TerminalApp::implementation return isUwp; } - // Method Description: - // - Called around the codebase to discover if Terminal is running elevated - // Arguments: - // - - reports internal state - // Return Value: - // - True if elevated, false otherwise. - bool TerminalWindow::IsElevated() const noexcept - { - // use C++11 magic statics to make sure we only do this once. - // This won't change over the lifetime of the application - - static const auto isElevated = []() { - // *** THIS IS A SINGLETON *** - auto result = false; - - // GH#2455 - Make sure to try/catch calls to Application::Current, - // because that _won't_ be an instance of TerminalApp::App in the - // LocalTests - try - { - result = ::winrt::Windows::UI::Xaml::Application::Current().as<::winrt::TerminalApp::App>().Logic().IsElevated(); - } - CATCH_LOG(); - return result; - }(); - - return isElevated; - } - // Method Description: // - Build the UI for the terminal app. Before this method is called, it // should not be assumed that the TerminalApp is usable. The Settings diff --git a/src/cascadia/TerminalApp/TerminalWindow.h b/src/cascadia/TerminalApp/TerminalWindow.h index 1a44601076..0f9b086591 100644 --- a/src/cascadia/TerminalApp/TerminalWindow.h +++ b/src/cascadia/TerminalApp/TerminalWindow.h @@ -65,7 +65,6 @@ namespace winrt::TerminalApp::implementation void Create(); bool IsUwp() const noexcept; - bool IsElevated() const noexcept; void Quit(); diff --git a/src/cascadia/TerminalApp/TerminalWindow.idl b/src/cascadia/TerminalApp/TerminalWindow.idl index a945f88e00..0dc6550e7b 100644 --- a/src/cascadia/TerminalApp/TerminalWindow.idl +++ b/src/cascadia/TerminalApp/TerminalWindow.idl @@ -51,8 +51,6 @@ namespace TerminalApp // registered?" when it definitely is. void Create(); - Boolean IsElevated(); - Boolean HasCommandlineArguments(); Int32 SetStartupCommandline(String[] commands); diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index 59ebb0ccd0..6f684f91d6 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -177,7 +177,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // 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()) + if (::Microsoft::Console::Utils::IsRunningElevated()) { // Only load shared properties if we're elevated FromJson(root, FileSource::Shared); @@ -225,7 +225,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // // After that's done, we'll write our Local properties into // elevated-state.json. - if (::Microsoft::Console::Utils::IsElevated()) + if (::Microsoft::Console::Utils::IsRunningElevated()) { std::string errs; std::unique_ptr reader{ Json::CharReaderBuilder{}.newCharReader() }; @@ -353,7 +353,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // permissions, so we don't potentially read malicious data. std::optional ApplicationState::_readLocalContents() const { - return ::Microsoft::Console::Utils::IsElevated() ? + return ::Microsoft::Console::Utils::IsRunningElevated() ? ReadUTF8FileIfExists(_elevatedPath, true) : ReadUTF8FileIfExists(_sharedPath, false); } @@ -374,7 +374,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // will atomically write to `user-state.json` void ApplicationState::_writeLocalContents(const std::string_view content) const { - if (::Microsoft::Console::Utils::IsElevated()) + if (::Microsoft::Console::Utils::IsRunningElevated()) { // DON'T use WriteUTF8FileAtomic, which will write to a temporary file // then rename that file to the final filename. That actually lets us diff --git a/src/types/inc/utils.hpp b/src/types/inc/utils.hpp index 3dbe7928c9..45e16a049d 100644 --- a/src/types/inc/utils.hpp +++ b/src/types/inc/utils.hpp @@ -96,7 +96,8 @@ namespace Microsoft::Console::Utils GUID CreateV5Uuid(const GUID& namespaceGuid, const std::span name); - bool IsElevated(); + bool CanUwpDragDrop(); + bool IsRunningElevated(); // This function is only ever used by the ConPTY connection in // TerminalConnection. However, that library does not have a good system of diff --git a/src/types/utils.cpp b/src/types/utils.cpp index dac6212c6e..94813bfc96 100644 --- a/src/types/utils.cpp +++ b/src/types/utils.cpp @@ -653,9 +653,18 @@ GUID Utils::CreateV5Uuid(const GUID& namespaceGuid, const std::span they _can_ drag drop } + // If they are running admin, they cannot drag drop. + return wil::test_token_membership(nullptr, SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS); + } + catch (...) + { + LOG_CAUGHT_EXCEPTION(); + // This failed? That's very peculiar indeed. Let's err on the side + // of "drag drop is broken", just in case. + return true; + } + }(); + + return !isDragDropBroken; +} + +// See CanUwpDragDrop, GH#13928 for why this is different. +bool Utils::IsRunningElevated() +{ + static auto isElevated = []() { + try + { return wil::test_token_membership(nullptr, SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS); } catch (...)