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

This reverts commit 3761694.

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

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

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

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,15 +246,29 @@ 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}",
         json={
             "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,24 +288,30 @@ 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):
     resp = test_client.delete(f"api/v1/dashboard/{dashboard_id}/filter_state/{KEY}")
     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

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