Skip to content

fix: Address dashboard permission regression in #23586 #24350

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.

Sign up for GitHub

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

Merged
john-bodley merged 1 commit into from
Jun 12, 2023
Merged

fix: Address dashboard permission regression in #23586 #24350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
93 changes: 56 additions & 37 deletions superset/security/manager.py
View file
Open in desktop
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:
Copy link
Copy Markdown
Member Author

@john-bodley john-bodley Jun 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic has been inlined below.

if not is_feature_enabled("DASHBOARD_RBAC") or not dashboard.roles:
Copy link
Copy Markdown
Member Author

@john-bodley john-bodley Jun 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a tad hard to digest, but it essence is checking if RBAC is disabled. A more digestible form (used below) to check if RBAC is enabled is,

if is_feature_enabled("DASHBOARD_RBAC") and 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())
Copy link
Copy Markdown
Member Author

@john-bodley john-bodley Jun 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm struggling to grok how/why the published state alters things. Note the challenge is logic outlined here isn't the entire corpus of rules from an access standpoint given that the view also had additional (potentially conflicting) logic based on whether the user had access to the underlying datasets.

Personally I feel the access rubric is already quite complex and rather difficult to grok as it pertains to embedded dashboards and the like. Additionally I'm not sure I agree with the logic with regards to RBAC when no roles are defined. Personally this feels like one shouldn't strictly have access as opposed to falling back to the legacy logic. Do dashboard owners et al. grok how this works?

Copy link
Copy Markdown
Member

@villebro villebro Jun 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally this feels like one shouldn't strictly have access as opposed to falling back to the legacy logic. Do dashboard owners et al. grok how this works?

The reason why it's like this is because otherwise it's impossible to support both regular RBAC and "dashboard RBAC" on the same running instance. I know it may be slightly confusing, but it's clearly documented, both in the modal and the docs, so changing this would be a pretty substantial breaking change.

Copy link
Copy Markdown
Member Author

@john-bodley john-bodley Jun 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@villebro is the desired end state to support only one or do you perceive both coexisting for some time?

Copy link
Copy Markdown
Member

@villebro villebro Jun 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@john-bodley I kind of have a SIP brewing for a more universal object level RBAC model. I'll circulate it with you once I have the main design ready, as I'm also curious to hear your thoughts on it.

or (not dashboard.published and not dashboard.roles)
Copy link
Copy Markdown
Member Author

@john-bodley john-bodley Jun 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't dashboards roles only a thing if RBAC is enabled?

)
if self.is_admin() or self.is_owner(dashboard):
return

# RBAC and legacy (datasource inferred) access controls.
Copy link
Copy Markdown
Member Author

@john-bodley john-bodley Jun 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TL;DR is this check now includes logic for the legacy access controls which were based (for right or wrong) around the notion that a user has access to the dashboard if they can access at least one of the underlying datasets which backs the dashboard. Said logic previously resided in the dashboard view.

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
Open in desktop
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
Copy link
Copy Markdown
Member Author

@john-bodley john-bodley Jun 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This decorator seemed superfluous as said logic could be handled elsewhere.

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
Open in desktop
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)
Copy link
Copy Markdown
Member Author

@john-bodley john-bodley Jun 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is concerning that this is different than the DashboardDAO.get_by_id_or_slug and that the DAO logic differs (from a filter perspective) whether you pass in a UUID versus a slug/ID.

Personally the UUID logic is just adding more complexity to an already partially intractable problem in terms of understanding (and thus enforcing) the security model. Adhering to the KISS principle in terms of the security model will actually make the service more secure as it'll be easier to grok and enforce.


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(
Copy link
Copy Markdown
Member Author

@john-bodley john-bodley Jun 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inlined later for improved readability.

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 = {
Copy link
Copy Markdown
Member Author

@john-bodley john-bodley Jun 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inlined later.

"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
Open in desktop
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