fix: Handle LDAP users and improve last user resolution - #489
Open
lannuttia wants to merge 42 commits into
Open
Conversation
Author
|
@wash2 This should be ready for review. It does refactor some stuff. I've personally used the changes on this branch to resolve all of the issues that I was experiencing with my corporate laptop and LDAP-backed user. I'll personally try it on my personal computer tonight since that uses a local user and report my findings, though I don't suspect there will be any regressions there. |
jackpot51
requested changes
Jul 14, 2026
jackpot51
left a comment
jackpot51
left a comment
Member
There was a problem hiding this comment.
Please disclose any AI/LLM usage.
Author
Ooof... Good callout. I have a plugin that automatically adds AI attribution for commits made via an AI Agent and that didn't add those attribution trailers because I forgot to configure it for this repository. I'll replay all the commits to get the trailers added. |
Reproduces and tests fix for bug where LDAP-backed users (or any user not in enumerated user_datas) would have empty username on subsequent login attempts, causing authentication failures. Test verifies that when last_user UID is not found in user_datas, the system preserves user information instead of defaulting to empty username. Coding-Agent: Claude Code Model: claude-sonnet-4-5
- Extract test constants for UIDs to reduce magic numbers - Add helper function to create test users (DRY principle) - Fix unused variable warning (_uid) - Add more specific assertions in LDAP missing test - Improve test comments and documentation Coding-Agent: Claude Code Model: claude-sonnet-4-5
Replace sentinel value approach with direct passwd database lookup. When last_user UID is not found in enumerated user_datas, query the system user database directly via pwd::Passwd::from_uid(). This properly handles LDAP users (and other non-enumerated accounts) without requiring manual username entry or sentinel values. Tests added: - test_real_system_user_lookup_via_passwd: Verifies passwd lookup - test_buggy_version_returns_empty_username: Documents original bug - test_uid_not_found_anywhere_returns_empty: Edge case handling Coding-Agent: Claude Code Model: claude-sonnet-4-5
Fixes issue where LDAP-backed users (or any non-enumerated accounts) would fail to login on subsequent attempts because their username was set to empty string. The bug occurred when: 1. User logs in with LDAP account (UID saved in config) 2. On next login, UID not found in user_datas (LDAP users not enumerated) 3. Code fell back to unwrap_or_default(), setting username to "" 4. Authentication failed because greetd received empty username The fix adds pwd::Passwd::from_uid() lookup between the user_datas search and the fallback, so non-enumerated users are found via the system user database. Tests added in greeter.rs: - test_last_user_in_enumerated_list - test_ldap_user_not_in_enumerated_list_bug - test_fallback_to_first_user_when_ldap_user_missing_and_locals_exist Coding-Agent: Claude Code Model: claude-sonnet-4-5
Add get_display_name_for_user() function that falls back to querying passwd when user is not in the enumerated user_datas list. This ensures LDAP users (and other non-enumerated accounts) see their full name in the greeter UI, extracted from the GECOS field in passwd. Changes: - Add get_display_name_for_user() helper function - Refactor UI code to display name even when data_idx is None - Add tests for display name extraction from passwd - Add test_username_selection_without_user_datas to prove greeter works with empty user_datas (passwd only) This continues the effort to reduce dependency on user_datas and make the greeter work correctly with non-enumerated user accounts. Coding-Agent: Claude Code Model: claude-sonnet-4-5
BREAKING CHANGE: Complete architectural refactoring to eliminate dependency on index-based user data lookups. Changes: - Rename NameIndexPair -> SelectedUser with uid field instead of data_idx - Change Flags.user_datas: Vec<UserData> -> user_configs: HashMap<u32, UserData> - Change Flags.user_icons: Vec<Option<Handle>> -> HashMap<u32, Handle> - All lookups now use UID as key instead of Vec index - Update get_display_name_for_user() to accept UID parameter - Add determine_username_from_last_user_v2() with HashMap API - Keep backward-compat wrapper for old tests Benefits: - Eliminates fragile index-based lookups - No more data_idx tracking throughout codebase - Direct UID-based access is clearer and more efficient - Greeter works correctly even when user not in daemon's enumeration - LDAP users fully supported via pwd::Passwd fallback All 15 tests pass (9 unit + 6 integration). Coding-Agent: Claude Code Model: claude-sonnet-4-5
Fixes regression where LDAP users (not enumerated by daemon) would see a grey background instead of wallpapers. The issue was that update_user_data() returned early when user was not in user_configs, preventing wallpaper loading. Now update_wallpapers() is always called, even with default UserData for users without configs, ensuring backgrounds are loaded. Includes test to verify wallpaper loading is attempted for all users. Coding-Agent: Claude Code Model: claude-sonnet-4-5
- Removed helper function should_call_wallpaper_loading() - Test now directly verifies update_wallpapers() works with default UserData - Eliminated unwrap() in update_user_data() by using match expression value - Code is cleaner and more direct Coding-Agent: Claude Code Model: claude-sonnet-4-5
Fixes gray background issue on greeter startup when LDAP user is selected. Previously, wallpaper loading only happened reactively on OutputEvent::Created, causing the initial render to show a gray background. Now init() proactively calls update_wallpapers() for the initially selected user (whether from user_configs or LDAP), ensuring wallpaper is ready for first render. Includes test to verify wallpaper initialization during app init. Coding-Agent: Claude Code Model: claude-sonnet-4-5
Coding-Agent: Claude Code Model: claude-sonnet-4-5
Removes wallpaper initialization tests and related code that were based on a local environment issue (missing git lfs pull), not a real bug. - Removed test_wallpaper_loading_with_default_user_data - Removed test_init_calls_wallpaper_setup_for_selected_user - Removed wallpaper loading logic in update_user_data() for LDAP users - Removed wallpaper initialization in init() Coding-Agent: Claude Code Model: claude-sonnet-4-5
Extracts duplicated passwd lookup pattern into a reusable helper function. This will be used to consolidate 4x duplicated calls in the codebase. Coding-Agent: Claude Code Model: claude-sonnet-4-5
…name Consolidates 2x passwd lookup pattern into the helper function: - Line 1405: Message::Username handler - Line 1428: Message::SubmitUsername handler This eliminates code duplication and improves maintainability. Coding-Agent: Claude Code Model: claude-sonnet-4-5
Extracts duplicated user_configs accessor pattern into a reusable method. This will be used to consolidate 10x duplicated calls in the codebase. Coding-Agent: Claude Code Model: claude-sonnet-4-5
…_user_config Consolidates 5x user_configs lookup pattern into the helper method: - Line 505: military_time in menu() left_element - Line 771: military_time in menu() right_element - Line 1057: set_xkb_config() user lookup - Line 1063: update_user_data() user lookup (with clone) - Line 1812: RandrUpdate kdl_output_lists lookup This eliminates code duplication and improves maintainability. Coding-Agent: Claude Code Model: claude-sonnet-4-5
Adds display_name field to SelectedUser to eliminate syscalls on every render. The display name is computed once when the user is selected and cached for subsequent UI renders. This eliminates the need to call pwd::Passwd::from_name() in the render path, improving performance. Coding-Agent: Claude Code Model: claude-sonnet-4-5
Deletes determine_username_from_last_user wrapper function that was only used for backward compatibility. All tests now use determine_username_from_last_user_v2 which uses HashMap<UID, UserData> instead of Vec<UserData>. Updated tests: - test_last_user_in_enumerated_list - test_ldap_user_not_in_enumerated_list_bug - test_fallback_to_first_user_when_ldap_user_missing_and_locals_exist Coding-Agent: Claude Code Model: claude-sonnet-4-5
High-priority fixes: - Performance: Use cached display_name in render path (avoid syscalls) - Test: Fix root UID test assertion (NonZeroU32::new(0) is None) - Structure: Extract determine_username_from_last_user to production Medium-priority fixes: - Structure: Extract make_selected_user() helper to reduce duplication - Test: Remove duplicate tests (test_uid_based_lookup_*) - Test: Strengthen assertion in test_get_display_name_from_passwd - Test: Use u32::MAX - 1 for guaranteed non-existent UID Documentation: - Add comprehensive comment about system-coupled tests and thread safety All changes maintain backward compatibility and improve code quality, test reliability, and performance. Coding-Agent: Claude Code Model: claude-sonnet-4-5
Remove unconventional `get_` prefix to follow Rust naming conventions. Getters in Rust typically omit the `get_` prefix when they are simple accessors without side effects. Changes: - Renamed method definition - Updated all 5 call sites in production code - Updated test function name and 3 test call sites - All tests passing (10/10) Coding-Agent: Claude Code Model: claude-sonnet-4-5
Remove redundant type encoding from variable name. The type Vec<UserData> is already clear from context, and "_vec" suffix violates "don't encode type in name" principle. Changes: - Renamed variable at line 128 - Updated for-loop iterator at line 140 - All tests passing (10/10) Coding-Agent: Claude Code Model: claude-sonnet-4-5
In test_username_selection_without_user_configs, replace weak assert!(!display_name.is_empty()) with precise assertion that verifies the exact expected value from GECOS or username fallback. This matches the pattern used in test_get_display_name_from_passwd and makes the test more robust by catching incorrect values that happen to be non-empty. Changes: - Calculate expected_display_name from current_user.gecos - Use assert_eq! with descriptive message - All tests passing (10/10) Coding-Agent: Claude Code Model: claude-sonnet-4-5
Remove test that only verifies struct field assignment with no behavioral assertions. The test creates a SelectedUser with hardcoded values and asserts those same values are readable, which provides zero value beyond basic compilation checking. The actual caching behavior (avoiding syscalls) is not tested or observable in this test. The struct's usage is already covered by other meaningful tests that verify end-to-end behavior. Changes: - Deleted test_selected_user_caches_display_name (18 lines) - Test count: 10 → 9 - All remaining tests passing (9/9) Coding-Agent: Claude Code Model: claude-sonnet-4-5
Avoid expensive UID resolution and display name lookup on every keystroke when typing a username. Previously, EnterUser called make_selected_user() which invokes: - resolve_uid_for_username() (passwd syscall) - get_display_name_for_user() (potential GECOS syscall) Now EnterUser only updates the username field, deferring full resolution until Username (submit). This eliminates syscalls during typing with no observable behavior change - partial usernames during typing won't resolve correctly anyway. Changes: - EnterUser: set username field directly (no UID/display lookup) - Username: still calls make_selected_user() (full resolution) - All tests passing (9/9) Coding-Agent: Claude Code Model: claude-sonnet-4-5
Coding-Agent: Claude Code Model: claude-sonnet-4-5
The field holds a SelectedUser struct, not a string username. This mechanical rename improves naming clarity across ~15 usage sites. Coding-Agent: Claude Code Model: claude-sonnet-4-5
…empty configs
Adds regression test to verify that determine_username_from_last_user
correctly returns ("", None) when last_user is None and user_configs is empty.
The implementation already handles this via unwrap_or_default().
Coding-Agent: Claude Code
Model: claude-sonnet-4-5
Adds regression test to verify that get_display_name_for_user returns the username as fallback when: - User does not exist in user_configs - User does not exist in passwd database - UID is None The implementation already handles this via the final fallback at line 1989. Coding-Agent: Claude Code Model: claude-sonnet-4-5
Improves test isolation and clarity by splitting one test with 3 scenarios into 3 independent tests: - test_selected_user_config_with_existing_user - test_selected_user_config_with_ldap_user - test_selected_user_config_with_no_uid Benefits: - Each test has clear single responsibility - Failure in one scenario doesn't skip later scenarios - Test names are more descriptive - Follows AAA pattern (Arrange-Act-Assert) explicitly Coding-Agent: Claude Code Model: claude-sonnet-4-5
Coding-Agent: Claude Code Model: claude-sonnet-4-5
…ted_user Coding-Agent: Claude Code Model: claude-sonnet-4-5
Function returns both username and UID, not just username. New name better describes the return value (String, Option<NonZeroU32>). Coding-Agent: Claude Code Model: claude-sonnet-4-5
Eliminates code duplication between UserData::from and get_display_name_for_user. Both now use parse_full_name_from_gecos(). Coding-Agent: Claude Code Model: claude-sonnet-4-5
When daemon is unavailable, user_data_fallback() queries passwd database and populates user_configs with real data, not an empty HashMap. Coding-Agent: Claude Code Model: claude-sonnet-4-5
Extract build_username_to_uid_index() as testable function. Test now verifies actual index-building logic from user_configs, providing regression protection for main() code path. Previous test only verified HashMap::insert/get mechanics. Coding-Agent: Claude Code Model: claude-sonnet-4-5
- Removed build_username_to_uid_index doc (function name already describes what) - Simplified username_to_uid field comment (type already shows it's a map) - Enhanced parse_full_name_from_gecos to explain gecos format convention (why we take first component) Comments should explain why, not what the code already says. Coding-Agent: Claude Code Model: claude-sonnet-4-5
…last_user_* Update test name to match current function name (determine_username_from_last_user was renamed to resolve_last_user in efce120). Coding-Agent: Claude Code Model: claude-sonnet-4-5
Removed 3 tests that constructed entire App via App::init() just to test trivial HashMap lookup (selected_user.uid → user_configs.get). These tests provided no regression protection - they tested Option::and_then and HashMap mechanics, not application logic. Coding-Agent: Claude Code Model: claude-sonnet-4-5
…me_from_gecos Avoids unnecessary clone by accepting Option<&str> directly instead of taking Option<String> by value and calling .as_ref(). Call sites updated to use .as_deref() to convert Option<String> → Option<&str>. Coding-Agent: Claude Code Model: claude-sonnet-4-5
…figs) Tests fallback to first user (lowest UID) when last_user is None but user_configs contains users. Implementation already handled this correctly, just missing test coverage. Coding-Agent: Claude Code Model: claude-sonnet-4-5
Removed comments that just describe what the code already says: - "Build HashMap of user configs indexed by UID" - "Extract and store icon if present" - "Store user config" - "Build reverse index for O(1) username → UID lookups" - "Find UID for this username using O(1) reverse index lookup" Code is self-documenting. Comments should explain why, not what. Coding-Agent: Claude Code Model: claude-sonnet-4-5
Coding-Agent: Claude Code Model: claude-sonnet-4-5
lannuttia
force-pushed
the
fix/last-user-unknown
branch
from
July 14, 2026 22:27
820df6e to
8ce2225
Compare
Author
|
And I can now confirm that I've been able to log in as a local user on my personal computer using this most recent version. Everything appears to be functional to me. |
Member
|
Do you fully understand the changes in this pull request? |
Author
At a high-level, LDAP users would not be able to log in because LDAP backed accounts are not enumerable by default on systems using SSSD. The user's account not being enumerable meant that it would not be found in Before my changes, if the user didn't show up in that user_datas vector, the first user's username in user_datas was used, if user_datas was empty , an empty string was used for the username and then that was sent to greetd for authentication. After my changes, We attempt to find the user in user_configs (which is populated by enumeration), if no user with a matching uid exists in user_configs, it attempts to find the user via A field called username_to_uid was added to Flags that maps a username to a UID so that we can get the UID from a username in O(1) instead of the O(n) we were getting by doing a linear search over the vector of enumerated users. |
Resolved conflicts in src/greeter.rs: - Removed standalone set_xkb_config() method (functionality moved to Common::update_user_data in master) - Kept update_user_data() implementation from master which already handles XKB config via Common::update_user_data() Model: claude-sonnet-4-5@20250929 Coding-Agent: OpenCode Co-authored-by: AI Assistant <ai@opencode.ai>
Author
|
Are there still any changes that need to be made here? |
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
2 participants
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
Fixes login failures and missing display names for LDAP users and other non-enumerated accounts.
Problems Solved
Login Persistence Failure
Missing Display Names
Technical Changes
LDAP User Support
pwd::Passwd::from_uid()for LDAP user lookup and login persistencePerformance Optimizations
make_selected_user()make_selected_user()execution from keystroke to submitCode Quality
Testing
All changes covered by unit tests including LDAP user scenarios verified against system passwd database.
Resolves #487