Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clearly differentiate running elevated vs. drag/drop breaking #14946

Merged
merged 7 commits into from
Mar 17, 2023
13 changes: 11 additions & 2 deletions src/cascadia/TerminalApp/AppLogic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Comment on lines +139 to +140
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this kinda isn't relevant to this PR but why do these two members exist? We're using magic statics in Utils, so what's the point in caching these values?


_reloadSettings = std::make_shared<ThrottledFuncTrailing<>>(winrt::Windows::System::DispatcherQueue::GetForCurrentThread(), std::chrono::milliseconds(100), [weakSelf = get_weak()]() {
if (auto self{ weakSelf.get() })
Expand Down Expand Up @@ -163,10 +168,14 @@ namespace winrt::TerminalApp::implementation
// - <none> - 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
Expand Down
4 changes: 3 additions & 1 deletion src/cascadia/TerminalApp/AppLogic.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 };

Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalApp/AppLogic.idl
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ namespace TerminalApp

Boolean IsUwp();
void RunAsUwp();
Boolean IsElevated();
Boolean IsRunningElevated();
Boolean CanDragDrop();

Boolean HasSettingsStartupActions();

Expand Down
55 changes: 27 additions & 28 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<TerminalApp::App>().Logic().IsRunningElevated();
}
CATCH_LOG();
return false;
}
bool TerminalPage::CanDragDrop() const noexcept
{
try
{
return Application::Current().as<TerminalApp::App>().Logic().CanDragDrop();
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
}
CATCH_LOG();
return true;
}

void TerminalPage::Create()
Expand All @@ -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 });

Expand Down Expand Up @@ -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)
Expand All @@ -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;
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -2876,7 +2875,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);
Expand Down Expand Up @@ -4016,7 +4015,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)
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalApp/TerminalPage.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
29 changes: 0 additions & 29 deletions src/cascadia/TerminalApp/TerminalWindow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -201,35 +201,6 @@ namespace winrt::TerminalApp::implementation
return isUwp;
}

// Method Description:
// - Called around the codebase to discover if Terminal is running elevated
// Arguments:
// - <none> - 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
Expand Down
1 change: 0 additions & 1 deletion src/cascadia/TerminalApp/TerminalWindow.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ namespace winrt::TerminalApp::implementation
void Create();

bool IsUwp() const noexcept;
bool IsElevated() const noexcept;

void Quit();

Expand Down
2 changes: 0 additions & 2 deletions src/cascadia/TerminalApp/TerminalWindow.idl
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,6 @@ namespace TerminalApp
// registered?" when it definitely is.
void Create();

Boolean IsElevated();

Boolean HasCommandlineArguments();

Int32 SetStartupCommandline(String[] commands);
Expand Down
8 changes: 4 additions & 4 deletions src/cascadia/TerminalSettingsModel/ApplicationState.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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<Json::CharReader> reader{ Json::CharReaderBuilder{}.newCharReader() };
Expand Down Expand Up @@ -353,7 +353,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
// permissions, so we don't potentially read malicious data.
std::optional<std::string> ApplicationState::_readLocalContents() const
{
return ::Microsoft::Console::Utils::IsElevated() ?
return ::Microsoft::Console::Utils::IsRunningElevated() ?
ReadUTF8FileIfExists(_elevatedPath, true) :
ReadUTF8FileIfExists(_sharedPath, false);
}
Expand All @@ -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
Expand Down
3 changes: 2 additions & 1 deletion src/types/inc/utils.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@ namespace Microsoft::Console::Utils

GUID CreateV5Uuid(const GUID& namespaceGuid, const std::span<const std::byte> 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
Expand Down
35 changes: 33 additions & 2 deletions src/types/utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -653,9 +653,18 @@ GUID Utils::CreateV5Uuid(const GUID& namespaceGuid, const std::span<const std::b
return EndianSwap(newGuid);
}

bool Utils::IsElevated()
// * Elevated users cannot use the modern drag drop experience. This is
// specifically normal users running the Terminal as admin
// * The Default Administrator, who does not have a split token, CAN drag drop
// perfectly fine. So in that case, we want to return false.
// * This has to be kept separate from IsRunningElevated, which is exclusively
// used for "is this instance running as admin".
bool Utils::CanUwpDragDrop()
{
static auto isElevated = []() {
// There's a lot of wacky double negatives here so that the logic is
// basically the same as IsRunningElevated, but the end result semantically
// makes sense as "CanDragDrop".
static auto isDragDropBroken = []() {
try
{
wil::unique_handle processToken{ GetCurrentProcessToken() };
Expand All @@ -670,8 +679,30 @@ bool Utils::IsElevated()
//
// See GH#7754, GH#11096
return false;
// drag drop is _not_ broken -> 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, you did the hard part of writing the logic. Why not remove the double negative by...

  • isDragDropBroken --> canDragDrop
  • L681 false --> true
  • L693 true --> false
  • L697 !isDragDropBroken --> canDragDrop

(pipe the ! into the returned results, basically)

}

// 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 (...)
Expand Down