Fix user removed from local groups

Only remove the user from local groups which the user was added to by
authd. Previously, authd removed the user from all local groups which
are not configured in Microsoft Entra, so if the user was added to any
local groups manually, it was removed from those groups again during the
next login.

Closes #576

internal/users/cache/db.go

@@ -19,21 +19,22 @@ var (
 )
 
 const (
-	userByNameBucketName   = "UserByName"
-	userByIDBucketName     = "UserByID"
-	groupByNameBucketName  = "GroupByName"
-	groupByIDBucketName    = "GroupByID"
-	userToGroupsBucketName = "UserToGroups"
-	groupToUsersBucketName = "GroupToUsers"
-	userToBrokerBucketName = "UserToBroker"
+	userByNameBucketName        = "UserByName"
+	userByIDBucketName          = "UserByID"
+	groupByNameBucketName       = "GroupByName"
+	groupByIDBucketName         = "GroupByID"
+	userToGroupsBucketName      = "UserToGroups"
+	groupToUsersBucketName      = "GroupToUsers"
+	userToBrokerBucketName      = "UserToBroker"
+	userToLocalGroupsBucketName = "UserToLocalGroups"
 )
 
 var (
 	allBuckets = [][]byte{
 		[]byte(userByNameBucketName), []byte(userByIDBucketName),
 		[]byte(groupByNameBucketName), []byte(groupByIDBucketName),
 		[]byte(userToGroupsBucketName), []byte(groupToUsersBucketName),
-		[]byte(userToBrokerBucketName),
+		[]byte(userToBrokerBucketName), []byte(userToLocalGroupsBucketName),
 	}
 )
 

internal/users/cache/db_test.go

