From 841350f55671b5e50f9439f0f3f0a4f1bb828f74 Mon Sep 17 00:00:00 2001 From: Sarah Gillespie <73313222+gillespi314@users.noreply.github.com> Date: Fri, 29 Mar 2024 14:55:03 -0500 Subject: [PATCH] Add cross-platform check for duplicate MDM profile names (#17916) --- changes/17559-batch-set-duplicate-mdm | 1 + server/service/apple_mdm.go | 29 ++++++++- server/service/apple_mdm_test.go | 6 ++ server/service/client.go | 9 +-- server/service/integration_ddm_test.go | 3 +- server/service/integration_mdm_test.go | 39 +++++++++++ server/service/mdm.go | 89 ++++++++++++++++++++------ 7 files changed, 149 insertions(+), 27 deletions(-) create mode 100644 changes/17559-batch-set-duplicate-mdm diff --git a/changes/17559-batch-set-duplicate-mdm b/changes/17559-batch-set-duplicate-mdm new file mode 100644 index 000000000..f037326ff --- /dev/null +++ b/changes/17559-batch-set-duplicate-mdm @@ -0,0 +1 @@ +- Added cross-platform check for duplicate MDM profiles names in batch set MDM profiles API. diff --git a/server/service/apple_mdm.go b/server/service/apple_mdm.go index c230ccbec..ddbd5e1e4 100644 --- a/server/service/apple_mdm.go +++ b/server/service/apple_mdm.go @@ -1752,6 +1752,34 @@ func (svc *Service) BatchSetMDMAppleProfiles(ctx context.Context, tmID *uint, tm profs = append(profs, mdmProf) } + if !skipBulkPending { + // check for duplicates with existing profiles, skipBulkPending signals that the caller + // is responsible for ensuring that the profiles names are unique (e.g., MDMAppleMatchPreassignment) + allProfs, _, err := svc.ds.ListMDMConfigProfiles(ctx, tmID, fleet.ListOptions{PerPage: 0}) + if err != nil { + return ctxerr.Wrap(ctx, err, "list mdm config profiles") + } + for _, p := range allProfs { + if byName[p.Name] { + switch { + case strings.HasPrefix(p.ProfileUUID, "a"): + // do nothing, all existing mobileconfigs will be replaced and we've already checked + // the new mobileconfigs for duplicates + continue + case strings.HasPrefix(p.ProfileUUID, "w"): + err := fleet.NewInvalidArgumentError("PayloadDisplayName", fmt.Sprintf( + "Couldn’t edit custom_settings. A Windows configuration profile shares the same name as a macOS configuration profile (PayloadDisplayName): %q", p.Name)) + return ctxerr.Wrap(ctx, err, "duplicate xml and mobileconfig by name") + default: + err := fleet.NewInvalidArgumentError("PayloadDisplayName", fmt.Sprintf( + "Couldn’t edit custom_settings. More than one configuration profile have the same name (PayloadDisplayName): %q", p.Name)) + return ctxerr.Wrap(ctx, err, "duplicate json and mobileconfig by name") + } + } + byName[p.Name] = true + } + } + if dryRun { return nil } @@ -2753,7 +2781,6 @@ func ReconcileAppleDeclarations( commander *apple_mdm.MDMAppleCommander, logger kitlog.Logger, ) error { - // batch set declarations as pending changedHosts, err := ds.MDMAppleBatchSetHostDeclarationState(ctx) if err != nil { diff --git a/server/service/apple_mdm_test.go b/server/service/apple_mdm_test.go index 764f4c50a..2f22fa721 100644 --- a/server/service/apple_mdm_test.go +++ b/server/service/apple_mdm_test.go @@ -1430,6 +1430,9 @@ func TestMDMBatchSetAppleProfiles(t *testing.T) { ds.BulkSetPendingMDMHostProfilesFunc = func(ctx context.Context, hids, tids []uint, puuids, uuids []string) error { return nil } + ds.ListMDMConfigProfilesFunc = func(ctx context.Context, tid *uint, opt fleet.ListOptions) ([]*fleet.MDMConfigProfilePayload, *fleet.PaginationMetadata, error) { + return nil, nil, nil + } type testCase struct { name string @@ -1741,6 +1744,9 @@ func TestMDMBatchSetAppleProfilesBoolArgs(t *testing.T) { ds.BulkSetPendingMDMHostProfilesFunc = func(ctx context.Context, hids, tids []uint, profileUUIDs, uuids []string) error { return nil } + ds.ListMDMConfigProfilesFunc = func(ctx context.Context, tid *uint, opt fleet.ListOptions) ([]*fleet.MDMConfigProfilePayload, *fleet.PaginationMetadata, error) { + return nil, nil, nil + } ctx = viewer.NewContext(ctx, viewer.Viewer{User: &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)}}) ctx = license.NewContext(ctx, &fleet.LicenseInfo{Tier: fleet.TierPremium}) diff --git a/server/service/client.go b/server/service/client.go index 659c4bfff..0492f6486 100644 --- a/server/service/client.go +++ b/server/service/client.go @@ -286,7 +286,8 @@ func (c *Client) runAppConfigChecks(fn func(ac *fleet.EnrichedAppConfig) error) // getProfilesContents takes file paths and creates a slice of profile payloads // ready to batch-apply. func getProfilesContents(baseDir string, profiles []fleet.MDMProfileSpec) ([]fleet.MDMProfileBatchPayload, error) { - fileNameMap := make(map[string]struct{}, len(profiles)) + // map to check for duplicate names + extByName := make(map[string]string, len(profiles)) result := make([]fleet.MDMProfileBatchPayload, 0, len(profiles)) for _, profile := range profiles { @@ -306,10 +307,10 @@ func getProfilesContents(baseDir string, profiles []fleet.MDMProfileSpec) ([]fle } name = strings.TrimSpace(mc.Name) } - if _, isDuplicate := fileNameMap[name]; isDuplicate { - return nil, errors.New("Couldn't edit windows_settings.custom_settings. More than one configuration profile have the same name (Windows .xml file name or macOS PayloadDisplayName).") + if e, isDuplicate := extByName[name]; isDuplicate { + return nil, errors.New(fmtDuplicateNameErrMsg(name, e, ext)) } - fileNameMap[name] = struct{}{} + extByName[name] = ext result = append(result, fleet.MDMProfileBatchPayload{ Name: name, Contents: fileContents, diff --git a/server/service/integration_ddm_test.go b/server/service/integration_ddm_test.go index 617976281..67d316ea5 100644 --- a/server/service/integration_ddm_test.go +++ b/server/service/integration_ddm_test.go @@ -88,7 +88,7 @@ func (s *integrationMDMTestSuite) TestAppleDDMBatchUpload() { {Name: "bad2", Contents: newDeclBytes(2, `"baz": "bing"`)}, }}, http.StatusUnprocessableEntity) errMsg = extractServerErrorText(res.Body) - require.Contains(t, errMsg, "A declaration profile with this name already exists.") + require.Contains(t, errMsg, "More than one configuration profile have the same name") // Same identifier should fail res = s.Do("POST", "/api/latest/fleet/mdm/profiles/batch", batchSetMDMProfilesRequest{Profiles: []fleet.MDMProfileBatchPayload{ @@ -877,7 +877,6 @@ func (s *integrationMDMTestSuite) TestDDMUnsupportedDevice() { require.Contains(t, profs["I1"].Detail, "Feature Disabled") require.Equal(t, &fleet.MDMDeliveryFailed, profs["I2"].Status) require.Contains(t, profs["I2"].Detail, "Feature Disabled") - } func (s *integrationMDMTestSuite) TestDDMNoDeclarationsLeft() { diff --git a/server/service/integration_mdm_test.go b/server/service/integration_mdm_test.go index 2ec8f3ca3..7e9d9e6a5 100644 --- a/server/service/integration_mdm_test.go +++ b/server/service/integration_mdm_test.go @@ -10990,11 +10990,50 @@ func (s *integrationMDMTestSuite) TestBatchSetMDMProfiles() { fmt.Sprintf(`{"team_id": %d, "team_name": %q}`, tm.ID, tm.Name), 0, ) + s.lastActivityOfTypeMatches( fleet.ActivityTypeEditedDeclarationProfile{}.ActivityName(), fmt.Sprintf(`{"team_id": %d, "team_name": %q}`, tm.ID, tm.Name), 0, ) + + // names cannot be duplicated across platforms + declBytes := json.RawMessage(`{ + "Type": "com.apple.configuration.decl.foo", + "Identifier": "com.fleet.config.foo", + "Payload": { + "ServiceType": "com.apple.bash", + "DataAssetReference": "com.fleet.asset.bash" + }}`) + mcBytes := mobileconfigForTest("N1", "I1") + winBytes := syncMLForTest("./Foo/Bar") + + for _, p := range []struct { + payload []fleet.MDMProfileBatchPayload + expectErr string + }{ + { + payload: []fleet.MDMProfileBatchPayload{{Name: "N1", Contents: mcBytes}, {Name: "N1", Contents: winBytes}}, + expectErr: "More than one configuration profile have the same name 'N1' (Windows .xml file name or macOS .mobileconfig PayloadDisplayName).", + }, + { + payload: []fleet.MDMProfileBatchPayload{{Name: "N1", Contents: declBytes}, {Name: "N1", Contents: winBytes}}, + expectErr: "More than one configuration profile have the same name 'N1' (macOS .json file name or Windows .xml file name).", + }, + { + payload: []fleet.MDMProfileBatchPayload{{Name: "N1", Contents: mcBytes}, {Name: "N1", Contents: declBytes}}, + expectErr: "More than one configuration profile have the same name 'N1' (macOS .json file name or macOS .mobileconfig PayloadDisplayName).", + }, + } { + // team profiles + res = s.Do("POST", "/api/v1/fleet/mdm/profiles/batch", batchSetMDMProfilesRequest{Profiles: p.payload}, http.StatusUnprocessableEntity, "team_id", strconv.Itoa(int(tm.ID))) + errMsg = extractServerErrorText(res.Body) + require.Contains(t, errMsg, p.expectErr) + // no team profiles + res = s.Do("POST", "/api/v1/fleet/mdm/profiles/batch", batchSetMDMProfilesRequest{Profiles: p.payload}, http.StatusUnprocessableEntity) + errMsg = extractServerErrorText(res.Body) + require.Contains(t, errMsg, p.expectErr) + } } func (s *integrationMDMTestSuite) TestBatchSetMDMProfilesBackwardsCompat() { diff --git a/server/service/mdm.go b/server/service/mdm.go index b6c932437..821ba0b1d 100644 --- a/server/service/mdm.go +++ b/server/service/mdm.go @@ -1527,6 +1527,10 @@ func (svc *Service) BatchSetMDMProfiles( return ctxerr.Wrap(ctx, err, "validating Windows profiles") } + if err := svc.validateCrossPlatformProfileNames(ctx, appleProfiles, windowsProfiles, appleDecls); err != nil { + return ctxerr.Wrap(ctx, err, "validating cross-platform profile names") + } + if dryRun { return nil } @@ -1578,6 +1582,70 @@ func (svc *Service) BatchSetMDMProfiles( return nil } +func (svc *Service) validateCrossPlatformProfileNames(ctx context.Context, appleProfiles []*fleet.MDMAppleConfigProfile, windowsProfiles []*fleet.MDMWindowsConfigProfile, appleDecls []*fleet.MDMAppleDeclaration) error { + // map all profile names to check for duplicates, regardless of platform; key is name, value is one of + // ".mobileconfig" or ".json" or ".xml" + extByName := make(map[string]string, len(appleProfiles)+len(windowsProfiles)+len(appleDecls)) + for i, p := range appleProfiles { + if v, ok := extByName[p.Name]; ok { + err := fleet.NewInvalidArgumentError(fmt.Sprintf("appleProfiles[%d]", i), fmtDuplicateNameErrMsg(p.Name, ".mobileconfig", v)) + return ctxerr.Wrap(ctx, err, "duplicate mobileconfig profile by name") + } + extByName[p.Name] = ".mobileconfig" + } + for i, p := range windowsProfiles { + if v, ok := extByName[p.Name]; ok { + err := fleet.NewInvalidArgumentError(fmt.Sprintf("windowsProfiles[%d]", i), fmtDuplicateNameErrMsg(p.Name, ".xml", v)) + return ctxerr.Wrap(ctx, err, "duplicate xml by name") + } + extByName[p.Name] = ".xml" + } + for i, p := range appleDecls { + if v, ok := extByName[p.Name]; ok { + err := fleet.NewInvalidArgumentError(fmt.Sprintf("appleDecls[%d]", i), fmtDuplicateNameErrMsg(p.Name, ".json", v)) + return ctxerr.Wrap(ctx, err, "duplicate json by name") + } + extByName[p.Name] = ".json" + } + + return nil +} + +func fmtDuplicateNameErrMsg(name, fileType1, fileType2 string) string { + var part1 string + switch fileType1 { + case ".xml": + part1 = "Windows .xml file name" + case ".mobileconfig": + part1 = "macOS .mobileconfig PayloadDisplayName" + case ".json": + part1 = "macOS .json file name" + } + + var part2 string + switch fileType2 { + case ".xml": + part2 = "Windows .xml file name" + case ".mobileconfig": + part2 = "macOS .mobileconfig PayloadDisplayName" + case ".json": + part2 = "macOS .json file name" + } + + base := fmt.Sprintf(`Couldn’t edit custom_settings. More than one configuration profile have the same name '%s'`, name) + detail := ` (%s).` + switch { + case part1 == part2: + return fmt.Sprintf(base+detail, part1) + case part1 != "" && part2 != "": + return fmt.Sprintf(base+detail, fmt.Sprintf("%s or %s", part1, part2)) + case part1 != "" || part2 != "": + return fmt.Sprintf(base+detail, part1+part2) + default: + return base + "." // should never happen + } +} + func (svc *Service) authorizeBatchProfiles(ctx context.Context, tmID *uint, tmName *string) (*uint, *string, error) { if tmID != nil && tmName != nil { svc.authz.SkipAuthorization(ctx) // so that the error message is not replaced by "forbidden" @@ -1658,26 +1726,7 @@ func getAppleProfiles( } } - v, ok := byName[mdmDecl.Name] - switch { - case !ok: - byName[mdmDecl.Name] = "declaration" - case v == "mobileconfig": - return nil, nil, ctxerr.Wrap(ctx, - fleet.NewInvalidArgumentError(mdmDecl.Name, "A configuration profile with this name already exists."), - "duplicate mobileconfig profile by name") - case v == "declaration": - return nil, nil, ctxerr.Wrap(ctx, - fleet.NewInvalidArgumentError(mdmDecl.Name, "A declaration profile with this name already exists."), - "duplicate declaration profile by name") - default: - // this should never happen but just in case - return nil, nil, ctxerr.Wrap(ctx, - fleet.NewInvalidArgumentError(mdmDecl.Name, "A profile with this name already exists."), - "duplicate profile by name") - } - - v, ok = byIdent[mdmDecl.Identifier] + v, ok := byIdent[mdmDecl.Identifier] switch { case !ok: byIdent[mdmDecl.Identifier] = "declaration"