fix(setup): restore MultiSelect for PAM service selection; rewrite DM descriptions#44
Open
tyvsmith wants to merge 1 commit into
Open
Conversation
… descriptions Fixes a UX regression introduced in #43 that replaced the single MultiSelect screen with per-service Confirm prompts (one dialog per candidate service). Changes: - Restore dialoguer::MultiSelect in wizard_pam_setup: all detected services are listed on one screen with [x]/[ ] toggles, pre-checked per default_enabled, confirmed with a single Enter keypress. Non-TTY / non-interactive mode auto-selects per defaults (unchanged). - Rewrite display-manager descriptions to remove the alarmist "declining is recommended unless you have recovery access" language. auth sufficient pam_facelock.so falls through to password on module failure, so lockout is not a real risk. The genuine concern with DMs is unverified integration, not lockout — GDM's auth stack is genuinely unusual so it retains a hedge; SDDM and LightDM use conventional stacks and need no warning. - Set default_enabled = true for all three DMs (consistent with how sudo/polkit-1/lockers are handled — same actual risk profile). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a setup wizard UX regression by restoring a single interactive multi-select screen for choosing PAM services, and updates display-manager service descriptions/defaults to reflect the actual (non-lockout) behavior and current integration confidence.
Changes:
- Restore
dialoguer::MultiSelectinwizard_pam_setup(replacing per-serviceConfirmprompts). - Rewrite PAM candidate descriptions for GDM/SDDM/LightDM and adjust their
default_enabledvalues.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+967
to
+971
| for service in &selected_services { | ||
| println!(" Configuring PAM for {service}..."); | ||
| match pam_install(service, true) { | ||
| Ok(()) => configured.push(service.clone()), | ||
| Err(e) => { |
Comment on lines
+1449
to
+1450
| description: "GDM login screen (GNOME) \u{2014} integration not yet verified with GDM's auth flow", | ||
| default_enabled: true, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MultiSelectscreen (all services listed with[x]/[ ]toggles, one Enter to confirm) with N sequentialConfirmprompts — one per candidate service. This restores the original single-screen UX.auth sufficient pam_facelock.sofalls through to password auth when the module is absent or failing, so there is no lockout risk. The real concern with display managers is unverified integration, not lockout.Changes
wizard_pam_setup: replaced thefor-loop ofConfirmprompts with a singledialoguer::MultiSelectcall. Non-TTY / non-interactive mode unchanged (auto-selects perdefault_enabled).gdm-passworddescription: now says "integration not yet verified with GDM's auth flow" (GDM has an unusual stack — hedge retained).sddm/lightdmdescriptions: alarmist suffix removed; both use conventional PAM stacks.default_enabledfor all three DMs changed fromfalse→true(consistent with sudo/polkit-1/lockers — same actual risk profile now that lockout fear is corrected).Test plan
cargo fmt --all -- --check— cleancargo build --workspace— passescargo test --workspace— 262 tests pass, 0 failurescargo clippy --workspace -- -D warnings— cleancargo run --bin facelock -- setup --help— parses correctlyInteractive TTY verification (manual — not automatable in CI):
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com