From 48766fb6fce6ee4a6a4d9f225528e4f961bd5374 Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Mon, 17 Nov 2025 16:46:30 +0100 Subject: [PATCH 01/16] Remove leftover golden files Remove leftover golden files from tests that don't exist anymore by running git rm -r internal/**/testdata/golden TESTS_UPDATE_GOLDEN=1 go test ./internal/... --- .../groups-backup | 4 -- .../groups | 1 - .../groups | 0 ...arn_when_groups_file_has_invalid_gid.group | 6 -- ...n_groups_file_has_invalid_gid.group.backup | 6 -- ...g_manager_with_partially_invalid_id_ranges | 64 ------------------- ...anager_with_potentially_invalid_GID_ranges | 64 ------------------- ...anager_with_potentially_invalid_UID_ranges | 64 ------------------- 8 files changed, 209 deletions(-) delete mode 100644 internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNames/groups-backup delete mode 100644 internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNamesAlreadyUpdated/groups delete mode 100644 internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNamesEmptyDB/groups delete mode 100644 internal/users/localentries/testdata/golden/TestUpdateGroups/Warn_when_groups_file_has_invalid_gid.group delete mode 100644 internal/users/localentries/testdata/golden/TestUpdateGroups/Warn_when_groups_file_has_invalid_gid.group.backup delete mode 100644 internal/users/testdata/golden/TestNewManager/Warns_creating_manager_with_partially_invalid_id_ranges delete mode 100644 internal/users/testdata/golden/TestNewManager/Warns_creating_manager_with_potentially_invalid_GID_ranges delete mode 100644 internal/users/testdata/golden/TestNewManager/Warns_creating_manager_with_potentially_invalid_UID_ranges diff --git a/internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNames/groups-backup b/internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNames/groups-backup deleted file mode 100644 index 68df9429eb..0000000000 --- a/internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNames/groups-backup +++ /dev/null @@ -1,4 +0,0 @@ -root:x:0: -other-local-group:x:1234: -other-local-group-with-users:x:4321:user-foo,user-bar -LocalTestGroup:x:12345:TestUser diff --git a/internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNamesAlreadyUpdated/groups b/internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNamesAlreadyUpdated/groups deleted file mode 100644 index 1e941b4c8f..0000000000 --- a/internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNamesAlreadyUpdated/groups +++ /dev/null @@ -1 +0,0 @@ -LocalTestGroup:x:12345:testuser diff --git a/internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNamesEmptyDB/groups b/internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNamesEmptyDB/groups deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/internal/users/localentries/testdata/golden/TestUpdateGroups/Warn_when_groups_file_has_invalid_gid.group b/internal/users/localentries/testdata/golden/TestUpdateGroups/Warn_when_groups_file_has_invalid_gid.group deleted file mode 100644 index d8ec794f3d..0000000000 --- a/internal/users/localentries/testdata/golden/TestUpdateGroups/Warn_when_groups_file_has_invalid_gid.group +++ /dev/null @@ -1,6 +0,0 @@ -localgroup1:x:41:otheruser,myuser -localgroup2:x:42:otheruser2 -localgroup3:x:gid:invalidGID -localgroup4:x:44:otheruser2 -cloudgroup1:x:9998:otheruser3 -cloudgroup2:x:9999:otheruser4 diff --git a/internal/users/localentries/testdata/golden/TestUpdateGroups/Warn_when_groups_file_has_invalid_gid.group.backup b/internal/users/localentries/testdata/golden/TestUpdateGroups/Warn_when_groups_file_has_invalid_gid.group.backup deleted file mode 100644 index 5a55ebf584..0000000000 --- a/internal/users/localentries/testdata/golden/TestUpdateGroups/Warn_when_groups_file_has_invalid_gid.group.backup +++ /dev/null @@ -1,6 +0,0 @@ -localgroup1:x:41:otheruser -localgroup2:x:42:otheruser2 -localgroup3:x:gid:invalidGID -localgroup4:x:44:otheruser2 -cloudgroup1:x:9998:otheruser3 -cloudgroup2:x:9999:otheruser4 diff --git a/internal/users/testdata/golden/TestNewManager/Warns_creating_manager_with_partially_invalid_id_ranges b/internal/users/testdata/golden/TestNewManager/Warns_creating_manager_with_partially_invalid_id_ranges deleted file mode 100644 index 5d465349a9..0000000000 --- a/internal/users/testdata/golden/TestNewManager/Warns_creating_manager_with_partially_invalid_id_ranges +++ /dev/null @@ -1,64 +0,0 @@ -users: - - name: user1 - uid: 1111 - gid: 11111 - gecos: |- - User1 gecos - On multiple lines - dir: /home/user1 - shell: /bin/bash - broker_id: broker-id - - name: user2 - uid: 2222 - gid: 22222 - gecos: User2 - dir: /home/user2 - shell: /bin/dash - broker_id: broker-id - - name: user3 - uid: 3333 - gid: 33333 - gecos: User3 - dir: /home/user3 - shell: /bin/zsh - broker_id: broker-id - - name: userwithoutbroker - uid: 4444 - gid: 44444 - gecos: userwithoutbroker - dir: /home/userwithoutbroker - shell: /bin/sh -groups: - - name: group1 - gid: 11111 - ugid: "12345678" - - name: group2withoutugid - gid: 22222 - ugid: "" - - name: group3 - gid: 33333 - ugid: "34567812" - - name: group4 - gid: 44444 - ugid: "45678123" - - name: commongroup - gid: 99999 - ugid: "87654321" -users_to_groups: - - uid: 1111 - gid: 11111 - - uid: 1111 - gid: 99999 - - uid: 2222 - gid: 22222 - - uid: 2222 - gid: 99999 - - uid: 3333 - gid: 33333 - - uid: 3333 - gid: 99999 - - uid: 4444 - gid: 44444 - - uid: 4444 - gid: 99999 -schema_version: 1 diff --git a/internal/users/testdata/golden/TestNewManager/Warns_creating_manager_with_potentially_invalid_GID_ranges b/internal/users/testdata/golden/TestNewManager/Warns_creating_manager_with_potentially_invalid_GID_ranges deleted file mode 100644 index 5d465349a9..0000000000 --- a/internal/users/testdata/golden/TestNewManager/Warns_creating_manager_with_potentially_invalid_GID_ranges +++ /dev/null @@ -1,64 +0,0 @@ -users: - - name: user1 - uid: 1111 - gid: 11111 - gecos: |- - User1 gecos - On multiple lines - dir: /home/user1 - shell: /bin/bash - broker_id: broker-id - - name: user2 - uid: 2222 - gid: 22222 - gecos: User2 - dir: /home/user2 - shell: /bin/dash - broker_id: broker-id - - name: user3 - uid: 3333 - gid: 33333 - gecos: User3 - dir: /home/user3 - shell: /bin/zsh - broker_id: broker-id - - name: userwithoutbroker - uid: 4444 - gid: 44444 - gecos: userwithoutbroker - dir: /home/userwithoutbroker - shell: /bin/sh -groups: - - name: group1 - gid: 11111 - ugid: "12345678" - - name: group2withoutugid - gid: 22222 - ugid: "" - - name: group3 - gid: 33333 - ugid: "34567812" - - name: group4 - gid: 44444 - ugid: "45678123" - - name: commongroup - gid: 99999 - ugid: "87654321" -users_to_groups: - - uid: 1111 - gid: 11111 - - uid: 1111 - gid: 99999 - - uid: 2222 - gid: 22222 - - uid: 2222 - gid: 99999 - - uid: 3333 - gid: 33333 - - uid: 3333 - gid: 99999 - - uid: 4444 - gid: 44444 - - uid: 4444 - gid: 99999 -schema_version: 1 diff --git a/internal/users/testdata/golden/TestNewManager/Warns_creating_manager_with_potentially_invalid_UID_ranges b/internal/users/testdata/golden/TestNewManager/Warns_creating_manager_with_potentially_invalid_UID_ranges deleted file mode 100644 index 5d465349a9..0000000000 --- a/internal/users/testdata/golden/TestNewManager/Warns_creating_manager_with_potentially_invalid_UID_ranges +++ /dev/null @@ -1,64 +0,0 @@ -users: - - name: user1 - uid: 1111 - gid: 11111 - gecos: |- - User1 gecos - On multiple lines - dir: /home/user1 - shell: /bin/bash - broker_id: broker-id - - name: user2 - uid: 2222 - gid: 22222 - gecos: User2 - dir: /home/user2 - shell: /bin/dash - broker_id: broker-id - - name: user3 - uid: 3333 - gid: 33333 - gecos: User3 - dir: /home/user3 - shell: /bin/zsh - broker_id: broker-id - - name: userwithoutbroker - uid: 4444 - gid: 44444 - gecos: userwithoutbroker - dir: /home/userwithoutbroker - shell: /bin/sh -groups: - - name: group1 - gid: 11111 - ugid: "12345678" - - name: group2withoutugid - gid: 22222 - ugid: "" - - name: group3 - gid: 33333 - ugid: "34567812" - - name: group4 - gid: 44444 - ugid: "45678123" - - name: commongroup - gid: 99999 - ugid: "87654321" -users_to_groups: - - uid: 1111 - gid: 11111 - - uid: 1111 - gid: 99999 - - uid: 2222 - gid: 22222 - - uid: 2222 - gid: 99999 - - uid: 3333 - gid: 33333 - - uid: 3333 - gid: 99999 - - uid: 4444 - gid: 44444 - - uid: 4444 - gid: 99999 -schema_version: 1 From 98b3d0b3168d4be3c18d397bb4ff53d7c3652f84 Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Mon, 17 Nov 2025 15:31:38 +0100 Subject: [PATCH 02/16] Fix test name We use underscores instead of spaces in test names. --- internal/users/manager_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/users/manager_test.go b/internal/users/manager_test.go index 69e5ebffa9..6039281193 100644 --- a/internal/users/manager_test.go +++ b/internal/users/manager_test.go @@ -192,7 +192,7 @@ func TestUpdateUser(t *testing.T) { "Successfully_update_user_updating_local_groups": {groupsCase: "mixed-groups-authd-first", localGroupsFile: "users_in_groups.group"}, "Successfully_update_user_updating_local_groups_with_changes": {groupsCase: "mixed-groups-authd-first", localGroupsFile: "user_mismatching_groups.group"}, "UID_does_not_change_if_user_already_exists": {userCase: "same-name-different-uid", dbFile: "one_user_and_group", wantSameUID: true}, - "Successfully update user with different capitalization": {userCase: "different-capitalization-same-uid", dbFile: "one_user_and_group"}, + "Successfully_update_user_with_different_capitalization": {userCase: "different-capitalization-same-uid", dbFile: "one_user_and_group"}, "GID_does_not_change_if_group_with_same_UGID_exists": {groupsCase: "different-name-same-ugid", dbFile: "one_user_and_group"}, "GID_does_not_change_if_group_with_same_name_and_empty_UGID_exists": {groupsCase: "authd-group", dbFile: "group-with-empty-UGID"}, "Removing_last_user_from_a_group_keeps_the_group_record": {groupsCase: "no-groups", dbFile: "one_user_and_group"}, From 69fd38a9bfdbb1afbd6a2425fa4ff902a651099f Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Mon, 17 Nov 2025 15:32:52 +0100 Subject: [PATCH 03/16] Use lowercase username in GetPreviousBroker --- internal/services/pam/pam.go | 19 +++++++++++-------- internal/services/pam/pam_test.go | 1 + 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/internal/services/pam/pam.go b/internal/services/pam/pam.go index 86dcc33fbd..983b26dc53 100644 --- a/internal/services/pam/pam.go +++ b/internal/services/pam/pam.go @@ -63,13 +63,16 @@ func (s Service) AvailableBrokers(ctx context.Context, _ *authd.Empty) (*authd.A // GetPreviousBroker returns the previous broker set for a given user, if any. // If the user is not in our cache/database, it will try to check if it’s on the system, and return then "local". func (s Service) GetPreviousBroker(ctx context.Context, req *authd.GPBRequest) (*authd.GPBResponse, error) { + // authd usernames are lowercase + username := strings.ToLower(req.GetUsername()) + // Use in memory cache first - if b := s.brokerManager.BrokerForUser(req.GetUsername()); b != nil { + if b := s.brokerManager.BrokerForUser(username); b != nil { return &authd.GPBResponse{PreviousBroker: b.ID}, nil } // Load from database. - brokerID, err := s.userManager.BrokerForUser(req.GetUsername()) + brokerID, err := s.userManager.BrokerForUser(username) // User is not in our database. if err != nil && errors.Is(err, users.NoDataFoundError{}) { // FIXME: this part will not be here in the v2 API version, as we won’t have GetPreviousBroker and handle @@ -81,8 +84,8 @@ func (s Service) GetPreviousBroker(ctx context.Context, req *authd.GPBRequest) ( } // User not accessible through NSS, first time login or no valid user. Anyway, no broker selected. - if _, err := user.Lookup(req.GetUsername()); err != nil { - log.Debugf(ctx, "GetPreviousBroker: User %q not found", req.GetUsername()) + if _, err := user.Lookup(username); err != nil { + log.Debugf(ctx, "GetPreviousBroker: User %q not found", username) return &authd.GPBResponse{}, nil } @@ -90,13 +93,13 @@ func (s Service) GetPreviousBroker(ctx context.Context, req *authd.GPBRequest) ( // service (passwd, winbind, sss…) is handling that user. brokerID = brokers.LocalBrokerName } else if err != nil { - log.Infof(ctx, "GetPreviousBroker: Could not get broker for user %q from database: %v", req.GetUsername(), err) + log.Infof(ctx, "GetPreviousBroker: Could not get broker for user %q from database: %v", username, err) return &authd.GPBResponse{}, nil } // No error but the brokerID is empty (broker in database but default broker not stored yet due no successful login) if brokerID == "" { - log.Infof(ctx, "GetPreviousBroker: No broker set for user %q, letting the user select a new one", req.GetUsername()) + log.Infof(ctx, "GetPreviousBroker: No broker set for user %q, letting the user select a new one", username) return &authd.GPBResponse{}, nil } @@ -113,8 +116,8 @@ func (s Service) GetPreviousBroker(ctx context.Context, req *authd.GPBRequest) ( if brokerID == brokers.LocalBrokerName { return &authd.GPBResponse{PreviousBroker: brokerID}, nil } - if err = s.brokerManager.SetDefaultBrokerForUser(brokerID, req.GetUsername()); err != nil { - log.Warningf(ctx, "GetPreviousBroker: Could not cache broker %q for user %q: %v", brokerID, req.GetUsername(), err) + if err = s.brokerManager.SetDefaultBrokerForUser(brokerID, username); err != nil { + log.Warningf(ctx, "GetPreviousBroker: Could not cache broker %q for user %q: %v", brokerID, username, err) return &authd.GPBResponse{}, nil } diff --git a/internal/services/pam/pam_test.go b/internal/services/pam/pam_test.go index 5fdb1b163d..3d8bdda8ef 100644 --- a/internal/services/pam/pam_test.go +++ b/internal/services/pam/pam_test.go @@ -137,6 +137,7 @@ func TestGetPreviousBroker(t *testing.T) { "Success_getting_previous_broker": {user: "userwithbroker", wantBroker: mockBrokerGeneratedID}, "For_local_user,_get_local_broker": {user: currentUsername, wantBroker: brokers.LocalBrokerName}, "For_unmanaged_user_and_only_one_broker,_get_local_broker": {user: "nonexistent", onlyLocalBroker: true, wantBroker: brokers.LocalBrokerName}, + "Username_is_case_insensitive": {user: "UserWithBroker", wantBroker: mockBrokerGeneratedID}, "Returns_empty_when_user_does_not_exist": {user: "nonexistent", wantBroker: ""}, "Returns_empty_when_user_does_not_have_a_broker": {user: "userwithoutbroker", wantBroker: ""}, From 5f84f807dca538bae0464a7905bc349b24ea59c3 Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Mon, 17 Nov 2025 15:43:07 +0100 Subject: [PATCH 04/16] Use lowercase username in SetDefaultBrokerForUser --- internal/services/pam/pam.go | 18 +++-- internal/services/pam/pam_test.go | 1 + .../Username_is_case_insensitive/cache.db | 76 +++++++++++++++++++ 3 files changed, 88 insertions(+), 7 deletions(-) create mode 100644 internal/services/pam/testdata/golden/TestSetDefaultBrokerForUser/Username_is_case_insensitive/cache.db diff --git a/internal/services/pam/pam.go b/internal/services/pam/pam.go index 983b26dc53..59e192cae1 100644 --- a/internal/services/pam/pam.go +++ b/internal/services/pam/pam.go @@ -324,25 +324,29 @@ func (s Service) IsAuthenticated(ctx context.Context, req *authd.IARequest) (res func (s Service) SetDefaultBrokerForUser(ctx context.Context, req *authd.SDBFURequest) (empty *authd.Empty, err error) { defer decorate.OnError(&err, "can't set default broker %q for user %q", req.GetBrokerId(), req.GetUsername()) - if req.GetUsername() == "" { + // authd usernames are lowercase + username := strings.ToLower(req.GetUsername()) + brokerID := req.GetBrokerId() + + if username == "" { log.Errorf(ctx, "SetDefaultBrokerForUser: No user name given") return nil, status.Error(codes.InvalidArgument, "no user name given") } // Don't allow setting the default broker to the local broker, because the decision to use the local broker should // be made each time the user tries to log in, based on whether the user is provided by any other NSS service. - if req.GetBrokerId() == brokers.LocalBrokerName { - log.Errorf(ctx, "SetDefaultBrokerForUser: Can't set local broker as default for user %q", req.GetUsername()) + if brokerID == brokers.LocalBrokerName { + log.Errorf(ctx, "SetDefaultBrokerForUser: Can't set local broker as default for user %q", username) return nil, status.Error(codes.InvalidArgument, "can't set local broker as default") } - if err = s.brokerManager.SetDefaultBrokerForUser(req.GetBrokerId(), req.GetUsername()); err != nil { - log.Errorf(ctx, "SetDefaultBrokerForUser: Could not set default broker %q for user %q: %v", req.GetBrokerId(), req.GetUsername(), err) + if err = s.brokerManager.SetDefaultBrokerForUser(brokerID, username); err != nil { + log.Errorf(ctx, "SetDefaultBrokerForUser: Could not set default broker %q for user %q: %v", brokerID, username, err) return &authd.Empty{}, err } - if err = s.userManager.UpdateBrokerForUser(req.GetUsername(), req.GetBrokerId()); err != nil { - log.Errorf(ctx, "SetDefaultBrokerForUser: Could not update broker for user %q in database: %v", req.GetUsername(), err) + if err = s.userManager.UpdateBrokerForUser(username, brokerID); err != nil { + log.Errorf(ctx, "SetDefaultBrokerForUser: Could not update broker for user %q in database: %v", username, err) return &authd.Empty{}, err } diff --git a/internal/services/pam/pam_test.go b/internal/services/pam/pam_test.go index 3d8bdda8ef..df01acf5de 100644 --- a/internal/services/pam/pam_test.go +++ b/internal/services/pam/pam_test.go @@ -614,6 +614,7 @@ func TestSetDefaultBrokerForUser(t *testing.T) { }{ "Set_default_broker_for_existing_user_with_no_broker": {username: "usersetbroker"}, "Update_default_broker_for_existing_user_with_a_broker": {username: "userupdatebroker"}, + "Username_is_case_insensitive": {username: "UserSetBroker"}, "Error_when_setting_default_broker_to_local_broker": {username: "userlocalbroker", brokerID: brokers.LocalBrokerName, wantErr: true}, "Error_when_not_root": {username: "usersetbroker", currentUserNotRoot: true, wantErr: true}, diff --git a/internal/services/pam/testdata/golden/TestSetDefaultBrokerForUser/Username_is_case_insensitive/cache.db b/internal/services/pam/testdata/golden/TestSetDefaultBrokerForUser/Username_is_case_insensitive/cache.db new file mode 100644 index 0000000000..3bd5df74ef --- /dev/null +++ b/internal/services/pam/testdata/golden/TestSetDefaultBrokerForUser/Username_is_case_insensitive/cache.db @@ -0,0 +1,76 @@ +users: + - name: userwithbroker + uid: 1111 + gid: 11111 + gecos: |- + userwithbroker gecos + On multiple lines + dir: /home/userwithbroker + shell: /bin/bash + broker_id: broker-id + - name: userwithinactivebroker + uid: 2222 + gid: 22222 + gecos: userwithinactivebroker + dir: /home/userwithinactivebroker + shell: /bin/dash + broker_id: inactive-broker-id + - name: userlocalbroker + uid: 3333 + gid: 33333 + gecos: userlocalbroker + dir: /home/userlocalbroker + shell: /bin/zsh + - name: usersetbroker + uid: 4444 + gid: 44444 + gecos: usersetbroker + dir: /home/usersetbroker + shell: /bin/dash + broker_id: "1902181170" + - name: userupdatebroker + uid: 5555 + gid: 55555 + gecos: userupdatebroker + dir: /home/userupdatebroker + shell: /bin/zsh + broker_id: tobereplaced-broker-id +groups: + - name: group1 + gid: 11111 + ugid: group1 + - name: group2 + gid: 22222 + ugid: group2 + - name: group3 + gid: 33333 + ugid: group3 + - name: group4 + gid: 44444 + ugid: group4 + - name: group5 + gid: 55555 + ugid: group5 + - name: commongroup + gid: 99999 + ugid: commongroup +users_to_groups: + - uid: 1111 + gid: 11111 + - uid: 2222 + gid: 22222 + - uid: 2222 + gid: 99999 + - uid: 3333 + gid: 33333 + - uid: 3333 + gid: 99999 + - uid: 4444 + gid: 44444 + - uid: 4444 + gid: 99999 + - uid: 5555 + gid: 55555 + - uid: 5555 + gid: 99999 +schema_version: 2 From cd31aca5e909fd83438ad10e40b70edd19de63e0 Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Mon, 17 Nov 2025 15:43:24 +0100 Subject: [PATCH 05/16] Minor refactoring Immediately convert username to lowercase, for consistency with how it's done in the other methods. --- internal/services/pam/pam.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/services/pam/pam.go b/internal/services/pam/pam.go index 59e192cae1..ba40b23c4c 100644 --- a/internal/services/pam/pam.go +++ b/internal/services/pam/pam.go @@ -128,13 +128,11 @@ func (s Service) GetPreviousBroker(ctx context.Context, req *authd.GPBRequest) ( // SelectBroker starts a new session and selects the requested broker for the user. func (s Service) SelectBroker(ctx context.Context, req *authd.SBRequest) (resp *authd.SBResponse, err error) { - username := req.GetUsername() + // authd usernames are lowercase + username := strings.ToLower(req.GetUsername()) brokerID := req.GetBrokerId() lang := req.GetLang() - // authd usernames are lowercase - username = strings.ToLower(username) - if username == "" { log.Errorf(ctx, "SelectBroker: No user name provided") return nil, status.Error(codes.InvalidArgument, "no user name provided") From 174db19ce8b9f87c699511fd76117c74b4369f44 Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Mon, 17 Nov 2025 16:27:52 +0100 Subject: [PATCH 06/16] Convert groups to lowercase in pam.Service.IsAuthenticated --- internal/services/pam/pam.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/services/pam/pam.go b/internal/services/pam/pam.go index ba40b23c4c..a6b2fea97a 100644 --- a/internal/services/pam/pam.go +++ b/internal/services/pam/pam.go @@ -288,8 +288,11 @@ func (s Service) IsAuthenticated(ctx context.Context, req *authd.IARequest) (res return nil, fmt.Errorf("user data from broker invalid: %v", err) } - // authd uses lowercase usernames + // authd uses lowercase user and group names uInfo.Name = strings.ToLower(uInfo.Name) + for i, g := range uInfo.Groups { + uInfo.Groups[i].Name = strings.ToLower(g.Name) + } // Check if the user is locked. We can only do this after the broker has granted access, because we want to avoid // leaking whether a user exists or not to unauthenticated users. From 8b24d4221ad0ca75f399ca37e09005b9400732e6 Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Mon, 17 Nov 2025 16:28:50 +0100 Subject: [PATCH 07/16] TestIsAuthenticated: Test uppercase user and group names --- internal/services/pam/pam_test.go | 28 +++++++++++++---- .../IsAuthenticated | 4 +++ .../cache.db | 20 +++++++++++++ .../IsAuthenticated | 4 +++ .../cache.db | 30 +++++++++++++++++++ internal/testutils/broker.go | 7 +++++ 6 files changed, 87 insertions(+), 6 deletions(-) create mode 100644 internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_user_with_uppercase/IsAuthenticated create mode 100644 internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_user_with_uppercase/cache.db create mode 100644 internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_groups_with_uppercase/IsAuthenticated create mode 100644 internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_groups_with_uppercase/cache.db diff --git a/internal/services/pam/pam_test.go b/internal/services/pam/pam_test.go index df01acf5de..055b2d43cf 100644 --- a/internal/services/pam/pam_test.go +++ b/internal/services/pam/pam_test.go @@ -436,11 +436,13 @@ func TestIsAuthenticated(t *testing.T) { // There is no wantErr as it's stored in the golden file. }{ - "Successfully_authenticate": {username: "success"}, - "Successfully_authenticate_if_first_call_is_canceled": {username: "ia_second_call", secondCall: true, cancelFirstCall: true}, - "Denies_authentication_when_broker_times_out": {username: "ia_timeout"}, - "Update_existing_DB_on_success": {username: "success", existingDB: "cache-with-user.db"}, - "Update_local_groups": {username: "success_with_local_groups", localGroupsFile: "valid.group"}, + "Successfully_authenticate": {username: "success"}, + "Successfully_authenticate_if_first_call_is_canceled": {username: "ia_second_call", secondCall: true, cancelFirstCall: true}, + "Denies_authentication_when_broker_times_out": {username: "ia_timeout"}, + "Update_existing_DB_on_success": {username: "success", existingDB: "cache-with-user.db"}, + "Update_local_groups": {username: "success_with_local_groups", localGroupsFile: "valid.group"}, + "Successfully_authenticate_user_with_uppercase": {username: "SUCCESS"}, + "Successfully_authenticate_with_groups_with_uppercase": {username: "success_with_uppercase_groups"}, // service errors "Error_when_not_root": {username: "success", currentUserNotRoot: true}, @@ -480,7 +482,7 @@ func TestIsAuthenticated(t *testing.T) { managerOpts := []users.Option{ users.WithIDGenerator(&users.IDGeneratorMock{ UIDsToGenerate: []uint32{1111}, - GIDsToGenerate: []uint32{22222}, + GIDsToGenerate: []uint32{22222, 33333, 44444}, }), } @@ -548,6 +550,20 @@ func TestIsAuthenticated(t *testing.T) { got = permissions.Z_ForTests_IdempotentPermissionError(got) golden.CheckOrUpdate(t, got, golden.WithPath("IsAuthenticated")) + // Check that all usernames in the database are lowercase + allUsers, err := m.AllUsers() + require.NoError(t, err, "Setup: failed to get users from manager") + for _, u := range allUsers { + require.Equal(t, strings.ToLower(u.Name), u.Name, "all usernames in the database should be lowercase") + } + + // Check that all groups in the database are lowercase + groups, err := m.AllGroups() + require.NoError(t, err, "Setup: failed to get groups from manager") + for _, group := range groups { + require.Equal(t, strings.ToLower(group.Name), group.Name, "all groups in the database should be lowercase") + } + // Check that database has been updated too. gotDB, err := db.Z_ForTests_DumpNormalizedYAML(userstestutils.GetManagerDB(m)) require.NoError(t, err, "Setup: failed to dump database for comparing") diff --git a/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_user_with_uppercase/IsAuthenticated b/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_user_with_uppercase/IsAuthenticated new file mode 100644 index 0000000000..0db1ac0491 --- /dev/null +++ b/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_user_with_uppercase/IsAuthenticated @@ -0,0 +1,4 @@ +FIRST CALL: + access: granted + msg: + err: diff --git a/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_user_with_uppercase/cache.db b/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_user_with_uppercase/cache.db new file mode 100644 index 0000000000..852b775995 --- /dev/null +++ b/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_user_with_uppercase/cache.db @@ -0,0 +1,20 @@ +users: + - name: testisauthenticated/successfully_authenticate_user_with_uppercase_separator_success + uid: 1111 + gid: 1111 + gecos: gecos for success + dir: /home/success + shell: /bin/sh/success +groups: + - name: testisauthenticated/successfully_authenticate_user_with_uppercase_separator_success + gid: 1111 + ugid: testisauthenticated/successfully_authenticate_user_with_uppercase_separator_success + - name: group-success + gid: 22222 + ugid: ugid-success +users_to_groups: + - uid: 1111 + gid: 1111 + - uid: 1111 + gid: 22222 +schema_version: 2 diff --git a/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_groups_with_uppercase/IsAuthenticated b/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_groups_with_uppercase/IsAuthenticated new file mode 100644 index 0000000000..0db1ac0491 --- /dev/null +++ b/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_groups_with_uppercase/IsAuthenticated @@ -0,0 +1,4 @@ +FIRST CALL: + access: granted + msg: + err: diff --git a/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_groups_with_uppercase/cache.db b/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_groups_with_uppercase/cache.db new file mode 100644 index 0000000000..6ebea1477d --- /dev/null +++ b/internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_groups_with_uppercase/cache.db @@ -0,0 +1,30 @@ +users: + - name: testisauthenticated/successfully_authenticate_with_groups_with_uppercase_separator_success_with_uppercase_groups + uid: 1111 + gid: 1111 + gecos: gecos for success_with_uppercase_groups + dir: /home/success_with_uppercase_groups + shell: /bin/sh/success_with_uppercase_groups +groups: + - name: testisauthenticated/successfully_authenticate_with_groups_with_uppercase_separator_success_with_uppercase_groups + gid: 1111 + ugid: testisauthenticated/successfully_authenticate_with_groups_with_uppercase_separator_success_with_uppercase_groups + - name: group-success_with_uppercase_groups + gid: 22222 + ugid: ugid-success_with_uppercase_groups + - name: group1 + gid: 33333 + ugid: "12345678" + - name: group2 + gid: 44444 + ugid: "87654321" +users_to_groups: + - uid: 1111 + gid: 1111 + - uid: 1111 + gid: 22222 + - uid: 1111 + gid: 33333 + - uid: 1111 + gid: 44444 +schema_version: 2 diff --git a/internal/testutils/broker.go b/internal/testutils/broker.go index 98e4d38504..c744da7eb6 100644 --- a/internal/testutils/broker.go +++ b/internal/testutils/broker.go @@ -276,6 +276,13 @@ func (b *BrokerBusMock) IsAuthenticated(sessionID, authenticationData string) (a extragroups := []groupJSONInfo{{Name: "localgroup1"}, {Name: "localgroup3"}} data = fmt.Sprintf(`{"userinfo": %s}`, userInfoFromName(sessionID, extragroups)) + case "success_with_uppercase_groups": + extragroups := []groupJSONInfo{ + {Name: "GROUP1", UGID: "12345678"}, + {Name: "GROUP2", UGID: "87654321"}, + } + data = fmt.Sprintf(`{"userinfo": %s}`, userInfoFromName(sessionID, extragroups)) + case "ia_invalid_access": access = "invalid" From cf81760c4c639e7b98635483fe78b45c8c1da8f5 Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Mon, 17 Nov 2025 16:34:29 +0100 Subject: [PATCH 08/16] Fix typo --- internal/brokers/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/brokers/manager.go b/internal/brokers/manager.go index ad33c59710..b24ada69e8 100644 --- a/internal/brokers/manager.go +++ b/internal/brokers/manager.go @@ -166,7 +166,7 @@ func (m *Manager) BrokerFromSessionID(id string) (broker *Broker, err error) { return broker, nil } -// NewSession create a new session for the broker and store the sesssionID on the manager. +// NewSession create a new session for the broker and store the sessionID on the manager. func (m *Manager) NewSession(brokerID, username, lang, mode string) (sessionID string, encryptionKey string, err error) { broker, err := m.brokerFromID(brokerID) if err != nil { From 1971dcf4a6795b3c3d5540c387dbdee5f2b1da40 Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Mon, 17 Nov 2025 16:36:22 +0100 Subject: [PATCH 09/16] Convert names to lowercase in user service methods --- internal/services/user/user.go | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/internal/services/user/user.go b/internal/services/user/user.go index a67fa258d2..321971a66e 100644 --- a/internal/services/user/user.go +++ b/internal/services/user/user.go @@ -40,7 +40,8 @@ func NewService(ctx context.Context, userManager *users.Manager, brokerManager * // GetUserByName returns the user entry for the given username. func (s Service) GetUserByName(ctx context.Context, req *authd.GetUserByNameRequest) (*authd.User, error) { - name := req.GetName() + // authd usernames are lowercase + name := strings.ToLower(req.GetName()) if name == "" { log.Warningf(ctx, "GetUserByName: no user name provided") return nil, status.Error(codes.InvalidArgument, "no user name provided") @@ -122,11 +123,14 @@ func (s Service) LockUser(ctx context.Context, req *authd.LockUserRequest) (*aut return nil, err } - if req.GetName() == "" { + // authd uses lowercase usernames. + name := strings.ToLower(req.GetName()) + + if name == "" { return nil, status.Error(codes.InvalidArgument, "no user name provided") } - if err := s.userManager.LockUser(req.GetName()); err != nil { + if err := s.userManager.LockUser(name); err != nil { return nil, grpcError(err) } @@ -139,11 +143,14 @@ func (s Service) UnlockUser(ctx context.Context, req *authd.UnlockUserRequest) ( return nil, err } - if req.GetName() == "" { + // authd uses lowercase usernames. + name := strings.ToLower(req.GetName()) + + if name == "" { return nil, status.Error(codes.InvalidArgument, "no user name provided") } - if err := s.userManager.UnlockUser(req.GetName()); err != nil { + if err := s.userManager.UnlockUser(name); err != nil { return nil, grpcError(err) } @@ -152,12 +159,15 @@ func (s Service) UnlockUser(ctx context.Context, req *authd.UnlockUserRequest) ( // GetGroupByName returns the group entry for the given group name. func (s Service) GetGroupByName(ctx context.Context, req *authd.GetGroupByNameRequest) (*authd.Group, error) { - if req.GetName() == "" { + // authd uses lowercase group names. + name := strings.ToLower(req.GetName()) + + if name == "" { log.Warningf(ctx, "GetGroupByName: no group name provided") return nil, status.Error(codes.InvalidArgument, "no group name provided") } - g, err := s.userManager.GroupByName(req.GetName()) + g, err := s.userManager.GroupByName(name) if errors.Is(err, users.NoDataFoundError{}) { // Only log this at debug level, see GetUserByName for details log.Debugf(context.Background(), "GetGroupByName: %v", err) From 78c8b275f6759d59602c05f9cd1d2c2ebc9ca0d6 Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Mon, 17 Nov 2025 16:40:52 +0100 Subject: [PATCH 10/16] Fix test name We use underscores instead of spaces in test names. --- internal/services/user/user_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/services/user/user_test.go b/internal/services/user/user_test.go index 80ddad7ec6..fac0398c1e 100644 --- a/internal/services/user/user_test.go +++ b/internal/services/user/user_test.go @@ -53,7 +53,7 @@ func TestGetUserByName(t *testing.T) { wantErrNotExists bool }{ "Return_existing_user": {username: "user1"}, - "Return existing user with different capitalization": {username: "USER1"}, + "Return_existing_user_with_different_capitalization": {username: "USER1"}, "Precheck_user_if_not_in_db": {username: "user-pre-check", shouldPreCheck: true}, "Prechecked_user_with_upper_cases_in_username_has_same_id_as_lower_case": {username: "User-Pre-Check", shouldPreCheck: true}, From cb419206613cac295fb84c78f11cba3b5e023cbd Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Mon, 17 Nov 2025 16:41:34 +0100 Subject: [PATCH 11/16] Test GetGroupByName with uppercase group --- .../Return_existing_group_with_different_capitalization | 5 +++++ internal/services/user/user_test.go | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 internal/services/user/testdata/golden/TestGetGroupByName/Return_existing_group_with_different_capitalization diff --git a/internal/services/user/testdata/golden/TestGetGroupByName/Return_existing_group_with_different_capitalization b/internal/services/user/testdata/golden/TestGetGroupByName/Return_existing_group_with_different_capitalization new file mode 100644 index 0000000000..3b895b5e0a --- /dev/null +++ b/internal/services/user/testdata/golden/TestGetGroupByName/Return_existing_group_with_different_capitalization @@ -0,0 +1,5 @@ +name: group1 +gid: 11111 +members: + - user1 +passwd: "" diff --git a/internal/services/user/user_test.go b/internal/services/user/user_test.go index fac0398c1e..d48e095571 100644 --- a/internal/services/user/user_test.go +++ b/internal/services/user/user_test.go @@ -119,7 +119,8 @@ func TestGetGroupByName(t *testing.T) { wantErr bool wantErrNotExists bool }{ - "Return_existing_group": {groupname: "group1"}, + "Return_existing_group": {groupname: "group1"}, + "Return_existing_group_with_different_capitalization": {groupname: "GROUP1"}, "Error_with_typed_GRPC_notfound_code_on_unexisting_user": {groupname: "does-not-exists", wantErr: true, wantErrNotExists: true}, "Error_on_missing_name": {wantErr: true}, From 258dbb88814121d5ecc0ee7372701a6692cec3bc Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Mon, 17 Nov 2025 16:42:07 +0100 Subject: [PATCH 12/16] Check that user and group names are lowercase --- internal/services/user/user_test.go | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/internal/services/user/user_test.go b/internal/services/user/user_test.go index d48e095571..7754404f10 100644 --- a/internal/services/user/user_test.go +++ b/internal/services/user/user_test.go @@ -6,6 +6,7 @@ import ( "net" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/require" @@ -73,8 +74,13 @@ func TestGetUserByName(t *testing.T) { client := newUserServiceClient(t, tc.dbFile) - got, err := client.GetUserByName(context.Background(), &authd.GetUserByNameRequest{Name: tc.username, ShouldPreCheck: tc.shouldPreCheck}) - requireExpectedResult(t, "GetUserByName", got, err, tc.wantErr, tc.wantErrNotExists) + u, err := client.GetUserByName(context.Background(), &authd.GetUserByNameRequest{Name: tc.username, ShouldPreCheck: tc.shouldPreCheck}) + requireExpectedResult(t, "GetUserByName", u, err, tc.wantErr, tc.wantErrNotExists) + + // Check that the user name is lowercase + if u != nil { + require.Equal(t, u.Name, strings.ToLower(u.Name), "User name should be lowercase") + } if !tc.shouldPreCheck || tc.wantErr { return @@ -129,8 +135,13 @@ func TestGetGroupByName(t *testing.T) { t.Run(name, func(t *testing.T) { client := newUserServiceClient(t, tc.dbFile) - got, err := client.GetGroupByName(context.Background(), &authd.GetGroupByNameRequest{Name: tc.groupname}) - requireExpectedResult(t, "GetGroupByName", got, err, tc.wantErr, tc.wantErrNotExists) + group, err := client.GetGroupByName(context.Background(), &authd.GetGroupByNameRequest{Name: tc.groupname}) + requireExpectedResult(t, "GetGroupByName", group, err, tc.wantErr, tc.wantErrNotExists) + + // Check that the group name is lowercase + if group != nil { + require.Equal(t, group.Name, strings.ToLower(group.Name), "Group name should be lowercase") + } }) } } From 71cbe6a54bb9beab1137ec8c9b334f4a9b192a76 Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Mon, 17 Nov 2025 16:51:25 +0100 Subject: [PATCH 13/16] Test LockUser and UnlockUser with uppercase --- internal/services/user/user_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/services/user/user_test.go b/internal/services/user/user_test.go index 7754404f10..d479c708e0 100644 --- a/internal/services/user/user_test.go +++ b/internal/services/user/user_test.go @@ -233,7 +233,8 @@ func TestLockUser(t *testing.T) { wantErr bool }{ - "Successfully_lock_user": {username: "user1"}, + "Successfully_lock_user": {username: "user1"}, + "Successfully_lock_user_with_uppercase": {username: "USER1"}, "Error_when_username_is_empty": {wantErr: true}, "Error_when_user_does_not_exist": {username: "doesnotexist", wantErr: true}, @@ -262,7 +263,8 @@ func TestUnlockUser(t *testing.T) { wantErr bool }{ - "Successfully_unlock_user": {username: "user1"}, + "Successfully_unlock_user": {username: "user1"}, + "Successfully_unlock_user_with_uppercase": {username: "USER1"}, "Error_when_username_is_empty": {wantErr: true}, "Error_when_user_does_not_exist": {username: "doesnotexist", wantErr: true}, From 8a18e386959563d9b3ca6db3e1061977cd098643 Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Mon, 17 Nov 2025 17:03:21 +0100 Subject: [PATCH 14/16] Rename test cases For consistency with the other *_with_uppercase test cases. --- ...apitalization => Return_existing_group_with_uppercase} | 0 ...capitalization => Return_existing_user_with_uppercase} | 0 internal/services/user/user_test.go | 8 ++++---- 3 files changed, 4 insertions(+), 4 deletions(-) rename internal/services/user/testdata/golden/TestGetGroupByName/{Return_existing_group_with_different_capitalization => Return_existing_group_with_uppercase} (100%) rename internal/services/user/testdata/golden/TestGetUserByName/{Return_existing_user_with_different_capitalization => Return_existing_user_with_uppercase} (100%) diff --git a/internal/services/user/testdata/golden/TestGetGroupByName/Return_existing_group_with_different_capitalization b/internal/services/user/testdata/golden/TestGetGroupByName/Return_existing_group_with_uppercase similarity index 100% rename from internal/services/user/testdata/golden/TestGetGroupByName/Return_existing_group_with_different_capitalization rename to internal/services/user/testdata/golden/TestGetGroupByName/Return_existing_group_with_uppercase diff --git a/internal/services/user/testdata/golden/TestGetUserByName/Return_existing_user_with_different_capitalization b/internal/services/user/testdata/golden/TestGetUserByName/Return_existing_user_with_uppercase similarity index 100% rename from internal/services/user/testdata/golden/TestGetUserByName/Return_existing_user_with_different_capitalization rename to internal/services/user/testdata/golden/TestGetUserByName/Return_existing_user_with_uppercase diff --git a/internal/services/user/user_test.go b/internal/services/user/user_test.go index d479c708e0..c47a24c7be 100644 --- a/internal/services/user/user_test.go +++ b/internal/services/user/user_test.go @@ -53,8 +53,8 @@ func TestGetUserByName(t *testing.T) { wantErr bool wantErrNotExists bool }{ - "Return_existing_user": {username: "user1"}, - "Return_existing_user_with_different_capitalization": {username: "USER1"}, + "Return_existing_user": {username: "user1"}, + "Return_existing_user_with_uppercase": {username: "USER1"}, "Precheck_user_if_not_in_db": {username: "user-pre-check", shouldPreCheck: true}, "Prechecked_user_with_upper_cases_in_username_has_same_id_as_lower_case": {username: "User-Pre-Check", shouldPreCheck: true}, @@ -125,8 +125,8 @@ func TestGetGroupByName(t *testing.T) { wantErr bool wantErrNotExists bool }{ - "Return_existing_group": {groupname: "group1"}, - "Return_existing_group_with_different_capitalization": {groupname: "GROUP1"}, + "Return_existing_group": {groupname: "group1"}, + "Return_existing_group_with_uppercase": {groupname: "GROUP1"}, "Error_with_typed_GRPC_notfound_code_on_unexisting_user": {groupname: "does-not-exists", wantErr: true, wantErrNotExists: true}, "Error_on_missing_name": {wantErr: true}, From c4e47f6fea3ac7dd0edb1a80a01b5ca11d1f2900 Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Mon, 17 Nov 2025 17:09:26 +0100 Subject: [PATCH 15/16] Store DB content as golden file in lock/unlock tests The tests change DB content, so we should also check the result. --- .../TestLockUser/Successfully_lock_user | 50 +++++++++++++++++++ .../Successfully_lock_user_with_uppercase | 50 +++++++++++++++++++ .../TestUnlockUser/Successfully_unlock_user | 49 ++++++++++++++++++ .../Successfully_unlock_user_with_uppercase | 49 ++++++++++++++++++ internal/services/user/user_test.go | 31 ++++++++---- 5 files changed, 218 insertions(+), 11 deletions(-) create mode 100644 internal/services/user/testdata/golden/TestLockUser/Successfully_lock_user create mode 100644 internal/services/user/testdata/golden/TestLockUser/Successfully_lock_user_with_uppercase create mode 100644 internal/services/user/testdata/golden/TestUnlockUser/Successfully_unlock_user create mode 100644 internal/services/user/testdata/golden/TestUnlockUser/Successfully_unlock_user_with_uppercase diff --git a/internal/services/user/testdata/golden/TestLockUser/Successfully_lock_user b/internal/services/user/testdata/golden/TestLockUser/Successfully_lock_user new file mode 100644 index 0000000000..62f11236fc --- /dev/null +++ b/internal/services/user/testdata/golden/TestLockUser/Successfully_lock_user @@ -0,0 +1,50 @@ +users: + - name: user1 + uid: 1111 + gid: 11111 + gecos: |- + User1 gecos + On multiple lines + dir: /home/user1 + shell: /bin/bash + broker_id: broker-id + locked: true + - name: user2 + uid: 2222 + gid: 22222 + gecos: User2 + dir: /home/user2 + shell: /bin/dash + broker_id: broker-id + - name: user3 + uid: 3333 + gid: 33333 + gecos: User3 + dir: /home/user3 + shell: /bin/zsh + broker_id: broker-id +groups: + - name: group1 + gid: 11111 + ugid: group1 + - name: group2 + gid: 22222 + ugid: group2 + - name: group3 + gid: 33333 + ugid: group3 + - name: commongroup + gid: 99999 + ugid: commongroup +users_to_groups: + - uid: 1111 + gid: 11111 + - uid: 2222 + gid: 22222 + - uid: 2222 + gid: 99999 + - uid: 3333 + gid: 33333 + - uid: 3333 + gid: 99999 +schema_version: 2 diff --git a/internal/services/user/testdata/golden/TestLockUser/Successfully_lock_user_with_uppercase b/internal/services/user/testdata/golden/TestLockUser/Successfully_lock_user_with_uppercase new file mode 100644 index 0000000000..62f11236fc --- /dev/null +++ b/internal/services/user/testdata/golden/TestLockUser/Successfully_lock_user_with_uppercase @@ -0,0 +1,50 @@ +users: + - name: user1 + uid: 1111 + gid: 11111 + gecos: |- + User1 gecos + On multiple lines + dir: /home/user1 + shell: /bin/bash + broker_id: broker-id + locked: true + - name: user2 + uid: 2222 + gid: 22222 + gecos: User2 + dir: /home/user2 + shell: /bin/dash + broker_id: broker-id + - name: user3 + uid: 3333 + gid: 33333 + gecos: User3 + dir: /home/user3 + shell: /bin/zsh + broker_id: broker-id +groups: + - name: group1 + gid: 11111 + ugid: group1 + - name: group2 + gid: 22222 + ugid: group2 + - name: group3 + gid: 33333 + ugid: group3 + - name: commongroup + gid: 99999 + ugid: commongroup +users_to_groups: + - uid: 1111 + gid: 11111 + - uid: 2222 + gid: 22222 + - uid: 2222 + gid: 99999 + - uid: 3333 + gid: 33333 + - uid: 3333 + gid: 99999 +schema_version: 2 diff --git a/internal/services/user/testdata/golden/TestUnlockUser/Successfully_unlock_user b/internal/services/user/testdata/golden/TestUnlockUser/Successfully_unlock_user new file mode 100644 index 0000000000..27a57ce023 --- /dev/null +++ b/internal/services/user/testdata/golden/TestUnlockUser/Successfully_unlock_user @@ -0,0 +1,49 @@ +users: + - name: user1 + uid: 1111 + gid: 11111 + gecos: |- + User1 gecos + On multiple lines + dir: /home/user1 + shell: /bin/bash + broker_id: broker-id + - name: user2 + uid: 2222 + gid: 22222 + gecos: User2 + dir: /home/user2 + shell: /bin/dash + broker_id: broker-id + - name: user3 + uid: 3333 + gid: 33333 + gecos: User3 + dir: /home/user3 + shell: /bin/zsh + broker_id: broker-id +groups: + - name: group1 + gid: 11111 + ugid: group1 + - name: group2 + gid: 22222 + ugid: group2 + - name: group3 + gid: 33333 + ugid: group3 + - name: commongroup + gid: 99999 + ugid: commongroup +users_to_groups: + - uid: 1111 + gid: 11111 + - uid: 2222 + gid: 22222 + - uid: 2222 + gid: 99999 + - uid: 3333 + gid: 33333 + - uid: 3333 + gid: 99999 +schema_version: 2 diff --git a/internal/services/user/testdata/golden/TestUnlockUser/Successfully_unlock_user_with_uppercase b/internal/services/user/testdata/golden/TestUnlockUser/Successfully_unlock_user_with_uppercase new file mode 100644 index 0000000000..27a57ce023 --- /dev/null +++ b/internal/services/user/testdata/golden/TestUnlockUser/Successfully_unlock_user_with_uppercase @@ -0,0 +1,49 @@ +users: + - name: user1 + uid: 1111 + gid: 11111 + gecos: |- + User1 gecos + On multiple lines + dir: /home/user1 + shell: /bin/bash + broker_id: broker-id + - name: user2 + uid: 2222 + gid: 22222 + gecos: User2 + dir: /home/user2 + shell: /bin/dash + broker_id: broker-id + - name: user3 + uid: 3333 + gid: 33333 + gecos: User3 + dir: /home/user3 + shell: /bin/zsh + broker_id: broker-id +groups: + - name: group1 + gid: 11111 + ugid: group1 + - name: group2 + gid: 22222 + ugid: group2 + - name: group3 + gid: 33333 + ugid: group3 + - name: commongroup + gid: 99999 + ugid: commongroup +users_to_groups: + - uid: 1111 + gid: 11111 + - uid: 2222 + gid: 22222 + - uid: 2222 + gid: 99999 + - uid: 3333 + gid: 33333 + - uid: 3333 + gid: 99999 +schema_version: 2 diff --git a/internal/services/user/user_test.go b/internal/services/user/user_test.go index c47a24c7be..8389960207 100644 --- a/internal/services/user/user_test.go +++ b/internal/services/user/user_test.go @@ -20,6 +20,7 @@ import ( "github.com/ubuntu/authd/internal/users" "github.com/ubuntu/authd/internal/users/db" userslocking "github.com/ubuntu/authd/internal/users/locking" + userstestutils "github.com/ubuntu/authd/internal/users/testutils" "github.com/ubuntu/authd/log" "google.golang.org/grpc" "google.golang.org/grpc/codes" @@ -72,7 +73,7 @@ func TestGetUserByName(t *testing.T) { userslocking.Z_ForTests_OverrideLockingWithCleanup(t) } - client := newUserServiceClient(t, tc.dbFile) + client, _ := newUserServiceClient(t, tc.dbFile) u, err := client.GetUserByName(context.Background(), &authd.GetUserByNameRequest{Name: tc.username, ShouldPreCheck: tc.shouldPreCheck}) requireExpectedResult(t, "GetUserByName", u, err, tc.wantErr, tc.wantErrNotExists) @@ -108,7 +109,7 @@ func TestGetUserByID(t *testing.T) { } for name, tc := range tests { t.Run(name, func(t *testing.T) { - client := newUserServiceClient(t, tc.dbFile) + client, _ := newUserServiceClient(t, tc.dbFile) got, err := client.GetUserByID(context.Background(), &authd.GetUserByIDRequest{Id: tc.uid}) requireExpectedResult(t, "GetUserByID", got, err, tc.wantErr, tc.wantErrNotExists) @@ -133,7 +134,7 @@ func TestGetGroupByName(t *testing.T) { } for name, tc := range tests { t.Run(name, func(t *testing.T) { - client := newUserServiceClient(t, tc.dbFile) + client, _ := newUserServiceClient(t, tc.dbFile) group, err := client.GetGroupByName(context.Background(), &authd.GetGroupByNameRequest{Name: tc.groupname}) requireExpectedResult(t, "GetGroupByName", group, err, tc.wantErr, tc.wantErrNotExists) @@ -162,7 +163,7 @@ func TestGetGroupByID(t *testing.T) { } for name, tc := range tests { t.Run(name, func(t *testing.T) { - client := newUserServiceClient(t, tc.dbFile) + client, _ := newUserServiceClient(t, tc.dbFile) got, err := client.GetGroupByID(context.Background(), &authd.GetGroupByIDRequest{Id: tc.gid}) requireExpectedResult(t, "GetGroupByID", got, err, tc.wantErr, tc.wantErrNotExists) @@ -185,7 +186,7 @@ func TestListUsers(t *testing.T) { tc.dbFile = "default.db.yaml" } - client := newUserServiceClient(t, tc.dbFile) + client, _ := newUserServiceClient(t, tc.dbFile) resp, err := client.ListUsers(context.Background(), &authd.Empty{}) requireExpectedListResult(t, "ListUsers", resp.GetUsers(), err, tc.wantErr) @@ -208,7 +209,7 @@ func TestListGroups(t *testing.T) { tc.dbFile = "default.db.yaml" } - client := newUserServiceClient(t, tc.dbFile) + client, _ := newUserServiceClient(t, tc.dbFile) resp, err := client.ListGroups(context.Background(), &authd.Empty{}) if tc.wantErr { @@ -242,7 +243,7 @@ func TestLockUser(t *testing.T) { } for name, tc := range tests { t.Run(name, func(t *testing.T) { - client := newUserServiceClient(t, tc.sourceDB) + client, m := newUserServiceClient(t, tc.sourceDB) _, err := client.LockUser(context.Background(), &authd.LockUserRequest{Name: tc.username}) if tc.wantErr { @@ -250,6 +251,10 @@ func TestLockUser(t *testing.T) { return } require.NoError(t, err, "LockUser should not return an error, but did") + + dbContent, err := db.Z_ForTests_DumpNormalizedYAML(userstestutils.GetManagerDB(m)) + require.NoError(t, err, "Setup: failed to dump database for comparing") + golden.CheckOrUpdate(t, dbContent) }) } } @@ -276,7 +281,7 @@ func TestUnlockUser(t *testing.T) { tc.sourceDB = "locked-user.db.yaml" } - client := newUserServiceClient(t, tc.sourceDB) + client, m := newUserServiceClient(t, tc.sourceDB) _, err := client.UnlockUser(context.Background(), &authd.UnlockUserRequest{Name: tc.username}) if tc.wantErr { @@ -284,12 +289,16 @@ func TestUnlockUser(t *testing.T) { return } require.NoError(t, err, "UnlockUser should not return an error, but did") + + dbContent, err := db.Z_ForTests_DumpNormalizedYAML(userstestutils.GetManagerDB(m)) + require.NoError(t, err, "Setup: failed to dump database for comparing") + golden.CheckOrUpdate(t, dbContent) }) } } // newUserServiceClient returns a new gRPC client for the CLI service. -func newUserServiceClient(t *testing.T, dbFile string) (client authd.UserServiceClient) { +func newUserServiceClient(t *testing.T, dbFile string) (client authd.UserServiceClient, userManager *users.Manager) { t.Helper() tmpDir, err := os.MkdirTemp("", "authd-socket-dir") @@ -306,7 +315,7 @@ func newUserServiceClient(t *testing.T, dbFile string) (client authd.UserService require.NoError(t, err, "Setup: could not create database from testdata") } - userManager := newUserManagerForTests(t, dbFile) + userManager = newUserManagerForTests(t, dbFile) brokerManager := newBrokersManagerForTests(t) permissionsManager := permissions.New(permissions.Z_ForTests_WithCurrentUserAsRoot()) service := user.NewService(context.Background(), userManager, brokerManager, &permissionsManager) @@ -328,7 +337,7 @@ func newUserServiceClient(t *testing.T, dbFile string) (client authd.UserService t.Cleanup(func() { _ = conn.Close() }) // We don't care about the error on cleanup - return authd.NewUserServiceClient(conn) + return authd.NewUserServiceClient(conn), userManager } func enableCheckGlobalAccess(s user.Service) grpc.UnaryServerInterceptor { From 8066a6985d8f227a17c8abbcc7d64e6f072afdae Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Mon, 17 Nov 2025 15:47:06 +0100 Subject: [PATCH 16/16] Avoid unnecessary lowercase conversions We now only convert user and group names to lowercase in public API methods. In all other functions, we assume that the names are already in lowercase, to avoid repeatedly converting the same name to lowercase in each layer of our code. --- internal/brokers/manager.go | 3 --- internal/services/user/user.go | 3 --- internal/users/db/db_test.go | 12 ----------- ...pdating_user_with_different_capitalization | 17 ---------------- internal/users/db/update.go | 10 ---------- internal/users/db/users.go | 4 ---- internal/users/manager.go | 7 ------- internal/users/manager_test.go | 2 -- ...es_of_authd_groups_are_stored_in_lowercase | 20 ------------------- ..._update_user_with_different_capitalization | 18 ----------------- 10 files changed, 96 deletions(-) delete mode 100644 internal/users/db/testdata/golden/TestUpdateUserEntry/Updating_user_with_different_capitalization delete mode 100644 internal/users/testdata/golden/TestUpdateUser/Names_of_authd_groups_are_stored_in_lowercase delete mode 100644 internal/users/testdata/golden/TestUpdateUser/Successfully_update_user_with_different_capitalization diff --git a/internal/brokers/manager.go b/internal/brokers/manager.go index b24ada69e8..850dcfd7c8 100644 --- a/internal/brokers/manager.go +++ b/internal/brokers/manager.go @@ -132,9 +132,6 @@ func (m *Manager) SetDefaultBrokerForUser(brokerID, username string) error { return fmt.Errorf("invalid broker: %v", err) } - // authd uses lowercase usernames - username = strings.ToLower(username) - m.usersToBrokerMu.Lock() defer m.usersToBrokerMu.Unlock() m.usersToBroker[username] = broker diff --git a/internal/services/user/user.go b/internal/services/user/user.go index 321971a66e..8a84597cd3 100644 --- a/internal/services/user/user.go +++ b/internal/services/user/user.go @@ -246,9 +246,6 @@ var errUserNotPermitted = errors.New("user not permitted to log in via SSH for t // It returns a types.UserEntry with a unique UID if the user is permitted to log in. // If the user is not permitted to log in by any broker, errUserNotPermitted is returned. func (s Service) userPreCheck(ctx context.Context, username string) (types.UserEntry, error) { - // authd uses lowercase usernames. - username = strings.ToLower(username) - // Check if any broker permits the user to log in via SSH for the first time. var userinfo string var err error diff --git a/internal/users/db/db_test.go b/internal/users/db/db_test.go index b26ff85e11..7dcf6bdf30 100644 --- a/internal/users/db/db_test.go +++ b/internal/users/db/db_test.go @@ -518,13 +518,6 @@ func TestUpdateUserEntry(t *testing.T) { Dir: "/home/user1", Shell: "/bin/bash", }, - "user1-with-capitalization": { - Name: "User1", - UID: 1111, - Gecos: "User1 gecos\nOn multiple lines", - Dir: "/home/user1", - Shell: "/bin/bash", - }, "user3": { Name: "user3", UID: 3333, @@ -562,7 +555,6 @@ func TestUpdateUserEntry(t *testing.T) { "Update_user_does_not_change_homedir_if_it_exists": {userCase: "user1-new-homedir", dbFile: "one_user_and_group"}, "Update_user_does_not_change_shell_if_it_exists": {userCase: "user1-new-shell", dbFile: "one_user_and_group"}, "Update_user_by_removing_optional_gecos_field_if_not_set": {userCase: "user1-without-gecos", dbFile: "one_user_and_group"}, - "Updating_user_with_different_capitalization": {userCase: "user1-with-capitalization", dbFile: "one_user_and_group"}, // Group updates "Update_user_by_adding_a_new_group": {groupCases: []string{"group1", "group2"}, dbFile: "one_user_and_group"}, @@ -869,10 +861,6 @@ func TestUpdateLockedFieldForUser(t *testing.T) { err := c.UpdateLockedFieldForUser("user1", true) require.NoError(t, err, "UpdateLockedFieldForUser for an existent user should not return an error") - // Update broker for existent user with different capitalization - err = c.UpdateLockedFieldForUser("USER1", true) - require.NoError(t, err, "UpdateLockedFieldForUser for an existent user with different capitalization should not return an error") - // Error when updating broker for nonexistent user err = c.UpdateLockedFieldForUser("nonexistent", false) require.Error(t, err, "UpdateLockedFieldForUser for a nonexistent user should return an error") diff --git a/internal/users/db/testdata/golden/TestUpdateUserEntry/Updating_user_with_different_capitalization b/internal/users/db/testdata/golden/TestUpdateUserEntry/Updating_user_with_different_capitalization deleted file mode 100644 index 1ead371466..0000000000 --- a/internal/users/db/testdata/golden/TestUpdateUserEntry/Updating_user_with_different_capitalization +++ /dev/null @@ -1,17 +0,0 @@ -users: - - name: user1 - uid: 1111 - gid: 11111 - gecos: |- - User1 gecos - On multiple lines - dir: /home/user1 - shell: /bin/bash -groups: - - name: group1 - gid: 11111 - ugid: "12345678" -users_to_groups: - - uid: 1111 - gid: 11111 -schema_version: 2 diff --git a/internal/users/db/update.go b/internal/users/db/update.go index c2bd5a7fe8..2d25ee6784 100644 --- a/internal/users/db/update.go +++ b/internal/users/db/update.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "strings" "github.com/mattn/go-sqlite3" "github.com/ubuntu/authd/log" @@ -12,9 +11,6 @@ import ( // UpdateUserEntry inserts or updates user and group records from the user information. func (m *Manager) UpdateUserEntry(user UserRow, authdGroups []GroupRow, localGroups []string) (err error) { - // authd uses lowercase usernames - user.Name = strings.ToLower(user.Name) - // Start a transaction tx, err := m.db.Begin() if err != nil { @@ -164,9 +160,6 @@ func handleUsersToLocalGroupsUpdate(db queryable, uid uint32, localGroups []stri // UpdateBrokerForUser updates the last broker the user successfully authenticated with. func (m *Manager) UpdateBrokerForUser(username, brokerID string) error { - // authd uses lowercase usernames - username = strings.ToLower(username) - query := `UPDATE users SET broker_id = ? WHERE name = ?` res, err := m.db.Exec(query, brokerID, username) if err != nil { @@ -185,9 +178,6 @@ func (m *Manager) UpdateBrokerForUser(username, brokerID string) error { // UpdateLockedFieldForUser sets the "locked" field of a user record. func (m *Manager) UpdateLockedFieldForUser(username string, locked bool) error { - // authd uses lowercase usernames - username = strings.ToLower(username) - query := `UPDATE users SET locked = ? WHERE name = ?` res, err := m.db.Exec(query, locked, username) if err != nil { diff --git a/internal/users/db/users.go b/internal/users/db/users.go index 90369ca933..1dddf2d1d4 100644 --- a/internal/users/db/users.go +++ b/internal/users/db/users.go @@ -5,7 +5,6 @@ import ( "database/sql" "errors" "fmt" - "strings" "github.com/ubuntu/authd/log" ) @@ -68,9 +67,6 @@ func (m *Manager) UserByName(name string) (UserRow, error) { } func userByName(db queryable, name string) (UserRow, error) { - // authd uses lowercase usernames - name = strings.ToLower(name) - query := fmt.Sprintf(`SELECT %s FROM users WHERE name = ?`, publicUserColumns) row := db.QueryRow(query, name) diff --git a/internal/users/manager.go b/internal/users/manager.go index 9817ac1ef0..25044e5e15 100644 --- a/internal/users/manager.go +++ b/internal/users/manager.go @@ -8,7 +8,6 @@ import ( "math" "os" "slices" - "strings" "sync" "syscall" @@ -146,9 +145,6 @@ func (m *Manager) UpdateUser(u types.UserInfo) (err error) { return errors.New("empty username") } - // authd uses lowercase usernames - u.Name = strings.ToLower(u.Name) - // Prepend the user private group u.Groups = append([]types.GroupInfo{{Name: u.Name, UGID: u.Name}}, u.Groups...) userPrivateGroup := &u.Groups[0] @@ -248,9 +244,6 @@ func (m *Manager) UpdateUser(u types.UserInfo) (err error) { continue } - // authd groups are lowercase - g.Name = strings.ToLower(g.Name) - // It's not a local group, so before storing it in the database, check if a group with the same name already // exists. if err := m.checkGroupNameConflict(g.Name, g.UGID); err != nil { diff --git a/internal/users/manager_test.go b/internal/users/manager_test.go index 6039281193..4ae390ef8a 100644 --- a/internal/users/manager_test.go +++ b/internal/users/manager_test.go @@ -192,11 +192,9 @@ func TestUpdateUser(t *testing.T) { "Successfully_update_user_updating_local_groups": {groupsCase: "mixed-groups-authd-first", localGroupsFile: "users_in_groups.group"}, "Successfully_update_user_updating_local_groups_with_changes": {groupsCase: "mixed-groups-authd-first", localGroupsFile: "user_mismatching_groups.group"}, "UID_does_not_change_if_user_already_exists": {userCase: "same-name-different-uid", dbFile: "one_user_and_group", wantSameUID: true}, - "Successfully_update_user_with_different_capitalization": {userCase: "different-capitalization-same-uid", dbFile: "one_user_and_group"}, "GID_does_not_change_if_group_with_same_UGID_exists": {groupsCase: "different-name-same-ugid", dbFile: "one_user_and_group"}, "GID_does_not_change_if_group_with_same_name_and_empty_UGID_exists": {groupsCase: "authd-group", dbFile: "group-with-empty-UGID"}, "Removing_last_user_from_a_group_keeps_the_group_record": {groupsCase: "no-groups", dbFile: "one_user_and_group"}, - "Names_of_authd_groups_are_stored_in_lowercase": {groupsCase: "authd-group-with-uppercase"}, "Error_if_user_has_no_username": {userCase: "nameless", wantErr: true, noOutput: true}, "Error_if_group_has_no_name": {groupsCase: "nameless-group", wantErr: true, noOutput: true}, diff --git a/internal/users/testdata/golden/TestUpdateUser/Names_of_authd_groups_are_stored_in_lowercase b/internal/users/testdata/golden/TestUpdateUser/Names_of_authd_groups_are_stored_in_lowercase deleted file mode 100644 index 5b2e9128ed..0000000000 --- a/internal/users/testdata/golden/TestUpdateUser/Names_of_authd_groups_are_stored_in_lowercase +++ /dev/null @@ -1,20 +0,0 @@ -users: - - name: user1 - uid: 1111 - gid: 1111 - gecos: gecos for user1 - dir: /home/user1 - shell: /bin/bash -groups: - - name: user1 - gid: 1111 - ugid: user1 - - name: group1 - gid: 11111 - ugid: "1" -users_to_groups: - - uid: 1111 - gid: 1111 - - uid: 1111 - gid: 11111 -schema_version: 2 diff --git a/internal/users/testdata/golden/TestUpdateUser/Successfully_update_user_with_different_capitalization b/internal/users/testdata/golden/TestUpdateUser/Successfully_update_user_with_different_capitalization deleted file mode 100644 index 1f611d8210..0000000000 --- a/internal/users/testdata/golden/TestUpdateUser/Successfully_update_user_with_different_capitalization +++ /dev/null @@ -1,18 +0,0 @@ -users: - - name: user1 - uid: 1111 - gid: 1111 - gecos: gecos for User1 - dir: /home/user1 - shell: /bin/bash -groups: - - name: user1 - gid: 1111 - ugid: user1 - - name: group1 - gid: 11111 - ugid: "12345678" -users_to_groups: - - uid: 1111 - gid: 1111 -schema_version: 2