nullpo-head / WSL-Hello-sudo

Let's sudo by face recognition of Windows Hello on Windows Subsystem for Linux (WSL). It runs on both WSL 1 and WSL 2. This is a PAM module for Linux on WSL.
MIT License
1.21k stars 46 forks source link

feat: move hello to foreground once window exists #27

Closed jonaskuske closed 2 years ago

jonaskuske commented 2 years ago

This checks for the existence of the Windows Hello window (className Credential Dialog Xaml Host) and moves it to the foreground once it is found.

Not the optimal solution yet, as this happens only once per auth process. If the authentication fails (which sometimes happens for whatever reason, #1), the Hello prompt is recreated behind the Terminal and this time stays there as the FocusHelloWindow task has already returned.

A more comprehensive solution would require some form of AutomationEventHandler or usage of the Win32 window hook API, but that's nothing I can dig into right now and even without that, this is already a massive UX improvement.

fix #4

jonaskuske commented 2 years ago

Alright, I also got a version working that, once it moved the Hello window to the foreground, keeps checking every second if it's still in the foreground and if not puts it there again. But

What you you think @nullpo-head ?

nullpo-head commented 2 years ago

Hi Jonas! Thanks a lot for your big contribution again!!

I couldn't verify yet that it actually works when Hello errors because Hello auth didn't fail me yet today 😅

me neither, but I think we can get the feedbacks from the users in #4.

it's definitely quite "pushy" behavior.

I agree. Why don't we just setting it foreground only once? Do you have any cases where it is not enough?

jonaskuske commented 2 years ago

Do you have any cases where it is not enough?

The issue described in #1, which (unfortunately) happens quite regularly for me, too: You press "OK" on the Hello prompt, the window disappears but immediately re-appears again with an error message because the face unlock failed for whatever reason. (and keeps failing until you switch to PIN and back to Face Unlock once)

Without the continuous checks, the re-appearing Hello prompt appears in the background, so you got super confusing behavior: You click OK, Face Unlock disappears, you think you've authenticated successfully but instead the Face Unlock window just hid in the background, which takes some time to realize.

If instead we keep checking the window until the auth is complete, the re-appeared Unlock prompt and its error message are visible in the foreground and you know what's happening. And the only downside is that you can't intentionally let the pending unlock window linger in the background – for normal usage nothing changes, Hello appears, you hit OK/Cancel, done.

jonaskuske commented 2 years ago

Either way, any extension to fix the window in the foreground can and should be part of a separate PR, no need to discuss that before even the basic move-to-foreground functionality is added. This can be merged imo :)

But I'm afraid I can't make the same contribution to @BlackHoleFox's Rust implementation of the HelloAuthenticator as I really don't know a lot about Rust, and especially not how to achieve something akin to C# Tasks in Rust/Win32 :(

BlackHoleFox commented 2 years ago

Looks like your comment got duplicated, @jonaskuske :rip:

The changes here look somewhat simple so it wouldn't be hard to bring this to Rust. Biggest difference would be a background thread to replace the C# task, but that'd just be something like this. Everything else can come from the windows or windows-sys crate.

nullpo-head commented 2 years ago

You press "OK" on the Hello prompt, the window disappears but immediately re-appears again with an error message because the face unlock failed for whatever reason. (and keeps failing until you switch to PIN and back to Face Unlock once)

Without the continuous checks, the re-appearing Hello prompt appears in the background, so you got super confusing behavior:

I see. It makes sense.

nullpo-head commented 2 years ago

@jonaskuske It looks this does not work on Windows 11. Even if I move the Window Hello's dialog to the background, it was not moved to the foreground. Do you have any idea why? The window title seems the same, but I'm not sure. The output of winlister, though I'm not sure what is the best way to get the window's title.

Windows Security    Yes (984, 586)  (912, 801)  003D082E    Credential Dialog Xaml Host No  No  No  Yes 00003F24    00001210    C:\Windows\System32\CredentialUIBroker.exe  Microsoftョ Windowsョ Operating System    Credential Manager UI Host  10.0.22000.1 (WinBuild.160101.0800) Microsoft Corporation   
nullpo-head commented 2 years ago

BTW, could you rebase your branch to the latest master, please? I've made a change to CI so that it builds the release asset for pull requests for easier review.

jonaskuske commented 2 years ago

It looks this does not work on Windows 11

Oh, that's too bad! Unfortunately I don't have a W11 device to test on... Maybe now's the time to update my SB2? And yeah, the window className seems to be the same, so not sure what causes this.

But anyway, moving the window to the background manually won't work. I didn't commit the "pushy" implementation that stickies the window in the foreground (though I can if you want), it just moves it there once.

nullpo-head commented 2 years ago

I didn't commit the "pushy" implementation that stickies the window in the foreground (though I can if you want), it just moves it there once.

Ah, I missed that. I'm sorry for that. I now think the both of "pushy" and non-pushy make sense. So, if you're ok, I proceed with merging the current commit. If you want to bring back the behavior, I'll merge this after you make that change. I tested the change on my environment, too. Everything seems to work fine. Thanks for your contribution!

jonaskuske commented 2 years ago

See https://github.com/nullpo-head/WSL-Hello-sudo/pull/29, maybe we can get it to work on Windows 11 with one of the methods I tried there. If none of them work, sure, merge this one and we have Windows 10 support at least! :)

I now think the both of "pushy" and non-pushy make sense.

Also, ever since using my version here, I haven't seen the random Hello error a single time, so maybe moving the window to the foreground somehow fixes it and thus also makes the pushy behavior obsolete? 🙏 I'd keep it non-pushy for now and maybe add the pushy behavior later, if others still experience the problem even after this is merged.

nullpo-head commented 2 years ago

OK, let me merge this then. Thanks!

Luttik commented 2 years ago

I just tested locally and it seems to work 👍

danepowell commented 2 years ago

maybe we can get it to work on Windows 11 with one of the methods I tried there

This works for me on Windows 11. YMMV 😄

just4747 commented 2 years ago

Is there a way to implement this fix ona normal Win11 install or is this just a self-contained/testable type thing for special dev environments?