From fb156231b7dffe586eb344aa3b99c460f217156c Mon Sep 17 00:00:00 2001 From: AnushaErrabelli Date: Tue, 31 Jan 2023 13:21:02 -0500 Subject: [PATCH] Revert "fix: dashboard get by id or slug access filter (#22358)" This reverts commit 3761694d72ba77332d9af68ec67fb178a25b1292. --- superset/dashboards/dao.py | 23 ++----- .../integration_tests/dashboards/api_tests.py | 65 +++--------------- .../integration_tests/dashboards/dao_tests.py | 67 +++++++++---------- .../dashboards/filter_state/api_tests.py | 47 +++++++++---- .../dashboards/permalink/api_tests.py | 9 ++- 5 files changed, 86 insertions(+), 125 deletions(-) diff --git a/superset/dashboards/dao.py b/superset/dashboards/dao.py index c6577f1b0eb91..9f3d5178d7bb6 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, id_or_slug_filter +from superset.models.dashboard import Dashboard 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,22 +40,11 @@ class DashboardDAO(BaseDAO): base_filter = DashboardAccessFilter @staticmethod - 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() + def get_by_id_or_slug(id_or_slug: str) -> Dashboard: + dashboard = Dashboard.get(id_or_slug) if not dashboard: raise DashboardNotFoundError() + security_manager.raise_for_dashboard_access(dashboard) return dashboard @staticmethod @@ -78,7 +67,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 7288889bf1157..10ca16f4e7138 100644 --- a/tests/integration_tests/dashboards/api_tests.py +++ b/tests/integration_tests/dashboards/api_tests.py @@ -225,36 +225,15 @@ 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("create_dashboards") - def test_get_gamma_dashboard_datasets(self): + @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") + def test_get_draft_dashboard_datasets(self): """ - Check that a gamma user with data access can access dashboard/datasets + All users should have access to dashboards without roles """ - 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}/datasets" + uri = "api/v1/dashboard/world_health/datasets" response = self.get_assert_metric(uri, "get_datasets") - 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) + self.assertEqual(response.status_code, 200) @pytest.mark.usefixtures("create_dashboards") def get_dashboard_by_slug(self): @@ -340,45 +319,17 @@ 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_gamma_dashboard_charts(self): + def test_get_draft_dashboard_charts(self): """ - Check that a gamma user with data access can access dashboard/charts + All users should have access to draft dashboards without roles """ - 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): """ @@ -500,7 +451,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 == 404 + assert rv.status_code == 200 # 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 672e930364f2a..e9d73764955f0 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, security_manager +from superset import db from superset.dashboards.dao import DashboardDAO from superset.models.dashboard import Dashboard from tests.integration_tests.base_tests import SupersetTestCase @@ -88,42 +88,37 @@ def test_set_dash_metadata(self): DashboardDAO.set_dash_metadata(dash, original_data) @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") - @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() - ) + def test_get_dashboard_changed_on(self): + self.login(username="admin") + session = db.session() + dashboard = 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) - 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 - ) + 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 + ) - DashboardDAO.set_dash_metadata(dashboard, original_data) - db.session.merge(dashboard) - db.session.commit() + DashboardDAO.set_dash_metadata(dashboard, original_data) + session.merge(dashboard) + 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 15b479686a4ec..1df752b230d40 100644 --- a/tests/integration_tests/dashboards/filter_state/api_tests.py +++ b/tests/integration_tests/dashboards/filter_state/api_tests.py @@ -90,15 +90,18 @@ def test_post_bad_request_non_json_string( assert resp.status_code == 400 -def test_post_access_denied(test_client, login_as, dashboard_id: int): - login_as("gamma") +@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() payload = { "value": INITIAL_VALUE, } resp = test_client.post( f"api/v1/dashboard/{dashboard_id}/filter_state", json=payload ) - assert resp.status_code == 404 + assert resp.status_code == 403 def test_post_same_key_for_same_tab_id(test_client, login_as_admin, dashboard_id: int): @@ -243,7 +246,21 @@ def test_put_bad_request_non_json_string( assert resp.status_code == 400 -def test_put_access_denied(test_client, login_as, dashboard_id: int): +@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): login_as("gamma") resp = test_client.put( f"api/v1/dashboard/{dashboard_id}/filter_state/{KEY}", @@ -251,7 +268,7 @@ def test_put_access_denied(test_client, login_as, dashboard_id: int): "value": UPDATED_VALUE, }, ) - assert resp.status_code == 404 + assert resp.status_code == 403 def test_get_key_not_found(test_client, login_as_admin, dashboard_id: int): @@ -271,10 +288,13 @@ def test_get_dashboard_filter_state(test_client, login_as_admin, dashboard_id: i assert INITIAL_VALUE == data.get("value") -def test_get_access_denied(test_client, login_as, dashboard_id): - login_as("gamma") +@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() resp = test_client.get(f"api/v1/dashboard/{dashboard_id}/filter_state/{KEY}") - assert resp.status_code == 404 + assert resp.status_code == 403 def test_delete(test_client, login_as_admin, dashboard_id: int): @@ -282,13 +302,16 @@ def test_delete(test_client, login_as_admin, dashboard_id: int): assert resp.status_code == 200 -def test_delete_access_denied(test_client, login_as, dashboard_id: int): - login_as("gamma") +@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() resp = test_client.delete(f"api/v1/dashboard/{dashboard_id}/filter_state/{KEY}") - assert resp.status_code == 404 + assert resp.status_code == 403 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 == 404 + assert resp.status_code == 403 diff --git a/tests/integration_tests/dashboards/permalink/api_tests.py b/tests/integration_tests/dashboards/permalink/api_tests.py index 40a312ef855a1..018e06cd49ac8 100644 --- a/tests/integration_tests/dashboards/permalink/api_tests.py +++ b/tests/integration_tests/dashboards/permalink/api_tests.py @@ -87,10 +87,13 @@ def test_post( db.session.commit() -def test_post_access_denied(test_client, login_as, dashboard_id: int): - login_as("gamma") +@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() resp = test_client.post(f"api/v1/dashboard/{dashboard_id}/permalink", json=STATE) - assert resp.status_code == 404 + assert resp.status_code == 403 def test_post_invalid_schema(test_client, login_as_admin, dashboard_id: int):