On auth requests (gdm, sudo) fprint-pam/fprintd currently claims the device which opens it, possibly waking it up from autosuspend. This should not be needed if there are no fingerprints enrolled at all. We currently already are indicating (on the login-screen and during sudo) if there are fingerprints enrolled (we only show the "swipe your finger" messages when there are prints enrolled), so a possible has-prints-enrolled API would not give away any new information.
Created attachment 129583 [details] [review] file_storage: implement storage.has_prints()
Created attachment 129584 [details] [review] main: initialize store.has_prints if provided For the file storage we know that it is implemented, but for dynamically loaded modules we cannot for sure say, so we check and fall back to our own version that just reports an error.
Created attachment 129585 [details] [review] device: extract polkit related functions to auth.c The functsion related to authorization are actually not tied to a specific device (they only access the PolkitAuthority member of device). Therefore this function were made device-independent and moved to auth.c so they can be re-used from, e.g. manager.c.
Created attachment 129586 [details] [review] manager: add HasPrints() DBus method The intended use for this method is to check if the given user has any fingerprints enrolled for any device at all, so we can skip the opening of devices in case there are no prints.
Created attachment 129587 [details] [review] auth: make check_for_username work for manager too fprint_check_for_username now takes a boolean flag (device) to indicate if we want the device.setusername action or the manager.setusername action.
Created attachment 129588 [details] [review] tests: add 'fprint-check' to check system is on Will use the manager.HasPrints() DBus method to query if the fingerprint system has any prints for the given user.
Created attachment 129589 [details] [review] pam: check if user has prints before device open If the user does not have any prints registered for any devices we might as well not bother to open the device and possible safe some power with this.
Created attachment 129590 [details] [review] auth: plug tiny memory leak In fprint_check_polkit_for_action we need to free the sender.
Comment on attachment 129583 [details] [review] file_storage: implement storage.has_prints() Review of attachment 129583 [details] [review]: ----------------------------------------------------------------- ::: src/file_storage.c @@ +282,4 @@ > return list; > } > > +int file_storage_has_prints(const char *username) This needs to be a gboolean. @@ +294,5 @@ > + if (r < 0) { > + return r; > + } > + > + // look for any 'finger' matches in the path. C-comments please.
Comment on attachment 129584 [details] [review] main: initialize store.has_prints if provided Review of attachment 129584 [details] [review]: ----------------------------------------------------------------- We don't have any external plugins, so we can just bail when it fails, no need for a fallback.
Comment on attachment 129585 [details] [review] device: extract polkit related functions to auth.c Review of attachment 129585 [details] [review]: ----------------------------------------------------------------- > The functsion function ::: src/Makefile.am @@ +10,4 @@ > AM_CFLAGS = $(WARN_CFLAGS) $(FPRINT_CFLAGS) $(DAEMON_CFLAGS) -DLOCALEDIR=\""$(datadir)/locale"\" -DPLUGINDIR=\""$(libdir)/fprintd/modules"\" > > libfprintd_la_SOURCES = \ > + auth.c manager.c device.c \ huh, where's the .h file? ::: src/auth.c @@ +1,1 @@ > + Copyright notice? @@ +3,5 @@ > +#include "fprintd.h" > + > +#include <pwd.h> > + > +extern DBusGConnection *fprintd_dbus_conn; No, find another way.
Comment on attachment 129586 [details] [review] manager: add HasPrints() DBus method Review of attachment 129586 [details] [review]: ----------------------------------------------------------------- NAK. This needs to live at the device level, so you don't need to introduce a new polkit authorisation, or try to poke at devices that are in use.
Comment on attachment 129587 [details] [review] auth: make check_for_username work for manager too Review of attachment 129587 [details] [review]: ----------------------------------------------------------------- No.
Comment on attachment 129590 [details] [review] auth: plug tiny memory leak Pushed the mem leak in its original code.
Created attachment 129663 [details] [review] pam: separate device opening and claiming For some operations, i.e. listing the enrolled prints, the device does not need to be claimed. Therefore the claiming can be delayed until we actually start the verification process, allowing us to query the fingerprint system if the user has any prints enrolled.
Created attachment 129664 [details] [review] pam: exit early if user has no prints enrolled Before claiming the device and therefore potentially activating the actual hardware, a call is made to see if the user has any prints registered at all.
Comment on attachment 129664 [details] [review] pam: exit early if user has no prints enrolled Review of attachment 129664 [details] [review]: ----------------------------------------------------------------- ::: pam/pam_fprintd.c @@ +447,2 @@ > > + if (have_prints && claim_device(pamh, dev, username)) { I'd prefer: if (have_prints) { ... }
Created attachment 129668 [details] [review] pam: exit early if user has no prints enrolled Before claiming the device and therefore potentially activating the actual hardware, a call is made to see if the user has any prints registered at all.
2nd patch's commit message was changed, and some indentation re-done. Attachment 129663 [details] pushed as f54a90e - pam: separate device opening and claiming
Created attachment 129670 [details] [review] Open the device on-daemon only I implemented the on-demand opening of the device on verification/enrollment. Tested and worked as expected but crashes the gnome-control-center panel because the on-demand opening is not done for the num-enroll-stages (gobject/dbus) property of the device, so that returns -1 (which apparently happens to crash g-c-c). I think committed patches are good enough, I just wanted to have the patch and the idea documented here.
(In reply to Christian Kellner from comment #20) > Created attachment 129670 [details] [review] [review] > Open the device on-daemon only > > I implemented the on-demand opening of the device on > verification/enrollment. Tested and worked as expected but crashes the > gnome-control-center panel because the on-demand opening is not done for the > num-enroll-stages (gobject/dbus) property of the device, so that returns -1 > (which apparently happens to crash g-c-c). I think committed patches are > good enough, I just wanted to have the patch and the idea documented here. That's fine, the patches that don't change anything internally are better :)
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.