@@ -156,9 +156,10 @@ func TestUpdateUserEntry(t *testing.T) {
 	}
 
 	tests := map[string]struct {
-		userCase   string
-		groupCases []string
-		dbFile     string
+		userCase    string
+		groupCases  []string
+		localGroups []string
+		dbFile      string
 
 		wantErr bool
 	}{
@@ -219,7 +220,7 @@ func TestUpdateUserEntry(t *testing.T) {
 			}
 			user.GID = groups[0].GID
 
-			err := c.UpdateUserEntry(user, groups)
+			err := c.UpdateUserEntry(user, groups, tc.localGroups)
 			if tc.wantErr {
 				require.Error(t, err, "UpdateFromUserInfo should return an error but didn't")
 				return

internal/users/cache/delete.go

@@ -34,6 +34,7 @@ func (c *Cache) DeleteUser(uid uint32) error {
 // deleteUserFromGroup removes the uid from the group.
 // If the group is empty after the uid gets removed, the group is deleted from the database.
 func deleteUserFromGroup(buckets map[string]bucketWithName, uid, gid uint32) error {
+	log.Debugf(context.TODO(), "Removing user %d from group %d", uid, gid)
 	groupToUsers, err := getFromBucket[groupToUsersDB](buckets[groupToUsersBucketName], gid)
 	if err != nil && !errors.Is(err, NoDataFoundError{}) {
 		return err

internal/users/cache/getgroups.go

@@ -31,6 +31,74 @@ func (c *Cache) GroupByName(name string) (GroupDB, error) {
 	return getGroup(c, groupByNameBucketName, name)
 }
 
+// UserGroups returns all groups for a given user or an error if the database is corrupted or no entry was found.
+func (c *Cache) UserGroups(uid uint32) ([]GroupDB, error) {
+	c.mu.RLock()
+	defer c.mu.RUnlock()
+	var groups []GroupDB
+	err := c.db.View(func(tx *bbolt.Tx) error {
+		buckets, err := getAllBuckets(tx)
+		if err != nil {
+			return err
+		}
+
+		// Get group ids for the user.
+		groupsForUser, err := getFromBucket[userToGroupsDB](buckets[userToGroupsBucketName], uid)
+		if err != nil {
+			return err
+		}
+
+		for _, gid := range groupsForUser.GIDs {
+			// we should always get an entry
+			g, err := getFromBucket[groupDB](buckets[groupByIDBucketName], gid)
+			if err != nil {
+				return err
+			}
+
+			// Get user names in the group.
+			users, err := getUsersInGroup(buckets, gid)
+			if err != nil {
+				return err
+			}
+
+			groups = append(groups, NewGroupDB(g.Name, g.GID, users))
+		}
+		return nil
+	})
+
+	if err != nil {
+		return nil, err
+	}
+
+	return groups, nil
+}
+
+// UserLocalGroups returns all local groups for a given user or an error if the database is corrupted or no entry was found.
+func (c *Cache) UserLocalGroups(uid uint32) ([]string, error) {
+	c.mu.RLock()
+	defer c.mu.RUnlock()
+	var localGroups []string
+	err := c.db.View(func(tx *bbolt.Tx) error {
+		buckets, err := getAllBuckets(tx)
+		if err != nil {
+			return err
+		}
+
+		localGroups, err = getFromBucket[[]string](buckets[userToLocalGroupsBucketName], uid)
+		if err != nil {
+			return err
+		}
+
+		return nil
+	})
+
+	if err != nil {
+		return nil, err
+	}
+
+	return localGroups, nil
+}
+
 // AllGroups returns all groups or an error if the database is corrupted.
 func (c *Cache) AllGroups() (all []GroupDB, err error) {
 	c.mu.RLock()

internal/users/cache/testdata/TestDeleteUser/golden/delete_existing_user

@@ -5,3 +5,4 @@ UserByID: {}
 UserByName: {}
 UserToBroker: {}
 UserToGroups: {}
+UserToLocalGroups: {}

internal/users/cache/testdata/TestDeleteUser/golden/delete_existing_user_keeping_other_users_intact

@@ -28,3 +28,4 @@ UserToGroups:
     "2222": '{"UID":2222,"GIDs":[22222,99999]}'
     "3333": '{"UID":3333,"GIDs":[33333,99999]}'
     "4444": '{"UID":4444,"GIDs":[44444]}'
+UserToLocalGroups: {}

internal/users/cache/testdata/TestNew/golden/new_recreates_any_missing_buckets_and_delete_unknowns

@@ -7,3 +7,4 @@ 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"}'
 UserToBroker: {}
 UserToGroups: {}
+UserToLocalGroups: {}

internal/users/cache/testdata/TestNew/golden/new_removes_orphaned_user_records_from_userbyid_bucket

@@ -5,3 +5,4 @@ UserByID: {}
 UserByName: {}
 UserToBroker: {}
 UserToGroups: {}
+UserToLocalGroups: {}

internal/users/cache/testdata/TestNew/golden/new_with_already_existing_database

@@ -35,3 +35,4 @@ UserToGroups:
     "2222": '{"UID":2222,"GIDs":[22222,99999]}'
     "3333": '{"UID":3333,"GIDs":[33333,99999]}'
     "4444": '{"UID":4444,"GIDs":[44444]}'
+UserToLocalGroups: {}

internal/users/cache/testdata/TestNew/golden/new_without_any_initialized_database

@@ -5,3 +5,4 @@ UserByID: {}
 UserByName: {}
 UserToBroker: {}
 UserToGroups: {}
+UserToLocalGroups: {}

internal/users/cache/testdata/TestUpdateUserEntry/golden/add_user_to_group_from_another_user

@@ -35,3 +35,5 @@ UserToGroups:
     "2222": '{"UID":2222,"GIDs":[22222,99999]}'
     "3333": '{"UID":3333,"GIDs":[33333,99999]}'
     "4444": '{"UID":4444,"GIDs":[44444]}'
+UserToLocalGroups:
+    "1111": "null"

internal/users/cache/testdata/TestUpdateUserEntry/golden/insert_new_user

@@ -11,3 +11,5 @@ UserByName:
 UserToBroker: {}
 UserToGroups:
     "1111": '{"UID":1111,"GIDs":[11111]}'
+UserToLocalGroups:
+    "1111": "null"

internal/users/cache/testdata/TestUpdateUserEntry/golden/insert_new_user_without_optional_gecos_field

@@ -11,3 +11,5 @@ UserByName:
 UserToBroker: {}
 UserToGroups:
     "1111": '{"UID":1111,"GIDs":[11111]}'
+UserToLocalGroups:
+    "1111": "null"

internal/users/cache/testdata/TestUpdateUserEntry/golden/invalid_value_entries_in_other_user_and_groups_don't_impact_current_request

@@ -29,3 +29,5 @@ UserToGroups:
     "1111": '{"UID":1111,"GIDs":[11111]}'
     "2222": '"not-a-valid-json"'
     "3333": '"not-a-valid-json"'
+UserToLocalGroups:
+    "1111": "null"

internal/users/cache/testdata/TestUpdateUserEntry/golden/invalid_value_entry_in_groupbyname_recreates_entries

@@ -12,3 +12,5 @@ UserToBroker:
     "1111": '"broker-id"'
 UserToGroups:
     "1111": '{"UID":1111,"GIDs":[11111]}'
+UserToLocalGroups:
+    "1111": "null"

internal/users/cache/testdata/TestUpdateUserEntry/golden/invalid_value_entry_in_userbyname_recreates_entries

@@ -12,3 +12,5 @@ UserToBroker:
     "1111": '"broker-id"'
 UserToGroups:
     "1111": '{"UID":1111,"GIDs":[11111]}'
+UserToLocalGroups:
+    "1111": "null"

internal/users/cache/testdata/TestUpdateUserEntry/golden/remove_group_from_user

@@ -12,3 +12,5 @@ UserToBroker:
     "1111": '"broker-id"'
 UserToGroups:
     "1111": '{"UID":1111,"GIDs":[22222]}'
+UserToLocalGroups:
+    "1111": "null"

internal/users/cache/testdata/TestUpdateUserEntry/golden/remove_user_from_a_group_still_part_from_another_user

@@ -35,3 +35,5 @@ UserToGroups:
     "2222": '{"UID":2222,"GIDs":[22222,99999]}'
     "3333": '{"UID":3333,"GIDs":[33333]}'
     "4444": '{"UID":4444,"GIDs":[44444]}'
+UserToLocalGroups:
+    "3333": "null"

internal/users/cache/testdata/TestUpdateUserEntry/golden/update_last_login_time_for_user

@@ -12,3 +12,5 @@ UserToBroker:
     "1111": '"broker-id"'
 UserToGroups:
     "1111": '{"UID":1111,"GIDs":[11111]}'
+UserToLocalGroups:
+    "1111": "null"

internal/users/cache/testdata/TestUpdateUserEntry/golden/update_only_user_even_if_we_have_multiple_of_them

@@ -35,3 +35,5 @@ UserToGroups:
     "2222": '{"UID":2222,"GIDs":[22222,99999]}'
     "3333": '{"UID":3333,"GIDs":[33333,99999]}'
     "4444": '{"UID":4444,"GIDs":[44444]}'
+UserToLocalGroups:
+    "1111": "null"

internal/users/cache/testdata/TestUpdateUserEntry/golden/update_user_by_adding_a_new_default_group

@@ -15,3 +15,5 @@ UserToBroker:
     "1111": '"broker-id"'
 UserToGroups:
     "1111": '{"UID":1111,"GIDs":[22222,11111]}'
+UserToLocalGroups:
+    "1111": "null"

internal/users/cache/testdata/TestUpdateUserEntry/golden/update_user_by_adding_a_new_group

@@ -15,3 +15,5 @@ UserToBroker:
     "1111": '"broker-id"'
 UserToGroups:
     "1111": '{"UID":1111,"GIDs":[11111,22222]}'
+UserToLocalGroups:
+    "1111": "null"

internal/users/cache/testdata/TestUpdateUserEntry/golden/update_user_by_changing_attributes

@@ -12,3 +12,5 @@ UserToBroker:
     "1111": '"broker-id"'
 UserToGroups:
     "1111": '{"UID":1111,"GIDs":[11111]}'
+UserToLocalGroups:
+    "1111": "null"

internal/users/cache/testdata/TestUpdateUserEntry/golden/update_user_by_removing_optional_gecos_field_if_not_set

@@ -12,3 +12,5 @@ UserToBroker:
     "1111": '"broker-id"'
 UserToGroups:
     "1111": '{"UID":1111,"GIDs":[11111]}'
+UserToLocalGroups:
+    "1111": "null"

internal/users/cache/testdata/TestUpdateUserEntry/golden/update_user_does_not_change_homedir_if_it_exists

@@ -12,3 +12,5 @@ UserToBroker:
     "1111": '"broker-id"'
 UserToGroups:
     "1111": '{"UID":1111,"GIDs":[11111]}'
+UserToLocalGroups:
+    "1111": "null"

internal/users/cache/update.go

@@ -14,7 +14,7 @@ import (
 )
 
 // UpdateUserEntry inserts or updates user and group buckets from the user information.
-func (c *Cache) UpdateUserEntry(usr UserDB, groupContents []GroupDB) error {
+func (c *Cache) UpdateUserEntry(usr UserDB, authdGroups []GroupDB, localGroups []string) error {
 	c.mu.RLock()
 	defer c.mu.RUnlock()
 
@@ -41,15 +41,18 @@ func (c *Cache) UpdateUserEntry(usr UserDB, groupContents []GroupDB) error {
 		}
 
 		/* 2. Handle groups update */
-		if err := updateGroups(buckets, groupContents); err != nil {
+		if err := updateGroups(buckets, authdGroups); err != nil {
 			return err
 		}
 
 		/* 3. Users and groups mapping buckets */
-		if err := updateUsersAndGroups(buckets, userDB.UID, groupContents, previousGroupsForCurrentUser.GIDs); err != nil {
+		if err := updateUsersAndGroups(buckets, userDB.UID, authdGroups, previousGroupsForCurrentUser.GIDs); err != nil {
 			return err
 		}
 
+		/* 4. Update user to local groups bucket */
+		updateBucket(buckets[userToLocalGroupsBucketName], userDB.UID, localGroups)
+
 		return nil
 	})
 

internal/users/localgroups/localgroups.go

@@ -13,6 +13,7 @@ import (
 	"sync"
 
 	"github.com/ubuntu/authd/internal/log"
+	"github.com/ubuntu/authd/internal/sliceutils"
 	"github.com/ubuntu/decorate"
 )
 
@@ -34,22 +35,26 @@ type Option func(*options)
 var localGroupsMu = &sync.RWMutex{}
 
 // Update synchronizes for the given user the local group list with the current group list from UserInfo.
-func Update(username string, groups []string, args ...Option) (err error) {
-	defer decorate.OnError(&err, "could not update local groups for user %q", username)
+func Update(username string, newGroups []string, oldGroups []string, args ...Option) (err error) {
+	log.Debugf(context.TODO(), "Updating local groups for user %q, new groups: %v, old groups: %v", username, newGroups, oldGroups)
+	defer decorate.OnError(&err, "could not update local newGroups for user %q", username)
 
 	opts := defaultOptions
 	for _, arg := range args {
 		arg(&opts)
 	}
 
-	currentLocalGroups, err := existingLocalGroups(username, opts.groupPath)
+	currentGroups, err := existingLocalGroups(username, opts.groupPath)
 	if err != nil {
 		return err
 	}
 
 	localGroupsMu.Lock()
 	defer localGroupsMu.Unlock()
-	groupsToAdd, groupsToRemove := computeGroupOperation(groups, currentLocalGroups)
+	groupsToAdd := sliceutils.Difference(newGroups, currentGroups)
+	log.Debugf(context.TODO(), "Adding to groups: %v", groupsToAdd)
+	groupsToRemove := sliceutils.Difference(oldGroups, newGroups)
+	log.Debugf(context.TODO(), "Removing from groups: %v", groupsToRemove)
 
 	for _, g := range groupsToRemove {
 		args := opts.gpasswdCmd[1:]
@@ -110,38 +115,6 @@ func existingLocalGroups(user, groupPath string) (groups []string, err error) {
 	return groups, nil
 }
 
-// computeGroupOperation returns which local groups to add and which to remove comparing with the existing group state.
-// Only local groups (with no GID) are considered from GroupInfo.
-func computeGroupOperation(newGroupsInfo []string, currentLocalGroups []string) (groupsToAdd []string, groupsToRemove []string) {
-	newGroups := make(map[string]struct{})
-	for _, grp := range newGroupsInfo {
-		newGroups[grp] = struct{}{}
-	}
-
-	currGroups := make(map[string]struct{})
-	for _, g := range currentLocalGroups {
-		currGroups[g] = struct{}{}
-	}
-
-	for g := range newGroups {
-		// already in current group file
-		if _, ok := currGroups[g]; ok {
-			continue
-		}
-		groupsToAdd = append(groupsToAdd, g)
-	}
-
-	for g := range currGroups {
-		// was in that group but not anymore
-		if _, ok := newGroups[g]; ok {
-			continue
-		}
-		groupsToRemove = append(groupsToRemove, g)
-	}
-
-	return groupsToAdd, groupsToRemove
-}
-
 // CleanUser removes the user from all local groups.
 func CleanUser(user string, args ...Option) (err error) {
 	defer decorate.OnError(&err, "could not clean user %q from local groups", user)