-
-
Notifications
You must be signed in to change notification settings - Fork 80
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
auth: Support parallel fingerprint auth #514
Conversation
Hello, tested this feature, here is what I think:
|
1dd8e93
to
3d5fd21
Compare
This is now fixed, it was a bad state during some refactoring.
I didn't want to mess with the input-field so as to not introduce edge cases with the password input/error handling, so you can use $FPRINTMESSAGE in a label.
I added an option general:fingerprint_ready_message that $FPRINTMESSAGE will display when ready for fingerprint input. |
Indeed the bug is fixed, thanks! About fingerprint_ready_message, I meant another thing, sorry for the unclear explanation. I've meant that when you touch the scanner, the system should immediately display some message like "scanning..." to indicate that scan is started. I think it would be useful for improving responsiveness as sometimes the scan can be longer than 500ms But it looks like fprint does not expose such interface, so it is probably impractical to do. fingerprint_ready_message is also useful, so I would leave it as is |
In my opinion it would make sens to be able to have the fp feedback in the input-field, if it is empty. But for that we might want to add an abstraction so that we don't have to add logic for an additional auth module in the input-field. That could be done later. |
There's a property on the device for whether the finger is present, so I changed to use the PropertiesChanged signal to listen for finger presence and display a message at that time (general:fingerprint_present_message). On my device, it's pretty snappy to read my fingerprint, so I don't like the extra messages. |
The rest looks pretty good. |
The PR needs changes to flake.nix to add extra buildInputs: sdbus-cpp and systemdLibs. |
I have did a quick check of this using fingerprint sensor in the ThinkPad T14s (AMD) Gen 3 and everything seem to be working well. Thanks |
974803e
to
c61aea2
Compare
@vaxerski I think this looks good |
b3128e9
to
fe10cda
Compare
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 nitpicks, rest is alr, needs a wiki mr
Opened hyprwm/hyprland-wiki#816 for the wiki |
Non-code related comment (apologies), but just so I understand, does this remove the need for pam_fprintd? Many users are using various workarounds (see #258), so they will have (Collapsed for readability, click headings to expand) pam_fprintd (installed with fprintd)
pam-fprint-grosshack
With this new parallel auth support, can we go back to using the default pam file? hyprlock default pam file
Once the PR is merged I will write some comments on #258 advising people on how to revert some of the workarounds so they can switch to using the built-in parallel auth functionality. Although, with some of the cursed methods people came up with (bash scripts, temp files, other abominations) I probably won't go that deep haha |
Yes, with the new parallel auth support, users should go back to the default pam file. |
apart from that lgtm |
I chose to use Fprint's dbus interface directly rather than going through pam (which uses Fprint's dbus interface) due to poor handling of system sleep somewhere between fprintd and pam. When preparing for sleep, fprintd puts the device to sleep, which causes VerifyStatus to emit with verify-unknown-error, which normally should be responded to by calling both Device.StopVerify and Device.Release (and this is what pam does). Unfortunately, if you try to release the device when the system is preparing for sleep, you'll get an error that the device is busy and then you can't can't claim or release the device for 30 seconds. pam also has a max timeout for pam_fprintd.so of 99 seconds, and so if we used pam, we'd have to deal with the timeouts and keep restarting the auth conversation. gdm/gnome-session lock seems to get around these issues by having a shutter on top of the lock screen that you have to interact with first that gives gnome-session a trigger to start fingerprint auth.
Quite a few fixes since last release. I think a hyprlock release is due. |
I need @fufexan to fix nix ci and then we can release |
man I love fufexan |
I chose to use Fprint's dbus interface directly rather than going through pam (which uses Fprint's dbus interface) due to poor handling of system sleep somewhere between fprintd and pam. When preparing for sleep, fprintd puts the device to sleep, which causes VerifyStatus to emit with verify-unknown-error, which normally should be responded to by calling both Device.StopVerify and Device.Release (and this is what pam does). Unfortunately, if you try to release the device when the system is preparing for sleep, you'll get an error that the device is busy and then you can't can't claim or release the device for 30 seconds.
pam also has a max timeout for pam_fprintd.so of 99 seconds, and so if we used pam, we'd have to deal with the timeouts and keep restarting the auth conversation.
gdm/gnome-session lock seems to get around these issues by having a shutter on top of the lock screen that you have to interact with first that gives gnome-session a trigger to start fingerprint auth.
Fixes #258