From 8bcdb82d55b292279110c00e052bb3051a00bdd5 Mon Sep 17 00:00:00 2001 From: Gabriel Hernandez Date: Thu, 30 Jan 2025 11:17:36 +0000 Subject: [PATCH] update error message for same name profile (#25673) For #17700 improve error message when a custom profile is edited or uploaded with one that has the same name. updates the error message for `POST /fleet/mdm/profiles` and in fleetctl when using gitops - [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] Added/updated automated tests - [x] Manual QA for all new/changed functionality --- server/service/apple_mdm.go | 14 +++-- server/service/apple_mdm_test.go | 12 ++-- server/service/client.go | 4 +- .../service/integration_mdm_profiles_test.go | 33 +++++----- server/service/mdm.go | 60 +++++-------------- server/service/mdm_test.go | 4 +- 6 files changed, 52 insertions(+), 75 deletions(-) diff --git a/server/service/apple_mdm.go b/server/service/apple_mdm.go index 3b277fcf98..88ebcfbb03 100644 --- a/server/service/apple_mdm.go +++ b/server/service/apple_mdm.go @@ -63,6 +63,10 @@ const ( FleetVarHostEndUserEmailIDP = "HOST_END_USER_EMAIL_IDP" ) +const ( + SameProfileNameUploadErrorMsg = "Couldn't add. A configuration profile with this name already exists (PayloadDisplayName for .mobileconfig and file name for .json and .xml)." +) + var ( profileVariableRegex = regexp.MustCompile(`(\$FLEET_VAR_(?P\w+))|(\${FLEET_VAR_(?P\w+)})`) fleetVarNDESSCEPChallengeRegexp = regexp.MustCompile(fmt.Sprintf(`(\$FLEET_VAR_%s)|(\${FLEET_VAR_%s})`, FleetVarNDESSCEPChallenge, @@ -424,7 +428,7 @@ func (svc *Service) NewMDMAppleConfigProfile(ctx context.Context, teamID uint, r if err != nil { var existsErr existsErrorInterface if errors.As(err, &existsErr) { - msg := "Couldn't upload. A configuration profile with this name already exists." + msg := SameProfileNameUploadErrorMsg if re, ok := existsErr.(interface{ Resource() string }); ok { if re.Resource() == "MDMAppleConfigProfile.PayloadIdentifier" { msg = "Couldn't upload. A configuration profile with this identifier (PayloadIdentifier) already exists." @@ -2015,14 +2019,14 @@ func (svc *Service) BatchSetMDMAppleProfiles(ctx context.Context, tmID *uint, tm if byName[mdmProf.Name] { return ctxerr.Wrap(ctx, - fleet.NewInvalidArgumentError(fmt.Sprintf("profiles[%d]", i), fmt.Sprintf("Couldn’t edit custom_settings. More than one configuration profile have the same name (PayloadDisplayName): %q", mdmProf.Name)), + fleet.NewInvalidArgumentError(fmt.Sprintf("profiles[%d]", i), fmt.Sprintf("Couldn't edit custom_settings. More than one configuration profile have the same name (PayloadDisplayName): %q", mdmProf.Name)), "duplicate mobileconfig profile by name") } byName[mdmProf.Name] = true if byIdent[mdmProf.Identifier] { return ctxerr.Wrap(ctx, - fleet.NewInvalidArgumentError(fmt.Sprintf("profiles[%d]", i), fmt.Sprintf("Couldn’t edit custom_settings. More than one configuration profile have the same identifier (PayloadIdentifier): %q", mdmProf.Identifier)), + fleet.NewInvalidArgumentError(fmt.Sprintf("profiles[%d]", i), fmt.Sprintf("Couldn't edit custom_settings. More than one configuration profile have the same identifier (PayloadIdentifier): %q", mdmProf.Identifier)), "duplicate mobileconfig profile by identifier") } byIdent[mdmProf.Identifier] = true @@ -2046,11 +2050,11 @@ func (svc *Service) BatchSetMDMAppleProfiles(ctx context.Context, tmID *uint, tm 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)) + "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)) + "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") } } diff --git a/server/service/apple_mdm_test.go b/server/service/apple_mdm_test.go index 09e167d38c..3f23aab409 100644 --- a/server/service/apple_mdm_test.go +++ b/server/service/apple_mdm_test.go @@ -3713,7 +3713,8 @@ func TestRenewSCEPCertificatesBranches(t *testing.T) { } appleStore.EnqueueCommandFunc = func(ctx context.Context, id []string, cmd *mdm.CommandWithSubtype) (map[string]error, - error) { + error, + ) { require.Equal(t, "InstallProfile", cmd.Command.Command.RequestType) wantCommandUUID = cmd.CommandUUID return map[string]error{}, nil @@ -3740,7 +3741,8 @@ func TestRenewSCEPCertificatesBranches(t *testing.T) { } appleStore.EnqueueCommandFunc = func(ctx context.Context, id []string, cmd *mdm.CommandWithSubtype) (map[string]error, - error) { + error, + ) { return map[string]error{}, errors.New("foo") } }, @@ -3754,7 +3756,8 @@ func TestRenewSCEPCertificatesBranches(t *testing.T) { return []fleet.SCEPIdentityAssociation{{HostUUID: "hostUUID2", EnrollReference: "ref1"}}, nil } appleStore.EnqueueCommandFunc = func(ctx context.Context, id []string, cmd *mdm.CommandWithSubtype) (map[string]error, - error) { + error, + ) { require.Equal(t, "InstallProfile", cmd.Command.Command.RequestType) wantCommandUUID = cmd.CommandUUID return map[string]error{}, nil @@ -3780,7 +3783,8 @@ func TestRenewSCEPCertificatesBranches(t *testing.T) { } appleStore.EnqueueCommandFunc = func(ctx context.Context, id []string, cmd *mdm.CommandWithSubtype) (map[string]error, - error) { + error, + ) { return map[string]error{}, errors.New("foo") } }, diff --git a/server/service/client.go b/server/service/client.go index c58b98d73c..ef38eb596e 100644 --- a/server/service/client.go +++ b/server/service/client.go @@ -372,8 +372,8 @@ func getProfilesContents(baseDir string, macProfiles []fleet.MDMProfileSpec, win } // check for duplicate names across all profiles - if e, isDuplicate := extByName[name]; isDuplicate { - return nil, errors.New(fmtDuplicateNameErrMsg(name, e, ext)) + if _, isDuplicate := extByName[name]; isDuplicate { + return nil, errors.New(fmtDuplicateNameErrMsg(name)) } extByName[name] = ext diff --git a/server/service/integration_mdm_profiles_test.go b/server/service/integration_mdm_profiles_test.go index 88f7b01b85..940b5d238f 100644 --- a/server/service/integration_mdm_profiles_test.go +++ b/server/service/integration_mdm_profiles_test.go @@ -83,7 +83,7 @@ func (s *integrationMDMTestSuite) TestAppleProfileManagement() { s.Do("POST", "/api/v1/fleet/mdm/apple/profiles/batch", batchSetMDMAppleProfilesRequest{Profiles: globalProfiles}, http.StatusNoContent) // invalid secrets - var invalidSecretsProfile = []byte(` + invalidSecretsProfile := []byte(` @@ -1351,8 +1351,10 @@ func (s *integrationMDMTestSuite) TestPuppetMatchPreassignProfiles() { {Identifier: mobileconfig.FleetCARootConfigPayloadIdentifier, OperationType: fleet.MDMOperationTypeInstall, Status: &fleet.MDMDeliveryPending}, // Profiles from previous team being deleted {Identifier: "i2", OperationType: fleet.MDMOperationTypeRemove, Status: &fleet.MDMDeliveryPending}, - {Identifier: mobileconfig.FleetFileVaultPayloadIdentifier, OperationType: fleet.MDMOperationTypeRemove, - Status: &fleet.MDMDeliveryPending}, + { + Identifier: mobileconfig.FleetFileVaultPayloadIdentifier, OperationType: fleet.MDMOperationTypeRemove, + Status: &fleet.MDMDeliveryPending, + }, }, }) @@ -2994,19 +2996,19 @@ func (s *integrationMDMTestSuite) TestMDMConfigProfileCRUD() { teamWinProfUUID := createWindowsProfile("win-team-profile", testTeam.ID, nil) // Windows profile name conflicts with Apple's for no team - assertWindowsProfile("apple-global-profile.xml", "./Test", 0, nil, http.StatusConflict, "Couldn't upload. A configuration profile with this name already exists.") + assertWindowsProfile("apple-global-profile.xml", "./Test", 0, nil, http.StatusConflict, SameProfileNameUploadErrorMsg) // but no conflict for team 1 assertWindowsProfile("apple-global-profile.xml", "./Test", testTeam.ID, nil, http.StatusOK, "") // Apple profile name conflicts with Windows' for no team - assertAppleProfile("win-global-profile.mobileconfig", "win-global-profile", "test-global-ident-2", 0, nil, http.StatusConflict, "Couldn't upload. A configuration profile with this name already exists.") + assertAppleProfile("win-global-profile.mobileconfig", "win-global-profile", "test-global-ident-2", 0, nil, http.StatusConflict, SameProfileNameUploadErrorMsg) // but no conflict for team 1 assertAppleProfile("win-global-profile.mobileconfig", "win-global-profile", "test-global-ident-2", testTeam.ID, nil, http.StatusOK, "") // Windows profile name conflicts with Apple's for team 1 - assertWindowsProfile("apple-team-profile.xml", "./Test", testTeam.ID, nil, http.StatusConflict, "Couldn't upload. A configuration profile with this name already exists.") + assertWindowsProfile("apple-team-profile.xml", "./Test", testTeam.ID, nil, http.StatusConflict, SameProfileNameUploadErrorMsg) // but no conflict for no-team assertWindowsProfile("apple-team-profile.xml", "./Test", 0, nil, http.StatusOK, "") // Apple profile name conflicts with Windows' for team 1 - assertAppleProfile("win-team-profile.mobileconfig", "win-team-profile", "test-team-ident-2", testTeam.ID, nil, http.StatusConflict, "Couldn't upload. A configuration profile with this name already exists.") + assertAppleProfile("win-team-profile.mobileconfig", "win-team-profile", "test-team-ident-2", testTeam.ID, nil, http.StatusConflict, SameProfileNameUploadErrorMsg) // but no conflict for no-team assertAppleProfile("win-team-profile.mobileconfig", "win-team-profile", "test-team-ident-2", 0, nil, http.StatusOK, "") @@ -3023,9 +3025,9 @@ func (s *integrationMDMTestSuite) TestMDMConfigProfileCRUD() { // name is pulled from filename, it conflicts with existing macOS config profile assertAppleDeclaration("win-global-profile.json", "test-declaration-ident-2", 0, nil, http.StatusConflict, "win-global-profile already exists") // windows profile name conflicts with existing declaration - assertWindowsProfile("apple-declaration.xml", "./Test", 0, nil, http.StatusConflict, "Couldn't upload. A configuration profile with this name already exists.") + assertWindowsProfile("apple-declaration.xml", "./Test", 0, nil, http.StatusConflict, SameProfileNameUploadErrorMsg) // macOS profile name conflicts with existing declaration - assertAppleProfile("apple-declaration.mobileconfig", "apple-declaration", "test-declaration-ident", 0, nil, http.StatusConflict, "Couldn't upload. A configuration profile with this name already exists.") + assertAppleProfile("apple-declaration.mobileconfig", "apple-declaration", "test-declaration-ident", 0, nil, http.StatusConflict, SameProfileNameUploadErrorMsg) // not an xml nor mobileconfig file assertWindowsProfile("foo.txt", "./Test", 0, nil, http.StatusBadRequest, "Couldn't add profile. The file should be a .mobileconfig, XML, or JSON file.") @@ -4330,15 +4332,15 @@ func (s *integrationMDMTestSuite) TestBatchSetMDMProfiles() { }{ { 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).", + expectErr: "More than one configuration profile have the same name 'N1'", }, { 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).", + expectErr: "More than one configuration profile have the same name 'N1'", }, { 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).", + expectErr: "More than one configuration profile have the same name 'N1'", }, } { // team profiles @@ -5382,7 +5384,8 @@ func (s *integrationMDMTestSuite) TestAppleDDMSecretVariablesUpload() { } func (s *integrationMDMTestSuite) testSecretVariablesUpload(newProfileBytes func(i int) []byte, - getProfileContents func(profileUUID string) string, fileExtension string, platform string) { + getProfileContents func(profileUUID string) string, fileExtension string, platform string, +) { t := s.T() const numProfiles = 2 var profiles [][]byte @@ -5459,7 +5462,6 @@ func (s *integrationMDMTestSuite) testSecretVariablesUpload(newProfileBytes func } s.DoJSON("GET", "/api/latest/fleet/mdm/profiles", &listMDMConfigProfilesRequest{}, http.StatusOK, &listResp) require.Empty(t, listResp.Profiles) - } // TestAppleConfigSecretVariablesUpload tests uploading Apple config profiles with secrets via the /configuration_profiles endpoint @@ -5513,7 +5515,6 @@ func (s *integrationMDMTestSuite) TestAppleConfigSecretVariablesUpload() { } s.testSecretVariablesUpload(newProfileBytes, getProfileContents, "mobileconfig", "darwin") - } // TestWindowsConfigSecretVariablesUpload tests uploading Windows profiles with secrets via the /configuration_profiles endpoint @@ -5543,7 +5544,6 @@ func (s *integrationMDMTestSuite) TestWindowsConfigSecretVariablesUpload() { } s.testSecretVariablesUpload(newProfileBytes, getProfileContents, "xml", "windows") - } func (s *integrationMDMTestSuite) TestAppleProfileDeletion() { @@ -5698,5 +5698,4 @@ func (s *integrationMDMTestSuite) TestAppleProfileDeletion() { profiles, err = s.ds.GetHostMDMAppleProfiles(ctx, host2.UUID) require.NoError(t, err) assert.Len(t, profiles, 3) - } diff --git a/server/service/mdm.go b/server/service/mdm.go index d0de896f68..22026f7f86 100644 --- a/server/service/mdm.go +++ b/server/service/mdm.go @@ -1435,7 +1435,7 @@ func (svc *Service) NewMDMWindowsConfigProfile(ctx context.Context, teamID uint, if err != nil { var existsErr existsErrorInterface if errors.As(err, &existsErr) { - err = fleet.NewInvalidArgumentError("profile", "Couldn't upload. A configuration profile with this name already exists."). + err = fleet.NewInvalidArgumentError("profile", SameProfileNameUploadErrorMsg). WithStatus(http.StatusConflict) } return nil, ctxerr.Wrap(ctx, err) @@ -1776,27 +1776,28 @@ func validateFleetVariables(ctx context.Context, appleProfiles map[int]*fleet.MD } func (svc *Service) validateCrossPlatformProfileNames(ctx context.Context, appleProfiles map[int]*fleet.MDMAppleConfigProfile, - windowsProfiles map[int]*fleet.MDMWindowsConfigProfile, appleDecls map[int]*fleet.MDMAppleDeclaration) error { + windowsProfiles map[int]*fleet.MDMWindowsConfigProfile, appleDecls map[int]*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)) + if _, ok := extByName[p.Name]; ok { + err := fleet.NewInvalidArgumentError(fmt.Sprintf("appleProfiles[%d]", i), fmtDuplicateNameErrMsg(p.Name)) 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)) + if _, ok := extByName[p.Name]; ok { + err := fleet.NewInvalidArgumentError(fmt.Sprintf("windowsProfiles[%d]", i), fmtDuplicateNameErrMsg(p.Name)) 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)) + if _, ok := extByName[p.Name]; ok { + err := fleet.NewInvalidArgumentError(fmt.Sprintf("appleDecls[%d]", i), fmtDuplicateNameErrMsg(p.Name)) return ctxerr.Wrap(ctx, err, "duplicate json by name") } extByName[p.Name] = ".json" @@ -1805,39 +1806,9 @@ func (svc *Service) validateCrossPlatformProfileNames(ctx context.Context, apple 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 fmtDuplicateNameErrMsg(name string) string { + const SameProfileNameErrorMsg = "Couldn't edit custom_settings. More than one configuration profile have the same name '%s' (PayloadDisplayName for .mobileconfig and file name for .json and .xml)." + return fmt.Sprintf(SameProfileNameErrorMsg, name) } func (svc *Service) authorizeBatchProfiles(ctx context.Context, tmID *uint, tmName *string) (*uint, *string, error) { @@ -2010,14 +1981,13 @@ func getAppleProfiles( if mdmProf.Name != prof.Name { return nil, nil, ctxerr.Wrap(ctx, - fleet.NewInvalidArgumentError(prof.Name, fmt.Sprintf("Couldn’t edit custom_settings. The name provided for the profile must match the profile PayloadDisplayName: %q", mdmProf.Name)), + fleet.NewInvalidArgumentError(prof.Name, fmt.Sprintf("Couldn't edit custom_settings. The name provided for the profile must match the profile PayloadDisplayName: %q", mdmProf.Name)), "duplicate mobileconfig profile by name") } - // TODO: confirm error messages if _, ok := byName[mdmProf.Name]; ok { return nil, nil, ctxerr.Wrap(ctx, - fleet.NewInvalidArgumentError(prof.Name, fmt.Sprintf("Couldn’t edit custom_settings. More than one configuration profile have the same name (PayloadDisplayName): %q", mdmProf.Name)), + fleet.NewInvalidArgumentError(prof.Name, fmt.Sprintf("Couldn't edit custom_settings. More than one configuration profile have the same name (PayloadDisplayName): %q", mdmProf.Name)), "duplicate mobileconfig profile by name") } byName[mdmProf.Name] = "mobileconfig" @@ -2025,7 +1995,7 @@ func getAppleProfiles( // TODO: confirm error messages if _, ok := byIdent[mdmProf.Identifier]; ok { return nil, nil, ctxerr.Wrap(ctx, - fleet.NewInvalidArgumentError(prof.Name, fmt.Sprintf("Couldn’t edit custom_settings. More than one configuration profile have the same identifier (PayloadIdentifier): %q", mdmProf.Identifier)), + fleet.NewInvalidArgumentError(prof.Name, fmt.Sprintf("Couldn't edit custom_settings. More than one configuration profile have the same identifier (PayloadIdentifier): %q", mdmProf.Identifier)), "duplicate mobileconfig profile by identifier") } byIdent[mdmProf.Identifier] = "mobileconfig" diff --git a/server/service/mdm_test.go b/server/service/mdm_test.go index 74a59f5a0d..0ec610a515 100644 --- a/server/service/mdm_test.go +++ b/server/service/mdm_test.go @@ -1207,7 +1207,7 @@ func TestUploadWindowsMDMConfigProfileValidations(t *testing.T) { {"random non-xml data", 0, "\x00\x01\x02", true, "The file should include valid XML:"}, {"valid windows profile", 0, ``, true, ""}, {"mdm not enabled", 0, ``, false, "Windows MDM isn't turned on."}, - {"duplicate profile name", 0, `duplicate`, true, "configuration profile with this name already exists."}, + {"duplicate profile name", 0, `duplicate`, true, "configuration profile with this name already exists"}, {"multiple Replace", 0, `ab`, true, ""}, {"Replace and non-Replace", 0, `ab`, true, "Windows configuration profiles can only have or top level elements."}, {"BitLocker profile", 0, `./Device/Vendor/MSFT/BitLocker/AllowStandardUserEncryption`, true, "Custom configuration profiles can't include BitLocker settings."}, @@ -1219,7 +1219,7 @@ func TestUploadWindowsMDMConfigProfileValidations(t *testing.T) { {"team random non-xml data", 1, "\x00\x01\x02", true, "The file should include valid XML:"}, {"team valid windows profile", 1, ``, true, ""}, {"team mdm not enabled", 1, ``, false, "Windows MDM isn't turned on."}, - {"team duplicate profile name", 1, `duplicate`, true, "configuration profile with this name already exists."}, + {"team duplicate profile name", 1, `duplicate`, true, "configuration profile with this name already exists"}, {"team multiple Replace", 1, `ab`, true, ""}, {"team Replace and non-Replace", 1, `ab`, true, "Windows configuration profiles can only have or top level elements."}, {"team BitLocker profile", 1, `./Device/Vendor/MSFT/BitLocker/AllowStandardUserEncryption`, true, "Custom configuration profiles can't include BitLocker settings."},