fix: dashboard get by id or slug access filter

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

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

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

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