mirror of https://github.com/apache/superset.git
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 <33317356+villebro@users.noreply.github.com> * 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 <33317356+villebro@users.noreply.github.com>
This commit is contained in:
Amit Miran
GitHub
parent
15ac075b78
commit
8c5b6b1263
|
|
@ -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>
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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": _(
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue