Skip to content

Commit

Permalink
fix: Address dashboard permission regression in #23586
Browse files Browse the repository at this point in the history
  • Loading branch information
@john-bodley
john-bodley committed Jun 10, 2023
1 parent 0e3f1f6 commit 9bba03b
Show file tree
Hide file tree
Showing 7 changed files with 166 additions and 194 deletions.
93 changes: 56 additions & 37 deletions superset/security/manager.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -348,30 +348,28 @@ def can_access_all_queries(self) -> bool:

def can_access_all_datasources(self) -> bool:
"""
Return True if the user can fully access all the Superset datasources, False
otherwise.
Return True if the user can access all the datasources, False otherwise.
:returns: Whether the user can fully access all Superset datasources
:returns: Whether the user can access all the datasources
"""

return self.can_access("all_datasource_access", "all_datasource_access")

def can_access_all_databases(self) -> bool:
"""
Return True if the user can fully access all the Superset databases, False
otherwise.
Return True if the user can access all the databases, False otherwise.
:returns: Whether the user can fully access all Superset databases
:returns: Whether the user can access all the databases
"""

return self.can_access("all_database_access", "all_database_access")

def can_access_database(self, database: "Database") -> bool:
"""
Return True if the user can fully access the Superset database, False otherwise.
Return True if the user can access the specified database, False otherwise.
:param database: The Superset database
:returns: Whether the user can fully access the Superset database
:param database: The database
:returns: Whether the user can access the database
"""

