Fix login failing with "could not generate GID for group" (#742)

UDENG-5814

internal/services/pam/testdata/TestIsAuthenticated/cache-with-conflicting-uid.db

@@ -1,9 +1,12 @@
 GroupByID:
     "1797931382": '{"Name":"different-user-same-uid","GID":1797931382,"UGID":"different-user-same-uid"}'
-    "1840530284": '{"Name":"group-success","GID":1840530284,"UGID":"group-success"}'
+    "1840530284": '{"Name":"group-success","GID":1840530284,"UGID":"ugid-success"}'
 GroupByName:
     different-user-same-uid: '{"Name":"different-user-same-uid","GID":1797931382,"UGID":"different-user-same-uid"}'
-    group-success: '{"Name":"group-success","GID":1840530284,"UGID":"group-success"}'
+    group-success: '{"Name":"group-success","GID":1840530284,"UGID":"ugid-success"}'
+GroupByUGID:
+    different-user-same-uid: '{"Name":"different-user-same-uid","GID":1797931382,"UGID":"different-user-same-uid"}'
+    ugid-success: '{"Name":"group-success","GID":1840530284,"UGID":"ugid-success"}'
 GroupToUsers:
     "1797931382": '{"GID":1797931382,"UIDs":[1797931382]}'
     "1840530284": '{"GID":1840530284,"UIDs":[1797931382]}'

internal/services/pam/testdata/TestIsAuthenticated/cache-with-user.db

@@ -1,7 +1,9 @@
 GroupByID:
-    "88888": '{"Name":"group-success","GID":88888,"UGID":"group-success"}'
+    "88888": '{"Name":"group-success","GID":88888,"UGID":"ugid-success"}'
 GroupByName:
-    group-success: '{"Name":"group-success","GID":88888,"UGID":"group-success"}'
+    group-success: '{"Name":"group-success","GID":88888,"UGID":"ugid-success"}'
+GroupByUGID:
+    ugid-success: '{"Name":"group-success","GID":88888,"UGID":"ugid-success"}'
 GroupToUsers:
     "88888": '{"GID":88888,"UIDs":[77777]}'
 UserByID:

internal/services/pam/testdata/golden/TestIsAuthenticated/Update_existing_DB_on_success/cache.db

@@ -1,17 +1,15 @@
 GroupByID:
     "1111": '{"Name":"TestIsAuthenticated/Update_existing_DB_on_success_separator_success","GID":1111,"UGID":"TestIsAuthenticated/Update_existing_DB_on_success_separator_success"}'
-    "2222": '{"Name":"group-success","GID":2222,"UGID":"ugid-success"}'
-    "88888": '{"Name":"group-success","GID":88888,"UGID":"group-success"}'
+    "88888": '{"Name":"group-success","GID":88888,"UGID":"ugid-success"}'
 GroupByName:
     TestIsAuthenticated/Update_existing_DB_on_success_separator_success: '{"Name":"TestIsAuthenticated/Update_existing_DB_on_success_separator_success","GID":1111,"UGID":"TestIsAuthenticated/Update_existing_DB_on_success_separator_success"}'
-    group-success: '{"Name":"group-success","GID":2222,"UGID":"ugid-success"}'
+    group-success: '{"Name":"group-success","GID":88888,"UGID":"ugid-success"}'
 GroupByUGID:
     TestIsAuthenticated/Update_existing_DB_on_success_separator_success: '{"Name":"TestIsAuthenticated/Update_existing_DB_on_success_separator_success","GID":1111,"UGID":"TestIsAuthenticated/Update_existing_DB_on_success_separator_success"}'
-    ugid-success: '{"Name":"group-success","GID":2222,"UGID":"ugid-success"}'
+    ugid-success: '{"Name":"group-success","GID":88888,"UGID":"ugid-success"}'
 GroupToUsers:
     "1111": '{"GID":1111,"UIDs":[1111]}'
-    "2222": '{"GID":2222,"UIDs":[1111]}'
-    "88888": '{"GID":88888,"UIDs":[77777]}'
+    "88888": '{"GID":88888,"UIDs":[77777,1111]}'
 UserByID:
     "1111": '{"Name":"TestIsAuthenticated/Update_existing_DB_on_success_separator_success","UID":1111,"GID":1111,"Gecos":"gecos for success","Dir":"/home/success","Shell":"/bin/sh/success","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"ABCDETIME"}'
     "77777": '{"Name":"otheruser","UID":77777,"GID":88888,"Gecos":"gecos for other user","Dir":"/home/otheruser","Shell":"/bin/sh/otheruser","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"AAAAATIME"}'
@@ -21,7 +19,7 @@ UserByName:
 UserToBroker:
     "77777": '"broker-id"'
 UserToGroups:
-    "1111": '{"UID":1111,"GIDs":[1111,2222]}'
+    "1111": '{"UID":1111,"GIDs":[1111,88888]}'
     "77777": '{"UID":77777,"GIDs":[88888]}'
 UserToLocalGroups:
     "1111": "null"

internal/users/manager.go

@@ -149,7 +149,7 @@ func (m *Manager) UpdateUser(u types.UserInfo) (err error) {
 
 	var authdGroups []cache.GroupDB
 	var localGroups []string
-	for i, g := range u.Groups {
+	for _, g := range u.Groups {
 		if g.Name == "" {
 			return fmt.Errorf("empty group name for user %q", u.Name)
 		}
@@ -161,15 +161,14 @@ func (m *Manager) UpdateUser(u types.UserInfo) (err error) {
 		}
 
 		// Check if the group already exists in the database
-		// We search by UGID because this is a non-local group
-		// and it should have a unique UGID
-		oldGroup, err := m.cache.GroupByUGID(g.UGID)
+		oldGroup, err := m.findGroup(g)
 		if err != nil && !errors.Is(err, cache.NoDataFoundError{}) {
+			// Unexpected error
 			return err
 		}
 		// Keep the old GID if the group already exists in the database, to avoid permission issues
-		if !errors.Is(err, cache.NoDataFoundError{}) {
-			u.Groups[i].GID = &oldGroup.GID
+		if err == nil {
+			g.GID = &oldGroup.GID
 		}
 
 		if g.GID == nil {
@@ -213,6 +212,27 @@ func (m *Manager) UpdateUser(u types.UserInfo) (err error) {
 	return nil
 }
 
+func (m *Manager) findGroup(group types.GroupInfo) (oldGroup cache.GroupDB, err error) {
+	// Search by UGID first to support renaming groups
+	oldGroup, err = m.cache.GroupByUGID(group.UGID)
+	if err == nil {
+		return oldGroup, nil
+	}
+	if !errors.Is(err, cache.NoDataFoundError{}) {
+		// Unexpected error
+		return oldGroup, err
+	}
+
+	// The group was not found by UGID. Search by name, because we didn't store the UGID in 0.3.7 and earlier.
+	log.Debugf(context.Background(), "Group %q not found by UGID %q, trying lookup by name", group.Name, group.UGID)
+	oldGroup, err = m.cache.GroupByName(group.Name)
+	if err == nil && oldGroup.UGID != "" {
+		// There is a group with the same name but a different UGID, which should not happen
+		return oldGroup, fmt.Errorf("group %q already exists with UGID %q (expected %q)", group.Name, oldGroup.UGID, group.UGID)
+	}
+	return oldGroup, err
+}
+
 // checkHomeDirOwnership checks if the home directory of the user is owned by the user and the user's group.
 // If not, it logs a warning.
 func checkHomeDirOwnership(home string, uid, gid uint32) error {

internal/users/manager_test.go

@@ -1,6 +1,7 @@
 package users_test
 
 import (
+	"context"
 	"os"
 	"path/filepath"
 	"testing"
@@ -102,7 +103,12 @@ func TestStop(t *testing.T) {
 
 type userCase struct {
 	types.UserInfo
-	UID uint32
+	UID uint32 // The UID to generate for this user
+}
+
+type groupCase struct {
+	types.GroupInfo
+	GID uint32 // The GID to generate for this group
 }
 
 func TestUpdateUser(t *testing.T) {
@@ -114,24 +120,26 @@ func TestUpdateUser(t *testing.T) {
 		"different-name-same-uid": {UserInfo: types.UserInfo{Name: "newuser1"}, UID: 1111},
 	}
 
-	groupsCases := map[string][]types.GroupInfo{
-		"cloud-group": {{Name: "group1", GID: ptrUint32(11111), UGID: "1"}},
-		"local-group": {{Name: "localgroup1", GID: nil, UGID: ""}},
-		"mixed-groups-cloud-first": {
-			{Name: "group1", GID: ptrUint32(11111), UGID: "1"},
-			{Name: "localgroup1", GID: nil, UGID: ""},
+	groupsCases := map[string][]groupCase{
+		"authd-group": {{GroupInfo: types.GroupInfo{Name: "group1", UGID: "1"}, GID: 11111}},
+		"local-group": {{GroupInfo: types.GroupInfo{Name: "localgroup1", UGID: ""}}},
+		"mixed-groups-authd-first": {
+			{GroupInfo: types.GroupInfo{Name: "group1", UGID: "1"}, GID: 11111},
+			{GroupInfo: types.GroupInfo{Name: "localgroup1", UGID: ""}},
 		},
 		"mixed-groups-local-first": {
-			{Name: "localgroup1", GID: nil, UGID: ""},
-			{Name: "group1", GID: ptrUint32(11111), UGID: "1"},
+			{GroupInfo: types.GroupInfo{Name: "localgroup1", UGID: ""}},
+			{GroupInfo: types.GroupInfo{Name: "group1", UGID: "1"}, GID: 11111},
 		},
 		"mixed-groups-gpasswd-fail": {
-			{Name: "group1", GID: ptrUint32(11111), UGID: "1"},
-			{Name: "gpasswdfail", GID: nil, UGID: ""},
+			{GroupInfo: types.GroupInfo{Name: "group1", UGID: "1"}, GID: 11111},
+			{GroupInfo: types.GroupInfo{Name: "gpasswdfail", UGID: ""}},
 		},
-		"nameless-group":          {{Name: "", GID: ptrUint32(11111), UGID: "1"}},
-		"different-name-same-gid": {{Name: "newgroup1", GID: ptrUint32(11111), UGID: "1"}},
+		"nameless-group":          {{GroupInfo: types.GroupInfo{Name: "", UGID: "1"}, GID: 11111}},
+		"different-name-same-gid": {{GroupInfo: types.GroupInfo{Name: "newgroup1", UGID: "1"}, GID: 11111}},
 		"no-groups":               {},
+		// This group case has no GID to generate, because it's expected that the GID of the old group is re-used
+		"different-name-same-ugid": {{GroupInfo: types.GroupInfo{Name: "renamed-group", UGID: "12345678"}}},
 	}
 
 	tests := map[string]struct {
@@ -145,19 +153,22 @@ func TestUpdateUser(t *testing.T) {
 		noOutput    bool
 		wantSameUID bool
 	}{
-		"Successfully update user":                       {groupsCase: "cloud-group"},
-		"Successfully update user updating local groups": {groupsCase: "mixed-groups-cloud-first", localGroupsFile: "users_in_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":                                          {groupsCase: "authd-group"},
+		"Successfully update user updating local groups":                    {groupsCase: "mixed-groups-authd-first", localGroupsFile: "users_in_groups.group"},
+		"UID does not change if user already exists":                        {userCase: "same-name-different-uid", dbFile: "one_user_and_group", wantSameUID: true},
+		"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"},
 
-		"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},
-		"Error if group has conflicting gid": {groupsCase: "different-name-same-gid", dbFile: "one_user_and_group", wantErr: true, noOutput: true},
+		"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},
+		"Error if group has conflicting gid":                      {groupsCase: "different-name-same-gid", dbFile: "one_user_and_group", wantErr: true, noOutput: true},
+		"Error if group with same name but different UGID exists": {groupsCase: "authd-group", dbFile: "one_user_and_group", wantErr: true, noOutput: true},
 
 		"Error when updating local groups remove user from db":                              {groupsCase: "mixed-groups-gpasswd-fail", localGroupsFile: "gpasswdfail_in_deleted_group.group", wantErr: true},
 		"Error when updating local groups remove user from db without touching other users": {dbFile: "multiple_users_and_groups", groupsCase: "mixed-groups-gpasswd-fail", localGroupsFile: "gpasswdfail_in_deleted_group.group", wantErr: true},
 		"Error when updating local groups remove user from db even if already existed":      {userCase: "user2", dbFile: "multiple_users_and_groups", groupsCase: "mixed-groups-gpasswd-fail", localGroupsFile: "gpasswdfail_in_deleted_group.group", wantErr: true},
 
-		"Error on invalid entry": {groupsCase: "cloud-group", dbFile: "invalid_entry_in_userToGroups", localGroupsFile: "users_in_groups.group", wantErr: true, noOutput: true},
+		"Error on invalid entry": {groupsCase: "authd-group", dbFile: "invalid_entry_in_userToGroups", localGroupsFile: "users_in_groups.group", wantErr: true, noOutput: true},
 	}
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
@@ -178,19 +189,23 @@ func TestUpdateUser(t *testing.T) {
 			user.Dir = "/home/" + user.Name
 			user.Shell = "/bin/bash"
 			user.Gecos = "gecos for " + user.Name
-			user.Groups = groupsCases[tc.groupsCase]
+			for _, g := range groupsCases[tc.groupsCase] {
+				user.Groups = append(user.Groups, g.GroupInfo)
+			}
 
 			cacheDir := t.TempDir()
 			if tc.dbFile != "" {
 				cache.Z_ForTests_CreateDBFromYAML(t, filepath.Join("testdata", "db", tc.dbFile+".db.yaml"), cacheDir)
 			}
 
-			gids := []uint32{user.UID}
-			for _, group := range user.Groups {
-				if group.GID != nil {
-					gids = append(gids, *group.GID)
+			// One GID is generated for the user private group
+			gids := []uint32{11110}
+			for _, group := range groupsCases[tc.groupsCase] {
+				if group.GID != 0 {
+					gids = append(gids, group.GID)
 				}
 			}
+
 			managerOpts := []users.Option{
 				users.WithIDGenerator(&idgenerator.IDGeneratorMock{
 					UIDsToGenerate: []uint32{user.UID},
@@ -207,6 +222,7 @@ func TestUpdateUser(t *testing.T) {
 			}
 
 			err := m.UpdateUser(user.UserInfo)
+			log.Debugf(context.Background(), "UpdateUser error: %v", err)
 
 			requireErrorAssertions(t, err, nil, tc.wantErr)
 			if tc.wantErr && tc.noOutput {
@@ -594,10 +610,6 @@ func newManagerForTests(t *testing.T, cacheDir string, opts ...users.Option) *us
 	return m
 }
 
-func ptrUint32(v uint32) *uint32 {
-	return &v
-}
-
 func TestMain(m *testing.M) {
 	log.SetLevel(log.DebugLevel)
 	m.Run()

internal/users/testdata/db/group-with-empty-UGID.db.yaml

@@ -0,0 +1,16 @@
+GroupByID:
+  "11111": '{"Name":"group1","GID":11111}'
+GroupByName:
+  group1: '{"Name":"group1","GID":11111}'
+GroupByUGID:
+  "12345678": '{"Name":"group1","GID":11111}'
+GroupToUsers:
+  "11111": '{"GID":11111,"UIDs":[1111]}'
+UserByID:
+  "1111": '{"Name":"user1","UID":1111,"GID":11111,"Gecos":"User1 gecos\nOn multiple lines","Dir":"/home/user1","Shell":"/bin/bash","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"AAAAATIME"}'
+UserByName:
+  user1: '{"Name":"user1","UID":1111,"GID":11111,"Gecos":"User1 gecos\nOn multiple lines","Dir":"/home/user1","Shell":"/bin/bash","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"AAAAATIME"}'
+UserToGroups:
+  "1111": '{"UID":1111,"GIDs":[11111]}'
+UserToBroker:
+  "1111": '"broker-id"'

internal/users/testdata/golden/TestUpdateUser/GID_does_not_change_if_group_with_same_UGID_exists

@@ -0,0 +1,23 @@
+|
+    GroupByID:
+        "11110": '{"Name":"user1","GID":11110,"UGID":"user1"}'
+        "11111": '{"Name":"renamed-group","GID":11111,"UGID":"12345678"}'
+    GroupByName:
+        renamed-group: '{"Name":"renamed-group","GID":11111,"UGID":"12345678"}'
+        user1: '{"Name":"user1","GID":11110,"UGID":"user1"}'
+    GroupByUGID:
+        "12345678": '{"Name":"renamed-group","GID":11111,"UGID":"12345678"}'
+        user1: '{"Name":"user1","GID":11110,"UGID":"user1"}'
+    GroupToUsers:
+        "11110": '{"GID":11110,"UIDs":[1111]}'
+        "11111": '{"GID":11111,"UIDs":[1111]}'
+    UserByID:
+        "1111": '{"Name":"user1","UID":1111,"GID":11110,"Gecos":"gecos for user1","Dir":"/home/user1","Shell":"/bin/bash","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"ABCDETIME"}'
+    UserByName:
+        user1: '{"Name":"user1","UID":1111,"GID":11110,"Gecos":"gecos for user1","Dir":"/home/user1","Shell":"/bin/bash","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"ABCDETIME"}'
+    UserToBroker:
+        "1111": '"broker-id"'
+    UserToGroups:
+        "1111": '{"UID":1111,"GIDs":[11110,11111]}'
+    UserToLocalGroups:
+        "1111": "null"

internal/users/testdata/golden/TestUpdateUser/GID_does_not_change_if_group_with_same_name_and_empty_UGID_exists

@@ -0,0 +1,24 @@
+|
+    GroupByID:
+        "11110": '{"Name":"user1","GID":11110,"UGID":"user1"}'
+        "11111": '{"Name":"group1","GID":11111,"UGID":"1"}'
+    GroupByName:
+        group1: '{"Name":"group1","GID":11111,"UGID":"1"}'
+        user1: '{"Name":"user1","GID":11110,"UGID":"user1"}'
+    GroupByUGID:
+        "1": '{"Name":"group1","GID":11111,"UGID":"1"}'
+        "12345678": '{"Name":"group1","GID":11111}'
+        user1: '{"Name":"user1","GID":11110,"UGID":"user1"}'
+    GroupToUsers:
+        "11110": '{"GID":11110,"UIDs":[1111]}'
+        "11111": '{"GID":11111,"UIDs":[1111]}'
+    UserByID:
+        "1111": '{"Name":"user1","UID":1111,"GID":11110,"Gecos":"gecos for user1","Dir":"/home/user1","Shell":"/bin/bash","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"ABCDETIME"}'
+    UserByName:
+        user1: '{"Name":"user1","UID":1111,"GID":11110,"Gecos":"gecos for user1","Dir":"/home/user1","Shell":"/bin/bash","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"ABCDETIME"}'
+    UserToBroker:
+        "1111": '"broker-id"'
+    UserToGroups:
+        "1111": '{"UID":1111,"GIDs":[11110,11111]}'
+    UserToLocalGroups:
+        "1111": "null"

internal/users/testdata/golden/TestUpdateUser/Successfully_update_user

@@ -1,22 +1,22 @@
 |
     GroupByID:
-        "1111": '{"Name":"user1","GID":1111,"UGID":"user1"}'
+        "11110": '{"Name":"user1","GID":11110,"UGID":"user1"}'
         "11111": '{"Name":"group1","GID":11111,"UGID":"1"}'
     GroupByName:
         group1: '{"Name":"group1","GID":11111,"UGID":"1"}'
-        user1: '{"Name":"user1","GID":1111,"UGID":"user1"}'
+        user1: '{"Name":"user1","GID":11110,"UGID":"user1"}'
     GroupByUGID:
         "1": '{"Name":"group1","GID":11111,"UGID":"1"}'
-        user1: '{"Name":"user1","GID":1111,"UGID":"user1"}'
+        user1: '{"Name":"user1","GID":11110,"UGID":"user1"}'
     GroupToUsers:
-        "1111": '{"GID":1111,"UIDs":[1111]}'
+        "11110": '{"GID":11110,"UIDs":[1111]}'
         "11111": '{"GID":11111,"UIDs":[1111]}'
     UserByID:
-        "1111": '{"Name":"user1","UID":1111,"GID":1111,"Gecos":"gecos for user1","Dir":"/home/user1","Shell":"/bin/bash","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"ABCDETIME"}'
+        "1111": '{"Name":"user1","UID":1111,"GID":11110,"Gecos":"gecos for user1","Dir":"/home/user1","Shell":"/bin/bash","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"ABCDETIME"}'
     UserByName:
-        user1: '{"Name":"user1","UID":1111,"GID":1111,"Gecos":"gecos for user1","Dir":"/home/user1","Shell":"/bin/bash","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"ABCDETIME"}'
+        user1: '{"Name":"user1","UID":1111,"GID":11110,"Gecos":"gecos for user1","Dir":"/home/user1","Shell":"/bin/bash","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"ABCDETIME"}'
     UserToBroker: {}
     UserToGroups:
-        "1111": '{"UID":1111,"GIDs":[1111,11111]}'
+        "1111": '{"UID":1111,"GIDs":[11110,11111]}'
     UserToLocalGroups:
         "1111": "null"