fixup! Generate unique UIDs and GIDs

internal/users/manager.go

@@ -134,17 +134,12 @@ func (m *Manager) UpdateUser(u types.UserInfo) (err error) {
 		// created by some other NSS source, this also registers a temporary user in our NSS handler. We remove that
 		// temporary user before returning from this function, at which point the user is added to the database (so we
 		// don't need the temporary user anymore to keep the UID unique).
-		var cleanup func() error
+		var cleanup func()
 		uid, cleanup, err = m.temporaryRecords.RegisterUser(u.Name)
 		if err != nil {
 			return fmt.Errorf("could not register user %q: %w", u.Name, err)
 		}
-
-		defer func() {
-			if cleanupErr := cleanup(); cleanupErr != nil {
-				err = errors.Join(err, cleanupErr)
-			}
-		}()
+		defer cleanup()
 	} else {
 		uid = oldUser.UID
 	}
@@ -187,12 +182,7 @@ func (m *Manager) UpdateUser(u types.UserInfo) (err error) {
 				return fmt.Errorf("could not generate GID for group %q: %v", g.Name, err)
 			}
 
-			defer func() {
-				cleanupErr := cleanup()
-				if cleanupErr != nil {
-					err = errors.Join(err, fmt.Errorf("could not remove temporary group %q: %v", g.Name, cleanupErr))
-				}
-			}()
+			defer cleanup()
 
 			g.GID = &gid
 		}

internal/users/tempentries/groups.go

