From f3d7ed86a8065f820a90cea62506eaef26ccb133 Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Mon, 31 Mar 2025 11:42:43 -0400 Subject: [PATCH] Bugfix: support removing labels associated with profiles (custom settings) in gitops (#27546) --- ...04-remove-config-profile-labels-via-gitops | 1 + .../gitops_enterprise_integration_test.go | 303 +++++++++++++++++- server/datastore/mysql/apple_mdm.go | 72 ++++- server/datastore/mysql/apple_mdm_test.go | 35 +- server/datastore/mysql/mdm.go | 34 +- server/datastore/mysql/mdm_test.go | 91 +++++- server/datastore/mysql/microsoft_mdm.go | 16 +- server/test/mdm.go | 22 ++ 8 files changed, 536 insertions(+), 38 deletions(-) create mode 100644 changes/27404-remove-config-profile-labels-via-gitops diff --git a/changes/27404-remove-config-profile-labels-via-gitops b/changes/27404-remove-config-profile-labels-via-gitops new file mode 100644 index 0000000000..61b0a584bd --- /dev/null +++ b/changes/27404-remove-config-profile-labels-via-gitops @@ -0,0 +1 @@ +* Fixed an issue where removing label conditions on configuration profiles (e.g. `labels_include_any`, `labels_include_all` or `labels_exclude_any`) did not clear the labels associated with the profile when applied via `fleetctl gitops`. diff --git a/cmd/fleetctl/gitops_enterprise_integration_test.go b/cmd/fleetctl/gitops_enterprise_integration_test.go index 57bae116fe..72faca1aa5 100644 --- a/cmd/fleetctl/gitops_enterprise_integration_test.go +++ b/cmd/fleetctl/gitops_enterprise_integration_test.go @@ -121,6 +121,37 @@ func (s *enterpriseIntegrationGitopsTestSuite) TearDownSuite() { require.NoError(s.T(), err) } +func (s *enterpriseIntegrationGitopsTestSuite) TearDownTest() { + t := s.T() + ctx := context.Background() + + teams, err := s.ds.ListTeams(ctx, fleet.TeamFilter{User: test.UserAdmin}, fleet.ListOptions{}) + require.NoError(t, err) + for _, tm := range teams { + err := s.ds.DeleteTeam(ctx, tm.ID) + require.NoError(t, err) + } + + // Clean software installers in "No team" (the others are deleted in ts.ds.DeleteTeam above). + mysql.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { + _, err := q.ExecContext(ctx, `DELETE FROM software_installers WHERE global_or_team_id = 0;`) + return err + }) + mysql.ExecAdhocSQL(t, s.ds, func(tx sqlx.ExtContext) error { + _, err := tx.ExecContext(ctx, "DELETE FROM vpp_apps;") + return err + }) + + lbls, err := s.ds.ListLabels(ctx, fleet.TeamFilter{User: test.UserAdmin}, fleet.ListOptions{}) + require.NoError(t, err) + for _, lbl := range lbls { + if lbl.LabelType != fleet.LabelTypeBuiltIn { + err := s.ds.DeleteLabel(ctx, lbl.Name) + require.NoError(t, err) + } + } +} + // TestFleetGitops runs `fleetctl gitops` command on configs in https://github.com/fleetdm/fleet-gitops repo. // Changes to that repo may cause this test to fail. func (s *enterpriseIntegrationGitopsTestSuite) TestFleetGitops() { @@ -448,7 +479,7 @@ org_settings: custom_scep_proxy: - name: CustomScepProxy url: %s - challenge: challenge + challenge: challenge policies: queries: `, dirPath, digiCertServer.URL, scepServer.URL+"/scep")) @@ -513,3 +544,273 @@ queries: assert.Empty(t, appConfig.Integrations.CustomSCEPProxy.Value) } + +// TestUnsetConfigurationProfileLabels tests the removal of labels associated with a +// configuration profile via gitops. +func (s *enterpriseIntegrationGitopsTestSuite) TestUnsetConfigurationProfileLabels() { + t := s.T() + ctx := context.Background() + + user := s.createGitOpsUser(t) + fleetctlConfig := s.createFleetctlConfig(t, user) + lbl, err := s.ds.NewLabel(ctx, &fleet.Label{Name: "Label1", Query: "SELECT 1"}) + require.NoError(t, err) + require.NotZero(t, lbl.ID) + + profileFile, err := os.CreateTemp(t.TempDir(), "*.mobileconfig") + require.NoError(t, err) + _, err = profileFile.WriteString(test.GenerateMDMAppleProfile("test", "test", uuid.NewString())) + require.NoError(t, err) + err = profileFile.Close() + require.NoError(t, err) + + const ( + globalTemplate = ` +agent_options: +controls: + macos_settings: + custom_settings: + - path: %s +%s +org_settings: + server_settings: + server_url: $FLEET_URL + org_info: + org_name: Fleet + secrets: +policies: +queries: +` + withLabelsIncludeAny = ` + labels_include_any: + - Label1 +` + emptyLabelsIncludeAny = ` + labels_include_any: +` + teamTemplate = ` +controls: + macos_settings: + custom_settings: + - path: %s +%s +software: +queries: +policies: +agent_options: +name: %s +team_settings: + secrets: [{"secret":"enroll_secret"}] +` + withLabelsIncludeAll = ` + labels_include_all: + - Label1 +` + ) + + globalFile, err := os.CreateTemp(t.TempDir(), "*.yml") + require.NoError(t, err) + _, err = globalFile.WriteString(fmt.Sprintf(globalTemplate, profileFile.Name(), withLabelsIncludeAny)) + require.NoError(t, err) + err = globalFile.Close() + require.NoError(t, err) + + teamName := uuid.NewString() + teamFile, err := os.CreateTemp(t.TempDir(), "*.yml") + require.NoError(t, err) + _, err = teamFile.WriteString(fmt.Sprintf(teamTemplate, profileFile.Name(), withLabelsIncludeAll, teamName)) + require.NoError(t, err) + err = teamFile.Close() + require.NoError(t, err) + + // Set the required environment variables + t.Setenv("FLEET_URL", s.server.URL) + + // Apply configs + _ = runAppForTest(t, []string{"gitops", "--config", fleetctlConfig.Name(), "-f", globalFile.Name(), "-f", teamFile.Name(), "--dry-run"}) + _ = runAppForTest(t, []string{"gitops", "--config", fleetctlConfig.Name(), "-f", globalFile.Name(), "-f", teamFile.Name()}) + + // get the team ID + team, err := s.ds.TeamByName(ctx, teamName) + require.NoError(t, err) + + // the custom setting is scoped by the label for no team + profs, _, err := s.ds.ListMDMConfigProfiles(ctx, nil, fleet.ListOptions{}) + require.NoError(t, err) + require.Len(t, profs, 1) + require.Len(t, profs[0].LabelsIncludeAny, 1) + require.Equal(t, "Label1", profs[0].LabelsIncludeAny[0].LabelName) + + // the custom setting is scoped by the label for team + profs, _, err = s.ds.ListMDMConfigProfiles(ctx, &team.ID, fleet.ListOptions{}) + require.NoError(t, err) + require.Len(t, profs, 1) + require.Len(t, profs[0].LabelsIncludeAll, 1) + require.Equal(t, "Label1", profs[0].LabelsIncludeAll[0].LabelName) + + // remove the label conditions + err = os.WriteFile(globalFile.Name(), []byte(fmt.Sprintf(globalTemplate, profileFile.Name(), emptyLabelsIncludeAny)), 0o644) + require.NoError(t, err) + err = os.WriteFile(teamFile.Name(), []byte(fmt.Sprintf(teamTemplate, profileFile.Name(), "", teamName)), 0o644) + require.NoError(t, err) + + // Apply configs + _ = runAppForTest(t, []string{"gitops", "--config", fleetctlConfig.Name(), "-f", globalFile.Name(), "-f", teamFile.Name(), "--dry-run"}) + _ = runAppForTest(t, []string{"gitops", "--config", fleetctlConfig.Name(), "-f", globalFile.Name(), "-f", teamFile.Name()}) + + // the custom setting is not scoped by label anymore + profs, _, err = s.ds.ListMDMConfigProfiles(ctx, nil, fleet.ListOptions{}) + require.NoError(t, err) + require.Len(t, profs, 1) + require.Len(t, profs[0].LabelsIncludeAny, 0) + + profs, _, err = s.ds.ListMDMConfigProfiles(ctx, &team.ID, fleet.ListOptions{}) + require.NoError(t, err) + require.Len(t, profs, 1) + require.Len(t, profs[0].LabelsIncludeAll, 0) +} + +// TestUnsetSoftwareInstallerLabels tests the removal of labels associated with a +// software installer via gitops. +func (s *enterpriseIntegrationGitopsTestSuite) TestUnsetSoftwareInstallerLabels() { + t := s.T() + ctx := context.Background() + + user := s.createGitOpsUser(t) + fleetctlConfig := s.createFleetctlConfig(t, user) + lbl, err := s.ds.NewLabel(ctx, &fleet.Label{Name: "Label1", Query: "SELECT 1"}) + require.NoError(t, err) + require.NotZero(t, lbl.ID) + + const ( + globalTemplate = ` +agent_options: +controls: +org_settings: + server_settings: + server_url: $FLEET_URL + org_info: + org_name: Fleet + secrets: +policies: +queries: +` + + noTeamTemplate = `name: No team +controls: +policies: +software: + packages: + - url: ${SOFTWARE_INSTALLER_URL}/ruby.deb +%s +` + withLabelsIncludeAny = ` + labels_include_any: + - Label1 +` + emptyLabelsIncludeAny = ` + labels_include_any: +` + teamTemplate = ` +controls: +software: + packages: + - url: ${SOFTWARE_INSTALLER_URL}/ruby.deb +%s +queries: +policies: +agent_options: +name: %s +team_settings: + secrets: [{"secret":"enroll_secret"}] +` + withLabelsExcludeAny = ` + labels_exclude_any: + - Label1 +` + ) + + globalFile, err := os.CreateTemp(t.TempDir(), "*.yml") + require.NoError(t, err) + _, err = globalFile.WriteString(globalTemplate) + require.NoError(t, err) + err = globalFile.Close() + require.NoError(t, err) + + noTeamFile, err := os.CreateTemp(t.TempDir(), "*.yml") + require.NoError(t, err) + _, err = noTeamFile.WriteString(fmt.Sprintf(noTeamTemplate, withLabelsIncludeAny)) + require.NoError(t, err) + err = noTeamFile.Close() + require.NoError(t, err) + noTeamFilePath := filepath.Join(filepath.Dir(noTeamFile.Name()), "no-team.yml") + err = os.Rename(noTeamFile.Name(), noTeamFilePath) + require.NoError(t, err) + + teamName := uuid.NewString() + teamFile, err := os.CreateTemp(t.TempDir(), "*.yml") + require.NoError(t, err) + _, err = teamFile.WriteString(fmt.Sprintf(teamTemplate, withLabelsExcludeAny, teamName)) + require.NoError(t, err) + err = teamFile.Close() + require.NoError(t, err) + + // Set the required environment variables + t.Setenv("FLEET_URL", s.server.URL) + startSoftwareInstallerServer(t) + + // Apply configs + _ = runAppForTest(t, []string{"gitops", "--config", fleetctlConfig.Name(), "-f", globalFile.Name(), "-f", noTeamFilePath, "-f", teamFile.Name(), "--dry-run"}) + _ = runAppForTest(t, []string{"gitops", "--config", fleetctlConfig.Name(), "-f", globalFile.Name(), "-f", noTeamFilePath, "-f", teamFile.Name()}) + + // get the team ID + team, err := s.ds.TeamByName(ctx, teamName) + require.NoError(t, err) + + // the installer is scoped by the label for no team + titles, _, _, err := s.ds.ListSoftwareTitles(ctx, fleet.SoftwareTitleListOptions{AvailableForInstall: true, TeamID: ptr.Uint(0)}, fleet.TeamFilter{User: test.UserAdmin}) + require.NoError(t, err) + require.Len(t, titles, 1) + require.NotNil(t, titles[0].SoftwarePackage) + noTeamTitleID := titles[0].ID + meta, err := s.ds.GetSoftwareInstallerMetadataByTeamAndTitleID(ctx, nil, noTeamTitleID, false) + require.NoError(t, err) + require.Len(t, meta.LabelsIncludeAny, 1) + require.Equal(t, "Label1", meta.LabelsIncludeAny[0].LabelName) + + // the installer is scoped by the label for team + titles, _, _, err = s.ds.ListSoftwareTitles(ctx, fleet.SoftwareTitleListOptions{TeamID: &team.ID}, fleet.TeamFilter{User: test.UserAdmin}) + require.NoError(t, err) + require.Len(t, titles, 1) + require.NotNil(t, titles[0].SoftwarePackage) + teamTitleID := titles[0].ID + meta, err = s.ds.GetSoftwareInstallerMetadataByTeamAndTitleID(ctx, &team.ID, teamTitleID, false) + require.NoError(t, err) + require.Len(t, meta.LabelsExcludeAny, 1) + require.Equal(t, "Label1", meta.LabelsExcludeAny[0].LabelName) + + // remove the label conditions + err = os.WriteFile(noTeamFilePath, []byte(fmt.Sprintf(noTeamTemplate, emptyLabelsIncludeAny)), 0o644) + require.NoError(t, err) + err = os.WriteFile(teamFile.Name(), []byte(fmt.Sprintf(teamTemplate, "", teamName)), 0o644) + require.NoError(t, err) + + // Apply configs + _ = runAppForTest(t, []string{"gitops", "--config", fleetctlConfig.Name(), "-f", globalFile.Name(), "-f", noTeamFilePath, "-f", teamFile.Name(), "--dry-run"}) + _ = runAppForTest(t, []string{"gitops", "--config", fleetctlConfig.Name(), "-f", globalFile.Name(), "-f", noTeamFilePath, "-f", teamFile.Name()}) + + // the installer is not scoped by label anymore + meta, err = s.ds.GetSoftwareInstallerMetadataByTeamAndTitleID(ctx, nil, noTeamTitleID, false) + require.NoError(t, err) + require.NotNil(t, meta.TitleID) + require.Equal(t, noTeamTitleID, *meta.TitleID) + require.Len(t, meta.LabelsExcludeAny, 0) + require.Len(t, meta.LabelsIncludeAny, 0) + + meta, err = s.ds.GetSoftwareInstallerMetadataByTeamAndTitleID(ctx, &team.ID, teamTitleID, false) + require.NoError(t, err) + require.NotNil(t, meta.TitleID) + require.Equal(t, teamTitleID, *meta.TitleID) + require.Len(t, meta.LabelsExcludeAny, 0) + require.Len(t, meta.LabelsIncludeAny, 0) +} diff --git a/server/datastore/mysql/apple_mdm.go b/server/datastore/mysql/apple_mdm.go index e465a9e519..083341e29e 100644 --- a/server/datastore/mysql/apple_mdm.go +++ b/server/datastore/mysql/apple_mdm.go @@ -99,7 +99,11 @@ INSERT INTO cp.LabelsExcludeAny[i].RequireAll = false labels = append(labels, cp.LabelsExcludeAny[i]) } - if _, err := batchSetProfileLabelAssociationsDB(ctx, tx, labels, "darwin"); err != nil { + var profWithoutLabels []string + if len(labels) == 0 { + profWithoutLabels = append(profWithoutLabels, profUUID) + } + if _, err := batchSetProfileLabelAssociationsDB(ctx, tx, labels, profWithoutLabels, "darwin"); err != nil { return ctxerr.Wrap(ctx, err, "inserting darwin profile label associations") } @@ -1845,6 +1849,7 @@ ON DUPLICATE KEY UPDATE // between macOS and Windows, but at the time of this // implementation we're under tight time constraints. incomingLabels := []fleet.ConfigurationProfileLabel{} + var profsWithoutLabels []string if len(incomingIdents) > 0 { var newlyInsertedProfs []*fleet.MDMAppleConfigProfile // load current profiles (again) that match the incoming profiles by name to grab their uuids @@ -1868,33 +1873,37 @@ ON DUPLICATE KEY UPDATE return false, ctxerr.Wrapf(ctx, err, "profile %q is in the database but was not incoming", newlyInsertedProf.Identifier) } + var profHasLabel bool for _, label := range incomingProf.LabelsIncludeAll { label.ProfileUUID = newlyInsertedProf.ProfileUUID label.Exclude = false label.RequireAll = true incomingLabels = append(incomingLabels, label) + profHasLabel = true } for _, label := range incomingProf.LabelsIncludeAny { label.ProfileUUID = newlyInsertedProf.ProfileUUID label.Exclude = false label.RequireAll = false incomingLabels = append(incomingLabels, label) + profHasLabel = true } for _, label := range incomingProf.LabelsExcludeAny { label.ProfileUUID = newlyInsertedProf.ProfileUUID label.Exclude = true label.RequireAll = false incomingLabels = append(incomingLabels, label) + profHasLabel = true + } + if !profHasLabel { + profsWithoutLabels = append(profsWithoutLabels, newlyInsertedProf.ProfileUUID) } } } - // FIXME: At what point are we deleting label associations for existing profiles (e.g. if the user - // removes all labels from a profile in gitops, shouldn't we remove the old associations)? - // insert label associations var updatedLabels bool - if updatedLabels, err = batchSetProfileLabelAssociationsDB(ctx, tx, incomingLabels, + if updatedLabels, err = batchSetProfileLabelAssociationsDB(ctx, tx, incomingLabels, profsWithoutLabels, "darwin"); err != nil || strings.HasPrefix(ds.testBatchSetMDMAppleProfilesErr, "labels") { if err == nil { err = errors.New(ds.testBatchSetMDMAppleProfilesErr) @@ -4257,6 +4266,7 @@ func (ds *Datastore) batchSetMDMAppleDeclarations(ctx context.Context, tx sqlx.E func (ds *Datastore) updateDeclarationsLabelAssociations(ctx context.Context, tx sqlx.ExtContext, incomingDeclarationsMap map[string]*fleet.MDMAppleDeclaration, teamID uint) (updatedDB bool, err error) { var incomingLabels []fleet.ConfigurationProfileLabel + var declWithoutLabels []string if len(incomingDeclarationsMap) > 0 { incomingNames := make([]string, 0, len(incomingDeclarationsMap)) for _, p := range incomingDeclarationsMap { @@ -4282,29 +4292,36 @@ func (ds *Datastore) updateDeclarationsLabelAssociations(ctx context.Context, tx newlyInsertedDecl.Name) } + var declHasLabel bool for _, label := range incomingDecl.LabelsIncludeAll { label.ProfileUUID = newlyInsertedDecl.DeclarationUUID label.Exclude = false label.RequireAll = true incomingLabels = append(incomingLabels, label) + declHasLabel = true } for _, label := range incomingDecl.LabelsIncludeAny { label.ProfileUUID = newlyInsertedDecl.DeclarationUUID label.Exclude = false label.RequireAll = false incomingLabels = append(incomingLabels, label) + declHasLabel = true } for _, label := range incomingDecl.LabelsExcludeAny { label.ProfileUUID = newlyInsertedDecl.DeclarationUUID label.Exclude = true label.RequireAll = false incomingLabels = append(incomingLabels, label) + declHasLabel = true + } + if !declHasLabel { + declWithoutLabels = append(declWithoutLabels, newlyInsertedDecl.DeclarationUUID) } } } if updatedDB, err = batchSetDeclarationLabelAssociationsDB(ctx, tx, - incomingLabels); err != nil || strings.HasPrefix(ds.testBatchSetMDMAppleProfilesErr, "labels") { + incomingLabels, declWithoutLabels); err != nil || strings.HasPrefix(ds.testBatchSetMDMAppleProfilesErr, "labels") { if err == nil { err = errors.New(ds.testBatchSetMDMAppleProfilesErr) } @@ -4531,7 +4548,11 @@ func (ds *Datastore) insertOrUpsertMDMAppleDeclaration(ctx context.Context, insO declaration.LabelsExcludeAny[i].RequireAll = false labels = append(labels, declaration.LabelsExcludeAny[i]) } - if _, err := batchSetDeclarationLabelAssociationsDB(ctx, tx, labels); err != nil { + var declWithoutLabels []string + if len(labels) == 0 { + declWithoutLabels = []string{declUUID} + } + if _, err := batchSetDeclarationLabelAssociationsDB(ctx, tx, labels, declWithoutLabels); err != nil { return ctxerr.Wrap(ctx, err, "inserting mdm declaration label associations") } @@ -4546,21 +4567,28 @@ func (ds *Datastore) insertOrUpsertMDMAppleDeclaration(ctx context.Context, insO } func batchSetDeclarationLabelAssociationsDB(ctx context.Context, tx sqlx.ExtContext, - declarationLabels []fleet.ConfigurationProfileLabel, + declarationLabels []fleet.ConfigurationProfileLabel, declUUIDsWithoutLabels []string, ) (updatedDB bool, err error) { - if len(declarationLabels) == 0 { + if len(declarationLabels)+len(declUUIDsWithoutLabels) == 0 { return false, nil } - // delete any profile+label tuple that is NOT in the list of provided tuples - // but are associated with the provided profiles (so we don't delete - // unrelated profile+label tuples) + // delete any decl+label tuple that is NOT in the list of provided tuples + // but are associated with the provided declarations (so we don't delete + // unrelated decl+label tuples) deleteStmt := ` DELETE FROM mdm_declaration_labels WHERE (apple_declaration_uuid, label_id) NOT IN (%s) AND apple_declaration_uuid IN (?) ` + // used when only declUUIDsWithoutLabels is provided, there are no + // labels to keep, delete all labels for declarations in this list. + deleteNoLabelStmt := ` + DELETE FROM mdm_declaration_labels + WHERE apple_declaration_uuid IN (?) + ` + upsertStmt := ` INSERT INTO mdm_declaration_labels (apple_declaration_uuid, label_id, label_name, exclude, require_all) @@ -4577,6 +4605,23 @@ func batchSetDeclarationLabelAssociationsDB(ctx context.Context, tx sqlx.ExtCont WHERE (apple_declaration_uuid, label_name) IN (%s) ` + if len(declarationLabels) == 0 { + deleteNoLabelStmt, args, err := sqlx.In(deleteNoLabelStmt, declUUIDsWithoutLabels) + if err != nil { + return false, ctxerr.Wrap(ctx, err, "sqlx.In delete labels for declarations without labels") + } + + var result sql.Result + if result, err = tx.ExecContext(ctx, deleteNoLabelStmt, args...); err != nil { + return false, ctxerr.Wrap(ctx, err, "deleting labels for declarations without labels") + } + if result != nil { + rows, _ := result.RowsAffected() + updatedDB = rows > 0 + } + return updatedDB, nil + } + var ( insertBuilder strings.Builder selectOrDeleteBuilder strings.Builder @@ -4639,10 +4684,11 @@ func batchSetDeclarationLabelAssociationsDB(ctx context.Context, tx sqlx.ExtCont deleteStmt = fmt.Sprintf(deleteStmt, selectOrDeleteBuilder.String()) - profUUIDs := make([]string, 0, len(setProfileUUIDs)) + profUUIDs := make([]string, 0, len(setProfileUUIDs)+len(declUUIDsWithoutLabels)) for k := range setProfileUUIDs { profUUIDs = append(profUUIDs, k) } + profUUIDs = append(profUUIDs, declUUIDsWithoutLabels...) deleteArgs := deleteParams deleteArgs = append(deleteArgs, profUUIDs) diff --git a/server/datastore/mysql/apple_mdm_test.go b/server/datastore/mysql/apple_mdm_test.go index d18463ee3f..8ae089b909 100644 --- a/server/datastore/mysql/apple_mdm_test.go +++ b/server/datastore/mysql/apple_mdm_test.go @@ -1082,14 +1082,21 @@ func expectAppleDeclarations( tmID = ptr.Uint(0) } - var got []*fleet.MDMAppleDeclaration + ctx := context.Background() + var gotUUIDs []string ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { - ctx := context.Background() - return sqlx.SelectContext(ctx, q, &got, - `SELECT declaration_uuid, team_id, identifier, name, raw_json, token, created_at, uploaded_at FROM mdm_apple_declarations WHERE team_id = ?`, + return sqlx.SelectContext(ctx, q, &gotUUIDs, + `SELECT declaration_uuid FROM mdm_apple_declarations WHERE team_id = ?`, tmID) }) + // load each declaration, this will also load its labels + var got []*fleet.MDMAppleDeclaration + for _, declUUID := range gotUUIDs { + decl, err := ds.GetMDMAppleDeclaration(ctx, declUUID) + require.NoError(t, err) + got = append(got, decl) + } // create map of expected declarations keyed by identifier wantMap := make(map[string]*fleet.MDMAppleDeclaration, len(want)) for _, cp := range want { @@ -1105,6 +1112,12 @@ func expectAppleDeclarations( return json.Marshal(ifce) } + jsonMustMarshal := func(v any) string { + b, err := json.Marshal(v) + require.NoError(t, err) + return string(b) + } + // compare only the fields we care about, and build the resulting map of // declaration identifier as key to declaration UUID as value m := make(map[string]string) @@ -1143,7 +1156,12 @@ func expectAppleDeclarations( require.Equal(t, wantD.Name, gotD.Name) require.Equal(t, wantD.Identifier, gotD.Identifier) - require.Equal(t, wantD.LabelsIncludeAll, gotD.LabelsIncludeAll) + + // for labels, only care about ID and Name (the exclude, require all + // fields, etc. are reflected by the field that contains the label) + require.Equal(t, jsonMustMarshal(wantD.LabelsIncludeAll), jsonMustMarshal(gotD.LabelsIncludeAll)) + require.Equal(t, jsonMustMarshal(wantD.LabelsIncludeAny), jsonMustMarshal(gotD.LabelsIncludeAny)) + require.Equal(t, jsonMustMarshal(wantD.LabelsExcludeAny), jsonMustMarshal(gotD.LabelsExcludeAny)) } return m } @@ -1355,9 +1373,12 @@ func declForTest(name, identifier, payloadContent string, labels ...*fleet.Label } for _, l := range labels { - if strings.HasPrefix(l.Name, "exclude-") { + switch { + case strings.HasPrefix(l.Name, "exclude-"): decl.LabelsExcludeAny = append(decl.LabelsExcludeAny, fleet.ConfigurationProfileLabel{LabelName: l.Name, LabelID: l.ID}) - } else { + case strings.HasPrefix(l.Name, "inclany-"): + decl.LabelsIncludeAny = append(decl.LabelsIncludeAny, fleet.ConfigurationProfileLabel{LabelName: l.Name, LabelID: l.ID}) + default: decl.LabelsIncludeAll = append(decl.LabelsIncludeAll, fleet.ConfigurationProfileLabel{LabelName: l.Name, LabelID: l.ID}) } } diff --git a/server/datastore/mysql/mdm.go b/server/datastore/mysql/mdm.go index 3d8e646230..7dec6753ef 100644 --- a/server/datastore/mysql/mdm.go +++ b/server/datastore/mysql/mdm.go @@ -1018,11 +1018,10 @@ func batchSetProfileLabelAssociationsDB( ctx context.Context, tx sqlx.ExtContext, profileLabels []fleet.ConfigurationProfileLabel, + profileUUIDsWithoutLabels []string, platform string, ) (updatedDB bool, err error) { - if len(profileLabels) == 0 { - // FIXME: At what point are we deleting all labels for a profile (e.g., the user might - // remove all labels from an existing profile)? + if len(profileLabels)+len(profileUUIDsWithoutLabels) == 0 { return false, nil } @@ -1050,6 +1049,13 @@ func batchSetProfileLabelAssociationsDB( %s_profile_uuid IN (?) ` + // used when only profileUUIDsWithoutLabels is provided, there are no + // labels to keep, delete all labels for profiles in this list. + deleteNoLabelStmt := ` + DELETE FROM mdm_configuration_profile_labels + WHERE %s_profile_uuid IN (?) + ` + upsertStmt := ` INSERT INTO mdm_configuration_profile_labels (%s_profile_uuid, label_id, label_name, exclude, require_all) @@ -1066,6 +1072,24 @@ func batchSetProfileLabelAssociationsDB( WHERE (%s_profile_uuid, label_name) IN (%s) ` + if len(profileLabels) == 0 { + deleteNoLabelStmt = fmt.Sprintf(deleteNoLabelStmt, platformPrefix) + deleteNoLabelStmt, args, err := sqlx.In(deleteNoLabelStmt, profileUUIDsWithoutLabels) + if err != nil { + return false, ctxerr.Wrap(ctx, err, "sqlx.In delete labels for profiles without labels") + } + + var result sql.Result + if result, err = tx.ExecContext(ctx, deleteNoLabelStmt, args...); err != nil { + return false, ctxerr.Wrap(ctx, err, "deleting labels for profiles without labels") + } + if result != nil { + rows, _ := result.RowsAffected() + updatedDB = rows > 0 + } + return updatedDB, nil + } + var ( insertBuilder strings.Builder selectOrDeleteBuilder strings.Builder @@ -1075,6 +1099,7 @@ func batchSetProfileLabelAssociationsDB( setProfileUUIDs = make(map[string]struct{}) ) + labelsToInsert := make(map[string]*fleet.ConfigurationProfileLabel, len(profileLabels)) for i, pl := range profileLabels { labelsToInsert[fmt.Sprintf("%s\n%s", pl.ProfileUUID, pl.LabelName)] = &profileLabels[i] @@ -1128,10 +1153,11 @@ func batchSetProfileLabelAssociationsDB( deleteStmt = fmt.Sprintf(deleteStmt, platformPrefix, selectOrDeleteBuilder.String(), platformPrefix) - profUUIDs := make([]string, 0, len(setProfileUUIDs)) + profUUIDs := make([]string, 0, len(setProfileUUIDs)+len(profileUUIDsWithoutLabels)) for k := range setProfileUUIDs { profUUIDs = append(profUUIDs, k) } + profUUIDs = append(profUUIDs, profileUUIDsWithoutLabels...) deleteArgs := deleteParams deleteArgs = append(deleteArgs, profUUIDs) diff --git a/server/datastore/mysql/mdm_test.go b/server/datastore/mysql/mdm_test.go index 56e7844fc7..589d702fdf 100644 --- a/server/datastore/mysql/mdm_test.go +++ b/server/datastore/mysql/mdm_test.go @@ -351,6 +351,8 @@ func testMDMCommands(t *testing.T, ds *Datastore) { } func testBatchSetMDMProfiles(t *testing.T, ds *Datastore) { + ctx := context.Background() + applyAndExpect := func( newAppleSet []*fleet.MDMAppleConfigProfile, newWindowsSet []*fleet.MDMWindowsConfigProfile, @@ -361,7 +363,6 @@ func testBatchSetMDMProfiles(t *testing.T, ds *Datastore) { wantAppleDecl []*fleet.MDMAppleDeclaration, wantUpdates fleet.MDMProfilesUpdates, ) { - ctx := context.Background() updates, err := ds.BatchSetMDMProfiles(ctx, tmID, newAppleSet, newWindowsSet, newAppleDeclSet) require.NoError(t, err) expectAppleProfiles(t, ds, tmID, wantApple) @@ -546,6 +547,53 @@ func testBatchSetMDMProfiles(t *testing.T, ds *Datastore) { applyAndExpect(nil, nil, nil, ptr.Uint(1), nil, nil, nil, fleet.MDMProfilesUpdates{AppleConfigProfile: true, WindowsConfigProfile: true, AppleDeclaration: true}, ) + + // create some labels to test batch-setting label-scoped declarations + lblExcl, err := ds.NewLabel(ctx, &fleet.Label{Name: "exclude-label-1", Query: "select 1"}) + require.NoError(t, err) + lblExcl2, err := ds.NewLabel(ctx, &fleet.Label{Name: "exclude-label-2", Query: "select 2"}) + require.NoError(t, err) + lblInclAny, err := ds.NewLabel(ctx, &fleet.Label{Name: "include-label-3", Query: "select 3"}) + require.NoError(t, err) + lblInclAny2, err := ds.NewLabel(ctx, &fleet.Label{Name: "include-label-4", Query: "select 4"}) + require.NoError(t, err) + lblInclAll, err := ds.NewLabel(ctx, &fleet.Label{Name: "inclall-label-5", Query: "select 5"}) + require.NoError(t, err) + lblInclAll2, err := ds.NewLabel(ctx, &fleet.Label{Name: "inclall-label-6", Query: "select 6"}) + require.NoError(t, err) + + // we only care about declarations here, as batch-setting labels for profiles + // is tested elsewhere. + applyAndExpect(nil, nil, []*fleet.MDMAppleDeclaration{ + declForTest("D1", "D1", "foo", lblExcl, lblExcl2), + declForTest("D2", "D2", "foo", lblInclAll, lblInclAll2), + }, nil, + nil, nil, []*fleet.MDMAppleDeclaration{ + declForTest("D1", "D1", "foo", lblExcl, lblExcl2), + declForTest("D2", "D2", "foo", lblInclAll, lblInclAll2), + }, + // this removed the apple and windows profiles for no team, so updated is true + fleet.MDMProfilesUpdates{AppleConfigProfile: true, WindowsConfigProfile: true, AppleDeclaration: true}, + ) + + applyAndExpect(nil, nil, []*fleet.MDMAppleDeclaration{ + declForTest("D1", "D1", "foo", lblInclAny, lblInclAny2), + declForTest("D2", "D2", "foo"), + }, nil, + nil, nil, []*fleet.MDMAppleDeclaration{ + declForTest("D1", "D1", "foo", lblInclAny, lblInclAny2), + declForTest("D2", "D2", "foo"), + }, + fleet.MDMProfilesUpdates{AppleConfigProfile: false, WindowsConfigProfile: false, AppleDeclaration: true}, + ) + applyAndExpect(nil, nil, []*fleet.MDMAppleDeclaration{ + declForTest("D1", "D1", "foo"), + }, nil, + nil, nil, []*fleet.MDMAppleDeclaration{ + declForTest("D1", "D1", "foo"), + }, + fleet.MDMProfilesUpdates{AppleConfigProfile: false, WindowsConfigProfile: false, AppleDeclaration: true}, + ) } func testListMDMConfigProfiles(t *testing.T, ds *Datastore) { @@ -6125,14 +6173,14 @@ func testBatchSetProfileLabelAssociations(t *testing.T, ds *Datastore) { wantOtherWin := []fleet.ConfigurationProfileLabel{ {ProfileUUID: otherWinProfile.ProfileUUID, LabelName: label.Name, LabelID: label.ID}, } - updatedDB, err := batchSetProfileLabelAssociationsDB(ctx, ds.writer(ctx), wantOtherWin, "windows") + updatedDB, err := batchSetProfileLabelAssociationsDB(ctx, ds.writer(ctx), wantOtherWin, []string{windowsProfile.ProfileUUID}, "windows") require.NoError(t, err) assert.True(t, updatedDB) // make it an "exclude" label on the other macos profile wantOtherMac := []fleet.ConfigurationProfileLabel{ {ProfileUUID: otherMacProfile.ProfileUUID, LabelName: label.Name, LabelID: label.ID, Exclude: true}, } - updatedDB, err = batchSetProfileLabelAssociationsDB(ctx, ds.writer(ctx), wantOtherMac, "darwin") + updatedDB, err = batchSetProfileLabelAssociationsDB(ctx, ds.writer(ctx), wantOtherMac, []string{macOSProfile.ProfileUUID}, "darwin") require.NoError(t, err) assert.True(t, updatedDB) @@ -6167,7 +6215,7 @@ func testBatchSetProfileLabelAssociations(t *testing.T, ds *Datastore) { t.Run("empty input "+platform, func(t *testing.T) { want := []fleet.ConfigurationProfileLabel{} err := ds.withTx(ctx, func(tx sqlx.ExtContext) error { - updatedDB, err := batchSetProfileLabelAssociationsDB(ctx, tx, want, platform) + updatedDB, err := batchSetProfileLabelAssociationsDB(ctx, tx, want, nil, platform) require.NoError(t, err) assert.False(t, updatedDB) return err @@ -6184,7 +6232,7 @@ func testBatchSetProfileLabelAssociations(t *testing.T, ds *Datastore) { {ProfileUUID: uuid, LabelName: label.Name, LabelID: label.ID}, } err := ds.withTx(ctx, func(tx sqlx.ExtContext) error { - updatedDB, err := batchSetProfileLabelAssociationsDB(ctx, tx, profileLabels, platform) + updatedDB, err := batchSetProfileLabelAssociationsDB(ctx, tx, profileLabels, nil, platform) require.NoError(t, err) assert.True(t, updatedDB) return err @@ -6200,7 +6248,7 @@ func testBatchSetProfileLabelAssociations(t *testing.T, ds *Datastore) { {ProfileUUID: uuid, LabelName: label.Name, LabelID: label.ID, Exclude: true}, } err = ds.withTx(ctx, func(tx sqlx.ExtContext) error { - updatedDB, err := batchSetProfileLabelAssociationsDB(ctx, tx, profileLabels, platform) + updatedDB, err := batchSetProfileLabelAssociationsDB(ctx, tx, profileLabels, nil, platform) require.NoError(t, err) assert.True(t, updatedDB) return err @@ -6218,7 +6266,7 @@ func testBatchSetProfileLabelAssociations(t *testing.T, ds *Datastore) { } err := ds.withTx(ctx, func(tx sqlx.ExtContext) error { - _, err := batchSetProfileLabelAssociationsDB(ctx, tx, invalidProfileLabels, platform) + _, err := batchSetProfileLabelAssociationsDB(ctx, tx, invalidProfileLabels, nil, platform) return err }) require.Error(t, err) @@ -6230,7 +6278,7 @@ func testBatchSetProfileLabelAssociations(t *testing.T, ds *Datastore) { {ProfileUUID: uuid, LabelName: label.Name, LabelID: 12345}, } err := ds.withTx(ctx, func(tx sqlx.ExtContext) error { - _, err := batchSetProfileLabelAssociationsDB(ctx, tx, invalidProfileLabels, platform) + _, err := batchSetProfileLabelAssociationsDB(ctx, tx, invalidProfileLabels, nil, platform) return err }) require.Error(t, err) @@ -6240,7 +6288,7 @@ func testBatchSetProfileLabelAssociations(t *testing.T, ds *Datastore) { {ProfileUUID: uuid, LabelName: "xyz", LabelID: 1235}, } err = ds.withTx(ctx, func(tx sqlx.ExtContext) error { - _, err := batchSetProfileLabelAssociationsDB(ctx, tx, invalidProfileLabels, platform) + _, err := batchSetProfileLabelAssociationsDB(ctx, tx, invalidProfileLabels, nil, platform) return err }) require.Error(t, err) @@ -6262,7 +6310,7 @@ func testBatchSetProfileLabelAssociations(t *testing.T, ds *Datastore) { {ProfileUUID: uuid, LabelName: newLabel.Name, LabelID: newLabel.ID, Exclude: true}, } err = ds.withTx(ctx, func(tx sqlx.ExtContext) error { - updatedDB, err := batchSetProfileLabelAssociationsDB(ctx, tx, profileLabels, platform) + updatedDB, err := batchSetProfileLabelAssociationsDB(ctx, tx, profileLabels, nil, platform) require.NoError(t, err) assert.True(t, updatedDB) return err @@ -6276,7 +6324,7 @@ func testBatchSetProfileLabelAssociations(t *testing.T, ds *Datastore) { {ProfileUUID: uuid, LabelName: label.Name, LabelID: label.ID}, } err = ds.withTx(ctx, func(tx sqlx.ExtContext) error { - updatedDB, err := batchSetProfileLabelAssociationsDB(ctx, tx, profileLabels, platform) + updatedDB, err := batchSetProfileLabelAssociationsDB(ctx, tx, profileLabels, nil, platform) require.NoError(t, err) assert.True(t, updatedDB) return err @@ -6284,9 +6332,29 @@ func testBatchSetProfileLabelAssociations(t *testing.T, ds *Datastore) { require.NoError(t, err) expectLabels(t, uuid, platform, profileLabels) + // batch apply again this time without any label + err = ds.withTx(ctx, func(tx sqlx.ExtContext) error { + updatedDB, err := batchSetProfileLabelAssociationsDB(ctx, tx, nil, []string{uuid}, platform) + require.NoError(t, err) + assert.True(t, updatedDB) + return err + }) + require.NoError(t, err) + expectLabels(t, uuid, platform, nil) + // does not change other profiles expectLabels(t, otherWinProfile.ProfileUUID, "windows", wantOtherWin) expectLabels(t, otherMacProfile.ProfileUUID, "darwin", wantOtherMac) + + // batch apply again with no change returns false + err = ds.withTx(ctx, func(tx sqlx.ExtContext) error { + updatedDB, err := batchSetProfileLabelAssociationsDB(ctx, tx, nil, []string{uuid}, platform) + require.NoError(t, err) + assert.False(t, updatedDB) + return err + }) + require.NoError(t, err) + expectLabels(t, uuid, platform, nil) }) } @@ -6296,6 +6364,7 @@ func testBatchSetProfileLabelAssociations(t *testing.T, ds *Datastore) { ctx, tx, []fleet.ConfigurationProfileLabel{{}}, + nil, "unsupported", ) return err diff --git a/server/datastore/mysql/microsoft_mdm.go b/server/datastore/mysql/microsoft_mdm.go index 80c934a1a5..e50a811247 100644 --- a/server/datastore/mysql/microsoft_mdm.go +++ b/server/datastore/mysql/microsoft_mdm.go @@ -1731,7 +1731,11 @@ INSERT INTO cp.LabelsExcludeAny[i].Exclude = true labels = append(labels, cp.LabelsExcludeAny[i]) } - if _, err := batchSetProfileLabelAssociationsDB(ctx, tx, labels, "windows"); err != nil { + var profsWithoutLabel []string + if len(labels) == 0 { + profsWithoutLabel = append(profsWithoutLabel, profileUUID) + } + if _, err := batchSetProfileLabelAssociationsDB(ctx, tx, labels, profsWithoutLabel, "windows"); err != nil { return ctxerr.Wrap(ctx, err, "inserting windows profile label associations") } @@ -1948,6 +1952,7 @@ ON DUPLICATE KEY UPDATE // between macOS and Windows, but at the time of this // implementation we're under tight time constraints. incomingLabels := []fleet.ConfigurationProfileLabel{} + var profsWithoutLabel []string if len(incomingNames) > 0 { var newlyInsertedProfs []*fleet.MDMWindowsConfigProfile // load current profiles (again) that match the incoming profiles by name to grab their uuids @@ -1971,30 +1976,37 @@ ON DUPLICATE KEY UPDATE return false, ctxerr.Wrapf(ctx, err, "profile %q is in the database but was not incoming", newlyInsertedProf.Name) } + var profHasLabel bool for _, label := range incomingProf.LabelsIncludeAll { label.ProfileUUID = newlyInsertedProf.ProfileUUID label.Exclude = false label.RequireAll = true incomingLabels = append(incomingLabels, label) + profHasLabel = true } for _, label := range incomingProf.LabelsIncludeAny { label.ProfileUUID = newlyInsertedProf.ProfileUUID label.Exclude = false label.RequireAll = false incomingLabels = append(incomingLabels, label) + profHasLabel = true } for _, label := range incomingProf.LabelsExcludeAny { label.ProfileUUID = newlyInsertedProf.ProfileUUID label.Exclude = true label.RequireAll = false incomingLabels = append(incomingLabels, label) + profHasLabel = true + } + if !profHasLabel { + profsWithoutLabel = append(profsWithoutLabel, newlyInsertedProf.ProfileUUID) } } } // insert/delete the label associations var updatedLabels bool - if updatedLabels, err = batchSetProfileLabelAssociationsDB(ctx, tx, incomingLabels, + if updatedLabels, err = batchSetProfileLabelAssociationsDB(ctx, tx, incomingLabels, profsWithoutLabel, "windows"); err != nil || strings.HasPrefix(ds.testBatchSetMDMWindowsProfilesErr, "labels") { if err == nil { err = errors.New(ds.testBatchSetMDMWindowsProfilesErr) diff --git a/server/test/mdm.go b/server/test/mdm.go index 0b5e8a5760..cfb29f30d4 100644 --- a/server/test/mdm.go +++ b/server/test/mdm.go @@ -67,3 +67,25 @@ func CreateVPPTokenEncodedAfterMigration(expiration time.Time, orgName, location } return dataTokenJson, nil } + +func GenerateMDMAppleProfile(ident, displayName, uuid string) string { + return fmt.Sprintf(` + + + + PayloadContent + + PayloadIdentifier + %s + PayloadDisplayName + %s + PayloadUUID + %s + PayloadType + Configuration + PayloadVersion + 1 + + +`, ident, displayName, uuid) +}