codeant-ai-for-open-source[bot] commented on code in PR #38361: URL: https://github.com/apache/superset/pull/38361#discussion_r2885470436
########## tests/unit_tests/security/test_granular_export_permissions.py: ########## @@ -0,0 +1,110 @@ +# 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. + +from unittest.mock import MagicMock, patch + +from superset.security.manager import SupersetSecurityManager + + +def test_granular_export_permissions_registered_in_create_custom_permissions( + app_context: None, +) -> None: + """Verify that create_custom_permissions registers all granular export perms.""" + from superset.extensions import appbuilder + + sm = SupersetSecurityManager(appbuilder) + sm.add_permission_view_menu = MagicMock() + + sm.create_custom_permissions() + + calls = [ + (call.args[0], call.args[1]) + for call in sm.add_permission_view_menu.call_args_list + ] + assert ("can_export_data", "Superset") in calls + assert ("can_export_image", "Superset") in calls + assert ("can_copy_clipboard", "Superset") in calls + + +def test_sqllab_extra_permission_views_include_export_perms() -> None: + """Verify SQLLAB_EXTRA_PERMISSION_VIEWS includes granular export perms.""" + assert ("can_export_data", "Superset") in ( + SupersetSecurityManager.SQLLAB_EXTRA_PERMISSION_VIEWS + ) + assert ("can_copy_clipboard", "Superset") in ( + SupersetSecurityManager.SQLLAB_EXTRA_PERMISSION_VIEWS + ) + + +def test_gamma_excluded_pvms_excludes_export_data_and_image() -> None: + """Verify GAMMA_EXCLUDED_PVMS excludes can_export_data and can_export_image.""" + assert ("can_export_data", "Superset") in ( + SupersetSecurityManager.GAMMA_EXCLUDED_PVMS + ) + assert ("can_export_image", "Superset") in ( + SupersetSecurityManager.GAMMA_EXCLUDED_PVMS + ) + + +def test_gamma_excluded_pvms_allows_copy_clipboard() -> None: + """Verify GAMMA_EXCLUDED_PVMS does NOT exclude can_copy_clipboard.""" + assert ("can_copy_clipboard", "Superset") not in ( + SupersetSecurityManager.GAMMA_EXCLUDED_PVMS + ) + + +def test_is_gamma_pvm_excludes_export_data(app_context: None) -> None: + """Verify _is_gamma_pvm returns False for can_export_data.""" + from superset.extensions import appbuilder + + sm = SupersetSecurityManager(appbuilder) + pvm = MagicMock() + pvm.permission.name = "can_export_data" + pvm.view_menu.name = "Superset" + + assert sm._is_gamma_pvm(pvm) is False + + +def test_is_gamma_pvm_excludes_export_image(app_context: None) -> None: + """Verify _is_gamma_pvm returns False for can_export_image.""" + from superset.extensions import appbuilder + + sm = SupersetSecurityManager(appbuilder) + pvm = MagicMock() + pvm.permission.name = "can_export_image" + pvm.view_menu.name = "Superset" + + assert sm._is_gamma_pvm(pvm) is False + + +def test_is_gamma_pvm_allows_copy_clipboard(app_context: None) -> None: + """Verify _is_gamma_pvm returns True for can_copy_clipboard.""" + from superset.extensions import appbuilder + + sm = SupersetSecurityManager(appbuilder) + pvm = MagicMock() + pvm.permission.name = "can_copy_clipboard" + pvm.view_menu.name = "Superset" + # Ensure the pvm doesn't trigger other exclusion checks + with ( + patch.object(sm, "_is_user_defined_permission", return_value=False), + patch.object(sm, "_is_admin_only", return_value=False), + patch.object(sm, "_is_alpha_only", return_value=False), + patch.object(sm, "_is_sql_lab_only", return_value=False), + patch.object(sm, "_is_accessible_to_all", return_value=False), + ): + assert sm._is_gamma_pvm(pvm) is True Review Comment: **Suggestion:** The `with` statement is incorrectly written as `with (...)` which creates a single tuple of context managers instead of multiple context managers; since tuples don't implement the context manager protocol, this will raise an `AttributeError: __enter__` when the test runs and prevent the assertion from executing. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ test_is_gamma_pvm_allows_copy_clipboard crashes with AttributeError. - ❌ Gamma PVM behavior for can_copy_clipboard remains untested. - ⚠️ Security regression tests for granular exports may be unreliable. ``` </details> ```suggestion with patch.object(sm, "_is_user_defined_permission", return_value=False), \ patch.object(sm, "_is_admin_only", return_value=False), \ patch.object(sm, "_is_alpha_only", return_value=False), \ patch.object(sm, "_is_sql_lab_only", return_value=False), \ patch.object(sm, "_is_accessible_to_all", return_value=False): assert sm._is_gamma_pvm(pvm) is True ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the specific unit test `test_is_gamma_pvm_allows_copy_clipboard` via `pytest tests/unit_tests/security/test_granular_export_permissions.py::test_is_gamma_pvm_allows_copy_clipboard`. This test is defined in `tests/unit_tests/security/test_granular_export_permissions.py` lines 25–40 (file lines 94–109 from the tool output). 2. During test execution, Python reaches the `with (` block at `tests/unit_tests/security/test_granular_export_permissions.py:103-110`, where multiple `patch.object(...)` calls are written inside parentheses as `with (patch.object(...), patch.object(...), ...)`. 3. According to the Python `with` statement grammar, `with (cm1, cm2):` is parsed as a single context manager whose expression is the tuple `(cm1, cm2)`, not as multiple context managers; at runtime, Python evaluates the tuple of `_patch` objects and then attempts to call `__enter__` on the tuple itself. 4. Because `tuple` does not implement the context manager protocol, entering the `with` block raises `AttributeError: __enter__` before the body executes, so the assertion `assert sm._is_gamma_pvm(pvm) is True` never runs and the test fails, leaving `_is_gamma_pvm` in `superset/security/manager.py:1476-1482` unverified for the `can_copy_clipboard` permission. ``` </details> <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_granular_export_permissions.py **Line:** 103:110 **Comment:** *Logic Error: The `with` statement is incorrectly written as `with (...)` which creates a single tuple of context managers instead of multiple context managers; since tuples don't implement the context manager protocol, this will raise an `AttributeError: __enter__` when the test runs and prevent the assertion from executing. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38361&comment_hash=df678beb580db5f24983deec851c46ba13c4041b481f1bc0c62ee1a5f8085c94&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38361&comment_hash=df678beb580db5f24983deec851c46ba13c4041b481f1bc0c62ee1a5f8085c94&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]
