msyavuz commented on code in PR #40707: URL: https://github.com/apache/superset/pull/40707#discussion_r3420449267
########## tests/unit_tests/security/test_permission_hooks.py: ########## @@ -0,0 +1,202 @@ +# 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. +"""Tests for config-based permission extension hooks. + +Follows the same pattern as EXTRA_DYNAMIC_QUERY_FILTERS tests in +tests/unit_tests/databases/api_test.py. +""" + +from unittest.mock import Mock + +import pytest + +from superset import db + + [email protected] +def sample_chart(app_context: None): + """Create a minimal chart for testing.""" + from superset.models.slice import Slice + + chart = Slice( + slice_name="test_permission_hooks_chart", + datasource_type="table", + viz_type="table", + params="{}", + ) + db.session.add(chart) + db.session.flush() + yield chart + db.session.delete(chart) + db.session.flush() + + [email protected] +def sample_user(app_context: None): + """Create a minimal user for testing.""" + from flask_appbuilder.security.sqla.models import User + + user = User( + first_name="test", + last_name="hooks", + username="test_permission_hooks_user", + email="[email protected]", + ) + db.session.add(user) + db.session.flush() + yield user + db.session.delete(user) + db.session.flush() + + +def test_extra_owners_resolver_injects_into_extra_owners(sample_chart, monkeypatch): + """EXTRA_OWNERS_RESOLVER populates Slice.data['extra_owners'], not 'owners'.""" + from flask import current_app + + original_owner_ids = [o.id for o in sample_chart.owners] + + # Without config — extra_owners is empty, owners unchanged + monkeypatch.setitem(current_app.config, "EXTRA_OWNERS_RESOLVER", None) + data = sample_chart.data + assert data["owners"] == original_owner_ids + assert data["extra_owners"] == [] + + # With config — extra_owners populated, owners unchanged + def _resolver(resource): + return [{"id": 99999, "first_name": "Folder", "last_name": "Editor"}] + + resolver_mock = Mock(side_effect=_resolver) + monkeypatch.setitem(current_app.config, "EXTRA_OWNERS_RESOLVER", resolver_mock) + + data = sample_chart.data + assert data["owners"] == original_owner_ids + assert 99999 in data["extra_owners"] + assert resolver_mock.call_count == 1 + + +def test_extra_owners_resolver_empty_returns_unchanged(sample_chart, monkeypatch): + """EXTRA_OWNERS_RESOLVER returning empty list leaves extra_owners empty.""" + from flask import current_app + + resolver_mock = Mock(return_value=[]) + monkeypatch.setitem(current_app.config, "EXTRA_OWNERS_RESOLVER", resolver_mock) + + data = sample_chart.data + original_owner_ids = [o.id for o in sample_chart.owners] + assert data["owners"] == original_owner_ids + assert data["extra_owners"] == [] + assert resolver_mock.call_count == 1 + + +def test_raise_for_access_bypass_skips_checks(app_context: None, monkeypatch): + """EXTRA_RAISE_FOR_ACCESS_BYPASS returning True skips all permission checks.""" + from flask import current_app + + from superset import security_manager + + bypass_mock = Mock(return_value=True) + monkeypatch.setitem( + current_app.config, "EXTRA_RAISE_FOR_ACCESS_BYPASS", bypass_mock + ) + + security_manager.raise_for_access(dashboard=None, chart=None) + assert bypass_mock.call_count == 1 + + +def test_raise_for_access_no_bypass_without_config(app_context: None, monkeypatch): + """Without EXTRA_RAISE_FOR_ACCESS_BYPASS, normal checks proceed.""" + from flask import current_app + + from superset import security_manager + + monkeypatch.setitem(current_app.config, "EXTRA_RAISE_FOR_ACCESS_BYPASS", None) + security_manager.raise_for_access(dashboard=None, chart=None) + + +def test_ownership_check_allows_non_owner(sample_chart, sample_user, monkeypatch): + """EXTRA_OWNERSHIP_CHECKS returning True allows a non-owner to pass.""" + from flask import current_app, g + + from superset import security_manager + + check_mock = Mock(return_value=True) + monkeypatch.setitem(current_app.config, "EXTRA_OWNERSHIP_CHECKS", check_mock) Review Comment: This is not checked by raise_for_ownership ########## tests/unit_tests/security/test_permission_hooks.py: ########## @@ -0,0 +1,202 @@ +# 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. +"""Tests for config-based permission extension hooks. + +Follows the same pattern as EXTRA_DYNAMIC_QUERY_FILTERS tests in +tests/unit_tests/databases/api_test.py. +""" + +from unittest.mock import Mock + +import pytest + +from superset import db + + [email protected] +def sample_chart(app_context: None): + """Create a minimal chart for testing.""" + from superset.models.slice import Slice + + chart = Slice( + slice_name="test_permission_hooks_chart", + datasource_type="table", + viz_type="table", + params="{}", + ) + db.session.add(chart) + db.session.flush() + yield chart + db.session.delete(chart) + db.session.flush() + + [email protected] +def sample_user(app_context: None): + """Create a minimal user for testing.""" + from flask_appbuilder.security.sqla.models import User + + user = User( + first_name="test", + last_name="hooks", + username="test_permission_hooks_user", + email="[email protected]", + ) + db.session.add(user) + db.session.flush() + yield user + db.session.delete(user) + db.session.flush() + + +def test_extra_owners_resolver_injects_into_extra_owners(sample_chart, monkeypatch): + """EXTRA_OWNERS_RESOLVER populates Slice.data['extra_owners'], not 'owners'.""" + from flask import current_app + + original_owner_ids = [o.id for o in sample_chart.owners] + + # Without config — extra_owners is empty, owners unchanged + monkeypatch.setitem(current_app.config, "EXTRA_OWNERS_RESOLVER", None) + data = sample_chart.data + assert data["owners"] == original_owner_ids + assert data["extra_owners"] == [] + + # With config — extra_owners populated, owners unchanged + def _resolver(resource): + return [{"id": 99999, "first_name": "Folder", "last_name": "Editor"}] + + resolver_mock = Mock(side_effect=_resolver) + monkeypatch.setitem(current_app.config, "EXTRA_OWNERS_RESOLVER", resolver_mock) + + data = sample_chart.data + assert data["owners"] == original_owner_ids + assert 99999 in data["extra_owners"] + assert resolver_mock.call_count == 1 + + +def test_extra_owners_resolver_empty_returns_unchanged(sample_chart, monkeypatch): + """EXTRA_OWNERS_RESOLVER returning empty list leaves extra_owners empty.""" + from flask import current_app + + resolver_mock = Mock(return_value=[]) + monkeypatch.setitem(current_app.config, "EXTRA_OWNERS_RESOLVER", resolver_mock) + + data = sample_chart.data + original_owner_ids = [o.id for o in sample_chart.owners] + assert data["owners"] == original_owner_ids + assert data["extra_owners"] == [] + assert resolver_mock.call_count == 1 + + +def test_raise_for_access_bypass_skips_checks(app_context: None, monkeypatch): + """EXTRA_RAISE_FOR_ACCESS_BYPASS returning True skips all permission checks.""" + from flask import current_app + + from superset import security_manager + + bypass_mock = Mock(return_value=True) + monkeypatch.setitem( + current_app.config, "EXTRA_RAISE_FOR_ACCESS_BYPASS", bypass_mock + ) + + security_manager.raise_for_access(dashboard=None, chart=None) + assert bypass_mock.call_count == 1 + + +def test_raise_for_access_no_bypass_without_config(app_context: None, monkeypatch): + """Without EXTRA_RAISE_FOR_ACCESS_BYPASS, normal checks proceed.""" + from flask import current_app + + from superset import security_manager + + monkeypatch.setitem(current_app.config, "EXTRA_RAISE_FOR_ACCESS_BYPASS", None) + security_manager.raise_for_access(dashboard=None, chart=None) + + +def test_ownership_check_allows_non_owner(sample_chart, sample_user, monkeypatch): + """EXTRA_OWNERSHIP_CHECKS returning True allows a non-owner to pass.""" + from flask import current_app, g + + from superset import security_manager + + check_mock = Mock(return_value=True) + monkeypatch.setitem(current_app.config, "EXTRA_OWNERSHIP_CHECKS", check_mock) + monkeypatch.setattr(g, "user", sample_user, raising=False) + + security_manager.raise_for_ownership(sample_chart) + check_mock.assert_called_once() + + +def test_owner_auto_add_skip_prevents_auto_add( + sample_user, app_context: None, monkeypatch +): + """EXTRA_OWNER_AUTO_ADD_SKIP returning True prevents auto-adding current user.""" + from flask import current_app, g + from flask_appbuilder.security.sqla.models import User + + from superset.commands.utils import populate_owner_list + + other_user = User( + first_name="other", + last_name="skip", + username="test_skip_other", + email="[email protected]", + ) + db.session.add(other_user) + db.session.flush() + + skip_mock = Mock(return_value=True) + monkeypatch.setitem(current_app.config, "EXTRA_OWNER_AUTO_ADD_SKIP", skip_mock) Review Comment: Same here -- 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]
