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
Add a file for storing elevated-only state #11222
Merged
Merged
Changes from 38 commits
Commits
Show all changes
43 commits
Select commit
Hold shift + click to select a range
6be6972
This is everything from dev/migrie/f/non-terminal-content-elevation-w…
zadjii-msft c106f64
:facepalm:
zadjii-msft 51e0473
minor nits
zadjii-msft da0cc7b
todo! in the code
zadjii-msft 4e69a32
bunch of new allowed words
zadjii-msft 6757452
fix the tests
zadjii-msft a4acdeb
blindly remove ElevatedState
zadjii-msft 5685063
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft 9b3b9e0
remove baseapplicationstate and just merge it back in
zadjii-msft 507a48e
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft b1b1bef
this is a crazy idea that I hate but gotta start somewhere
zadjii-msft 7e2e4ea
I think this works ALMOST as I want
zadjii-msft f3738f5
Move window state, approvedCommandlines into `user-state.json`
zadjii-msft a6e044d
pr nits
zadjii-msft eee657b
fix hot reloading for this file
zadjii-msft 02e9871
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft 5ff9a24
okay so I guess that's not a word
zadjii-msft 620ee30
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft d053f6c
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft 866832b
This seemingly works the way I'd expect, going to merge into the warn…
zadjii-msft b4e0496
cleanup
zadjii-msft 9ff2775
Merge branch 'dev/migrie/f/just-trying-something-on-just-elevated-sta…
zadjii-msft 48b20de
Add some logging. Can't seem to get the crash to repro?
zadjii-msft 3c1866a
Merge remote-tracking branch 'origin/dev/migrie/b/1.12-crash-on-exit'…
zadjii-msft aea3752
we want this
zadjii-msft 0e7217d
Merge commit 'aea37520b3cdb1c1752a6c8e0ff598991518ce28' into dev/migr…
zadjii-msft ae99ce9
teh
zadjii-msft bf3c6e7
spel is hard
zadjii-msft faa06f8
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft 945c81d
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft 9b4ae9e
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft 4976a09
this is a collection of the trivial nits while I wait on a reply to t…
zadjii-msft fd849a5
trying to do the thing eryksun mentioned. This seems to actually work…
zadjii-msft 25b2675
this works really quite well
zadjii-msft b212871
this is the right way to initialize the unique_hlocal_security_descri…
zadjii-msft 4f16dfb
many comments
zadjii-msft a93d17e
I believe this is the rest of the comments
zadjii-msft ce6a9c5
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft db9cbf3
spell
zadjii-msft 08cbd16
the last of it?
zadjii-msft 999f21f
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft 33e96e7
mitigate a TOCTOU
zadjii-msft 7f03d4d
dustins nits
zadjii-msft File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
admins | ||
apc | ||
calt | ||
ccmp | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,8 @@ | |
#include <WtExeUtils.h> | ||
#include <wil/token_helpers.h > | ||
|
||
#include "../../types/inc/utils.hpp" | ||
|
||
using namespace winrt::Windows::ApplicationModel; | ||
using namespace winrt::Windows::ApplicationModel::DataTransfer; | ||
using namespace winrt::Windows::UI::Xaml; | ||
|
@@ -131,38 +133,6 @@ static Documents::Run _BuildErrorRun(const winrt::hstring& text, const ResourceD | |
return textRun; | ||
} | ||
|
||
// Method Description: | ||
// - Returns whether the user is either a member of the Administrators group or | ||
// is currently elevated. | ||
// - This will return **FALSE** if the user has UAC disabled entirely, because | ||
// there's no separation of power between the user and an admin in that case. | ||
// Return Value: | ||
// - true if the user is an administrator | ||
static bool _isUserAdmin() noexcept | ||
try | ||
{ | ||
wil::unique_handle processToken{ GetCurrentProcessToken() }; | ||
const auto elevationType = wil::get_token_information<TOKEN_ELEVATION_TYPE>(processToken.get()); | ||
const auto elevationState = wil::get_token_information<TOKEN_ELEVATION>(processToken.get()); | ||
if (elevationType == TokenElevationTypeDefault && elevationState.TokenIsElevated) | ||
{ | ||
// In this case, the user has UAC entirely disabled. This is sort of | ||
// weird, we treat this like the user isn't an admin at all. There's no | ||
// separation of powers, so the things we normally want to gate on | ||
// "having special powers" doesn't apply. | ||
// | ||
// See GH#7754, GH#11096 | ||
return false; | ||
} | ||
|
||
return wil::test_token_membership(nullptr, SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS); | ||
} | ||
catch (...) | ||
{ | ||
LOG_CAUGHT_EXCEPTION(); | ||
return false; | ||
} | ||
|
||
namespace winrt::TerminalApp::implementation | ||
{ | ||
// Function Description: | ||
|
@@ -214,7 +184,7 @@ namespace winrt::TerminalApp::implementation | |
// The TerminalPage has to be constructed during our construction, to | ||
// make sure that there's a terminal page for callers of | ||
// SetTitleBarContent | ||
_isElevated = _isUserAdmin(); | ||
_isElevated = ::Microsoft::Console::Utils::IsElevated(); | ||
_root = winrt::make_self<TerminalPage>(); | ||
|
||
_reloadSettings = std::make_shared<ThrottledFuncTrailing<>>(winrt::Windows::System::DispatcherQueue::GetForCurrentThread(), std::chrono::milliseconds(100), [weakSelf = get_weak()]() { | ||
|
@@ -910,8 +880,6 @@ namespace winrt::TerminalApp::implementation | |
void AppLogic::_RegisterSettingsChange() | ||
{ | ||
const std::filesystem::path settingsPath{ std::wstring_view{ CascadiaSettings::SettingsPath() } }; | ||
const std::filesystem::path statePath{ std::wstring_view{ ApplicationState::SharedInstance().FilePath() } }; | ||
|
||
_reader.create( | ||
settingsPath.parent_path().c_str(), | ||
false, | ||
|
@@ -920,14 +888,29 @@ namespace winrt::TerminalApp::implementation | |
// editors, who will write a temp file, then rename it to be the | ||
// actual file you wrote. So listen for that too. | ||
wil::FolderChangeEvents::FileName | wil::FolderChangeEvents::LastWriteTime, | ||
[this, settingsBasename = settingsPath.filename(), stateBasename = statePath.filename()](wil::FolderChangeEvent, PCWSTR fileModified) { | ||
const auto modifiedBasename = std::filesystem::path{ fileModified }.filename(); | ||
[this, settingsBasename = settingsPath.filename()](wil::FolderChangeEvent, PCWSTR fileModified) { | ||
// DO NOT create a static reference to ApplicationState::SharedInstance here. | ||
// | ||
// ApplicationState::SharedInstance already caches its own | ||
// static ref. If _we_ keep a static ref to the member in | ||
// AppState, then our reference will keep ApplicationState alive | ||
// after the `ActionToStringMap` gets cleaned up. Then, when we | ||
// try to persist the actions in the window state, we won't be | ||
// able to. We'll try to look up the action and the map just | ||
// won't exist. We'll explode, even though the Terminal is | ||
// tearing down anyways. So we'll just die, but still inboke | ||
// WinDBG's post-mortem debugger, who won't be able to attach to | ||
// the process that's already exiting. | ||
// | ||
// So DON'T ~give a mouse a cookie~ take a static ref here. | ||
|
||
const winrt::hstring modifiedBasename{ std::filesystem::path{ fileModified }.filename().c_str() }; | ||
|
||
if (modifiedBasename == settingsBasename) | ||
{ | ||
_reloadSettings->Run(); | ||
} | ||
else if (modifiedBasename == stateBasename) | ||
else if (ApplicationState::SharedInstance().IsStatePath(modifiedBasename)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need to construct a hstring to pass it to a COM call. All std strings implicitly convert to |
||
{ | ||
_reloadState(); | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor performance concern -- this allocates an hstring (another copy) for every file that is ever changed in this folder. Is there something we can do to not do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually we'll have to pass the path to
ApplicationState::IsStatePath(...)
, which would need to take thehstring
, since that lives in TSM.dll.We could break the encapsulation and have
AppLogic
know what the app state paths are. Alternatively, we could not really worry about someone writing to a bunch of files in this directory in rapid succession like a madman 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use
.native()
more liberally onstd::filesystem::path
s. It returns a constant reference to the underlying wide string for free.