You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by dp...@apache.org on 2024/02/06 12:14:11 UTC
(superset) branch master updated: fix: chart import validation (#26993)
This is an automated email from the ASF dual-hosted git repository.
dpgaspar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 5b34395689 fix: chart import validation (#26993)
5b34395689 is described below
commit 5b343956899371f0cb606d998a4b1a5d78919569
Author: Daniel Vaz Gaspar <da...@gmail.com>
AuthorDate: Tue Feb 6 12:14:02 2024 +0000
fix: chart import validation (#26993)
---
superset/commands/chart/importers/v1/utils.py | 13 +-
superset/commands/dashboard/importers/v1/utils.py | 8 +-
superset/commands/dataset/importers/v1/utils.py | 7 +-
superset/errors.py | 1 +
superset/jinja_context.py | 9 +-
superset/security/manager.py | 41 ++++++
superset/utils/core.py | 9 ++
tests/integration_tests/charts/commands_tests.py | 2 +-
.../integration_tests/dashboards/commands_tests.py | 2 +-
tests/integration_tests/datasets/commands_tests.py | 2 +-
.../charts/commands/importers/v1/import_test.py | 164 ++++++++++++++++-----
.../commands/importers/v1/import_test.py | 152 ++++++++-----------
tests/unit_tests/security/manager_test.py | 140 ++++++++++++++++++
13 files changed, 404 insertions(+), 146 deletions(-)
diff --git a/superset/commands/chart/importers/v1/utils.py b/superset/commands/chart/importers/v1/utils.py
index f905f8cc3a..2aac3ea9c4 100644
--- a/superset/commands/chart/importers/v1/utils.py
+++ b/superset/commands/chart/importers/v1/utils.py
@@ -20,7 +20,6 @@ import json
from inspect import isclass
from typing import Any
-from flask import g
from sqlalchemy.orm import Session
from superset import security_manager
@@ -28,7 +27,7 @@ from superset.commands.exceptions import ImportFailedError
from superset.migrations.shared.migrate_viz import processors
from superset.migrations.shared.migrate_viz.base import MigrateViz
from superset.models.slice import Slice
-from superset.utils.core import AnnotationType
+from superset.utils.core import AnnotationType, get_user
def filter_chart_annotations(chart_config: dict[str, Any]) -> None:
@@ -55,6 +54,12 @@ def import_chart(
can_write = ignore_permissions or security_manager.can_access("can_write", "Chart")
existing = session.query(Slice).filter_by(uuid=config["uuid"]).first()
if existing:
+ if overwrite and can_write and get_user():
+ if not security_manager.can_access_chart(existing):
+ raise ImportFailedError(
+ "A chart already exists and user doesn't "
+ "have permissions to overwrite it"
+ )
if not overwrite or not can_write:
return existing
config["id"] = existing.id
@@ -77,8 +82,8 @@ def import_chart(
if chart.id is None:
session.flush()
- if hasattr(g, "user") and g.user:
- chart.owners.append(g.user)
+ if user := get_user():
+ chart.owners.append(user)
return chart
diff --git a/superset/commands/dashboard/importers/v1/utils.py b/superset/commands/dashboard/importers/v1/utils.py
index 712161bc16..b8ac3144db 100644
--- a/superset/commands/dashboard/importers/v1/utils.py
+++ b/superset/commands/dashboard/importers/v1/utils.py
@@ -19,12 +19,12 @@ import json
import logging
from typing import Any
-from flask import g
from sqlalchemy.orm import Session
from superset import security_manager
from superset.commands.exceptions import ImportFailedError
from superset.models.dashboard import Dashboard
+from superset.utils.core import get_user
logger = logging.getLogger(__name__)
@@ -157,7 +157,7 @@ def import_dashboard(
)
existing = session.query(Dashboard).filter_by(uuid=config["uuid"]).first()
if existing:
- if overwrite and can_write and hasattr(g, "user") and g.user:
+ if overwrite and can_write and get_user():
if not security_manager.can_access_dashboard(existing):
raise ImportFailedError(
"A dashboard already exists and user doesn't "
@@ -191,7 +191,7 @@ def import_dashboard(
if dashboard.id is None:
session.flush()
- if hasattr(g, "user") and g.user:
- dashboard.owners.append(g.user)
+ if user := get_user():
+ dashboard.owners.append(user)
return dashboard
diff --git a/superset/commands/dataset/importers/v1/utils.py b/superset/commands/dataset/importers/v1/utils.py
index 5a9ac54714..014a864da4 100644
--- a/superset/commands/dataset/importers/v1/utils.py
+++ b/superset/commands/dataset/importers/v1/utils.py
@@ -22,7 +22,7 @@ from typing import Any
from urllib import request
import pandas as pd
-from flask import current_app, g
+from flask import current_app
from sqlalchemy import BigInteger, Boolean, Date, DateTime, Float, String, Text
from sqlalchemy.exc import MultipleResultsFound
from sqlalchemy.orm import Session
@@ -33,6 +33,7 @@ from superset.commands.dataset.exceptions import DatasetForbiddenDataURI
from superset.commands.exceptions import ImportFailedError
from superset.connectors.sqla.models import SqlaTable
from superset.models.core import Database
+from superset.utils.core import get_user
logger = logging.getLogger(__name__)
@@ -176,8 +177,8 @@ def import_dataset(
if data_uri and (not table_exists or force_data):
load_data(data_uri, dataset, dataset.database, session)
- if hasattr(g, "user") and g.user:
- dataset.owners.append(g.user)
+ if user := get_user():
+ dataset.owners.append(user)
return dataset
diff --git a/superset/errors.py b/superset/errors.py
index 87cdf77b99..6be4f966ce 100644
--- a/superset/errors.py
+++ b/superset/errors.py
@@ -66,6 +66,7 @@ class SupersetErrorType(StrEnum):
MISSING_OWNERSHIP_ERROR = "MISSING_OWNERSHIP_ERROR"
USER_ACTIVITY_SECURITY_ACCESS_ERROR = "USER_ACTIVITY_SECURITY_ACCESS_ERROR"
DASHBOARD_SECURITY_ACCESS_ERROR = "DASHBOARD_SECURITY_ACCESS_ERROR"
+ CHART_SECURITY_ACCESS_ERROR = "CHART_SECURITY_ACCESS_ERROR"
# Other errors
BACKEND_TIMEOUT_ERROR = "BACKEND_TIMEOUT_ERROR"
diff --git a/superset/jinja_context.py b/superset/jinja_context.py
index 3b046b732e..54d1f54866 100644
--- a/superset/jinja_context.py
+++ b/superset/jinja_context.py
@@ -36,7 +36,7 @@ from superset.exceptions import SupersetTemplateException
from superset.extensions import feature_flag_manager
from superset.utils.core import (
convert_legacy_filters_into_adhoc,
- get_user_id,
+ get_user,
merge_extra_filters,
)
@@ -110,11 +110,10 @@ class ExtraCache:
:returns: The user ID
"""
- if hasattr(g, "user") and g.user:
- id_ = get_user_id()
+ if user := get_user():
if add_to_cache_keys:
- self.cache_key_wrapper(id_)
- return id_
+ self.cache_key_wrapper(user.id)
+ return user.id
return None
def current_username(self, add_to_cache_keys: bool = True) -> Optional[str]:
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 8268b19411..72d3be27d3 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -85,6 +85,7 @@ if TYPE_CHECKING:
)
from superset.models.core import Database
from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
from superset.models.sql_lab import Query
from superset.sql_parse import Table
from superset.viz import BaseViz
@@ -422,6 +423,19 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
return True
+ def can_access_chart(self, chart: "Slice") -> bool:
+ """
+ Return True if the user can access the specified chart, False otherwise.
+ :param chart: The chart
+ :return: Whether the user can access the chart
+ """
+ try:
+ self.raise_for_access(chart=chart)
+ except SupersetSecurityException:
+ return False
+
+ return True
+
def get_dashboard_access_error_object( # pylint: disable=invalid-name
self,
dashboard: "Dashboard", # pylint: disable=unused-argument
@@ -439,6 +453,23 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
level=ErrorLevel.ERROR,
)
+ def get_chart_access_error_object( # pylint: disable=invalid-name
+ self,
+ dashboard: "Dashboard", # pylint: disable=unused-argument
+ ) -> SupersetError:
+ """
+ Return the error object for the denied Superset dashboard.
+
+ :param dashboard: The denied Superset dashboard
+ :returns: The error object
+ """
+
+ return SupersetError(
+ error_type=SupersetErrorType.CHART_SECURITY_ACCESS_ERROR,
+ message="You don't have access to this chart.",
+ level=ErrorLevel.ERROR,
+ )
+
@staticmethod
def get_datasource_access_error_msg(datasource: "BaseDatasource") -> str:
"""
@@ -1822,6 +1853,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
# pylint: disable=too-many-arguments,too-many-branches,too-many-locals,too-many-statements
self,
dashboard: Optional["Dashboard"] = None,
+ chart: Optional["Slice"] = None,
database: Optional["Database"] = None,
datasource: Optional["BaseDatasource"] = None,
query: Optional["Query"] = None,
@@ -2044,6 +2076,15 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
self.get_dashboard_access_error_object(dashboard)
)
+ if chart:
+ if self.is_admin() or self.is_owner(chart):
+ return
+
+ if chart.datasource and self.can_access_datasource(chart.datasource):
+ return
+
+ raise SupersetSecurityException(self.get_chart_access_error_object(chart))
+
def get_user_by_username(self, username: str) -> Optional[User]:
"""
Retrieves a user by it's username case sensitive. Optional session parameter
diff --git a/superset/utils/core.py b/superset/utils/core.py
index 3a761cad1f..df12aafe07 100644
--- a/superset/utils/core.py
+++ b/superset/utils/core.py
@@ -1344,6 +1344,15 @@ def split_adhoc_filters_into_base_filters( # pylint: disable=invalid-name
form_data["filters"] = simple_where_filters
+def get_user() -> User | None:
+ """
+ Get the current user (if defined).
+
+ :returns: The current user
+ """
+ return g.user if hasattr(g, "user") else None
+
+
def get_username() -> str | None:
"""
Get username (if defined) associated with the current user.
diff --git a/tests/integration_tests/charts/commands_tests.py b/tests/integration_tests/charts/commands_tests.py
index b6adf197f5..a72a716d17 100644
--- a/tests/integration_tests/charts/commands_tests.py
+++ b/tests/integration_tests/charts/commands_tests.py
@@ -171,7 +171,7 @@ class TestExportChartsCommand(SupersetTestCase):
class TestImportChartsCommand(SupersetTestCase):
- @patch("superset.commands.chart.importers.v1.utils.g")
+ @patch("superset.utils.core.g")
@patch("superset.security.manager.g")
def test_import_v1_chart(self, sm_g, utils_g):
"""Test that we can import a chart"""
diff --git a/tests/integration_tests/dashboards/commands_tests.py b/tests/integration_tests/dashboards/commands_tests.py
index 6e9beab249..c1b65ee74e 100644
--- a/tests/integration_tests/dashboards/commands_tests.py
+++ b/tests/integration_tests/dashboards/commands_tests.py
@@ -486,7 +486,7 @@ class TestImportDashboardsCommand(SupersetTestCase):
db.session.delete(dataset)
db.session.commit()
- @patch("superset.commands.dashboard.importers.v1.utils.g")
+ @patch("superset.utils.core.g")
@patch("superset.security.manager.g")
def test_import_v1_dashboard(self, sm_g, utils_g):
"""Test that we can import a dashboard"""
diff --git a/tests/integration_tests/datasets/commands_tests.py b/tests/integration_tests/datasets/commands_tests.py
index b45bbdb76d..7b6066a22a 100644
--- a/tests/integration_tests/datasets/commands_tests.py
+++ b/tests/integration_tests/datasets/commands_tests.py
@@ -339,7 +339,7 @@ class TestImportDatasetsCommand(SupersetTestCase):
db.session.delete(dataset)
db.session.commit()
- @patch("superset.commands.dataset.importers.v1.utils.g")
+ @patch("superset.utils.core.g")
@patch("superset.security.manager.g")
@pytest.mark.usefixtures("load_energy_table_with_slice")
def test_import_v1_dataset(self, sm_g, utils_g):
diff --git a/tests/unit_tests/charts/commands/importers/v1/import_test.py b/tests/unit_tests/charts/commands/importers/v1/import_test.py
index 903b7468ba..bcff3ee411 100644
--- a/tests/unit_tests/charts/commands/importers/v1/import_test.py
+++ b/tests/unit_tests/charts/commands/importers/v1/import_test.py
@@ -17,104 +17,130 @@
# pylint: disable=unused-argument, import-outside-toplevel, unused-import, invalid-name
import copy
+from collections.abc import Generator
import pytest
+from flask_appbuilder.security.sqla.models import Role, User
from pytest_mock import MockFixture
from sqlalchemy.orm.session import Session
+from superset import security_manager
+from superset.commands.chart.importers.v1.utils import import_chart
from superset.commands.exceptions import ImportFailedError
+from superset.connectors.sqla.models import Database, SqlaTable
+from superset.models.slice import Slice
+from superset.utils.core import override_user
+from tests.integration_tests.fixtures.importexport import chart_config
-def test_import_chart(mocker: MockFixture, session: Session) -> None:
+@pytest.fixture
+def session_with_data(session: Session) -> Generator[Session, None, None]:
+ engine = session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ dataset = SqlaTable(
+ table_name="test_table",
+ metrics=[],
+ main_dttm_col=None,
+ database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"),
+ )
+ session.add(dataset)
+ session.flush()
+ slice = Slice(
+ id=1,
+ datasource_id=dataset.id,
+ datasource_type="table",
+ datasource_name="tmp_perm_table",
+ slice_name="slice_name",
+ uuid=chart_config["uuid"],
+ )
+ session.add(slice)
+ session.flush()
+
+ yield session
+ session.rollback()
+
+
+@pytest.fixture
+def session_with_schema(session: Session) -> Generator[Session, None, None]:
+ from superset.connectors.sqla.models import SqlaTable
+
+ engine = session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ yield session
+
+
+def test_import_chart(mocker: MockFixture, session_with_schema: Session) -> None:
"""
Test importing a chart.
"""
- from superset import security_manager
- from superset.commands.chart.importers.v1.utils import import_chart
- from superset.connectors.sqla.models import SqlaTable
- from superset.models.core import Database
- from superset.models.slice import Slice
- from tests.integration_tests.fixtures.importexport import chart_config
mocker.patch.object(security_manager, "can_access", return_value=True)
- engine = session.get_bind()
- Slice.metadata.create_all(engine) # pylint: disable=no-member
-
config = copy.deepcopy(chart_config)
config["datasource_id"] = 1
config["datasource_type"] = "table"
- chart = import_chart(session, config)
+ chart = import_chart(session_with_schema, config)
assert chart.slice_name == "Deck Path"
assert chart.viz_type == "deck_path"
assert chart.is_managed_externally is False
assert chart.external_url is None
+ # Assert that the can write to chart was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Chart")
-def test_import_chart_managed_externally(mocker: MockFixture, session: Session) -> None:
+
+def test_import_chart_managed_externally(
+ mocker: MockFixture, session_with_schema: Session
+) -> None:
"""
Test importing a chart that is managed externally.
"""
- from superset import security_manager
- from superset.commands.chart.importers.v1.utils import import_chart
- from superset.connectors.sqla.models import SqlaTable
- from superset.models.core import Database
- from superset.models.slice import Slice
- from tests.integration_tests.fixtures.importexport import chart_config
-
mocker.patch.object(security_manager, "can_access", return_value=True)
- engine = session.get_bind()
- Slice.metadata.create_all(engine) # pylint: disable=no-member
-
config = copy.deepcopy(chart_config)
config["datasource_id"] = 1
config["datasource_type"] = "table"
config["is_managed_externally"] = True
config["external_url"] = "https://example.org/my_chart"
- chart = import_chart(session, config)
+ chart = import_chart(session_with_schema, config)
assert chart.is_managed_externally is True
assert chart.external_url == "https://example.org/my_chart"
+ # Assert that the can write to chart was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Chart")
+
def test_import_chart_without_permission(
mocker: MockFixture,
- session: Session,
+ session_with_schema: Session,
) -> None:
"""
Test importing a chart when a user doesn't have permissions to create.
"""
- from superset import security_manager
- from superset.commands.chart.importers.v1.utils import import_chart
- from superset.connectors.sqla.models import SqlaTable
- from superset.models.core import Database
- from superset.models.slice import Slice
- from tests.integration_tests.fixtures.importexport import chart_config
-
mocker.patch.object(security_manager, "can_access", return_value=False)
- engine = session.get_bind()
- Slice.metadata.create_all(engine) # pylint: disable=no-member
-
config = copy.deepcopy(chart_config)
config["datasource_id"] = 1
config["datasource_type"] = "table"
with pytest.raises(ImportFailedError) as excinfo:
- import_chart(session, config)
+ import_chart(session_with_schema, config)
assert (
str(excinfo.value)
== "Chart doesn't exist and user doesn't have permission to create charts"
)
+ # Assert that the can write to chart was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Chart")
-def test_filter_chart_annotations(mocker: MockFixture, session: Session) -> None:
+def test_filter_chart_annotations(session: Session) -> None:
"""
Test importing a chart.
"""
- from superset import security_manager
from superset.commands.chart.importers.v1.utils import filter_chart_annotations
from tests.integration_tests.fixtures.importexport import (
chart_config_with_mixed_annotations,
@@ -127,3 +153,67 @@ def test_filter_chart_annotations(mocker: MockFixture, session: Session) -> None
assert len(annotation_layers) == 1
assert all([al["annotationType"] == "FORMULA" for al in annotation_layers])
+
+
+def test_import_existing_chart_without_permission(
+ mocker: MockFixture,
+ session_with_data: Session,
+) -> None:
+ """
+ Test importing a chart when a user doesn't have permissions to modify.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "can_access_chart", return_value=False)
+
+ slice = (
+ session_with_data.query(Slice)
+ .filter(Slice.uuid == chart_config["uuid"])
+ .one_or_none()
+ )
+
+ with override_user("admin"):
+ with pytest.raises(ImportFailedError) as excinfo:
+ import_chart(session_with_data, chart_config, overwrite=True)
+ assert (
+ str(excinfo.value)
+ == "A chart already exists and user doesn't have permissions to overwrite it"
+ )
+
+ # Assert that the can write to chart was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Chart")
+ security_manager.can_access_chart.assert_called_once_with(slice)
+
+
+def test_import_existing_chart_with_permission(
+ mocker: MockFixture,
+ session_with_data: Session,
+) -> None:
+ """
+ Test importing a chart that exists when a user has access permission to that chart.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "can_access_chart", return_value=True)
+
+ admin = User(
+ first_name="Alice",
+ last_name="Doe",
+ email="adoe@example.org",
+ username="admin",
+ roles=[Role(name="Admin")],
+ )
+
+ config = copy.deepcopy(chart_config)
+ config["datasource_id"] = 1
+ config["datasource_type"] = "table"
+
+ slice = (
+ session_with_data.query(Slice)
+ .filter(Slice.uuid == config["uuid"])
+ .one_or_none()
+ )
+
+ with override_user(admin):
+ import_chart(session_with_data, config, overwrite=True)
+ # Assert that the can write to chart was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Chart")
+ security_manager.can_access_chart.assert_called_once_with(slice)
diff --git a/tests/unit_tests/dashboards/commands/importers/v1/import_test.py b/tests/unit_tests/dashboards/commands/importers/v1/import_test.py
index 190f0660f5..afbce49cd9 100644
--- a/tests/unit_tests/dashboards/commands/importers/v1/import_test.py
+++ b/tests/unit_tests/dashboards/commands/importers/v1/import_test.py
@@ -17,32 +17,57 @@
# pylint: disable=unused-argument, import-outside-toplevel, unused-import, invalid-name
import copy
+from collections.abc import Generator
import pytest
+from flask_appbuilder.security.sqla.models import Role, User
from pytest_mock import MockFixture
from sqlalchemy.orm.session import Session
+from superset import security_manager
+from superset.commands.dashboard.importers.v1.utils import import_dashboard
from superset.commands.exceptions import ImportFailedError
+from superset.models.dashboard import Dashboard
from superset.utils.core import override_user
+from tests.integration_tests.fixtures.importexport import dashboard_config
-def test_import_dashboard(mocker: MockFixture, session: Session) -> None:
- """
- Test importing a dashboard.
- """
- from superset import security_manager
- from superset.commands.dashboard.importers.v1.utils import import_dashboard
- from superset.models.slice import Slice
- from tests.integration_tests.fixtures.importexport import dashboard_config
+@pytest.fixture
+def session_with_data(session: Session) -> Generator[Session, None, None]:
+ engine = session.get_bind()
+ Dashboard.metadata.create_all(engine) # pylint: disable=no-member
+
+ dashboard = Dashboard(
+ id=100,
+ dashboard_title="Test dash",
+ slug=None,
+ slices=[],
+ published=True,
+ uuid=dashboard_config["uuid"],
+ )
+
+ session.add(dashboard)
+ session.flush()
+ yield session
+ session.rollback()
- mocker.patch.object(security_manager, "can_access", return_value=True)
+@pytest.fixture
+def session_with_schema(session: Session) -> Generator[Session, None, None]:
engine = session.get_bind()
- Slice.metadata.create_all(engine) # pylint: disable=no-member
+ Dashboard.metadata.create_all(engine) # pylint: disable=no-member
- config = copy.deepcopy(dashboard_config)
+ yield session
+ session.rollback()
+
+
+def test_import_dashboard(mocker: MockFixture, session_with_schema: Session) -> None:
+ """
+ Test importing a dashboard.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
- dashboard = import_dashboard(session, config)
+ dashboard = import_dashboard(session_with_schema, dashboard_config)
assert dashboard.dashboard_title == "Test dash"
assert dashboard.description is None
assert dashboard.is_managed_externally is False
@@ -53,26 +78,18 @@ def test_import_dashboard(mocker: MockFixture, session: Session) -> None:
def test_import_dashboard_managed_externally(
mocker: MockFixture,
- session: Session,
+ session_with_schema: Session,
) -> None:
"""
Test importing a dashboard that is managed externally.
"""
- from superset import security_manager
- from superset.commands.dashboard.importers.v1.utils import import_dashboard
- from superset.models.slice import Slice
- from tests.integration_tests.fixtures.importexport import dashboard_config
-
mocker.patch.object(security_manager, "can_access", return_value=True)
- engine = session.get_bind()
- Slice.metadata.create_all(engine) # pylint: disable=no-member
-
config = copy.deepcopy(dashboard_config)
config["is_managed_externally"] = True
config["external_url"] = "https://example.org/my_dashboard"
- dashboard = import_dashboard(session, config)
+ dashboard = import_dashboard(session_with_schema, config)
assert dashboard.is_managed_externally is True
assert dashboard.external_url == "https://example.org/my_dashboard"
@@ -82,25 +99,15 @@ def test_import_dashboard_managed_externally(
def test_import_dashboard_without_permission(
mocker: MockFixture,
- session: Session,
+ session_with_schema: Session,
) -> None:
"""
Test importing a dashboard when a user doesn't have permissions to create.
"""
- from superset import security_manager
- from superset.commands.dashboard.importers.v1.utils import import_dashboard
- from superset.models.slice import Slice
- from tests.integration_tests.fixtures.importexport import dashboard_config
-
mocker.patch.object(security_manager, "can_access", return_value=False)
- engine = session.get_bind()
- Slice.metadata.create_all(engine) # pylint: disable=no-member
-
- config = copy.deepcopy(dashboard_config)
-
with pytest.raises(ImportFailedError) as excinfo:
- import_dashboard(session, config)
+ import_dashboard(session_with_schema, dashboard_config)
assert (
str(excinfo.value)
== "Dashboard doesn't exist and user doesn't have permission to create dashboards"
@@ -112,72 +119,43 @@ def test_import_dashboard_without_permission(
def test_import_existing_dashboard_without_permission(
mocker: MockFixture,
- session: Session,
+ session_with_data: Session,
) -> None:
"""
Test importing a dashboard when a user doesn't have permissions to create.
"""
- from superset import security_manager
- from superset.commands.dashboard.importers.v1.utils import g, import_dashboard
- from superset.models.dashboard import Dashboard
- from superset.models.slice import Slice
- from tests.integration_tests.fixtures.importexport import dashboard_config
-
mocker.patch.object(security_manager, "can_access", return_value=True)
mocker.patch.object(security_manager, "can_access_dashboard", return_value=False)
- mock_g = mocker.patch(
- "superset.commands.dashboard.importers.v1.utils.g"
- ) # Replace with the actual path to g
- mock_g.user = mocker.MagicMock(return_value=True)
-
- engine = session.get_bind()
- Slice.metadata.create_all(engine) # pylint: disable=no-member
- Dashboard.metadata.create_all(engine) # pylint: disable=no-member
- dashboard_obj = Dashboard(
- id=100,
- dashboard_title="Test dash",
- slug=None,
- slices=[],
- published=True,
- uuid="c4b28c4e-a1fe-4cf8-a5ac-d6f11d6fdd51",
+ dashboard = (
+ session_with_data.query(Dashboard)
+ .filter(Dashboard.uuid == dashboard_config["uuid"])
+ .one_or_none()
)
- session.add(dashboard_obj)
- session.flush()
- config = copy.deepcopy(dashboard_config)
- with pytest.raises(ImportFailedError) as excinfo:
- import_dashboard(session, config, overwrite=True)
- assert (
- str(excinfo.value)
- == "A dashboard already exists and user doesn't have permissions to overwrite it"
- )
+ with override_user("admin"):
+ with pytest.raises(ImportFailedError) as excinfo:
+ import_dashboard(session_with_data, dashboard_config, overwrite=True)
+ assert (
+ str(excinfo.value)
+ == "A dashboard already exists and user doesn't have permissions to overwrite it"
+ )
# Assert that the can write to dashboard was checked
security_manager.can_access.assert_called_once_with("can_write", "Dashboard")
- security_manager.can_access_dashboard.assert_called_once_with(dashboard_obj)
+ security_manager.can_access_dashboard.assert_called_once_with(dashboard)
def test_import_existing_dashboard_with_permission(
mocker: MockFixture,
- session: Session,
+ session_with_data: Session,
) -> None:
"""
- Test importing a dashboard when a user doesn't have permissions to create.
+ Test importing a dashboard that exists when a user has access permission to that dashboard.
"""
- from flask_appbuilder.security.sqla.models import Role, User
-
- from superset import security_manager
- from superset.commands.dashboard.importers.v1.utils import import_dashboard
- from superset.models.dashboard import Dashboard
- from tests.integration_tests.fixtures.importexport import dashboard_config
-
mocker.patch.object(security_manager, "can_access", return_value=True)
mocker.patch.object(security_manager, "can_access_dashboard", return_value=True)
- engine = session.get_bind()
- Dashboard.metadata.create_all(engine) # pylint: disable=no-member
-
admin = User(
first_name="Alice",
last_name="Doe",
@@ -186,20 +164,14 @@ def test_import_existing_dashboard_with_permission(
roles=[Role(name="Admin")],
)
- dashboard_obj = Dashboard(
- id=100,
- dashboard_title="Test dash",
- slug=None,
- slices=[],
- published=True,
- uuid="c4b28c4e-a1fe-4cf8-a5ac-d6f11d6fdd51",
+ dashboard = (
+ session_with_data.query(Dashboard)
+ .filter(Dashboard.uuid == dashboard_config["uuid"])
+ .one_or_none()
)
- session.add(dashboard_obj)
- session.flush()
- config = copy.deepcopy(dashboard_config)
with override_user(admin):
- import_dashboard(session, config, overwrite=True)
+ import_dashboard(session_with_data, dashboard_config, overwrite=True)
# Assert that the can write to dashboard was checked
security_manager.can_access.assert_called_once_with("can_write", "Dashboard")
- security_manager.can_access_dashboard.assert_called_once_with(dashboard_obj)
+ security_manager.can_access_dashboard.assert_called_once_with(dashboard)
diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py
index ad6e53e993..7d2b9153a3 100644
--- a/tests/unit_tests/security/manager_test.py
+++ b/tests/unit_tests/security/manager_test.py
@@ -16,12 +16,16 @@
# under the License.
import pytest
+from flask_appbuilder.security.sqla.models import Role, User
from pytest_mock import MockFixture
from superset.common.query_object import QueryObject
+from superset.connectors.sqla.models import Database, SqlaTable
from superset.exceptions import SupersetSecurityException
from superset.extensions import appbuilder
+from superset.models.slice import Slice
from superset.security.manager import SupersetSecurityManager
+from superset.utils.core import override_user
def test_security_manager(app_context: None) -> None:
@@ -164,3 +168,139 @@ def test_raise_for_access_query_default_schema(
== """You need access to the following tables: `public.ab_user`,
`all_database_access` or `all_datasource_access` permission"""
)
+
+
+def test_raise_for_access_chart_for_datasource_permission(
+ mocker: MockFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that the security manager can raise an exception for chart access,
+ when the user does not have access to the chart datasource
+ """
+ sm = SupersetSecurityManager(appbuilder)
+ session = sm.get_session
+
+ engine = session.get_bind()
+ Slice.metadata.create_all(engine) # pylint: disable=no-member
+
+ alpha = User(
+ first_name="Alice",
+ last_name="Doe",
+ email="adoe@example.org",
+ username="admin",
+ roles=[Role(name="Alpha")],
+ )
+
+ dataset = SqlaTable(
+ table_name="test_table",
+ metrics=[],
+ main_dttm_col=None,
+ database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"),
+ )
+ session.add(dataset)
+ session.flush()
+
+ slice = Slice(
+ id=1,
+ datasource_id=dataset.id,
+ datasource_type="table",
+ datasource_name="tmp_perm_table",
+ slice_name="slice_name",
+ )
+ session.add(slice)
+ session.flush()
+
+ mocker.patch.object(sm, "can_access_datasource", return_value=False)
+ with override_user(alpha):
+ with pytest.raises(SupersetSecurityException) as excinfo:
+ sm.raise_for_access(
+ chart=slice,
+ )
+ assert str(excinfo.value) == "You don't have access to this chart."
+
+ mocker.patch.object(sm, "can_access_datasource", return_value=True)
+ with override_user(alpha):
+ sm.raise_for_access(
+ chart=slice,
+ )
+
+
+def test_raise_for_access_chart_on_admin(
+ app_context: None,
+) -> None:
+ """
+ Test that the security manager can raise an exception for chart access,
+ when the user does not have access to the chart datasource
+ """
+ from flask_appbuilder.security.sqla.models import Role, User
+
+ from superset.models.slice import Slice
+ from superset.utils.core import override_user
+
+ sm = SupersetSecurityManager(appbuilder)
+ session = sm.get_session
+
+ engine = session.get_bind()
+ Slice.metadata.create_all(engine) # pylint: disable=no-member
+
+ admin = User(
+ first_name="Alice",
+ last_name="Doe",
+ email="adoe@example.org",
+ username="admin",
+ roles=[Role(name="Admin")],
+ )
+
+ slice = Slice(
+ id=1,
+ datasource_id=1,
+ datasource_type="table",
+ datasource_name="tmp_perm_table",
+ slice_name="slice_name",
+ )
+ session.add(slice)
+ session.flush()
+
+ with override_user(admin):
+ sm.raise_for_access(
+ chart=slice,
+ )
+
+
+def test_raise_for_access_chart_owner(
+ app_context: None,
+) -> None:
+ """
+ Test that the security manager can raise an exception for chart access,
+ when the user does not have access to the chart datasource
+ """
+ sm = SupersetSecurityManager(appbuilder)
+ session = sm.get_session
+
+ engine = session.get_bind()
+ Slice.metadata.create_all(engine) # pylint: disable=no-member
+
+ alpha = User(
+ first_name="Alice",
+ last_name="Doe",
+ email="adoe@example.org",
+ username="admin",
+ roles=[Role(name="Alpha")],
+ )
+
+ slice = Slice(
+ id=1,
+ datasource_id=1,
+ datasource_type="table",
+ datasource_name="tmp_perm_table",
+ slice_name="slice_name",
+ owners=[alpha],
+ )
+ session.add(slice)
+ session.flush()
+
+ with override_user(alpha):
+ sm.raise_for_access(
+ chart=slice,
+ )