codeant-ai-for-open-source[bot] commented on code in PR #41843: URL: https://github.com/apache/superset/pull/41843#discussion_r3565525724
########## tests/unit_tests/security/test_permission_instructions_link.py: ########## @@ -0,0 +1,140 @@ +# 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, +) +from superset.sql.parse import Table + +MANAGER = "superset.security.manager" Review Comment: β **Customized review instruction saved!** **Instruction:** > Do not flag module-level constants or local variables with inferable types solely for missing type annotations; this codebase intentionally leaves those unannotated when mypy passes. **Applied to:** - `**/*.py` --- π‘ *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## superset/security/manager.py: ########## @@ -123,6 +124,42 @@ def get_conf() -> Any: return current_app.config +def _render_permission_instructions_link( + *, + datasource_id: str = "", + datasource_name: str = "", + table_names: str = "", +) -> Optional[str]: + """Render the configured ``PERMISSION_INSTRUCTIONS_LINK``. + + The configured URL may contain ``{datasource_id}``, ``{datasource_name}``, + ``{table_names}`` and ``{username}`` placeholders, which are substituted with + URL-encoded values so the link can deep-link into an organization's access + request system. A URL with no placeholders is returned unchanged, and an + empty/unset config returns ``None`` (no link). Unsupplied placeholders are + replaced with an empty string. + """ + link = get_conf().get("PERMISSION_INSTRUCTIONS_LINK") Review Comment: β **Customized review instruction saved!** **Instruction:** > Do not flag unannotated module-level constants or local variables in Python when their types are clearly inferable and mypy passes; this project does not require explicit annotations in those cases. **Applied to:** - `**/*.py` --- π‘ *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## superset/security/manager.py: ########## @@ -123,6 +124,42 @@ def get_conf() -> Any: return current_app.config +def _render_permission_instructions_link( + *, + datasource_id: str = "", + datasource_name: str = "", + table_names: str = "", +) -> Optional[str]: + """Render the configured ``PERMISSION_INSTRUCTIONS_LINK``. + + The configured URL may contain ``{datasource_id}``, ``{datasource_name}``, + ``{table_names}`` and ``{username}`` placeholders, which are substituted with + URL-encoded values so the link can deep-link into an organization's access + request system. A URL with no placeholders is returned unchanged, and an + empty/unset config returns ``None`` (no link). Unsupplied placeholders are + replaced with an empty string. + """ + link = get_conf().get("PERMISSION_INSTRUCTIONS_LINK") + if not link: + return None + + username = "" Review Comment: β **Customized review instruction saved!** **Instruction:** > Do not flag missing type annotations for Python locals or module-level constants when the type is inferable and mypy already passes. **Applied to:** - `**/*.py` --- π‘ *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## 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: β **Customized review instruction saved!** **Instruction:** > In DatasourceSecurityAccessErrorMessage, keep access-denial messaging gated on the security payload (`extra.datasource_name`/`extra.tables`); do not treat payload-less `DATASOURCE_SECURITY_ACCESS_ERROR` cases as access denials. **Applied to:** - `superset-frontend/src/components/ErrorMessage/**` --- π‘ *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## tests/unit_tests/security/test_permission_instructions_link.py: ########## @@ -0,0 +1,140 @@ +# 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, +) +from superset.sql.parse import Table + +MANAGER = "superset.security.manager" + + +def _render( + template: str | None, + *, + username: str = "alice", + anonymous: bool = False, + **kwargs: str, +) -> str | None: + 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() -> None: + assert _render("") is None + assert _render(None) is None + + +def test_plain_link_without_placeholders_is_unchanged() -> None: + assert _render("https://wiki.example.com/data-access";) == ( + "https://wiki.example.com/data-access"; + ) + + +def test_datasource_placeholders_are_filled_and_url_encoded() -> None: + 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() -> None: + 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() -> None: + out = _render( + "https://acme.example.com/req?u={username}";, + anonymous=True, + ) + assert out == "https://acme.example.com/req?u="; + + +def test_unsupplied_placeholders_render_empty() -> None: + # datasource link doesn't supply table_names; that token renders as + # an empty value (matching the helper's documented behavior) + 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="; + + +def test_get_datasource_access_link_pulls_from_datasource_data() -> None: + ds = MagicMock() Review Comment: β **Customized review instruction saved!** **Instruction:** > Do not require type annotations for locals or module-level constants when the type is already inferable; mypy coverage is sufficient. **Applied to:** - `**/*.py` --- π‘ *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## tests/unit_tests/security/test_permission_instructions_link.py: ########## @@ -0,0 +1,140 @@ +# 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, +) +from superset.sql.parse import Table + +MANAGER = "superset.security.manager" + + +def _render( + template: str | None, + *, + username: str = "alice", + anonymous: bool = False, + **kwargs: str, +) -> str | None: + 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() -> None: + assert _render("") is None + assert _render(None) is None + + +def test_plain_link_without_placeholders_is_unchanged() -> None: + assert _render("https://wiki.example.com/data-access";) == ( + "https://wiki.example.com/data-access"; + ) + + +def test_datasource_placeholders_are_filled_and_url_encoded() -> None: + 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() -> None: + 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() -> None: + out = _render( + "https://acme.example.com/req?u={username}";, + anonymous=True, + ) + assert out == "https://acme.example.com/req?u="; + + +def test_unsupplied_placeholders_render_empty() -> None: + # datasource link doesn't supply table_names; that token renders as + # an empty value (matching the helper's documented behavior) + 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="; + + +def test_get_datasource_access_link_pulls_from_datasource_data() -> None: + ds = MagicMock() + ds.data = {"id": 12, "name": "Quarterly Sales"} + with ( + patch( + f"{MANAGER}.get_conf", + return_value={ + "PERMISSION_INSTRUCTIONS_LINK": ( + "https://acme.example.com/req?id={datasource_id}"; + "&name={datasource_name}" + ) + }, + ), + patch(f"{MANAGER}.g") as g_mock, + ): + g_mock.user.is_anonymous = False + g_mock.user.username = "alice" + out = SupersetSecurityManager.get_datasource_access_link(ds) + assert out == "https://acme.example.com/req?id=12&name=Quarterly%20Sales"; + + +def test_get_table_access_link_joins_table_names() -> None: + sm = SupersetSecurityManager.__new__(SupersetSecurityManager) Review Comment: β **Customized review instruction saved!** **Instruction:** > Do not flag module-level constants or local variables with inferable types for missing explicit type annotations when the codebaseβs type checker already passes. **Applied to:** - `**/*.py` --- π‘ *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* -- 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]