return (
Expand All @@ -382,11 +380,11 @@ def can_access_database(self, database: "Database") -> bool:

def can_access_schema(self, datasource: "BaseDatasource") -> bool:
"""
Return True if the user can fully access the schema associated with the Superset
Return True if the user can access the schema associated with specified
datasource, False otherwise.
:param datasource: The Superset datasource
:returns: Whether the user can fully access the datasource's schema
:param datasource: The datasource
:returns: Whether the user can access the datasource's schema
"""

return (
Expand All @@ -397,11 +395,10 @@ def can_access_schema(self, datasource: "BaseDatasource") -> bool:

def can_access_datasource(self, datasource: "BaseDatasource") -> bool:
"""
Return True if the user can fully access of the Superset datasource, False
otherwise.
Return True if the user can access the specified datasource, False otherwise.
:param datasource: The Superset datasource
:returns: Whether the user can fully access the Superset datasource
:param datasource: The datasource
:returns: Whether the user can access the datasource
"""

try:
Expand All @@ -411,6 +408,24 @@ def can_access_datasource(self, datasource: "BaseDatasource") -> bool:

return True

def can_access_dashboard(self, dashboard: "Dashboard") -> bool:
"""
Return True if the user can access the specified dashboard, False otherwise.
:param dashboard: The dashboard
:returns: Whether the user can access the dashboard
"""

# pylint: disable=import-outside-toplevel
from superset.dashboards.commands.exceptions import DashboardAccessDeniedError

try:
self.raise_for_dashboard_access(dashboard)
except DashboardAccessDeniedError:
return False

return True

@staticmethod
def get_datasource_access_error_msg(datasource: "BaseDatasource") -> str:
"""
Expand Down Expand Up @@ -1995,38 +2010,42 @@ def raise_for_user_activity_access(user_id: int) -> None:
def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None:
"""
Raise an exception if the user cannot access the dashboard.
This does not check for the required role/permission pairs,
it only concerns itself with entity relationships.
This does not check for the required role/permission pairs, it only concerns
itself with entity relationships.
:param dashboard: Dashboard the user wants access to
:raises DashboardAccessDeniedError: If the user cannot access the resource
"""

# pylint: disable=import-outside-toplevel
from superset import is_feature_enabled
from superset.dashboards.commands.exceptions import DashboardAccessDeniedError

def has_rbac_access() -> bool:
if not is_feature_enabled("DASHBOARD_RBAC") or not dashboard.roles:
return True

return any(
dashboard_role.id
in [user_role.id for user_role in self.get_user_roles()]
for dashboard_role in dashboard.roles
)

if self.is_guest_user() and dashboard.embedded:
can_access = self.has_guest_access(dashboard)
if self.has_guest_access(dashboard):
return
else:
can_access = (
self.is_admin()
or self.is_owner(dashboard)
or (dashboard.published and has_rbac_access())
or (not dashboard.published and not dashboard.roles)
)
if self.is_admin() or self.is_owner(dashboard):
return

# RBAC and legacy (datasource inferred) access controls.
if is_feature_enabled("DASHBOARD_RBAC") and dashboard.roles:
if dashboard.published and {role.id for role in dashboard.roles} & {
role.id for role in self.get_user_roles()
}:
return
elif (
not dashboard.published
or not dashboard.datasources
or any(
self.can_access_datasource(datasource)
for datasource in dashboard.datasources
)
):
return

if not can_access:
raise DashboardAccessDeniedError()
raise DashboardAccessDeniedError()

@staticmethod
def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool:
Expand Down
27 changes: 0 additions & 27 deletions superset/utils/decorators.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,10 @@
import time
from collections.abc import Iterator
from contextlib import contextmanager
from functools import wraps
from typing import Any, Callable, TYPE_CHECKING

from flask import current_app, Response

from superset import is_feature_enabled
from superset.dashboards.commands.exceptions import DashboardAccessDeniedError
from superset.utils import core as utils
from superset.utils.dates import now_as_float

Expand Down Expand Up @@ -114,27 +111,3 @@ def wrapped(*args: Any, **kwargs: Any) -> Any:

def on_security_exception(self: Any, ex: Exception) -> Response:
return self.response(403, **{"message": utils.error_msg_from_exception(ex)})


# noinspection PyPackageRequirements
def check_dashboard_access(on_error: Callable[[str], Any]) -> Callable[..., Any]:
def decorator(f: Callable[..., Any]) -> Callable[..., Any]:
@wraps(f)
def wrapper(self: Any, *args: Any, **kwargs: Any) -> Any:
# pylint: disable=import-outside-toplevel
from superset.models.dashboard import Dashboard

dashboard = Dashboard.get(str(kwargs["dashboard_id_or_slug"]))
if is_feature_enabled("DASHBOARD_RBAC"):
try:
current_app.appbuilder.sm.raise_for_dashboard_access(dashboard)
except DashboardAccessDeniedError as ex:
return on_error(str(ex))
except Exception as exception:
raise exception

return f(self, *args, dashboard=dashboard, **kwargs)

return wrapper

return decorator
77 changes: 28 additions & 49 deletions superset/views/core.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@
ReservedUrlParameters,
)
from superset.utils.dates import now_as_float
from superset.utils.decorators import check_dashboard_access
from superset.views.base import (
api,
BaseSupersetView,
Expand Down Expand Up @@ -190,7 +189,6 @@
"disable_data_preview",
]

DASHBOARD_LIST_URL = "/dashboard/list/"
DATASOURCE_MISSING_ERR = __("The data source seems to have been deleted")
USER_MISSING_ERR = __("The user seems to have been deleted")
PARAMETER_MISSING_ERR = __(
Expand Down Expand Up @@ -1641,76 +1639,57 @@ def favstar( # pylint: disable=no-self-use
@has_access
@expose("/dashboard/<dashboard_id_or_slug>/")
@event_logger.log_this_with_extra_payload
@check_dashboard_access(
on_error=lambda msg: redirect_with_flash(DASHBOARD_LIST_URL, msg, "danger")
)
def dashboard(
self,
dashboard_id_or_slug: str, # pylint: disable=unused-argument
dashboard_id_or_slug: str,
add_extra_log_payload: Callable[..., None] = lambda **kwargs: None,
dashboard: Dashboard | None = None,
) -> FlaskResponse:
"""
Server side rendering for a dashboard
:param dashboard_id_or_slug: identifier for dashboard. used in the decorators
Server side rendering for a dashboard.
:param dashboard_id_or_slug: identifier for dashboard
:param add_extra_log_payload: added by `log_this_with_manual_updates`, set a
default value to appease pylint
:param dashboard: added by `check_dashboard_access`
"""

dashboard = Dashboard.get(dashboard_id_or_slug)

if not dashboard:
abort(404)

assert dashboard is not None

has_access_ = False
for datasource in dashboard.datasources:
datasource = DatasourceDAO.get_datasource(
datasource_type=DatasourceType(datasource.type),
datasource_id=datasource.id,
session=db.session(),
try:
security_manager.raise_for_dashboard_access(dashboard)
except DashboardAccessDeniedError as ex:
return redirect_with_flash(
url="/dashboard/list/",
message=utils.error_msg_from_exception(ex),
category="danger",
)
if datasource and security_manager.can_access_datasource(
datasource=datasource,
):
has_access_ = True

if has_access_:
break

if dashboard.datasources and not has_access_:
flash(DashboardAccessDeniedError.message, "danger")
return redirect(DASHBOARD_LIST_URL)

dash_edit_perm = security_manager.is_owner(
dashboard
) and security_manager.can_access("can_save_dash", "Superset")
edit_mode = (
request.args.get(utils.ReservedUrlParameters.EDIT_MODE.value) == "true"
)

standalone_mode = ReservedUrlParameters.is_standalone_mode()

add_extra_log_payload(
dashboard_id=dashboard.id,
dashboard_version="v2",
dash_edit_perm=dash_edit_perm,
edit_mode=edit_mode,
dash_edit_perm=(
security_manager.is_owner(dashboard)
and security_manager.can_access("can_save_dash", "Superset")
),
edit_mode=(
request.args.get(ReservedUrlParameters.EDIT_MODE.value) == "true"
),
)

bootstrap_data = {
"user": bootstrap_user_data(g.user, include_perms=True),
"common": common_bootstrap_payload(g.user),
}

return self.render_template(
"superset/spa.html",
entry="spa",
# dashboard title is always visible
title=dashboard.dashboard_title,
title=dashboard.dashboard_title, # dashboard title is always visible
bootstrap_data=json.dumps(
bootstrap_data, default=utils.pessimistic_json_iso_dttm_ser
{
"user": bootstrap_user_data(g.user, include_perms=True),
"common": common_bootstrap_payload(g.user),
},
default=utils.pessimistic_json_iso_dttm_ser,
),
standalone_mode=standalone_mode,
standalone_mode=ReservedUrlParameters.is_standalone_mode(),
)

@has_access
Expand Down
42 changes: 42 additions & 0 deletions tests/integration_tests/base_tests.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,48 @@ def put_assert_metric(
def get_dttm(cls):
return datetime.strptime("2019-01-02 03:04:05.678900", "%Y-%m-%d %H:%M:%S.%f")

def insert_dashboard(
self,
dashboard_title: str,
slug: Optional[str],
owners: list[int],
roles: list[int] = [],
created_by=None,
slices: Optional[list[Slice]] = None,
position_json: str = "",
css: str = "",
json_metadata: str = "",
published: bool = False,
certified_by: Optional[str] = None,
certification_details: Optional[str] = None,
) -> Dashboard:
obj_owners = list()
obj_roles = list()
slices = slices or []
for owner in owners:
user = db.session.query(security_manager.user_model).get(owner)
obj_owners.append(user)
for role in roles:
role_obj = db.session.query(security_manager.role_model).get(role)
obj_roles.append(role_obj)
dashboard = Dashboard(
dashboard_title=dashboard_title,
slug=slug,
owners=obj_owners,
roles=obj_roles,
position_json=position_json,
css=css,
json_metadata=json_metadata,
slices=slices,
published=published,
created_by=created_by,
certified_by=certified_by,
certification_details=certification_details,
)
db.session.add(dashboard)
db.session.commit()
return dashboard


@contextmanager
def db_insert_temp_object(obj: DeclarativeMeta):
Expand Down
Loading

0 comments on commit 9bba03b

Please sign in to comment.