From eb7d4cba42f50adfa6a22b54911dc71f0abc2a87 Mon Sep 17 00:00:00 2001 From: endimonan <65144790+endimonan@users.noreply.github.com> Date: Thu, 13 Aug 2026 03:51:39 +0200 Subject: [PATCH] fix(explore): hide Superset annotation source for users without annotation access (#43006) Co-authored-by: Evan Rusackas --- .../AnnotationLayer.subdirectory.test.tsx | 1 + .../AnnotationLayer.test.tsx | 147 +++++++++++++++++- .../AnnotationLayer.tsx | 90 ++++++++--- .../AnnotationLayerControl/index.test.tsx | 52 +++++++ .../controls/AnnotationLayerControl/index.tsx | 15 +- 5 files changed, 277 insertions(+), 28 deletions(-) create mode 100644 superset-frontend/src/explore/components/controls/AnnotationLayerControl/index.test.tsx diff --git a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.subdirectory.test.tsx b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.subdirectory.test.tsx index 8cf57bc96c3..6df6bf1b4e1 100644 --- a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.subdirectory.test.tsx +++ b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.subdirectory.test.tsx @@ -54,6 +54,7 @@ const defaultProps = { vizType: VizType.Table, annotationType: ANNOTATION_TYPES_METADATA.EVENT.value, sourceType: 'Table', + canReadAnnotation: true, }; beforeAll(() => { diff --git a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.test.tsx b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.test.tsx index d9a8cd04d03..fbad8478620 100644 --- a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.test.tsx +++ b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.test.tsx @@ -40,6 +40,7 @@ const defaultProps = { value: '', vizType: VizType.Table, annotationType: ANNOTATION_TYPES_METADATA.FORMULA.value, + canReadAnnotation: true, }; const nativeLayerApiRoute = 'glob:*/api/v1/annotation_layer/*'; @@ -47,6 +48,11 @@ const chartApiRoute = /\/api\/v1\/chart\/\?q=.+/; const chartApiWithIdRoute = /\/api\/v1\/chart\/\w+\?q=.+/; const chartApiWithIdRouteName = 'chart-with-id'; +const nativeLayerRouteName = 'native-layer'; + +const nativeLayerResult = { + result: [{ name: 'Chart A', id: 'a' }], +}; const withIdResult = { result: { @@ -107,8 +113,8 @@ beforeAll(() => { value => value.value, ); - fetchMock.get(nativeLayerApiRoute, { - result: [{ name: 'Chart A', id: 'a' }], + fetchMock.get(nativeLayerApiRoute, nativeLayerResult, { + name: nativeLayerRouteName, }); fetchMock.get(chartApiRoute, { @@ -132,6 +138,12 @@ beforeAll(() => { ); }); +// Call history is shared across tests; without this, call-count assertions +// depend on execution order and fail under `jest --randomize`. +beforeEach(() => { + fetchMock.clearHistory(); +}); + const waitForRender = (props?: any) => waitFor(() => render()); @@ -259,6 +271,137 @@ test('fetches chart on mount if value present', async () => { expect(fetchMock.callHistory.calls(chartApiWithIdRoute).length).toBe(1); }); +test('hides the Superset annotation source without annotation read access', async () => { + await waitForRender({ + annotationType: ANNOTATION_TYPES_METADATA.EVENT.value, + canReadAnnotation: false, + }); + userEvent.click( + screen.getByRole('combobox', { name: 'Annotation source type' }), + ); + expect(await screen.findByText('Table')).toBeInTheDocument(); + expect(screen.queryByText('Superset annotation')).not.toBeInTheDocument(); +}); + +test('keeps formula annotations available without annotation read access', async () => { + await waitForRender({ canReadAnnotation: false }); + expect(screen.getByRole('textbox', { name: 'Formula' })).toBeInTheDocument(); +}); + +test('keeps a saved native layer intact without annotation read access', async () => { + const addAnnotationLayer = jest.fn(); + await waitForRender({ + name: 'Test', + value: 1, + annotationType: ANNOTATION_TYPES_METADATA.EVENT.value, + sourceType: 'NATIVE', + canReadAnnotation: false, + addAnnotationLayer, + }); + + // The saved source stays selected, and the value select is inert with an + // explanation instead of surfacing a Forbidden error. + expect(await screen.findByText('Superset annotation')).toBeInTheDocument(); + expect( + screen.getByRole('combobox', { name: 'Annotation layer value' }), + ).toBeDisabled(); + expect( + screen.getByText("You don't have permission to view annotation layers."), + ).toBeInTheDocument(); + + // The saved reference is still valid: re-applying preserves it as is. + userEvent.click(screen.getByRole('button', { name: 'Apply' })); + expect(addAnnotationLayer).toHaveBeenCalledWith( + expect.objectContaining({ + sourceType: 'NATIVE', + value: 1, + }), + ); + + // Neither the by-id fetch nor the listing may fire; both are known 403s. + expect(fetchMock.callHistory.calls(nativeLayerApiRoute).length).toBe(0); +}); + +test('hydrates the applied native layer name for authorized users', async () => { + // The show endpoint returns a single object, unlike the list mock. + fetchMock.modifyRoute(nativeLayerRouteName, { + response: { result: { id: 1, name: 'My layer' } }, + }); + + try { + await waitForRender({ + name: 'Test', + value: 1, + annotationType: ANNOTATION_TYPES_METADATA.EVENT.value, + sourceType: 'NATIVE', + }); + + expect(await screen.findByText('My layer')).toBeInTheDocument(); + expect(fetchMock.callHistory.calls(nativeLayerApiRoute).length).toBe(1); + } finally { + fetchMock.modifyRoute(nativeLayerRouteName, { + response: nativeLayerResult, + }); + } +}); + +test('lets a saved native layer switch to a permitted source', async () => { + await waitForRender({ + name: 'Test', + value: 1, + annotationType: ANNOTATION_TYPES_METADATA.EVENT.value, + sourceType: 'NATIVE', + canReadAnnotation: false, + }); + + userEvent.click( + screen.getByRole('combobox', { name: 'Annotation source type' }), + ); + userEvent.click(await screen.findByText('Table')); + + // The chart selector takes over, enabled. + expect(await screen.findByText('Chart')).toBeInTheDocument(); + expect( + screen.getByRole('combobox', { name: 'Annotation layer value' }), + ).toBeEnabled(); + + // Reopen the source dropdown: it re-renders from the new options, and the + // native option is gone for good. + userEvent.click( + screen.getByRole('combobox', { name: 'Annotation source type' }), + ); + await waitFor(() => + expect(screen.queryByText('Superset annotation')).not.toBeInTheDocument(), + ); +}); + +test('survives a native annotation layer fetch that fails', async () => { + const logError = jest.spyOn(logging, 'error').mockImplementation(() => {}); + fetchMock.modifyRoute(nativeLayerRouteName, { response: 403 }); + + try { + await waitForRender({ + name: 'Test', + value: 1, + annotationType: ANNOTATION_TYPES_METADATA.EVENT.value, + sourceType: 'NATIVE', + }); + + expect(screen.getByRole('textbox', { name: 'Name' })).toBeInTheDocument(); + await waitFor(() => + expect(logError).toHaveBeenCalledWith( + expect.stringContaining('Failed to load annotation layer 1'), + expect.anything(), + ), + ); + } finally { + fetchMock.modifyRoute(nativeLayerRouteName, { + response: nativeLayerResult, + }); + logError.mockRestore(); + } +}); + test('keeps apply disabled when missing required fields', async () => { // With EVENT type and Table source, the component requires selecting a chart // and filling in required fields. Without completing these, Apply should be disabled. diff --git a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.tsx b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.tsx index 3cbfe4510e8..9f1b9abb33f 100644 --- a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.tsx +++ b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.tsx @@ -30,6 +30,7 @@ import { AsyncSelect, EmptyState, ColorPicker, + Typography, } from '@superset-ui/core/components'; import { SupersetClient, @@ -105,6 +106,7 @@ interface AnnotationLayerProps { vizType?: string; error?: string; colorScheme?: string; + canReadAnnotation: boolean; addAnnotationLayer?: (annotation: Record) => void; removeAnnotationLayer?: () => void; close?: () => void; @@ -166,6 +168,10 @@ const getSliceFormData = ( const reportChartFailure = (id: string | number) => (error: unknown) => logging.error(`Failed to load annotation source chart ${id}`, error); +const reportAnnotationLayerFailure = + (id: string | number) => (error: unknown) => + logging.error(`Failed to load annotation layer ${id}`, error); + const toSliceData = (formData: Record): SliceData => ({ data: { ...formData, @@ -232,6 +238,7 @@ function AnnotationLayer({ vizType, error, colorScheme = 'd3Category10', + canReadAnnotation, addAnnotationLayer = () => {}, removeAnnotationLayer = () => {}, close = () => {}, @@ -307,17 +314,24 @@ function AnnotationLayer({ value: key === VizType.Line ? 'line' : key, label: chartMetadata?.name || key, })); - // Prepend native source if applicable + // Prepend native source if applicable. Listing native annotation layers + // requires can_read on Annotation; without it the option is offered only + // while it is the layer's current selection, so a saved native layer + // stays intact instead of being silently invalidated. const annotationMeta = ANNOTATION_TYPES_METADATA[ annoType as keyof typeof ANNOTATION_TYPES_METADATA ]; - if (annotationMeta && 'supportNativeSource' in annotationMeta) { + if ( + annotationMeta && + 'supportNativeSource' in annotationMeta && + (canReadAnnotation || sourceType === ANNOTATION_SOURCE_TYPES.NATIVE) + ) { sources.unshift(ANNOTATION_SOURCE_TYPES_METADATA.NATIVE); } return sources; }, - [], + [canReadAnnotation, sourceType], ); const shouldFetchAppliedAnnotation = useCallback( @@ -488,14 +502,16 @@ function AnnotationLayer({ (id: string | number): void => { SupersetClient.get({ endpoint: `/api/v1/annotation_layer/${id}`, - }).then(({ json }) => { - const { result } = json; - const layer = result; - setValue({ - value: layer.id, - label: layer.name, - }); - }); + }) + .then(({ json }) => { + const { result } = json; + const layer = result; + setValue({ + value: layer.id, + label: layer.name, + }); + }) + .catch(reportAnnotationLayerFailure(id)); }, [], ); @@ -503,12 +519,21 @@ function AnnotationLayer({ const fetchAppliedAnnotation = useCallback( (id: string | number): void => { if (sourceType === ANNOTATION_SOURCE_TYPES.NATIVE) { - fetchAppliedNativeAnnotation(id); + // Without can_read on Annotation the request is known to 403; keep the + // raw id as the value so the saved layer remains valid and untouched. + if (canReadAnnotation) { + fetchAppliedNativeAnnotation(id); + } } else { fetchAppliedChart(id); } }, - [sourceType, fetchAppliedNativeAnnotation, fetchAppliedChart], + [ + sourceType, + canReadAnnotation, + fetchAppliedNativeAnnotation, + fetchAppliedChart, + ], ); // componentDidMount - fetch applied annotation if needed @@ -754,18 +779,34 @@ function AnnotationLayer({ Example: '2x+5'`); } if (requiresQuery(sourceType ?? undefined)) { + // Listing native annotation layers requires can_read on Annotation. + // Keep the select visible but inert so the saved reference can still be + // removed, restyled, or switched to a permitted source; the select stays + // lazy, so no forbidden request is ever fired. + const isBlockedNativeSource = + sourceType === ANNOTATION_SOURCE_TYPES.NATIVE && !canReadAnnotation; return ( - } - /> + <> + } + disabled={isBlockedNativeSource} + /> + {isBlockedNativeSource && ( +
+ + {t("You don't have permission to view annotation layers.")} + +
+ )} + ); } if (annotationType === ANNOTATION_TYPES.FORMULA) { @@ -794,6 +835,7 @@ function AnnotationLayer({ sourceType, annotationType, value, + canReadAnnotation, getSupportedSourceTypes, fetchOptions, handleSelectValue, diff --git a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/index.test.tsx b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/index.test.tsx new file mode 100644 index 00000000000..0e5ed6bfa48 --- /dev/null +++ b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/index.test.tsx @@ -0,0 +1,52 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { mapStateToProps } from './index'; + +type State = Parameters[0]; + +const buildState = (user: unknown): State => + ({ + charts: {}, + explore: { + controls: { + viz_type: { value: 'line' }, + color_scheme: { value: 'supersetColors' }, + }, + }, + user, + }) as unknown as State; + +test('grants canReadAnnotation when a role holds can_read on Annotation', () => { + const state = buildState({ + roles: { Gamma: [['can_read', 'Annotation']] }, + }); + expect(mapStateToProps(state).canReadAnnotation).toBe(true); +}); + +test('denies canReadAnnotation when no role holds the permission', () => { + const state = buildState({ + roles: { Gamma: [['can_read', 'Chart']] }, + }); + expect(mapStateToProps(state).canReadAnnotation).toBe(false); +}); + +test('denies canReadAnnotation when the user has no roles', () => { + expect(mapStateToProps(buildState({})).canReadAnnotation).toBe(false); + expect(mapStateToProps(buildState(undefined)).canReadAnnotation).toBe(false); +}); diff --git a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/index.tsx b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/index.tsx index dffdd8e62f9..41f83c84540 100644 --- a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/index.tsx +++ b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/index.tsx @@ -33,6 +33,7 @@ import { } from '@superset-ui/core/components'; import { getChartKey } from 'src/explore/exploreUtils'; import { runAnnotationQuery } from 'src/components/Chart/chartAction'; +import { findPermission } from 'src/utils/findPermission'; import CustomListItem from 'src/explore/components/controls/CustomListItem'; import { ChartState, ExplorePageState } from 'src/explore/types'; import { AnyAction } from 'redux'; @@ -64,6 +65,7 @@ export interface Props { annotationError: Record; annotationQuery: Record; vizType: string; + canReadAnnotation: boolean; validationErrors: JsonObject[]; name: string; actions: { @@ -85,6 +87,7 @@ function AnnotationLayerControl({ annotationError = {}, annotationQuery = {}, vizType = '', + canReadAnnotation, validationErrors, name, actions, @@ -180,6 +183,7 @@ function AnnotationLayerControl({ error={error} colorScheme={colorScheme} vizType={vizType} + canReadAnnotation={canReadAnnotation} addAnnotationLayer={(newAnnotation: Annotation) => addAnnotationLayer(annotation, newAnnotation) } @@ -195,6 +199,7 @@ function AnnotationLayerControl({ [ colorScheme, vizType, + canReadAnnotation, addAnnotationLayer, removeAnnotationLayer, handleVisibleChange, @@ -283,10 +288,13 @@ function AnnotationLayerControl({ // Tried to hook this up through stores/control.jsx instead of using redux // directly, could not figure out how to get access to the color_scheme -function mapStateToProps({ +// Exported for tests: the permission wiring below is not covered by tsc +// (a missing state prop silently falls through to untyped ownProps). +export function mapStateToProps({ charts, explore, -}: Pick) { + user, +}: Pick) { const chartKey = getChartKey(explore); const defaultChartState: Partial = { @@ -303,6 +311,9 @@ function mapStateToProps({ annotationError: chart.annotationError ?? {}, annotationQuery: chart.annotationQuery ?? {}, vizType: explore.controls?.viz_type.value, + // Mirrors the backend gate on GET /api/v1/annotation_layer/ + // (class_permission_name "Annotation", get_list -> can_read). + canReadAnnotation: findPermission('can_read', 'Annotation', user?.roles), }; }