Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: chart import validation #26993

Merged
merged 9 commits into from
Feb 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions superset/commands/chart/importers/v1/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,14 @@
from inspect import isclass
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.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:
Expand All @@ -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
Expand All @@ -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

Expand Down
8 changes: 4 additions & 4 deletions superset/commands/dashboard/importers/v1/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@
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__)

Expand Down Expand Up @@ -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 "
Expand Down Expand Up @@ -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
7 changes: 4 additions & 3 deletions superset/commands/dataset/importers/v1/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
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
Expand All @@ -33,6 +33,7 @@
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__)

Expand Down Expand Up @@ -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

Expand Down
1 change: 1 addition & 0 deletions superset/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
9 changes: 4 additions & 5 deletions superset/jinja_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
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,
)

Expand Down Expand Up @@ -110,11 +110,10 @@
:returns: The user ID
"""

if hasattr(g, "user") and g.user:
id_ = get_user_id()
if user := get_user():

Check warning on line 113 in superset/jinja_context.py

View check run for this annotation

Codecov / codecov/patch

superset/jinja_context.py#L113

Added line #L113 was not covered by tests
if add_to_cache_keys:
self.cache_key_wrapper(id_)
return id_
self.cache_key_wrapper(user.id)
return user.id

Check warning on line 116 in superset/jinja_context.py

View check run for this annotation

Codecov / codecov/patch

superset/jinja_context.py#L115-L116

Added lines #L115 - L116 were not covered by tests
return None

def current_username(self, add_to_cache_keys: bool = True) -> Optional[str]:
Expand Down
41 changes: 41 additions & 0 deletions superset/security/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
)
from superset.models.core import Database
from superset.models.dashboard import Dashboard
from superset.models.slice import Slice

Check warning on line 88 in superset/security/manager.py

View check run for this annotation

Codecov / codecov/patch

superset/security/manager.py#L88

Added line #L88 was not covered by tests
from superset.models.sql_lab import Query
from superset.sql_parse import Table
from superset.viz import BaseViz
Expand Down Expand Up @@ -422,6 +423,19 @@

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

Check warning on line 435 in superset/security/manager.py

View check run for this annotation

Codecov / codecov/patch

superset/security/manager.py#L434-L435

Added lines #L434 - L435 were not covered by tests

return True

def get_dashboard_access_error_object( # pylint: disable=invalid-name
self,
dashboard: "Dashboard", # pylint: disable=unused-argument
Expand All @@ -439,6 +453,23 @@
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:
"""
Expand Down Expand Up @@ -1822,6 +1853,7 @@
# 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,
Expand Down Expand Up @@ -2044,6 +2076,15 @@
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
Expand Down
9 changes: 9 additions & 0 deletions superset/utils/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion tests/integration_tests/charts/commands_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ def test_export_chart_command_no_related(self, mock_g):


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"""
Expand Down
2 changes: 1 addition & 1 deletion tests/integration_tests/dashboards/commands_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ def test_import_v0_dashboard_cli_export(self):
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"""
Expand Down
2 changes: 1 addition & 1 deletion tests/integration_tests/datasets/commands_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ def test_import_v0_dataset_ui_export(self):
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):
Expand Down
Loading
Loading