From e1243e07ca029bcaf94f09c50c5650960ef3268d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20H=C3=A4ggmark?= Date: Thu, 21 Jan 2021 06:19:03 +0100 Subject: [PATCH] Graph: Fixes so only users with correct permissions can add annotations (#30419) * Graph: Fixes so only users with edit permissions can add annotations * Tests: corrects test message text * Chore: changes after PR comments --- .../annotations/annotation_tooltip.ts | 2 +- .../dashboard/state/DashboardModel.test.ts | 21 +++++ .../dashboard/state/DashboardModel.ts | 4 + public/app/plugins/panel/graph/graph.ts | 6 +- .../plugins/panel/graph/specs/graph.test.ts | 81 ++++++++++++------- 5 files changed, 79 insertions(+), 35 deletions(-) diff --git a/public/app/features/annotations/annotation_tooltip.ts b/public/app/features/annotations/annotation_tooltip.ts index 7af08278183..df6fb9e7d09 100644 --- a/public/app/features/annotations/annotation_tooltip.ts +++ b/public/app/features/annotations/annotation_tooltip.ts @@ -59,7 +59,7 @@ export function annotationTooltipDirective( `; // Show edit icon only for users with at least Editor role - if (event.id && dashboard.meta.canEdit) { + if (event.id && dashboard.canAddAnnotations()) { header += ` diff --git a/public/app/features/dashboard/state/DashboardModel.test.ts b/public/app/features/dashboard/state/DashboardModel.test.ts index 1c86ce885fb..17f240e4086 100644 --- a/public/app/features/dashboard/state/DashboardModel.test.ts +++ b/public/app/features/dashboard/state/DashboardModel.test.ts @@ -684,4 +684,25 @@ describe('DashboardModel', () => { expect(legendsOn.length).toBe(0); }); }); + + describe('canAddAnnotations', () => { + it.each` + canEdit | canMakeEditable | expected + ${false} | ${false} | ${false} + ${false} | ${true} | ${true} + ${true} | ${false} | ${true} + ${true} | ${true} | ${true} + `( + 'when called with canEdit:{$canEdit}, canMakeEditable:{$canMakeEditable} and expected:{$expected}', + ({ canEdit, canMakeEditable, expected }) => { + const dashboard = new DashboardModel({}); + dashboard.meta.canEdit = canEdit; + dashboard.meta.canMakeEditable = canMakeEditable; + + const result = dashboard.canAddAnnotations(); + + expect(result).toBe(expected); + } + ); + }); }); diff --git a/public/app/features/dashboard/state/DashboardModel.ts b/public/app/features/dashboard/state/DashboardModel.ts index 333610ce1bc..63a0c13c356 100644 --- a/public/app/features/dashboard/state/DashboardModel.ts +++ b/public/app/features/dashboard/state/DashboardModel.ts @@ -1016,6 +1016,10 @@ export class DashboardModel { return this.getVariablesFromState(); }; + canAddAnnotations() { + return this.meta.canEdit || this.meta.canMakeEditable; + } + private getPanelRepeatVariable(panel: PanelModel) { return this.getVariablesFromState().find((variable) => variable.name === panel.repeat); } diff --git a/public/app/plugins/panel/graph/graph.ts b/public/app/plugins/panel/graph/graph.ts index 872a04e80de..c93c5ff7a3d 100644 --- a/public/app/plugins/panel/graph/graph.ts +++ b/public/app/plugins/panel/graph/graph.ts @@ -180,7 +180,7 @@ class GraphElement { return; } - if ((ranges.ctrlKey || ranges.metaKey) && (this.dashboard.meta.canEdit || this.dashboard.meta.canMakeEditable)) { + if ((ranges.ctrlKey || ranges.metaKey) && this.dashboard.canAddAnnotations()) { // Add annotation setTimeout(() => { this.eventManager.updateTime(ranges.xaxis); @@ -201,7 +201,7 @@ class GraphElement { ): (() => MenuItemsGroup[]) => { return () => { // Fixed context menu items - const items: MenuItemsGroup[] = this.dashboard?.editable + const items: MenuItemsGroup[] = this.dashboard.canAddAnnotations() ? [ { items: [ @@ -253,7 +253,7 @@ class GraphElement { } // skip if dashboard is not saved yet (exists in db) or user cannot edit - if (!this.dashboard.id || (!this.dashboard.meta.canEdit && !this.dashboard.meta.canMakeEditable)) { + if (!this.dashboard.id || !this.dashboard.canAddAnnotations()) { return; } diff --git a/public/app/plugins/panel/graph/specs/graph.test.ts b/public/app/plugins/panel/graph/specs/graph.test.ts index b64c50d38ce..a2826688e76 100644 --- a/public/app/plugins/panel/graph/specs/graph.test.ts +++ b/public/app/plugins/panel/graph/specs/graph.test.ts @@ -1,3 +1,15 @@ +import '../module'; +import { GraphCtrl } from '../module'; +import { MetricsPanelCtrl } from 'app/features/panel/metrics_panel_ctrl'; +import { PanelCtrl } from 'app/features/panel/panel_ctrl'; +import config from 'app/core/config'; + +import TimeSeries from 'app/core/time_series2'; +import $ from 'jquery'; +import { graphDirective, GraphElement } from '../graph'; +import { dateTime, EventBusSrv } from '@grafana/data'; +import { DashboardModel } from '../../../../features/dashboard/state'; + jest.mock('app/features/annotations/all', () => ({ EventManager: () => { return { @@ -16,17 +28,6 @@ jest.mock('app/core/core', () => ({ }, })); -import '../module'; -import { GraphCtrl } from '../module'; -import { MetricsPanelCtrl } from 'app/features/panel/metrics_panel_ctrl'; -import { PanelCtrl } from 'app/features/panel/panel_ctrl'; -import config from 'app/core/config'; - -import TimeSeries from 'app/core/time_series2'; -import $ from 'jquery'; -import { graphDirective, GraphElement } from '../graph'; -import { dateTime, EventBusSrv } from '@grafana/data'; - const ctx = {} as any; let ctrl: any; const scope = { @@ -1288,25 +1289,9 @@ describe('grafanaGraph', () => { }); describe('getContextMenuItemsSupplier', () => { - function getGraphElement({ editable }: { editable?: boolean } = {}) { - const element = new GraphElement( - { - ctrl: { - contextMenuCtrl: {}, - dashboard: { editable, events: { on: jest.fn() } }, - events: { on: jest.fn() }, - }, - }, - { mouseleave: jest.fn(), bind: jest.fn() } as any, - {} as any - ); - - return element; - } - - describe('when called and dashboard is editable', () => { + describe('when called and user can edit the dashboard', () => { it('then the correct menu items should be returned', () => { - const element = getGraphElement({ editable: true }); + const element = getGraphElement({ canEdit: true, canMakeEditable: false }); const result = element.getContextMenuItemsSupplier({ x: 1, y: 1 })(); @@ -1318,9 +1303,23 @@ describe('grafanaGraph', () => { }); }); - describe('when called and dashboard is not editable', () => { + describe('when called and user can make the dashboard editable', () => { it('then the correct menu items should be returned', () => { - const element = getGraphElement({ editable: false }); + const element = getGraphElement({ canEdit: false, canMakeEditable: true }); + + const result = element.getContextMenuItemsSupplier({ x: 1, y: 1 })(); + + expect(result.length).toEqual(1); + expect(result[0].items.length).toEqual(1); + expect(result[0].items[0].label).toEqual('Add annotation'); + expect(result[0].items[0].icon).toEqual('comment-alt'); + expect(result[0].items[0].onClick).toBeDefined(); + }); + }); + + describe('when called and user can not edit the dashboard and can not make the dashboard editable', () => { + it('then the correct menu items should be returned', () => { + const element = getGraphElement({ canEdit: false, canMakeEditable: false }); const result = element.getContextMenuItemsSupplier({ x: 1, y: 1 })(); @@ -1329,3 +1328,23 @@ describe('grafanaGraph', () => { }); }); }); + +function getGraphElement({ canEdit, canMakeEditable }: { canEdit?: boolean; canMakeEditable?: boolean } = {}) { + const dashboard = new DashboardModel({}); + dashboard.events.on = jest.fn(); + dashboard.meta.canEdit = canEdit; + dashboard.meta.canMakeEditable = canMakeEditable; + const element = new GraphElement( + { + ctrl: { + contextMenuCtrl: {}, + dashboard, + events: { on: jest.fn() }, + }, + }, + { mouseleave: jest.fn(), bind: jest.fn() } as any, + {} as any + ); + + return element; +}