addressing PR feedback

api/types/constants.go

@@ -1289,7 +1289,7 @@ const (
 	// TeleportManagedGroup is a default group that users of the
 	// teleport automated user provisioning system get added to
 	// when provisioned in KEEP mode. This prevents already
-	// existing users from being tampered with ordeleted.
+	// existing users from being tampered with or deleted.
 	TeleportManagedGroup = "teleport-managed"
 )
 

lib/srv/usermgmt.go

@@ -26,6 +26,7 @@ import (
 	"maps"
 	"os/user"
 	"regexp"
+	"slices"
 	"strings"
 	"syscall"
 	"time"
@@ -225,8 +226,8 @@ func (u *HostSudoersManagement) RemoveSudoers(name string) error {
 	return nil
 }
 
-// UnmanagedUserErr is returned when attempting to modify or interact with a user that is not managed by Teleport.
-var UnmanagedUserErr = errors.New("user not managed by teleport")
+// unmanagedUserErr is returned when attempting to modify or interact with a user that is not managed by Teleport.
+var unmanagedUserErr = errors.New("user not managed by teleport")
 
 func (u *HostUserManagement) updateUser(name string, ui services.HostUsersInfo) error {
 
@@ -250,23 +251,18 @@ func (u *HostUserManagement) updateUser(name string, ui services.HostUsersInfo)
 		currentGroups[group.Name] = struct{}{}
 	}
 
-	// allow for direct assignming of teleport-managed group even if the user is not currently managed
-	assigningManagedGroup := false
-	for _, group := range ui.Groups {
-		if group == types.TeleportManagedGroup {
-			assigningManagedGroup = true
-			break
-		}
-	}
+	// allow for explicit assignment of teleport-managed group in order to facilitate migrating KEEP users that existed before we added
+	// the teleport-managed group
+	migrateKeepUser := slices.Contains(ui.Groups, types.TeleportManagedGroup)
 
-	if assigningManagedGroup && ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP {
-		return trace.Errorf("can not explicitly assign %q group in 'INSECURE_DROP' mode: %w", types.TeleportManagedGroup, UnmanagedUserErr)
+	if migrateKeepUser && ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP {
+		return trace.Errorf("can not explicitly assign %q group in 'INSECURE_DROP' mode: %w", types.TeleportManagedGroup, unmanagedUserErr)
 	}
 
 	_, hasSystemGroup := currentGroups[types.TeleportServiceGroup]
 	_, hasManagedGroup := currentGroups[types.TeleportManagedGroup]
-	if !hasSystemGroup && !hasManagedGroup && !assigningManagedGroup {
-		return trace.Errorf("%q %w", name, UnmanagedUserErr)
+	if !hasSystemGroup && !hasManagedGroup && !migrateKeepUser {
+		return trace.Errorf("%q %w", name, unmanagedUserErr)
 	}
 
 	if ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP {
@@ -286,7 +282,7 @@ func (u *HostUserManagement) updateUser(name string, ui services.HostUsersInfo)
 		}
 
 		// no need to duplicate the group if it's already there
-		if !assigningManagedGroup {
+		if !migrateKeepUser {
 			ui.Groups = append(ui.Groups, types.TeleportManagedGroup)
 		}
 	}
@@ -367,7 +363,7 @@ func (u *HostUserManagement) createUser(name string, ui services.HostUsersInfo)
 }
 
 func (u *HostUserManagement) ensureGroupsExist(groups ...string) error {
-	errs := make([]error, 0, len(groups))
+	var errs []error
 
 	for _, group := range groups {
 		if err := u.createGroupIfNotExist(group); err != nil {

lib/srv/usermgmt_test.go

@@ -245,7 +245,7 @@ func TestUserMgmt_CreateTemporaryUser(t *testing.T) {
 
 	// an existing, unmanaged user should not be changed
 	closer, err = users.UpsertUser("simon", userinfo)
-	require.ErrorIs(t, err, UnmanagedUserErr)
+	require.ErrorIs(t, err, unmanagedUserErr)
 	require.Equal(t, nil, closer)
 }
 
@@ -291,12 +291,12 @@ func TestUserMgmtSudoers_CreateTemporaryUser(t *testing.T) {
 		_, err := users.UpsertUser("testuser", services.HostUsersInfo{
 			Mode: types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP,
 		})
-		require.ErrorIs(t, err, UnmanagedUserErr)
+		require.ErrorIs(t, err, unmanagedUserErr)
 		backend.CreateGroup(types.TeleportServiceGroup, "")
 		_, err = users.UpsertUser("testuser", services.HostUsersInfo{
 			Mode: types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP,
 		})
-		require.ErrorIs(t, err, UnmanagedUserErr)
+		require.ErrorIs(t, err, unmanagedUserErr)
 	})
 }
 
@@ -453,6 +453,7 @@ func Test_UpdateUserGroups_Keep(t *testing.T) {
 	assert.NotEqual(t, nil, closer)
 	assert.Equal(t, 2, backend.setUserGroupsCalls)
 	assert.ElementsMatch(t, append(userinfo.Groups, types.TeleportServiceGroup), backend.users["alice"])
+	assert.NotContains(t, backend.users["alice"], types.TeleportManagedGroup)
 }
 
 func Test_UpdateUserGroups_Drop(t *testing.T) {
@@ -508,6 +509,7 @@ func Test_UpdateUserGroups_Drop(t *testing.T) {
 	assert.Equal(t, 2, backend.setUserGroupsCalls)
 	assert.Equal(t, 1, backend.createHomeDirectoryCalls)
 	assert.ElementsMatch(t, append(userinfo.Groups, types.TeleportManagedGroup), backend.users["alice"])
+	assert.NotContains(t, backend.users["alice"], types.TeleportServiceGroup)
 }
 
 func Test_DontManageExistingUser(t *testing.T) {
@@ -536,15 +538,15 @@ func Test_DontManageExistingUser(t *testing.T) {
 
 	// Update user in DROP mode
 	closer, err := users.UpsertUser("alice", userinfo)
-	assert.Error(t, err, UnmanagedUserErr)
+	assert.Error(t, err, unmanagedUserErr)
 	assert.Equal(t, nil, closer)
 	assert.Zero(t, backend.setUserGroupsCalls)
 	assert.ElementsMatch(t, allGroups, backend.users["alice"])
 
 	// Update user in KEEP mode
 	userinfo.Mode = types.CreateHostUserMode_HOST_USER_MODE_KEEP
 	closer, err = users.UpsertUser("alice", userinfo)
-	assert.ErrorIs(t, err, UnmanagedUserErr)
+	assert.ErrorIs(t, err, unmanagedUserErr)
 	assert.Equal(t, nil, closer)
 	assert.Zero(t, backend.setUserGroupsCalls)
 	assert.ElementsMatch(t, allGroups, backend.users["alice"])
@@ -575,7 +577,7 @@ func Test_DontUpdateUnmanagedUsers(t *testing.T) {
 
 	// Try to update existing, unmanaged user in KEEP mode
 	closer, err := users.UpsertUser("alice", userinfo)
-	assert.ErrorIs(t, err, UnmanagedUserErr)
+	assert.ErrorIs(t, err, unmanagedUserErr)
 	assert.Equal(t, nil, closer)
 	assert.Zero(t, backend.setUserGroupsCalls)
 	assert.ElementsMatch(t, allGroups[2:], backend.users["alice"])
@@ -587,7 +589,7 @@ func Test_DontUpdateUnmanagedUsers(t *testing.T) {
 
 	// Try to update existing, unmanaged user in DROP mode
 	closer, err = users.UpsertUser("alice", userinfo)
-	assert.ErrorIs(t, err, UnmanagedUserErr)
+	assert.ErrorIs(t, err, unmanagedUserErr)
 	assert.Equal(t, nil, closer)
 	assert.Zero(t, backend.setUserGroupsCalls)
 	assert.ElementsMatch(t, allGroups[2:], backend.users["alice"])
@@ -621,7 +623,7 @@ func Test_AllowExplicitlyManageExistingUsers(t *testing.T) {
 
 	// Take ownership of existing user when in KEEP mode
 	closer, err := users.UpsertUser("alice-keep", userinfo)
-	assert.NoError(t, err, UnmanagedUserErr)
+	assert.NoError(t, err)
 	assert.Equal(t, nil, closer)
 	assert.Equal(t, 1, backend.setUserGroupsCalls)
 	// slice off the end because teleport-system should be explicitly excluded
@@ -631,7 +633,7 @@ func Test_AllowExplicitlyManageExistingUsers(t *testing.T) {
 	// Don't take ownership of existing user when in DROP mode
 	userinfo.Mode = types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP
 	closer, err = users.UpsertUser("alice-drop", userinfo)
-	assert.ErrorIs(t, err, UnmanagedUserErr)
+	assert.ErrorIs(t, err, unmanagedUserErr)
 	assert.Contains(t, err.Error(), "can not explicitly assign")
 	assert.Equal(t, nil, closer)
 	assert.Equal(t, 1, backend.setUserGroupsCalls)