rusackas commented on code in PR #40815: URL: https://github.com/apache/superset/pull/40815#discussion_r4018365106
########## superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/useMatrixifyAllowedValues.ts: ########## @@ -0,0 +1,145 @@ +/** + * 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 { useEffect, useMemo, useState } from 'react'; +import { SupersetClient } from '../../..'; +import { getMatrixifyConfig, MatrixifyFormData } from '../../types/matrixify'; + +export type MatrixifyAllowedValuesStatus = 'success' | 'loading' | 'error'; + +export interface MatrixifyAllowedValuesState { + status: MatrixifyAllowedValuesStatus; + /** + * Map of dimension column name -> set of string-normalized values the + * current viewer is allowed to see (RLS applied by the backend). + */ + allowedByColumn: Record<string, Set<string>>; +} + +/** + * Collect the distinct dimension columns referenced by the matrixify axes. + */ +function getDimensionColumns(formData: MatrixifyFormData): string[] { + const config = getMatrixifyConfig(formData as any); + if (!config) { + return []; + } + const columns = new Set<string>(); + [config.rows, config.columns].forEach(axis => { + if (axis.mode === 'dimensions' && axis.dimension?.dimension) { + columns.add(axis.dimension.dimension); + } + }); + return Array.from(columns); +} + +/** + * Fetch the distinct values a viewer is permitted to see for a column. This + * reuses the datasource ``/column/<col>/values/`` endpoint, which applies the + * requesting user's row-level security filters server-side. + */ +async function fetchAllowedValues( + datasource: string, + column: string, + signal: AbortSignal, +): Promise<any[]> { + const [id, type] = String(datasource).split('__'); + const endpoint = `/api/v1/datasource/${type}/${id}/column/${encodeURIComponent( + column, + )}/values/`; + const { json } = await SupersetClient.get({ endpoint, signal }); + return json?.result || []; +} + +/** + * Resolve, per render, which dimension values the current viewer is allowed to + * see. Matrixify axis values are frozen into ``formData`` at design time, so + * without this the grid would be built from the chart author's RLS context and + * leak values (as subplot headers + empty cells) to restricted viewers. The + * renderer intersects the stored values against the returned allow-list. + * + * Fails closed: while loading the grid must not render, and on error the + * allow-list is treated as empty rather than falling back to the unfiltered + * (leaking) list. + */ +export function useMatrixifyAllowedValues( + formData: MatrixifyFormData, +): MatrixifyAllowedValuesState { + const datasource = (formData as any)?.datasource as string | undefined; + // Serialize the dimension columns to a primitive key. ``formData`` is a fresh + // object on most renders, so the effect must depend on this stable string + // rather than the (always-new) array, otherwise it would refetch in a loop. + const columnsKey = useMemo( + () => JSON.stringify([...getDimensionColumns(formData)].sort()), + [formData], + ); + + const [state, setState] = useState<MatrixifyAllowedValuesState>({ + status: columnsKey === '[]' ? 'success' : 'loading', + allowedByColumn: {}, + }); Review Comment: This is the stale-state race I flagged on my first pass, fixed since: the hook now derives status by comparing `resolved.key` against the live `fetchKey` (see `useMatrixifyAllowedValues.ts` on `e266594`), so a render with a new axis/datasource reports `loading` immediately instead of reusing the old `success` state. Resolving. ########## superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/useMatrixifyAllowedValues.ts: ########## @@ -0,0 +1,145 @@ +/** + * 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 { useEffect, useMemo, useState } from 'react'; +import { SupersetClient } from '../../..'; +import { getMatrixifyConfig, MatrixifyFormData } from '../../types/matrixify'; + +export type MatrixifyAllowedValuesStatus = 'success' | 'loading' | 'error'; + +export interface MatrixifyAllowedValuesState { + status: MatrixifyAllowedValuesStatus; + /** + * Map of dimension column name -> set of string-normalized values the + * current viewer is allowed to see (RLS applied by the backend). + */ + allowedByColumn: Record<string, Set<string>>; +} + +/** + * Collect the distinct dimension columns referenced by the matrixify axes. + */ +function getDimensionColumns(formData: MatrixifyFormData): string[] { + const config = getMatrixifyConfig(formData as any); + if (!config) { + return []; + } + const columns = new Set<string>(); + [config.rows, config.columns].forEach(axis => { + if (axis.mode === 'dimensions' && axis.dimension?.dimension) { + columns.add(axis.dimension.dimension); + } + }); + return Array.from(columns); +} + +/** + * Fetch the distinct values a viewer is permitted to see for a column. This + * reuses the datasource ``/column/<col>/values/`` endpoint, which applies the + * requesting user's row-level security filters server-side. + */ +async function fetchAllowedValues( + datasource: string, + column: string, + signal: AbortSignal, +): Promise<any[]> { + const [id, type] = String(datasource).split('__'); + const endpoint = `/api/v1/datasource/${type}/${id}/column/${encodeURIComponent( + column, + )}/values/`; + const { json } = await SupersetClient.get({ endpoint, signal }); + return json?.result || []; +} + +/** + * Resolve, per render, which dimension values the current viewer is allowed to + * see. Matrixify axis values are frozen into ``formData`` at design time, so + * without this the grid would be built from the chart author's RLS context and + * leak values (as subplot headers + empty cells) to restricted viewers. The + * renderer intersects the stored values against the returned allow-list. + * + * Fails closed: while loading the grid must not render, and on error the + * allow-list is treated as empty rather than falling back to the unfiltered + * (leaking) list. + */ +export function useMatrixifyAllowedValues( + formData: MatrixifyFormData, +): MatrixifyAllowedValuesState { + const datasource = (formData as any)?.datasource as string | undefined; Review Comment: Same round of cleanup, `datasource` is a typed field on `MatrixifyAllowedValuesFormData` now instead of `(formData as any)?.datasource`. No more `any` cast on this path. Resolving. ########## superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/useMatrixifyAllowedValues.ts: ########## @@ -0,0 +1,145 @@ +/** + * 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 { useEffect, useMemo, useState } from 'react'; +import { SupersetClient } from '../../..'; +import { getMatrixifyConfig, MatrixifyFormData } from '../../types/matrixify'; + +export type MatrixifyAllowedValuesStatus = 'success' | 'loading' | 'error'; + +export interface MatrixifyAllowedValuesState { + status: MatrixifyAllowedValuesStatus; + /** + * Map of dimension column name -> set of string-normalized values the + * current viewer is allowed to see (RLS applied by the backend). + */ + allowedByColumn: Record<string, Set<string>>; +} + +/** + * Collect the distinct dimension columns referenced by the matrixify axes. + */ +function getDimensionColumns(formData: MatrixifyFormData): string[] { + const config = getMatrixifyConfig(formData as any); + if (!config) { + return []; + } + const columns = new Set<string>(); + [config.rows, config.columns].forEach(axis => { + if (axis.mode === 'dimensions' && axis.dimension?.dimension) { + columns.add(axis.dimension.dimension); + } + }); + return Array.from(columns); +} + +/** + * Fetch the distinct values a viewer is permitted to see for a column. This + * reuses the datasource ``/column/<col>/values/`` endpoint, which applies the + * requesting user's row-level security filters server-side. + */ +async function fetchAllowedValues( + datasource: string, + column: string, + signal: AbortSignal, +): Promise<any[]> { + const [id, type] = String(datasource).split('__'); + const endpoint = `/api/v1/datasource/${type}/${id}/column/${encodeURIComponent( + column, + )}/values/`; + const { json } = await SupersetClient.get({ endpoint, signal }); + return json?.result || []; +} + +/** + * Resolve, per render, which dimension values the current viewer is allowed to + * see. Matrixify axis values are frozen into ``formData`` at design time, so + * without this the grid would be built from the chart author's RLS context and + * leak values (as subplot headers + empty cells) to restricted viewers. The + * renderer intersects the stored values against the returned allow-list. + * + * Fails closed: while loading the grid must not render, and on error the + * allow-list is treated as empty rather than falling back to the unfiltered + * (leaking) list. + */ +export function useMatrixifyAllowedValues( + formData: MatrixifyFormData, +): MatrixifyAllowedValuesState { + const datasource = (formData as any)?.datasource as string | undefined; + // Serialize the dimension columns to a primitive key. ``formData`` is a fresh + // object on most renders, so the effect must depend on this stable string + // rather than the (always-new) array, otherwise it would refetch in a loop. + const columnsKey = useMemo( + () => JSON.stringify([...getDimensionColumns(formData)].sort()), + [formData], + ); + + const [state, setState] = useState<MatrixifyAllowedValuesState>({ + status: columnsKey === '[]' ? 'success' : 'loading', + allowedByColumn: {}, + }); + + useEffect(() => { + const dimensionColumns: string[] = JSON.parse(columnsKey); + + // Metrics-only matrixify has no dimension axes: nothing to resolve. + if (dimensionColumns.length === 0) { + setState({ status: 'success', allowedByColumn: {} }); + return undefined; + } + + if (!datasource) { + setState({ status: 'error', allowedByColumn: {} }); + return undefined; + } + + const controller = new AbortController(); + setState(prev => ({ ...prev, status: 'loading' })); + + (async () => { + try { + const results = await Promise.all( + dimensionColumns.map(column => + fetchAllowedValues(datasource, column, controller.signal).then( + values => [column, values] as const, + ), + ), + ); + if (controller.signal.aborted) { + return; + } + const allowedByColumn: Record<string, Set<string>> = {}; + results.forEach(([column, values]) => { + allowedByColumn[column] = new Set(values.map(v => String(v))); + }); Review Comment: Think this one's a false positive. `column` only ever comes from `getDimensionColumns`, real dataset column names pulled from the chart's own `matrixify_dimension_rows`/`_columns` config, not user input. And assigning `allowedByColumn['__proto__']` on a plain object literal just swaps that one object's own prototype link, it doesn't touch `Object.prototype` globally. Resolving. ########## superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.tsx: ########## @@ -121,10 +142,48 @@ function MatrixifyGridRenderer({ height, hooks, }: MatrixifyGridRendererProps) { - // Generate grid structure from form data + // Resolve which dimension values the current viewer is allowed to see, with + // row-level security applied server-side. Matrixify axis values are frozen + // into formData at design time, so without this the grid would be built from + // the chart author's RLS context and leak values to restricted viewers. + const { status: allowedStatus, allowedByColumn } = + useMatrixifyAllowedValues(formData); Review Comment: Also looks like a false positive: the `formData` passed in is `chartProps.rawFormData` (`SuperChart.tsx`), which always carries the base `datasource` string on every chart render path, explore, dashboard, or embedded. The `datasource` prop on the renderer is a separate resolved-metadata object, not the id/type string this hook needs, so there's nothing to swap. Resolving. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
