Alerting: Fix access to alerts for viewer with editor permissions when RBAC is disabled (#49270)

* Add folder edit permission for users with Viewer role
* relax permissions required to create an alert when RBAC is disabled

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
 

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())

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(

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')

public/app/features/alerting/unified/RuleEditor.test.tsx

@@ -98,6 +98,7 @@ describe('RuleEditor', () => {
   beforeEach(() => {
     jest.resetAllMocks();
     contextSrv.isEditor = true;
+    contextSrv.hasEditPermissionInFolders = true;
   });
 
   disableRBAC();

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);

public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/AlertType.tsx

@@ -85,8 +85,11 @@ export const AlertType: FC<Props> = ({ 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[] = [];

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()],

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<Props> = 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<Props> = React.memo(({ group, namespace, expandAll }
         );
       }
     }
-  } else if (canEditCloudRules && hasRuler(rulesSource)) {
+  } else if (canEditRules(rulesSource.name) && hasRuler(rulesSource)) {
     if (!isFederated) {
       actionIcons.push(
         <ActionIcon

public/app/features/alerting/unified/hooks/useIsRuleEditable.test.tsx

@@ -6,7 +6,7 @@ import { contextSrv } from 'app/core/services/context_srv';
 import { configureStore } from 'app/store/configureStore';
 import { AccessControlAction, FolderDTO, StoreState } from 'app/types';
 
-import { mockFolder, mockRulerAlertingRule, mockRulerGrafanaRule } from '../mocks';
+import { enableRBAC, mockFolder, mockRulerAlertingRule, mockRulerGrafanaRule } from '../mocks';
 
 import { useFolder } from './useFolder';
 import { useIsRuleEditable } from './useIsRuleEditable';
@@ -21,7 +21,7 @@ const mocks = {
 
 describe('useIsRuleEditable', () => {
   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();

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,
   };
 }

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);
+    },
   };
 }