Skip to content
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

Change the display order of PAM messages #824

Merged
merged 3 commits into from
Sep 11, 2023

Conversation

Gliese852
Copy link
Contributor

@Gliese852 Gliese852 commented Aug 8, 2023

This is some suggestion to improve (in my opinion) the interaction between the user and the PAM module.

There are 3 test users on my computer, one just logs in with a password, the second one with the fingerprint scanner (pam_fprintd.so) and the third one with the help of howdy.

Howdy works great, but I noticed a somewhat awkward behavior:

  • The first two users who are not using howdy see the message "There is no face model known", although they did not intend to use this method.
  • Each user sees the message "Attempting facial authentication" if the corresponding option is active, including those, who don't need it.
  • in case of unsuccessful authentication (by timeout), the user does not see any message.

This request represents my attempt to improve it.

Since the comparison takes place in a separate script, I decided to connect to its stdout and, upon receiving the HAS_MODEL string, prompt the user. Not sure if this is the best way, but it seems simple and painless enough.

P.S. I just brought the behavior closer to that of the pam_fprintd.so module, and it seems to me quite convenient - if the user has not configured a fingerprint, we silently skip it, if we have configured it, we write an invitation, if authentication is unsuccessful, we write an error message and exit.

  - do not show a message if the face model is not found

  - show a message if the user could not be recognized

  - show prompt if face model found (and enabled option)

  - enable the "detection_notice" option by default as this will only be
    shown to users who created the face model
howdy/src/compare.py Outdated Show resolved Hide resolved
howdy/src/config.ini Outdated Show resolved Hide resolved
howdy/src/pam/main.cc Show resolved Hide resolved
Revert the use of pipe to signal from the script that the face model is
not found and do it directly in the module.
@boltgolt
Copy link
Owner

boltgolt commented Sep 5, 2023

Sorry for taking this long to take a look at this properly! I think we need to leave the config value to false for now, per my comment. There's also a linter warning in the file from the github action, i think that is a really simple change. Thank you for contributing code!

howdy/src/pam/main.cc:259:5: error: do not use 'else' after 'return' [readability-else-after-return,-warnings-as-errors]
  } else if (config.GetBoolean("core", "detection_notice", true)) {

@boltgolt
Copy link
Owner

Thank you!

@boltgolt boltgolt added this to the Release 3.0.0 milestone Sep 11, 2023
@boltgolt boltgolt merged commit f7649fc into boltgolt:beta Sep 11, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants