codeant-ai-for-open-source[bot] commented on code in PR #41843: URL: https://github.com/apache/superset/pull/41843#discussion_r3565423328
########## superset-frontend/src/components/ErrorMessage/DatasourceSecurityAccessErrorMessage.tsx: ########## @@ -0,0 +1,156 @@ +/** + * 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 { ReactNode } from 'react'; +import { t, tn } from '@apache-superset/core/translation'; +import { Typography } from '@superset-ui/core/components'; + +import type { ErrorMessageComponentProps } from './types'; +import { IssueCode } from './IssueCode'; +import { ErrorAlert } from './ErrorAlert'; + +interface DatasourceSecurityAccessExtra { + owners?: string[]; + link?: string; + datasource?: number | string; + datasource_name?: string; + tables?: string[]; + issue_codes?: { + code: number; + message: string; + }[]; +} + +/** + * Shown when a viewer opens a chart but lacks permission to its underlying data + * (DATASOURCE_SECURITY_ACCESS_ERROR / TABLE_SECURITY_ACCESS_ERROR). Surfaces a + * plain-language explanation, who to contact, and a "Request access" link when + * the deployment configures PERMISSION_INSTRUCTIONS_LINK. + */ +export function DatasourceSecurityAccessErrorMessage({ + error, + source, + closable, +}: ErrorMessageComponentProps<DatasourceSecurityAccessExtra | null>) { + const { extra, level, message } = error; + const isVisualization = ['dashboard', 'explore'].includes(source || ''); + + // DATASOURCE_SECURITY_ACCESS_ERROR is also raised for non-access failures + // (e.g. virtual-dataset SQL validation: "Only SELECT statements are + // allowed"). Those errors carry no access payload — render them plainly + // rather than misleading the user with request-access guidance. + const isAccessDenial = !!extra?.datasource_name || !!extra?.tables?.length; + if (!isAccessDenial) { + return ( + <ErrorAlert + errorType={t('Unexpected error')} + message={message} + type={level} + closable={closable} + /> + ); + } Review Comment: **Suggestion:** The access-denial detection is keyed only to `extra.datasource_name`/`extra.tables`, so valid `DATASOURCE_SECURITY_ACCESS_ERROR` responses that omit those fields (for example semantic-view permission denials) are incorrectly downgraded to an “Unexpected error” alert. Use the error type (or a broader security-error condition) to preserve access-denied messaging instead of treating missing payload fields as non-security failures. [incorrect condition logic] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx ⚠️ Semantic-view access errors show generic unexpected error label. ⚠️ Viewers lose clear permission-denial context and guidance. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Enable the SEMANTIC_LAYERS feature so semantic views are exposed alongside datasets, as described in `docs/static/feature-flags.json:84-88` and implemented by the `SemanticView` model in `superset/semantic_layers/models.py:194-196`. 2. Trigger a semantic view access check for a user without the required permissions; the check is performed by `SemanticView.raise_for_access()` in `superset/semantic_layers/models.py:2-24`. 3. When access is denied, `raise_for_access()` raises a `SupersetSecurityException` wrapping a `SupersetError` with `error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR`, message `"You don't have access to this semantic view."`, and no `extra` payload (same file, lines 18-23). 4. On the frontend, `superset-frontend/src/setup/setupErrorMessages.ts:20-25` maps `DATASOURCE_SECURITY_ACCESS_ERROR` to `DatasourceSecurityAccessErrorMessage`, which at `superset-frontend/src/components/ErrorMessage/DatasourceSecurityAccessErrorMessage.tsx:53-67` computes `isAccessDenial` solely from `extra.datasource_name` and `extra.tables`. Because semantic-view errors lack these fields, `isAccessDenial` is false and the component returns an `ErrorAlert` with `errorType={t('Unexpected error')}`, causing a genuine permission denial to appear as a generic unexpected error with no dedicated access-denied messaging. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=17d527ff97904889a896389a9fec508f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=17d527ff97904889a896389a9fec508f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/components/ErrorMessage/DatasourceSecurityAccessErrorMessage.tsx **Line:** 57:67 **Comment:** *Incorrect Condition Logic: The access-denial detection is keyed only to `extra.datasource_name`/`extra.tables`, so valid `DATASOURCE_SECURITY_ACCESS_ERROR` responses that omit those fields (for example semantic-view permission denials) are incorrectly downgraded to an “Unexpected error” alert. Use the error type (or a broader security-error condition) to preserve access-denied messaging instead of treating missing payload fields as non-security failures. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41843&comment_hash=5b3835f241497f93e36bc35db734f3d5291444c2ae07605f7a54faa74f660b6a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41843&comment_hash=5b3835f241497f93e36bc35db734f3d5291444c2ae07605f7a54faa74f660b6a&reaction=dislike'>👎</a> ########## superset-frontend/src/components/ErrorMessage/DatasourceSecurityAccessErrorMessage.tsx: ########## @@ -0,0 +1,156 @@ +/** + * 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 { ReactNode } from 'react'; +import { t, tn } from '@apache-superset/core/translation'; +import { Typography } from '@superset-ui/core/components'; + +import type { ErrorMessageComponentProps } from './types'; +import { IssueCode } from './IssueCode'; +import { ErrorAlert } from './ErrorAlert'; + +interface DatasourceSecurityAccessExtra { + owners?: string[]; + link?: string; + datasource?: number | string; + datasource_name?: string; + tables?: string[]; + issue_codes?: { + code: number; + message: string; + }[]; +} + +/** + * Shown when a viewer opens a chart but lacks permission to its underlying data + * (DATASOURCE_SECURITY_ACCESS_ERROR / TABLE_SECURITY_ACCESS_ERROR). Surfaces a + * plain-language explanation, who to contact, and a "Request access" link when + * the deployment configures PERMISSION_INSTRUCTIONS_LINK. + */ +export function DatasourceSecurityAccessErrorMessage({ + error, + source, + closable, +}: ErrorMessageComponentProps<DatasourceSecurityAccessExtra | null>) { + const { extra, level, message } = error; + const isVisualization = ['dashboard', 'explore'].includes(source || ''); + + // DATASOURCE_SECURITY_ACCESS_ERROR is also raised for non-access failures + // (e.g. virtual-dataset SQL validation: "Only SELECT statements are + // allowed"). Those errors carry no access payload — render them plainly + // rather than misleading the user with request-access guidance. + const isAccessDenial = !!extra?.datasource_name || !!extra?.tables?.length; + if (!isAccessDenial) { + return ( + <ErrorAlert + errorType={t('Unexpected error')} + message={message} + type={level} + closable={closable} + /> + ); + } + + let explanation: string; + if (extra?.datasource_name) { + explanation = isVisualization + ? t( + 'This chart uses the "%s" dataset, which you do not have ' + + 'permission to view.', + extra.datasource_name, + ) + : t( + 'This query uses the "%s" dataset, which you do not have ' + + 'permission to view.', + extra.datasource_name, + ); + } else { + explanation = isVisualization + ? t( + 'You do not have access to the data behind this chart ' + + '(tables: %s).', + extra?.tables?.join(', '), + ) + : t( + 'You do not have access to the following tables: %s.', // sqllab + extra?.tables?.join(', '), + ); + } + + const owners = extra?.owners; + const ownerLine = + isVisualization && owners && owners.length > 0 + ? tn( + 'To request access, reach out to the chart owner: %s.', + 'To request access, reach out to the chart owners: %s.', + owners.length, + owners.join(', '), + ) + : t('To request access, contact your Superset administrator.'); Review Comment: **Suggestion:** This component expects owner names from `extra.owners`, but the backend security error payloads for datasource/table access denials only provide datasource/table/link fields, so the “chart owners” branch is effectively dead in production and users always get the admin fallback. Populate owners from the actual chart context source used by the app (or add owners to the backend error payload) so the owner-contact guidance actually works. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx ⚠️ Chart access errors never show chart owner names. ⚠️ Viewers must escalate to admins instead of owners. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. As a Gamma viewer, open a chart whose underlying dataset you cannot access; the frontend issues a chart data request to `/api/v1/chart/data`, and the backend returns a 403 with `error_type=DATASOURCE_SECURITY_ACCESS_ERROR` as verified in `tests/integration_tests/charts/data/api_tests.py:712-10`. 2. The backend constructs this error via `SupersetSecurityManager.get_datasource_access_error_object()` in `superset/security/manager.py:1810-1827`, setting `extra` to include `"link"`, `"datasource"`, and `"datasource_name"` but not an `"owners"` field. 3. Frontend error configuration in `superset-frontend/src/setup/setupErrorMessages.ts:20-25` registers `DatasourceSecurityAccessErrorMessage` for both `ErrorTypeEnum.DATASOURCE_SECURITY_ACCESS_ERROR` and `ErrorTypeEnum.TABLE_SECURITY_ACCESS_ERROR`, so datasource/table permission failures use this component. 4. Inside `superset-frontend/src/components/ErrorMessage/DatasourceSecurityAccessErrorMessage.tsx:95-104`, the component reads `const owners = extra?.owners;` and shows chart-owner guidance only when `isVisualization && owners && owners.length > 0`. Repository search shows no production code populates `extra.owners` (only the unit test `DatasourceSecurityAccessErrorMessage.test.tsx:35-41` sets it), so `owners` is always undefined at runtime and the condition never passes; users therefore always see the fallback `"To request access, contact your Superset administrator."` instead of the intended chart-owner contact line. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a0949539b0374a79909fc532d1dbcd8e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a0949539b0374a79909fc532d1dbcd8e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/components/ErrorMessage/DatasourceSecurityAccessErrorMessage.tsx **Line:** 95:104 **Comment:** *Api Mismatch: This component expects owner names from `extra.owners`, but the backend security error payloads for datasource/table access denials only provide datasource/table/link fields, so the “chart owners” branch is effectively dead in production and users always get the admin fallback. Populate owners from the actual chart context source used by the app (or add owners to the backend error payload) so the owner-contact guidance actually works. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41843&comment_hash=9356c00ed765fa83b345845d3874db29b3192f2a553ce9098a672b965d230d97&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41843&comment_hash=9356c00ed765fa83b345845d3874db29b3192f2a553ce9098a672b965d230d97&reaction=dislike'>👎</a> -- 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]
