diff --git a/pkg/services/ngalert/models/receivers.go b/pkg/services/ngalert/models/receivers.go index 1130711da90..1601ddc94e5 100644 --- a/pkg/services/ngalert/models/receivers.go +++ b/pkg/services/ngalert/models/receivers.go @@ -130,6 +130,9 @@ func (r *Receiver) WithExistingSecureFields(existing *Receiver, integrationSecur // Validate validates all integration settings, ensuring that the integrations are correctly configured. func (r *Receiver) Validate(decryptFn DecryptFn) error { var errs []error + if r.Name == "" { + errs = append(errs, fmt.Errorf("name should not be an empty string")) + } for _, integration := range r.Integrations { if err := integration.Validate(decryptFn); err != nil { errs = append(errs, err) diff --git a/pkg/services/ngalert/notifier/receiver_svc_test.go b/pkg/services/ngalert/notifier/receiver_svc_test.go index 85e7733f4bd..221f3e805d0 100644 --- a/pkg/services/ngalert/notifier/receiver_svc_test.go +++ b/pkg/services/ngalert/notifier/receiver_svc_test.go @@ -476,6 +476,12 @@ func TestReceiverService_Create(t *testing.T) { }, }, }, + { + name: "receiver with empty name fails", + user: writer, + receiver: models.CopyReceiverWith(baseReceiver, models.ReceiverMuts.WithName("")), + expectedErr: legacy_storage.ErrReceiverInvalid, + }, } { t.Run(tc.name, func(t *testing.T) { sut := createReceiverServiceSut(t, &secretsService) @@ -901,6 +907,16 @@ func TestReceiverService_UpdateReceiverName(t *testing.T) { assert.NotNil(t, ruleStore.Calls[0].Args[4]) assert.Falsef(t, ruleStore.Calls[0].Args[5].(bool), "dryrun expected to be false") }) + + t.Run("returns ErrReceiverInvalid if empty name", func(t *testing.T) { + ruleStore := &fakeAlertRuleNotificationStore{} + sut := createReceiverServiceSut(t, &secretsService) + sut.ruleNotificationsStore = ruleStore + baseReceiver.Name = "" + + _, err := sut.UpdateReceiver(context.Background(), &baseReceiver, nil, writer.GetOrgID(), writer) + require.ErrorIs(t, err, legacy_storage.ErrReceiverInvalid) + }) } func TestReceiverServiceAC_Read(t *testing.T) { diff --git a/pkg/services/ngalert/provisioning/contactpoints.go b/pkg/services/ngalert/provisioning/contactpoints.go index d25aa4e959f..14abb8ceaa5 100644 --- a/pkg/services/ngalert/provisioning/contactpoints.go +++ b/pkg/services/ngalert/provisioning/contactpoints.go @@ -309,13 +309,14 @@ func (ecp *ContactPointService) UpdateContactPoint(ctx context.Context, orgID in return err } - oldReceiverName, fullRemoval, newReceiverCreated := stitchReceiver(revision.Config, mergedReceiver) - if oldReceiverName == "" { - return fmt.Errorf("contact point with uid '%s' not found", mergedReceiver.UID) + oldReceiverNameRef, fullRemoval, newReceiverCreated := stitchReceiver(revision.Config, mergedReceiver) + if oldReceiverNameRef == nil { + return fmt.Errorf("%w: contact point with uid '%s' not found", ErrNotFound, mergedReceiver.UID) } + oldReceiverName := *oldReceiverNameRef err = ecp.xact.InTransaction(ctx, func(ctx context.Context) error { - if mergedReceiver.Name != oldReceiverName { + if mergedReceiver.Name != oldReceiverName && oldReceiverName != "" { if newReceiverCreated { // Copy receiver permissions permissionsUpdated, err := ecp.resourcePermissions.CopyPermissions(ctx, orgID, nil, legacy_storage.NameToUid(oldReceiverName), legacy_storage.NameToUid(mergedReceiver.Name)) @@ -373,12 +374,12 @@ func (ecp *ContactPointService) DeleteContactPoint(ctx context.Context, orgID in } } } - if fullRemoval && revision.ReceiverNameUsedByRoutes(name) { + if fullRemoval && name != "" && revision.ReceiverNameUsedByRoutes(name) { return ErrContactPointReferenced.Errorf("") } return ecp.xact.InTransaction(ctx, func(ctx context.Context) error { - if fullRemoval { + if fullRemoval && name != "" { used, err := ecp.notificationSettingsStore.ListNotificationSettings(ctx, models.ListNotificationSettingsQuery{OrgID: orgID, ReceiverName: name}) if err != nil { return fmt.Errorf("failed to query alert rules for reference to the contact point '%s': %w", name, err) @@ -443,7 +444,7 @@ func (ecp *ContactPointService) encryptValue(value string) (string, error) { // stitchReceiver modifies a receiver, target, in an alertmanager configStore. It modifies the given configStore in-place. // Returns true if the configStore was altered in any way, and false otherwise. // If integration was moved to another group and it was the last in the previous group, the second parameter contains the name of the old group that is gone -func stitchReceiver(cfg *apimodels.PostableUserConfig, target *apimodels.PostableGrafanaReceiver) (oldReceiverName string, fullRemoval bool, newReceiverCreated bool) { +func stitchReceiver(cfg *apimodels.PostableUserConfig, target *apimodels.PostableGrafanaReceiver) (oldReceiverName *string, fullRemoval bool, newReceiverCreated bool) { // Algorithm to fix up receivers. Receivers are very complex and depend heavily on internal consistency. // All receivers in a given receiver group have the same name. We must maintain this across renames. groupLoop: @@ -451,7 +452,8 @@ groupLoop: // Does the current group contain the grafana receiver we're interested in? for i, grafanaReceiver := range receiverGroup.GrafanaManagedReceivers { if grafanaReceiver.UID == target.UID { - oldReceiverName = receiverGroup.Name + name := receiverGroup.Name + oldReceiverName = &name // If it's a basic field change, simply replace it. Done! // // NOTE: @@ -519,6 +521,9 @@ groupLoop: } func ValidateContactPoint(ctx context.Context, e *apimodels.EmbeddedContactPoint, decryptFunc alertingNotify.GetDecryptedValueFn) error { + if e.Name == "" { + return errors.New("name is required") + } iType, err := alertingNotify.IntegrationTypeFromString(e.Type) if err != nil { return err diff --git a/pkg/services/ngalert/provisioning/contactpoints_test.go b/pkg/services/ngalert/provisioning/contactpoints_test.go index 34535c1a504..ddddadb1104 100644 --- a/pkg/services/ngalert/provisioning/contactpoints_test.go +++ b/pkg/services/ngalert/provisioning/contactpoints_test.go @@ -114,16 +114,6 @@ func TestIntegrationContactPointService(t *testing.T) { require.Equal(t, customUID, cps[0].UID) }) - t.Run("it's not possible to use invalid UID", func(t *testing.T) { - customUID := strings.Repeat("1", util.MaxUIDLength+1) - sut := createContactPointServiceSut(t, secretsService) - newCp := createTestContactPoint() - newCp.UID = customUID - - _, err := sut.CreateContactPoint(context.Background(), 1, redactedUser, newCp, models.ProvenanceAPI) - require.ErrorIs(t, err, ErrValidation) - }) - t.Run("it's not possible to use the same uid twice", func(t *testing.T) { customUID := "1337" sut := createContactPointServiceSut(t, secretsService) @@ -137,14 +127,40 @@ func TestIntegrationContactPointService(t *testing.T) { require.Error(t, err) }) - t.Run("create rejects contact points that fail validation", func(t *testing.T) { + t.Run("create rejects invalid contact points", func(t *testing.T) { sut := createContactPointServiceSut(t, secretsService) - newCp := createTestContactPoint() - newCp.Type = "" + testCases := []struct { + name string + cp func(*definitions.EmbeddedContactPoint) + }{ + { + name: "empty type", + cp: func(cp *definitions.EmbeddedContactPoint) { + cp.Type = "" + }, + }, + { + name: "empty name", + cp: func(cp *definitions.EmbeddedContactPoint) { + cp.Name = "" + }, + }, + { + name: "invalid UID", + cp: func(cp *definitions.EmbeddedContactPoint) { + cp.UID = strings.Repeat("1", util.MaxUIDLength+1) + }, + }, + } - _, err := sut.CreateContactPoint(context.Background(), 1, redactedUser, newCp, models.ProvenanceAPI) - - require.ErrorIs(t, err, ErrValidation) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + newCp := createTestContactPoint() + tc.cp(&newCp) + _, err := sut.CreateContactPoint(context.Background(), 1, redactedUser, newCp, models.ProvenanceAPI) + require.ErrorIs(t, err, ErrValidation) + }) + } }) t.Run("create accepts contact point with type in different cases", func(t *testing.T) { @@ -162,40 +178,57 @@ func TestIntegrationContactPointService(t *testing.T) { assert.EqualValues(t, slack.Type, got[0].Type) }) - t.Run("update rejects contact points with no settings", func(t *testing.T) { + t.Run("update rejects invalid contact points", func(t *testing.T) { + testCases := []struct { + name string + cp func(*definitions.EmbeddedContactPoint) + }{ + { + name: "empty type", + cp: func(cp *definitions.EmbeddedContactPoint) { + cp.Type = "" + }, + }, + { + name: "empty name", + cp: func(cp *definitions.EmbeddedContactPoint) { + cp.Name = "" + }, + }, + { + name: "nil settings", + cp: func(cp *definitions.EmbeddedContactPoint) { + cp.Settings = nil + }, + }, + { + name: "invalid settings after merge", + cp: func(cp *definitions.EmbeddedContactPoint) { + cp.Settings, _ = simplejson.NewJson([]byte(`{}`)) + }, + }, + } + sut := createContactPointServiceSut(t, secretsService) newCp := createTestContactPoint() newCp, err := sut.CreateContactPoint(context.Background(), 1, redactedUser, newCp, models.ProvenanceAPI) require.NoError(t, err) - newCp.Settings = nil - err = sut.UpdateContactPoint(context.Background(), 1, newCp, models.ProvenanceAPI) - - require.ErrorIs(t, err, ErrValidation) - }) - - t.Run("update rejects contact points with no type", func(t *testing.T) { - sut := createContactPointServiceSut(t, secretsService) - newCp := createTestContactPoint() - newCp, err := sut.CreateContactPoint(context.Background(), 1, redactedUser, newCp, models.ProvenanceAPI) - require.NoError(t, err) - newCp.Type = "" - - err = sut.UpdateContactPoint(context.Background(), 1, newCp, models.ProvenanceAPI) - - require.ErrorIs(t, err, ErrValidation) - }) - - t.Run("update rejects contact points which fail validation after merging", func(t *testing.T) { - sut := createContactPointServiceSut(t, secretsService) - newCp := createTestContactPoint() - newCp, err := sut.CreateContactPoint(context.Background(), 1, redactedUser, newCp, models.ProvenanceAPI) - require.NoError(t, err) - newCp.Settings, _ = simplejson.NewJson([]byte(`{}`)) - - err = sut.UpdateContactPoint(context.Background(), 1, newCp, models.ProvenanceAPI) - - require.ErrorIs(t, err, ErrValidation) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + cp := definitions.EmbeddedContactPoint{ + UID: newCp.UID, + Name: newCp.Name, + Type: newCp.Type, + Settings: newCp.Settings.DeepCopy(), + DisableResolveMessage: newCp.DisableResolveMessage, + Provenance: newCp.Provenance, + } + tc.cp(&cp) + err = sut.UpdateContactPoint(context.Background(), 1, cp, models.ProvenanceAPI) + require.ErrorIs(t, err, ErrValidation) + }) + } }) t.Run("update accepts contact points with type in another case", func(t *testing.T) { @@ -592,7 +625,7 @@ func TestStitchReceivers(t *testing.T) { initial *definitions.PostableUserConfig new *definitions.PostableGrafanaReceiver expCfg definitions.PostableApiAlertingConfig - expOldReceiver string + expOldReceiver *string expCreatedReceiver bool expFullRemoval bool } @@ -603,7 +636,7 @@ func TestStitchReceivers(t *testing.T) { new: &definitions.PostableGrafanaReceiver{ UID: "does not exist", }, - expOldReceiver: "", + expOldReceiver: nil, expCfg: createTestConfigWithReceivers().AlertmanagerConfig, }, { @@ -613,7 +646,7 @@ func TestStitchReceivers(t *testing.T) { Name: "receiver-2", Type: "teams", }, - expOldReceiver: "receiver-2", + expOldReceiver: util.Pointer("receiver-2"), expCfg: definitions.PostableApiAlertingConfig{ Config: definitions.Config{ Route: &definitions.Route{ @@ -674,7 +707,7 @@ func TestStitchReceivers(t *testing.T) { Name: "new-receiver", Type: "slack", }, - expOldReceiver: "receiver-1", + expOldReceiver: util.Pointer("receiver-1"), expCreatedReceiver: true, expFullRemoval: true, expCfg: definitions.PostableApiAlertingConfig{ @@ -737,7 +770,7 @@ func TestStitchReceivers(t *testing.T) { Name: "receiver-1", Type: "slack", }, - expOldReceiver: "receiver-2", + expOldReceiver: util.Pointer("receiver-2"), expCreatedReceiver: false, expCfg: definitions.PostableApiAlertingConfig{ Config: definitions.Config{ @@ -858,7 +891,7 @@ func TestStitchReceivers(t *testing.T) { Name: "receiver-2", Type: "slack", }, - expOldReceiver: "receiver-1", + expOldReceiver: util.Pointer("receiver-1"), expCreatedReceiver: false, expCfg: definitions.PostableApiAlertingConfig{ Config: definitions.Config{ @@ -1002,7 +1035,7 @@ func TestStitchReceivers(t *testing.T) { Name: "receiver-4", Type: "slack", }, - expOldReceiver: "receiver-1", + expOldReceiver: util.Pointer("receiver-1"), expCreatedReceiver: false, expCfg: definitions.PostableApiAlertingConfig{ Config: definitions.Config{ @@ -1087,7 +1120,7 @@ func TestStitchReceivers(t *testing.T) { Name: "brand-new-group", Type: "opsgenie", }, - expOldReceiver: "receiver-2", + expOldReceiver: util.Pointer("receiver-2"), expCreatedReceiver: true, expCfg: definitions.PostableApiAlertingConfig{ Config: definitions.Config{ @@ -1159,7 +1192,7 @@ func TestStitchReceivers(t *testing.T) { Name: "brand-new-group", Type: "opsgenie", }, - expOldReceiver: "receiver-2", // Not the inconsistent receiver-3? + expOldReceiver: util.Pointer("receiver-2"), // Not the inconsistent receiver-3? expCreatedReceiver: true, expCfg: definitions.PostableApiAlertingConfig{ Config: definitions.Config{ @@ -1282,7 +1315,7 @@ func TestStitchReceivers(t *testing.T) { Name: "receiver-1", Type: "slack", }, - expOldReceiver: "receiver-2", + expOldReceiver: util.Pointer("receiver-2"), expCreatedReceiver: false, expFullRemoval: true, expCfg: definitions.PostableApiAlertingConfig{ @@ -1337,7 +1370,7 @@ func TestStitchReceivers(t *testing.T) { } renamedReceiver, fullRemoval, createdReceiver := stitchReceiver(cfg, c.new) - assert.Equalf(t, c.expOldReceiver, renamedReceiver, "expected old receiver to be %s, got %s", c.expOldReceiver, renamedReceiver) + assert.EqualValuesf(t, c.expOldReceiver, renamedReceiver, "expected old receiver to be %v, got %v", c.expOldReceiver, renamedReceiver) assert.Equalf(t, c.expFullRemoval, fullRemoval, "expected full removal to be %t, got %t", c.expFullRemoval, fullRemoval) assert.Equalf(t, c.expCreatedReceiver, createdReceiver, "expected created receiver to be %t, got %t", c.expCreatedReceiver, createdReceiver) require.Equal(t, c.expCfg, cfg.AlertmanagerConfig)