From e133492ed413438f6a81cd956c8d7712f83f1579 Mon Sep 17 00:00:00 2001 From: Kevin Minehart <5140827+kminehart@users.noreply.github.com> Date: Tue, 16 Dec 2025 17:00:54 +0100 Subject: [PATCH] [release-12.0.8] Alerting: Fix contact points issue (#115410) * Alerting: Protect sensitive fields of contact points from unauthorized modification - Introduce a new permission alert.notifications.receivers.protected:write. The permission is granted to contact point administrators. - Introduce field Protected to NotifierOption - Introduce DiffReport for models.Integrations with focus on Settings. The diff report is extended with methods that return all keys that are different between two settings. - Add new annotation 'grafana.com/access/CanModifyProtected' to Receiver model - Update receiver service to enforce the permission and return status 403 if unauthorized user modifies protected field - Update receiver testing APIs to enforce permission and return status 403 if unauthorized user modifies protected field. - Update UI to disable protected fields if user cannot modify them Co-authored-by: Sonia Aguilar * fix linter error * prettier:write --------- Co-authored-by: Yuri Tseretyan Co-authored-by: Sonia Aguilar --- .betterer.results | 20 +- .../notifications/receiver/conversions.go | 9 +- pkg/services/accesscontrol/models.go | 1 + .../ossaccesscontrol/receivers.go | 2 +- pkg/services/ngalert/accesscontrol.go | 3 +- .../ngalert/accesscontrol/receivers.go | 64 ++- .../ngalert/accesscontrol/receivers_test.go | 35 +- pkg/services/ngalert/api/api_alertmanager.go | 6 +- pkg/services/ngalert/models/permissions.go | 10 +- pkg/services/ngalert/models/receivers.go | 189 +++++++- pkg/services/ngalert/models/receivers_test.go | 242 ++++++++++ pkg/services/ngalert/models/testing.go | 6 + .../ngalert/notifier/alertmanager_config.go | 2 +- .../channels_config/available_channels.go | 20 +- .../notifier/channels_config/plugin.go | 1 + pkg/services/ngalert/notifier/crypto.go | 44 +- pkg/services/ngalert/notifier/receiver_svc.go | 36 ++ .../ngalert/notifier/receiver_svc_test.go | 58 ++- .../migrations/accesscontrol/alerting.go | 46 ++ .../sqlstore/migrations/migrations.go | 2 + .../notifications/receivers/receiver_test.go | 104 ++-- .../contact-points/EditContactPoint.test.tsx | 2 +- .../contact-points/useContactPoints.ts | 55 +-- .../components/receivers/GlobalConfigForm.tsx | 1 + .../NewReceiverView.test.tsx.snap | 2 +- .../receivers/form/ChannelOptions.tsx | 115 ++++- .../receivers/form/ChannelSubForm.test.tsx | 249 ++++++++++ .../receivers/form/ChannelSubForm.tsx | 185 +++++--- .../receivers/form/CloudReceiverForm.tsx | 1 + .../receivers/form/GrafanaReceiverForm.tsx | 23 +- .../receivers/form/ReceiverForm.tsx | 36 +- .../GrafanaReceiverForm.test.tsx.snap | 6 +- .../form/fields/OptionField.test.tsx | 444 ++++++++++++++++++ .../receivers/form/fields/OptionField.tsx | 142 +++--- .../form/fields/SubformArrayField.tsx | 16 +- .../receivers/form/fields/SubformField.tsx | 28 +- .../components/receivers/form/util.test.ts | 1 - .../alerting/unified/mockGrafanaNotifiers.ts | 5 + .../alerting/unified/types/receiver-form.ts | 3 +- .../__snapshots__/receiver-form.test.ts.snap | 4 +- .../alerting/unified/utils/k8s/constants.ts | 2 + .../alerting/unified/utils/k8s/utils.ts | 3 + .../unified/utils/receiver-form.test.ts | 33 +- .../alerting/unified/utils/receiver-form.ts | 76 +-- .../plugins/datasource/alertmanager/types.ts | 9 +- public/app/types/accessControl.ts | 1 + public/app/types/alerting.ts | 14 + public/locales/en-US/grafana.json | 10 + 48 files changed, 1939 insertions(+), 427 deletions(-) create mode 100644 public/app/features/alerting/unified/components/receivers/form/ChannelSubForm.test.tsx create mode 100644 public/app/features/alerting/unified/components/receivers/form/fields/OptionField.test.tsx diff --git a/.betterer.results b/.betterer.results index 5c947a9303a..a9ad4b3f6f6 100644 --- a/.betterer.results +++ b/.betterer.results @@ -1126,12 +1126,7 @@ exports[`better eslint`] = { [0, 0, 0, "No untranslated strings. Wrap text with ", "2"] ], "public/app/features/alerting/unified/components/receivers/form/ChannelOptions.tsx:5381": [ - [0, 0, 0, "Do not use any type assertions.", "0"], - [0, 0, 0, "Unexpected any. Specify a different type.", "1"], - [0, 0, 0, "Unexpected any. Specify a different type.", "2"] - ], - "public/app/features/alerting/unified/components/receivers/form/ChannelSubForm.tsx:5381": [ - [0, 0, 0, "No untranslated strings in text props. Wrap text with or use t()", "0"] + [0, 0, 0, "Do not use any type assertions.", "0"] ], "public/app/features/alerting/unified/components/receivers/form/CloudReceiverForm.tsx:5381": [ [0, 0, 0, "No untranslated strings. Wrap text with ", "0"] @@ -1142,12 +1137,6 @@ exports[`better eslint`] = { "public/app/features/alerting/unified/components/receivers/form/GrafanaReceiverForm.tsx:5381": [ [0, 0, 0, "No untranslated strings. Wrap text with ", "0"] ], - "public/app/features/alerting/unified/components/receivers/form/ReceiverForm.tsx:5381": [ - [0, 0, 0, "Do not use any type assertions.", "0"], - [0, 0, 0, "Do not use any type assertions.", "1"], - [0, 0, 0, "No untranslated strings. Wrap text with ", "2"], - [0, 0, 0, "Unexpected any. Specify a different type.", "3"] - ], "public/app/features/alerting/unified/components/receivers/form/TestContactPointModal.tsx:5381": [ [0, 0, 0, "No untranslated strings. Wrap text with ", "0"], [0, 0, 0, "No untranslated strings. Wrap text with ", "1"] @@ -1156,9 +1145,7 @@ exports[`better eslint`] = { [0, 0, 0, "Do not use any type assertions.", "0"], [0, 0, 0, "Unexpected any. Specify a different type.", "1"], [0, 0, 0, "Unexpected any. Specify a different type.", "2"], - [0, 0, 0, "Unexpected any. Specify a different type.", "3"], - [0, 0, 0, "Unexpected any. Specify a different type.", "4"], - [0, 0, 0, "Unexpected any. Specify a different type.", "5"] + [0, 0, 0, "Unexpected any. Specify a different type.", "3"] ], "public/app/features/alerting/unified/components/receivers/form/fields/SubformArrayField.tsx:5381": [ [0, 0, 0, "No untranslated strings in text props. Wrap text with or use t()", "0"], @@ -1378,8 +1365,7 @@ exports[`better eslint`] = { [0, 0, 0, "No untranslated strings. Wrap text with ", "1"] ], "public/app/features/alerting/unified/types/receiver-form.ts:5381": [ - [0, 0, 0, "Unexpected any. Specify a different type.", "0"], - [0, 0, 0, "Unexpected any. Specify a different type.", "1"] + [0, 0, 0, "Unexpected any. Specify a different type.", "0"] ], "public/app/features/alerting/unified/utils/misc.test.ts:5381": [ [0, 0, 0, "Unexpected any. Specify a different type.", "0"], diff --git a/pkg/registry/apis/alerting/notifications/receiver/conversions.go b/pkg/registry/apis/alerting/notifications/receiver/conversions.go index 6ba2b6b5975..91e30674a83 100644 --- a/pkg/registry/apis/alerting/notifications/receiver/conversions.go +++ b/pkg/registry/apis/alerting/notifications/receiver/conversions.go @@ -105,10 +105,11 @@ func convertToK8sResource( } var permissionMapper = map[ngmodels.ReceiverPermission]string{ - ngmodels.ReceiverPermissionReadSecret: "canReadSecrets", - ngmodels.ReceiverPermissionAdmin: "canAdmin", - ngmodels.ReceiverPermissionWrite: "canWrite", - ngmodels.ReceiverPermissionDelete: "canDelete", + ngmodels.ReceiverPermissionReadSecret: "canReadSecrets", + ngmodels.ReceiverPermissionAdmin: "canAdmin", + ngmodels.ReceiverPermissionWrite: "canWrite", + ngmodels.ReceiverPermissionDelete: "canDelete", + ngmodels.ReceiverPermissionModifyProtected: "canModifyProtected", } func convertToDomainModel(receiver *model.Receiver) (*ngmodels.Receiver, map[string][]string, error) { diff --git a/pkg/services/accesscontrol/models.go b/pkg/services/accesscontrol/models.go index dc12171eaa5..73e90f528e5 100644 --- a/pkg/services/accesscontrol/models.go +++ b/pkg/services/accesscontrol/models.go @@ -458,6 +458,7 @@ const ( ActionAlertingReceiversReadSecrets = "alert.notifications.receivers.secrets:read" ActionAlertingReceiversCreate = "alert.notifications.receivers:create" ActionAlertingReceiversUpdate = "alert.notifications.receivers:write" + ActionAlertingReceiversUpdateProtected = "alert.notifications.receivers.protected:write" ActionAlertingReceiversDelete = "alert.notifications.receivers:delete" ActionAlertingReceiversTest = "alert.notifications.receivers:test" ActionAlertingReceiversPermissionsRead = "receivers.permissions:read" diff --git a/pkg/services/accesscontrol/ossaccesscontrol/receivers.go b/pkg/services/accesscontrol/ossaccesscontrol/receivers.go index 3ef4638b3dc..03d81818fa7 100644 --- a/pkg/services/accesscontrol/ossaccesscontrol/receivers.go +++ b/pkg/services/accesscontrol/ossaccesscontrol/receivers.go @@ -24,7 +24,7 @@ import ( var ReceiversViewActions = []string{accesscontrol.ActionAlertingReceiversRead} var ReceiversEditActions = append(ReceiversViewActions, []string{accesscontrol.ActionAlertingReceiversUpdate, accesscontrol.ActionAlertingReceiversDelete}...) -var ReceiversAdminActions = append(ReceiversEditActions, []string{accesscontrol.ActionAlertingReceiversReadSecrets, accesscontrol.ActionAlertingReceiversPermissionsRead, accesscontrol.ActionAlertingReceiversPermissionsWrite}...) +var ReceiversAdminActions = append(ReceiversEditActions, []string{accesscontrol.ActionAlertingReceiversReadSecrets, accesscontrol.ActionAlertingReceiversPermissionsRead, accesscontrol.ActionAlertingReceiversPermissionsWrite, accesscontrol.ActionAlertingReceiversUpdateProtected}...) // defaultPermissions returns the default permissions for a newly created receiver. func defaultPermissions() []accesscontrol.SetResourcePermissionCommand { diff --git a/pkg/services/ngalert/accesscontrol.go b/pkg/services/ngalert/accesscontrol.go index 259db2bd5a9..82fe1483e15 100644 --- a/pkg/services/ngalert/accesscontrol.go +++ b/pkg/services/ngalert/accesscontrol.go @@ -290,12 +290,13 @@ var ( Role: accesscontrol.RoleDTO{ Name: accesscontrol.FixedRolePrefix + "alerting:admin", DisplayName: "Full admin access", - Description: "Full write access in Grafana and all external providers, including their permissions and secrets", + Description: "Full write access in Grafana and all external providers, including their permissions, protected fields and secrets", Group: AlertRolesGroup, Permissions: accesscontrol.ConcatPermissions(alertingWriterRole.Role.Permissions, []accesscontrol.Permission{ {Action: accesscontrol.ActionAlertingReceiversPermissionsRead, Scope: ac.ScopeReceiversAll}, {Action: accesscontrol.ActionAlertingReceiversPermissionsWrite, Scope: ac.ScopeReceiversAll}, {Action: accesscontrol.ActionAlertingReceiversReadSecrets, Scope: ac.ScopeReceiversAll}, + {Action: accesscontrol.ActionAlertingReceiversUpdateProtected, Scope: ac.ScopeReceiversAll}, }), }, Grants: []string{string(org.RoleAdmin)}, diff --git a/pkg/services/ngalert/accesscontrol/receivers.go b/pkg/services/ngalert/accesscontrol/receivers.go index 1016f246a41..0f2b36e4086 100644 --- a/pkg/services/ngalert/accesscontrol/receivers.go +++ b/pkg/services/ngalert/accesscontrol/receivers.go @@ -137,6 +137,26 @@ var ( ) } + // Asserts pre-conditions for access to modify protected fields of receivers. If this evaluates to false, the user cannot modify protected fields of any receivers. + updateReceiversProtectedPreConditionsEval = ac.EvalAll( + updateReceiversPreConditionsEval, + ac.EvalPermission(ac.ActionAlertingReceiversUpdateProtected), // Action for receivers. UID scope. + ) + + // Asserts access to modify protected fields of a specific receiver. + updateReceiverProtectedEval = func(uid string) ac.Evaluator { + return ac.EvalAll( + updateReceiverEval(uid), + ac.EvalPermission(ac.ActionAlertingReceiversUpdateProtected, ScopeReceiversProvider.GetResourceScopeUID(uid)), + ) + } + + // Asserts access to modify protected fields of all receivers. + updateAllReceiverProtectedEval = ac.EvalAll( + updateAllReceiversEval, + ac.EvalPermission(ac.ActionAlertingReceiversUpdateProtected, ScopeReceiversAll), + ) + // Delete // Asserts pre-conditions for delete access to receivers. If this evaluates to false, the user cannot delete any receivers. @@ -183,12 +203,13 @@ var ( ) type ReceiverAccess[T models.Identified] struct { - read actionAccess[T] - readDecrypted actionAccess[T] - create actionAccess[T] - update actionAccess[T] - delete actionAccess[T] - permissions actionAccess[T] + read actionAccess[T] + readDecrypted actionAccess[T] + create actionAccess[T] + update actionAccess[T] + updateProtected actionAccess[T] + delete actionAccess[T] + permissions actionAccess[T] } // NewReceiverAccess creates a new ReceiverAccess service. If includeProvisioningActions is true, the service will include @@ -243,6 +264,18 @@ func NewReceiverAccess[T models.Identified](a ac.AccessControl, includeProvision }, authorizeAll: updateAllReceiversEval, }, + updateProtected: actionAccess[T]{ + genericService: genericService{ + ac: a, + }, + resource: "receiver", + action: "update protected fields of", // this produces message "user is not authorized to update protected fields of X receiver" + authorizeSome: updateReceiversProtectedPreConditionsEval, + authorizeOne: func(receiver models.Identified) ac.Evaluator { + return updateReceiverProtectedEval(receiver.GetUID()) + }, + authorizeAll: updateAllReceiverProtectedEval, + }, delete: actionAccess[T]{ genericService: genericService{ ac: a, @@ -353,6 +386,14 @@ func (s ReceiverAccess[T]) AuthorizeUpdate(ctx context.Context, user identity.Re return s.update.Authorize(ctx, user, receiver) } +func (s ReceiverAccess[T]) HasUpdateProtected(ctx context.Context, user identity.Requester, receiver T) (bool, error) { + return s.updateProtected.Has(ctx, user, receiver) +} + +func (s ReceiverAccess[T]) AuthorizeUpdateProtected(ctx context.Context, user identity.Requester, receiver T) error { + return s.updateProtected.Authorize(ctx, user, receiver) +} + // Global // AuthorizeCreate checks if user has access to create receivers. Returns an error if user does not have access. @@ -422,6 +463,12 @@ func (s ReceiverAccess[T]) Access(ctx context.Context, user identity.Requester, basePerms.Set(models.ReceiverPermissionDelete, true) // Has access to all receivers. } + if err := s.updateProtected.AuthorizePreConditions(ctx, user); err != nil { + basePerms.Set(models.ReceiverPermissionModifyProtected, false) + } else if err := s.updateProtected.AuthorizeAll(ctx, user); err == nil { + basePerms.Set(models.ReceiverPermissionModifyProtected, true) + } + if basePerms.AllSet() { // Shortcut for the case when all permissions are known based on preconditions. result := make(map[string]models.ReceiverPermissionSet, len(receivers)) @@ -454,6 +501,11 @@ func (s ReceiverAccess[T]) Access(ctx context.Context, user identity.Requester, permSet.Set(models.ReceiverPermissionDelete, err == nil) } + if _, ok := permSet.Has(models.ReceiverPermissionModifyProtected); !ok { + err := s.updateProtected.authorize(ctx, user, rcv) + permSet.Set(models.ReceiverPermissionModifyProtected, err == nil) + } + result[rcv.GetUID()] = permSet } return result, nil diff --git a/pkg/services/ngalert/accesscontrol/receivers_test.go b/pkg/services/ngalert/accesscontrol/receivers_test.go index 6ad2e1db463..c9af7f85915 100644 --- a/pkg/services/ngalert/accesscontrol/receivers_test.go +++ b/pkg/services/ngalert/accesscontrol/receivers_test.go @@ -132,7 +132,7 @@ func TestReceiverAccess(t *testing.T) { recv3.UID: permissions(), }, }, - //{ + // { // name: "legacy global notifications provisioning writer should have full write on provisioning only", // user: newViewUser(ac.Permission{Action: ac.ActionAlertingNotificationsProvisioningWrite}), // expected: map[string]models.ReceiverPermissionSet{ @@ -145,8 +145,8 @@ func TestReceiverAccess(t *testing.T) { // recv2.UID: permissions(models.ReceiverPermissionWrite, models.ReceiverPermissionDelete), // recv3.UID: permissions(models.ReceiverPermissionWrite, models.ReceiverPermissionDelete), // }, - //}, - //{ + // }, + // { // name: "legacy global provisioning writer should have full write on provisioning only", // user: newViewUser(ac.Permission{Action: ac.ActionAlertingProvisioningWrite}), // expected: map[string]models.ReceiverPermissionSet{ @@ -159,7 +159,7 @@ func TestReceiverAccess(t *testing.T) { // recv2.UID: permissions(models.ReceiverPermissionWrite, models.ReceiverPermissionDelete), // recv3.UID: permissions(models.ReceiverPermissionWrite, models.ReceiverPermissionDelete), // }, - //}, + // }, // Receiver create { name: "receiver create should not have write", @@ -204,6 +204,33 @@ func TestReceiverAccess(t *testing.T) { recv3.UID: permissions(), }, }, + { + name: "update protected cannot update receivers", + user: newEmptyUser( + ac.Permission{Action: ac.ActionAlertingReceiversRead, Scope: ScopeReceiversAll}, + ac.Permission{Action: ac.ActionAlertingReceiversUpdateProtected, Scope: ScopeReceiversAll}, + ), + expected: map[string]models.ReceiverPermissionSet{ + recv1.UID: permissions(), + recv2.UID: permissions(), + recv3.UID: permissions(), + }, + }, + { + name: "update protected receivers", + user: newEmptyUser( + ac.Permission{Action: ac.ActionAlertingReceiversRead, Scope: ScopeReceiversAll}, + ac.Permission{Action: ac.ActionAlertingReceiversUpdateProtected, Scope: ScopeReceiversProvider.GetResourceScopeUID(recv1.UID)}, + ac.Permission{Action: ac.ActionAlertingReceiversUpdate, Scope: ScopeReceiversProvider.GetResourceScopeUID(recv1.UID)}, + ac.Permission{Action: ac.ActionAlertingReceiversUpdate, Scope: ScopeReceiversProvider.GetResourceScopeUID(recv2.UID)}, + ac.Permission{Action: ac.ActionAlertingReceiversUpdateProtected, Scope: ScopeReceiversProvider.GetResourceScopeUID(recv3.UID)}, + ), + expected: map[string]models.ReceiverPermissionSet{ + recv1.UID: permissions(models.ReceiverPermissionWrite, models.ReceiverPermissionModifyProtected), + recv2.UID: permissions(models.ReceiverPermissionWrite), + recv3.UID: permissions(), + }, + }, // Receiver delete. { name: "global receiver delete should have delete but no write", diff --git a/pkg/services/ngalert/api/api_alertmanager.go b/pkg/services/ngalert/api/api_alertmanager.go index cdba800aab8..b55cd86b532 100644 --- a/pkg/services/ngalert/api/api_alertmanager.go +++ b/pkg/services/ngalert/api/api_alertmanager.go @@ -18,6 +18,7 @@ import ( contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/featuremgmt" apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" + "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/ngalert/notifier" "github.com/grafana/grafana/pkg/services/ngalert/notifier/legacy_storage" "github.com/grafana/grafana/pkg/services/ngalert/store" @@ -32,6 +33,7 @@ const ( type receiversAuthz interface { FilterRead(ctx context.Context, user identity.Requester, receivers ...ReceiverStatus) ([]ReceiverStatus, error) + AuthorizeUpdateProtected(context.Context, identity.Requester, ReceiverStatus) error } type AlertmanagerSrv struct { @@ -210,7 +212,9 @@ func (srv AlertmanagerSrv) RouteGetReceivers(c *contextmodel.ReqContext) respons } func (srv AlertmanagerSrv) RoutePostTestReceivers(c *contextmodel.ReqContext, body apimodels.TestReceiversConfigBodyParams) response.Response { - if err := srv.crypto.ProcessSecureSettings(c.Req.Context(), c.GetOrgID(), body.Receivers); err != nil { + if err := srv.crypto.ProcessSecureSettings(c.Req.Context(), c.GetOrgID(), body.Receivers, func(receiverName string, paths []models.IntegrationFieldPath) error { + return srv.receiverAuthz.AuthorizeUpdateProtected(c.Req.Context(), c.SignedInUser, ReceiverStatus{Name: receiverName}) + }); err != nil { var unknownReceiverError UnknownReceiverError if errors.As(err, &unknownReceiverError) { return ErrResp(http.StatusBadRequest, err, "") diff --git a/pkg/services/ngalert/models/permissions.go b/pkg/services/ngalert/models/permissions.go index a34e184fc99..ce52755eac8 100644 --- a/pkg/services/ngalert/models/permissions.go +++ b/pkg/services/ngalert/models/permissions.go @@ -9,10 +9,11 @@ import ( type ReceiverPermission string const ( - ReceiverPermissionReadSecret ReceiverPermission = "secrets" - ReceiverPermissionAdmin ReceiverPermission = "admin" - ReceiverPermissionWrite ReceiverPermission = "write" - ReceiverPermissionDelete ReceiverPermission = "delete" + ReceiverPermissionReadSecret ReceiverPermission = "secrets" + ReceiverPermissionAdmin ReceiverPermission = "admin" + ReceiverPermissionWrite ReceiverPermission = "write" + ReceiverPermissionDelete ReceiverPermission = "delete" + ReceiverPermissionModifyProtected ReceiverPermission = "modify-protected" ) // ReceiverPermissions returns all possible silence permissions. @@ -22,6 +23,7 @@ func ReceiverPermissions() []ReceiverPermission { ReceiverPermissionAdmin, ReceiverPermissionWrite, ReceiverPermissionDelete, + ReceiverPermissionModifyProtected, } } diff --git a/pkg/services/ngalert/models/receivers.go b/pkg/services/ngalert/models/receivers.go index 60d95aaad04..8ae59964b56 100644 --- a/pkg/services/ngalert/models/receivers.go +++ b/pkg/services/ngalert/models/receivers.go @@ -8,13 +8,18 @@ import ( "fmt" "maps" "math" + "reflect" "slices" "sort" "strings" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" alertingNotify "github.com/grafana/alerting/notify" "github.com/grafana/grafana/pkg/services/ngalert/notifier/channels_config" + + "github.com/grafana/grafana/pkg/util/cmputil" ) // GetReceiverQuery represents a query for a single receiver. @@ -161,9 +166,10 @@ type IntegrationConfig struct { // IntegrationField represents a field in an integration configuration. type IntegrationField struct { - Name string - Fields map[string]IntegrationField - Secure bool + Name string + Fields map[string]IntegrationField + Secure bool + Protected bool } type IntegrationFieldPath []string @@ -192,7 +198,11 @@ func (f IntegrationFieldPath) String() string { } func (f IntegrationFieldPath) Append(segment string) IntegrationFieldPath { - return append(f, segment) + // Copy the existing path to avoid modifying the original slice. + newPath := make(IntegrationFieldPath, len(f)+1) + copy(newPath, f) + newPath[len(newPath)-1] = segment + return newPath } // IntegrationConfigFromType returns an integration configuration for a given integration type. If the integration type is @@ -213,9 +223,10 @@ func IntegrationConfigFromType(integrationType string) (IntegrationConfig, error func notifierOptionToIntegrationField(option channels_config.NotifierOption) IntegrationField { f := IntegrationField{ - Name: option.PropertyName, - Secure: option.Secure, - Fields: make(map[string]IntegrationField, len(option.SubformOptions)), + Name: option.PropertyName, + Secure: option.Secure, + Protected: option.Protected, + Fields: make(map[string]IntegrationField, len(option.SubformOptions)), } for _, subformOption := range option.SubformOptions { f.Fields[subformOption.PropertyName] = notifierOptionToIntegrationField(subformOption) @@ -288,9 +299,10 @@ func (field *IntegrationField) GetField(path IntegrationFieldPath) (IntegrationF func (field *IntegrationField) Clone() IntegrationField { f := IntegrationField{ - Name: field.Name, - Secure: field.Secure, - Fields: make(map[string]IntegrationField, len(field.Fields)), + Name: field.Name, + Secure: field.Secure, + Fields: make(map[string]IntegrationField, len(field.Fields)), + Protected: field.Protected, } for subName, sub := range field.Fields { f.Fields[subName] = sub.Clone() @@ -653,3 +665,160 @@ func writeSettings(f fingerprint, m map[string]any) { } } } + +type IntegrationDiffReport struct { + cmputil.DiffReport +} + +// expandPaths recursively collects all sub-paths for keys in the provided map value +func (r IntegrationDiffReport) expandPaths(basePath IntegrationFieldPath, mapVal reflect.Value) []IntegrationFieldPath { + result := make([]IntegrationFieldPath, 0) + iter := mapVal.MapRange() + for iter.Next() { + keyStr := fmt.Sprintf("%v", iter.Key()) // Assume string keys + p := basePath.Append(keyStr) + // Recurse if the sub-value is another map + if m, ok := r.getMap(iter.Value()); ok { + result = append(result, r.expandPaths(p, m)...) + continue + } + result = append(result, p) + } + return result +} + +func (r IntegrationDiffReport) getMap(v reflect.Value) (reflect.Value, bool) { + if v.Kind() == reflect.Map { + return v, true + } + if v.Kind() == reflect.Ptr || v.Kind() == reflect.Interface { + return r.getMap(v.Elem()) + } + return reflect.Value{}, false +} + +func (r IntegrationDiffReport) needExpand(diff cmputil.Diff) (reflect.Value, bool) { + ml, lok := r.getMap(diff.Left) + mr, rok := r.getMap(diff.Right) + if lok == rok { + return reflect.Value{}, false + } + if lok { + return ml, true + } + return mr, true +} + +func (r IntegrationDiffReport) GetSettingsPaths() []IntegrationFieldPath { + diffs := r.GetDiffsForField("Settings") + paths := make([]IntegrationFieldPath, 0, len(diffs)) + for _, diff := range diffs { + // diff.Path has format like Settings[url] or Settings[sub-form][field] + p := diff.Path + var path IntegrationFieldPath + for { + start := strings.Index(p, "[") + if start == -1 { + break + } + p = p[start+1:] + end := strings.Index(p, "]") + if end == -1 { + break + } + fieldName := p[:end] + p = p[end+1:] + path = append(path, fieldName) + } + if m, ok := r.needExpand(diff); ok { + paths = append(paths, r.expandPaths(path, m)...) + continue + } + if len(path) > 0 { + paths = append(paths, path) + } + } + return paths +} + +func (r IntegrationDiffReport) GetSecureSettingsPaths() []IntegrationFieldPath { + diffs := r.GetDiffsForField("SecureSettings") + paths := make([]IntegrationFieldPath, 0, len(diffs)) + for _, diff := range diffs { + if diff.Path == "SecureSettings" { + if m, ok := r.needExpand(diff); ok { + paths = append(paths, r.expandPaths(nil, m)...) + } + continue + } + // diff.Path has format like SecureSettings[field.sub-field.sub] + p := NewIntegrationFieldPath(diff.Path[len("SecureSettings[") : len(diff.Path)-1]) + paths = append(paths, p) + } + return paths +} + +func (integration *Integration) Diff(incoming Integration) IntegrationDiffReport { + var reporter cmputil.DiffReporter + var settingsCmp = cmpopts.AcyclicTransformer("settingsMap", func(in map[string]any) map[string]any { + if in == nil { + return map[string]any{} + } + return in + }) + var secureCmp = cmpopts.AcyclicTransformer("secureMap", func(in map[string]string) map[string]string { + if in == nil { + return map[string]string{} + } + return in + }) + schemaCmp := cmp.Comparer(func(a, b IntegrationConfig) bool { + return a.Type == b.Type + }) + var cur Integration + if integration != nil { + cur = *integration + } + cmp.Equal(cur, incoming, cmp.Reporter(&reporter), settingsCmp, secureCmp, schemaCmp) + return IntegrationDiffReport{DiffReport: reporter.Diffs} +} + +// HasReceiversDifferentProtectedFields returns true if the receiver has any protected fields that are different from the incoming receiver. +func HasReceiversDifferentProtectedFields(existing, incoming *Receiver) map[string][]IntegrationFieldPath { + existingIntegrations := make(map[string]*Integration, len(existing.Integrations)) + for _, integration := range existing.Integrations { + existingIntegrations[integration.UID] = integration + } + + var result = make(map[string][]IntegrationFieldPath) + for _, in := range incoming.Integrations { + if in.UID == "" { + continue + } + ex, ok := existingIntegrations[in.UID] + if !ok { + continue + } + paths := HasIntegrationsDifferentProtectedFields(ex, in) + if len(paths) > 0 { + result[in.UID] = paths + } + } + return result +} + +// HasIntegrationsDifferentProtectedFields returns list of paths to protected fields that are different between two integrations. +func HasIntegrationsDifferentProtectedFields(existing, incoming *Integration) []IntegrationFieldPath { + diff := existing.Diff(*incoming) + // The incoming receiver always has both secret and non-secret fields in Settings. + // So, if it's specified and happens to be sensitive, we consider it changed + var result []IntegrationFieldPath + settingsDiff := diff.GetSettingsPaths() + for _, path := range settingsDiff { + f, _ := incoming.Config.GetField(path) + if f.Protected { + result = append(result, path) + } + } + return result +} diff --git a/pkg/services/ngalert/models/receivers_test.go b/pkg/services/ngalert/models/receivers_test.go index d83f346f0ae..5eea50a91ef 100644 --- a/pkg/services/ngalert/models/receivers_test.go +++ b/pkg/services/ngalert/models/receivers_test.go @@ -2,6 +2,7 @@ package models import ( "reflect" + "slices" "testing" alertingNotify "github.com/grafana/alerting/notify" @@ -408,3 +409,244 @@ func TestReceiver_Fingerprint(t *testing.T) { } }) } + +func TestIntegrationDiff(t *testing.T) { + s := IntegrationConfig{Type: "test"} + a := Integration{ + UID: "test-uid", + Name: "test-name", + Config: s, + DisableResolveMessage: false, + Settings: map[string]any{ + "url": "http://localhost", + "name": 123, + "flag": true, + "child": map[string]any{ + "sub-form-field": "test", + }, + }, + SecureSettings: map[string]string{ + "password": "12345", + "token": "token-12345", + }, + } + + t.Run("no diff if equal", func(t *testing.T) { + result := a.Diff(a) + assert.Empty(t, result) + }) + + t.Run("should deep compare settings", func(t *testing.T) { + b := a + b.Settings = map[string]any{ + "url": "http://localhost:123", + "flag": false, + "child": map[string]any{ + "sub-form-field": "test123", + "sub-child": map[string]any{ + "test": "test", + }, + }, + } + + result := a.Diff(b) + assert.ElementsMatch(t, + []string{"Settings[url]", "Settings[name]", "Settings[flag]", "Settings[child][sub-form-field]", "Settings[child][sub-child]"}, + result.Paths()) + }) + + t.Run("should shallow compare schemas", func(t *testing.T) { + b := a + b.Config = IntegrationConfig{Type: "test2"} + result := a.Diff(b) + assert.ElementsMatch(t, + []string{"Config"}, + result.Paths()) + }) + + t.Run("should compare with zero objects", func(t *testing.T) { + result := a.Diff(Integration{}) + assert.ElementsMatch(t, + []string{ + "UID", + "Name", + "Config", + "Settings[child]", + "Settings[flag]", + "Settings[name]", + "Settings[url]", + "SecureSettings[password]", + "SecureSettings[token]", + }, + result.Paths()) + }) +} + +func TestIntegrationDiffReport_GetSettingsPaths(t *testing.T) { + a := Integration{ + UID: "test-uid", + Name: "test-name", + Config: IntegrationConfig{}, + DisableResolveMessage: false, + Settings: map[string]any{ + "url": "http://localhost", + "child": map[string]any{ + "field": "test", + "sub-child": map[string]any{ + "test": "test", + }, + }, + }, + } + + testCases := []struct { + name string + left map[string]any + right map[string]any + paths []string + }{ + { + name: "empty", + left: map[string]any{}, + right: map[string]any{}, + }, + { + name: "left is empty", + left: map[string]any{}, + right: map[string]any{ + "field": "test", + }, + paths: []string{"field"}, + }, + { + name: "right is empty", + left: map[string]any{ + "field": "test", + }, + right: map[string]any{}, + paths: []string{"field"}, + }, + { + name: "expands nested", + left: map[string]any{ + "field": map[string]any{ + "sub-field": map[string]any{ + "test": "test", + }, + }, + }, + right: map[string]any{ + "another": map[string]any{ + "sub-field": map[string]any{ + "test": "test", + }, + }, + }, + paths: []string{ + "field.sub-field.test", + "another.sub-field.test", + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + b := a + b.Settings = tc.right + a.Settings = tc.left + diff := a.Diff(b) + + actual := diff.GetSettingsPaths() + actualStrings := make([]string, 0, len(actual)) + for _, f := range actual { + actualStrings = append(actualStrings, f.String()) + } + assert.ElementsMatch(t, tc.paths, actualStrings) + }) + } +} + +func TestHasDifferentProtectedFields(t *testing.T) { + m := IntegrationMuts + + testCase := []struct { + name string + existing Integration + incoming Integration + expected map[string][]string + }{ + { + name: "different UID do not match", + existing: IntegrationGen(m.WithUID("existing"), m.WithValidConfig("webhook"))(), + incoming: IntegrationGen( + m.WithValidConfig("webhook"), + m.AddSetting("url", "http://some-other-url"), + m.WithUID("incoming"), + )(), + expected: nil, + }, + { + name: "find url protected", + existing: IntegrationGen(m.WithUID("1"), m.WithValidConfig("webhook"))(), + incoming: IntegrationGen( + m.WithValidConfig("webhook"), + m.AddSetting("url", "http://some-other-url"), + m.WithUID("1"), + )(), + expected: map[string][]string{ + "1": { + "url", + }, + }, + }, + { + name: "secure and protected", // simulate the situation when protected secured field is in secure settings but the incoming one has it in settings + existing: IntegrationGen( + m.WithUID("1"), + m.WithValidConfig("discord"), + m.RemoveSetting("url"), + m.WithSecureSettings(map[string]string{ + "url": "", + }))(), + incoming: IntegrationGen( + m.WithValidConfig("discord"), + m.AddSetting("url", "http://some-other-url"), + m.WithSecureSettings(nil), + m.WithUID("1"), + )(), + expected: map[string][]string{ + "1": { + "url", + }, + }, + }, + } + + for _, tc := range testCase { + t.Run(tc.name, func(t *testing.T) { + existing := &Receiver{ + Integrations: []*Integration{ + &tc.existing, + }, + } + incoming := &Receiver{ + Integrations: []*Integration{ + &tc.incoming, + }, + } + actual := HasReceiversDifferentProtectedFields(existing, incoming) + if len(tc.expected) == 0 { + require.Empty(t, actual) + return + } + actualStrings := make(map[string][]string, len(actual)) + for uid, paths := range actual { + for _, path := range paths { + actualStrings[uid] = append(actualStrings[uid], path.String()) + } + slices.Sort(actualStrings[uid]) + } + assert.EqualValues(t, tc.expected, actualStrings) + }) + } +} diff --git a/pkg/services/ngalert/models/testing.go b/pkg/services/ngalert/models/testing.go index 424144dd0f8..cd465259e55 100644 --- a/pkg/services/ngalert/models/testing.go +++ b/pkg/services/ngalert/models/testing.go @@ -1386,3 +1386,9 @@ func ConvertToRecordingRule(rule *AlertRule) { func nameToUid(name string) string { // Avoid legacy_storage.NameToUid import cycle. return base64.RawURLEncoding.EncodeToString([]byte(name)) } + +func (n IntegrationMutators) RemoveSetting(key string) Mutator[Integration] { + return func(c *Integration) { + delete(c.Settings, key) + } +} diff --git a/pkg/services/ngalert/notifier/alertmanager_config.go b/pkg/services/ngalert/notifier/alertmanager_config.go index 963c7f58ff4..67ff1dc2747 100644 --- a/pkg/services/ngalert/notifier/alertmanager_config.go +++ b/pkg/services/ngalert/notifier/alertmanager_config.go @@ -293,7 +293,7 @@ func (moa *MultiOrgAlertmanager) SaveAndApplyAlertmanagerConfiguration(ctx conte } cleanPermissionsErr := err - if err := moa.Crypto.ProcessSecureSettings(ctx, org, config.AlertmanagerConfig.Receivers); err != nil { + if err := moa.Crypto.ProcessSecureSettings(ctx, org, config.AlertmanagerConfig.Receivers, nil); err != nil { return fmt.Errorf("failed to post process Alertmanager configuration: %w", err) } diff --git a/pkg/services/ngalert/notifier/channels_config/available_channels.go b/pkg/services/ngalert/notifier/channels_config/available_channels.go index ed84d01e231..2486f36c553 100644 --- a/pkg/services/ngalert/notifier/channels_config/available_channels.go +++ b/pkg/services/ngalert/notifier/channels_config/available_channels.go @@ -128,6 +128,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { PropertyName: "url", Required: true, Secure: true, + Protected: true, }, { Label: "Message Type", @@ -174,6 +175,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { Placeholder: "http://localhost:8082", PropertyName: "kafkaRestProxy", Required: true, + Protected: true, }, { Label: "Topic", @@ -374,6 +376,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { InputType: InputTypeText, Placeholder: alertingPagerduty.DefaultURL, PropertyName: "url", + Protected: true, }, }, }, @@ -391,6 +394,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { PropertyName: "url", Required: true, Secure: true, + Protected: true, }, { // New in 8.0. Label: "Message Type", @@ -436,6 +440,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { InputType: InputTypeText, PropertyName: "url", Required: true, + Protected: true, }, { Label: "HTTP Method", @@ -685,6 +690,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { Secure: true, Required: true, DependsOn: "token", + Protected: true, }, { // New in 8.4. Label: "Endpoint URL", @@ -693,6 +699,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { Description: "Optionally provide a custom Slack message API endpoint for non-webhook requests, default is https://slack.com/api/chat.postMessage", Placeholder: "Slack endpoint url", PropertyName: "endpointUrl", + Protected: true, }, { Label: "Color", @@ -732,6 +739,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { Placeholder: "http://sensu-api.local:8080", PropertyName: "url", Required: true, + Protected: true, }, { Label: "API Key", @@ -790,6 +798,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { Placeholder: "Teams incoming webhook url", PropertyName: "url", Required: true, + Protected: true, }, { Label: "Title", @@ -908,6 +917,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { InputType: InputTypeText, PropertyName: "url", Required: true, + Protected: true, }, { Label: "HTTP Method", @@ -1102,6 +1112,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { Secure: true, Required: true, DependsOn: "secret", + Protected: true, }, { Label: "Agent ID", @@ -1187,6 +1198,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { Placeholder: "http://localhost:9093", PropertyName: "url", Required: true, + Protected: true, }, { Label: "Basic Auth User", @@ -1233,6 +1245,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { PropertyName: "url", Required: true, Secure: true, + Protected: true, }, { Label: "Avatar URL", @@ -1262,6 +1275,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { PropertyName: "url", Required: true, Secure: true, + Protected: true, }, { Label: "Title", @@ -1382,6 +1396,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { Description: "The URL of the MQTT broker.", PropertyName: "brokerUrl", Required: true, + Protected: true, }, { Label: "Topic", @@ -1539,6 +1554,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { Placeholder: "https://api.opsgenie.com/v2/alerts", PropertyName: "apiUrl", Required: true, + Protected: true, }, { Label: "Message", @@ -1635,6 +1651,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { Placeholder: "https://api.ciscospark.com/v1/messages", Description: "API endpoint at which we'll send webhooks to.", PropertyName: "api_url", + Protected: true, }, { Label: "Room ID", @@ -1669,7 +1686,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { Type: "sns", Name: "AWS SNS", Description: "Sends notifications to AWS Simple Notification Service", - Heading: "Webex settings", + Heading: "AWS SNS settings", Options: []NotifierOption{ { Label: "The Amazon SNS API URL", @@ -1791,6 +1808,7 @@ func GetAvailableNotifiers() []*NotifierPlugin { PropertyName: "api_url", Description: "Supported v2 or v3 APIs", Required: true, + Protected: true, }, { Label: "HTTP Basic Authentication - Username", diff --git a/pkg/services/ngalert/notifier/channels_config/plugin.go b/pkg/services/ngalert/notifier/channels_config/plugin.go index 5a8b7527604..e1d010fa77f 100644 --- a/pkg/services/ngalert/notifier/channels_config/plugin.go +++ b/pkg/services/ngalert/notifier/channels_config/plugin.go @@ -25,6 +25,7 @@ type NotifierOption struct { Secure bool `json:"secure"` DependsOn string `json:"dependsOn"` SubformOptions []NotifierOption `json:"subformOptions"` + Protected bool `json:"protected"` } // ElementType is the type of element that can be rendered in the frontend. diff --git a/pkg/services/ngalert/notifier/crypto.go b/pkg/services/ngalert/notifier/crypto.go index d38f9d5f1c7..c325aaffaa2 100644 --- a/pkg/services/ngalert/notifier/crypto.go +++ b/pkg/services/ngalert/notifier/crypto.go @@ -9,18 +9,21 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" + "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/ngalert/notifier/channels_config" "github.com/grafana/grafana/pkg/services/ngalert/store" "github.com/grafana/grafana/pkg/services/secrets" ) +type AuthorizeProtectedFn func(uid string, paths []models.IntegrationFieldPath) error + // Crypto allows decryption of Alertmanager Configuration and encryption of arbitrary payloads. type Crypto interface { - LoadSecureSettings(ctx context.Context, orgId int64, receivers []*definitions.PostableApiReceiver) error + LoadSecureSettings(ctx context.Context, orgId int64, receivers []*definitions.PostableApiReceiver, fn AuthorizeProtectedFn) error Encrypt(ctx context.Context, payload []byte, opt secrets.EncryptionOptions) ([]byte, error) getDecryptedSecret(r *definitions.PostableGrafanaReceiver, key string) (string, error) - ProcessSecureSettings(ctx context.Context, orgId int64, recvs []*definitions.PostableApiReceiver) error + ProcessSecureSettings(ctx context.Context, orgId int64, recvs []*definitions.PostableApiReceiver, fn AuthorizeProtectedFn) error } // alertmanagerCrypto implements decryption of Alertmanager configuration and encryption of arbitrary payloads based on Grafana's encryptions. @@ -39,7 +42,7 @@ func NewCrypto(secrets secrets.Service, configs configurationStore, log log.Logg } // ProcessSecureSettings encrypts new secure settings and loads existing secure settings from the database. -func (c *alertmanagerCrypto) ProcessSecureSettings(ctx context.Context, orgId int64, recvs []*definitions.PostableApiReceiver) error { +func (c *alertmanagerCrypto) ProcessSecureSettings(ctx context.Context, orgId int64, recvs []*definitions.PostableApiReceiver, authorizeProtected AuthorizeProtectedFn) error { // First, we encrypt the new or updated secure settings. Then, we load the existing secure settings from the database // and add back any that weren't updated. // We perform these steps in this order to ensure the hash of the secure settings remains stable when no secure @@ -50,7 +53,7 @@ func (c *alertmanagerCrypto) ProcessSecureSettings(ctx context.Context, orgId in return fmt.Errorf("failed to encrypt receivers: %w", err) } - if err := c.LoadSecureSettings(ctx, orgId, recvs); err != nil { + if err := c.LoadSecureSettings(ctx, orgId, recvs, authorizeProtected); err != nil { return err } @@ -152,7 +155,7 @@ func encryptReceiverConfigs(c []*definitions.PostableApiReceiver, encrypt defini } // LoadSecureSettings adds the corresponding unencrypted secrets stored to the list of input receivers. -func (c *alertmanagerCrypto) LoadSecureSettings(ctx context.Context, orgId int64, receivers []*definitions.PostableApiReceiver) error { +func (c *alertmanagerCrypto) LoadSecureSettings(ctx context.Context, orgId int64, receivers []*definitions.PostableApiReceiver, authorizeProtected AuthorizeProtectedFn) error { // Get the last known working configuration. amConfig, err := c.configs.GetLatestAlertmanagerConfiguration(ctx, orgId) if err != nil { @@ -161,10 +164,10 @@ func (c *alertmanagerCrypto) LoadSecureSettings(ctx context.Context, orgId int64 return fmt.Errorf("failed to get latest configuration: %w", err) } } - + var currentConfig *definitions.PostableUserConfig currentReceiverMap := make(map[string]*definitions.PostableGrafanaReceiver) if amConfig != nil { - currentConfig, err := Load([]byte(amConfig.AlertmanagerConfiguration)) + currentConfig, err = Load([]byte(amConfig.AlertmanagerConfiguration)) // If the current config is un-loadable, treat it as if it never existed. Providing a new, valid config should be able to "fix" this state. if err != nil { c.log.Warn("Last known alertmanager configuration was invalid. Overwriting...") @@ -194,6 +197,33 @@ func (c *alertmanagerCrypto) LoadSecureSettings(ctx context.Context, orgId int64 return UnknownReceiverError{UID: gr.UID} } + if authorizeProtected != nil { + incoming, errIn := PostableGrafanaReceiverToIntegration(gr) + existing, errEx := PostableGrafanaReceiverToIntegration(cgmr) + var secure []models.IntegrationFieldPath + authz := true + if errIn == nil && errEx == nil { + secure = models.HasIntegrationsDifferentProtectedFields(existing, incoming) + authz = len(secure) > 0 + } + // if conversion failed, consider there are changes and authorize + if authz && currentConfig != nil { + var receiverName string + NAME: + for _, rcv := range currentConfig.AlertmanagerConfig.Receivers { + for _, intg := range rcv.GrafanaManagedReceivers { + if intg.UID == cgmr.UID { + receiverName = rcv.Name + break NAME + } + } + } + if err := authorizeProtected(receiverName, secure); err != nil { + return err + } + } + } + // Frontend sends only the secure settings that have to be updated // Therefore we have to copy from the last configuration only those secure settings not included in the request for key, encryptedValue := range cgmr.SecureSettings { diff --git a/pkg/services/ngalert/notifier/receiver_svc.go b/pkg/services/ngalert/notifier/receiver_svc.go index a4ba285d311..95bbc47c00f 100644 --- a/pkg/services/ngalert/notifier/receiver_svc.go +++ b/pkg/services/ngalert/notifier/receiver_svc.go @@ -5,6 +5,7 @@ import ( "encoding/base64" "errors" "fmt" + "slices" "strings" "go.opentelemetry.io/otel/attribute" @@ -75,6 +76,9 @@ type receiverAccessControlService interface { AuthorizeUpdate(context.Context, identity.Requester, *models.Receiver) error AuthorizeDeleteByUID(context.Context, identity.Requester, string) error + HasUpdateProtected(context.Context, identity.Requester, *models.Receiver) (bool, error) + AuthorizeUpdateProtected(context.Context, identity.Requester, *models.Receiver) error + Access(ctx context.Context, user identity.Requester, receivers ...*models.Receiver) (map[string]models.ReceiverPermissionSet, error) } @@ -511,6 +515,18 @@ func (rs *ReceiverService) UpdateReceiver(ctx context.Context, r *models.Receive return nil, err } + // if user does not have permissions to update protected, check the diff and return error if there is a change in protected fields + canUpdateProtected, _ := rs.authz.HasUpdateProtected(ctx, user, r) + if !canUpdateProtected { + diff := models.HasReceiversDifferentProtectedFields(existing, r) + if len(diff) > 0 { + err = rs.authz.AuthorizeUpdateProtected(ctx, user, r) + if err != nil { + return nil, makeProtectedFieldsAuthzError(err, diff) + } + } + } + // We need to perform two important steps to process settings on an updated integration: // 1. Encrypt new or updated secret fields as they will arrive in plain text. // 2. For updates, callers do not re-send unchanged secure settings and instead mark them in SecureFields. We need @@ -805,3 +821,23 @@ func (rs *ReceiverService) RenameReceiverInDependentResources(ctx context.Contex } return nil } + +func makeProtectedFieldsAuthzError(err error, diff map[string][]models.IntegrationFieldPath) error { + var authzErr errutil.Error + if !errors.As(err, &authzErr) { + return err + } + if authzErr.PublicPayload == nil { + authzErr.PublicPayload = map[string]interface{}{} + } + fields := make(map[string][]string, len(diff)) + for field, paths := range diff { + fields[field] = make([]string, len(paths)) + for i, path := range paths { + fields[field][i] = path.String() + } + slices.Sort(fields[field]) + } + authzErr.PublicPayload["changed_protected_fields"] = fields + return authzErr +} diff --git a/pkg/services/ngalert/notifier/receiver_svc_test.go b/pkg/services/ngalert/notifier/receiver_svc_test.go index d8b5d019237..db23d56fa07 100644 --- a/pkg/services/ngalert/notifier/receiver_svc_test.go +++ b/pkg/services/ngalert/notifier/receiver_svc_test.go @@ -275,7 +275,7 @@ func TestReceiverService_Delete(t *testing.T) { deleteUID: baseReceiver.UID, callerProvenance: definitions.Provenance(models.ProvenanceFile), existing: util.Pointer(models.CopyReceiverWith(baseReceiver, models.ReceiverMuts.WithProvenance(models.ProvenanceAPI))), - //expectedErr: validation.MakeErrProvenanceChangeNotAllowed(models.ProvenanceAPI, models.ProvenanceFile), + // expectedErr: validation.MakeErrProvenanceChangeNotAllowed(models.ProvenanceAPI, models.ProvenanceFile), }, { name: "delete receiver with optimistic version mismatch fails", @@ -532,8 +532,9 @@ func TestReceiverService_Update(t *testing.T) { writer := &user.SignedInUser{OrgID: 1, Permissions: map[int64]map[string][]string{ 1: { - accesscontrol.ActionAlertingNotificationsWrite: nil, - accesscontrol.ActionAlertingNotificationsRead: nil, + accesscontrol.ActionAlertingNotificationsWrite: nil, + accesscontrol.ActionAlertingNotificationsRead: nil, + accesscontrol.ActionAlertingReceiversUpdateProtected: {ac.ScopeReceiversAll}, }, }} decryptUser := &user.SignedInUser{OrgID: 1, Permissions: map[int64]map[string][]string{ @@ -673,7 +674,7 @@ func TestReceiverService_Update(t *testing.T) { user: writer, receiver: models.CopyReceiverWith(baseReceiver, models.ReceiverMuts.WithProvenance(models.ProvenanceAPI)), existing: util.Pointer(models.CopyReceiverWith(baseReceiver, models.ReceiverMuts.WithProvenance(models.ProvenanceFile))), - //expectedErr: validation.MakeErrProvenanceChangeNotAllowed(models.ProvenanceFile, models.ProvenanceAPI), + // expectedErr: validation.MakeErrProvenanceChangeNotAllowed(models.ProvenanceFile, models.ProvenanceAPI), expectedUpdate: models.CopyReceiverWith(baseReceiver, models.ReceiverMuts.WithProvenance(models.ProvenanceAPI), rm.Encrypted(models.Base64Enrypt)), @@ -1125,7 +1126,7 @@ func TestReceiverServiceAC_Update(t *testing.T) { }, }} - slackIntegration := models.IntegrationGen(models.IntegrationMuts.WithName("test receiver"), models.IntegrationMuts.WithValidConfig("slack")) + slackIntegration := models.IntegrationGen(models.IntegrationMuts.WithName("test receiver"), models.IntegrationMuts.WithValidConfig("webhook")) emailIntegration := models.IntegrationGen(models.IntegrationMuts.WithName("test receiver"), models.IntegrationMuts.WithValidConfig("email")) recv1 := models.ReceiverGen(models.ReceiverMuts.WithName("receiver1"), models.ReceiverMuts.WithIntegrations(slackIntegration(), emailIntegration()))() recv2 := models.ReceiverGen(models.ReceiverMuts.WithName("receiver2"), models.ReceiverMuts.WithIntegrations(slackIntegration(), emailIntegration()))() @@ -1137,8 +1138,8 @@ func TestReceiverServiceAC_Update(t *testing.T) { name string permissions map[string][]string existing []models.Receiver - - hasAccess []models.Receiver + incoming []models.Receiver + hasAccess []models.Receiver }{ { name: "not authorized without permissions", @@ -1226,6 +1227,43 @@ func TestReceiverServiceAC_Update(t *testing.T) { existing: allReceivers(), hasAccess: []models.Receiver{recv1, recv3}, }, + { + name: "protected fields modified without permission", + permissions: map[string][]string{ + accesscontrol.ActionAlertingReceiversUpdate: {ac.ScopeReceiversAll}, + accesscontrol.ActionAlertingReceiversRead: {ac.ScopeReceiversAll}, + }, + existing: []models.Receiver{ + recv1, + }, + incoming: []models.Receiver{ + func() models.Receiver { + f := recv1.Clone() + f.Integrations[0].Settings["url"] = "https://example.com/new" + return f + }(), + }, + hasAccess: nil, + }, + { + name: "protected fields modified with permission", + permissions: map[string][]string{ + accesscontrol.ActionAlertingReceiversUpdate: {ac.ScopeReceiversAll}, + accesscontrol.ActionAlertingReceiversRead: {ac.ScopeReceiversAll}, + accesscontrol.ActionAlertingReceiversUpdateProtected: {ac.ScopeReceiversAll}, + }, + existing: []models.Receiver{ + recv1, + }, + incoming: []models.Receiver{ + func() models.Receiver { + f := recv1.Clone() + f.Integrations[0].Settings["url"] = "https://example.com/new" + return f + }(), + }, + hasAccess: []models.Receiver{recv1}, + }, } for _, tc := range testCases { @@ -1251,7 +1289,11 @@ func TestReceiverServiceAC_Update(t *testing.T) { } return false } - for _, recv := range allReceivers() { + incoming := allReceivers() + if tc.incoming != nil { + incoming = tc.incoming + } + for _, recv := range incoming { clone := recv.Clone() clone.Version = versions[recv.UID] response, err := sut.UpdateReceiver(context.Background(), &clone, nil, orgId, usr) diff --git a/pkg/services/sqlstore/migrations/accesscontrol/alerting.go b/pkg/services/sqlstore/migrations/accesscontrol/alerting.go index 15a92ba777a..edec940c8c7 100644 --- a/pkg/services/sqlstore/migrations/accesscontrol/alerting.go +++ b/pkg/services/sqlstore/migrations/accesscontrol/alerting.go @@ -116,3 +116,49 @@ func (m *receiverCreateScopeMigration) Exec(sess *xorm.Session, mg *migrator.Mig func AddReceiverCreateScopeMigration(mg *migrator.Migrator) { mg.AddMigration("remove scope from alert.notifications.receivers:create", &receiverCreateScopeMigration{}) } + +type receiverProtectedFieldsEditor struct { + migrator.MigrationBase +} + +var _ migrator.CodeMigration = new(alertingMigrator) + +func (m *receiverProtectedFieldsEditor) SQL(migrator.Dialect) string { + return "code migration" +} + +func (m *receiverProtectedFieldsEditor) Exec(sess *xorm.Session, mg *migrator.Migrator) error { + sql := `SELECT * + FROM permission AS P + WHERE action = 'alert.notifications.receivers.secrets:read' + AND EXISTS(SELECT 1 FROM role AS R WHERE R.id = P.role_id AND R.name LIKE 'managed:%') + AND NOT EXISTS(SELECT 1 + FROM permission AS P2 + WHERE P2.role_id = P.role_id + AND P2.action = 'alert.notifications.receivers.protected:write' AND P2.scope = P.scope + )` + var results []accesscontrol.Permission + if err := sess.SQL(sql).Find(&results); err != nil { + return fmt.Errorf("failed to query permissions: %w", err) + } + + permissionsToCreate := make([]accesscontrol.Permission, 0, len(results)) + rolesAffected := make(map[int64][]string, 0) + for _, result := range results { + result.ID = 0 + result.Action = "alert.notifications.receivers.protected:write" + result.Created = time.Now() + result.Updated = time.Now() + permissionsToCreate = append(permissionsToCreate, result) + rolesAffected[result.RoleID] = append(rolesAffected[result.RoleID], result.Identifier) + } + _, err := sess.InsertMulti(&permissionsToCreate) + for id, ids := range rolesAffected { + mg.Logger.Debug("Added permission 'alert.notifications.receivers.protected:write' to managed role", "roleID", id, "identifiers", ids) + } + return err +} + +func AddReceiverProtectedFieldsEditor(mg *migrator.Migrator) { + mg.AddMigration("add 'alert.notifications.receivers.protected:write' to receiver admins", &receiverProtectedFieldsEditor{}) +} diff --git a/pkg/services/sqlstore/migrations/migrations.go b/pkg/services/sqlstore/migrations/migrations.go index 99ccb2e4522..d5806f2b412 100644 --- a/pkg/services/sqlstore/migrations/migrations.go +++ b/pkg/services/sqlstore/migrations/migrations.go @@ -153,4 +153,6 @@ func (oss *OSSMigrations) AddMigration(mg *Migrator) { accesscontrol.AddDatasourceDrilldownRemovalMigration(mg) ualert.DropTitleUniqueIndexMigration(mg) + + accesscontrol.AddReceiverProtectedFieldsEditor(mg) } diff --git a/pkg/tests/apis/alerting/notifications/receivers/receiver_test.go b/pkg/tests/apis/alerting/notifications/receivers/receiver_test.go index 43bc217123b..4fbafc3ed2c 100644 --- a/pkg/tests/apis/alerting/notifications/receivers/receiver_test.go +++ b/pkg/tests/apis/alerting/notifications/receivers/receiver_test.go @@ -149,7 +149,7 @@ func TestIntegrationResourcePermissions(t *testing.T) { adminClient := test_common.NewReceiverClient(t, admin) writeACMetadata := []string{"canWrite", "canDelete"} - allACMetadata := []string{"canWrite", "canDelete", "canReadSecrets", "canAdmin"} + allACMetadata := []string{"canWrite", "canDelete", "canReadSecrets", "canAdmin", "canModifyProtected"} mustID := func(user apis.User) int64 { id, err := user.Identity.GetInternalID() @@ -412,13 +412,14 @@ func TestIntegrationAccessControl(t *testing.T) { org1 := helper.Org1 type testCase struct { - user apis.User - canRead bool - canUpdate bool - canCreate bool - canDelete bool - canReadSecrets bool - canAdmin bool + user apis.User + canRead bool + canUpdate bool + canUpdateProtected bool + canCreate bool + canDelete bool + canReadSecrets bool + canAdmin bool } // region users unauthorized := helper.CreateUser("unauthorized", "Org1", org.RoleNone, []resourcepermissions.SetResourcePermissionCommand{}) @@ -481,20 +482,22 @@ func TestIntegrationAccessControl(t *testing.T) { testCases := []testCase{ { - user: unauthorized, - canRead: false, - canUpdate: false, - canCreate: false, - canDelete: false, + user: unauthorized, + canRead: false, + canUpdate: false, + canUpdateProtected: false, + canCreate: false, + canDelete: false, }, { - user: org1.Admin, - canRead: true, - canCreate: true, - canUpdate: true, - canDelete: true, - canAdmin: true, - canReadSecrets: true, + user: org1.Admin, + canRead: true, + canCreate: true, + canUpdate: true, + canUpdateProtected: true, + canDelete: true, + canAdmin: true, + canReadSecrets: true, }, { user: org1.Editor, @@ -543,22 +546,24 @@ func TestIntegrationAccessControl(t *testing.T) { canDelete: true, }, { - user: adminLikeUser, - canRead: true, - canCreate: true, - canUpdate: true, - canDelete: true, - canAdmin: true, - canReadSecrets: true, + user: adminLikeUser, + canRead: true, + canCreate: true, + canUpdate: true, + canUpdateProtected: true, + canDelete: true, + canAdmin: true, + canReadSecrets: true, }, { - user: adminLikeUserLongName, - canRead: true, - canCreate: true, - canUpdate: true, - canDelete: true, - canAdmin: true, - canReadSecrets: true, + user: adminLikeUserLongName, + canRead: true, + canCreate: true, + canUpdate: true, + canUpdateProtected: true, + canDelete: true, + canAdmin: true, + canReadSecrets: true, }, } @@ -616,6 +621,9 @@ func TestIntegrationAccessControl(t *testing.T) { if tc.canUpdate { expectedWithMetadata.SetAccessControl("canWrite") } + if tc.canUpdateProtected { + expectedWithMetadata.SetAccessControl("canModifyProtected") + } if tc.canDelete { expectedWithMetadata.SetAccessControl("canDelete") } @@ -679,6 +687,32 @@ func TestIntegrationAccessControl(t *testing.T) { require.Truef(t, errors.IsNotFound(err), "Should get NotFound error but got: %s", err) }) }) + + updatedExpected = expected.Copy().(*v0alpha1.Receiver) + updatedExpected.Spec.Integrations = []v0alpha1.Integration{ + createIntegration(t, "webhook"), + } + + expected, err = adminClient.Update(ctx, updatedExpected, v1.UpdateOptions{}) + require.NoErrorf(t, err, "Payload %s", string(d)) + require.NotNil(t, expected) + + updatedProtected := expected.Copy().(*v0alpha1.Receiver) + updatedProtected.Spec.Integrations[0].Settings["url"] = "http://localhost:8080/webhook" + + if tc.canUpdateProtected { + t.Run("should be able to update protected fields of the receiver", func(t *testing.T) { + updated, err := client.Update(ctx, updatedProtected, v1.UpdateOptions{}) + require.NoErrorf(t, err, "Payload %s", string(d)) + require.NotNil(t, updated) + expected = updated + }) + } else { + t.Run("should be forbidden to edit protected fields of the receiver", func(t *testing.T) { + _, err := client.Update(ctx, updatedProtected, v1.UpdateOptions{}) + require.Truef(t, errors.IsForbidden(err), "should get Forbidden error but got %s", err) + }) + } } else { t.Run("should be forbidden to update receiver", func(t *testing.T) { _, err := client.Update(ctx, updatedExpected, v1.UpdateOptions{}) @@ -691,6 +725,7 @@ func TestIntegrationAccessControl(t *testing.T) { require.Truef(t, errors.IsForbidden(err), "should get Forbidden error but got %s", err) }) }) + require.Falsef(t, tc.canUpdateProtected, "Invalid combination of assertions. CanUpdateProtected should be false") } deleteOptions := v1.DeleteOptions{Preconditions: &v1.Preconditions{ResourceVersion: util.Pointer(expected.ResourceVersion)}} @@ -1310,6 +1345,7 @@ func TestIntegrationCRUD(t *testing.T) { receiver.SetAccessControl("canDelete") receiver.SetAccessControl("canReadSecrets") receiver.SetAccessControl("canAdmin") + receiver.SetAccessControl("canModifyProtected") receiver.SetInUse(0, nil) // Use export endpoint because it's the only way to get decrypted secrets fast. diff --git a/public/app/features/alerting/unified/components/contact-points/EditContactPoint.test.tsx b/public/app/features/alerting/unified/components/contact-points/EditContactPoint.test.tsx index 3a7895c1a29..b43671ce874 100644 --- a/public/app/features/alerting/unified/components/contact-points/EditContactPoint.test.tsx +++ b/public/app/features/alerting/unified/components/contact-points/EditContactPoint.test.tsx @@ -44,7 +44,7 @@ beforeEach(() => { grantUserPermissions([AccessControlAction.AlertingNotificationsRead, AccessControlAction.AlertingNotificationsWrite]); }); -const getTemplatePreviewContent = async () => within(screen.getByTestId('template-preview')).getByTestId('mockeditor'); +const getTemplatePreviewContent = async () => within(screen.getByTestId('template-preview')).findByTestId('mockeditor'); const templatesSelectorTestId = 'existing-templates-selector'; diff --git a/public/app/features/alerting/unified/components/contact-points/useContactPoints.ts b/public/app/features/alerting/unified/components/contact-points/useContactPoints.ts index e1e9cc3f8e8..26dcce00b4c 100644 --- a/public/app/features/alerting/unified/components/contact-points/useContactPoints.ts +++ b/public/app/features/alerting/unified/components/contact-points/useContactPoints.ts @@ -1,9 +1,3 @@ -/** - * This hook will combine data from both the Alertmanager config - * and (if available) it will also fetch the status from the Grafana Managed status endpoint - */ - -import { merge, set } from 'lodash'; import { useMemo } from 'react'; import { receiversApi } from 'app/features/alerting/unified/api/receiversK8sApi'; @@ -13,11 +7,7 @@ import { BaseAlertmanagerArgs, Skippable } from 'app/features/alerting/unified/t import { cloudNotifierTypes } from 'app/features/alerting/unified/utils/cloud-alertmanager-notifier-types'; import { GRAFANA_RULES_SOURCE_NAME } from 'app/features/alerting/unified/utils/datasource'; import { isK8sEntityProvisioned, shouldUseK8sApi } from 'app/features/alerting/unified/utils/k8s/utils'; -import { - GrafanaManagedContactPoint, - GrafanaManagedReceiverConfig, - Receiver, -} from 'app/plugins/datasource/alertmanager/types'; +import { GrafanaManagedContactPoint, Receiver } from 'app/plugins/datasource/alertmanager/types'; import { getAPINamespace } from '../../../../../api/utils'; import { alertmanagerApi } from '../../api/alertmanagerApi'; @@ -327,47 +317,6 @@ export function useDeleteContactPoint({ alertmanager }: BaseAlertmanagerArgs) { return useK8sApi ? deleteFromK8sAPI : deleteFromAlertmanagerConfiguration; } -/** - * Turns a Grafana Managed receiver config into a format that can be sent to the k8s API - * - * When updating secure settings, we need to send a value of `true` for any secure setting that we want to keep the same. - * - * Any other setting that has a value in `secureSettings` will correspond to a new value for that setting - - * so we should not tell the API that we want to preserve it. Those values will instead be sent within `settings` - */ -const mapIntegrationSettingsForK8s = (integration: GrafanaManagedReceiverConfig): GrafanaManagedReceiverConfig => { - const { secureSettings, settings, ...restOfIntegration } = integration; - const secureFields = Object.entries(secureSettings || {}).reduce((acc, [key, value]) => { - // If a secure field has no (changed) value, then we tell the backend to persist it - if (value === undefined) { - return { - ...acc, - [key]: true, - }; - } - return acc; - }, {}); - - const mappedSecureSettings = Object.entries(secureSettings || {}).reduce((acc, [key, value]) => { - // If the value is an empty string/falsy value, then we need to omit it from the payload - // so the backend knows to remove it - if (!value) { - return acc; - } - - // Otherwise, we send the value of the secure field - return set(acc, key, value); - }, {}); - - // Merge settings properly with lodash so we don't lose any information from nested keys/secure settings - const mergedSettings = merge({}, settings, mappedSecureSettings); - - return { - ...restOfIntegration, - secureFields, - settings: mergedSettings, - }; -}; const grafanaContactPointToK8sReceiver = ( contactPoint: GrafanaManagedContactPoint, id?: string, @@ -380,7 +329,7 @@ const grafanaContactPointToK8sReceiver = ( }, spec: { title: contactPoint.name, - integrations: (contactPoint.grafana_managed_receiver_configs || []).map(mapIntegrationSettingsForK8s), + integrations: contactPoint.grafana_managed_receiver_configs || [], }, }; }; diff --git a/public/app/features/alerting/unified/components/receivers/GlobalConfigForm.tsx b/public/app/features/alerting/unified/components/receivers/GlobalConfigForm.tsx index e932fc4ea5e..5431c2391f0 100644 --- a/public/app/features/alerting/unified/components/receivers/GlobalConfigForm.tsx +++ b/public/app/features/alerting/unified/components/receivers/GlobalConfigForm.tsx @@ -87,6 +87,7 @@ export const GlobalConfigForm = ({ config, alertManagerSourceName }: Props) => { option={option} error={errors[option.propertyName]} pathPrefix={''} + secureFields={{}} /> ))}
diff --git a/public/app/features/alerting/unified/components/receivers/__snapshots__/NewReceiverView.test.tsx.snap b/public/app/features/alerting/unified/components/receivers/__snapshots__/NewReceiverView.test.tsx.snap index f4c781df037..15473192d14 100644 --- a/public/app/features/alerting/unified/components/receivers/__snapshots__/NewReceiverView.test.tsx.snap +++ b/public/app/features/alerting/unified/components/receivers/__snapshots__/NewReceiverView.test.tsx.snap @@ -17,7 +17,7 @@ exports[`new receiver should be able to test and save a receiver 1`] = ` { "disableResolveMessage": false, "name": "test", - "secureSettings": {}, + "secureFields": {}, "settings": { "addresses": "tester@grafana.com", "singleEmail": false, diff --git a/public/app/features/alerting/unified/components/receivers/form/ChannelOptions.tsx b/public/app/features/alerting/unified/components/receivers/form/ChannelOptions.tsx index 1c99524f434..fc8d4cc73f1 100644 --- a/public/app/features/alerting/unified/components/receivers/form/ChannelOptions.tsx +++ b/public/app/features/alerting/unified/components/receivers/form/ChannelOptions.tsx @@ -2,22 +2,31 @@ import * as React from 'react'; import { DeepMap, FieldError, FieldErrors, useFormContext } from 'react-hook-form'; import { Field, SecretInput } from '@grafana/ui'; -import { NotificationChannelOption, NotificationChannelSecureFields } from 'app/types'; +import { NotificationChannelOption, NotificationChannelSecureFields, OptionMeta } from 'app/types'; -import { ChannelValues, ReceiverFormValues } from '../../../types/receiver-form'; +import { + ChannelValues, + CloudChannelValues, + GrafanaChannelValues, + ReceiverFormValues, +} from '../../../types/receiver-form'; import { OptionField } from './fields/OptionField'; export interface Props { defaultValues: R; selectedChannelOptions: NotificationChannelOption[]; - secureFields: NotificationChannelSecureFields; onResetSecureField: (key: string) => void; + onDeleteSubform?: (settingsPath: string, option: NotificationChannelOption) => void; errors?: FieldErrors; - pathPrefix?: string; + /** + * The path for the integration in the array of integrations. + * This is used to access the settings and secure fields for the integration in a type-safe way. + */ + integrationPrefix: `items.${number}`; + canEditProtectedFields: boolean; readOnly?: boolean; - customValidators?: Record['customValidator']>; } @@ -25,14 +34,24 @@ export function ChannelOptions({ defaultValues, selectedChannelOptions, onResetSecureField, - secureFields, + onDeleteSubform, errors, - pathPrefix = '', + integrationPrefix, readOnly = false, customValidators = {}, + canEditProtectedFields, }: Props): JSX.Element { - const { watch } = useFormContext>(); - const currentFormValues = watch(); // react hook form types ARE LYING! + const { watch } = useFormContext>(); + + const [settings, secureFields] = watch([`${integrationPrefix}.settings`, `${integrationPrefix}.secureFields`]); + + // Note: settingsPath includes a trailing dot for OptionField, unlike the path used in watch() + const settingsPath = `${integrationPrefix}.settings.` as const; + + const getOptionMeta = (option: NotificationChannelOption): OptionMeta => ({ + required: determineRequired(option, settings, secureFields), + readOnly: determineReadOnly(option, settings, secureFields, canEditProtectedFields), + }); return ( <> @@ -41,43 +60,97 @@ export function ChannelOptions({ // Some options can be dependent on other options, this determines what is selected in the dependency options // I think this needs more thought. // pathPrefix = items.index. - const paths = pathPrefix.split('.'); - const selectedOptionValue = - paths.length >= 2 ? currentFormValues.items?.[Number(paths[1])].settings?.[option.showWhen.field] : undefined; + // const paths = pathPrefix.split('.'); + const selectedOptionValue = settings?.[option.showWhen.field]; if (option.showWhen.field && selectedOptionValue !== option.showWhen.is) { return null; } - if (secureFields && secureFields[option.propertyName]) { + if (secureFields && secureFields[option.secureFieldKey ?? option.propertyName]) { return ( - - onResetSecureField(option.propertyName)} isConfigured /> + + onResetSecureField(option.secureFieldKey ?? option.propertyName)} + isConfigured + /> ); } - const error: FieldError | DeepMap | undefined = ( - (option.secure ? errors?.secureSettings : errors?.settings) as DeepMap | undefined - )?.[option.propertyName]; + const errorSource = option.secure ? errors?.secureFields : errors?.settings; + const propertyKey = option.secureFieldKey ?? option.propertyName; + // eslint-disable-next-line @typescript-eslint/consistent-type-assertions + const error = ( + errorSource as Record, FieldError>> | undefined + )?.[propertyKey]; const defaultValue = defaultValues?.settings?.[option.propertyName]; return ( ); })} ); } + +const determineRequired = ( + option: NotificationChannelOption, + settings: Record, + secureFields: NotificationChannelSecureFields +) => { + if (!option.required) { + return false; + } + + if (!option.dependsOn) { + return option.required ? 'Required' : false; + } + + // TODO: This doesn't work with nested secureFields. + const dependentOn = Boolean(settings[option.dependsOn]) || Boolean(secureFields[option.dependsOn]); + + if (dependentOn) { + return false; + } + + return 'Required'; +}; + +const determineReadOnly = ( + option: NotificationChannelOption, + settings: Record, + secureFields: NotificationChannelSecureFields, + canEditProtectedFields: boolean +) => { + if (option.protected && !canEditProtectedFields) { + return true; + } + + // Handle fields with dependencies (e.g., field B depends on field A being set) + if (!option.dependsOn) { + return false; + } + + // TODO: This doesn't work with nested secureFields. + return Boolean(settings[option.dependsOn]) || Boolean(secureFields[option.dependsOn]); +}; diff --git a/public/app/features/alerting/unified/components/receivers/form/ChannelSubForm.test.tsx b/public/app/features/alerting/unified/components/receivers/form/ChannelSubForm.test.tsx new file mode 100644 index 00000000000..e427f98d483 --- /dev/null +++ b/public/app/features/alerting/unified/components/receivers/form/ChannelSubForm.test.tsx @@ -0,0 +1,249 @@ +import 'core-js/stable/structured-clone'; +import { FormProvider, useForm } from 'react-hook-form'; +import { clickSelectOption } from 'test/helpers/selectOptionInTest'; +import { render } from 'test/test-utils'; +import { byRole, byTestId } from 'testing-library-selector'; + +import { grafanaAlertNotifiers } from 'app/features/alerting/unified/mockGrafanaNotifiers'; +import { AlertmanagerProvider } from 'app/features/alerting/unified/state/AlertmanagerContext'; + +import { ChannelSubForm } from './ChannelSubForm'; +import { GrafanaCommonChannelSettings } from './GrafanaCommonChannelSettings'; +import { Notifier } from './notifiers'; + +type TestChannelValues = { + __id: string; + type: string; + settings: Record; + secureFields: Record; +}; + +type TestReceiverFormValues = { + name: string; + items: TestChannelValues[]; +}; + +const ui = { + typeSelector: byTestId('items.0.type'), + settings: { + webhook: { + url: byRole('textbox', { name: /^URL/ }), + optionalSettings: byRole('button', { name: /optional webhook settings/i }), + title: { + container: byTestId('items.0.settings.title'), + input: byRole('textbox', { name: /^Title/ }), + }, + message: { + container: byTestId('items.0.settings.message'), + input: byRole('textbox', { name: /^Message/ }), + }, + }, + slack: { + recipient: byTestId('items.0.settings.recipient'), + token: byTestId('items.0.settings.token'), + username: byTestId('items.0.settings.username'), + webhookUrl: byRole('textbox', { name: /^Webhook URL/ }), + }, + googlechat: { + optionalSettings: byRole('button', { name: /optional google hangouts chat settings/i }), + url: byRole('textbox', { name: /^URL/ }), + title: { + input: byRole('textbox', { name: /^Title/ }), + container: byTestId('items.0.settings.title'), + }, + message: { + input: byRole('textbox', { name: /^Message/ }), + container: byTestId('items.0.settings.message'), + }, + }, + }, +}; + +const notifiers: Notifier[] = [ + { dto: grafanaAlertNotifiers.webhook, meta: { enabled: true, order: 1 } }, + { dto: grafanaAlertNotifiers.slack, meta: { enabled: true, order: 2 } }, + { dto: grafanaAlertNotifiers.googlechat, meta: { enabled: true, order: 3 } }, + { dto: grafanaAlertNotifiers.sns, meta: { enabled: true, order: 4 } }, + { dto: grafanaAlertNotifiers.oncall, meta: { enabled: true, order: 5 } }, +]; + +describe('ChannelSubForm', () => { + function TestFormWrapper({ defaults, initial }: { defaults: TestChannelValues; initial?: TestChannelValues }) { + const form = useForm({ + defaultValues: { + name: 'test-contact-point', + items: [defaults], + }, + }); + + return ( + + + + + + ); + } + + function renderForm(defaults: TestChannelValues, initial?: TestChannelValues) { + return render(); + } + + it('switching type hides prior fields and shows new ones', async () => { + renderForm({ + __id: 'id-0', + type: 'webhook', + settings: { url: '' }, + secureFields: {}, + }); + + expect(ui.typeSelector.get()).toHaveTextContent('Webhook'); + + expect(ui.settings.webhook.url.get()).toBeInTheDocument(); + + expect(ui.settings.slack.recipient.query()).not.toBeInTheDocument(); + + await clickSelectOption(ui.typeSelector.get(), 'Slack'); + expect(ui.typeSelector.get()).toHaveTextContent('Slack'); + + expect(ui.settings.slack.recipient.get()).toBeInTheDocument(); + expect(ui.settings.slack.token.get()).toBeInTheDocument(); + expect(ui.settings.slack.username.get()).toBeInTheDocument(); + }); + + it('should clear secure fields when switching integration types', async () => { + const googlechatDefaults: TestChannelValues = { + __id: 'id-0', + type: 'googlechat', + settings: { title: 'Alert Title', message: 'Alert Message' }, + secureFields: { url: true }, + }; + + const { user } = renderForm(googlechatDefaults, googlechatDefaults); + + expect(ui.typeSelector.get()).toHaveTextContent('Google Hangouts Chat'); + + expect(ui.settings.googlechat.url.get()).toBeDisabled(); + expect(ui.settings.googlechat.url.get()).toHaveValue('configured'); + + await user.click(ui.settings.googlechat.optionalSettings.get()); + + expect(ui.settings.googlechat.title.input.get()).toHaveValue('Alert Title'); + expect(ui.settings.googlechat.message.input.get()).toHaveValue('Alert Message'); + + await clickSelectOption(ui.typeSelector.get(), 'Webhook'); + expect(ui.typeSelector.get()).toHaveTextContent('Webhook'); + + // Webhook URL field should now be present and empty (settings cleared) + expect(ui.settings.webhook.url.get()).toHaveValue(''); + expect(ui.settings.webhook.title.container.get()).toBeInTheDocument(); + expect(ui.settings.webhook.message.container.get()).toBeInTheDocument(); + + // If value for templated fields is empty the input should not be present + expect(ui.settings.webhook.message.input.query()).not.toBeInTheDocument(); + expect(ui.settings.webhook.title.input.query()).not.toBeInTheDocument(); + }); + + it('should clear settings when switching from webhook to googlechat', async () => { + const webhookDefaults: TestChannelValues = { + __id: 'id-0', + type: 'webhook', + settings: { url: 'https://example.com/webhook', title: 'Webhook Title', message: 'Webhook Message' }, + secureFields: {}, + }; + + const { user } = renderForm(webhookDefaults, webhookDefaults); + + expect(ui.typeSelector.get()).toHaveTextContent('Webhook'); + + expect(ui.settings.webhook.url.get()).toHaveValue('https://example.com/webhook'); + + await user.click(ui.settings.webhook.optionalSettings.get()); + expect(ui.settings.webhook.title.input.get()).toHaveValue('Webhook Title'); + expect(ui.settings.webhook.message.input.get()).toHaveValue('Webhook Message'); + + await clickSelectOption(ui.typeSelector.get(), 'Google Hangouts Chat'); + expect(ui.typeSelector.get()).toHaveTextContent('Google Hangouts Chat'); + + // Google Chat URL field should now be present and empty (settings cleared) + expect(ui.settings.googlechat.url.get()).toHaveValue(''); + expect(ui.settings.googlechat.title.container.get()).toBeInTheDocument(); + expect(ui.settings.googlechat.message.container.get()).toBeInTheDocument(); + + // If value for templated fields is empty the input should not be present + expect(ui.settings.googlechat.message.input.query()).not.toBeInTheDocument(); + expect(ui.settings.googlechat.title.input.query()).not.toBeInTheDocument(); + }); + + it('should restore initial values when switching back to original type', async () => { + const googlechatDefaults: TestChannelValues = { + __id: 'id-0', + type: 'googlechat', + settings: { title: 'Original Title', message: 'Original Message' }, + secureFields: { url: true }, + }; + + const { user } = renderForm(googlechatDefaults, googlechatDefaults); + + expect(ui.typeSelector.get()).toHaveTextContent('Google Hangouts Chat'); + + expect(ui.settings.googlechat.url.get()).toBeDisabled(); + expect(ui.settings.googlechat.url.get()).toHaveValue('configured'); + + await user.click(ui.settings.googlechat.optionalSettings.get()); + + expect(ui.settings.googlechat.title.input.get()).toHaveValue('Original Title'); + expect(ui.settings.googlechat.message.input.get()).toHaveValue('Original Message'); + + // Switch to a different type + await clickSelectOption(ui.typeSelector.get(), 'Webhook'); + expect(ui.typeSelector.get()).toHaveTextContent('Webhook'); + expect(ui.settings.webhook.url.get()).toHaveValue(''); + + // Switch back to the original type + await clickSelectOption(ui.typeSelector.get(), 'Google Hangouts Chat'); + expect(ui.typeSelector.get()).toHaveTextContent('Google Hangouts Chat'); + + // Original settings and secure fields should be restored + expect(ui.settings.googlechat.url.get()).toBeDisabled(); + expect(ui.settings.googlechat.url.get()).toHaveValue('configured'); + + expect(ui.settings.googlechat.title.input.get()).toHaveValue('Original Title'); + expect(ui.settings.googlechat.message.input.get()).toHaveValue('Original Message'); + }); + + it('should maintain secure field isolation across multiple type switches', async () => { + const googlechatDefaults: TestChannelValues = { + __id: 'id-0', + type: 'googlechat', + settings: {}, + secureFields: { url: true }, + }; + + renderForm(googlechatDefaults, googlechatDefaults); + + expect(ui.typeSelector.get()).toHaveTextContent('Google Hangouts Chat'); + expect(ui.settings.googlechat.url.get()).toBeDisabled(); + expect(ui.settings.googlechat.url.get()).toHaveValue('configured'); + + // Switch to Slack + await clickSelectOption(ui.typeSelector.get(), 'Slack'); + expect(ui.typeSelector.get()).toHaveTextContent('Slack'); + + // Slack should not have any secure fields from Google Chat + const slackUrl = ui.settings.slack.webhookUrl.get(); + expect(slackUrl).toBeEnabled(); + expect(slackUrl).toHaveValue(''); + }); +}); diff --git a/public/app/features/alerting/unified/components/receivers/form/ChannelSubForm.tsx b/public/app/features/alerting/unified/components/receivers/form/ChannelSubForm.tsx index ff6b52d051d..758ef67f6c4 100644 --- a/public/app/features/alerting/unified/components/receivers/form/ChannelSubForm.tsx +++ b/public/app/features/alerting/unified/components/receivers/form/ChannelSubForm.tsx @@ -1,35 +1,41 @@ import { css } from '@emotion/css'; import { sortBy } from 'lodash'; import * as React from 'react'; -import { useCallback, useEffect, useMemo, useState } from 'react'; -import { Controller, FieldErrors, FieldValues, useFormContext } from 'react-hook-form'; +import { useEffect, useMemo } from 'react'; +import { Controller, FieldErrors, useFormContext } from 'react-hook-form'; import { GrafanaTheme2, SelectableValue } from '@grafana/data'; import { Alert, Button, Field, Select, Stack, Text, useStyles2 } from '@grafana/ui'; import { Trans, t } from 'app/core/internationalization'; +import { NotificationChannelOption } from 'app/types'; -import { useUnifiedAlertingSelector } from '../../../hooks/useUnifiedAlertingSelector'; -import { ChannelValues, CommonSettingsComponentType } from '../../../types/receiver-form'; +import { + ChannelValues, + CloudChannelValues, + CommonSettingsComponentType, + GrafanaChannelValues, + ReceiverFormValues, +} from '../../../types/receiver-form'; import { OnCallIntegrationType } from '../grafanaAppReceivers/onCall/useOnCallIntegration'; import { ChannelOptions } from './ChannelOptions'; import { CollapsibleSection } from './CollapsibleSection'; import { Notifier } from './notifiers'; -interface Props { +interface Props { defaultValues: R; initialValues?: R; - pathPrefix: string; + pathPrefix: `items.${number}.`; + integrationIndex: number; notifiers: Notifier[]; onDuplicate: () => void; onTest?: () => void; commonSettingsComponent: CommonSettingsComponentType; - - secureFields?: Record; errors?: FieldErrors; onDelete?: () => void; isEditable?: boolean; isTestable?: boolean; + canEditProtectedFields: boolean; customValidators?: React.ComponentProps['customValidators']; } @@ -38,74 +44,130 @@ export function ChannelSubForm({ defaultValues, initialValues, pathPrefix, + integrationIndex, onDuplicate, onDelete, onTest, notifiers, errors, - secureFields, commonSettingsComponent: CommonSettingsComponent, isEditable = true, isTestable, + canEditProtectedFields, customValidators = {}, }: Props): JSX.Element { const styles = useStyles2(getStyles); + const { control, watch, register, trigger, formState, setValue, getValues } = + useFormContext>(); - const fieldName = useCallback((fieldName: string) => `${pathPrefix}${fieldName}`, [pathPrefix]); + const channelFieldPath = `items.${integrationIndex}` as const; + const typeFieldPath = `${channelFieldPath}.type` as const; + const settingsFieldPath = `${channelFieldPath}.settings` as const; + const secureFieldsPath = `${channelFieldPath}.secureFields` as const; - const { control, watch, register, trigger, formState, setValue } = useFormContext(); - const selectedType = watch(fieldName('type')) ?? defaultValues.type; // nope, setting "default" does not work at all. - const parse_mode = watch(fieldName('settings.parse_mode')); - const { loading: testingReceiver } = useUnifiedAlertingSelector((state) => state.testReceivers); + const selectedType = watch(typeFieldPath) ?? defaultValues.type; + const parse_mode = watch(`${settingsFieldPath}.parse_mode`); // TODO I don't like integration specific code here but other ways require a bigger refactoring - const onCallIntegrationType = watch(fieldName('settings.integration_type')); + const onCallIntegrationType = watch(`${settingsFieldPath}.integration_type`); const isTestAvailable = onCallIntegrationType !== OnCallIntegrationType.NewIntegration; useEffect(() => { - register(`${pathPrefix}.__id`); + register(`${channelFieldPath}.__id`); /* Need to manually register secureFields or else they'll be lost when testing a contact point */ - register(`${pathPrefix}.secureFields`); - }, [register, pathPrefix]); + register(`${channelFieldPath}.secureFields`); + }, [register, channelFieldPath]); // Prevent forgetting about initial values when switching the integration type and the oncall integration type useEffect(() => { // Restore values when switching back from a changed integration to the default one - const subscription = watch((v, { name, type }) => { - const value = name ? v[name] : ''; - if (initialValues && name === fieldName('type') && value === initialValues.type && type === 'change') { - setValue(fieldName('settings'), initialValues.settings); + const subscription = watch((formValues, { name, type }) => { + // @ts-expect-error name is valid key for formValues + const value = name ? getValues(name, formValues) : ''; + if (initialValues && name === typeFieldPath && value === initialValues.type && type === 'change') { + setValue(settingsFieldPath, initialValues.settings); + setValue(secureFieldsPath, initialValues.secureFields); + } else if (name === typeFieldPath && type === 'change') { + // When switching to a new notifier, set the default settings to remove all existing settings + // from the previous notifier + const newNotifier = notifiers.find(({ dto: { type } }) => type === value); + const defaultNotifierSettings = newNotifier ? getDefaultNotifierSettings(newNotifier) : {}; + + // Not sure why, but verriding settingsFieldPath is not enough if notifiers have the same settings fields, like url, title + const currentSettings = getValues(settingsFieldPath) ?? {}; + Object.keys(currentSettings).forEach((key) => { + if (!defaultNotifierSettings[key]) { + setValue(`${settingsFieldPath}.${key}`, defaultNotifierSettings[key]); + } + }); + + setValue(settingsFieldPath, defaultNotifierSettings); + setValue(secureFieldsPath, {}); } + // Restore initial value of an existing oncall integration if ( initialValues && - name === fieldName('settings.integration_type') && + name === `${settingsFieldPath}.integration_type` && value === OnCallIntegrationType.ExistingIntegration ) { - setValue(fieldName('settings.url'), initialValues.settings.url); + setValue(`${settingsFieldPath}.url`, initialValues.settings.url); } }); return () => subscription.unsubscribe(); - }, [selectedType, initialValues, setValue, fieldName, watch]); - - const [_secureFields, setSecureFields] = useState>(secureFields ?? {}); + }, [ + selectedType, + initialValues, + setValue, + settingsFieldPath, + typeFieldPath, + secureFieldsPath, + getValues, + watch, + defaultValues.settings, + defaultValues.secureFields, + notifiers, + ]); const onResetSecureField = (key: string) => { - if (_secureFields[key]) { - const updatedSecureFields = { ..._secureFields }; - updatedSecureFields[key] = ''; - setSecureFields(updatedSecureFields); - setValue(`${pathPrefix}.secureFields`, updatedSecureFields); + // formSecureFields might not be up to date if this function is called multiple times in a row + const currentSecureFields = getValues(`${channelFieldPath}.secureFields`); + if (currentSecureFields[key]) { + setValue(`${channelFieldPath}.secureFields`, { ...currentSecureFields, [key]: '' }); } }; + const findSecureFieldsRecursively = (options: NotificationChannelOption[]): string[] => { + const secureFields: string[] = []; + options?.forEach((option) => { + if (option.secure && option.secureFieldKey) { + secureFields.push(option.secureFieldKey); + } + if (option.subformOptions) { + secureFields.push(...findSecureFieldsRecursively(option.subformOptions)); + } + }); + return secureFields; + }; + + const onDeleteSubform = (settingsPath: string, option: NotificationChannelOption) => { + // Get all subform options with secure=true recursively. + const relatedSecureFields = findSecureFieldsRecursively(option.subformOptions ?? []); + relatedSecureFields.forEach((key) => { + onResetSecureField(key); + }); + const fieldPath = settingsPath.startsWith(`${channelFieldPath}.settings.`) + ? settingsPath.slice(`${channelFieldPath}.settings.`.length) + : settingsPath; + setValue(`${settingsFieldPath}.${fieldPath}`, undefined); + }; + const typeOptions = useMemo( (): SelectableValue[] => - sortBy(notifiers, ({ dto, meta }) => [meta?.order ?? 0, dto.name]) - // .notifiers.sort((a, b) => a.dto.name.localeCompare(b.dto.name)) - .map(({ dto: { name, type }, meta }) => ({ + sortBy(notifiers, ({ dto, meta }) => [meta?.order ?? 0, dto.name]).map( + ({ dto: { name, type }, meta }) => ({ // @ts-expect-error ReactNode is supported label: ( @@ -116,7 +178,8 @@ export function ChannelSubForm({ value: type, description: meta?.description, isDisabled: meta ? !meta.enabled : false, - })), + }) + ), [notifiers] ); @@ -137,8 +200,8 @@ export function ChannelSubForm({ const showTelegramWarning = isTelegram && !isParseModeNone; // if there are mandatory options defined, optional options will be hidden by a collapse // if there aren't mandatory options, all options will be shown without collapse - const mandatoryOptions = notifier?.dto.options.filter((o) => o.required); - const optionalOptions = notifier?.dto.options.filter((o) => !o.required); + const mandatoryOptions = notifier?.dto.options.filter((o) => o.required) ?? []; + const optionalOptions = notifier?.dto.options.filter((o) => !o.required) ?? []; const contactPointTypeInputId = `contact-point-type-${pathPrefix}`; return ( @@ -151,7 +214,8 @@ export function ChannelSubForm({ data-testid={`${pathPrefix}type`} > ( option.validationRule ? validateOption(v, option.validationRule, option.required) : true, @@ -233,7 +274,7 @@ const OptionInput: FC = ({ onSelectTemplate={onSelectTemplate} > {isEncryptedInput ? ( - onResetSecureField?.(nestedKey)} isConfigured /> + onResetSecureField?.(secureFieldKey)} isConfigured /> ) : (