iop
/
superset
4
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix: Address dashboard permission regression in #23586 (#24350)

This commit is contained in:
John Bodley 2023-06-12 11:15:27 -07:00 committed by GitHub
parent c728cdf501
commit a3aacf2527
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 166 additions and 194 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

93
superset/security/manager.py
View File

@ -348,30 +348,28 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
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 (
@ -382,11 +380,11 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
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 (
@ -397,11 +395,10 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
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:
@ -411,6 +408,24 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
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:
"""
@ -1998,38 +2013,42 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
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
if not can_access:
raise DashboardAccessDeniedError()
# 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
raise DashboardAccessDeniedError()
@staticmethod
def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool:

27
superset/utils/decorators.py
View File

@ -19,13 +19,10 @@ from __future__ import annotations
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
@ -114,27 +111,3 @@ def debounce(duration: float | int = 0.1) -> Callable[..., 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
superset/views/core.py
View File

@ -128,7 +128,6 @@ from superset.utils.async_query_manager import AsyncQueryTokenException
from superset.utils.cache import etag_cache
from superset.utils.core import DatasourceType, get_user_id, ReservedUrlParameters
from superset.utils.dates import now_as_float
from superset.utils.decorators import check_dashboard_access
from superset.views.base import (
api,
BaseSupersetView,
@ -183,7 +182,6 @@ DATABASE_KEYS = [
"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 = __(
@ -1482,76 +1480,57 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods
@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

42
tests/integration_tests/base_tests.py
View File

@ -504,6 +504,48 @@ class SupersetTestCase(TestCase):
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):

80
tests/integration_tests/dashboards/api_tests.py
View File

@ -19,7 +19,6 @@
import json
from io import BytesIO
from time import sleep
from typing import Optional
from unittest.mock import ANY, patch
from zipfile import is_zipfile, ZipFile
@ -77,48 +76,6 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixi
"published": False,
}
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
@pytest.fixture()
def create_dashboards(self):
with self.create_app().app_context():
@ -507,43 +464,6 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixi
db.session.delete(dashboard)
db.session.commit()
def test_get_draft_dashboard_without_roles_by_uuid(self):
"""
Dashboard API: Test get draft dashboard without roles by uuid
"""
admin = self.get_user("admin")
dashboard = self.insert_dashboard("title", "slug1", [admin.id])
assert not dashboard.published
assert dashboard.roles == []
self.login(username="gamma")
uri = f"api/v1/dashboard/{dashboard.uuid}"
rv = self.client.get(uri)
assert rv.status_code == 200
# rollback changes
db.session.delete(dashboard)
db.session.commit()
def test_cannot_get_draft_dashboard_with_roles_by_uuid(self):
"""
Dashboard API: Test get dashboard by uuid
"""
admin = self.get_user("admin")
admin_role = self.get_role("Admin")
dashboard = self.insert_dashboard(
"title", "slug1", [admin.id], roles=[admin_role.id]
)
assert not dashboard.published
assert dashboard.roles == [admin_role]
self.login(username="gamma")
uri = f"api/v1/dashboard/{dashboard.uuid}"
rv = self.client.get(uri)
assert rv.status_code == 403
# rollback changes
db.session.delete(dashboard)
db.session.commit()
def test_get_dashboards_changed_on(self):
"""
Dashboard API: Test get dashboards changed on

4
tests/integration_tests/dashboards/security/security_dataset_tests.py
View File

@ -22,6 +22,7 @@ import pytest
from flask import escape
from superset import app
from superset.dashboards.dao import DashboardDAO
from superset.models import core as models
from tests.integration_tests.dashboards.base_case import DashboardTestCase
from tests.integration_tests.dashboards.consts import *
@ -223,7 +224,7 @@ class TestDashboardDatasetSecurity(DashboardTestCase):
"""
admin = self.get_user("admin")
title = f"title{random_str()}"
create_dashboard_to_db(title, "slug1", owners=[admin])
dashboard = create_dashboard_to_db(title, "slug1", owners=[admin])
self.login(username="gamma")
arguments = {
@ -234,3 +235,4 @@ class TestDashboardDatasetSecurity(DashboardTestCase):
self.assert200(rv)
data = json.loads(rv.data.decode("utf-8"))
self.assertEqual(0, data["count"])
DashboardDAO.delete(dashboard)

37
tests/integration_tests/dashboards/security/security_rbac_tests.py
View File

@ -395,3 +395,40 @@ class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity):
# post
for dash in published_dashboards + draft_dashboards:
revoke_access_to_dashboard(dash, "Public")
def test_get_draft_dashboard_without_roles_by_uuid(self):
"""
Dashboard API: Test get draft dashboard without roles by uuid
"""
admin = self.get_user("admin")
dashboard = self.insert_dashboard("title", "slug1", [admin.id])
assert not dashboard.published
assert dashboard.roles == []
self.login(username="gamma")
uri = f"api/v1/dashboard/{dashboard.uuid}"
rv = self.client.get(uri)
assert rv.status_code == 200
# rollback changes
db.session.delete(dashboard)
db.session.commit()
def test_cannot_get_draft_dashboard_with_roles_by_uuid(self):
"""
Dashboard API: Test get dashboard by uuid
"""
admin = self.get_user("admin")
admin_role = self.get_role("Admin")
dashboard = self.insert_dashboard(
"title", "slug1", [admin.id], roles=[admin_role.id]
)
assert not dashboard.published
assert dashboard.roles == [admin_role]
self.login(username="gamma")
uri = f"api/v1/dashboard/{dashboard.uuid}"
rv = self.client.get(uri)
assert rv.status_code == 403
# rollback changes
db.session.delete(dashboard)
db.session.commit()