@@ -70,7 +70,7 @@ func groupEntry(group groupRecord) types.GroupEntry {
 //
 // Returns the generated GID and a cleanup function that should be called to remove the temporary group once the group
 // was added to the database.
-func (r *temporaryGroupRecords) RegisterGroup(name string) (gid uint32, cleanup func() error, err error) {
+func (r *temporaryGroupRecords) RegisterGroup(name string) (gid uint32, cleanup func(), err error) {
 	r.registerMu.Lock()
 	defer r.registerMu.Unlock()
 
@@ -102,19 +102,15 @@ func (r *temporaryGroupRecords) RegisterGroup(name string) (gid uint32, cleanup
 
 		unique, err := r.uniqueNameAndGID(name, gid, tmpID)
 		if err != nil {
-			if cleanupErr := cleanup(); cleanupErr != nil {
-				err = errors.Join(err, cleanupErr)
-			}
+			cleanup()
 			return 0, nil, fmt.Errorf("could not check if GID %d is unique: %w", gid, err)
 		}
 		if unique {
 			break
 		}
 
 		// If the GID is not unique, remove the temporary group and generate a new one in the next iteration.
-		if err := cleanup(); err != nil {
-			return 0, nil, fmt.Errorf("could not remove temporary group %q: %w", name, err)
-		}
+		cleanup()
 	}
 
 	log.Debugf(context.Background(), "Registered group %q with GID %d", name, gid)
@@ -142,7 +138,7 @@ func (r *temporaryGroupRecords) uniqueNameAndGID(name string, gid uint32, tmpID
 	return true, nil
 }
 
-func (r *temporaryGroupRecords) addTemporaryGroup(gid uint32, name string) (tmpID string, cleanup func() error, err error) {
+func (r *temporaryGroupRecords) addTemporaryGroup(gid uint32, name string) (tmpID string, cleanup func(), err error) {
 	r.rwMu.Lock()
 	defer r.rwMu.Unlock()
 
@@ -157,23 +153,23 @@ func (r *temporaryGroupRecords) addTemporaryGroup(gid uint32, name string) (tmpI
 	r.groups[gid] = groupRecord{name: name, gid: gid, passwd: tmpID}
 	r.gidByName[name] = gid
 
-	cleanup = func() error { return r.deleteTemporaryGroup(gid) }
+	cleanup = func() { r.deleteTemporaryGroup(gid) }
 
 	return tmpID, cleanup, nil
 }
 
-func (r *temporaryGroupRecords) deleteTemporaryGroup(gid uint32) error {
+func (r *temporaryGroupRecords) deleteTemporaryGroup(gid uint32) {
 	r.rwMu.Lock()
 	defer r.rwMu.Unlock()
 
 	group, ok := r.groups[gid]
 	if !ok {
-		return fmt.Errorf("temporary group with GID %d does not exist", gid)
+		log.Warningf(context.Background(), "Can't delete temporary group with GID %d, it does not exist", gid)
+		return
 	}
 
 	delete(r.groups, gid)
 	delete(r.gidByName, group.name)
 
 	log.Debugf(context.Background(), "Removed temporary record for group %q with GID %d", group.name, gid)
-	return nil
 }

internal/users/tempentries/groups_test.go

@@ -55,8 +55,7 @@ func TestRegisterGroup(t *testing.T) {
 			checkGroup(t, group)
 
 			// Delete the temporary group
-			err = cleanup()
-			require.NoError(t, err, "Cleanup should not return an error, but did")
+			cleanup()
 
 			// Check that the temporary group was deleted
 			_, err = records.GroupByID(gid)
@@ -109,13 +108,9 @@ func TestGroupByIDAndName(t *testing.T) {
 				require.Equal(t, gidToGenerate, gid, "GID should be the one generated by the IDGenerator")
 
 				if tc.groupAlreadyRemoved {
-					err = cleanup()
-					require.NoError(t, err, "Cleanup should not return an error, but did")
+					cleanup()
 				} else {
-					defer func() {
-						err := cleanup()
-						require.NoError(t, err, "Cleanup should not return an error, but did")
-					}()
+					defer cleanup()
 				}
 			}
 

internal/users/tempentries/tempentries.go

@@ -61,7 +61,7 @@ func (r *TemporaryRecords) UserByName(name string) (types.UserEntry, error) {
 //
 // Returns the generated UID and a cleanup function that should be called to remove the temporary user once the user was
 // added to the database.
-func (r *TemporaryRecords) RegisterUser(name string) (uid uint32, cleanup func() error, err error) {
+func (r *TemporaryRecords) RegisterUser(name string) (uid uint32, cleanup func(), err error) {
 	r.temporaryUserRecords.registerMu.Lock()
 	defer r.temporaryUserRecords.registerMu.Unlock()
 
@@ -105,27 +105,23 @@ func (r *TemporaryRecords) RegisterUser(name string) (uid uint32, cleanup func()
 		unique, err := r.temporaryUserRecords.uniqueNameAndUID(name, uid, tmpID)
 		if err != nil {
 			err = fmt.Errorf("checking UID and name uniqueness: %w", err)
-			if cleanupErr := cleanup(); cleanupErr != nil {
-				err = errors.Join(err, cleanupErr)
-			}
+			cleanup()
 			return 0, nil, err
 		}
 		if unique {
 			break
 		}
 
 		// If the UID is not unique, remove the temporary user and generate a new one in the next iteration.
-		if err := cleanup(); err != nil {
-			return 0, nil, fmt.Errorf("could not remove temporary user %q: %w", name, err)
-		}
+		cleanup()
 	}
 
 	log.Debugf(context.Background(), "Added temporary record for user %q with UID %d", name, uid)
 	return uid, cleanup, nil
 }
 
 // replacePreAuthUser replaces a pre-auth user with a temporary user with the same name and UID.
-func (r *TemporaryRecords) replacePreAuthUser(user types.UserEntry, name string) (uid uint32, cleanup func() error, err error) {
+func (r *TemporaryRecords) replacePreAuthUser(user types.UserEntry, name string) (uid uint32, cleanup func(), err error) {
 	var tmpID string
 	tmpID, cleanup, err = r.addTemporaryUser(user.UID, name)
 	if err != nil {
@@ -139,16 +135,12 @@ func (r *TemporaryRecords) replacePreAuthUser(user types.UserEntry, name string)
 	unique, err := r.temporaryUserRecords.uniqueNameAndUID(name, user.UID, tmpID)
 	if err != nil {
 		err = fmt.Errorf("checking UID and name uniqueness: %w", err)
-		if cleanupErr := cleanup(); cleanupErr != nil {
-			err = errors.Join(err, cleanupErr)
-		}
+		cleanup()
 		return 0, nil, err
 	}
 	if !unique {
 		err = fmt.Errorf("UID (%d) or name (%q) from pre-auth user are not unique", user.UID, name)
-		if cleanupErr := cleanup(); cleanupErr != nil {
-			err = errors.Join(err, cleanupErr)
-		}
+		cleanup()
 		return 0, nil, err
 	}
 

internal/users/tempentries/tempentries_test.go

@@ -89,8 +89,7 @@ func TestRegisterUser(t *testing.T) {
 			require.NoError(t, err, "UserByID should not return an error, but did")
 			checkUser(t, user)
 
-			err = cleanup()
-			require.NoError(t, err, "Cleanup should not return an error, but did")
+			cleanup()
 		})
 	}
 }
@@ -140,13 +139,9 @@ func TestUserByIDAndName(t *testing.T) {
 				require.Equal(t, uidToGenerate, uid, "UID should be the one generated by the IDGenerator")
 
 				if tc.userAlreadyRemoved {
-					err = cleanup()
-					require.NoError(t, err, "Cleanup should not return an error, but did")
+					cleanup()
 				} else {
-					defer func() {
-						err := cleanup()
-						require.NoError(t, err, "Cleanup should not return an error, but did")
-					}()
+					defer cleanup()
 				}
 			}
 

internal/users/tempentries/users.go

@@ -96,7 +96,7 @@ func (r *temporaryUserRecords) uniqueNameAndUID(name string, uid uint32, tmpID s
 
 // addTemporaryUser adds a temporary user with a random name and the given UID. It returns the generated name.
 // If the UID is already registered, it returns a errUserAlreadyExists.
-func (r *temporaryUserRecords) addTemporaryUser(uid uint32, name string) (tmpID string, cleanup func() error, err error) {
+func (r *temporaryUserRecords) addTemporaryUser(uid uint32, name string) (tmpID string, cleanup func(), err error) {
 	r.rwMu.Lock()
 	defer r.rwMu.Unlock()
 
@@ -111,23 +111,23 @@ func (r *temporaryUserRecords) addTemporaryUser(uid uint32, name string) (tmpID
 	r.users[uid] = userRecord{name: name, uid: uid, gecos: tmpID}
 	r.uidByName[name] = uid
 
-	cleanup = func() error { return r.deleteTemporaryUser(uid) }
+	cleanup = func() { r.deleteTemporaryUser(uid) }
 
 	return tmpID, cleanup, nil
 }
 
 // deleteTemporaryUser deletes the temporary user with the given UID.
-func (r *temporaryUserRecords) deleteTemporaryUser(uid uint32) error {
+func (r *temporaryUserRecords) deleteTemporaryUser(uid uint32) {
 	r.rwMu.Lock()
 	defer r.rwMu.Unlock()
 
 	user, ok := r.users[uid]
 	if !ok {
-		return fmt.Errorf("temporary user with UID %d does not exist", uid)
+		log.Warningf(context.Background(), "Can't delete temporary user with UID %d, it does not exist", uid)
+		return
 	}
 	delete(r.users, uid)
 	delete(r.uidByName, user.name)
 
 	log.Debugf(context.Background(), "Removed temporary record for user %q with UID %d", user.name, uid)
-	return nil
 }