From 32039295f84301882ff150ce3c04945170ca17ff Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Mon, 6 Oct 2025 01:39:31 -0300 Subject: [PATCH 1/3] fix: Support metric macro for embedded users --- superset/jinja_context.py | 8 ++- tests/unit_tests/jinja_context_test.py | 73 ++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/superset/jinja_context.py b/superset/jinja_context.py index d43744fac255..f359e1521e76 100644 --- a/superset/jinja_context.py +++ b/superset/jinja_context.py @@ -1086,7 +1086,13 @@ def metric_macro( if not dataset_id: dataset_id = get_dataset_id_from_context(metric_key) - dataset = DatasetDAO.find_by_id(dataset_id) + # Embedded user access is validated at the dashboard level, so we bypass + # the regular DAO filter for them + dataset = ( + DatasetDAO.find_by_id(dataset_id) + if not security_manager.is_guest_user() + else DatasetDAO.find_by_id(dataset_id, skip_base_filter=True) + ) if not dataset: raise DatasetNotFoundError(f"Dataset ID {dataset_id} not found.") diff --git a/tests/unit_tests/jinja_context_test.py b/tests/unit_tests/jinja_context_test.py index 30ada6968174..42d4cfad965c 100644 --- a/tests/unit_tests/jinja_context_test.py +++ b/tests/unit_tests/jinja_context_test.py @@ -1566,3 +1566,76 @@ def test_jinja2_server_error_handling(mocker: MockerFixture) -> None: assert "Internal Jinja2 template error" in str(exception) assert "MemoryError" in str(exception) assert "Out of memory" in str(exception) + + +def test_metric_macro_regular_user_uses_base_filter(mocker: MockerFixture) -> None: + """ + Test that the ``metric_macro`` uses base filter for regular users. + + Regular users should have standard RBAC/RLS filters applied when accessing datasets. + """ + mock_is_guest_user = mocker.patch("superset.security_manager.is_guest_user") + mock_is_guest_user.return_value = False + + DatasetDAO = mocker.patch("superset.daos.dataset.DatasetDAO") # noqa: N806 + DatasetDAO.find_by_id.return_value = SqlaTable( + table_name="test_dataset", + metrics=[ + SqlMetric(metric_name="count", expression="COUNT(*)"), + ], + database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"), + schema="my_schema", + sql=None, + ) + + env = SandboxedEnvironment(undefined=DebugUndefined) + assert metric_macro(env, {}, "count", 1) == "COUNT(*)" + + # Verify that find_by_id was called without skip_base_filter + DatasetDAO.find_by_id.assert_called_once_with(1) + + +def test_metric_macro_regular_user_raises_no_access(mocker: MockerFixture) -> None: + """ + Test that the ``metric_macro`` raises for regular user without dataset access. + """ + mock_is_guest_user = mocker.patch("superset.security_manager.is_guest_user") + mock_is_guest_user.return_value = False + + DatasetDAO = mocker.patch("superset.daos.dataset.DatasetDAO") # noqa: N806 + DatasetDAO.find_by_id.return_value = None + + env = SandboxedEnvironment(undefined=DebugUndefined) + with pytest.raises(DatasetNotFoundError) as excinfo: + assert metric_macro(env, {}, "count", 1) == "COUNT(*)" + + assert str(excinfo.value) == "Dataset ID 1 not found." + DatasetDAO.find_by_id.assert_called_once_with(1) + + +def test_metric_macro_embedded_user_skips_base_filter(mocker: MockerFixture) -> None: + """ + Test that the ``metric_macro`` skips base filter for embedded users. + + Embedded users have dashboard-level access control via their embedding token, + so we bypass the regular dataset DAO filters for them. + """ + mock_is_guest_user = mocker.patch("superset.security_manager.is_guest_user") + mock_is_guest_user.return_value = True + + DatasetDAO = mocker.patch("superset.daos.dataset.DatasetDAO") # noqa: N806 + DatasetDAO.find_by_id.return_value = SqlaTable( + table_name="test_dataset", + metrics=[ + SqlMetric(metric_name="count", expression="COUNT(*)"), + ], + database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"), + schema="my_schema", + sql=None, + ) + + env = SandboxedEnvironment(undefined=DebugUndefined) + assert metric_macro(env, {}, "count", 1) == "COUNT(*)" + + # Verify that find_by_id was called with skip_base_filter=True + DatasetDAO.find_by_id.assert_called_once_with(1, skip_base_filter=True) From 4762c45974d5f5bd54fafdafd590a0b94a45f3c2 Mon Sep 17 00:00:00 2001 From: Vitor Avila <96086495+Vitor-Avila@users.noreply.github.com> Date: Mon, 6 Oct 2025 12:40:19 -0300 Subject: [PATCH 2/3] Update superset/jinja_context.py Co-authored-by: Beto Dealmeida --- superset/jinja_context.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/superset/jinja_context.py b/superset/jinja_context.py index f359e1521e76..1634af83929b 100644 --- a/superset/jinja_context.py +++ b/superset/jinja_context.py @@ -1088,10 +1088,9 @@ def metric_macro( # Embedded user access is validated at the dashboard level, so we bypass # the regular DAO filter for them - dataset = ( - DatasetDAO.find_by_id(dataset_id) - if not security_manager.is_guest_user() - else DatasetDAO.find_by_id(dataset_id, skip_base_filter=True) + dataset = DatasetDAO.find_by_id( + dataset_id, + skip_base_filter=security_manager.is_guest_user(), ) if not dataset: raise DatasetNotFoundError(f"Dataset ID {dataset_id} not found.") From dc3ffeaae8d57456e72399eab58abcf8dad8171b Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Mon, 6 Oct 2025 12:52:58 -0300 Subject: [PATCH 3/3] Fixing tests after PR feedback --- tests/unit_tests/jinja_context_test.py | 146 ++++++++++++------------- 1 file changed, 73 insertions(+), 73 deletions(-) diff --git a/tests/unit_tests/jinja_context_test.py b/tests/unit_tests/jinja_context_test.py index 42d4cfad965c..985dd909eead 100644 --- a/tests/unit_tests/jinja_context_test.py +++ b/tests/unit_tests/jinja_context_test.py @@ -1252,6 +1252,79 @@ def test_metric_macro_no_dataset_id_available_in_request_form_data( assert metric_macro(env, {}, "macro_key") == "COUNT(*)" +def test_metric_macro_regular_user_uses_base_filter(mocker: MockerFixture) -> None: + """ + Test that the ``metric_macro`` uses base filter for regular users. + + Regular users should have standard RBAC/RLS filters applied when accessing datasets. + """ + mock_is_guest_user = mocker.patch("superset.security_manager.is_guest_user") + mock_is_guest_user.return_value = False + + DatasetDAO = mocker.patch("superset.daos.dataset.DatasetDAO") # noqa: N806 + DatasetDAO.find_by_id.return_value = SqlaTable( + table_name="test_dataset", + metrics=[ + SqlMetric(metric_name="count", expression="COUNT(*)"), + ], + database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"), + schema="my_schema", + sql=None, + ) + + env = SandboxedEnvironment(undefined=DebugUndefined) + assert metric_macro(env, {}, "count", 1) == "COUNT(*)" + + # Verify that find_by_id was called without skip_base_filter + DatasetDAO.find_by_id.assert_called_once_with(1, skip_base_filter=False) + + +def test_metric_macro_regular_user_raises_no_access(mocker: MockerFixture) -> None: + """ + Test that the ``metric_macro`` raises for regular user without dataset access. + """ + mock_is_guest_user = mocker.patch("superset.security_manager.is_guest_user") + mock_is_guest_user.return_value = False + + DatasetDAO = mocker.patch("superset.daos.dataset.DatasetDAO") # noqa: N806 + DatasetDAO.find_by_id.return_value = None + + env = SandboxedEnvironment(undefined=DebugUndefined) + with pytest.raises(DatasetNotFoundError) as excinfo: + assert metric_macro(env, {}, "count", 1) == "COUNT(*)" + + assert str(excinfo.value) == "Dataset ID 1 not found." + DatasetDAO.find_by_id.assert_called_once_with(1, skip_base_filter=False) + + +def test_metric_macro_embedded_user_skips_base_filter(mocker: MockerFixture) -> None: + """ + Test that the ``metric_macro`` skips base filter for embedded users. + + Embedded users have dashboard-level access control via their embedding token, + so we bypass the regular dataset DAO filters for them. + """ + mock_is_guest_user = mocker.patch("superset.security_manager.is_guest_user") + mock_is_guest_user.return_value = True + + DatasetDAO = mocker.patch("superset.daos.dataset.DatasetDAO") # noqa: N806 + DatasetDAO.find_by_id.return_value = SqlaTable( + table_name="test_dataset", + metrics=[ + SqlMetric(metric_name="count", expression="COUNT(*)"), + ], + database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"), + schema="my_schema", + sql=None, + ) + + env = SandboxedEnvironment(undefined=DebugUndefined) + assert metric_macro(env, {}, "count", 1) == "COUNT(*)" + + # Verify that find_by_id was called with skip_base_filter=True + DatasetDAO.find_by_id.assert_called_once_with(1, skip_base_filter=True) + + @pytest.mark.parametrize( "description,args,kwargs,sqlalchemy_uri,queries,time_filter,removed_filters,applied_filters", [ @@ -1566,76 +1639,3 @@ def test_jinja2_server_error_handling(mocker: MockerFixture) -> None: assert "Internal Jinja2 template error" in str(exception) assert "MemoryError" in str(exception) assert "Out of memory" in str(exception) - - -def test_metric_macro_regular_user_uses_base_filter(mocker: MockerFixture) -> None: - """ - Test that the ``metric_macro`` uses base filter for regular users. - - Regular users should have standard RBAC/RLS filters applied when accessing datasets. - """ - mock_is_guest_user = mocker.patch("superset.security_manager.is_guest_user") - mock_is_guest_user.return_value = False - - DatasetDAO = mocker.patch("superset.daos.dataset.DatasetDAO") # noqa: N806 - DatasetDAO.find_by_id.return_value = SqlaTable( - table_name="test_dataset", - metrics=[ - SqlMetric(metric_name="count", expression="COUNT(*)"), - ], - database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"), - schema="my_schema", - sql=None, - ) - - env = SandboxedEnvironment(undefined=DebugUndefined) - assert metric_macro(env, {}, "count", 1) == "COUNT(*)" - - # Verify that find_by_id was called without skip_base_filter - DatasetDAO.find_by_id.assert_called_once_with(1) - - -def test_metric_macro_regular_user_raises_no_access(mocker: MockerFixture) -> None: - """ - Test that the ``metric_macro`` raises for regular user without dataset access. - """ - mock_is_guest_user = mocker.patch("superset.security_manager.is_guest_user") - mock_is_guest_user.return_value = False - - DatasetDAO = mocker.patch("superset.daos.dataset.DatasetDAO") # noqa: N806 - DatasetDAO.find_by_id.return_value = None - - env = SandboxedEnvironment(undefined=DebugUndefined) - with pytest.raises(DatasetNotFoundError) as excinfo: - assert metric_macro(env, {}, "count", 1) == "COUNT(*)" - - assert str(excinfo.value) == "Dataset ID 1 not found." - DatasetDAO.find_by_id.assert_called_once_with(1) - - -def test_metric_macro_embedded_user_skips_base_filter(mocker: MockerFixture) -> None: - """ - Test that the ``metric_macro`` skips base filter for embedded users. - - Embedded users have dashboard-level access control via their embedding token, - so we bypass the regular dataset DAO filters for them. - """ - mock_is_guest_user = mocker.patch("superset.security_manager.is_guest_user") - mock_is_guest_user.return_value = True - - DatasetDAO = mocker.patch("superset.daos.dataset.DatasetDAO") # noqa: N806 - DatasetDAO.find_by_id.return_value = SqlaTable( - table_name="test_dataset", - metrics=[ - SqlMetric(metric_name="count", expression="COUNT(*)"), - ], - database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"), - schema="my_schema", - sql=None, - ) - - env = SandboxedEnvironment(undefined=DebugUndefined) - assert metric_macro(env, {}, "count", 1) == "COUNT(*)" - - # Verify that find_by_id was called with skip_base_filter=True - DatasetDAO.find_by_id.assert_called_once_with(1, skip_base_filter=True)