diff --git a/pkg/services/ngalert/CHANGELOG.md b/pkg/services/ngalert/CHANGELOG.md index d76e90d81e4..ed0113fead0 100644 --- a/pkg/services/ngalert/CHANGELOG.md +++ b/pkg/services/ngalert/CHANGELOG.md @@ -57,6 +57,7 @@ Scopes must have an order to ensure consistency and ease of search, this helps u - [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 +- [BUGFIX] Fix access to alerts for Viewer role with editor permissions in folder #49270 ## 8.5.3 diff --git a/pkg/services/ngalert/api/api_ruler.go b/pkg/services/ngalert/api/api_ruler.go index f3d3fe694a3..22238d85cab 100644 --- a/pkg/services/ngalert/api/api_ruler.go +++ b/pkg/services/ngalert/api/api_ruler.go @@ -358,20 +358,21 @@ func (srv RulerSrv) updateAlertRulesInGroup(c *models.ReqContext, groupKey ngmod return nil } - authorizedChanges, err := authorizeRuleChanges(groupChanges, func(evaluator accesscontrol.Evaluator) bool { - return hasAccess(accesscontrol.ReqOrgAdminOrEditor, evaluator) - }) - if err != nil { - return err - } - - if authorizedChanges.isEmpty() { - logger.Info("no authorized changes detected in the request. Do nothing", "not_authorized_add", len(groupChanges.New), "not_authorized_update", len(groupChanges.Update), "not_authorized_delete", len(groupChanges.Delete)) - return nil - } - - if len(groupChanges.Delete) > len(authorizedChanges.Delete) { - logger.Info("user is not authorized to delete one or many rules in the group. those rules will be skipped", "expected", len(groupChanges.Delete), "authorized", len(authorizedChanges.Delete)) + authorizedChanges := groupChanges // if RBAC is disabled the permission are limited to folder access that is done upstream + if !srv.ac.IsDisabled() { + authorizedChanges, err = authorizeRuleChanges(groupChanges, func(evaluator accesscontrol.Evaluator) bool { + return hasAccess(accesscontrol.ReqOrgAdminOrEditor, evaluator) + }) + if err != nil { + return err + } + if authorizedChanges.isEmpty() { + logger.Info("no authorized changes detected in the request. Do nothing", "not_authorized_add", len(groupChanges.New), "not_authorized_update", len(groupChanges.Update), "not_authorized_delete", len(groupChanges.Delete)) + return nil + } + if len(groupChanges.Delete) > len(authorizedChanges.Delete) { + logger.Info("user is not authorized to delete one or many rules in the group. those rules will be skipped", "expected", len(groupChanges.Delete), "authorized", len(authorizedChanges.Delete)) + } } provenances, err := srv.provenanceStore.GetProvenances(c.Req.Context(), c.OrgId, (&ngmodels.AlertRule{}).ResourceType()) diff --git a/pkg/services/ngalert/api/authorization.go b/pkg/services/ngalert/api/authorization.go index 3fe488421d3..70ff0034dd3 100644 --- a/pkg/services/ngalert/api/authorization.go +++ b/pkg/services/ngalert/api/authorization.go @@ -49,6 +49,7 @@ func (api *API) authorize(method, path string) web.Handler { case http.MethodGet + "/api/ruler/grafana/api/v1/rules": eval = ac.EvalPermission(ac.ActionAlertingRuleRead) case http.MethodPost + "/api/ruler/grafana/api/v1/rules/{Namespace}": + fallback = middleware.ReqSignedIn // if RBAC is disabled then we need to delegate permission check to folder because its permissions can allow editing for Viewer role scope := dashboards.ScopeFoldersProvider.GetResourceScopeName(ac.Parameter(":Namespace")) // more granular permissions are enforced by the handler via "authorizeRuleChanges" eval = ac.EvalAny( diff --git a/public/app/features/alerting/routes.tsx b/public/app/features/alerting/routes.tsx index 76becd6a717..e85f2ed3130 100644 --- a/public/app/features/alerting/routes.tsx +++ b/public/app/features/alerting/routes.tsx @@ -235,7 +235,7 @@ const unifiedRoutes: RouteDescriptor[] = [ pageClass: 'page-alerting', roles: evaluateAccess( [AccessControlAction.AlertingRuleCreate, AccessControlAction.AlertingRuleExternalWrite], - [OrgRole.Editor, OrgRole.Admin] + [OrgRole.Viewer, OrgRole.Editor, OrgRole.Admin] // Needs to include viewer because there may be Viewers with Edit permissions in folders ), component: SafeDynamicImport( () => import(/* webpackChunkName: "AlertingRuleForm"*/ 'app/features/alerting/unified/RuleEditor') @@ -246,7 +246,7 @@ const unifiedRoutes: RouteDescriptor[] = [ pageClass: 'page-alerting', roles: evaluateAccess( [AccessControlAction.AlertingRuleUpdate, AccessControlAction.AlertingRuleExternalWrite], - [OrgRole.Editor, OrgRole.Admin] + [OrgRole.Viewer, OrgRole.Editor, OrgRole.Admin] // Needs to include viewer because there may be Viewers with Edit permissions in folders ), component: SafeDynamicImport( () => import(/* webpackChunkName: "AlertingRuleForm"*/ 'app/features/alerting/unified/RuleEditor') diff --git a/public/app/features/alerting/unified/RuleEditor.test.tsx b/public/app/features/alerting/unified/RuleEditor.test.tsx index 6762a9be554..a140d9e7be4 100644 --- a/public/app/features/alerting/unified/RuleEditor.test.tsx +++ b/public/app/features/alerting/unified/RuleEditor.test.tsx @@ -98,6 +98,7 @@ describe('RuleEditor', () => { beforeEach(() => { jest.resetAllMocks(); contextSrv.isEditor = true; + contextSrv.hasEditPermissionInFolders = true; }); disableRBAC(); diff --git a/public/app/features/alerting/unified/RuleList.test.tsx b/public/app/features/alerting/unified/RuleList.test.tsx index 1eb9c5dc23c..177b8b904f9 100644 --- a/public/app/features/alerting/unified/RuleList.test.tsx +++ b/public/app/features/alerting/unified/RuleList.test.tsx @@ -7,6 +7,7 @@ import { Router } from 'react-router-dom'; import { byLabelText, byRole, byTestId, byText } from 'testing-library-selector'; import { locationService, setDataSourceSrv } from '@grafana/runtime'; +import { contextSrv } from 'app/core/services/context_srv'; import { configureStore } from 'app/store/configureStore'; import { AccessControlAction } from 'app/types'; import { PromAlertingRuleState, PromApplication } from 'app/types/unified-alerting-dto'; @@ -116,6 +117,10 @@ const ui = { }; describe('RuleList', () => { + beforeEach(() => { + contextSrv.isEditor = true; + }); + afterEach(() => { jest.resetAllMocks(); setDataSourceSrv(undefined as any); diff --git a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/AlertType.tsx b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/AlertType.tsx index f022b78d33a..f406b94855d 100644 --- a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/AlertType.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/AlertType.tsx @@ -85,8 +85,11 @@ export const AlertType: FC = ({ editingExistingRule }) => { }; function getAvailableRuleTypes() { - const canCreateGrafanaRules = contextSrv.hasPermission(AccessControlAction.AlertingRuleCreate); - const canCreateCloudRules = contextSrv.hasPermission(AccessControlAction.AlertingRuleExternalWrite); + const canCreateGrafanaRules = contextSrv.hasAccess( + AccessControlAction.AlertingRuleCreate, + contextSrv.hasEditPermissionInFolders + ); + const canCreateCloudRules = contextSrv.hasAccess(AccessControlAction.AlertingRuleExternalWrite, contextSrv.isEditor); const defaultRuleType = canCreateGrafanaRules ? RuleFormType.grafana : RuleFormType.cloudAlerting; const enabledRuleTypes: RuleFormType[] = []; diff --git a/public/app/features/alerting/unified/components/rules/RulesGroup.test.tsx b/public/app/features/alerting/unified/components/rules/RulesGroup.test.tsx index c4513bcaeb4..6a0fb892c40 100644 --- a/public/app/features/alerting/unified/components/rules/RulesGroup.test.tsx +++ b/public/app/features/alerting/unified/components/rules/RulesGroup.test.tsx @@ -4,6 +4,7 @@ import React from 'react'; import { Provider } from 'react-redux'; import { byTestId, byText } from 'testing-library-selector'; +import { contextSrv } from 'app/core/services/context_srv'; import { configureStore } from 'app/store/configureStore'; import { CombinedRuleGroup, CombinedRuleNamespace } from 'app/types/unified-alerting'; @@ -16,7 +17,9 @@ jest.mock('../../hooks/useHasRuler', () => ({ useHasRuler: () => hasRulerMock, })); -beforeEach(() => hasRulerMock.mockReset()); +beforeEach(() => { + hasRulerMock.mockReset(); +}); const ui = { editGroupButton: byTestId('edit-group'), @@ -61,6 +64,10 @@ describe('Rules group tests', () => { }); describe('When the datasource is not grafana', () => { + beforeEach(() => { + contextSrv.isEditor = true; + }); + const group: CombinedRuleGroup = { name: 'TestGroup', rules: [mockCombinedRule()], diff --git a/public/app/features/alerting/unified/components/rules/RulesGroup.tsx b/public/app/features/alerting/unified/components/rules/RulesGroup.tsx index d87303d3760..be9c29a91f8 100644 --- a/public/app/features/alerting/unified/components/rules/RulesGroup.tsx +++ b/public/app/features/alerting/unified/components/rules/RulesGroup.tsx @@ -5,14 +5,13 @@ import { useDispatch } from 'react-redux'; import { GrafanaTheme2 } from '@grafana/data'; import { Badge, ConfirmModal, HorizontalGroup, Icon, Spinner, Tooltip, useStyles2 } from '@grafana/ui'; -import { contextSrv } from 'app/core/services/context_srv'; import kbn from 'app/core/utils/kbn'; -import { AccessControlAction } from 'app/types'; import { CombinedRuleGroup, CombinedRuleNamespace } from 'app/types/unified-alerting'; import { useFolder } from '../../hooks/useFolder'; import { useHasRuler } from '../../hooks/useHasRuler'; import { deleteRulesGroupAction } from '../../state/actions'; +import { useRulesAccess } from '../../utils/accessControlHooks'; import { GRAFANA_RULES_SOURCE_NAME, isCloudRulesSource } from '../../utils/datasource'; import { isFederatedRuleGroup, isGrafanaRulerRule } from '../../utils/rules'; import { CollapseToggle } from '../CollapseToggle'; @@ -38,7 +37,7 @@ export const RulesGroup: FC = React.memo(({ group, namespace, expandAll } const [isDeletingGroup, setIsDeletingGroup] = useState(false); const [isCollapsed, setIsCollapsed] = useState(!expandAll); - const canEditCloudRules = contextSrv.hasPermission(AccessControlAction.AlertingRuleExternalWrite); + const { canEditRules } = useRulesAccess(); useEffect(() => { setIsCollapsed(!expandAll); @@ -96,7 +95,7 @@ export const RulesGroup: FC = React.memo(({ group, namespace, expandAll } ); } } - } else if (canEditCloudRules && hasRuler(rulesSource)) { + } else if (canEditRules(rulesSource.name) && hasRuler(rulesSource)) { if (!isFederated) { actionIcons.push( { describe('RBAC enabled', () => { - jest.spyOn(contextSrv, 'accessControlEnabled').mockReturnValue(true); + enableRBAC(); describe('Grafana rules', () => { it('Should allow editing when the user has the alert rule update permission and folder permissions', () => { mockPermissions([AccessControlAction.AlertingRuleUpdate]); @@ -81,6 +81,10 @@ describe('useIsRuleEditable', () => { }); describe('Cloud rules', () => { + beforeEach(() => { + contextSrv.isEditor = true; + }); + it('Should allow editing and deleting when the user has alert rule external write permission', () => { mockPermissions([AccessControlAction.AlertingRuleExternalWrite]); const wrapper = getProviderWrapper(); diff --git a/public/app/features/alerting/unified/hooks/useIsRuleEditable.ts b/public/app/features/alerting/unified/hooks/useIsRuleEditable.ts index 84ba418c685..d2a7b7d4445 100644 --- a/public/app/features/alerting/unified/hooks/useIsRuleEditable.ts +++ b/public/app/features/alerting/unified/hooks/useIsRuleEditable.ts @@ -18,8 +18,8 @@ export function useIsRuleEditable(rulesSourceName: string, rule?: RulerRuleDTO): const folderUID = rule && isGrafanaRulerRule(rule) ? rule.grafana_alert.namespace_uid : undefined; const rulePermission = getRulesPermissions(rulesSourceName); - const hasEditPermission = contextSrv.hasAccess(rulePermission.update, contextSrv.isEditor); - const hasRemovePermission = contextSrv.hasAccess(rulePermission.delete, contextSrv.isEditor); + const hasEditPermission = contextSrv.hasPermission(rulePermission.update); + const hasRemovePermission = contextSrv.hasPermission(rulePermission.delete); const { folder, loading } = useFolder(folderUID); @@ -27,7 +27,8 @@ export function useIsRuleEditable(rulesSourceName: string, rule?: RulerRuleDTO): return { isEditable: false, isRemovable: false, loading: false }; } - // grafana rules can be edited if user can edit the folder they're in + // Grafana rules can be edited if user can edit the folder they're in + // When RBAC is disabled access to a folder is the only requirement for managing rules if (isGrafanaRulerRule(rule)) { if (!folderUID) { throw new Error( @@ -44,8 +45,8 @@ export function useIsRuleEditable(rulesSourceName: string, rule?: RulerRuleDTO): // prom rules are only editable by users with Editor role and only if rules source supports editing const isRulerAvailable = Boolean(dataSources[rulesSourceName]?.result?.rulerConfig); return { - isEditable: hasEditPermission && isRulerAvailable, - isRemovable: hasRemovePermission && isRulerAvailable, + isEditable: hasEditPermission && contextSrv.isEditor && isRulerAvailable, + isRemovable: hasRemovePermission && contextSrv.isEditor && isRulerAvailable, loading: dataSources[rulesSourceName]?.loading, }; } diff --git a/public/app/features/alerting/unified/utils/access-control.ts b/public/app/features/alerting/unified/utils/access-control.ts index 6561b005dcc..1ccd6399ab0 100644 --- a/public/app/features/alerting/unified/utils/access-control.ts +++ b/public/app/features/alerting/unified/utils/access-control.ts @@ -1,7 +1,7 @@ import { contextSrv } from 'app/core/services/context_srv'; import { AccessControlAction } from 'app/types'; -import { isGrafanaRulesSource } from './datasource'; +import { GRAFANA_RULES_SOURCE_NAME, isGrafanaRulesSource } from './datasource'; type RulesSourceType = 'grafana' | 'external'; @@ -108,12 +108,15 @@ export function evaluateAccess(actions: AccessControlAction[], fallBackUserRoles export function getRulesAccess() { return { canCreateGrafanaRules: - contextSrv.hasAccess(AccessControlAction.FoldersRead, contextSrv.isEditor) && - contextSrv.hasAccess(rulesPermissions.create.grafana, contextSrv.isEditor), + contextSrv.hasAccess(AccessControlAction.FoldersRead, contextSrv.hasEditPermissionInFolders) && + contextSrv.hasAccess(rulesPermissions.create.grafana, contextSrv.hasEditPermissionInFolders), canCreateCloudRules: contextSrv.hasAccess(AccessControlAction.DataSourcesRead, contextSrv.isEditor) && contextSrv.hasAccess(rulesPermissions.create.external, contextSrv.isEditor), - canEditRules: (rulesSourceName: string) => - contextSrv.hasAccess(getRulesPermissions(rulesSourceName).update, contextSrv.isEditor), + canEditRules: (rulesSourceName: string) => { + const permissionFallback = + rulesSourceName === GRAFANA_RULES_SOURCE_NAME ? contextSrv.hasEditPermissionInFolders : contextSrv.isEditor; + return contextSrv.hasAccess(getRulesPermissions(rulesSourceName).update, permissionFallback); + }, }; }