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