From efb02da0e3172a62348c09a515606f3f89db3012 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 7 Dec 2022 14:56:18 +0000 Subject: [PATCH 1/8] fix: dashboard get by id or slug access filter --- superset/dashboards/dao.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/superset/dashboards/dao.py b/superset/dashboards/dao.py index 9f3d5178d7bb6..b2dcc42f107ab 100644 --- a/superset/dashboards/dao.py +++ b/superset/dashboards/dao.py @@ -19,15 +19,15 @@ from datetime import datetime from typing import Any, Dict, List, Optional, Union +from flask_appbuilder.models.sqla.interface import SQLAInterface from sqlalchemy.exc import SQLAlchemyError -from superset import security_manager from superset.dao.base import BaseDAO from superset.dashboards.commands.exceptions import DashboardNotFoundError from superset.dashboards.filters import DashboardAccessFilter from superset.extensions import db from superset.models.core import FavStar, FavStarClassName -from superset.models.dashboard import Dashboard +from superset.models.dashboard import Dashboard, id_or_slug_filter from superset.models.slice import Slice from superset.utils.core import get_user_id from superset.utils.dashboard_filter_scopes_converter import copy_filter_scopes @@ -41,10 +41,21 @@ class DashboardDAO(BaseDAO): @staticmethod def get_by_id_or_slug(id_or_slug: str) -> Dashboard: - dashboard = Dashboard.get(id_or_slug) + query = ( + db.session.query(Dashboard) + .filter(id_or_slug_filter(id_or_slug)) + .outerjoin(Slice, Dashboard.slices) + .outerjoin(Slice.table) + .outerjoin(Dashboard.owners) + .outerjoin(Dashboard.roles) + ) + # Apply dashboard base filters + query = DashboardAccessFilter("id", SQLAInterface(Dashboard, db.session)).apply( + query, None + ) + dashboard = query.one_or_none() if not dashboard: raise DashboardNotFoundError() - security_manager.raise_for_dashboard_access(dashboard) return dashboard @staticmethod From c80cc0da8b939f2fdbfd5a1c6fd0329ebb7f9e0b Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 9 Dec 2022 14:45:36 +0000 Subject: [PATCH 2/8] fix tests --- superset/dashboards/filters.py | 1 - .../integration_tests/dashboards/api_tests.py | 65 ++++++++++++++++--- 2 files changed, 57 insertions(+), 9 deletions(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index e09609ff511e0..afe45cd1f3cd8 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -182,7 +182,6 @@ def apply(self, query: Query, value: Any) -> Query: *feature_flagged_filters, ) ) - return query diff --git a/tests/integration_tests/dashboards/api_tests.py b/tests/integration_tests/dashboards/api_tests.py index 1ca80aae38dfe..97875d7dbd198 100644 --- a/tests/integration_tests/dashboards/api_tests.py +++ b/tests/integration_tests/dashboards/api_tests.py @@ -225,15 +225,36 @@ def test_get_dashboard_datasets_not_found(self): response = self.get_assert_metric(uri, "get_datasets") self.assertEqual(response.status_code, 404) - @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") - def test_get_draft_dashboard_datasets(self): + @pytest.mark.usefixtures("create_dashboards") + def test_get_gamma_dashboard_datasets(self): """ - All users should have access to dashboards without roles + Check that a gamma user with data access can access dashboard/datasets """ + from superset.connectors.sqla.models import SqlaTable + + # Set correct role permissions + gamma_role = security_manager.find_role("Gamma") + fixture_dataset = db.session.query(SqlaTable).get(1) + data_access_pvm = security_manager.add_permission_view_menu( + "datasource_access", fixture_dataset.perm + ) + gamma_role.permissions.append(data_access_pvm) + db.session.commit() + self.login(username="gamma") - uri = "api/v1/dashboard/world_health/datasets" + dashboard = self.dashboards[0] + dashboard.published = True + db.session.commit() + + uri = f"api/v1/dashboard/{dashboard.id}/datasets" response = self.get_assert_metric(uri, "get_datasets") - self.assertEqual(response.status_code, 200) + assert response.status_code == 200 + + # rollback permission change + data_access_pvm = security_manager.find_permission_view_menu( + "datasource_access", fixture_dataset.perm + ) + security_manager.del_permission_role(gamma_role, data_access_pvm) @pytest.mark.usefixtures("create_dashboards") def get_dashboard_by_slug(self): @@ -319,17 +340,45 @@ def test_get_dashboard_charts_not_found(self): response = self.get_assert_metric(uri, "get_charts") self.assertEqual(response.status_code, 404) + @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") + def test_get_dashboard_datasets_not_allowed(self): + self.login(username="gamma") + uri = "api/v1/dashboard/world_health/datasets" + response = self.get_assert_metric(uri, "get_datasets") + self.assertEqual(response.status_code, 404) + @pytest.mark.usefixtures("create_dashboards") - def test_get_draft_dashboard_charts(self): + def test_get_gamma_dashboard_charts(self): """ - All users should have access to draft dashboards without roles + Check that a gamma user with data access can access dashboard/charts """ + from superset.connectors.sqla.models import SqlaTable + + # Set correct role permissions + gamma_role = security_manager.find_role("Gamma") + fixture_dataset = db.session.query(SqlaTable).get(1) + data_access_pvm = security_manager.add_permission_view_menu( + "datasource_access", fixture_dataset.perm + ) + gamma_role.permissions.append(data_access_pvm) + db.session.commit() + self.login(username="gamma") + dashboard = self.dashboards[0] + dashboard.published = True + db.session.commit() + uri = f"api/v1/dashboard/{dashboard.id}/charts" response = self.get_assert_metric(uri, "get_charts") assert response.status_code == 200 + # rollback permission change + data_access_pvm = security_manager.find_permission_view_menu( + "datasource_access", fixture_dataset.perm + ) + security_manager.del_permission_role(gamma_role, data_access_pvm) + @pytest.mark.usefixtures("create_dashboards") def test_get_dashboard_charts_empty(self): """ @@ -451,7 +500,7 @@ def test_get_dashboard_no_data_access(self): self.login(username="gamma") uri = f"api/v1/dashboard/{dashboard.id}" rv = self.client.get(uri) - assert rv.status_code == 200 + assert rv.status_code == 404 # rollback changes db.session.delete(dashboard) db.session.commit() From c81586d744b498201476ed3e7a352cf9911417df Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 9 Dec 2022 17:18:19 +0000 Subject: [PATCH 3/8] fix tests --- superset/dashboards/dao.py | 4 +- .../integration_tests/dashboards/dao_tests.py | 67 ++++++++++--------- 2 files changed, 38 insertions(+), 33 deletions(-) diff --git a/superset/dashboards/dao.py b/superset/dashboards/dao.py index b2dcc42f107ab..c6577f1b0eb91 100644 --- a/superset/dashboards/dao.py +++ b/superset/dashboards/dao.py @@ -40,7 +40,7 @@ class DashboardDAO(BaseDAO): base_filter = DashboardAccessFilter @staticmethod - def get_by_id_or_slug(id_or_slug: str) -> Dashboard: + def get_by_id_or_slug(id_or_slug: Union[int, str]) -> Dashboard: query = ( db.session.query(Dashboard) .filter(id_or_slug_filter(id_or_slug)) @@ -78,7 +78,7 @@ def get_dashboard_changed_on( :returns: The datetime the dashboard was last changed. """ - dashboard = ( + dashboard: Dashboard = ( DashboardDAO.get_by_id_or_slug(id_or_slug_or_dashboard) if isinstance(id_or_slug_or_dashboard, str) else id_or_slug_or_dashboard diff --git a/tests/integration_tests/dashboards/dao_tests.py b/tests/integration_tests/dashboards/dao_tests.py index e9d73764955f0..672e930364f2a 100644 --- a/tests/integration_tests/dashboards/dao_tests.py +++ b/tests/integration_tests/dashboards/dao_tests.py @@ -18,11 +18,11 @@ import copy import json import time - +from unittest.mock import patch import pytest import tests.integration_tests.test_app # pylint: disable=unused-import -from superset import db +from superset import db, security_manager from superset.dashboards.dao import DashboardDAO from superset.models.dashboard import Dashboard from tests.integration_tests.base_tests import SupersetTestCase @@ -88,37 +88,42 @@ def test_set_dash_metadata(self): DashboardDAO.set_dash_metadata(dash, original_data) @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") - def test_get_dashboard_changed_on(self): - self.login(username="admin") - session = db.session() - dashboard = session.query(Dashboard).filter_by(slug="world_health").first() + @patch("superset.utils.core.g") + @patch("superset.security.manager.g") + def test_get_dashboard_changed_on(self, mock_sm_g, mock_g): + mock_g.user = mock_sm_g.user = security_manager.find_user("admin") + with self.client.application.test_request_context(): + self.login(username="admin") + dashboard = ( + db.session.query(Dashboard).filter_by(slug="world_health").first() + ) - changed_on = dashboard.changed_on.replace(microsecond=0) - assert changed_on == DashboardDAO.get_dashboard_changed_on(dashboard) - assert changed_on == DashboardDAO.get_dashboard_changed_on("world_health") + changed_on = dashboard.changed_on.replace(microsecond=0) + assert changed_on == DashboardDAO.get_dashboard_changed_on(dashboard) + assert changed_on == DashboardDAO.get_dashboard_changed_on("world_health") - old_changed_on = dashboard.changed_on + old_changed_on = dashboard.changed_on - # freezegun doesn't work for some reason, so we need to sleep here :( - time.sleep(1) - data = dashboard.data - positions = data["position_json"] - data.update({"positions": positions}) - original_data = copy.deepcopy(data) + # freezegun doesn't work for some reason, so we need to sleep here :( + time.sleep(1) + data = dashboard.data + positions = data["position_json"] + data.update({"positions": positions}) + original_data = copy.deepcopy(data) - data.update({"foo": "bar"}) - DashboardDAO.set_dash_metadata(dashboard, data) - session.merge(dashboard) - session.commit() - new_changed_on = DashboardDAO.get_dashboard_changed_on(dashboard) - assert old_changed_on.replace(microsecond=0) < new_changed_on - assert new_changed_on == DashboardDAO.get_dashboard_and_datasets_changed_on( - dashboard - ) - assert new_changed_on == DashboardDAO.get_dashboard_and_slices_changed_on( - dashboard - ) + data.update({"foo": "bar"}) + DashboardDAO.set_dash_metadata(dashboard, data) + db.session.merge(dashboard) + db.session.commit() + new_changed_on = DashboardDAO.get_dashboard_changed_on(dashboard) + assert old_changed_on.replace(microsecond=0) < new_changed_on + assert new_changed_on == DashboardDAO.get_dashboard_and_datasets_changed_on( + dashboard + ) + assert new_changed_on == DashboardDAO.get_dashboard_and_slices_changed_on( + dashboard + ) - DashboardDAO.set_dash_metadata(dashboard, original_data) - session.merge(dashboard) - session.commit() + DashboardDAO.set_dash_metadata(dashboard, original_data) + db.session.merge(dashboard) + db.session.commit() From d87266f926859bc70c182e631b1a3f773b6f9f34 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 13 Dec 2022 17:27:10 +0000 Subject: [PATCH 4/8] include dashboard RBAC sec check --- superset/dashboards/dao.py | 4 ++++ tests/integration_tests/dashboards/filter_state/api_tests.py | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/superset/dashboards/dao.py b/superset/dashboards/dao.py index c6577f1b0eb91..90b11a9ab219e 100644 --- a/superset/dashboards/dao.py +++ b/superset/dashboards/dao.py @@ -41,6 +41,8 @@ class DashboardDAO(BaseDAO): @staticmethod def get_by_id_or_slug(id_or_slug: Union[int, str]) -> Dashboard: + from superset import security_manager + query = ( db.session.query(Dashboard) .filter(id_or_slug_filter(id_or_slug)) @@ -56,6 +58,8 @@ def get_by_id_or_slug(id_or_slug: Union[int, str]) -> Dashboard: dashboard = query.one_or_none() if not dashboard: raise DashboardNotFoundError() + # Check dashboard RBAC + security_manager.raise_for_dashboard_access(dashboard) return dashboard @staticmethod diff --git a/tests/integration_tests/dashboards/filter_state/api_tests.py b/tests/integration_tests/dashboards/filter_state/api_tests.py index 1df752b230d40..65db841425f96 100644 --- a/tests/integration_tests/dashboards/filter_state/api_tests.py +++ b/tests/integration_tests/dashboards/filter_state/api_tests.py @@ -268,7 +268,7 @@ def test_put_not_owner(test_client, login_as, dashboard_id: int): "value": UPDATED_VALUE, }, ) - assert resp.status_code == 403 + assert resp.status_code == 404 def test_get_key_not_found(test_client, login_as_admin, dashboard_id: int): @@ -314,4 +314,4 @@ def test_delete_access_denied( def test_delete_not_owner(test_client, login_as, dashboard_id: int): login_as("gamma") resp = test_client.delete(f"api/v1/dashboard/{dashboard_id}/filter_state/{KEY}") - assert resp.status_code == 403 + assert resp.status_code == 404 From f623207ed5ed6760725ddcaed9bda423b3b43a54 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 14 Dec 2022 09:28:46 +0000 Subject: [PATCH 5/8] lint --- superset/dashboards/dao.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/superset/dashboards/dao.py b/superset/dashboards/dao.py index 90b11a9ab219e..0d58e159670be 100644 --- a/superset/dashboards/dao.py +++ b/superset/dashboards/dao.py @@ -22,6 +22,7 @@ from flask_appbuilder.models.sqla.interface import SQLAInterface from sqlalchemy.exc import SQLAlchemyError +from superset import security_manager from superset.dao.base import BaseDAO from superset.dashboards.commands.exceptions import DashboardNotFoundError from superset.dashboards.filters import DashboardAccessFilter @@ -41,8 +42,6 @@ class DashboardDAO(BaseDAO): @staticmethod def get_by_id_or_slug(id_or_slug: Union[int, str]) -> Dashboard: - from superset import security_manager - query = ( db.session.query(Dashboard) .filter(id_or_slug_filter(id_or_slug)) From ce9428fdbc20a531e9fbc886d35165b3e6df132e Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 14 Dec 2022 10:23:28 +0000 Subject: [PATCH 6/8] revert EOL removal --- superset/dashboards/filters.py | 1 + 1 file changed, 1 insertion(+) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index afe45cd1f3cd8..e09609ff511e0 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -182,6 +182,7 @@ def apply(self, query: Query, value: Any) -> Query: *feature_flagged_filters, ) ) + return query From 9bf63fcd75c0cc912da4a18af0f424f4ed36323d Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 14 Dec 2022 11:53:51 +0000 Subject: [PATCH 7/8] fix tests --- superset/dashboards/dao.py | 2 - .../dashboards/filter_state/api_tests.py | 43 +++++-------------- 2 files changed, 10 insertions(+), 35 deletions(-) diff --git a/superset/dashboards/dao.py b/superset/dashboards/dao.py index 0d58e159670be..0acea2af911da 100644 --- a/superset/dashboards/dao.py +++ b/superset/dashboards/dao.py @@ -57,8 +57,6 @@ def get_by_id_or_slug(id_or_slug: Union[int, str]) -> Dashboard: dashboard = query.one_or_none() if not dashboard: raise DashboardNotFoundError() - # Check dashboard RBAC - security_manager.raise_for_dashboard_access(dashboard) return dashboard @staticmethod diff --git a/tests/integration_tests/dashboards/filter_state/api_tests.py b/tests/integration_tests/dashboards/filter_state/api_tests.py index 65db841425f96..15b479686a4ec 100644 --- a/tests/integration_tests/dashboards/filter_state/api_tests.py +++ b/tests/integration_tests/dashboards/filter_state/api_tests.py @@ -90,18 +90,15 @@ def test_post_bad_request_non_json_string( assert resp.status_code == 400 -@patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") -def test_post_access_denied( - mock_raise_for_dashboard_access, test_client, login_as_admin, dashboard_id: int -): - mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() +def test_post_access_denied(test_client, login_as, dashboard_id: int): + login_as("gamma") payload = { "value": INITIAL_VALUE, } resp = test_client.post( f"api/v1/dashboard/{dashboard_id}/filter_state", json=payload ) - assert resp.status_code == 403 + assert resp.status_code == 404 def test_post_same_key_for_same_tab_id(test_client, login_as_admin, dashboard_id: int): @@ -246,21 +243,7 @@ def test_put_bad_request_non_json_string( assert resp.status_code == 400 -@patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") -def test_put_access_denied( - mock_raise_for_dashboard_access, test_client, login_as_admin, dashboard_id: int -): - mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() - resp = test_client.put( - f"api/v1/dashboard/{dashboard_id}/filter_state/{KEY}", - json={ - "value": UPDATED_VALUE, - }, - ) - assert resp.status_code == 403 - - -def test_put_not_owner(test_client, login_as, dashboard_id: int): +def test_put_access_denied(test_client, login_as, dashboard_id: int): login_as("gamma") resp = test_client.put( f"api/v1/dashboard/{dashboard_id}/filter_state/{KEY}", @@ -288,13 +271,10 @@ def test_get_dashboard_filter_state(test_client, login_as_admin, dashboard_id: i assert INITIAL_VALUE == data.get("value") -@patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") -def test_get_access_denied( - mock_raise_for_dashboard_access, test_client, login_as_admin, dashboard_id -): - mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() +def test_get_access_denied(test_client, login_as, dashboard_id): + login_as("gamma") resp = test_client.get(f"api/v1/dashboard/{dashboard_id}/filter_state/{KEY}") - assert resp.status_code == 403 + assert resp.status_code == 404 def test_delete(test_client, login_as_admin, dashboard_id: int): @@ -302,13 +282,10 @@ def test_delete(test_client, login_as_admin, dashboard_id: int): assert resp.status_code == 200 -@patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") -def test_delete_access_denied( - mock_raise_for_dashboard_access, test_client, login_as_admin, dashboard_id: int -): - mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() +def test_delete_access_denied(test_client, login_as, dashboard_id: int): + login_as("gamma") resp = test_client.delete(f"api/v1/dashboard/{dashboard_id}/filter_state/{KEY}") - assert resp.status_code == 403 + assert resp.status_code == 404 def test_delete_not_owner(test_client, login_as, dashboard_id: int): From b5b1e84fe058e50e8f1ee850b5d5800be649e14f Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 14 Dec 2022 13:07:32 +0000 Subject: [PATCH 8/8] fix tests --- superset/dashboards/dao.py | 1 - .../integration_tests/dashboards/permalink/api_tests.py | 9 +++------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/superset/dashboards/dao.py b/superset/dashboards/dao.py index 0acea2af911da..c6577f1b0eb91 100644 --- a/superset/dashboards/dao.py +++ b/superset/dashboards/dao.py @@ -22,7 +22,6 @@ from flask_appbuilder.models.sqla.interface import SQLAInterface from sqlalchemy.exc import SQLAlchemyError -from superset import security_manager from superset.dao.base import BaseDAO from superset.dashboards.commands.exceptions import DashboardNotFoundError from superset.dashboards.filters import DashboardAccessFilter diff --git a/tests/integration_tests/dashboards/permalink/api_tests.py b/tests/integration_tests/dashboards/permalink/api_tests.py index 018e06cd49ac8..40a312ef855a1 100644 --- a/tests/integration_tests/dashboards/permalink/api_tests.py +++ b/tests/integration_tests/dashboards/permalink/api_tests.py @@ -87,13 +87,10 @@ def test_post( db.session.commit() -@patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") -def test_post_access_denied( - mock_raise_for_dashboard_access, test_client, login_as_admin, dashboard_id: int -): - mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() +def test_post_access_denied(test_client, login_as, dashboard_id: int): + login_as("gamma") resp = test_client.post(f"api/v1/dashboard/{dashboard_id}/permalink", json=STATE) - assert resp.status_code == 403 + assert resp.status_code == 404 def test_post_invalid_schema(test_client, login_as_admin, dashboard_id: int):