diff --git a/superset/dashboards/dao.py b/superset/dashboards/dao.py index 9f3d5178d7bb..c6577f1b0eb9 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 @@ -40,11 +40,22 @@ class DashboardDAO(BaseDAO): base_filter = DashboardAccessFilter @staticmethod - def get_by_id_or_slug(id_or_slug: str) -> Dashboard: - dashboard = Dashboard.get(id_or_slug) + 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)) + .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 @@ -67,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/api_tests.py b/tests/integration_tests/dashboards/api_tests.py index 10ca16f4e713..7288889bf115 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() diff --git a/tests/integration_tests/dashboards/dao_tests.py b/tests/integration_tests/dashboards/dao_tests.py index e9d73764955f..672e930364f2 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() diff --git a/tests/integration_tests/dashboards/filter_state/api_tests.py b/tests/integration_tests/dashboards/filter_state/api_tests.py index 1df752b230d4..15b479686a4e 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}", @@ -268,7 +251,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): @@ -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,16 +282,13 @@ 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): 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 diff --git a/tests/integration_tests/dashboards/permalink/api_tests.py b/tests/integration_tests/dashboards/permalink/api_tests.py index 018e06cd49ac..40a312ef855a 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):