sadpandajoe commented on code in PR #45060: URL: https://github.com/apache/superset/pull/45060#discussion_r4233787656
########## tests/integration_tests/charts/semantic_metadata_authz_tests.py: ########## @@ -0,0 +1,469 @@ +# 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. +"""Check whether denied chart-data requests call semantic provider metadata.""" + +from collections.abc import Callable +from typing import Any +from unittest.mock import Mock, patch, PropertyMock + +import pytest +import sqlalchemy as sa +from flask import current_app, g, Response + +from superset.connectors.sqla.models import SqlaTable +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType +from superset.exceptions import SupersetSecurityException +from superset.extensions import db, security_manager +from superset.models.dashboard import Dashboard +from superset.security.manager import SupersetSecurityManager +from superset.semantic_layers.models import SemanticLayer, SemanticView +from superset.utils import json +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.conftest import with_feature_flags +from tests.integration_tests.fixtures.birth_names_dashboard import ( + load_birth_names_dashboard_with_slices, # noqa: F401 + load_birth_names_data, # noqa: F401 +) + + +def _query_context_checks(spy: Mock) -> int: + """Count access decisions made on a query context (preflight or final).""" + return sum(1 for call in spy.call_args_list if "query_context" in call.kwargs) + + +class TestSemanticMetadataAuthorization(SupersetTestCase): + """Exercise the chart-data route with a provider that records metadata calls.""" + + def test_denied_chart_data_skips_provider_metadata(self) -> None: + """A denied request should not load dimensions or metrics.""" + self.login("gamma") + layer: SemanticLayer = SemanticLayer(name="authz-metadata-layer", type="test") + view: SemanticView = SemanticView( + name="authz-metadata-view", semantic_layer=layer + ) + db.session.add(view) + db.session.commit() + provider: Mock = Mock() + provider.get_dimensions.return_value = set() + provider.get_metrics.return_value = set() + try: + with patch.object( + SemanticView, + "implementation", + new_callable=PropertyMock, + return_value=provider, + ): + response: Response = self.client.post( + "/api/v1/chart/data", + json={ + "datasource": {"id": view.id, "type": "semantic_view"}, + "queries": [{"columns": [], "metrics": []}], + }, + ) + assert response.status_code == 403, response.json + provider.get_dimensions.assert_not_called() + provider.get_metrics.assert_not_called() + finally: + db.session.rollback() + db.session.delete(view) + db.session.delete(layer) + db.session.commit() + + def test_denied_legacy_query_api_skips_provider_metadata(self) -> None: + """The legacy ``/api/v1/query/`` route also denies before metadata.""" + self.login("gamma") + layer: SemanticLayer = SemanticLayer(name="legacy-metadata-layer", type="test") + view: SemanticView = SemanticView( + name="legacy-metadata-view", semantic_layer=layer + ) + db.session.add(view) + db.session.commit() + provider: Mock = Mock() + provider.get_dimensions.return_value = set() + provider.get_metrics.return_value = set() + try: + with patch.object( + SemanticView, + "implementation", + new_callable=PropertyMock, + return_value=provider, + ): + response: Response = self.client.post( + "/api/v1/query/", + data={ + "query_context": json.dumps( + { + "datasource": { + "id": view.id, + "type": "semantic_view", + }, + "queries": [{"columns": [], "metrics": []}], + } + ) + }, + ) + assert response.status_code == 403, response.json + provider.get_dimensions.assert_not_called() + provider.get_metrics.assert_not_called() + finally: + db.session.rollback() + db.session.delete(view) + db.session.delete(layer) + db.session.commit() + + def test_allowed_chart_data_uses_provider_metadata(self) -> None: + """An entitled role still builds and validates a semantic query.""" + self.login("gamma") + layer: SemanticLayer = SemanticLayer(name="allowed-metadata-layer", type="test") + view: SemanticView = SemanticView( + name="allowed-metadata-view", semantic_layer=layer + ) + db.session.add(view) + db.session.commit() + provider: Mock = Mock() + provider.get_dimensions.return_value = set() + provider.get_metrics.return_value = set() + original_can_access: Callable[[str, str], bool] = security_manager.can_access + + def can_access(permission_name: str, view_name: str) -> bool: + """Give Gamma this view's datasource grant for the request.""" + if permission_name == "datasource_access" and view_name == view.perm: + return True + return original_can_access(permission_name, view_name) + + access_spy: Mock + try: + with ( + patch.object( + SemanticView, + "implementation", + new_callable=PropertyMock, + return_value=provider, + ), + patch.object(security_manager, "can_access", side_effect=can_access), + patch.object( + security_manager, + "raise_for_access", + wraps=security_manager.raise_for_access, + ) as access_spy, + ): + response: Response = self.client.post( + "/api/v1/chart/data", + json={ + "datasource": {"id": view.id, "type": "semantic_view"}, + "queries": [{"columns": [], "metrics": []}], + "result_type": "query", + }, + ) + assert response.status_code == 200, response.json + provider.get_dimensions.assert_called() + # The preflight and the final check each decide on a query context. + assert _query_context_checks(access_spy) == 2 + finally: + db.session.rollback() + db.session.delete(view) + db.session.delete(layer) + db.session.commit() + + def test_payload_based_access_bypass_still_sees_queries(self) -> None: + """An operator bypass hook keeps receiving the request's real queries.""" + self.login("gamma") + layer: SemanticLayer = SemanticLayer(name="bypass-metadata-layer", type="test") + view: SemanticView = SemanticView( + name="bypass-metadata-view", semantic_layer=layer + ) + db.session.add(view) + db.session.commit() + provider: Mock = Mock() + provider.get_dimensions.return_value = set() + provider.get_metrics.return_value = set() + + def bypass(**kwargs: object) -> bool: + """Grant only when the request's queries are visible to the hook.""" + query_context: object = kwargs["query_context"] + return bool(getattr(query_context, "queries", None)) + + try: + with ( + patch.dict( + current_app.config, {"EXTRA_RAISE_FOR_ACCESS_BYPASS": bypass} + ), + patch.object( + SemanticView, + "implementation", + new_callable=PropertyMock, + return_value=provider, + ), + ): + response: Response = self.client.post( + "/api/v1/chart/data", + json={ + "datasource": {"id": view.id, "type": "semantic_view"}, + "queries": [{"columns": [], "metrics": []}], + "result_type": "query", + }, + ) + assert response.status_code == 200, response.json + finally: + db.session.rollback() + db.session.delete(view) + db.session.delete(layer) + db.session.commit() + + def test_overridden_access_check_still_sees_queries(self) -> None: + """A custom security manager's raise_for_access keeps receiving the + request's real queries, so the preflight does not run for it.""" + self.login("gamma") + layer: SemanticLayer = SemanticLayer( + name="override-metadata-layer", type="test" + ) + view: SemanticView = SemanticView( + name="override-metadata-view", semantic_layer=layer + ) + db.session.add(view) + db.session.commit() + provider: Mock = Mock() + provider.get_dimensions.return_value = set() + provider.get_metrics.return_value = set() + manager: SupersetSecurityManager = security_manager._get_current_object() # noqa: SLF001 + stock_class: type[SupersetSecurityManager] = type(manager) + original_can_access: Callable[[str, str], bool] = security_manager.can_access + + class QueryCheckingSecurityManager(stock_class): # type: ignore[valid-type, misc] + """Authorizes per requested query, like a column or metric check.""" + + def raise_for_access(self, **kwargs: Any) -> None: + query_context: Any = kwargs.get("query_context") + if query_context is not None and not query_context.queries: + raise SupersetSecurityException( + SupersetError( + message="No requested queries to authorize", + error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR, + level=ErrorLevel.ERROR, + ) + ) + super().raise_for_access(**kwargs) + + def can_access(permission_name: str, view_name: str) -> bool: + """Give Gamma this view's datasource grant for the request.""" + if permission_name == "datasource_access" and view_name == view.perm: + return True + return original_can_access(permission_name, view_name) + + # patch.object cannot restore __class__, so swap and restore it here. + manager.__class__ = QueryCheckingSecurityManager + try: + with ( + patch.object( + SemanticView, + "implementation", + new_callable=PropertyMock, + return_value=provider, + ), + patch.object(security_manager, "can_access", side_effect=can_access), + ): + response: Response = self.client.post( + "/api/v1/chart/data", + json={ + "datasource": {"id": view.id, "type": "semantic_view"}, + "queries": [{"columns": [], "metrics": []}], + "result_type": "query", + }, + ) + assert response.status_code == 200, response.json + finally: + manager.__class__ = stock_class + db.session.rollback() + db.session.delete(view) + db.session.delete(layer) + db.session.commit() + + @with_feature_flags(ENABLE_VIEWERS=True) + def test_denied_viewer_filter_skips_provider_metadata(self) -> None: Review Comment: The only viewer-mode test here is the denial case, so nothing pins that the preflight forwards the request's `form_data` to the access check. If it were dropped (`form_data=None` at `query_context_factory.py:110`), a non-guest user who reaches the semantic view only through a dashboard native filter (`dashboardId` + `native_filter_id`, resolved in `_native_filter_allowed_targets`) would get a 403 from the preflight, while this denial test, the directly-granted success test and the manager unit tests would all still pass. Could you add the allowed counterpart: Gamma with `ENABLE_VIEWERS`, a dashboard whose filter targets this view's id, asserting 200 and two query-context access checks? -- 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]
