fix: dashboard get by id or slug access filter (#22358)

superset/dashboards/dao.py

@@ -19,15 +19,15 @@ import logging
 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 @@ class DashboardDAO(BaseDAO):
         :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

tests/integration_tests/dashboards/api_tests.py

@@ -225,15 +225,36 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixi
         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 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixi
         response = self.get_assert_metric(uri, "get_charts")
         self.assertEqual(response.status_code, 404)
 
-    @pytest.mark.usefixtures("create_dashboards")
-    def test_get_draft_dashboard_charts(self):
-        """
-        All users should have access to draft dashboards without roles
-        """
+    @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):
+        """
+        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 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixi
         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()

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 @@ class TestDashboardDAO(SupersetTestCase):
         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()

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

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):