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

Add a file for storing elevated-only state #11222

Merged
merged 43 commits into from Nov 13, 2021
Merged
Show file tree
Hide file tree
Changes from 28 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 Sep 14, 2021
c106f64
:facepalm:
zadjii-msft Sep 14, 2021
51e0473
minor nits
zadjii-msft Sep 14, 2021
da0cc7b
todo! in the code
zadjii-msft Sep 14, 2021
4e69a32
bunch of new allowed words
zadjii-msft Sep 14, 2021
6757452
fix the tests
zadjii-msft Sep 14, 2021
a4acdeb
blindly remove ElevatedState
zadjii-msft Sep 20, 2021
5685063
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Sep 20, 2021
9b3b9e0
remove baseapplicationstate and just merge it back in
zadjii-msft Sep 20, 2021
507a48e
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Sep 22, 2021
b1b1bef
this is a crazy idea that I hate but gotta start somewhere
zadjii-msft Sep 22, 2021
7e2e4ea
I think this works ALMOST as I want
zadjii-msft Sep 22, 2021
f3738f5
Move window state, approvedCommandlines into `user-state.json`
zadjii-msft Sep 22, 2021
a6e044d
pr nits
zadjii-msft Sep 22, 2021
eee657b
fix hot reloading for this file
zadjii-msft Sep 22, 2021
02e9871
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Sep 22, 2021
5ff9a24
okay so I guess that's not a word
zadjii-msft Sep 22, 2021
620ee30
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Sep 28, 2021
d053f6c
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Sep 30, 2021
866832b
This seemingly works the way I'd expect, going to merge into the warn…
zadjii-msft Sep 30, 2021
b4e0496
cleanup
zadjii-msft Sep 30, 2021
9ff2775
Merge branch 'dev/migrie/f/just-trying-something-on-just-elevated-sta…
zadjii-msft Sep 30, 2021
48b20de
Add some logging. Can't seem to get the crash to repro?
zadjii-msft Sep 30, 2021
3c1866a
Merge remote-tracking branch 'origin/dev/migrie/b/1.12-crash-on-exit'…
zadjii-msft Sep 30, 2021
aea3752
we want this
zadjii-msft Sep 30, 2021
0e7217d
Merge commit 'aea37520b3cdb1c1752a6c8e0ff598991518ce28' into dev/migr…
zadjii-msft Sep 30, 2021
ae99ce9
teh
zadjii-msft Sep 30, 2021
bf3c6e7
spel is hard
zadjii-msft Sep 30, 2021
faa06f8
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Oct 7, 2021
945c81d
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Oct 27, 2021
9b4ae9e
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Nov 1, 2021
4976a09
this is a collection of the trivial nits while I wait on a reply to t…
zadjii-msft Nov 4, 2021
fd849a5
trying to do the thing eryksun mentioned. This seems to actually work…
zadjii-msft Nov 8, 2021
25b2675
this works really quite well
zadjii-msft Nov 9, 2021
b212871
this is the right way to initialize the unique_hlocal_security_descri…
zadjii-msft Nov 9, 2021
4f16dfb
many comments
zadjii-msft Nov 9, 2021
a93d17e
I believe this is the rest of the comments
zadjii-msft Nov 9, 2021
ce6a9c5
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Nov 9, 2021
db9cbf3
spell
zadjii-msft Nov 10, 2021
08cbd16
the last of it?
zadjii-msft Nov 10, 2021
999f21f
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Nov 11, 2021
33e96e7
mitigate a TOCTOU
zadjii-msft Nov 11, 2021
7f03d4d
dustins nits
zadjii-msft Nov 11, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spelling/allow/allow.txt
@@ -1,3 +1,4 @@
admins
apc
calt
ccmp
Expand Down
6 changes: 6 additions & 0 deletions .github/actions/spelling/allow/apis.txt
@@ -1,5 +1,7 @@
ACCEPTFILES
ACCESSDENIED
acl
aclapi
alignas
alignof
APPLYTOSUBMENUS
Expand All @@ -19,6 +21,7 @@ comparand
cstdint
CXICON
CYICON
Dacl
dataobject
dcomp
DERR
Expand Down Expand Up @@ -114,9 +117,12 @@ OSVERSIONINFOEXW
otms
OUTLINETEXTMETRICW
overridable
PACL
PAGESCROLL
PEXPLICIT
PICKFOLDERS
pmr
ptstr
rcx
REGCLS
RETURNCMD
Expand Down
38 changes: 12 additions & 26 deletions src/cascadia/TerminalApp/AppLogic.cpp
Expand Up @@ -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;
Expand Down Expand Up @@ -139,28 +141,8 @@ static Documents::Run _BuildErrorRun(const winrt::hstring& text, const ResourceD
// 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;
return Microsoft::Console::Utils::IsElevated();
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
}

namespace winrt::TerminalApp::implementation
Expand Down Expand Up @@ -895,8 +877,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,
Expand All @@ -905,14 +885,20 @@ 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. See
// https://github.com/microsoft/terminal/pull/11222/files/9ff2775122a496fb8b1bcc7a0b83a64ce5b26c5f#r719627541
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
// for why. ApplicationState::SharedInstance already caches it's
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
// own static ref.

const winrt::hstring modifiedBasename{ std::filesystem::path{ fileModified }.filename().c_str() };
Copy link
Member

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?

Copy link
Member Author

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 the hstring, 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 🤔

Copy link
Member

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 on std::filesystem::paths. It returns a constant reference to the underlying wide string for free.


if (modifiedBasename == settingsBasename)
{
_reloadSettings->Run();
}
else if (modifiedBasename == stateBasename)
else if (ApplicationState::SharedInstance().IsStatePath(modifiedBasename))
Copy link
Member

Choose a reason for hiding this comment

The 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 winrt::param::hstring.

{
_reloadState();
}
Expand Down
3 changes: 0 additions & 3 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Expand Up @@ -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;
}

Expand Down