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

Always start session on successful auth #107

Closed
wants to merge 22 commits into from

Conversation

smanilov
Copy link

@smanilov smanilov commented Oct 16, 2021

This results in automatic login after fingerprint is recognized.


Hello there! I couldn't figure out why the special prompted logic was there, but I'm sure there is a good reason :)

Could someone more familiar with the codebase please let me know if this approach breaks some valid use-case?

This results in automatic login after fingerprint is recognized.
@bluesabre
Copy link
Member

At this point in the authentication, it might be possible that PAM has more messages to deliver. But it might also be done in which case this patch would be fine.

@robert-ancell, it's been a decade since you've touched this part of the codebase... but do you have any suggestions here?

@robert-ancell
Copy link
Collaborator

I think the issue was you could select a user in the UI (e.g. by scrolling to them) and if PAM authenticated without input that would immediately log into that user. This is not necessarily what the user expected - in that case we want them to click a button to confirm they actually want to log in as that user. This makes sense on a command line, as the user explicitly triggered something to do the login (e.g. ran a command or entered a username).

@bluesabre
Copy link
Member

@robert-ancell That makes sense, thanks!
@smanilov Is there some event that could be captured when doing a fingerprint read? This would probably be the best place to hook in.

@smanilov
Copy link
Author

Thanks for the reply!

What Robert is saying makes sense to me as well.

Is there some event that could be captured when doing a fingerprint read? This would probably be the best place to hook in.

As far as I can tell, fingerprint auth happens in the PAM library. It is used by session-child, which is a child process in lightdm.

It seems to me that PAM abstracts away the method of authentication by design (fingerprint in this case), so I think it is unlikely that there could be an event special for the fingerprint auth that could be captured, but ignored for other types of PAM.

Would it be feasible to add a configuration flag along the lines of "auto-login-on-successful-pam", so that users could choose the behavior? Option 1 (flag is false): on successful fingerprint read or other PAM -> press button to log in; option 2 (flag is true): on successful fingerprint read or other PAM -> immediately log in.

This allows the user to choose whether to login automatically once PAM
succeeds for the selected user.

On one hand, automatic login makes using fingerprint login more pleasant
-- the user does not need to also press the "Log In" button, once the
fingerprint is recognized.

On the other hand, using different PAM methods, autologin might be
undesirable, as scrolling through the list of users might login a
selected user when it is undesired.

For this reason, having this functionality as a user-configurable
setting makes sense.
@smanilov
Copy link
Author

I just updated the PR to add the configuration option.

Please, let me know what you think :)

smanilov and others added 18 commits January 30, 2022 15:01
The indentation I used earlier was 2 spaces, while the file uses 4
spaces elsewhere.

Although this makes the diff harder to read, it makes the code
consistent.
Bumps [actions/setup-python](https://github.com/actions/setup-python) from 2 to 3.1.1.
- [Release notes](https://github.com/actions/setup-python/releases)
- [Commits](actions/setup-python@v2...v3.1.1)

---
updated-dependencies:
- dependency-name: actions/setup-python
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [actions/checkout](https://github.com/actions/checkout) from 2 to 3.0.2.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v2...v3.0.2)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [actions/setup-python](https://github.com/actions/setup-python) from 3.1.1 to 3.1.2.
- [Release notes](https://github.com/actions/setup-python/releases)
- [Commits](actions/setup-python@v3.1.1...v3.1.2)

---
updated-dependencies:
- dependency-name: actions/setup-python
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [actions/setup-python](https://github.com/actions/setup-python) from 3.1.2 to 4.0.0.
- [Release notes](https://github.com/actions/setup-python/releases)
- [Commits](actions/setup-python@v3.1.2...v4.0.0)

---
updated-dependencies:
- dependency-name: actions/setup-python
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [actions/setup-python](https://github.com/actions/setup-python) from 4.0.0 to 4.1.0.
- [Release notes](https://github.com/actions/setup-python/releases)
- [Commits](actions/setup-python@v4.0.0...v4.1.0)

---
updated-dependencies:
- dependency-name: actions/setup-python
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [actions/setup-python](https://github.com/actions/setup-python) from 4.1.0 to 4.2.0.
- [Release notes](https://github.com/actions/setup-python/releases)
- [Commits](actions/setup-python@v4.1.0...v4.2.0)

---
updated-dependencies:
- dependency-name: actions/setup-python
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
@smanilov
Copy link
Author

Hey, close to 1 year anniversary of the creation of this PR \o/

Could anyone take a look and merge this, if it's okay?

@smanilov
Copy link
Author

Please see #127 , which simplifies this PR.

@smanilov smanilov deleted the master branch September 27, 2022 09:28
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.

6 participants