Skip to content

Commit

Permalink
Minor fixes to policy software installations (#22148)
Browse files Browse the repository at this point in the history
PR for the three fixes described in #22104.

- [X] Added/updated tests
- [X] Manual QA for all new/changed functionality
  • Loading branch information
lucasmrod authored Sep 17, 2024
1 parent b93d262 commit 2d05f24
Show file tree
Hide file tree
Showing 15 changed files with 94 additions and 57 deletions.
2 changes: 1 addition & 1 deletion cmd/fleetctl/get_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2320,7 +2320,7 @@ func TestGetTeamsYAMLAndApply(t *testing.T) {
declaration.DeclarationUUID = uuid.NewString()
return declaration, nil
}
ds.BatchSetSoftwareInstallersFunc = func(ctx context.Context, tmID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwareInstaller, error) {
ds.BatchSetSoftwareInstallersFunc = func(ctx context.Context, tmID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwarePackageResponse, error) {
return nil, nil
}

Expand Down
41 changes: 32 additions & 9 deletions cmd/fleetctl/gitops_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ func TestGitOpsBasicGlobalPremium(t *testing.T) {
ds.NewJobFunc = func(ctx context.Context, job *fleet.Job) (*fleet.Job, error) {
return &fleet.Job{}, nil
}
ds.BatchSetSoftwareInstallersFunc = func(ctx context.Context, teamID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwareInstaller, error) {
ds.BatchSetSoftwareInstallersFunc = func(ctx context.Context, teamID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwarePackageResponse, error) {
return nil, nil
}

Expand Down Expand Up @@ -373,7 +373,7 @@ func TestGitOpsBasicTeam(t *testing.T) {
ds.DeleteMDMAppleDeclarationByNameFunc = func(ctx context.Context, teamID *uint, name string) error {
return nil
}
ds.BatchSetSoftwareInstallersFunc = func(ctx context.Context, teamID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwareInstaller, error) {
ds.BatchSetSoftwareInstallersFunc = func(ctx context.Context, teamID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwarePackageResponse, error) {
return nil, nil
}
ds.ApplyEnrollSecretsFunc = func(ctx context.Context, teamID *uint, secrets []*fleet.EnrollSecret) error {
Expand Down Expand Up @@ -804,7 +804,7 @@ func TestGitOpsFullTeam(t *testing.T) {
return nil
}
var appliedSoftwareInstallers []*fleet.UploadSoftwareInstallerPayload
ds.BatchSetSoftwareInstallersFunc = func(ctx context.Context, teamID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwareInstaller, error) {
ds.BatchSetSoftwareInstallersFunc = func(ctx context.Context, teamID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwarePackageResponse, error) {
appliedSoftwareInstallers = installers
return nil, nil
}
Expand Down Expand Up @@ -1055,7 +1055,7 @@ func TestGitOpsBasicGlobalAndTeam(t *testing.T) {
savedTeam = team
return team, nil
}
ds.BatchSetSoftwareInstallersFunc = func(ctx context.Context, teamID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwareInstaller, error) {
ds.BatchSetSoftwareInstallersFunc = func(ctx context.Context, teamID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwarePackageResponse, error) {
return nil, nil
}
ds.ListSoftwareTitlesFunc = func(ctx context.Context, opt fleet.SoftwareTitleListOptions, tmFilter fleet.TeamFilter) ([]fleet.SoftwareTitleListResult, int, *fleet.PaginationMetadata, error) {
Expand Down Expand Up @@ -1317,7 +1317,7 @@ func TestGitOpsBasicGlobalAndNoTeam(t *testing.T) {
savedTeam = team
return team, nil
}
ds.BatchSetSoftwareInstallersFunc = func(ctx context.Context, teamID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwareInstaller, error) {
ds.BatchSetSoftwareInstallersFunc = func(ctx context.Context, teamID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwarePackageResponse, error) {
return nil, nil
}
ds.ListSoftwareTitlesFunc = func(ctx context.Context, opt fleet.SoftwareTitleListOptions, tmFilter fleet.TeamFilter) ([]fleet.SoftwareTitleListResult, int, *fleet.PaginationMetadata, error) {
Expand Down Expand Up @@ -1442,6 +1442,20 @@ software:
`)
require.NoError(t, err)

noTeamFilePathPoliciesCalendarPath := filepath.Join(t.TempDir(), "no-team.yml")
noTeamFilePathPoliciesCalendar, err := os.Create(noTeamFilePathPoliciesCalendarPath)
require.NoError(t, err)
_, err = noTeamFilePathPoliciesCalendar.WriteString(`
controls:
policies:
- name: Foobar
query: SELECT 1 FROM osquery_info WHERE start_time < 0;
calendar_events_enabled: true
name: No team
software:
`)
require.NoError(t, err)

noTeamFilePathWithControls := filepath.Join(t.TempDir(), "no-team.yml")
noTeamFileWithControls, err := os.Create(noTeamFilePathWithControls)
require.NoError(t, err)
Expand Down Expand Up @@ -1480,16 +1494,25 @@ software:
require.Error(t, err)
assert.True(t, strings.Contains(err.Error(), "'controls' cannot be set on both global config and on no-team.yml"))
// Real run, both global and no-team.yml define controls.
_, err = runAppNoChecks([]string{"gitops", "-f", globalFileWithControls.Name(), "-f", teamFile.Name(), "-f", noTeamFileWithControls.Name(), "--dry-run"})
_, err = runAppNoChecks([]string{"gitops", "-f", globalFileWithControls.Name(), "-f", teamFile.Name(), "-f", noTeamFileWithControls.Name()})
require.Error(t, err)
assert.True(t, strings.Contains(err.Error(), "'controls' cannot be set on both global config and on no-team.yml"))

// Dry run, both global and no-team.yml defines policy with calendar events enabled.
_, err = runAppNoChecks([]string{"gitops", "-f", globalFileWithControls.Name(), "-f", teamFile.Name(), "-f", noTeamFilePathPoliciesCalendar.Name(), "--dry-run"})
require.Error(t, err)
assert.True(t, strings.Contains(err.Error(), "calendar events are not supported on \"No team\" policies: \"Foobar\""), err.Error())
// Real run, both global and no-team.yml define controls.
_, err = runAppNoChecks([]string{"gitops", "-f", globalFileWithControls.Name(), "-f", teamFile.Name(), "-f", noTeamFilePathPoliciesCalendar.Name()})
require.Error(t, err)
assert.True(t, strings.Contains(err.Error(), "calendar events are not supported on \"No team\" policies: \"Foobar\""), err.Error())

// Dry run, controls should be defined somewhere, either in no-team.yml or global.
_, err = runAppNoChecks([]string{"gitops", "-f", globalFileWithoutControlsAndSoftwareKeys.Name(), "-f", teamFile.Name(), "-f", noTeamFileWithoutControls.Name(), "--dry-run"})
require.Error(t, err)
assert.True(t, strings.Contains(err.Error(), "'controls' must be set on global config or no-team.yml"))
// Real run, both global and no-team.yml define controls.
_, err = runAppNoChecks([]string{"gitops", "-f", globalFileWithoutControlsAndSoftwareKeys.Name(), "-f", teamFile.Name(), "-f", noTeamFileWithoutControls.Name(), "--dry-run"})
_, err = runAppNoChecks([]string{"gitops", "-f", globalFileWithoutControlsAndSoftwareKeys.Name(), "-f", teamFile.Name(), "-f", noTeamFileWithoutControls.Name()})
require.Error(t, err)
assert.True(t, strings.Contains(err.Error(), "'controls' must be set on global config or no-team.yml"))

Expand Down Expand Up @@ -1645,7 +1668,7 @@ func TestGitOpsTeamSoftwareInstallersQueryEnv(t *testing.T) {

t.Setenv("QUERY_VAR", "IT_WORKS")

ds.BatchSetSoftwareInstallersFunc = func(ctx context.Context, tmID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwareInstaller, error) {
ds.BatchSetSoftwareInstallersFunc = func(ctx context.Context, tmID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwarePackageResponse, error) {
if installers[0].PreInstallQuery != "select IT_WORKS" {
return nil, fmt.Errorf("Missing env var, got %s", installers[0].PreInstallQuery)
}
Expand Down Expand Up @@ -2158,7 +2181,7 @@ func setupFullGitOpsPremiumServer(t *testing.T) (*mock.Store, **fleet.AppConfig,
declaration.DeclarationUUID = uuid.NewString()
return declaration, nil
}
ds.BatchSetSoftwareInstallersFunc = func(ctx context.Context, teamID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwareInstaller, error) {
ds.BatchSetSoftwareInstallersFunc = func(ctx context.Context, teamID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwarePackageResponse, error) {
return nil, nil
}

Expand Down
5 changes: 3 additions & 2 deletions ee/server/service/software_installers.go
Original file line number Diff line number Diff line change
Expand Up @@ -969,7 +969,8 @@ func (svc *Service) UninstallSoftwareTitle(ctx context.Context, hostID uint, sof
}

func (svc *Service) insertSoftwareUninstallRequest(ctx context.Context, executionID string, host *fleet.Host,
installer *fleet.SoftwareInstaller) error {
installer *fleet.SoftwareInstaller,
) error {
if err := svc.ds.InsertSoftwareUninstallRequest(ctx, executionID, host.ID, installer.InstallerID); err != nil {
return ctxerr.Wrap(ctx, err, "inserting software uninstall request")
}
Expand Down Expand Up @@ -1114,7 +1115,7 @@ const maxInstallerSizeBytes int64 = 1024 * 1024 * 500

func (svc *Service) BatchSetSoftwareInstallers(
ctx context.Context, tmName string, payloads []fleet.SoftwareInstallerPayload, dryRun bool,
) ([]fleet.SoftwareInstaller, error) {
) ([]fleet.SoftwarePackageResponse, error) {
if err := svc.authz.Authorize(ctx, &fleet.Team{}, fleet.ActionRead); err != nil {
return nil, err
}
Expand Down
3 changes: 3 additions & 0 deletions pkg/spec/gitops.go
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,9 @@ func parsePolicies(top map[string]json.RawMessage, result *GitOps, baseDir strin
} else {
item.Team = ""
}
if item.CalendarEventsEnabled && result.IsNoTeam() {
multiError = multierror.Append(multiError, fmt.Errorf("calendar events are not supported on \"No team\" policies: %q", item.Name))
}
}
duplicates := getDuplicateNames(
result.Policies, func(p *GitOpsPolicySpec) string {
Expand Down
2 changes: 1 addition & 1 deletion server/datastore/mysql/activities.go
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ func (ds *Datastore) ListHostUpcomingActivities(ctx context.Context, hostID uint
'software_package', si.filename,
'install_uuid', hsi.execution_id,
'status', CAST(hsi.status AS CHAR),
'self_service', si.self_service IS TRUE
'self_service', hsi.self_service IS TRUE
) as details
FROM
host_software_installs hsi
Expand Down
16 changes: 4 additions & 12 deletions server/datastore/mysql/software_installers.go
Original file line number Diff line number Diff line change
Expand Up @@ -768,7 +768,7 @@ func (ds *Datastore) CleanupUnusedSoftwareInstallers(ctx context.Context, softwa
return ctxerr.Wrap(ctx, err, "cleanup unused software installers")
}

func (ds *Datastore) BatchSetSoftwareInstallers(ctx context.Context, tmID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwareInstaller, error) {
func (ds *Datastore) BatchSetSoftwareInstallers(ctx context.Context, tmID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwarePackageResponse, error) {
const upsertSoftwareTitles = `
INSERT INTO software_titles
(name, source, browser)
Expand Down Expand Up @@ -880,16 +880,7 @@ ON DUPLICATE KEY UPDATE

const loadInsertedSoftwareInstallers = `
SELECT
id,
team_id,
storage_id,
filename,
version,
install_script_content_id,
pre_install_query,
post_install_script_content_id,
platform,
self_service,
title_id,
url
FROM
Expand All @@ -903,7 +894,7 @@ WHERE global_or_team_id = ?
globalOrTeamID = *tmID
}

var insertedSoftwareInstallers []fleet.SoftwareInstaller
var insertedSoftwareInstallers []fleet.SoftwarePackageResponse
if err := ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error {
// if no installers are provided, just delete whatever was in
// the table
Expand Down Expand Up @@ -1127,7 +1118,8 @@ func (ds *Datastore) GetSoftwareInstallersWithoutPackageIDs(ctx context.Context)
}

func (ds *Datastore) UpdateSoftwareInstallerWithoutPackageIDs(ctx context.Context, id uint,
payload fleet.UploadSoftwareInstallerPayload) error {
payload fleet.UploadSoftwareInstallerPayload,
) error {
uninstallScriptID, err := ds.getOrGenerateScriptContentsID(ctx, payload.UninstallScript)
if err != nil {
return ctxerr.Wrap(ctx, err, "get or generate uninstall script contents ID")
Expand Down
21 changes: 14 additions & 7 deletions server/datastore/mysql/software_installers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -653,12 +653,14 @@ func testBatchSetSoftwareInstallers(t *testing.T, ds *Datastore) {
PreInstallQuery: "foo",
UserID: user1.ID,
Platform: "darwin",
URL: "https://example.com",
}})
require.NoError(t, err)
require.Len(t, softwareInstallers, 1)
require.Equal(t, ins0, softwareInstallers[0].Name)
require.NotNil(t, softwareInstallers[0].TeamID)
require.Equal(t, team.ID, *softwareInstallers[0].TeamID)
require.NotNil(t, softwareInstallers[0].TitleID)
require.Equal(t, "darwin", softwareInstallers[0].Platform)
require.Equal(t, "https://example.com", softwareInstallers[0].URL)
assertSoftware([]fleet.SoftwareTitle{
{Name: ins0, Source: "apps", Browser: ""},
})
Expand All @@ -678,6 +680,7 @@ func testBatchSetSoftwareInstallers(t *testing.T, ds *Datastore) {
PreInstallQuery: "select 0 from foo;",
UserID: user1.ID,
Platform: "darwin",
URL: "https://example.com",
},
{
InstallScript: "install",
Expand All @@ -691,16 +694,19 @@ func testBatchSetSoftwareInstallers(t *testing.T, ds *Datastore) {
PreInstallQuery: "select 1 from bar;",
UserID: user1.ID,
Platform: "darwin",
URL: "https://example2.com",
},
})
require.NoError(t, err)
require.Len(t, softwareInstallers, 2)
require.Equal(t, ins0, softwareInstallers[0].Name)
require.NotNil(t, softwareInstallers[0].TitleID)
require.Equal(t, "darwin", softwareInstallers[0].Platform)
require.Equal(t, ins1, softwareInstallers[1].Name)
require.NotNil(t, softwareInstallers[0].TeamID)
require.Equal(t, team.ID, *softwareInstallers[0].TeamID)
require.Equal(t, "https://example.com", softwareInstallers[0].URL)
require.NotNil(t, softwareInstallers[1].TitleID)
require.Equal(t, "darwin", softwareInstallers[1].Platform)
require.NotNil(t, softwareInstallers[1].TeamID)
require.Equal(t, team.ID, *softwareInstallers[1].TeamID)
require.Equal(t, "https://example2.com", softwareInstallers[1].URL)
assertSoftware([]fleet.SoftwareTitle{
{Name: ins0, Source: "apps", Browser: ""},
{Name: ins1, Source: "apps", Browser: ""},
Expand All @@ -723,8 +729,9 @@ func testBatchSetSoftwareInstallers(t *testing.T, ds *Datastore) {
})
require.NoError(t, err)
require.Len(t, softwareInstallers, 1)
require.Equal(t, ins1, softwareInstallers[0].Name)
require.NotNil(t, softwareInstallers[0].TitleID)
require.NotNil(t, softwareInstallers[0].TeamID)
require.Empty(t, softwareInstallers[0].URL)
assertSoftware([]fleet.SoftwareTitle{
{Name: ins1, Source: "apps", Browser: ""},
})
Expand Down
2 changes: 1 addition & 1 deletion server/fleet/datastore.go
Original file line number Diff line number Diff line change
Expand Up @@ -1711,7 +1711,7 @@ type Datastore interface {
CleanupUnusedSoftwareInstallers(ctx context.Context, softwareInstallStore SoftwareInstallerStore, removeCreatedBefore time.Time) error

// BatchSetSoftwareInstallers sets the software installers for the given team or no team.
BatchSetSoftwareInstallers(ctx context.Context, tmID *uint, installers []*UploadSoftwareInstallerPayload) ([]SoftwareInstaller, error)
BatchSetSoftwareInstallers(ctx context.Context, tmID *uint, installers []*UploadSoftwareInstallerPayload) ([]SoftwarePackageResponse, error)

// HasSelfServiceSoftwareInstallers returns true if self-service software installers are available for the team or globally.
HasSelfServiceSoftwareInstallers(ctx context.Context, platform string, teamID *uint) (bool, error)
Expand Down
2 changes: 1 addition & 1 deletion server/fleet/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,7 @@ type Service interface {

// BatchSetSoftwareInstallers replaces the software installers for a specified team.
// Returns the metadata of inserted software installers.
BatchSetSoftwareInstallers(ctx context.Context, tmName string, payloads []SoftwareInstallerPayload, dryRun bool) ([]SoftwareInstaller, error)
BatchSetSoftwareInstallers(ctx context.Context, tmName string, payloads []SoftwareInstallerPayload, dryRun bool) ([]SoftwarePackageResponse, error)

// SelfServiceInstallSoftwareTitle installs a software title
// initiated by the user
Expand Down
11 changes: 11 additions & 0 deletions server/fleet/software_installer.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,17 @@ type SoftwareInstaller struct {
URL string `json:"url" db:"url"`
}

// SoftwarePackageResponse is the response type used when applying software by batch.
type SoftwarePackageResponse struct {
// TeamID is the ID of the team.
// A value of nil means it is scoped to hosts that are assigned to "No team".
TeamID *uint `json:"team_id" db:"team_id"`
// TitleID is the id of the software title associated with the software installer.
TitleID *uint `json:"title_id" db:"title_id"`
// URL is the source URL for this installer (set when uploading via batch/gitops).
URL string `json:"url" db:"url"`
}

// AuthzType implements authz.AuthzTyper.
func (s *SoftwareInstaller) AuthzType() string {
return "installable_entity"
Expand Down
4 changes: 2 additions & 2 deletions server/mock/datastore_mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -1070,7 +1070,7 @@ type GetSoftwareInstallResultsFunc func(ctx context.Context, resultsUUID string)

type CleanupUnusedSoftwareInstallersFunc func(ctx context.Context, softwareInstallStore fleet.SoftwareInstallerStore, removeCreatedBefore time.Time) error

type BatchSetSoftwareInstallersFunc func(ctx context.Context, tmID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwareInstaller, error)
type BatchSetSoftwareInstallersFunc func(ctx context.Context, tmID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwarePackageResponse, error)

type HasSelfServiceSoftwareInstallersFunc func(ctx context.Context, platform string, teamID *uint) (bool, error)

Expand Down Expand Up @@ -6369,7 +6369,7 @@ func (s *DataStore) CleanupUnusedSoftwareInstallers(ctx context.Context, softwar
return s.CleanupUnusedSoftwareInstallersFunc(ctx, softwareInstallStore, removeCreatedBefore)
}

func (s *DataStore) BatchSetSoftwareInstallers(ctx context.Context, tmID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwareInstaller, error) {
func (s *DataStore) BatchSetSoftwareInstallers(ctx context.Context, tmID *uint, installers []*fleet.UploadSoftwareInstallerPayload) ([]fleet.SoftwarePackageResponse, error) {
s.mu.Lock()
s.BatchSetSoftwareInstallersFuncInvoked = true
s.mu.Unlock()
Expand Down
Loading

0 comments on commit 2d05f24

Please sign in to comment.