gabotorresruiz commented on code in PR #45060:
URL: https://github.com/apache/superset/pull/45060#discussion_r4223138340
##########
superset/common/query_context_factory.py:
##########
@@ -72,6 +75,24 @@ def create( # pylint: disable=too-many-arguments
result_type = result_type or ChartDataResultType.FULL
result_format = result_format or ChartDataResultFormat.JSON
+ if (
+ self._authorize_semantic_before_metadata
+ and datasource_model_instance is not None
+ and DatasourceType(datasource["type"]) ==
DatasourceType.SEMANTIC_VIEW
+ ):
+ # Guest dashboard and payload checks need the completed query
context;
+ # keep their authorization path and timing unchanged.
+ if not security_manager.is_guest_user():
+ QueryContext(
+ datasource=datasource_model_instance,
+ queries=[],
Review Comment:
This block worries me a bit. `raise_for_access` hands the whole context to
the `EXTRA_RAISE_FOR_ACCESS_BYPASS` hook at
`superset/security/manager.py:4972`, and that hook is operator code sitting
outside the guest guard, so here it receives a context whose `queries` is
empty. I tried it with a hook that grants based on `query_context.queries`: the
identical request returns `200` on master and `403` on this branch, because the
preflight raises before the hook ever sees the real payload. Nothing widens,
but a caller that used to be allowed is now denied.
Are we sure no deployment reads `queries` in that hook? If not, skipping the
preflight when it is configured is cheap insurance:
```python
and not current_app.config.get("EXTRA_RAISE_FOR_ACCESS_BYPASS")
```
Either way I would land your first follow-up here rather than after: inside
Superset's own code the invariant does hold, but it holds because of exactly
one `is_guest_user()` guard at `superset/security/manager.py:5334`, and a unit
test asserting that `raise_for_access` never reads `query_context.queries` for
a non-guest caller is what keeps it true.
##########
tests/integration_tests/charts/semantic_metadata_authz_tests.py:
##########
@@ -0,0 +1,283 @@
+# 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 unittest.mock import Mock, patch, PropertyMock
+
+import sqlalchemy as sa
+from flask import current_app, g, Response
+
+from superset.connectors.sqla.models import SqlaTable
+from superset.extensions import db, security_manager
+from superset.models.dashboard import Dashboard
+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
+
+
+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_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)
+
+ 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
+ provider.get_dimensions.assert_called()
+ finally:
+ 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:
+ """A denied ordinary dashboard filter must not load provider
metadata."""
+ self.login("gamma")
+ layer: SemanticLayer = SemanticLayer(name="viewer-metadata-layer",
type="test")
+ view: SemanticView = SemanticView(
+ name="viewer-metadata-view", semantic_layer=layer
+ )
+ db.session.add(view)
+ db.session.flush()
+ metadata: str = json.dumps(
+ {
+ "native_filter_configuration": [
+ {
+ "id": "filter-1",
+ "targets": [
+ {
+ "datasetId": view.id + 1,
+ "datasourceType": "semantic_view",
+ }
+ ],
+ }
+ ]
+ }
+ )
+ dashboard_id: int = db.session.execute(
+ sa.insert(Dashboard.__table__).values(
+ dashboard_title="viewer-metadata-dashboard",
+ published=True,
+ json_metadata=metadata,
+ )
+ ).inserted_primary_key[0]
+ db.session.commit()
+ provider: Mock = Mock()
+ provider.get_dimensions.return_value = set()
+ provider.get_metrics.return_value = set()
+ provider.features = frozenset()
+ provider.selection_identity_version = None
+ provider.uid.return_value = "viewer-metadata-view"
+ try:
+ with (
+ patch.dict(current_app.config, {"VIEWER_PROMISCUOUS_MODE":
True}),
+ patch.object(
+ SemanticView,
+ "implementation",
+ new_callable=PropertyMock,
+ return_value=provider,
+ ),
+ patch.object(security_manager, "is_viewer", return_value=True),
+ ):
+ response: Response = self.client.post(
+ "/api/v1/chart/data",
+ json={
+ "datasource": {"id": view.id, "type": "semantic_view"},
+ "queries": [{"columns": [], "metrics": []}],
+ "form_data": {
+ "dashboardId": dashboard_id,
+ "type": "NATIVE_FILTER",
+ "native_filter_id": "filter-1",
+ },
+ },
+ )
+ 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.execute(
+ sa.delete(Dashboard.__table__).where(Dashboard.id ==
dashboard_id)
+ )
+ db.session.delete(view)
+ db.session.delete(layer)
+ db.session.commit()
+
+ @with_feature_flags(EMBEDDED_SUPERSET=True)
+ def test_guest_dashboard_filter_access_is_unchanged(self) -> None:
+ """A guest can still use a dashboard-scoped semantic native filter."""
+ self.login("gamma")
+ layer: SemanticLayer = SemanticLayer(name="guest-metadata-layer",
type="test")
+ view: SemanticView = SemanticView(
+ name="guest-metadata-view", semantic_layer=layer
+ )
+ db.session.add(view)
+ db.session.flush()
+ metadata: str = json.dumps(
+ {
+ "native_filter_configuration": [
+ {
+ "id": "filter-1",
+ "targets": [
+ {
+ "datasetId": view.id,
+ "datasourceType": "semantic_view",
+ "column": {"name": "category"},
+ }
+ ],
+ }
+ ]
+ }
+ )
+ dashboard_id: int = db.session.execute(
+ sa.insert(Dashboard.__table__).values(
+ dashboard_title="guest-metadata-dashboard",
+ published=True,
+ json_metadata=metadata,
+ )
+ ).inserted_primary_key[0]
+ db.session.commit()
+ provider: Mock = Mock()
+ provider.get_dimensions.return_value = set()
+ provider.get_metrics.return_value = set()
+ provider.features = frozenset()
+ provider.selection_identity_version = None
+ provider.uid.return_value = "guest-metadata-view"
+ try:
+ with (
+ patch.object(
+ SemanticView,
+ "implementation",
+ new_callable=PropertyMock,
+ return_value=provider,
+ ),
+ patch.object(security_manager, "is_guest_user",
return_value=True),
+ patch.object(security_manager, "has_guest_access",
return_value=True),
+ ):
+ g.user.rls = []
+ response: Response = self.client.post(
+ "/api/v1/chart/data",
+ json={
+ "datasource": {"id": view.id, "type": "semantic_view"},
+ "queries": [{"columns": [], "metrics": []}],
+ "result_type": "query",
+ "form_data": {
+ "dashboardId": dashboard_id,
+ "type": "NATIVE_FILTER",
+ "native_filter_id": "filter-1",
+ },
+ },
+ )
+ assert response.status_code == 200, response.json
+ provider.get_dimensions.assert_called()
+ finally:
+ db.session.rollback()
+ db.session.execute(
+ sa.delete(Dashboard.__table__).where(Dashboard.id ==
dashboard_id)
+ )
+ db.session.delete(view)
+ db.session.delete(layer)
+ db.session.commit()
+
+ def test_sql_dataset_chart_data_is_unchanged(self) -> None:
Review Comment:
This one does not declare a birth-names fixture, so the command in the
testing instructions, `pytest
tests/integration_tests/charts/semantic_metadata_authz_tests.py -q`, fails for
me at line 274 with `assert None is not None`. It passes in a full run only
because an earlier test already loaded the data. With the fixture declared all
5 pass:
```suggestion
@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
def test_sql_dataset_chart_data_is_unchanged(self) -> None:
```
plus `from tests.integration_tests.fixtures.birth_names_dashboard import
load_birth_names_dashboard_with_slices, load_birth_names_data`.
##########
superset/common/query_context_factory.py:
##########
@@ -72,6 +75,24 @@ def create( # pylint: disable=too-many-arguments
result_type = result_type or ChartDataResultType.FULL
result_format = result_format or ChartDataResultFormat.JSON
+ if (
+ self._authorize_semantic_before_metadata
+ and datasource_model_instance is not None
+ and DatasourceType(datasource["type"]) ==
DatasourceType.SEMANTIC_VIEW
+ ):
+ # Guest dashboard and payload checks need the completed query
context;
+ # keep their authorization path and timing unchanged.
+ if not security_manager.is_guest_user():
+ QueryContext(
+ datasource=datasource_model_instance,
+ queries=[],
+ slice_=slice_,
+ form_data=form_data,
+ result_type=result_type,
+ result_format=result_format,
+ cache_values={},
+ ).raise_for_access()
Review Comment:
Small accuracy note for the description: a denied request can still load
metadata. With `ENABLE_VIEWERS` and `VIEWER_PROMISCUOUS_MODE` on,
`has_drill_access` reaches `SemanticView.has_drill_by_columns`, which calls
`implementation.get_dimensions()` (`superset/semantic_layers/models.py:767`)
inside this check. A denied drill-by measured one call here against two on the
parent, so it improves but does not reach zero.
##########
superset/common/query_context_factory.py:
##########
@@ -41,9 +42,11 @@ def create_query_object_factory() -> QueryObjectFactory:
class QueryContextFactory: # pylint: disable=too-few-public-methods
_query_object_factory: QueryObjectFactory
+ _authorize_semantic_before_metadata: bool
- def __init__(self) -> None:
+ def __init__(self, authorize_semantic_before_metadata: bool = False) ->
None:
Review Comment:
Not a blocker, and the UI does not use it, but `POST /api/v1/query/`
(`superset/views/api.py:71`) builds the context first and calls
`raise_for_access()` after, so with the default `False` it still loads
dimensions before the `403`. I saw one `get_dimensions()` call there on this
branch. Worth opting it in too, or deliberately out of scope?
--
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]
