Allow software uninstalls, script-based lock/unlock/wipe, while scripts are globally disabled (#24815)

For #22875.

# Checklist for submitter

If some of the following don't apply, delete the relevant line.

<!-- Note that API documentation changes are now addressed by the
product design team. -->

- [x] Changes file added for user-visible changes in `changes/`,
`orbit/changes/` or `ee/fleetd-chrome/changes`.
See [Changes
files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/Committing-Changes.md#changes-files)
for more information.
- [x] Input data is properly validated, `SELECT *` is avoided, SQL
injection is prevented (using placeholders for values in statements)
- [x] Added/updated tests
- [x] If database migrations are included, checked table schema to
confirm autoupdate
- For database migrations:
- [x] Checked schema for all modified table for columns that will
auto-update timestamps during migration.
- [x] Confirmed that updating the timestamps is acceptable, and will not
cause unwanted side effects.
- [x] Ensured the correct collation is explicitly set for character
columns (`COLLATE utf8mb4_unicode_ci`).
- [x] Manual QA for all new/changed functionality

Author: iansltx

changes/22875-uninstall-with-scripts-disabled

@@ -0,0 +1 @@
+* Allowed software uninstalls and script-based host lock/unlock/wipe to run while global scripts are disabled.

cmd/fleetctl/mdm.go

@@ -265,16 +265,6 @@ func mdmWipeCommand() *cli.Command {
 				return err
 			}
 
-			config, err := client.GetAppConfig()
-			if err != nil {
-				return err
-			}
-
-			// linux hosts need scripts to be enabled in the org settings to wipe.
-			if host.Platform == "linux" && config.ServerSettings.ScriptsDisabled {
-				return errors.New("Can't wipe host because running scripts is disabled in organization settings.")
-			}
-
 			if err := client.MDMWipeHost(host.ID); err != nil {
 				return fmt.Errorf("Failed to wipe host: %w", err)
 			}

cmd/fleetctl/mdm_test.go

@@ -1246,7 +1246,7 @@ func TestMDMWipeCommand(t *testing.T) {
 		{appCfgAllMDM, "valid windows but host is locked", []string{"--host", winEnrolledLocked.host.UUID}, "Host cannot be wiped until it is unlocked."},
 		{appCfgAllMDM, "valid macos but host is locked", []string{"--host", macEnrolledLocked.host.UUID}, "Host cannot be wiped until it is unlocked."},
 		{appCfgAllMDM, "valid macos but host is locked", []string{"--host", macEnrolledLocked.host.UUID}, "Host cannot be wiped until it is unlocked."},
-		{appCfgScriptsDisabled, "valid linux but script are disabled", []string{"--host", linuxEnrolled.host.UUID}, "Can't wipe host because running scripts is disabled in organization settings."},
+		{appCfgScriptsDisabled, "valid linux and scripts are disabled", []string{"--host", linuxEnrolled.host.UUID}, ""},
 	}
 
 	successfulOutput := func(ident string) string {

cmd/fleetctl/scripts_test.go

@@ -338,7 +338,7 @@ Fleet records the last 10,000 characters to prevent downtime.
 			}
 			return &h, nil
 		}
-		ds.ListPendingHostScriptExecutionsFunc = func(ctx context.Context, hid uint) ([]*fleet.HostScriptResult, error) {
+		ds.ListPendingHostScriptExecutionsFunc = func(ctx context.Context, hid uint, onlyShowInternal bool) ([]*fleet.HostScriptResult, error) {
 			require.Equal(t, uint(42), hid)
 			if c.expectPending {
 				return []*fleet.HostScriptResult{{HostID: uint(42)}}, nil

ee/server/service/hosts.go

@@ -91,18 +91,6 @@ func (svc *Service) LockHost(ctx context.Context, hostID uint, viewPIN bool) (un
 				return "", ctxerr.Wrap(ctx, err, "check windows MDM enabled")
 			}
 		}
-		// on windows and linux, a script is used to lock the host so scripts must
-		// be enabled
-		appCfg, err := svc.ds.AppConfig(ctx)
-		if err != nil {
-			return "", ctxerr.Wrap(ctx, err, "get app config")
-		}
-		if appCfg.ServerSettings.ScriptsDisabled {
-			return "", ctxerr.Wrap(
-				ctx,
-				fleet.NewInvalidArgumentError("host_id", "Can't lock host because running scripts is disabled in organization settings."),
-			)
-		}
 		hostOrbitInfo, err := svc.ds.GetHostOrbitInfo(ctx, host.ID)
 		switch {
 		case err != nil:
@@ -181,8 +169,8 @@ func (svc *Service) UnlockHost(ctx context.Context, hostID uint) (string, error)
 		// is currently locked.
 
 	case "windows", "linux":
-		// on windows and linux, a script is used to lock the host so scripts must
-		// be enabled
+		// on Windows and Linux, a script is used to unlock the host so scripts must
+		// be enabled on the host
 		if host.FleetPlatform() == "windows" {
 			if err := svc.VerifyMDMWindowsConfigured(ctx); err != nil {
 				if errors.Is(err, fleet.ErrMDMNotConfigured) {
@@ -191,13 +179,6 @@ func (svc *Service) UnlockHost(ctx context.Context, hostID uint) (string, error)
 				return "", ctxerr.Wrap(ctx, err, "check windows MDM enabled")
 			}
 		}
-		appCfg, err := svc.ds.AppConfig(ctx)
-		if err != nil {
-			return "", ctxerr.Wrap(ctx, err, "get app config")
-		}
-		if appCfg.ServerSettings.ScriptsDisabled {
-			return "", ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("host_id", "Can't unlock host because running scripts is disabled in organization settings."))
-		}
 		hostOrbitInfo, err := svc.ds.GetHostOrbitInfo(ctx, host.ID)
 		switch {
 		case err != nil:
@@ -286,14 +267,7 @@ func (svc *Service) WipeHost(ctx context.Context, hostID uint) error {
 		requireMDM = true
 
 	case "linux":
-		// on linux, a script is used to wipe the host so scripts must be enabled
-		appCfg, err := svc.ds.AppConfig(ctx)
-		if err != nil {
-			return ctxerr.Wrap(ctx, err, "get app config")
-		}
-		if appCfg.ServerSettings.ScriptsDisabled {
-			return ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("host_id", "Can't wipe host because running scripts is disabled in organization settings."))
-		}
+		// on linux, a script is used to wipe the host so scripts must be enabled on the host
 		hostOrbitInfo, err := svc.ds.GetHostOrbitInfo(ctx, host.ID)
 		switch {
 		case err != nil:

ee/server/service/software_installers.go

@@ -1026,18 +1026,6 @@ func (svc *Service) installSoftwareTitleUsingInstaller(ctx context.Context, host
 }
 
 func (svc *Service) UninstallSoftwareTitle(ctx context.Context, hostID uint, softwareTitleID uint) error {
-	// First check if scripts are disabled globally. If so, no need for further processing.
-	cfg, err := svc.ds.AppConfig(ctx)
-	if err != nil {
-		svc.authz.SkipAuthorization(ctx)
-		return err
-	}
-
-	if cfg.ServerSettings.ScriptsDisabled {
-		svc.authz.SkipAuthorization(ctx)
-		return fleet.NewUserMessageError(errors.New(fleet.RunScriptScriptsDisabledGloballyErrMsg), http.StatusForbidden)
-	}
-
 	// we need to use ds.Host because ds.HostLite doesn't return the orbit node key
 	host, err := svc.ds.Host(ctx, hostID)
 	if err != nil {
@@ -1138,7 +1126,7 @@ func (svc *Service) UninstallSoftwareTitle(ctx context.Context, hostID uint, sof
 	if host.TeamID != nil {
 		teamID = *host.TeamID
 	}
-	// create the script execution request, the host will be notified of the
+	// create the script execution request; the host will be notified of the
 	// script execution request via the orbit config's Notifications mechanism.
 	request := fleet.HostScriptRequestPayload{
 		HostID:          host.ID,
@@ -1149,7 +1137,7 @@ func (svc *Service) UninstallSoftwareTitle(ctx context.Context, hostID uint, sof
 	if ctxUser := authz.UserFromContext(ctx); ctxUser != nil {
 		request.UserID = &ctxUser.ID
 	}
-	scriptResult, err := svc.ds.NewHostScriptExecutionRequest(ctx, &request)
+	scriptResult, err := svc.ds.NewInternalScriptExecutionRequest(ctx, &request)
 	if err != nil {
 		return ctxerr.Wrap(ctx, err, "create script execution request")
 	}

ee/server/service/software_installers_test.go

@@ -82,7 +82,9 @@ func TestInstallUninstallAuth(t *testing.T) {
 	svc := newTestService(t, ds)
 
 	ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) {
-		return &fleet.AppConfig{}, nil
+		return &fleet.AppConfig{
+			ServerSettings: fleet.ServerSettings{ScriptsDisabled: true}, // global scripts being disabled shouldn't impact (un)installs
+		}, nil
 	}
 	ds.HostFunc = func(ctx context.Context, id uint) (*fleet.Host, error) {
 		return &fleet.Host{
@@ -111,9 +113,8 @@ func TestInstallUninstallAuth(t *testing.T) {
 	ds.GetAnyScriptContentsFunc = func(ctx context.Context, id uint) ([]byte, error) {
 		return []byte("script"), nil
 	}
-	ds.NewHostScriptExecutionRequestFunc = func(ctx context.Context, request *fleet.HostScriptRequestPayload) (*fleet.HostScriptResult,
-		error,
-	) {
+	ds.NewInternalScriptExecutionRequestFunc = func(ctx context.Context, request *fleet.HostScriptRequestPayload) (*fleet.HostScriptResult,
+		error) {
 		return &fleet.HostScriptResult{
 			ExecutionID: "execution_id",
 		}, nil
@@ -187,18 +188,14 @@ func TestUninstallSoftwareTitle(t *testing.T) {
 		return host, nil
 	}
 
-	// Scripts disabled
+	// Global scripts disabled (doesn't matter)
 	ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) {
 		return &fleet.AppConfig{
 			ServerSettings: fleet.ServerSettings{
 				ScriptsDisabled: true,
 			},
 		}, nil
 	}
-	require.ErrorContains(t, svc.UninstallSoftwareTitle(context.Background(), 1, 10), fleet.RunScriptScriptsDisabledGloballyErrMsg)
-	ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) {
-		return &fleet.AppConfig{}, nil
-	}
 
 	// Host scripts disabled
 	host.ScriptsEnabled = ptr.Bool(false)

frontend/pages/admin/OrgSettingsPage/cards/Advanced/Advanced.tsx

@@ -347,15 +347,18 @@ const Advanced = ({
             parseTarget
             tooltipContent={
               <>
-                Disabling scripts will block access to run scripts. Scripts{" "}
-                <br /> may still be added and removed in the UI and API. <br />
+                Disabling script execution will block access to run scripts.
+                <br />
+                Scripts may still be added and removed in the UI and API.
+                <br />
                 <em>
-                  (Default: <strong>Off</strong>)
+                  (Default: <b>Off</b>)
                 </em>
               </>
             }
+            helpText="Features that run scripts under-the-hood (e.g. software install, lock/wipe) will still be available."
           >
-            Disable scripts
+            Disable script execution features
           </Checkbox>
           <Checkbox
             onChange={onInputChange}

server/datastore/mysql/migrations/tables/20241224000000_InternalHostScriptResults.go

@@ -0,0 +1,25 @@
+package tables
+
+import (
+	"database/sql"
+	"fmt"
+)
+
+func init() {
+	MigrationClient.AddMigration(Up_20241224000000, Down_20241224000000)
+}
+
+func Up_20241224000000(tx *sql.Tx) error {
+	// alter the host_script_results table to add an is_internal flag (for scripts that still execute
+	// when scripts are disabled globally)
+	_, err := tx.Exec(`ALTER TABLE host_script_results ADD COLUMN is_internal BOOLEAN DEFAULT FALSE`)
+	if err != nil {
+		return fmt.Errorf("add is_internal to host_script_results: %w", err)
+	}
+
+	return nil
+}
+
+func Down_20241224000000(tx *sql.Tx) error {
+	return nil
+}

server/datastore/mysql/migrations/tables/20241224000000_InternalHostScriptResults_test.go

@@ -0,0 +1,72 @@
+package tables
+
+import (
+	"testing"
+
+	"github.com/google/uuid"
+	"github.com/stretchr/testify/require"
+)
+
+func TestUp_20241224000000(t *testing.T) {
+	db := applyUpToPrev(t)
+
+	const (
+		insertScriptResultStmt = `INSERT INTO host_script_results (
+			host_id, execution_id, script_content_id, script_id, output
+		) VALUES (?, ?, ?, ?, '')`
+
+		insertScriptStmt = `INSERT INTO scripts (
+			team_id, global_or_team_id, name, script_content_id
+		) VALUES (?, ?, ?, ?)`
+
+		loadResultStmt = `SELECT
+			id, host_id, execution_id, script_id, is_internal
+		FROM host_script_results WHERE id = ?`
+	)
+
+	type script struct {
+		id, globalOrTeamID   int64
+		name, scriptContents string
+		teamID               *int64
+	}
+	type scriptResult struct {
+		id, hostID  int64
+		executionID string
+		scriptID    *int64
+		isInternal  bool
+	}
+
+	// create a global script
+	globalScript := script{
+		globalOrTeamID: 0,
+		teamID:         nil,
+		name:           "global-script",
+		scriptContents: "b",
+	}
+
+	scriptContentsID := execNoErrLastID(t, db, "INSERT INTO script_contents(contents, md5_checksum) VALUES (?, 'a')", globalScript.scriptContents)
+
+	res, err := db.Exec(insertScriptStmt, globalScript.teamID, globalScript.globalOrTeamID, globalScript.name, scriptContentsID)
+	require.NoError(t, err)
+	globalScript.id, _ = res.LastInsertId()
+
+	// create a host script result for that global script
+	globalScriptResult := scriptResult{
+		hostID:      123,
+		executionID: uuid.New().String(),
+		scriptID:    &globalScript.id,
+	}
+	res, err = db.Exec(insertScriptResultStmt, globalScriptResult.hostID, globalScriptResult.executionID, scriptContentsID, globalScriptResult.scriptID)
+	require.NoError(t, err)
+	globalScriptResult.id, _ = res.LastInsertId()
+
+	// Apply current migration.
+	applyNext(t, db)
+
+	// the global host script result should not be set as internal
+	var result scriptResult
+	err = db.QueryRow(loadResultStmt, globalScriptResult.id).Scan(&result.id, &result.hostID, &result.executionID, &result.scriptID, &result.isInternal)
+	require.NoError(t, err)
+	require.False(t, result.isInternal)
+	require.Equal(t, globalScriptResult, result)
+}