From 8f208e1833f9794937f3db1d6905c03b6bf8bcb2 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Mon, 1 Feb 2021 15:08:14 +0200 Subject: [PATCH 01/11] feat: dashboards API support for roles create/update + roles validation --- superset/commands/exceptions.py | 7 ++++++ superset/commands/utils.py | 19 +++++++++++++++- superset/dashboards/api.py | 6 ++++++ superset/dashboards/commands/create.py | 17 ++++++++++++--- superset/dashboards/commands/update.py | 24 ++++++++++++++++----- superset/dashboards/schemas.py | 8 +++++++ superset/security/manager.py | 18 +++++++++++++++- tests/base_tests.py | 9 ++++++++ tests/dashboards/api_tests.py | 30 ++++++++++++++++++++++++-- 9 files changed, 126 insertions(+), 12 deletions(-) diff --git a/superset/commands/exceptions.py b/superset/commands/exceptions.py index fdc7bee973e4f..6c36d54171e1d 100644 --- a/superset/commands/exceptions.py +++ b/superset/commands/exceptions.py @@ -85,6 +85,13 @@ def __init__(self) -> None: super().__init__([_("Owners are invalid")], field_name="owners") +class RolesNotFoundValidationError(ValidationError): + status = 422 + + def __init__(self) -> None: + super().__init__([_("Roles are invalid")], field_name="roles") + + class DatasourceNotFoundValidationError(ValidationError): status = 404 diff --git a/superset/commands/utils.py b/superset/commands/utils.py index 874ea4bc5fb93..eecd99607c256 100644 --- a/superset/commands/utils.py +++ b/superset/commands/utils.py @@ -16,11 +16,12 @@ # under the License. from typing import List, Optional -from flask_appbuilder.security.sqla.models import User +from flask_appbuilder.security.sqla.models import Role, User from superset.commands.exceptions import ( DatasourceNotFoundValidationError, OwnersNotFoundValidationError, + RolesNotFoundValidationError, ) from superset.connectors.base.models import BaseDatasource from superset.connectors.connector_registry import ConnectorRegistry @@ -48,6 +49,22 @@ def populate_owners(user: User, owners_ids: Optional[List[int]] = None) -> List[ return owners +def populate_roles(roles_ids: Optional[List[int]] = None) -> List[Role]: + """ + Helper function for commands, will fetch all roles from roles id's + Can raise ValidationError + :param roles_ids: A List of roles by id's + """ + roles = list() + if roles_ids: + for role_id in roles_ids: + role = security_manager.find_role_by_id(role_id) + if not role: + raise RolesNotFoundValidationError() + roles.append(role) + return roles + + def get_datasource_by_id(datasource_id: int, datasource_type: str) -> BaseDatasource: try: return ConnectorRegistry.get_datasource( diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index 61b56b1de1c6d..dbde513d3a6ea 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -106,6 +106,8 @@ class DashboardRestApi(BaseSupersetModelRestApi): "owners.username", "owners.first_name", "owners.last_name", + "roles.id", + "roles.name", "changed_by_name", "changed_by_url", "changed_by.username", @@ -142,6 +144,8 @@ class DashboardRestApi(BaseSupersetModelRestApi): "owners.username", "owners.first_name", "owners.last_name", + "roles.id", + "roles.name", ] list_select_columns = list_columns + ["changed_on", "changed_by_fk"] order_columns = [ @@ -156,6 +160,7 @@ class DashboardRestApi(BaseSupersetModelRestApi): "dashboard_title", "slug", "owners", + "roles", "position_json", "css", "json_metadata", @@ -168,6 +173,7 @@ class DashboardRestApi(BaseSupersetModelRestApi): "dashboard_title", "id", "owners", + "roles", "published", "slug", "changed_by", diff --git a/superset/dashboards/commands/create.py b/superset/dashboards/commands/create.py index dff25d6fcc4a5..133a520887049 100644 --- a/superset/dashboards/commands/create.py +++ b/superset/dashboards/commands/create.py @@ -22,7 +22,7 @@ from marshmallow import ValidationError from superset.commands.base import BaseCommand -from superset.commands.utils import populate_owners +from superset.commands.utils import populate_owners, populate_roles from superset.dao.exceptions import DAOCreateFailedError from superset.dashboards.commands.exceptions import ( DashboardCreateFailedError, @@ -51,7 +51,8 @@ def run(self) -> Model: def validate(self) -> None: exceptions: List[ValidationError] = list() - owner_ids: Optional[List[int]] = self._properties.get("owners") + owners_ids: Optional[List[int]] = self._properties.get("owners") + roles_ids: Optional[List[int]] = self._properties.get("roles") slug: str = self._properties.get("slug", "") # Validate slug uniqueness @@ -59,7 +60,7 @@ def validate(self) -> None: exceptions.append(DashboardSlugExistsValidationError()) try: - owners = populate_owners(self._actor, owner_ids) + owners = populate_owners(self._actor, owners_ids) self._properties["owners"] = owners except ValidationError as ex: exceptions.append(ex) @@ -67,3 +68,13 @@ def validate(self) -> None: exception = DashboardInvalidError() exception.add_list(exceptions) raise exception + + try: + roles = populate_roles(roles_ids) + self._properties["roles"] = roles + except ValidationError as ex: + exceptions.append(ex) + if exceptions: + exception = DashboardInvalidError() + exception.add_list(exceptions) + raise exception diff --git a/superset/dashboards/commands/update.py b/superset/dashboards/commands/update.py index 54c5de17a4c02..9c885c635abb3 100644 --- a/superset/dashboards/commands/update.py +++ b/superset/dashboards/commands/update.py @@ -22,7 +22,7 @@ from marshmallow import ValidationError from superset.commands.base import BaseCommand -from superset.commands.utils import populate_owners +from superset.commands.utils import populate_owners, populate_roles from superset.dao.exceptions import DAOUpdateFailedError from superset.dashboards.commands.exceptions import ( DashboardForbiddenError, @@ -58,7 +58,8 @@ def run(self) -> Model: def validate(self) -> None: exceptions: List[ValidationError] = [] - owner_ids: Optional[List[int]] = self._properties.get("owners") + owners_ids: Optional[List[int]] = self._properties.get("owners") + roles_ids: Optional[List[int]] = self._properties.get("roles") slug: Optional[str] = self._properties.get("slug") # Validate/populate model exists @@ -76,10 +77,10 @@ def validate(self) -> None: exceptions.append(DashboardSlugExistsValidationError()) # Validate/Populate owner - if owner_ids is None: - owner_ids = [owner.id for owner in self._model.owners] + if owners_ids is None: + owners_ids = [owner.id for owner in self._model.owners] try: - owners = populate_owners(self._actor, owner_ids) + owners = populate_owners(self._actor, owners_ids) self._properties["owners"] = owners except ValidationError as ex: exceptions.append(ex) @@ -87,3 +88,16 @@ def validate(self) -> None: exception = DashboardInvalidError() exception.add_list(exceptions) raise exception + + # Validate/Populate role + if roles_ids is None: + roles_ids = [role.id for role in self._model.roles] + try: + roles = populate_roles(roles_ids) + self._properties["roles"] = roles + except ValidationError as ex: + exceptions.append(ex) + if exceptions: + exception = DashboardInvalidError() + exception.add_list(exceptions) + raise exception diff --git a/superset/dashboards/schemas.py b/superset/dashboards/schemas.py index e3c43136cdea4..17e46d0354f1f 100644 --- a/superset/dashboards/schemas.py +++ b/superset/dashboards/schemas.py @@ -38,6 +38,12 @@ "Owner are users ids allowed to delete or change this dashboard. " "If left empty you will be one of the owners of the dashboard." ) +roles_description = ( + "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." +) position_json_description = ( "This json object describes the positioning of the widgets " "in the dashboard. It is dynamically generated when " @@ -136,6 +142,7 @@ class DashboardPostSchema(BaseDashboardSchema): description=slug_description, allow_none=True, validate=[Length(1, 255)] ) owners = fields.List(fields.Integer(description=owners_description)) + roles = fields.List(fields.Integer(description=roles_description)) position_json = fields.String( description=position_json_description, validate=validate_json ) @@ -158,6 +165,7 @@ class DashboardPutSchema(BaseDashboardSchema): owners = fields.List( fields.Integer(description=owners_description, allow_none=True) ) + roles = fields.List(fields.Integer(description=roles_description, allow_none=True)) position_json = fields.String( description=position_json_description, allow_none=True, validate=validate_json ) diff --git a/superset/security/manager.py b/superset/security/manager.py index 43461f10fcfea..f3800e6cd3518 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -18,7 +18,17 @@ """A set of constants and methods to manage permissions and security""" import logging import re -from typing import Any, Callable, cast, List, Optional, Set, Tuple, TYPE_CHECKING, Union +from typing import ( + Any, + Callable, + cast, + List, + Optional, + Set, + Tuple, + TYPE_CHECKING, + Union, +) from flask import current_app, g from flask_appbuilder import Model @@ -59,6 +69,7 @@ from superset.sql_parse import Table from superset.viz import BaseViz + logger = logging.getLogger(__name__) @@ -656,6 +667,11 @@ def _get_pvms_from_builtin_role(self, role_name: str) -> List[PermissionView]: role_from_permissions.append(pvm) return role_from_permissions + def find_role_by_id(self, role_id: int) -> Any: + return ( + self.get_session.query(self.role_model).filter_by(id=role_id).one_or_none() + ) + def copy_role( self, role_from_name: str, role_to_name: str, merge: bool = True ) -> None: diff --git a/tests/base_tests.py b/tests/base_tests.py index a5fd9d5382da3..7a3866cfaed94 100644 --- a/tests/base_tests.py +++ b/tests/base_tests.py @@ -182,6 +182,15 @@ def get_user(username: str) -> ab_models.User: ) return user + @staticmethod + def get_role(name: str) -> ab_models.User: + user = ( + db.session.query(security_manager.role_model) + .filter_by(name=name) + .one_or_none() + ) + return user + @classmethod def create_druid_test_objects(cls): # create druid cluster and druid datasources diff --git a/tests/dashboards/api_tests.py b/tests/dashboards/api_tests.py index 38ce1aee9522b..41b34c2f3ac47 100644 --- a/tests/dashboards/api_tests.py +++ b/tests/dashboards/api_tests.py @@ -71,6 +71,7 @@ def insert_dashboard( dashboard_title: str, slug: Optional[str], owners: List[int], + roles: List[int] = [], created_by=None, slices: Optional[List[Slice]] = None, position_json: str = "", @@ -79,14 +80,19 @@ def insert_dashboard( published: bool = False, ) -> 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, @@ -151,7 +157,9 @@ def test_get_dashboard(self): Dashboard API: Test get dashboard """ admin = self.get_user("admin") - dashboard = self.insert_dashboard("title", "slug1", [admin.id], admin) + dashboard = self.insert_dashboard( + "title", "slug1", [admin.id], created_by=admin + ) self.login(username="admin") uri = f"api/v1/dashboard/{dashboard.id}" rv = self.get_assert_metric(uri, "get") @@ -174,6 +182,7 @@ def test_get_dashboard(self): "last_name": "user", } ], + "roles": [], "position_json": "", "published": False, "url": "/superset/dashboard/slug1/", @@ -863,6 +872,19 @@ def test_create_dashboard_validate_owners(self): expected_response = {"message": {"owners": ["Owners are invalid"]}} self.assertEqual(response, expected_response) + def test_create_dashboard_validate_roles(self): + """ + Dashboard API: Test create validate roles + """ + dashboard_data = {"dashboard_title": "title1", "roles": [1000]} + self.login(username="admin") + uri = "api/v1/dashboard/" + rv = self.client.post(uri, json=dashboard_data) + self.assertEqual(rv.status_code, 422) + response = json.loads(rv.data.decode("utf-8")) + expected_response = {"message": {"roles": ["Roles are invalid"]}} + self.assertEqual(response, expected_response) + def test_create_dashboard_validate_json(self): """ Dashboard API: Test create validate json @@ -893,7 +915,10 @@ def test_update_dashboard(self): Dashboard API: Test update """ admin = self.get_user("admin") - dashboard_id = self.insert_dashboard("title1", "slug1", [admin.id]).id + admin_role = self.get_role("Admin") + dashboard_id = self.insert_dashboard( + "title1", "slug1", [admin.id], roles=[admin_role.id] + ).id self.login(username="admin") uri = f"api/v1/dashboard/{dashboard_id}" rv = self.put_assert_metric(uri, self.dashboard_data, "put") @@ -906,6 +931,7 @@ def test_update_dashboard(self): self.assertEqual(model.json_metadata, self.dashboard_data["json_metadata"]) self.assertEqual(model.published, self.dashboard_data["published"]) self.assertEqual(model.owners, [admin]) + self.assertEqual(model.roles, [admin_role]) db.session.delete(model) db.session.commit() From 8bd211f88916df2165b23c5a47f94f519a4f12b7 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Mon, 1 Feb 2021 17:13:09 +0200 Subject: [PATCH 02/11] fix: pre-commit ;) --- superset/security/manager.py | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index f3800e6cd3518..462e94233b0a4 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -18,17 +18,7 @@ """A set of constants and methods to manage permissions and security""" import logging import re -from typing import ( - Any, - Callable, - cast, - List, - Optional, - Set, - Tuple, - TYPE_CHECKING, - Union, -) +from typing import Any, Callable, cast, List, Optional, Set, Tuple, TYPE_CHECKING, Union from flask import current_app, g from flask_appbuilder import Model From 6ec0285957dfa08307a4fc5f7ddbc19feed35976 Mon Sep 17 00:00:00 2001 From: Amit Miran <47772523+amitmiran137@users.noreply.github.com> Date: Mon, 1 Feb 2021 18:40:47 +0200 Subject: [PATCH 03/11] Update superset/commands/utils.py Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> --- superset/commands/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/commands/utils.py b/superset/commands/utils.py index eecd99607c256..d5e17a792d982 100644 --- a/superset/commands/utils.py +++ b/superset/commands/utils.py @@ -52,7 +52,7 @@ def populate_owners(user: User, owners_ids: Optional[List[int]] = None) -> List[ def populate_roles(roles_ids: Optional[List[int]] = None) -> List[Role]: """ Helper function for commands, will fetch all roles from roles id's - Can raise ValidationError + :raises RolesNotFoundValidationError: If a role in the input list is not found :param roles_ids: A List of roles by id's """ roles = list() From 05fe8560a156e50a70afde302592554b6ca7f21f Mon Sep 17 00:00:00 2001 From: Amit Miran <47772523+amitmiran137@users.noreply.github.com> Date: Mon, 1 Feb 2021 18:44:53 +0200 Subject: [PATCH 04/11] Update tests/base_tests.py Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> --- tests/base_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/base_tests.py b/tests/base_tests.py index 7a3866cfaed94..a55302d1c06d3 100644 --- a/tests/base_tests.py +++ b/tests/base_tests.py @@ -183,7 +183,7 @@ def get_user(username: str) -> ab_models.User: return user @staticmethod - def get_role(name: str) -> ab_models.User: + def get_role(name: str) -> Optional[ab_models.User]: user = ( db.session.query(security_manager.role_model) .filter_by(name=name) From 48718ea11022796a60237b7e110469ad2be5a3d9 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Mon, 1 Feb 2021 18:45:10 +0200 Subject: [PATCH 05/11] fix: After CR --- superset/commands/utils.py | 20 ++++++++++---------- superset/dashboards/commands/create.py | 8 ++++---- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/superset/commands/utils.py b/superset/commands/utils.py index d5e17a792d982..cdf24e69e6795 100644 --- a/superset/commands/utils.py +++ b/superset/commands/utils.py @@ -29,19 +29,19 @@ from superset.extensions import db, security_manager -def populate_owners(user: User, owners_ids: Optional[List[int]] = None) -> List[User]: +def populate_owners(user: User, owner_ids: Optional[List[int]] = None) -> List[User]: """ Helper function for commands, will fetch all users from owners id's Can raise ValidationError :param user: The current user - :param owners_ids: A List of owners by id's + :param owner_ids: A List of owners by id's """ owners = list() - if not owners_ids: + if not owner_ids: return [user] - if user.id not in owners_ids: + if user.id not in owner_ids: owners.append(user) - for owner_id in owners_ids: + for owner_id in owner_ids: owner = security_manager.get_user_by_id(owner_id) if not owner: raise OwnersNotFoundValidationError() @@ -49,15 +49,15 @@ def populate_owners(user: User, owners_ids: Optional[List[int]] = None) -> List[ return owners -def populate_roles(roles_ids: Optional[List[int]] = None) -> List[Role]: +def populate_roles(role_ids: Optional[List[int]] = None) -> List[Role]: """ Helper function for commands, will fetch all roles from roles id's - :raises RolesNotFoundValidationError: If a role in the input list is not found - :param roles_ids: A List of roles by id's + :raises RolesNotFoundValidationError: If a role in the input list is not found + :param role_ids: A List of roles by id's """ roles = list() - if roles_ids: - for role_id in roles_ids: + if role_ids: + for role_id in role_ids: role = security_manager.find_role_by_id(role_id) if not role: raise RolesNotFoundValidationError() diff --git a/superset/dashboards/commands/create.py b/superset/dashboards/commands/create.py index 133a520887049..e351d675af4ad 100644 --- a/superset/dashboards/commands/create.py +++ b/superset/dashboards/commands/create.py @@ -51,8 +51,8 @@ def run(self) -> Model: def validate(self) -> None: exceptions: List[ValidationError] = list() - owners_ids: Optional[List[int]] = self._properties.get("owners") - roles_ids: Optional[List[int]] = self._properties.get("roles") + owner_ids: Optional[List[int]] = self._properties.get("owners") + role_ids: Optional[List[int]] = self._properties.get("roles") slug: str = self._properties.get("slug", "") # Validate slug uniqueness @@ -60,7 +60,7 @@ def validate(self) -> None: exceptions.append(DashboardSlugExistsValidationError()) try: - owners = populate_owners(self._actor, owners_ids) + owners = populate_owners(self._actor, owner_ids) self._properties["owners"] = owners except ValidationError as ex: exceptions.append(ex) @@ -70,7 +70,7 @@ def validate(self) -> None: raise exception try: - roles = populate_roles(roles_ids) + roles = populate_roles(role_ids) self._properties["roles"] = roles except ValidationError as ex: exceptions.append(ex) From 29bd1e9fcdfe73216ff26ae2dee6f9aeee99d443 Mon Sep 17 00:00:00 2001 From: Amit Miran <47772523+amitmiran137@users.noreply.github.com> Date: Mon, 1 Feb 2021 23:46:42 +0200 Subject: [PATCH 06/11] Update superset/commands/exceptions.py Co-authored-by: Jesse Yang --- superset/commands/exceptions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/commands/exceptions.py b/superset/commands/exceptions.py index 6c36d54171e1d..bb8992aeb0e26 100644 --- a/superset/commands/exceptions.py +++ b/superset/commands/exceptions.py @@ -89,7 +89,7 @@ class RolesNotFoundValidationError(ValidationError): status = 422 def __init__(self) -> None: - super().__init__([_("Roles are invalid")], field_name="roles") + super().__init__([_("Some roles do not exist")], field_name="roles") class DatasourceNotFoundValidationError(ValidationError): From 988f706d07853bba7d431f00ac46269d831b82f5 Mon Sep 17 00:00:00 2001 From: Amit Miran <47772523+amitmiran137@users.noreply.github.com> Date: Tue, 2 Feb 2021 06:51:33 +0200 Subject: [PATCH 07/11] fix: adjust the test --- tests/dashboards/api_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/dashboards/api_tests.py b/tests/dashboards/api_tests.py index 41b34c2f3ac47..ea6708d4d77a0 100644 --- a/tests/dashboards/api_tests.py +++ b/tests/dashboards/api_tests.py @@ -882,7 +882,7 @@ def test_create_dashboard_validate_roles(self): rv = self.client.post(uri, json=dashboard_data) self.assertEqual(rv.status_code, 422) response = json.loads(rv.data.decode("utf-8")) - expected_response = {"message": {"roles": ["Roles are invalid"]}} + expected_response = {"message": {"roles": ["Some roles do not exis"]}} self.assertEqual(response, expected_response) def test_create_dashboard_validate_json(self): From 9400161bfa6aeaf0bbb612f586fd34230641c5b3 Mon Sep 17 00:00:00 2001 From: Amit Miran <47772523+amitmiran137@users.noreply.github.com> Date: Tue, 2 Feb 2021 09:04:57 +0200 Subject: [PATCH 08/11] fix: missing 't' :) --- tests/dashboards/api_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/dashboards/api_tests.py b/tests/dashboards/api_tests.py index ea6708d4d77a0..50239b7ee74c6 100644 --- a/tests/dashboards/api_tests.py +++ b/tests/dashboards/api_tests.py @@ -882,7 +882,7 @@ def test_create_dashboard_validate_roles(self): rv = self.client.post(uri, json=dashboard_data) self.assertEqual(rv.status_code, 422) response = json.loads(rv.data.decode("utf-8")) - expected_response = {"message": {"roles": ["Some roles do not exis"]}} + expected_response = {"message": {"roles": ["Some roles do not exist"]}} self.assertEqual(response, expected_response) def test_create_dashboard_validate_json(self): From c5b3680a2313d1cb824610c61ca1a9fb21c894b9 Mon Sep 17 00:00:00 2001 From: Amit Miran <47772523+amitmiran137@users.noreply.github.com> Date: Thu, 4 Feb 2021 10:49:35 +0200 Subject: [PATCH 09/11] Update superset/commands/utils.py Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> --- superset/commands/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/commands/utils.py b/superset/commands/utils.py index cdf24e69e6795..c70a9e18f2fda 100644 --- a/superset/commands/utils.py +++ b/superset/commands/utils.py @@ -55,7 +55,7 @@ def populate_roles(role_ids: Optional[List[int]] = None) -> List[Role]: :raises RolesNotFoundValidationError: If a role in the input list is not found :param role_ids: A List of roles by id's """ - roles = list() + roles: List[Role] = [] if role_ids: for role_id in role_ids: role = security_manager.find_role_by_id(role_id) From 3b63b99dad8842757956618c728484364ae8057f Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Thu, 4 Feb 2021 11:33:28 +0200 Subject: [PATCH 10/11] chore: fix after CR --- superset/commands/utils.py | 8 +++----- superset/security/manager.py | 12 +++++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/superset/commands/utils.py b/superset/commands/utils.py index c70a9e18f2fda..8589bf87c5eec 100644 --- a/superset/commands/utils.py +++ b/superset/commands/utils.py @@ -57,11 +57,9 @@ def populate_roles(role_ids: Optional[List[int]] = None) -> List[Role]: """ roles: List[Role] = [] if role_ids: - for role_id in role_ids: - role = security_manager.find_role_by_id(role_id) - if not role: - raise RolesNotFoundValidationError() - roles.append(role) + roles = security_manager.find_roles_by_id(role_ids) + if len(roles)!=len(role_ids): + raise RolesNotFoundValidationError() return roles diff --git a/superset/security/manager.py b/superset/security/manager.py index 462e94233b0a4..c51358aa2d590 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -27,7 +27,7 @@ assoc_permissionview_role, assoc_user_role, PermissionView, - User, + User, Role, ) from flask_appbuilder.security.views import ( PermissionModelView, @@ -657,10 +657,12 @@ def _get_pvms_from_builtin_role(self, role_name: str) -> List[PermissionView]: role_from_permissions.append(pvm) return role_from_permissions - def find_role_by_id(self, role_id: int) -> Any: - return ( - self.get_session.query(self.role_model).filter_by(id=role_id).one_or_none() - ) + def find_roles_by_id(self, role_ids: List[int]) -> List[Role]: + """ + Find a List of models by a list of ids, if defined applies `base_filter` + """ + query = self.get_session.query(Role).filter(Role.id.in_(role_ids)) + return query.all() def copy_role( self, role_from_name: str, role_to_name: str, merge: bool = True From b735ef6a7800ef6ffe8c18870fca1f6ebef612da Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Thu, 4 Feb 2021 14:37:25 +0200 Subject: [PATCH 11/11] fix:pre-commit --- superset/commands/utils.py | 2 +- superset/security/manager.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/superset/commands/utils.py b/superset/commands/utils.py index 8589bf87c5eec..b39cf6434f063 100644 --- a/superset/commands/utils.py +++ b/superset/commands/utils.py @@ -58,7 +58,7 @@ def populate_roles(role_ids: Optional[List[int]] = None) -> List[Role]: roles: List[Role] = [] if role_ids: roles = security_manager.find_roles_by_id(role_ids) - if len(roles)!=len(role_ids): + if len(roles) != len(role_ids): raise RolesNotFoundValidationError() return roles diff --git a/superset/security/manager.py b/superset/security/manager.py index c51358aa2d590..ba59f68f7ae60 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -27,7 +27,8 @@ assoc_permissionview_role, assoc_user_role, PermissionView, - User, Role, + Role, + User, ) from flask_appbuilder.security.views import ( PermissionModelView,