codeant-ai-for-open-source[bot] commented on code in PR #41843: URL: https://github.com/apache/superset/pull/41843#discussion_r3533304537
########## superset-frontend/src/components/ErrorMessage/DatasourceSecurityAccessErrorMessage.tsx: ########## @@ -0,0 +1,128 @@ +/** + * 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 || ''); + + let explanation: string; + if (extra?.datasource_name) { + explanation = t( + 'This chart uses the "%s" dataset, which you do not have permission ' + + 'to view.', + extra.datasource_name, + ); + } else if (extra?.tables?.length) { + explanation = t( + 'You do not have access to the data behind this chart ' + '(tables: %s).', + extra.tables.join(', '), + ); Review Comment: **Suggestion:** The component hardcodes “chart” wording in both the title and explanation even when `source` is `sqllab`, so SQL Lab users receive incorrect guidance about charts instead of their query/table context. Branch the copy by source (or by error payload shape) so non-visualization contexts render accurate text. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ SQL Lab errors mis-described as chart data issues. - ⚠️ CRUD metadata errors use chart-centric wording. - ⚠️ Future non-chart consumers inherit misleading copy. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Inspect `DatasourceSecurityAccessErrorMessage` in `superset-frontend/src/components/ErrorMessage/DatasourceSecurityAccessErrorMessage.tsx:45-69`. The component receives a `source` prop and computes `isVisualization = ['dashboard', 'explore'].includes(source || '')` (line 51), but the `explanation` strings (lines 53-69) and the `errorType` passed to `ErrorAlert` (line 119) always talk about a “chart”. 2. Note the table-specific explanation branch at lines 60-64: `explanation = t('You do not have access to the data behind this chart ' + '(tables: %s).', extra.tables.join(', '));` which still hardcodes “chart” even though the error is about underlying tables/datasets, not necessarily a chart. 3. See how `ErrorMessageWithStackTrace` in `superset-frontend/src/components/ErrorMessage/ErrorMessageWithStackTrace.tsx:59-71` renders custom error components by looking up `error.errorType ?? error.error_type` in the registry and then calling the component with the `source` prop passed through unchanged. 4. Observe non-visualization call sites of `ErrorMessageWithStackTrace`, for example SQL Lab’s `ResultSet` (`superset-frontend/src/SqlLab/components/ResultSet/index.tsx:619-632`, which passes `source="sqllab"`) and `TableExploreTree` (`superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx:470-510`, which passes `source="crud"`). When these flows eventually render a `SupersetError` with `error_type` `DATASOURCE_SECURITY_ACCESS_ERROR` or `TABLE_SECURITY_ACCESS_ERROR`, the registry (configured in `setupErrorMessages.ts:95-105`) will pick `DatasourceSecurityAccessErrorMessage`, causing SQL Lab / CRUD screens to display “You don’t have access to this chart’s data” and “data behind this chart” even though no chart is involved. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d3e49a5c66034457b73b8e6f9543b6ea&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=d3e49a5c66034457b73b8e6f9543b6ea&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:** 61:64 **Comment:** *Logic Error: The component hardcodes “chart” wording in both the title and explanation even when `source` is `sqllab`, so SQL Lab users receive incorrect guidance about charts instead of their query/table context. Branch the copy by source (or by error payload shape) so non-visualization contexts render accurate text. 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=7a1901d7e842110d2227ae1701914cc74d41305075085485a170fd29f9ea7d8b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41843&comment_hash=7a1901d7e842110d2227ae1701914cc74d41305075085485a170fd29f9ea7d8b&reaction=dislike'>👎</a> ########## tests/unit_tests/security/test_permission_instructions_link.py: ########## @@ -0,0 +1,133 @@ +# 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. + +"""Unit tests for templated PERMISSION_INSTRUCTIONS_LINK rendering.""" + +from unittest.mock import MagicMock, patch + +from superset.security.manager import ( + _render_permission_instructions_link, + SupersetSecurityManager, +) + +MANAGER = "superset.security.manager" + + +def _render(template, *, username="alice", anonymous=False, **kwargs): + with ( + patch( + f"{MANAGER}.get_conf", + return_value={"PERMISSION_INSTRUCTIONS_LINK": template}, + ), + patch(f"{MANAGER}.g") as g_mock, + ): + g_mock.user.is_anonymous = anonymous + g_mock.user.username = username + return _render_permission_instructions_link(**kwargs) + + +def test_empty_or_unset_link_returns_none(): + assert _render("") is None + assert _render(None) is None + + +def test_plain_link_without_placeholders_is_unchanged(): + assert _render("https://wiki.example.com/data-access";) == ( + "https://wiki.example.com/data-access"; + ) + + +def test_datasource_placeholders_are_filled_and_url_encoded(): + out = _render( + "https://acme.example.com/req?id={datasource_id}"; + "&name={datasource_name}&u={username}", + datasource_id="12", + datasource_name="Quarterly Sales", + ) + # space in the dataset name is URL-encoded; username injected from g.user + assert out == ( + "https://acme.example.com/req?id=12&name=Quarterly%20Sales&u=alice"; + ) + + +def test_table_names_filled_and_encoded(): + out = _render( + "https://acme.example.com/req?tables={table_names}";, + table_names="public.sales,public.users", + ) + assert out == ( + "https://acme.example.com/req?tables=public.sales%2Cpublic.users"; + ) + + +def test_anonymous_user_renders_empty_username(): + out = _render( + "https://acme.example.com/req?u={username}";, + anonymous=True, + ) + assert out == "https://acme.example.com/req?u="; + + +def test_unreferenced_placeholders_left_untouched(): + # datasource link doesn't supply table_names; that token stays literal + out = _render( + "https://acme.example.com/req?id={datasource_id}&t={table_names}";, + datasource_id="9", + ) + assert out == "https://acme.example.com/req?id=9&t="; Review Comment: **Suggestion:** The test name/comment says unreferenced placeholders are “left untouched,” but the assertion checks they are replaced with an empty value, which contradicts the documented behavior in this test and makes the test intent misleading. Rename/update the test description (or the assertion) so behavior and documentation are consistent. [comment mismatch] <details> <summary><b>Severity Level:</b> Minor 🧹</summary> ```mdx - ⚠️ Test name misstates placeholder-substitution semantics. - ⚠️ Comment contradicts implemented empty-string behavior. - ⚠️ May confuse future maintainers reading test intent. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Review `_render_permission_instructions_link` in `superset/security/manager.py:28-61`. Its docstring states that “Unsupplied placeholders are replaced with an empty string” (line 40), and the implementation loops over tokens and replaces any `{token}` found in the configured URL with `quote(str(value), safe="")` (lines 52-60). For the default argument values, `datasource_id`, `datasource_name`, and `table_names` default to `""`. 2. In `tests/unit_tests/security/test_permission_instructions_link.py:85-91`, examine `test_unreferenced_placeholders_left_untouched`. The test name says “left untouched” and the inline comment claims “that token stays literal”, but the `_render` helper is invoked with a template containing `{table_names}` and `datasource_id="9"`, and **no** `table_names` keyword. 3. Given the function signature, `table_names` will be the default empty string, so `_render_permission_instructions_link` will see `{table_names}` in the URL and replace it with `quote(str(""))`, resulting in `https://acme.example.com/req?id=9&t=` — i.e. the placeholder is replaced, not left literal. 4. The assertion in the test, `assert out == "https://acme.example.com/req?id=9&t="` (line 91), therefore matches the implementation and the docstring, but contradicts both the test name `test_unreferenced_placeholders_left_untouched` and the comment “that token stays literal”, making the test’s documentation misleading about the actual contract (empty-string substitution). ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fcc813ac66374bd6b9a19bb56339e582&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=fcc813ac66374bd6b9a19bb56339e582&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:** tests/unit_tests/security/test_permission_instructions_link.py **Line:** 85:91 **Comment:** *Comment Mismatch: The test name/comment says unreferenced placeholders are “left untouched,” but the assertion checks they are replaced with an empty value, which contradicts the documented behavior in this test and makes the test intent misleading. Rename/update the test description (or the assertion) so behavior and documentation are consistent. 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=d77187250c0b0f59aa2349a2db60d2526499bd73beb489d45c5cace78bb276eb&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41843&comment_hash=d77187250c0b0f59aa2349a2db60d2526499bd73beb489d45c5cace78bb276eb&reaction=dislike'>👎</a> ########## superset-frontend/src/setup/setupErrorMessages.ts: ########## @@ -94,6 +95,14 @@ export default function setupErrorMessages() { ErrorTypeEnum.QUERY_SECURITY_ACCESS_ERROR, DatabaseErrorMessage, ); + errorMessageComponentRegistry.registerValue( + ErrorTypeEnum.DATASOURCE_SECURITY_ACCESS_ERROR, + DatasourceSecurityAccessErrorMessage, + ); Review Comment: **Suggestion:** Registering the custom datasource-access component for `DATASOURCE_SECURITY_ACCESS_ERROR` globally changes unrelated flows that reuse this error type (for example SQL validation errors like “Only SELECT statements are allowed”) into a misleading “no access to chart data” message. Keep those non-permission cases on `DatabaseErrorMessage` (or use a distinct backend error type for true permission denials) so callers that rely on the original message contract are not broken. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ SQL validation errors misrepresented as permission denials. - ⚠️ Dataset-edit failures show misleading access-denied messaging. - ⚠️ Breaks existing expectations for DATASOURCE error handling. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. On the backend, note that `SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR` is used for both permission and non-permission conditions. Besides dataset access denials in `SupersetSecurityManager.get_datasource_access_error_object` (`superset/security/manager.py:1633-17`), it is also used in `get_virtual_table_metadata` in `superset/connectors/sqla/utils.py:99-25`, where `parsed_script.has_mutation()` or `len(parsed_script.statements) > 1` raises `SupersetSecurityException(SupersetError(error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR, message=_("Only `SELECT` statements are allowed") / _("Only single queries supported"), level=ErrorLevel.ERROR))`. 2. These `SupersetError` instances are serialized into API responses and surfaced on the frontend as `SupersetError[]` entries; see the error-type mapping in `superset-frontend/packages/superset-ui-core/src/query/api/v1/types.ts:66-69` and the usage in `superset-frontend/src/hooks/apiResources/queryApi.ts:64-67`, which forwards `errorObj.errors` to UI consumers “used by <ErrorMessageWithStackTrace />`. 3. Inspect `ErrorMessageWithStackTrace` in `superset-frontend/src/components/ErrorMessage/ErrorMessageWithStackTrace.tsx:59-71`. It looks up `error.errorType ?? error.error_type` in the registry and, if a component is registered, renders it with the `error` object, regardless of whether the error was a permission denial or a SQL validation error. 4. In this PR, `setupErrorMessages` (`superset-frontend/src/setup/setupErrorMessages.ts:95-105`) now registers `ErrorTypeEnum.DATASOURCE_SECURITY_ACCESS_ERROR` and `ErrorTypeEnum.TABLE_SECURITY_ACCESS_ERROR` to the new `DatasourceSecurityAccessErrorMessage` instead of the generic `DatabaseErrorMessage`. As a result, any non-permission failure that reuses the same enum (e.g., the “Only `SELECT` statements are allowed” and “Only single queries supported” errors from `get_virtual_table_metadata`) will be rendered with a “You don’t have access to this chart’s data” access-denied UI and owner/contact guidance, rather than the more accurate database/SQL-focused messaging previously provided by `DatabaseErrorMessage`. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9ab756096eec48fd8d0ece40f4ce6b09&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=9ab756096eec48fd8d0ece40f4ce6b09&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/setup/setupErrorMessages.ts **Line:** 98:101 **Comment:** *Api Mismatch: Registering the custom datasource-access component for `DATASOURCE_SECURITY_ACCESS_ERROR` globally changes unrelated flows that reuse this error type (for example SQL validation errors like “Only SELECT statements are allowed”) into a misleading “no access to chart data” message. Keep those non-permission cases on `DatabaseErrorMessage` (or use a distinct backend error type for true permission denials) so callers that rely on the original message contract are not broken. 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=18992481a380d86fd21ffb68854a15f9e29ad4cd71416c94754bffbd519be24e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41843&comment_hash=18992481a380d86fd21ffb68854a15f9e29ad4cd71416c94754bffbd519be24e&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]
