Only convert user and group names to lowercase in API methods

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
@@ -166,7 +163,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 {

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,22 +84,22 @@ 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
 		}
 
 		// We could resolve the user through NSS, which means then that another non authd service
 		// 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
 	}
 
@@ -125,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")
@@ -287,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.
@@ -321,25 +325,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
 	}
 

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: ""},
@@ -435,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},
@@ -479,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},
 				}),
 			}
 
@@ -547,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")
@@ -613,6 +630,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},

internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_user_with_uppercase/IsAuthenticated

@@ -0,0 +1,4 @@
+FIRST CALL:
+	access: granted
+	msg: 
+	err: <nil>

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

internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_groups_with_uppercase/IsAuthenticated

@@ -0,0 +1,4 @@
+FIRST CALL:
+	access: granted
+	msg: 
+	err: <nil>

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

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

internal/services/user/testdata/golden/TestGetGroupByName/Return_existing_group_with_uppercase

@@ -0,0 +1,5 @@
+name: group1
+gid: 11111
+members:
+    - user1
+passwd: ""