From 258b3ab18b6e13aeaf48003361a2f23d65ecaf01 Mon Sep 17 00:00:00 2001 From: Yuriy Tseretyan Date: Fri, 20 May 2022 10:55:07 -0400 Subject: [PATCH] Alerting: Fix RBAC actions for notification policies (#49185) * squash actions "alert.notifications:update", "alert.notifications:create", "alert.notifications:delete" to "alert.notifications:write" * add migration * update UI to use the write action * update docs * changelog --- .../custom-role-actions-scopes.md | 4 +- .../rbac-fixed-basic-role-definitions.md | 2 +- pkg/services/accesscontrol/models.go | 6 +- pkg/services/ngalert/CHANGELOG.md | 1 + pkg/services/ngalert/accesscontrol.go | 12 +-- pkg/services/ngalert/api/authorization.go | 6 +- .../migrations/accesscontrol/alerting.go | 87 +++++++++++++++++++ .../sqlstore/migrations/migrations.go | 1 + public/app/features/alerting/routes.tsx | 14 +-- .../alerting/unified/Receivers.test.tsx | 4 +- .../alerting/unified/utils/access-control.ts | 6 +- public/app/types/accessControl.ts | 4 +- 12 files changed, 111 insertions(+), 36 deletions(-) create mode 100644 pkg/services/sqlstore/migrations/accesscontrol/alerting.go diff --git a/docs/sources/enterprise/access-control/custom-role-actions-scopes.md b/docs/sources/enterprise/access-control/custom-role-actions-scopes.md index c4761a6d3c0..9679536f5fa 100644 --- a/docs/sources/enterprise/access-control/custom-role-actions-scopes.md +++ b/docs/sources/enterprise/access-control/custom-role-actions-scopes.md @@ -30,10 +30,8 @@ The following list contains role-based access control actions. | `alert.instances:update` | n/a | Update and expire silences in the current organization. | | `alert.notifications.external:read` | `datasources:*`
`datasources:uid:*` | Read templates, contact points, notification policies, and mute timings in data sources that support alerting. | | `alert.notifications.external:write` | `datasources:*`
`datasources:uid:*` | Manage templates, contact points, notification policies, and mute timings in data sources that support alerting. | -| `alert.notifications:create` | n/a | Create templates, contact points, notification policies, and mute timings in the current organization. | -| `alert.notifications:delete` | n/a | Delete templates, contact points, notification policies, and mute timings in the current organization. | +| `alert.notifications:write` | n/a | Manage templates, contact points, notification policies, and mute timings in the current organization. | | `alert.notifications:read` | n/a | Read all templates, contact points, notification policies, and mute timings in the current organization. | -| `alert.notifications:update` | n/a | Update templates, contact points, notification policies, and mute timings in the current organization. | | `alert.rules.external:read` | `datasources:*`
`datasources:uid:*` | Read alert rules in data sources that support alerting (Prometheus, Mimir, and Loki) | | `alert.rules.external:write` | `datasources:*`
`datasources:uid:*` | Create, update, and delete alert rules in data sources that support alerting (Mimir and Loki). | | `alert.rules:create` | `folders:*`
`folders:uid:*` | Create Grafana alert rules in a folder. Combine this permission with `folders:read` in a scope that includes the folder and `datasources:query` in the scope of data sources the user can query. | diff --git a/docs/sources/enterprise/access-control/rbac-fixed-basic-role-definitions.md b/docs/sources/enterprise/access-control/rbac-fixed-basic-role-definitions.md index 43cd6c73079..a353e3a189b 100644 --- a/docs/sources/enterprise/access-control/rbac-fixed-basic-role-definitions.md +++ b/docs/sources/enterprise/access-control/rbac-fixed-basic-role-definitions.md @@ -28,7 +28,7 @@ The following tables list permissions associated with basic and fixed roles. | -------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `fixed:alerting.instances:editor` | All permissions from `fixed:alerting.instances:reader` and
`alert.instances:create`
`alert.instances:update` for organization scope
`alert.instances.external:write` for scope `datasources:*` | Create, update and expire all silences in the organization produced by Grafana, Mimir, and Loki.[\*](#alerting-roles) | | `fixed:alerting.instances:reader` | `alert.instances:read` for organization scope
`alert.instances.external:read` for scope `datasources:*` | Read all alerts and silences in the organization produced by Grafana Alerts and Mimir and Loki alerts and silences.[\*](#alerting-roles) | -| `fixed:alerting.notifications:editor` | All permissions from `fixed:alerting.notifications:reader` and
`alert.notifications:create`
`alert.notifications:update`
`alert.notifications:delete` for organization scope
`alert.notifications.external:read` for scope `datasources:*` | Create, update, and delete contact points, templates, mute timings and notification policies for Grafana and external Alertmanager.[\*](#alerting-roles) | +| `fixed:alerting.notifications:editor` | All permissions from `fixed:alerting.notifications:reader` and
`alert.notifications:write`for organization scope
`alert.notifications.external:read` for scope `datasources:*` | Create, update, and delete contact points, templates, mute timings and notification policies for Grafana and external Alertmanager.[\*](#alerting-roles) | | `fixed:alerting.notifications:reader` | `alert.notifications:read` for organization scope
`alert.notifications.external:read` for scope `datasources:*` | Read all Grafana and Alertmanager contact points, templates, and notification policies.[\*](#alerting-roles) | | `fixed:alerting.rules:editor` | All permissions from `fixed:alerting.rules:reader` and
`alert.rule:create`
`alert.rule:update`
`alert.rule:delete` for scope `folders:*`
`alert.rules.external:write` for scope `datasources:*` | Create, update, and delete all\* Grafana, Mimir, and Loki alert rules.[\*](#alerting-roles) | | `fixed:alerting.rules:reader` | `alert.rule:read` for scope `folders:*`
`alert.rules.external:read` for scope `datasources:*` | Read all\* Grafana, Mimir, and Loki alert rules.[\*](#alerting-roles) | diff --git a/pkg/services/accesscontrol/models.go b/pkg/services/accesscontrol/models.go index ca0ef468c49..83ca55c1a1c 100644 --- a/pkg/services/accesscontrol/models.go +++ b/pkg/services/accesscontrol/models.go @@ -372,10 +372,8 @@ const ( ActionAlertingInstanceRead = "alert.instances:read" // Alerting Notification policies actions - ActionAlertingNotificationsCreate = "alert.notifications:create" - ActionAlertingNotificationsRead = "alert.notifications:read" - ActionAlertingNotificationsUpdate = "alert.notifications:update" - ActionAlertingNotificationsDelete = "alert.notifications:delete" + ActionAlertingNotificationsRead = "alert.notifications:read" + ActionAlertingNotificationsWrite = "alert.notifications:write" // External alerting rule actions. We can only narrow it down to writes or reads, as we don't control the atomicity in the external system. ActionAlertingRuleExternalWrite = "alert.rules.external:write" diff --git a/pkg/services/ngalert/CHANGELOG.md b/pkg/services/ngalert/CHANGELOG.md index c7d1f5cda2e..d76e90d81e4 100644 --- a/pkg/services/ngalert/CHANGELOG.md +++ b/pkg/services/ngalert/CHANGELOG.md @@ -56,6 +56,7 @@ Scopes must have an order to ensure consistency and ease of search, this helps u - [FEATURE] Indicate whether alert rule is provisioned when GETting the rule #48458 - [BUGFIX] Migration: ignore alerts that do not belong to any existing organization\dashboard #49192 - [BUGFIX] Allow anonymous access to alerts #49203 +- [BUGFIX] RBAC: replace create\update\delete actions for notification policies by alert.notifications:write #49185 ## 8.5.3 diff --git a/pkg/services/ngalert/accesscontrol.go b/pkg/services/ngalert/accesscontrol.go index 6c77aff47a8..97340750f13 100644 --- a/pkg/services/ngalert/accesscontrol.go +++ b/pkg/services/ngalert/accesscontrol.go @@ -125,16 +125,10 @@ var ( DisplayName: "Notifications Editor", Description: "Can add, update, and delete contact points and notification policies in Grafana and external providers", Group: AlertRolesGroup, - Version: 1, + Version: 2, Permissions: accesscontrol.ConcatPermissions(notificationsReaderRole.Role.Permissions, []accesscontrol.Permission{ { - Action: accesscontrol.ActionAlertingNotificationsCreate, - }, - { - Action: accesscontrol.ActionAlertingNotificationsUpdate, - }, - { - Action: accesscontrol.ActionAlertingNotificationsDelete, + Action: accesscontrol.ActionAlertingNotificationsWrite, }, { Action: accesscontrol.ActionAlertingNotificationsExternalWrite, @@ -162,7 +156,7 @@ var ( DisplayName: "Full access", Description: "Can add,update and delete alert rules, instances, silences, contact points, and notification policies in Grafana and all external providers", Group: AlertRolesGroup, - Version: 2, + Version: 3, Permissions: accesscontrol.ConcatPermissions(rulesEditorRole.Role.Permissions, instancesEditorRole.Role.Permissions, notificationsEditorRole.Role.Permissions), }, Grants: []string{string(models.ROLE_EDITOR), string(models.ROLE_ADMIN)}, diff --git a/pkg/services/ngalert/api/authorization.go b/pkg/services/ngalert/api/authorization.go index d7b439e65ad..3fe488421d3 100644 --- a/pkg/services/ngalert/api/authorization.go +++ b/pkg/services/ngalert/api/authorization.go @@ -145,7 +145,7 @@ func (api *API) authorize(method, path string) web.Handler { // Grafana Paths case http.MethodDelete + "/api/alertmanager/grafana/config/api/v1/alerts": // reset alertmanager config to the default - eval = ac.EvalPermission(ac.ActionAlertingNotificationsDelete) + eval = ac.EvalPermission(ac.ActionAlertingNotificationsWrite) case http.MethodGet + "/api/alertmanager/grafana/config/api/v1/alerts": fallback = middleware.ReqEditorRole eval = ac.EvalPermission(ac.ActionAlertingNotificationsRead) @@ -153,14 +153,14 @@ func (api *API) authorize(method, path string) web.Handler { eval = ac.EvalPermission(ac.ActionAlertingNotificationsRead) case http.MethodPost + "/api/alertmanager/grafana/config/api/v1/alerts": // additional authorization is done in the request handler - eval = ac.EvalAny(ac.EvalPermission(ac.ActionAlertingNotificationsUpdate), ac.EvalPermission(ac.ActionAlertingNotificationsCreate), ac.EvalPermission(ac.ActionAlertingNotificationsDelete)) + eval = ac.EvalAny(ac.EvalPermission(ac.ActionAlertingNotificationsWrite)) case http.MethodPost + "/api/alertmanager/grafana/config/api/v1/receivers/test": fallback = middleware.ReqEditorRole eval = ac.EvalPermission(ac.ActionAlertingNotificationsRead) // External Alertmanager Paths case http.MethodDelete + "/api/alertmanager/{DatasourceUID}/config/api/v1/alerts": - eval = ac.EvalPermission(ac.ActionAlertingNotificationsDelete, datasources.ScopeProvider.GetResourceScopeUID(ac.Parameter(":DatasourceUID"))) + eval = ac.EvalPermission(ac.ActionAlertingNotificationsExternalWrite, datasources.ScopeProvider.GetResourceScopeUID(ac.Parameter(":DatasourceUID"))) case http.MethodGet + "/api/alertmanager/{DatasourceUID}/api/v2/status": eval = ac.EvalPermission(ac.ActionAlertingNotificationsExternalRead, datasources.ScopeProvider.GetResourceScopeUID(ac.Parameter(":DatasourceUID"))) case http.MethodGet + "/api/alertmanager/{DatasourceUID}/config/api/v1/alerts": diff --git a/pkg/services/sqlstore/migrations/accesscontrol/alerting.go b/pkg/services/sqlstore/migrations/accesscontrol/alerting.go new file mode 100644 index 00000000000..ad7a920d7b4 --- /dev/null +++ b/pkg/services/sqlstore/migrations/accesscontrol/alerting.go @@ -0,0 +1,87 @@ +package accesscontrol + +import ( + "fmt" + "time" + + "xorm.io/xorm" + + "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/services/sqlstore/migrator" +) + +func AddAlertingPermissionsMigrator(mg *migrator.Migrator) { + mg.AddMigration("alerting notification permissions", &alertingMigrator{}) +} + +type alertingMigrator struct { + sess *xorm.Session + migrator *migrator.Migrator + migrator.MigrationBase +} + +var _ migrator.CodeMigration = new(alertingMigrator) + +func (m *alertingMigrator) SQL(migrator.Dialect) string { + return "code migration" +} + +func (m *alertingMigrator) Exec(sess *xorm.Session, migrator *migrator.Migrator) error { + m.sess = sess + m.migrator = migrator + return m.migrateNotificationActions() +} + +func (m *alertingMigrator) migrateNotificationActions() error { + var results []accesscontrol.Permission + err := m.sess.Table(&accesscontrol.Permission{}).In("action", "alert.notifications:update", "alert.notifications:create", "alert.notifications:delete", accesscontrol.ActionAlertingNotificationsWrite).Find(&results) + if err != nil { + return fmt.Errorf("failed to query permission table: %w", err) + } + groupByRoleID := make(map[int64]bool) + toDelete := make([]interface{}, 0, len(results)) + for _, result := range results { + if result.Action == accesscontrol.ActionAlertingNotificationsWrite { + groupByRoleID[result.RoleID] = false + continue // do not delete this permission + } + if _, ok := groupByRoleID[result.RoleID]; !ok { + groupByRoleID[result.RoleID] = true + } + toDelete = append(toDelete, result.ID) + } + + toAdd := make([]accesscontrol.Permission, 0, len(groupByRoleID)) + + now := time.Now() + for roleID, add := range groupByRoleID { + if !add { + m.migrator.Logger.Info(fmt.Sprintf("skip adding action %s to role ID %d because it is already there", accesscontrol.ActionAlertingNotificationsWrite, roleID)) + continue + } + toAdd = append(toAdd, accesscontrol.Permission{ + RoleID: roleID, + Action: accesscontrol.ActionAlertingNotificationsWrite, + Scope: "", + Created: now, + Updated: now, + }) + } + + if len(toAdd) > 0 { + added, err := m.sess.Table(&accesscontrol.Permission{}).InsertMulti(toAdd) + if err != nil { + return fmt.Errorf("failed to insert new permissions:%w", err) + } + m.migrator.Logger.Debug(fmt.Sprintf("updated %d of %d roles with new permission %s", added, len(toAdd), accesscontrol.ActionAlertingNotificationsWrite)) + } + + if len(toDelete) > 0 { + _, err = m.sess.Table(&accesscontrol.Permission{}).In("id", toDelete...).Delete(accesscontrol.Permission{}) + if err != nil { + return fmt.Errorf("failed to delete deprecated permissions [alert.notifications:update, alert.notifications:create, alert.notifications:delete]:%w", err) + } + } + + return nil +} diff --git a/pkg/services/sqlstore/migrations/migrations.go b/pkg/services/sqlstore/migrations/migrations.go index 3f5fc017cd9..88fbd410519 100644 --- a/pkg/services/sqlstore/migrations/migrations.go +++ b/pkg/services/sqlstore/migrations/migrations.go @@ -80,6 +80,7 @@ func (*OSSMigrations) AddMigration(mg *Migrator) { if mg.Cfg.RBACEnabled { accesscontrol.AddTeamMembershipMigrations(mg) accesscontrol.AddDashboardPermissionsMigrator(mg) + accesscontrol.AddAlertingPermissionsMigrator(mg) } } addQueryHistoryStarMigrations(mg) diff --git a/public/app/features/alerting/routes.tsx b/public/app/features/alerting/routes.tsx index 9318f4ce812..76becd6a717 100644 --- a/public/app/features/alerting/routes.tsx +++ b/public/app/features/alerting/routes.tsx @@ -113,7 +113,7 @@ const unifiedRoutes: RouteDescriptor[] = [ { path: '/alerting/routes/mute-timing/new', roles: evaluateAccess( - [AccessControlAction.AlertingNotificationsCreate, AccessControlAction.AlertingNotificationsExternalWrite], + [AccessControlAction.AlertingNotificationsWrite, AccessControlAction.AlertingNotificationsExternalWrite], ['Editor', 'Admin'] ), component: SafeDynamicImport( @@ -123,7 +123,7 @@ const unifiedRoutes: RouteDescriptor[] = [ { path: '/alerting/routes/mute-timing/edit', roles: evaluateAccess( - [AccessControlAction.AlertingNotificationsUpdate, AccessControlAction.AlertingNotificationsExternalWrite], + [AccessControlAction.AlertingNotificationsWrite, AccessControlAction.AlertingNotificationsExternalWrite], ['Editor', 'Admin'] ), component: SafeDynamicImport( @@ -173,7 +173,7 @@ const unifiedRoutes: RouteDescriptor[] = [ { path: '/alerting/notifications/templates/new', roles: evaluateAccess( - [AccessControlAction.AlertingNotificationsCreate, AccessControlAction.AlertingNotificationsExternalWrite], + [AccessControlAction.AlertingNotificationsWrite, AccessControlAction.AlertingNotificationsExternalWrite], ['Editor', 'Admin'] ), component: SafeDynamicImport( @@ -183,7 +183,7 @@ const unifiedRoutes: RouteDescriptor[] = [ { path: '/alerting/notifications/templates/:id/edit', roles: evaluateAccess( - [AccessControlAction.AlertingNotificationsUpdate, AccessControlAction.AlertingNotificationsExternalWrite], + [AccessControlAction.AlertingNotificationsWrite, AccessControlAction.AlertingNotificationsExternalWrite], ['Editor', 'Admin'] ), component: SafeDynamicImport( @@ -193,7 +193,7 @@ const unifiedRoutes: RouteDescriptor[] = [ { path: '/alerting/notifications/receivers/new', roles: evaluateAccess( - [AccessControlAction.AlertingNotificationsCreate, AccessControlAction.AlertingNotificationsExternalWrite], + [AccessControlAction.AlertingNotificationsWrite, AccessControlAction.AlertingNotificationsExternalWrite], ['Editor', 'Admin'] ), component: SafeDynamicImport( @@ -203,7 +203,7 @@ const unifiedRoutes: RouteDescriptor[] = [ { path: '/alerting/notifications/receivers/:id/edit', roles: evaluateAccess( - [AccessControlAction.AlertingNotificationsUpdate, AccessControlAction.AlertingNotificationsExternalWrite], + [AccessControlAction.AlertingNotificationsWrite, AccessControlAction.AlertingNotificationsExternalWrite], ['Editor', 'Admin'] ), component: SafeDynamicImport( @@ -213,7 +213,7 @@ const unifiedRoutes: RouteDescriptor[] = [ { path: '/alerting/notifications/global-config', roles: evaluateAccess( - [AccessControlAction.AlertingNotificationsUpdate, AccessControlAction.AlertingNotificationsExternalWrite], + [AccessControlAction.AlertingNotificationsWrite, AccessControlAction.AlertingNotificationsExternalWrite], ['Editor', 'Admin'] ), component: SafeDynamicImport( diff --git a/public/app/features/alerting/unified/Receivers.test.tsx b/public/app/features/alerting/unified/Receivers.test.tsx index fc85f31a484..e7c9e7bda3c 100644 --- a/public/app/features/alerting/unified/Receivers.test.tsx +++ b/public/app/features/alerting/unified/Receivers.test.tsx @@ -137,9 +137,7 @@ describe('Receivers', () => { mocks.contextSrv.hasPermission.mockImplementation((action) => { const permissions = [ AccessControlAction.AlertingNotificationsRead, - AccessControlAction.AlertingNotificationsCreate, - AccessControlAction.AlertingNotificationsUpdate, - AccessControlAction.AlertingNotificationsDelete, + AccessControlAction.AlertingNotificationsWrite, AccessControlAction.AlertingNotificationsExternalRead, AccessControlAction.AlertingNotificationsExternalWrite, ]; diff --git a/public/app/features/alerting/unified/utils/access-control.ts b/public/app/features/alerting/unified/utils/access-control.ts index 57d785df5ec..6561b005dcc 100644 --- a/public/app/features/alerting/unified/utils/access-control.ts +++ b/public/app/features/alerting/unified/utils/access-control.ts @@ -34,15 +34,15 @@ export const notificationsPermissions = { external: AccessControlAction.AlertingNotificationsExternalRead, }, create: { - grafana: AccessControlAction.AlertingNotificationsCreate, + grafana: AccessControlAction.AlertingNotificationsWrite, external: AccessControlAction.AlertingNotificationsExternalWrite, }, update: { - grafana: AccessControlAction.AlertingNotificationsUpdate, + grafana: AccessControlAction.AlertingNotificationsWrite, external: AccessControlAction.AlertingNotificationsExternalWrite, }, delete: { - grafana: AccessControlAction.AlertingNotificationsDelete, + grafana: AccessControlAction.AlertingNotificationsWrite, external: AccessControlAction.AlertingNotificationsExternalWrite, }, }; diff --git a/public/app/types/accessControl.ts b/public/app/types/accessControl.ts index 12d2341559f..22bcf3d14c0 100644 --- a/public/app/types/accessControl.ts +++ b/public/app/types/accessControl.ts @@ -92,10 +92,8 @@ export enum AccessControlAction { AlertingInstanceRead = 'alert.instances:read', // Alerting Notification policies - AlertingNotificationsCreate = 'alert.notifications:create', AlertingNotificationsRead = 'alert.notifications:read', - AlertingNotificationsUpdate = 'alert.notifications:update', - AlertingNotificationsDelete = 'alert.notifications:delete', + AlertingNotificationsWrite = 'alert.notifications:write', // External alerting rule actions. AlertingRuleExternalWrite = 'alert.rules.external:write',