You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by vi...@apache.org on 2023/04/07 06:53:44 UTC
[superset] branch master updated: fix(dashboard-rbac): use normal rbac when no roles chosen (#23586)
This is an automated email from the ASF dual-hosted git repository.
villebro 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 a8230336ff fix(dashboard-rbac): use normal rbac when no roles chosen (#23586)
a8230336ff is described below
commit a8230336fffd87b1f6341896302d46d2cd12c818
Author: Ville Brofeldt <33...@users.noreply.github.com>
AuthorDate: Fri Apr 7 09:53:35 2023 +0300
fix(dashboard-rbac): use normal rbac when no roles chosen (#23586)
---
.../creating-your-first-dashboard.mdx | 5 +-
.../dashboard/components/PropertiesModal/index.tsx | 2 +-
superset/security/manager.py | 18 ++++---
superset/utils/decorators.py | 6 +--
superset/views/core.py | 51 +++++++++++--------
superset/views/dashboard/mixin.py | 2 +-
superset/views/utils.py | 8 ++-
.../dashboards/security/security_rbac_tests.py | 59 +++++++++++++++++++---
8 files changed, 107 insertions(+), 44 deletions(-)
diff --git a/docs/docs/creating-charts-dashboards/creating-your-first-dashboard.mdx b/docs/docs/creating-charts-dashboards/creating-your-first-dashboard.mdx
index aeea6c2014..23a6b777b9 100644
--- a/docs/docs/creating-charts-dashboards/creating-your-first-dashboard.mdx
+++ b/docs/docs/creating-charts-dashboards/creating-your-first-dashboard.mdx
@@ -184,9 +184,8 @@ Non-owner users access can be managed two different ways:
1. Dataset permissions - if you add to the relevant role permissions to datasets it automatically grants implicit access to all dashboards that uses those permitted datasets
2. Dashboard roles - if you enable **DASHBOARD_RBAC** [feature flag](https://superset.apache.org/docs/installation/configuring-superset#feature-flags) then you be able to manage which roles can access the dashboard
-- Having dashboard access implicitly grants read access to the associated datasets, therefore
-all charts will load their data even if feature flag is turned on and no roles assigned
-to roles the access will fallback to **Dataset permissions**
+ - Granting a role access to a dashboard will bypass dataset level checks. Having dashboard access implicitly grants read access to all the featured charts in the dashboard, and thereby also all the associated datasets.
+ - If no roles are specified for a dashboard, regular **Dataset permissions** will apply.
<img src={useBaseUrl("/img/tutorial/tutorial_dashboard_access.png" )} />
diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
index 9b044519d4..6519852324 100644
--- a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
+++ b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
@@ -560,7 +560,7 @@ const PropertiesModal = ({
</StyledFormItem>
<p className="help-block">
{t(
- 'Roles is a list which defines access to the dashboard. Granting a role access to a dashboard will bypass dataset level checks. If no roles are defined, then the dashboard is available to all roles.',
+ 'Roles is a list which defines access to the dashboard. Granting a role access to a dashboard will bypass dataset level checks. If no roles are defined, regular access permissions apply.',
)}
</p>
</Col>
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 9e7a8bbd4b..754a91d907 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -2046,7 +2046,10 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
from superset.dashboards.commands.exceptions import DashboardAccessDeniedError
def has_rbac_access() -> bool:
- return (not is_feature_enabled("DASHBOARD_RBAC")) or any(
+ 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
@@ -2073,15 +2076,18 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
from superset.models.dashboard import Dashboard
from superset.models.slice import Slice
+ dashboard_ids = db.session.query(Dashboard.id)
+ dashboard_ids = DashboardAccessFilter(
+ "id", SQLAInterface(Dashboard, db.session)
+ ).apply(dashboard_ids, None)
+
datasource_class = type(datasource)
query = (
- db.session.query(datasource_class)
+ db.session.query(Dashboard.id)
+ .join(Slice, Dashboard.slices)
.join(Slice.table)
.filter(datasource_class.id == datasource.id)
- )
-
- query = DashboardAccessFilter("id", SQLAInterface(Dashboard, db.session)).apply(
- query, None
+ .filter(Dashboard.id.in_(dashboard_ids))
)
exists = db.session.query(query.exists()).scalar()
diff --git a/superset/utils/decorators.py b/superset/utils/decorators.py
index 7852c4ff74..e77a559905 100644
--- a/superset/utils/decorators.py
+++ b/superset/utils/decorators.py
@@ -116,9 +116,7 @@ def on_security_exception(self: Any, ex: Exception) -> Response:
# noinspection PyPackageRequirements
-def check_dashboard_access(
- on_error: Callable[..., Any] = on_security_exception
-) -> Callable[..., Any]:
+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:
@@ -130,7 +128,7 @@ def check_dashboard_access(
try:
current_app.appbuilder.sm.raise_for_dashboard_access(dashboard)
except DashboardAccessDeniedError as ex:
- return on_error(self, ex)
+ return on_error(str(ex))
except Exception as exception:
raise exception
diff --git a/superset/views/core.py b/superset/views/core.py
index 6559db1254..7576f042e8 100755
--- a/superset/views/core.py
+++ b/superset/views/core.py
@@ -66,6 +66,7 @@ from superset.connectors.sqla.models import (
TableColumn,
)
from superset.constants import QUERY_EARLY_CANCEL_KEY
+from superset.dashboards.commands.exceptions import DashboardAccessDeniedError
from superset.dashboards.commands.importers.v0 import ImportDashboardsCommand
from superset.dashboards.dao import DashboardDAO
from superset.dashboards.permalink.commands.get import GetDashboardPermalinkCommand
@@ -166,6 +167,7 @@ from superset.views.utils import (
get_form_data,
get_viz,
loads_request_json,
+ redirect_with_flash,
sanitize_datasource_data,
)
from superset.viz import BaseViz
@@ -191,6 +193,7 @@ 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 = __(
@@ -1825,9 +1828,7 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods
@expose("/dashboard/<dashboard_id_or_slug>/")
@event_logger.log_this_with_extra_payload
@check_dashboard_access(
- on_error=lambda self, ex: Response(
- utils.error_msg_from_exception(ex), status=403
- )
+ on_error=lambda msg: redirect_with_flash(DASHBOARD_LIST_URL, msg, "danger")
)
def dashboard(
self,
@@ -1845,25 +1846,33 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods
if not dashboard:
abort(404)
- if config["ENABLE_ACCESS_REQUEST"]:
- for datasource in dashboard.datasources:
- datasource = DatasourceDAO.get_datasource(
- datasource_type=DatasourceType(datasource.type),
- datasource_id=datasource.id,
- session=db.session(),
+ has_access_ = False
+ for datasource in dashboard.datasources:
+ datasource = DatasourceDAO.get_datasource(
+ datasource_type=DatasourceType(datasource.type),
+ datasource_id=datasource.id,
+ session=db.session(),
+ )
+ if datasource and security_manager.can_access_datasource(
+ datasource=datasource,
+ ):
+ has_access_ = True
+
+ if has_access_ is False and config["ENABLE_ACCESS_REQUEST"]:
+ flash(
+ __(security_manager.get_datasource_access_error_msg(datasource)),
+ "danger",
)
- if datasource and not security_manager.can_access_datasource(
- datasource=datasource
- ):
- flash(
- __(
- security_manager.get_datasource_access_error_msg(datasource)
- ),
- "danger",
- )
- return redirect(
- f"/superset/request_access/?dashboard_id={dashboard.id}"
- )
+ return redirect(
+ f"/superset/request_access/?dashboard_id={dashboard.id}"
+ )
+
+ 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
diff --git a/superset/views/dashboard/mixin.py b/superset/views/dashboard/mixin.py
index 9fcee061a6..eb5416e0c5 100644
--- a/superset/views/dashboard/mixin.py
+++ b/superset/views/dashboard/mixin.py
@@ -65,7 +65,7 @@ class DashboardMixin: # pylint: disable=too-few-public-methods
"roles": _(
"Roles is a list which defines access to the dashboard. "
"Granting a role access to a dashboard will bypass dataset level checks."
- "If no roles are defined then the dashboard is available to all roles."
+ "If no roles are defined, regular access permissions apply."
),
"published": _(
"Determines whether or not this dashboard is "
diff --git a/superset/views/utils.py b/superset/views/utils.py
index 835c8eda0d..cd84d8e0a5 100644
--- a/superset/views/utils.py
+++ b/superset/views/utils.py
@@ -23,11 +23,12 @@ from urllib import parse
import msgpack
import pyarrow as pa
import simplejson as json
-from flask import g, has_request_context, request
+from flask import flash, g, has_request_context, redirect, request
from flask_appbuilder.security.sqla import models as ab_models
from flask_appbuilder.security.sqla.models import User
from flask_babel import _
from sqlalchemy.orm.exc import NoResultFound
+from werkzeug.wrappers.response import Response
import superset.models.core as models
from superset import app, dataframe, db, result_set, viz
@@ -592,3 +593,8 @@ def get_cta_schema_name(
if not func:
return None
return func(database, user, schema, sql)
+
+
+def redirect_with_flash(url: str, message: str, category: str) -> Response:
+ flash(message=message, category=category)
+ return redirect(url)
diff --git a/tests/integration_tests/dashboards/security/security_rbac_tests.py b/tests/integration_tests/dashboards/security/security_rbac_tests.py
index d425c0e711..a49e533f1b 100644
--- a/tests/integration_tests/dashboards/security/security_rbac_tests.py
+++ b/tests/integration_tests/dashboards/security/security_rbac_tests.py
@@ -91,13 +91,11 @@ class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity):
# act
response = self.get_dashboard_view_response(dashboard_to_access)
+ assert response.status_code == 302
request_payload = get_query_context("birth_names")
rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data")
- self.assertEqual(rv.status_code, 403)
-
- # assert
- self.assert403(response)
+ assert rv.status_code == 403
def test_get_dashboard_view__user_with_dashboard_permission_can_not_access_draft(
self,
@@ -114,11 +112,57 @@ class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity):
response = self.get_dashboard_view_response(dashboard_to_access)
# assert
- self.assert403(response)
+ assert response.status_code == 302
# post
revoke_access_to_dashboard(dashboard_to_access, new_role)
+ @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+ def test_get_dashboard_view__user_no_access_regular_rbac(self):
+ if backend() == "hive":
+ return
+
+ slice = (
+ db.session.query(Slice)
+ .filter_by(slice_name="Girl Name Cloud")
+ .one_or_none()
+ )
+ dashboard = create_dashboard_to_db(published=True, slices=[slice])
+ self.login("gamma")
+
+ # assert redirect on regular rbac access denied
+ response = self.get_dashboard_view_response(dashboard)
+ assert response.status_code == 302
+
+ request_payload = get_query_context("birth_names")
+ rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data")
+ assert rv.status_code == 403
+ db.session.delete(dashboard)
+ db.session.commit()
+
+ @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+ def test_get_dashboard_view__user_access_regular_rbac(self):
+ if backend() == "hive":
+ return
+
+ slice = (
+ db.session.query(Slice)
+ .filter_by(slice_name="Girl Name Cloud")
+ .one_or_none()
+ )
+ dashboard = create_dashboard_to_db(published=True, slices=[slice])
+ self.login("gamma_sqllab")
+
+ response = self.get_dashboard_view_response(dashboard)
+
+ assert response.status_code == 200
+
+ request_payload = get_query_context("birth_names")
+ rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data")
+ assert rv.status_code == 200
+ db.session.delete(dashboard)
+ db.session.commit()
+
@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
def test_get_dashboard_view__user_access_with_dashboard_permission(self):
if backend() == "hive":
@@ -155,13 +199,14 @@ class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity):
@pytest.mark.usefixtures("public_role_like_gamma")
def test_get_dashboard_view__public_user_can_not_access_without_permission(self):
dashboard_to_access = create_dashboard_to_db(published=True)
+ grant_access_to_dashboard(dashboard_to_access, "Alpha")
self.logout()
# act
response = self.get_dashboard_view_response(dashboard_to_access)
# assert
- self.assert403(response)
+ assert response.status_code == 302
@pytest.mark.usefixtures("public_role_like_gamma")
def test_get_dashboard_view__public_user_with_dashboard_permission_can_not_access_draft(
@@ -175,7 +220,7 @@ class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity):
response = self.get_dashboard_view_response(dashboard_to_access)
# assert
- self.assert403(response)
+ assert response.status_code == 302
# post
revoke_access_to_dashboard(dashboard_to_access, "Public")