[FileLocksmith]Query system processes if elevated #21688

jaimecbernardo · 2022-11-03T17:01:40Z

Summary of the Pull Request

File Locksmith can't see processes that belong to the system user.
This PR allows that when running elevated.

PR Checklist

Detailed Description of the Pull Request / Additional comments

When running elevated, ask for SeDebugPrivilege for the File Locksmith process in order to be able to view files used by system processes.
The user of each process is now sent with the process results instead of queried from the UI, so moved this function accordingly and removed the interop function from the API.
Add a warning in UI when listing a system process.
Also make the list of files selected by the context menu selectable as a minor fix.

Validation Steps Performed

Tried it on the Windows folder running as elevated and verified the system processes are shown.

jaimecbernardo · 2022-11-03T17:03:59Z

@niels9001 , is the added Glyph="" (warning) icon something we can use safely here? (wondering because of the asian fonts conflict)

yuyoyuppe

Tested locally, LGTM. Added a suggestion

src/modules/FileLocksmith/FileLocksmithLibInterop/NtdllExtensions.cpp

yuyoyuppe · 2022-11-03T18:07:17Z

src/modules/FileLocksmith/FileLocksmithUI/Converters/UserToSystemWarningVisibilityConverter.cs

    {
        public object Convert(object value, Type targetType, object parameter, string language)
        {
-            return NativeMethods.PidToUser((uint)value);
+            string user = ((string)value).ToUpperInvariant().TrimEnd('\0').Trim();


I believe we no longer have to trim \0

htcfreek · 2022-11-03T18:23:50Z

Should we show a warning bar (even if results are empty) when running as normal user that maybe some results are missing because of not enough permission?
Can we add a warning/confirmation message that is shown when you close the process owned by a different user or the system/nt-service, ... user?
Small bug also existing in v0.64.0: If you restart it as admin and the uac is disabled the "run elevated" icon isn't shown after restart. It should still be shown if you don't have admin permission.

htcfreek · 2022-11-03T18:41:48Z

src/modules/FileLocksmith/FileLocksmithUI/Views/MainPage.xaml

+                                        <FontIcon
+                                            Margin="0,0,8,0"
+                                            Glyph="&#xE7BA;"
+                                            Foreground="Yellow"


Is this visible in light mode? If not we should bind against a theme resource for the InfoBarIconBackgrond. You can look at the PT Run plugin list code for an example on how to do it.

Good catch!

Foreground="{ThemeResource InfoBarWarningSeverityIconBackground}"

Thank you! :) Very helpful!

jaimecbernardo · 2022-11-03T18:56:31Z

Should we show a warning bar (even if results are empty) when running as normal user that maybe some results are missing because of not enough permission?

Can we add a warning/confirmation message that is shown when you close the process owned by a different user or the system/nt-service, ... user?

Those are good ideas. Can you please create a UI improvement issue for 1 and 2? Those are a bit more code involved and should be separated.

jaimecbernardo · 2022-11-03T18:58:12Z

Small bug also existing in v0.64.0: If you restart it as admin and the uac is disabled the "run elevated" icon isn't shown after restart. It should still be shown if you don't have admin permission.

Yeah, the elevated detection logic could be better.

htcfreek · 2022-11-03T21:16:35Z

@jaimecbernardo
I will open the ui improvements issues tomorrow or on Sunday. Should I open an issue for the elevation logic improvement too?

jaimecbernardo · 2022-11-03T21:20:49Z

@jaimecbernardo I will open the ui improvements issues tomorrow or on Sunday. Should I open an issue for the elevation logic improvement too?

I'll try to include that in this PR, since it also uses elevation. Thank you!

…ons.cpp Co-authored-by: Andrey Nekrasov <yuyoyuppe@users.noreply.github.com>

jaimecbernardo · 2022-11-04T15:46:19Z

Hi, I've included the requested changes to the username function, added proper elevation detection similar to our other C++ utils and used a theme approppriate color. Thanks for your reviews and help!

yuyoyuppe

Nice!

htcfreek · 2022-11-06T12:10:42Z

Should we show a warning bar (even if results are empty) when running as normal user that maybe some results are missing because of not enough permission?

Can we add a warning/confirmation message that is shown when you close the process owned by a different user or the system/nt-service, ... user?

Those are good ideas. Can you please create a UI improvement issue for 1 and 2? Those are a bit more code involved and should be separated.

@jaimecbernardo
I have opened the issues.

* [FileLocksmith]Query system processes if elevated * Show warning if user is a system user * Make text in the file list selectable * Update src/modules/FileLocksmith/FileLocksmithLibInterop/NtdllExtensions.cpp Co-authored-by: Andrey Nekrasov <yuyoyuppe@users.noreply.github.com> * Trim \0 no longer required * Correct elevation detection logic * Use theme approppriate colors Co-authored-by: Andrey Nekrasov <yuyoyuppe@users.noreply.github.com>

jaimecbernardo added 3 commits November 3, 2022 16:51

[FileLocksmith]Query system processes if elevated

b0f9601

Show warning if user is a system user

b53f461

Make text in the file list selectable

fd72710

jaimecbernardo requested a review from yuyoyuppe November 3, 2022 17:01

jaimecbernardo mentioned this pull request Nov 3, 2022

File Locksmith does not recognize files locked by the system user #21661

Closed

yuyoyuppe reviewed Nov 3, 2022

View reviewed changes

htcfreek reviewed Nov 3, 2022

View reviewed changes

jaimecbernardo and others added 4 commits November 4, 2022 09:47

Update src/modules/FileLocksmith/FileLocksmithLibInterop/NtdllExtensi…

f320e48

…ons.cpp Co-authored-by: Andrey Nekrasov <yuyoyuppe@users.noreply.github.com>

Trim \0 no longer required

150534a

Correct elevation detection logic

78c2d20

Use theme approppriate colors

02800c9

jaimecbernardo requested review from yuyoyuppe and htcfreek November 4, 2022 15:45

yuyoyuppe approved these changes Nov 4, 2022

View reviewed changes

jaimecbernardo mentioned this pull request Nov 4, 2022

[FLS] UI fixes #21732

Merged

11 tasks

jaimecbernardo merged commit 66754af into main Nov 4, 2022

crutkas deleted the dev/jaime/file-locksmith-see-system-processes branch November 4, 2022 17:16

jaimecbernardo mentioned this pull request Nov 5, 2022

[File Locksmith] does not identify locks from other logged in users #21776

Closed

This was referenced Nov 6, 2022

[File Locksmith] Show warning bar when not running as admin #21805

Open

[File Locksmith] Request confirmation on closing processes #21807

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FileLocksmith]Query system processes if elevated #21688

