This is an automated email from the ASF dual-hosted git repository.
amitmiran 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 8c5b6b1 feat(dashboard_rbac): provide data access based on dashboard
access (#13992)
8c5b6b1 is described below
commit 8c5b6b12632380a004e5a3b4c1e5716915c1ae9c
Author: Amit Miran <[email protected]>
AuthorDate: Tue Apr 13 16:23:31 2021 +0300
feat(dashboard_rbac): provide data access based on dashboard access (#13992)
* feat: provide data access based onb dashboard access
* chore: adjust code after CR comments
* fix: add brackets
* fix: type
* chore: add tests
* fix: pre-commit
* fix: pre-commit and lint
* fix: fix test
* fix: pre-commit
* fix: fix local pylint warnings
* revert: birth_names pylint change bc it affects tests
* Update superset/security/manager.py
Co-authored-by: Ville Brofeldt <[email protected]>
* Update superset/security/manager.py
* Update tests/utils_tests.py
* fix: after CR
* fix: after CR from ville
* chore: update roles description
Co-authored-by: Ville Brofeldt <[email protected]>
---
.../dashboard/components/PropertiesModal/index.jsx | 2 +-
superset/security/manager.py | 37 +++++++++++++++++++---
superset/views/dashboard/mixin.py | 3 +-
tests/dashboards/security/security_rbac_tests.py | 28 ++++++++++++++--
4 files changed, 60 insertions(+), 10 deletions(-)
diff --git
a/superset-frontend/src/dashboard/components/PropertiesModal/index.jsx
b/superset-frontend/src/dashboard/components/PropertiesModal/index.jsx
index e58c715..c1114e7 100644
--- a/superset-frontend/src/dashboard/components/PropertiesModal/index.jsx
+++ b/superset-frontend/src/dashboard/components/PropertiesModal/index.jsx
@@ -399,7 +399,7 @@ class PropertiesModal extends React.PureComponent {
/>
<p className="help-block">
{t(
- 'Roles is a list which defines access to the dashboard. These
roles are always applied in addition to restrictions on dataset level access.
If no roles 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 defined then the dashboard is available to all roles.',
)}
</p>
</Col>
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 6472af6..7a62e1a 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -22,6 +22,7 @@ from typing import Any, Callable, cast, List, Optional, Set,
Tuple, TYPE_CHECKIN
from flask import current_app, g
from flask_appbuilder import Model
+from flask_appbuilder.models.sqla.interface import SQLAInterface
from flask_appbuilder.security.sqla.manager import SecurityManager
from flask_appbuilder.security.sqla.models import (
assoc_permissionview_role,
@@ -61,7 +62,6 @@ if TYPE_CHECKING:
from superset.sql_parse import Table
from superset.viz import BaseViz
-
logger = logging.getLogger(__name__)
@@ -925,7 +925,9 @@ class SupersetSecurityManager( # pylint:
disable=too-many-public-methods
)
)
- def raise_for_access( # pylint:
disable=too-many-arguments,too-many-branches
+ def raise_for_access(
+ # pylint: disable=too-many-arguments,too-many-branches,
+ # pylint: disable=too-many-locals
self,
database: Optional["Database"] = None,
datasource: Optional["BaseDatasource"] = None,
@@ -996,9 +998,15 @@ class SupersetSecurityManager( # pylint:
disable=too-many-public-methods
assert datasource
+ from superset.extensions import feature_flag_manager
+
if not (
self.can_access_schema(datasource)
or self.can_access("datasource_access", datasource.perm or "")
+ or (
+ feature_flag_manager.is_feature_enabled("DASHBOARD_RBAC")
+ and self.can_access_based_on_dashboard(datasource)
+ )
):
raise SupersetSecurityException(
self.get_datasource_access_error_object(datasource)
@@ -1101,8 +1109,8 @@ class SupersetSecurityManager( # pylint:
disable=too-many-public-methods
ids.sort() # Combinations rather than permutations
return ids
- # pylint: disable=no-self-use
- def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None:
+ @staticmethod
+ def raise_for_dashboard_access(dashboard: "Dashboard") -> None:
"""
Raise an exception if the user cannot access the dashboard.
@@ -1127,3 +1135,24 @@ class SupersetSecurityManager( # pylint:
disable=too-many-public-methods
if not can_access:
raise DashboardAccessDeniedError()
+
+ @staticmethod
+ def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool:
+ from superset import db
+ from superset.dashboards.filters import DashboardFilter
+ from superset.models.slice import Slice
+ from superset.models.dashboard import Dashboard
+
+ datasource_class = type(datasource)
+ query = (
+ db.session.query(datasource_class)
+ .join(Slice.table)
+ .filter(datasource_class.id == datasource.id)
+ )
+
+ query = DashboardFilter("id", SQLAInterface(Dashboard,
db.session)).apply(
+ query, None
+ )
+
+ exists = db.session.query(query.exists()).scalar()
+ return exists
diff --git a/superset/views/dashboard/mixin.py
b/superset/views/dashboard/mixin.py
index 273cfdf..9a55107 100644
--- a/superset/views/dashboard/mixin.py
+++ b/superset/views/dashboard/mixin.py
@@ -65,8 +65,7 @@ class DashboardMixin: # pylint:
disable=too-few-public-methods
"owners": _("Owners is a list of users who can alter the dashboard."),
"roles": _(
"Roles is a list which defines access to the dashboard. "
- "These roles are always applied in addition to restrictions on
dataset "
- "level access. "
+ "Granting a role access to a dashboard will bypass dataset level
checks."
"If no roles defined then the dashboard is available to all roles."
),
"published": _(
diff --git a/tests/dashboards/security/security_rbac_tests.py
b/tests/dashboards/security/security_rbac_tests.py
index 19885d9..42cab6c 100644
--- a/tests/dashboards/security/security_rbac_tests.py
+++ b/tests/dashboards/security/security_rbac_tests.py
@@ -27,7 +27,11 @@ from tests.dashboards.superset_factory_util import (
create_datasource_table_to_db,
create_slice_to_db,
)
+from tests.fixtures.birth_names_dashboard import
load_birth_names_dashboard_with_slices
from tests.fixtures.public_role import public_role_like_gamma
+from tests.fixtures.query_context import get_query_context
+
+CHART_DATA_URI = "api/v1/chart/data"
@mock.patch.dict(
@@ -65,16 +69,26 @@ class
TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity):
# assert
self.assert_dashboard_view_response(response, dashboard_to_access)
+ @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
def test_get_dashboard_view__user_can_not_access_without_permission(self):
username = random_str()
new_role = f"role_{random_str()}"
self.create_user_with_roles(username, [new_role],
should_create_roles=True)
- dashboard_to_access = create_dashboard_to_db(published=True)
+ slice = (
+ db.session.query(Slice)
+ .filter_by(slice_name="Girl Name Cloud")
+ .one_or_none()
+ )
+ dashboard_to_access = create_dashboard_to_db(published=True,
slices=[slice])
self.login(username)
# act
response = self.get_dashboard_view_response(dashboard_to_access)
+ request_payload = get_query_context("birth_names")
+ rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data")
+ self.assertEqual(rv.status_code, 401)
+
# assert
self.assert403(response)
@@ -98,6 +112,7 @@ class
TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity):
# 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_access_with_dashboard_permission(self):
# arrange
@@ -105,9 +120,12 @@ class
TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity):
new_role = f"role_{random_str()}"
self.create_user_with_roles(username, [new_role],
should_create_roles=True)
- dashboard_to_access = create_dashboard_to_db(
- published=True, slices=[create_slice_to_db()]
+ slice = (
+ db.session.query(Slice)
+ .filter_by(slice_name="Girl Name Cloud")
+ .one_or_none()
)
+ dashboard_to_access = create_dashboard_to_db(published=True,
slices=[slice])
self.login(username)
grant_access_to_dashboard(dashboard_to_access, new_role)
@@ -117,6 +135,10 @@ class
TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity):
# assert
self.assert_dashboard_view_response(response, dashboard_to_access)
+ request_payload = get_query_context("birth_names")
+ rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data")
+ self.assertEqual(rv.status_code, 200)
+
# post
revoke_access_to_dashboard(dashboard_to_access, new_role)
