From e0ac675a49dd0734da91a7de548ab17a1168c22c Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Fri, 10 Jan 2025 15:59:02 +0100 Subject: [PATCH] fixup! Generate unique UIDs and GIDs --- internal/users/manager.go | 16 +++------------ internal/users/tempentries/groups.go | 20 ++++++++----------- internal/users/tempentries/groups_test.go | 11 +++------- internal/users/tempentries/tempentries.go | 20 ++++++------------- .../users/tempentries/tempentries_test.go | 11 +++------- internal/users/tempentries/users.go | 10 +++++----- 6 files changed, 28 insertions(+), 60 deletions(-) diff --git a/internal/users/manager.go b/internal/users/manager.go index 75f143d4c..3796c8ac6 100644 --- a/internal/users/manager.go +++ b/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 } diff --git a/internal/users/tempentries/groups.go b/internal/users/tempentries/groups.go index fffc006f5..8334e4895 100644 --- a/internal/users/tempentries/groups.go +++ b/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,9 +102,7 @@ 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 { @@ -112,9 +110,7 @@ func (r *temporaryGroupRecords) RegisterGroup(name string) (gid uint32, cleanup } // 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 } diff --git a/internal/users/tempentries/groups_test.go b/internal/users/tempentries/groups_test.go index 94c5849ba..4dcc760cd 100644 --- a/internal/users/tempentries/groups_test.go +++ b/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() } } diff --git a/internal/users/tempentries/tempentries.go b/internal/users/tempentries/tempentries.go index ad6c87e7a..d5515732c 100644 --- a/internal/users/tempentries/tempentries.go +++ b/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,9 +105,7 @@ 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 { @@ -115,9 +113,7 @@ func (r *TemporaryRecords) RegisterUser(name string) (uid uint32, cleanup func() } // 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) @@ -125,7 +121,7 @@ func (r *TemporaryRecords) RegisterUser(name string) (uid uint32, cleanup func() } // 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 } diff --git a/internal/users/tempentries/tempentries_test.go b/internal/users/tempentries/tempentries_test.go index 199328d8b..a70b3c175 100644 --- a/internal/users/tempentries/tempentries_test.go +++ b/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() } } diff --git a/internal/users/tempentries/users.go b/internal/users/tempentries/users.go index e6e3b20fd..0cc7688a1 100644 --- a/internal/users/tempentries/users.go +++ b/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 }