Bug 99811

Summary: Needless opening of devices even when no fingerprints are enrolled
Product: libfprint Reporter: Christian Kellner <gicmo>
Component: fprintdAssignee: libfprint-bugs
Status: RESOLVED FIXED QA Contact:
Severity: enhancement    
Priority: medium CC: bugzilla, gicmo
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: file_storage: implement storage.has_prints()
main: initialize store.has_prints if provided
device: extract polkit related functions to auth.c
manager: add HasPrints() DBus method
auth: make check_for_username work for manager too
tests: add 'fprint-check' to check system is on
pam: check if user has prints before device open
auth: plug tiny memory leak
pam: separate device opening and claiming
pam: exit early if user has no prints enrolled
pam: exit early if user has no prints enrolled
Open the device on-daemon only

Description Christian Kellner 2017-02-14 12:04:06 UTC
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.
Comment 1 Christian Kellner 2017-02-14 12:07:34 UTC
Created attachment 129583 [details] [review]
file_storage: implement storage.has_prints()
Comment 2 Christian Kellner 2017-02-14 12:07:37 UTC
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.
Comment 3 Christian Kellner 2017-02-14 12:07:40 UTC
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.
Comment 4 Christian Kellner 2017-02-14 12:07:43 UTC
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.
Comment 5 Christian Kellner 2017-02-14 12:07:46 UTC
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.
Comment 6 Christian Kellner 2017-02-14 12:07:49 UTC
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.
Comment 7 Christian Kellner 2017-02-14 12:07:52 UTC
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.
Comment 8 Christian Kellner 2017-02-14 12:07:55 UTC
Created attachment 129590 [details] [review]
auth: plug tiny memory leak

In fprint_check_polkit_for_action we need to free the sender.
Comment 9 Bastien Nocera 2017-02-14 13:10:15 UTC
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 10 Bastien Nocera 2017-02-14 13:16:24 UTC
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 11 Bastien Nocera 2017-02-14 13:23:16 UTC
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 12 Bastien Nocera 2017-02-14 13:25:59 UTC
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 13 Bastien Nocera 2017-02-14 13:26:22 UTC
Comment on attachment 129587 [details] [review]
auth: make check_for_username work for manager too

Review of attachment 129587 [details] [review]:
-----------------------------------------------------------------

No.
Comment 14 Bastien Nocera 2017-02-14 13:30:36 UTC
Comment on attachment 129590 [details] [review]
auth: plug tiny memory leak

Pushed the mem leak in its original code.
Comment 15 Christian Kellner 2017-02-16 11:07:43 UTC
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.
Comment 16 Christian Kellner 2017-02-16 11:07:59 UTC
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 17 Bastien Nocera 2017-02-16 13:29:17 UTC
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) {
  ...
}
Comment 18 Christian Kellner 2017-02-16 13:39:35 UTC
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.
Comment 19 Bastien Nocera 2017-02-16 13:51:22 UTC
2nd patch's commit message was changed, and some indentation re-done.

Attachment 129663 [details] pushed as f54a90e - pam: separate device opening and claiming
Comment 20 Christian Kellner 2017-02-16 14:06:32 UTC
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.
Comment 21 Bastien Nocera 2017-02-16 14:13:01 UTC
(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.