Update DASHBOARD_RBAC feature to require permission to edit dashboard…

…'s roles (#40)

superset-frontend/src/dashboard/components/Header/index.jsx

@@ -693,6 +693,7 @@ class Header extends React.PureComponent {
             colorScheme={this.props.colorScheme}
             onSubmit={handleOnPropertiesChange}
             onlyApply
+            user={user}
           />
         )}
 

superset-frontend/src/dashboard/components/PropertiesModal/PropertiesModal.test.tsx

@@ -22,6 +22,7 @@ import fetchMock from 'fetch-mock';
 import userEvent from '@testing-library/user-event';
 import * as ColorSchemeControlWrapper from 'src/dashboard/components/ColorSchemeControlWrapper';
 import * as SupersetCore from '@superset-ui/core';
+import { getMockStoreWithNativeFilters } from 'spec/fixtures/mockStore';
 import PropertiesModal from '.';
 
 const spyIsFeatureEnabled = jest.spyOn(SupersetCore, 'isFeatureEnabled');
@@ -129,6 +130,7 @@ fetchMock.get('glob:*/api/v1/dashboard/26', {
     result: { ...dashboardInfo, json_metadata: mockedJsonMetadata },
   },
 });
+const mockUser = getMockStoreWithNativeFilters().getState().user;
 
 const createProps = () => ({
   certified_by: 'John Doe',
@@ -140,6 +142,13 @@ const createProps = () => ({
   onHide: jest.fn(),
   onSubmit: jest.fn(),
   addSuccessToast: jest.fn(),
+  user: {
+    ...mockUser,
+    roles: {
+      ...mockUser.roles,
+      dashboard_role_editor: [['can_edit', 'PinterestDashboardRoles']],
+    },
+  },
 });
 
 beforeEach(() => {
@@ -455,3 +464,16 @@ test('should show active owners without dashboard rbac', async () => {
   expect(options).toHaveLength(1);
   expect(options[0]).toHaveTextContent('Superset Admin');
 });
+
+test('should not show roles without dashboard rbac editor permissions', async () => {
+  spyIsFeatureEnabled.mockReturnValue(true);
+
+  const props = createProps();
+  const propsWithDashboardInfo = { ...props, user: {}, dashboardInfo };
+
+  render(<PropertiesModal {...propsWithDashboardInfo} />, {
+    useRedux: true,
+  });
+
+  expect(screen.queryByText('Roles')).not.toBeInTheDocument();
+});

superset-frontend/src/dashboard/components/PropertiesModal/index.tsx

@@ -44,13 +44,18 @@ import FilterScopeModal from 'src/dashboard/components/filterscope/FilterScopeMo
 import { getClientErrorObject } from 'src/utils/getClientErrorObject';
 import withToasts from 'src/components/MessageToasts/withToasts';
 import TagType from 'src/types/TagType';
+import { userHasPermission } from 'src/dashboard/util/permissionUtils';
 import {
   addTag,
   deleteTaggedObjects,
   fetchTags,
   OBJECT_TYPES,
 } from 'src/features/tags/tags';
 import { loadTags } from 'src/components/Tags/utils';
+import {
+  UndefinedUser,
+  UserWithPermissionsAndRoles,
+} from 'src/types/bootstrapTypes';
 
 const StyledFormItem = styled(FormItem)`
   margin-bottom: 0;
@@ -72,6 +77,7 @@ type PropertiesModalProps = {
   addSuccessToast: (message: string) => void;
   addDangerToast: (message: string) => void;
   onlyApply?: boolean;
+  user: UserWithPermissionsAndRoles | UndefinedUser;
 };
 
 type Roles = { id: number; name: string }[];
@@ -97,6 +103,7 @@ const PropertiesModal = ({
   dashboardId,
   dashboardInfo: currentDashboardInfo,
   dashboardTitle,
+  user,
   onHide = () => {},
   onlyApply = false,
   onSubmit = () => {},
@@ -113,6 +120,11 @@ const PropertiesModal = ({
   const saveLabel = onlyApply ? t('Apply') : t('Save');
   const [tags, setTags] = useState<TagType[]>([]);
   const categoricalSchemeRegistry = getCategoricalSchemeRegistry();
+  const canAccessRoles = userHasPermission(
+    user,
+    'PinterestDashboardRoles',
+    'can_edit',
+  );
 
   const tagsAsSelectValues = useMemo(() => {
     const selectTags = tags.map(tag => ({
@@ -698,7 +710,7 @@ const PropertiesModal = ({
             </p>
           </Col>
         </Row>
-        {isFeatureEnabled(FeatureFlag.DashboardRbac)
+        {isFeatureEnabled(FeatureFlag.DashboardRbac) && canAccessRoles
           ? getRowsWithRoles()
           : getRowsWithoutRoles()}
         <Row>

superset-frontend/src/features/home/DashboardTable.tsx

@@ -235,6 +235,7 @@ function DashboardTable({
           show
           onHide={() => setEditModal(undefined)}
           onSubmit={handleDashboardEdit}
+          user={user}
         />
       )}
       {dashboardToDelete && (

superset-frontend/src/pages/DashboardList/index.tsx

@@ -708,6 +708,7 @@ function DashboardList(props: DashboardListProps) {
                   show
                   onHide={() => setDashboardToEdit(null)}
                   onSubmit={handleDashboardEdit}
+                  user={user}
                 />
               )}
               {dashboardToDelete && (

superset/commands/dashboard/create.py

@@ -20,9 +20,11 @@
 from flask_appbuilder.models.sqla import Model
 from marshmallow import ValidationError
 
+from superset import security_manager
 from superset.commands.base import BaseCommand, CreateMixin
 from superset.commands.dashboard.exceptions import (
     DashboardCreateFailedError,
+    DashboardForbiddenError,
     DashboardInvalidError,
     DashboardSlugExistsValidationError,
 )
@@ -64,6 +66,17 @@ def validate(self) -> None:
         if exceptions:
             raise DashboardInvalidError(exceptions=exceptions)
 
+        if (
+            role_ids is not None
+            and len(role_ids)
+            and not security_manager.is_admin()
+            and not security_manager.can_access(
+                "can_edit",
+                "PinterestDashboardRoles",
+            )
+        ):
+            raise DashboardForbiddenError("User does not have access to edit roles")
+
         try:
             roles = populate_roles(role_ids)
             self._properties["roles"] = roles

superset/commands/dashboard/update.py

@@ -96,9 +96,17 @@ def validate(self) -> None:
         if exceptions:
             raise DashboardInvalidError(exceptions=exceptions)
 
-        # Validate/Populate role
+        # Validate whether user can edit dashboard's roles
+        model_roles_ids = [role.id for role in self._model.roles]
+        if roles_ids is not None and set(model_roles_ids) != set(roles_ids):
+            if not security_manager.is_admin() and not security_manager.can_access(
+                "can_edit",
+                "PinterestDashboardRoles",
+            ):
+                raise DashboardForbiddenError("User does not have access to edit roles")
+
         if roles_ids is None:
-            roles_ids = [role.id for role in self._model.roles]
+            roles_ids = model_roles_ids
         try:
             roles = populate_roles(roles_ids)
             self._properties["roles"] = roles

superset/dashboards/api.py

@@ -503,6 +503,8 @@ def post(self) -> Response:
                 exc_info=True,
             )
             return self.response_422(message=str(ex))
+        except DashboardForbiddenError:
+            return self.response_403()
 
     @expose("/<pk>", methods=("PUT",))
     @protect()

tests/integration_tests/pinterest/dashboard_rbac_tests.py

@@ -0,0 +1,185 @@
+import json
+
+import pytest
+from flask_appbuilder.security.sqla.models import (
+    assoc_permissionview_role,
+    PermissionView,
+    Role,
+)
+
+from superset import db, security_manager
+from superset.models.dashboard import Dashboard
+from tests.integration_tests.base_tests import SupersetTestCase
+from tests.integration_tests.conftest import with_feature_flags
+from tests.integration_tests.fixtures.birth_names_dashboard import (
+    load_birth_names_dashboard_with_slices,
+    load_birth_names_data,
+)
+from tests.integration_tests.utils.get_dashboards import get_dashboards_ids
+
+TEST_RBAC_EDITOR_ROLE = "rbac_editor"
+RBAC_EDITOR_PERMISSION = "can_edit"
+RBAC_EDITOR_VIEW_MENU_NAME = "PinterestDashboardRoles"
+
+RBAC_EDITOR_USERNAME = "rbac_editor_user"
+
+
+class TestPinterestDashboardRBAC(SupersetTestCase):
+    def setUp(self):
+        # Create rbac editor role that has permission to edit a dashboard's roles
+        security_manager.copy_role("Alpha", TEST_RBAC_EDITOR_ROLE, merge=False)
+        security_manager.add_permission_view_menu(
+            RBAC_EDITOR_PERMISSION, RBAC_EDITOR_VIEW_MENU_NAME
+        )
+        perm_view = security_manager.find_permission_view_menu(
+            RBAC_EDITOR_PERMISSION, RBAC_EDITOR_VIEW_MENU_NAME
+        )
+        security_manager.add_permission_role(
+            security_manager.find_role(TEST_RBAC_EDITOR_ROLE), perm_view
+        )
+
+        # Create rbac editor user
+        self.create_user(
+            RBAC_EDITOR_USERNAME,
+            "general",
+            TEST_RBAC_EDITOR_ROLE,
+            email=f"{RBAC_EDITOR_USERNAME}@fab.org",
+        )
+        db.session.commit()
+
+    def tearDown(self):
+        # Remove rbac editor user and role
+        pvs = (
+            db.session.query(PermissionView)
+            .join(assoc_permissionview_role)
+            .join(Role)
+            .filter(Role.id == security_manager.find_role(TEST_RBAC_EDITOR_ROLE).id)
+            .all()
+        )
+        for pv in pvs:
+            security_manager.del_permission_role(
+                security_manager.find_role(TEST_RBAC_EDITOR_ROLE), pv
+            )
+        pv = security_manager.find_permission_view_menu(
+            RBAC_EDITOR_PERMISSION, RBAC_EDITOR_VIEW_MENU_NAME
+        )
+        for role in ["Admin", "Alpha", "Gamma"]:
+            security_manager.del_permission_role(security_manager.find_role(role), pv)
+        db.session.delete(security_manager.find_role(TEST_RBAC_EDITOR_ROLE))
+        security_manager.del_permission_view_menu(
+            RBAC_EDITOR_PERMISSION, RBAC_EDITOR_VIEW_MENU_NAME
+        )
+        db.session.delete(security_manager.find_user(RBAC_EDITOR_USERNAME))
+        db.session.commit()
+
+    def _add_user_to_dashboard(self, dashboard_id: int, username: str):
+        dashboard_id = get_dashboards_ids(["births"])[0]
+        dashboard = Dashboard.get(dashboard_id)
+        dashboard.owners.append(security_manager.find_user(username))
+        db.session.commit()
+
+    @with_feature_flags(DASHBOARD_RBAC=True)
+    @pytest.mark.usefixtures(
+        "load_birth_names_dashboard_with_slices",
+    )
+    def test_non_rbac_editor_editing_dashboard_roles(
+        self,
+    ):
+        """Test that Alpha users (or users without can_edit PinterestDashboardRoles permission)
+        cannot edit dashboard roles"""
+        dashboard_id = get_dashboards_ids(["births"])[0]
+        self._add_user_to_dashboard(dashboard_id, "alpha")
+
+        self.login(username="alpha")
+
+        # Attempt to edit dashboard roles
+        dashboard_data = {
+            "roles": [5],
+        }
+        rv = self.client.put(f"api/v1/dashboard/{dashboard_id}", json=dashboard_data)
+        assert rv.status_code == 403
+        self.logout()
+
+    @with_feature_flags(DASHBOARD_RBAC=True)
+    @pytest.mark.usefixtures(
+        "load_birth_names_dashboard_with_slices",
+    )
+    def test_non_rbac_editor_editing_dashboard_properties(
+        self,
+    ):
+        """Test that Alpha users (or users without can_edit PinterestDashboardRoles permission)
+        can edit dashboard properties (that are not roles)"""
+        dashboard_id = get_dashboards_ids(["births"])[0]
+        self._add_user_to_dashboard(dashboard_id, "alpha")
+
+        self.login(username="alpha")
+
+        # Attempt to edit dashboard property
+        dashboard_data = {
+            "dashboard_title": "new title",
+        }
+        rv = self.client.put(f"api/v1/dashboard/{dashboard_id}", json=dashboard_data)
+        assert rv.status_code == 200
+        self.logout()
+
+    @with_feature_flags(DASHBOARD_RBAC=True)
+    @pytest.mark.usefixtures(
+        "load_birth_names_dashboard_with_slices",
+    )
+    def test_rbac_editor_editing_dashboard_roles(
+        self,
+    ):
+        """Test that users with can_edit PinterestDashboardRoles permission"""
+        dashboard_id = get_dashboards_ids(["births"])[0]
+        self._add_user_to_dashboard(dashboard_id, RBAC_EDITOR_USERNAME)
+
+        self.login(username=RBAC_EDITOR_USERNAME)
+
+        dashboard_data = {
+            "roles": [4],
+        }
+
+        rv = self.client.put(f"api/v1/dashboard/{dashboard_id}", json=dashboard_data)
+        assert rv.status_code == 200
+        self.logout()
+
+    @with_feature_flags(DASHBOARD_RBAC=True)
+    def test_non_rbac_editor_create_dashboard_with_roles(self):
+        alpha_id = self.get_user("alpha").id
+        dashboard_data = {
+            "dashboard_title": "title1",
+            "slug": "slug2",
+            "owners": [alpha_id],
+            "position_json": '{"a": "A"}',
+            "css": "css",
+            "json_metadata": '{"refresh_frequency": 30}',
+            "published": True,
+            "roles": [4],
+        }
+        self.login(username="alpha")
+        uri = "api/v1/dashboard/"
+        rv = self.client.post(uri, json=dashboard_data)
+        assert rv.status_code == 403
+        self.logout()
+
+    @with_feature_flags(DASHBOARD_RBAC=True)
+    def test_non_rbac_editor_create_dashboard_without_roles(self):
+        alpha_id = self.get_user("alpha").id
+        dashboard_data = {
+            "dashboard_title": "title1",
+            "slug": "slug2",
+            "owners": [alpha_id],
+            "position_json": '{"a": "A"}',
+            "css": "css",
+            "json_metadata": '{"refresh_frequency": 30}',
+            "published": True,
+        }
+        self.login(username="alpha")
+        uri = "api/v1/dashboard/"
+        rv = self.client.post(uri, json=dashboard_data)
+        assert rv.status_code == 201
+        self.logout()
+        data = json.loads(rv.data.decode("utf-8"))
+        model = db.session.query(Dashboard).get(data.get("id"))
+        db.session.delete(model)
+        db.session.commit()