From 54e1f1a159f98d560644ff0a2e28b3e9d7f7e754 Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Mon, 27 Jan 2025 15:39:06 +0100 Subject: [PATCH] Don't delete empty groups from database When a user logs in, we fetch the groups from the provider and update our database accordingly. When this causes a user to be removed from an authd group and that group doesn't have any other users in it, it's currently removed from our database. That's an issue for two reasons: 1. The next time a user who is a member of that group logs in, a new random GID is generated for that group, which means that any existing files owned by the group won't be accessible to members of the group anymore. 2. Whenever a another group is added, the random GID generated for that group can by chance be the same as the GID of the deleted group, allowing members access to existing files owned by the deleted group. With this commit, we don't delete empty groups but just keep them in the database, so that its GID is still reserved and reused the next time a user who is a member of that group logs in. --- internal/users/cache/delete.go | 28 ++----------------- .../TestDeleteUser/Delete_existing_user | 12 +++++--- ...e_existing_user_keeping_other_users_intact | 4 +++ .../Remove_group_from_user | 4 +++ ...UID_does_not_change_if_user_already_exists | 4 +++ 5 files changed, 22 insertions(+), 30 deletions(-) diff --git a/internal/users/cache/delete.go b/internal/users/cache/delete.go index cb672c967..e868521c6 100644 --- a/internal/users/cache/delete.go +++ b/internal/users/cache/delete.go @@ -41,34 +41,10 @@ func deleteUserFromGroup(buckets map[string]bucketWithName, uid, gid uint32) err } groupToUsers.UIDs = slices.DeleteFunc(groupToUsers.UIDs, func(id uint32) bool { return id == uid }) - if len(groupToUsers.UIDs) > 0 { - // Update the group entry with the new list of UIDs - updateBucket(buckets[groupToUsersBucketName], gid, groupToUsers) - return nil - } - // We now need to delete this group with no remaining user. - // We need the group.Name to delete from groupByName bucket. - group, err := getFromBucket[GroupDB](buckets[groupByIDBucketName], gid) - if err != nil { - return err - } + // Update the group entry with the new list of UIDs + updateBucket(buckets[groupToUsersBucketName], gid, groupToUsers) - gidKey := []byte(strconv.FormatUint(uint64(gid), 10)) - // Delete group - // Delete calls fail if the transaction is read only, so we should panic if this function is called in that context. - if err = buckets[groupToUsersBucketName].Delete(gidKey); err != nil { - panic(fmt.Sprintf("programming error: delete is not allowed in a RO transaction: %v", err)) - } - if err = buckets[groupByIDBucketName].Delete(gidKey); err != nil { - panic(fmt.Sprintf("programming error: delete is not allowed in a RO transaction: %v", err)) - } - if err = buckets[groupByNameBucketName].Delete([]byte(group.Name)); err != nil { - panic(fmt.Sprintf("programming error: delete is not allowed in a RO transaction: %v", err)) - } - if err = buckets[groupByUGIDBucketName].Delete([]byte(group.UGID)); err != nil { - panic(fmt.Sprintf("programming error: delete is not allowed in a RO transaction: %v", err)) - } return nil } diff --git a/internal/users/cache/testdata/golden/TestDeleteUser/Delete_existing_user b/internal/users/cache/testdata/golden/TestDeleteUser/Delete_existing_user index 79f78e127..4d53cbd4c 100644 --- a/internal/users/cache/testdata/golden/TestDeleteUser/Delete_existing_user +++ b/internal/users/cache/testdata/golden/TestDeleteUser/Delete_existing_user @@ -1,7 +1,11 @@ -GroupByID: {} -GroupByName: {} -GroupByUGID: {} -GroupToUsers: {} +GroupByID: + "11111": '{"Name":"group1","GID":11111,"UGID":"12345678"}' +GroupByName: + group1: '{"Name":"group1","GID":11111,"UGID":"12345678"}' +GroupByUGID: + "12345678": '{"Name":"group1","GID":11111,"UGID":"12345678"}' +GroupToUsers: + "11111": '{"GID":11111,"UIDs":[]}' UserByID: {} UserByName: {} UserToBroker: {} diff --git a/internal/users/cache/testdata/golden/TestDeleteUser/Delete_existing_user_keeping_other_users_intact b/internal/users/cache/testdata/golden/TestDeleteUser/Delete_existing_user_keeping_other_users_intact index d7195e86c..07d4ab7e0 100644 --- a/internal/users/cache/testdata/golden/TestDeleteUser/Delete_existing_user_keeping_other_users_intact +++ b/internal/users/cache/testdata/golden/TestDeleteUser/Delete_existing_user_keeping_other_users_intact @@ -1,19 +1,23 @@ GroupByID: + "11111": '{"Name":"group1","GID":11111,"UGID":"12345678"}' "22222": '{"Name":"group2","GID":22222,"UGID":"56781234"}' "33333": '{"Name":"group3","GID":33333,"UGID":"34567812"}' "44444": '{"Name":"group4","GID":44444,"UGID":"45678123"}' "99999": '{"Name":"commongroup","GID":99999,"UGID":"87654321"}' GroupByName: commongroup: '{"Name":"commongroup","GID":99999,"UGID":"87654321"}' + group1: '{"Name":"group1","GID":11111,"UGID":"12345678"}' group2: '{"Name":"group2","GID":22222,"UGID":"56781234"}' group3: '{"Name":"group3","GID":33333,"UGID":"34567812"}' group4: '{"Name":"group4","GID":44444,"UGID":"45678123"}' GroupByUGID: + "12345678": '{"Name":"group1","GID":11111,"UGID":"12345678"}' "34567812": '{"Name":"group3","GID":33333,"UGID":"34567812"}' "45678123": '{"Name":"group4","GID":44444,"UGID":"45678123"}' "56781234": '{"Name":"group2","GID":22222,"UGID":"56781234"}' "87654321": '{"Name":"commongroup","GID":99999,"UGID":"87654321"}' GroupToUsers: + "11111": '{"GID":11111,"UIDs":[]}' "22222": '{"GID":22222,"UIDs":[2222]}' "33333": '{"GID":33333,"UIDs":[3333]}' "44444": '{"GID":33333,"UIDs":[4444]}' diff --git a/internal/users/cache/testdata/golden/TestUpdateUserEntry/Remove_group_from_user b/internal/users/cache/testdata/golden/TestUpdateUserEntry/Remove_group_from_user index c708d8d1f..b48932105 100644 --- a/internal/users/cache/testdata/golden/TestUpdateUserEntry/Remove_group_from_user +++ b/internal/users/cache/testdata/golden/TestUpdateUserEntry/Remove_group_from_user @@ -1,10 +1,14 @@ GroupByID: + "11111": '{"Name":"group1","GID":11111,"UGID":"12345678"}' "22222": '{"Name":"group2","GID":22222,"UGID":"56781234"}' GroupByName: + group1: '{"Name":"group1","GID":11111,"UGID":"12345678"}' group2: '{"Name":"group2","GID":22222,"UGID":"56781234"}' GroupByUGID: + "12345678": '{"Name":"group1","GID":11111,"UGID":"12345678"}' "56781234": '{"Name":"group2","GID":22222,"UGID":"56781234"}' GroupToUsers: + "11111": '{"GID":11111,"UIDs":[]}' "22222": '{"GID":22222,"UIDs":[1111]}' UserByID: "1111": '{"Name":"user1","UID":1111,"GID":22222,"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":"ABCDETIME"}' diff --git a/internal/users/testdata/golden/TestUpdateUser/UID_does_not_change_if_user_already_exists b/internal/users/testdata/golden/TestUpdateUser/UID_does_not_change_if_user_already_exists index aca312d47..5e79e3ce1 100644 --- a/internal/users/testdata/golden/TestUpdateUser/UID_does_not_change_if_user_already_exists +++ b/internal/users/testdata/golden/TestUpdateUser/UID_does_not_change_if_user_already_exists @@ -1,12 +1,16 @@ | GroupByID: "11110": '{"Name":"user1","GID":11110,"UGID":"user1"}' + "11111": '{"Name":"group1","GID":11111,"UGID":"12345678"}' GroupByName: + group1: '{"Name":"group1","GID":11111,"UGID":"12345678"}' user1: '{"Name":"user1","GID":11110,"UGID":"user1"}' GroupByUGID: + "12345678": '{"Name":"group1","GID":11111,"UGID":"12345678"}' user1: '{"Name":"user1","GID":11110,"UGID":"user1"}' GroupToUsers: "11110": '{"GID":11110,"UIDs":[1111]}' + "11111": '{"GID":11111,"UIDs":[]}' 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: