aminghadersohi commented on code in PR #41753: URL: https://github.com/apache/superset/pull/41753#discussion_r3546804486
########## tests/unit_tests/charts/test_filters.py: ########## @@ -0,0 +1,119 @@ +# 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 the ``ChartFilter`` embedded-guest scoping branch.""" + +from typing import Any +from unittest.mock import MagicMock + +import pytest +from pytest_mock import MockerFixture +from sqlalchemy import create_engine + + +def test_chart_filter_scopes_guest_to_token_dashboards(mocker: MockerFixture) -> None: + """A guest's chart list is scoped to the dashboards in its token.""" + from superset.charts.filters import ChartFilter + from superset.extensions import security_manager + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice + + mocker.patch.object( + security_manager, "can_access_all_datasources", return_value=False + ) + mocker.patch( + "superset.charts.filters.guest_embedded_dashboard_filter", + return_value=Dashboard.id.in_([1, 2]), + ) + + captured: dict[str, Any] = {} + query: MagicMock = MagicMock() + + def _capture_filter(clause: object) -> MagicMock: + captured["clause"] = clause + return query + + query.filter.side_effect = _capture_filter + + filt: ChartFilter = ChartFilter.__new__(ChartFilter) + filt.model = Slice + result = filt.apply(query, None) + + assert result is query + query.filter.assert_called_once() + # Guest branch returns early: it must NOT take the datasource-access join path. + query.join.assert_not_called() + + compiled: str = str( + captured["clause"].compile( + create_engine("sqlite://"), compile_kwargs={"literal_binds": True} + ) + ) + assert "EXISTS" in compiled # scoped via the m2m dashboards relationship + assert "IN (1, 2)" in compiled + + +def test_chart_filter_non_guest_skips_guest_branch(mocker: MockerFixture) -> None: + """With no embedded guest, ChartFilter does not take the guest branch (it + proceeds to the datasource-access join instead of filtering on dashboards).""" + from superset.charts.filters import ChartFilter + from superset.extensions import security_manager + from superset.models.slice import Slice + + mocker.patch.object( + security_manager, "can_access_all_datasources", return_value=False + ) + guest_filter: MagicMock = mocker.patch( + "superset.charts.filters.guest_embedded_dashboard_filter", return_value=None + ) + # Stop apply right after the guest check so we don't exercise the (heavily + # env-dependent) datasource-access path — reaching the join proves the guest + # branch was skipped. + sentinel: RuntimeError = RuntimeError("reached datasource-access join") + query: MagicMock = MagicMock() + query.join.side_effect = sentinel + + filt: ChartFilter = ChartFilter.__new__(ChartFilter) + filt.model = Slice + + with pytest.raises(RuntimeError, match="datasource-access join"): Review Comment: CI is currently failing here: `AttributeError: user` (flask/ctx.py:54), not the expected `RuntimeError` sentinel. `ChartFilter.apply()`'s non-guest tail was refactored upstream to `return self._apply_viewers(query)`, which calls `get_user_id()` (touching `g.user`) before anything resembling `query.join(...)`. Since this test never sets up `g.user`, the `AttributeError` fires first and the sentinel never gets a chance to prove the guest branch was skipped. Not a security issue (the guest branch above is untouched and still short-circuits first) but this needs a fix — e.g. `mocker.patch("superset.charts.filters.get_user_id", return_value=1)` — or it will keep failing CI on every future push. -- 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]