[FileLocksmith]Query system processes if elevated #21688

jaimecbernardo commented Nov 3, 2022

jaimecbernardo commented Nov 3, 2022 •

edited

Loading

yuyoyuppe left a comment

yuyoyuppe Nov 3, 2022

htcfreek commented Nov 3, 2022

htcfreek Nov 3, 2022

jaimecbernardo Nov 3, 2022

htcfreek Nov 4, 2022

jaimecbernardo Nov 4, 2022

jaimecbernardo commented Nov 3, 2022

jaimecbernardo commented Nov 3, 2022

htcfreek commented Nov 3, 2022

jaimecbernardo commented Nov 3, 2022

jaimecbernardo commented Nov 4, 2022

yuyoyuppe left a comment

htcfreek commented Nov 6, 2022

[FileLocksmith]Query system processes if elevated #21688

[FileLocksmith]Query system processes if elevated #21688

Conversation

jaimecbernardo commented Nov 3, 2022

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

jaimecbernardo commented Nov 3, 2022 • edited Loading

yuyoyuppe left a comment

Choose a reason for hiding this comment

yuyoyuppe Nov 3, 2022

Choose a reason for hiding this comment

htcfreek commented Nov 3, 2022

htcfreek Nov 3, 2022

Choose a reason for hiding this comment

jaimecbernardo Nov 3, 2022

Choose a reason for hiding this comment

htcfreek Nov 4, 2022

Choose a reason for hiding this comment

jaimecbernardo Nov 4, 2022

Choose a reason for hiding this comment

jaimecbernardo commented Nov 3, 2022

jaimecbernardo commented Nov 3, 2022

htcfreek commented Nov 3, 2022

jaimecbernardo commented Nov 3, 2022

jaimecbernardo commented Nov 4, 2022

yuyoyuppe left a comment

Choose a reason for hiding this comment

htcfreek commented Nov 6, 2022

jaimecbernardo commented Nov 3, 2022 •

edited

Loading