From 5a5bf7b6a481557899f68317db0d639ee214df01 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 20 Nov 2020 11:41:21 +0000 Subject: [PATCH 01/18] feat(saved queries): security perm simplification --- superset/constants.py | 33 +++++++++++++++++++++++++++ superset/queries/saved_queries/api.py | 6 +++-- superset/views/sql_lab.py | 8 ++++++- 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/superset/constants.py b/superset/constants.py index 65ef2b13a90ed..167e128676177 100644 --- a/superset/constants.py +++ b/superset/constants.py @@ -63,3 +63,36 @@ class RouteMethod: # pylint: disable=too-few-public-methods CRUD_SET = {ADD, LIST, EDIT, DELETE, ACTION_POST, SHOW} RELATED_VIEW_SET = {ADD, LIST, EDIT, DELETE} REST_MODEL_VIEW_CRUD_SET = {DELETE, GET, GET_LIST, POST, PUT, INFO} + + +MODEL_VIEW_RW_METHOD_PERMISSION_MAP = { + "add": "write", + "api": "read", + "api_column_add": "write", + "api_column_edit": "write", + "api_create": "write", + "api_delete": "write", + "api_get": "read", + "api_read": "read", + "api_readvalues": "read", + "api_update": "write", + "delete": "write", + "download": "read", + "edit": "write", + "list": "read", + "muldelete": "write", + "show": "read", +} + +MODEL_API_RW_METHOD_PERMISSION_MAP = { + "bulk_delete": "write", + "delete": "write", + "distinct": "read", + "export": "read", + "get": "read", + "get_list": "read", + "info": "read", + "post": "write", + "put": "write", + "related": "read", +} diff --git a/superset/queries/saved_queries/api.py b/superset/queries/saved_queries/api.py index cb578976a9d5a..37e82f46d0d67 100644 --- a/superset/queries/saved_queries/api.py +++ b/superset/queries/saved_queries/api.py @@ -25,7 +25,7 @@ from flask_appbuilder.models.sqla.interface import SQLAInterface from flask_babel import ngettext -from superset.constants import RouteMethod +from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.databases.filters import DatabaseFilter from superset.models.sql_lab import SavedQuery from superset.queries.saved_queries.commands.bulk_delete import ( @@ -60,7 +60,9 @@ class SavedQueryRestApi(BaseSupersetModelRestApi): RouteMethod.DISTINCT, "bulk_delete", # not using RouteMethod since locally defined } - class_permission_name = "SavedQueryView" + class_permission_name = "SavedQuery" + method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP + resource_name = "saved_query" allow_browser_login = True diff --git a/superset/views/sql_lab.py b/superset/views/sql_lab.py index c0217687cd579..53a32c63e82c7 100644 --- a/superset/views/sql_lab.py +++ b/superset/views/sql_lab.py @@ -22,7 +22,7 @@ from flask_babel import lazy_gettext as _ from superset import db, is_feature_enabled -from superset.constants import RouteMethod +from superset.constants import MODEL_VIEW_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.models.sql_lab import Query, SavedQuery, TableSchema, TabState from superset.typing import FlaskResponse from superset.utils import core as utils @@ -36,6 +36,8 @@ class SavedQueryView( datamodel = SQLAInterface(SavedQuery) include_route_methods = RouteMethod.CRUD_SET + class_permission_name = "SavedQuery" + method_permission_name = MODEL_VIEW_RW_METHOD_PERMISSION_MAP list_title = _("List Saved Query") show_title = _("Show Saved Query") add_title = _("Add Saved Query") @@ -97,6 +99,10 @@ class SavedQueryViewApi(SavedQueryView): # pylint: disable=too-many-ancestors RouteMethod.API_UPDATE, RouteMethod.API_GET, } + + class_permission_name = "SavedQuery" + method_permission_name = MODEL_VIEW_RW_METHOD_PERMISSION_MAP + list_columns = [ "id", "label", From 7445db27503b44fbcb9a9e1619a5ad4b7ccd268e Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 20 Nov 2020 18:44:16 +0000 Subject: [PATCH 02/18] migration script and frontend --- .../CRUD/data/savedquery/SavedQueryList.tsx | 4 +- ...7dbf641_security_converge_saved_queries.py | 233 ++++++++++++++++++ 2 files changed, 235 insertions(+), 2 deletions(-) create mode 100644 superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py diff --git a/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx b/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx index 96f0f5f9e707e..2ccf208973ae3 100644 --- a/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx +++ b/superset-frontend/src/views/CRUD/data/savedquery/SavedQueryList.tsx @@ -91,8 +91,8 @@ function SavedQueryList({ setSavedQueryCurrentlyPreviewing, ] = useState(null); - const canEdit = hasPerm('can_edit'); - const canDelete = hasPerm('can_delete'); + const canEdit = hasPerm('can_write'); + const canDelete = hasPerm('can_write'); const openNewQuery = () => { window.open(`${window.location.origin}/superset/sqllab?new=true`); diff --git a/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py new file mode 100644 index 0000000000000..d3f9d690127b9 --- /dev/null +++ b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py @@ -0,0 +1,233 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""security converge saved queries + +Revision ID: e38177dbf641 +Revises: 49b5a32daba5 +Create Date: 2020-11-20 14:24:03.643031 + +""" + +# revision identifiers, used by Alembic. +revision = 'e38177dbf641' +down_revision = '49b5a32daba5' + +from typing import Dict, Tuple +from alembic import op +from sqlalchemy import ( + Column, + ForeignKey, + Integer, + Sequence, + String, + Table, + UniqueConstraint, +) +from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.ext.declarative import declarative_base +from sqlalchemy.orm import relationship, Session + +Base = declarative_base() + + +# Partial freeze of the current metadata db schema + + +class Permission(Base): + __tablename__ = "ab_permission" + id = Column(Integer, Sequence("ab_permission_id_seq"), primary_key=True) + name = Column(String(100), unique=True, nullable=False) + + def __repr__(self): + return f"{self.name}" + + +class ViewMenu(Base): + __tablename__ = "ab_view_menu" + id = Column(Integer, Sequence("ab_view_menu_id_seq"), primary_key=True) + name = Column(String(250), unique=True, nullable=False) + + def __repr__(self): + return f"{self.name}" + + def __eq__(self, other): + return (isinstance(other, self.__class__)) and (self.name == other.name) + + def __neq__(self, other): + return self.name != other.name + + +assoc_permissionview_role = Table( + "ab_permission_view_role", + Base.metadata, + Column("id", Integer, Sequence("ab_permission_view_role_id_seq"), primary_key=True), + Column("permission_view_id", Integer, ForeignKey("ab_permission_view.id")), + Column("role_id", Integer, ForeignKey("ab_role.id")), + UniqueConstraint("permission_view_id", "role_id"), +) + + +class Role(Base): + __tablename__ = "ab_role" + + id = Column(Integer, Sequence("ab_role_id_seq"), primary_key=True) + name = Column(String(64), unique=True, nullable=False) + permissions = relationship( + "PermissionView", secondary=assoc_permissionview_role, backref="role" + ) + + +class PermissionView(Base): + __tablename__ = "ab_permission_view" + __table_args__ = (UniqueConstraint("permission_id", "view_menu_id"),) + id = Column(Integer, Sequence("ab_permission_view_id_seq"), primary_key=True) + permission_id = Column(Integer, ForeignKey("ab_permission.id")) + permission = relationship("Permission") + view_menu_id = Column(Integer, ForeignKey("ab_view_menu.id")) + view_menu = relationship("ViewMenu") + + def __repr__(self): + return f"{self.permission} {self.view_menu}" + + +def add_pvms( + session: Session, pvm_data: Dict[str, Tuple[str, str]], commit: bool = False +) -> None: + for view_name, permissions in pvm_data.items(): + # Check and add the new View + new_view = session.query( + ViewMenu + ).filter(ViewMenu.name == view_name).one_or_none() + if not new_view: + new_view = ViewMenu(name=view_name) + session.add(new_view) + for permission_name in permissions: + # Check and add the new Permission + new_permission = session.query( + Permission + ).filter(Permission.name == permission_name).one_or_none() + if not new_permission: + new_permission = Permission(name=permission_name) + session.add(new_permission) + # Check and add the new PVM + new_pvm = session.query(PermissionView).filter( + PermissionView.view_menu_id == new_view.id, + PermissionView.permission_id == new_permission.id, + ).one_or_none() + if not new_pvm: + new_pvm = PermissionView(view_menu=new_view, permission=new_permission) + session.add(new_pvm) + if commit: + session.commit() + + +def find_pvm(session: Session, view_name: str, permission_name: str) -> PermissionView: + return ( + session.query(PermissionView) + .join(Permission) + .join(ViewMenu) + .filter( + ViewMenu.name == view_name, + Permission.name == permission_name + ) + ).one_or_none() + + +def migrate_roles( + session: Session, + pvm_key_map: Dict[Tuple[str, str], Tuple[str, str]], + commit: bool = False +) -> None: + from sqlalchemy.orm import Load + + pvm_map: Dict[PermissionView, PermissionView] = {} + for old_pvm_key, new_pvm_key in pvm_key_map.items(): + old_pvm = find_pvm(session, old_pvm_key[0], old_pvm_key[1]) + if old_pvm: + new_pvm = find_pvm(session, new_pvm_key[0], new_pvm_key[1]) + pvm_map[old_pvm] = new_pvm + else: + print(f"Could not find pvm: {old_pvm_key[0]}.{old_pvm_key[1]}") + roles = session.query(Role).options(Load(Role).joinedload(Role.permissions)).all() + # Replace old permissions by the new ones on all existing roles + for role in roles: + for old_pvm, new_pvm in pvm_map.items(): + if old_pvm in role.permissions: + role.permissions.remove(old_pvm) + if new_pvm not in role.permissions: + role.permissions.append(new_pvm) + session.merge(role) + # Delete old permissions + for old_pvm, new_pvm in pvm_map.items(): + old_permission_name = old_pvm.permission.name + old_view_name = old_pvm.view_menu.name + print(f"Going to delete pvm: {old_pvm}") + session.delete(old_pvm) + pvms_with_permission = ( + session.query(PermissionView) + .join(Permission) + .filter(Permission.name == old_permission_name) + ).first() + if not pvms_with_permission: + print(f"Going to delete permission: {old_pvm.permission}") + session.delete(old_pvm.permission) + pvms_with_view_menu = ( + session.query(PermissionView) + .join(ViewMenu) + .filter(ViewMenu.name == old_view_name) + ).first() + if not pvms_with_view_menu: + print(f"Going to delete view_menu: {old_pvm.view_menu}") + session.delete(old_pvm.view_menu) + + if commit: + session.commit() + + +def upgrade(): + bind = op.get_bind() + session = Session(bind=bind) + + new_pvms = { + "SavedQuery": ("can_read", "can_write",) + } + pvm_map = { + ("SavedQueryView", "can_list",): ("SavedQuery", "can_read",), + ("SavedQueryView", "can_show",): ("SavedQuery", "can_read",), + ("SavedQueryView", "can_add",): ("SavedQuery", "can_write",), + ("SavedQueryView", "can_edit",): ("SavedQuery", "can_write",), + ("SavedQueryView", "can_delete",): ("SavedQuery", "can_write",), + ("SavedQueryView", "muldelete",): ("SavedQuery", "can_write",), + ("SavedQueryView", "can_mulexport",): ("SavedQuery", "can_read",), + ("SavedQueryViewApi", "can_show",): ("SavedQuery", "can_read",), + ("SavedQueryViewApi", "can_edit",): ("SavedQuery", "can_write",), + ("SavedQueryViewApi", "can_list",): ("SavedQuery", "can_read",), + ("SavedQueryViewApi", "can_add",): ("SavedQuery", "can_write",), + ("SavedQueryViewApi", "muldelete",): ("SavedQuery", "can_write",), + } + # Add the new permissions on the migration itself + add_pvms(session, new_pvms) + migrate_roles(session, pvm_map) + try: + session.commit() + except SQLAlchemyError as ex: + print(f"An error occurred while migrating permissions: {ex}") + session.rollback() + + +def downgrade(): + pass From a73c5ce020717225c4d7504d0a223397759afe5b Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 23 Nov 2020 12:03:31 +0000 Subject: [PATCH 03/18] add downgrade procedure --- ...7dbf641_security_converge_saved_queries.py | 144 ++++++++++++------ 1 file changed, 96 insertions(+), 48 deletions(-) diff --git a/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py index d3f9d690127b9..e617ac8757934 100644 --- a/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py +++ b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py @@ -23,10 +23,11 @@ """ # revision identifiers, used by Alembic. -revision = 'e38177dbf641' -down_revision = '49b5a32daba5' +revision = "e38177dbf641" +down_revision = "49b5a32daba5" + +from typing import Dict, List, Tuple -from typing import Dict, Tuple from alembic import op from sqlalchemy import ( Column, @@ -90,6 +91,9 @@ class Role(Base): "PermissionView", secondary=assoc_permissionview_role, backref="role" ) + def __repr__(self): + return f"{self.name}" + class PermissionView(Base): __tablename__ = "ab_permission_view" @@ -109,25 +113,31 @@ def add_pvms( ) -> None: for view_name, permissions in pvm_data.items(): # Check and add the new View - new_view = session.query( - ViewMenu - ).filter(ViewMenu.name == view_name).one_or_none() + new_view = ( + session.query(ViewMenu).filter(ViewMenu.name == view_name).one_or_none() + ) if not new_view: new_view = ViewMenu(name=view_name) session.add(new_view) for permission_name in permissions: # Check and add the new Permission - new_permission = session.query( - Permission - ).filter(Permission.name == permission_name).one_or_none() + new_permission = ( + session.query(Permission) + .filter(Permission.name == permission_name) + .one_or_none() + ) if not new_permission: new_permission = Permission(name=permission_name) session.add(new_permission) # Check and add the new PVM - new_pvm = session.query(PermissionView).filter( - PermissionView.view_menu_id == new_view.id, - PermissionView.permission_id == new_permission.id, - ).one_or_none() + new_pvm = ( + session.query(PermissionView) + .filter( + PermissionView.view_menu_id == new_view.id, + PermissionView.permission_id == new_permission.id, + ) + .one_or_none() + ) if not new_pvm: new_pvm = PermissionView(view_menu=new_view, permission=new_permission) session.add(new_pvm) @@ -140,39 +150,45 @@ def find_pvm(session: Session, view_name: str, permission_name: str) -> Permissi session.query(PermissionView) .join(Permission) .join(ViewMenu) - .filter( - ViewMenu.name == view_name, - Permission.name == permission_name - ) + .filter(ViewMenu.name == view_name, Permission.name == permission_name) ).one_or_none() def migrate_roles( session: Session, - pvm_key_map: Dict[Tuple[str, str], Tuple[str, str]], - commit: bool = False + pvm_key_map: Dict[Tuple[str, str], Tuple[Tuple[str, str]]], + commit: bool = False, ) -> None: from sqlalchemy.orm import Load - pvm_map: Dict[PermissionView, PermissionView] = {} - for old_pvm_key, new_pvm_key in pvm_key_map.items(): + pvm_map: Dict[PermissionView, List[PermissionView]] = {} + for old_pvm_key, new_pvms in pvm_key_map.items(): old_pvm = find_pvm(session, old_pvm_key[0], old_pvm_key[1]) if old_pvm: - new_pvm = find_pvm(session, new_pvm_key[0], new_pvm_key[1]) - pvm_map[old_pvm] = new_pvm + for new_pvm_key in new_pvms: + new_pvm = find_pvm(session, new_pvm_key[0], new_pvm_key[1]) + if old_pvm not in pvm_map: + pvm_map[old_pvm] = [new_pvm] + else: + pvm_map[old_pvm].append(new_pvm) else: print(f"Could not find pvm: {old_pvm_key[0]}.{old_pvm_key[1]}") - roles = session.query(Role).options(Load(Role).joinedload(Role.permissions)).all() + # Replace old permissions by the new ones on all existing roles + roles = session.query(Role).options(Load(Role).joinedload(Role.permissions)).all() for role in roles: - for old_pvm, new_pvm in pvm_map.items(): + for old_pvm, new_pvms in pvm_map.items(): if old_pvm in role.permissions: + print(f"Removing {old_pvm} from {role}") role.permissions.remove(old_pvm) - if new_pvm not in role.permissions: - role.permissions.append(new_pvm) + for new_pvm in new_pvms: + if new_pvm not in role.permissions: + print(f"Add {new_pvm} to {role}") + role.permissions.append(new_pvm) session.merge(role) + # Delete old permissions - for old_pvm, new_pvm in pvm_map.items(): + for old_pvm, new_pvms in pvm_map.items(): old_permission_name = old_pvm.permission.name old_view_name = old_pvm.view_menu.name print(f"Going to delete pvm: {old_pvm}") @@ -198,36 +214,68 @@ def migrate_roles( session.commit() +def get_reversed_new_pvms() -> Dict[str, Tuple[str, str]]: + new_pvms = {} + for k, v in PVM_MAP.items(): + if k[0] not in new_pvms: + new_pvms[k[0]] = (k[1],) + else: + new_pvms[k[0]] = new_pvms[k[0]] + (k[1],) + return new_pvms + + +def get_reversed_pvm_map() -> Dict[Tuple[str, str], Tuple[Tuple[str, str]]]: + pvm_map = {} + for old_pvm, new_pvms in PVM_MAP.items(): + for new_pvm in new_pvms: + if new_pvm not in pvm_map: + pvm_map[new_pvm] = (old_pvm,) + else: + pvm_map[new_pvm] = pvm_map[new_pvm] + (old_pvm,) + return pvm_map + + +NEW_PVMS = {"SavedQuery": ("can_read", "can_write",)} +PVM_MAP = { + ("SavedQueryView", "can_list",): (("SavedQuery", "can_read",),), + ("SavedQueryView", "can_show",): (("SavedQuery", "can_read",),), + ("SavedQueryView", "can_add",): (("SavedQuery", "can_write",),), + ("SavedQueryView", "can_edit",): (("SavedQuery", "can_write",),), + ("SavedQueryView", "can_delete",): (("SavedQuery", "can_write",),), + ("SavedQueryView", "muldelete",): (("SavedQuery", "can_write",),), + ("SavedQueryView", "can_mulexport",): (("SavedQuery", "can_read",),), + ("SavedQueryViewApi", "can_show",): (("SavedQuery", "can_read",),), + ("SavedQueryViewApi", "can_edit",): (("SavedQuery", "can_write",),), + ("SavedQueryViewApi", "can_list",): (("SavedQuery", "can_read",),), + ("SavedQueryViewApi", "can_add",): (("SavedQuery", "can_write",),), + ("SavedQueryViewApi", "muldelete",): (("SavedQuery", "can_write",),), +} + + def upgrade(): bind = op.get_bind() session = Session(bind=bind) - new_pvms = { - "SavedQuery": ("can_read", "can_write",) - } - pvm_map = { - ("SavedQueryView", "can_list",): ("SavedQuery", "can_read",), - ("SavedQueryView", "can_show",): ("SavedQuery", "can_read",), - ("SavedQueryView", "can_add",): ("SavedQuery", "can_write",), - ("SavedQueryView", "can_edit",): ("SavedQuery", "can_write",), - ("SavedQueryView", "can_delete",): ("SavedQuery", "can_write",), - ("SavedQueryView", "muldelete",): ("SavedQuery", "can_write",), - ("SavedQueryView", "can_mulexport",): ("SavedQuery", "can_read",), - ("SavedQueryViewApi", "can_show",): ("SavedQuery", "can_read",), - ("SavedQueryViewApi", "can_edit",): ("SavedQuery", "can_write",), - ("SavedQueryViewApi", "can_list",): ("SavedQuery", "can_read",), - ("SavedQueryViewApi", "can_add",): ("SavedQuery", "can_write",), - ("SavedQueryViewApi", "muldelete",): ("SavedQuery", "can_write",), - } # Add the new permissions on the migration itself - add_pvms(session, new_pvms) - migrate_roles(session, pvm_map) + add_pvms(session, NEW_PVMS) + migrate_roles(session, PVM_MAP) try: session.commit() except SQLAlchemyError as ex: - print(f"An error occurred while migrating permissions: {ex}") + print(f"An error occurred while upgrading permissions: {ex}") session.rollback() def downgrade(): + bind = op.get_bind() + session = Session(bind=bind) + + # Add the new permissions on the migration itself + add_pvms(session, get_reversed_new_pvms()) + migrate_roles(session, get_reversed_pvm_map()) + try: + session.commit() + except SQLAlchemyError as ex: + print(f"An error occurred while downgrading permissions: {ex}") + session.rollback() pass From ce6080aa8b8228a1fab17094f0175f7f67be3848 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 23 Nov 2020 12:07:41 +0000 Subject: [PATCH 04/18] downgrade procedure, inferred from the upgrade data --- .../e38177dbf641_security_converge_saved_queries.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py index e617ac8757934..beead99971f08 100644 --- a/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py +++ b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py @@ -216,11 +216,11 @@ def migrate_roles( def get_reversed_new_pvms() -> Dict[str, Tuple[str, str]]: new_pvms = {} - for k, v in PVM_MAP.items(): - if k[0] not in new_pvms: - new_pvms[k[0]] = (k[1],) + for old_pvm, new_pvms in PVM_MAP.items(): + if old_pvm[0] not in new_pvms: + new_pvms[old_pvm[0]] = (old_pvm[1],) else: - new_pvms[k[0]] = new_pvms[k[0]] + (k[1],) + new_pvms[old_pvm[0]] = new_pvms[old_pvm[0]] + (old_pvm[1],) return new_pvms From 2a438127784dee1a08a965b667a19415df751985 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 23 Nov 2020 13:41:17 +0000 Subject: [PATCH 05/18] fix JS test --- .../views/CRUD/data/savedquery/SavedQueryList_spec.jsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset-frontend/spec/javascripts/views/CRUD/data/savedquery/SavedQueryList_spec.jsx b/superset-frontend/spec/javascripts/views/CRUD/data/savedquery/SavedQueryList_spec.jsx index 1420d86cec968..faa40c66d0325 100644 --- a/superset-frontend/spec/javascripts/views/CRUD/data/savedquery/SavedQueryList_spec.jsx +++ b/superset-frontend/spec/javascripts/views/CRUD/data/savedquery/SavedQueryList_spec.jsx @@ -70,8 +70,8 @@ const mockqueries = [...new Array(3)].map((_, i) => ({ })); fetchMock.get(queriesInfoEndpoint, { - permissions: ['can_delete'], -}); + permissions: ['can_write'], +}) fetchMock.get(queriesEndpoint, { result: mockqueries, count: 3, From b67ac5c12a131133635ecb9a95b05cc46a985453 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 23 Nov 2020 13:57:48 +0000 Subject: [PATCH 06/18] fix JS lint --- .../views/CRUD/data/savedquery/SavedQueryList_spec.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/spec/javascripts/views/CRUD/data/savedquery/SavedQueryList_spec.jsx b/superset-frontend/spec/javascripts/views/CRUD/data/savedquery/SavedQueryList_spec.jsx index faa40c66d0325..515c17344c3cf 100644 --- a/superset-frontend/spec/javascripts/views/CRUD/data/savedquery/SavedQueryList_spec.jsx +++ b/superset-frontend/spec/javascripts/views/CRUD/data/savedquery/SavedQueryList_spec.jsx @@ -71,7 +71,7 @@ const mockqueries = [...new Array(3)].map((_, i) => ({ fetchMock.get(queriesInfoEndpoint, { permissions: ['can_write'], -}) +}); fetchMock.get(queriesEndpoint, { result: mockqueries, count: 3, From a6d54217a45b6d5ac4fb0a2a677327f8edcea8fa Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 23 Nov 2020 15:17:04 +0000 Subject: [PATCH 07/18] add simple test --- ...8177dbf641_security_converge_saved_queries.py | 2 -- tests/queries/saved_queries/api_tests.py | 16 +++++++++++++++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py index beead99971f08..d7d3b26df5866 100644 --- a/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py +++ b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py @@ -171,8 +171,6 @@ def migrate_roles( pvm_map[old_pvm] = [new_pvm] else: pvm_map[old_pvm].append(new_pvm) - else: - print(f"Could not find pvm: {old_pvm_key[0]}.{old_pvm_key[1]}") # Replace old permissions by the new ones on all existing roles roles = session.query(Role).options(Load(Role).joinedload(Role.permissions)).all() diff --git a/tests/queries/saved_queries/api_tests.py b/tests/queries/saved_queries/api_tests.py index ce55f2413a443..82b82f9bd50e2 100644 --- a/tests/queries/saved_queries/api_tests.py +++ b/tests/queries/saved_queries/api_tests.py @@ -414,10 +414,24 @@ def test_info_saved_query(self): SavedQuery API: Test info """ self.login(username="admin") - uri = f"api/v1/saved_query/_info" + uri = "api/v1/saved_query/_info" rv = self.get_assert_metric(uri, "info") assert rv.status_code == 200 + def test_info_security_saved_query(self): + """ + SavedQuery API: Test info security + """ + self.login(username="admin") + params = {"keys": ["permissions"]} + uri = f"api/v1/saved_query/_info?q={prison.dumps(params)}" + rv = self.get_assert_metric(uri, "info") + data = json.loads(rv.data.decode("utf-8")) + assert rv.status_code == 200 + assert "can_read" in data["permissions"] + assert "can_write" in data["permissions"] + assert len(data["permissions"]) == 2 + def test_related_saved_query(self): """ SavedQuery API: Test related databases From dca7593093459bdbf9c25d2a3f93fdd097d7bcd9 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 23 Nov 2020 17:20:34 +0000 Subject: [PATCH 08/18] fix downgrade --- .../e38177dbf641_security_converge_saved_queries.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py index d7d3b26df5866..1d60c9d16549c 100644 --- a/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py +++ b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py @@ -213,13 +213,13 @@ def migrate_roles( def get_reversed_new_pvms() -> Dict[str, Tuple[str, str]]: - new_pvms = {} + reversed_pvms = {} for old_pvm, new_pvms in PVM_MAP.items(): - if old_pvm[0] not in new_pvms: - new_pvms[old_pvm[0]] = (old_pvm[1],) + if old_pvm[0] not in reversed_pvms: + reversed_pvms[old_pvm[0]] = (old_pvm[1],) else: - new_pvms[old_pvm[0]] = new_pvms[old_pvm[0]] + (old_pvm[1],) - return new_pvms + reversed_pvms[old_pvm[0]] = reversed_pvms[old_pvm[0]] + (old_pvm[1],) + return reversed_pvms def get_reversed_pvm_map() -> Dict[Tuple[str, str], Tuple[Tuple[str, str]]]: @@ -268,7 +268,7 @@ def downgrade(): bind = op.get_bind() session = Session(bind=bind) - # Add the new permissions on the migration itself + # Add the old permissions on the migration itself add_pvms(session, get_reversed_new_pvms()) migrate_roles(session, get_reversed_pvm_map()) try: From 5343b30de623b7266722feeda0b5f6e097fd2c45 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 24 Nov 2020 10:41:14 +0000 Subject: [PATCH 09/18] make migration scripts generic, fix typing --- superset/migrations/shared/__init__.py | 16 ++ .../migrations/shared/security_converge.py | 220 ++++++++++++++++++ ...7dbf641_security_converge_saved_queries.py | 211 +---------------- 3 files changed, 244 insertions(+), 203 deletions(-) create mode 100644 superset/migrations/shared/__init__.py create mode 100644 superset/migrations/shared/security_converge.py diff --git a/superset/migrations/shared/__init__.py b/superset/migrations/shared/__init__.py new file mode 100644 index 0000000000000..13a83393a9124 --- /dev/null +++ b/superset/migrations/shared/__init__.py @@ -0,0 +1,16 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. diff --git a/superset/migrations/shared/security_converge.py b/superset/migrations/shared/security_converge.py new file mode 100644 index 0000000000000..913f1fd9bca1f --- /dev/null +++ b/superset/migrations/shared/security_converge.py @@ -0,0 +1,220 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from typing import Dict, Iterable, List, Tuple + +from sqlalchemy import ( + Column, + ForeignKey, + Integer, + Sequence, + String, + Table, + UniqueConstraint, +) +from sqlalchemy.ext.declarative import declarative_base +from sqlalchemy.orm import relationship, Session + +Base = declarative_base() + +PvmType = Tuple[str, str] +PvmMigrationMapType = Dict[PvmType, Tuple[PvmType, ...]] + +# Partial freeze of the current metadata db schema + + +class Permission(Base): # type: ignore + __tablename__ = "ab_permission" + id = Column(Integer, Sequence("ab_permission_id_seq"), primary_key=True) + name = Column(String(100), unique=True, nullable=False) + + def __repr__(self) -> str: + return f"{self.name}" + + +class ViewMenu(Base): # type: ignore + __tablename__ = "ab_view_menu" + id = Column(Integer, Sequence("ab_view_menu_id_seq"), primary_key=True) + name = Column(String(250), unique=True, nullable=False) + + def __repr__(self) -> str: + return f"{self.name}" + + def __eq__(self, other: object) -> bool: + return (isinstance(other, self.__class__)) and (self.name == other.name) + + def __neq__(self, other: object) -> bool: + return (isinstance(other, self.__class__)) and self.name != other.name + + +assoc_permissionview_role = Table( + "ab_permission_view_role", + Base.metadata, + Column("id", Integer, Sequence("ab_permission_view_role_id_seq"), primary_key=True), + Column("permission_view_id", Integer, ForeignKey("ab_permission_view.id")), + Column("role_id", Integer, ForeignKey("ab_role.id")), + UniqueConstraint("permission_view_id", "role_id"), +) + + +class Role(Base): # type: ignore + __tablename__ = "ab_role" + + id = Column(Integer, Sequence("ab_role_id_seq"), primary_key=True) + name = Column(String(64), unique=True, nullable=False) + permissions = relationship( + "PermissionView", secondary=assoc_permissionview_role, backref="role" + ) + + def __repr__(self) -> str: + return f"{self.name}" + + +class PermissionView(Base): # type: ignore + __tablename__ = "ab_permission_view" + __table_args__ = (UniqueConstraint("permission_id", "view_menu_id"),) + id = Column(Integer, Sequence("ab_permission_view_id_seq"), primary_key=True) + permission_id = Column(Integer, ForeignKey("ab_permission.id")) + permission = relationship("Permission") + view_menu_id = Column(Integer, ForeignKey("ab_view_menu.id")) + view_menu = relationship("ViewMenu") + + def __repr__(self) -> str: + return f"{self.permission} {self.view_menu}" + + +def add_pvms( + session: Session, pvm_data: Dict[str, Tuple[str, ...]], commit: bool = False +) -> None: + for view_name, permissions in pvm_data.items(): + # Check and add the new View + new_view = ( + session.query(ViewMenu).filter(ViewMenu.name == view_name).one_or_none() + ) + if not new_view: + new_view = ViewMenu(name=view_name) + session.add(new_view) + for permission_name in permissions: + # Check and add the new Permission + new_permission = ( + session.query(Permission) + .filter(Permission.name == permission_name) + .one_or_none() + ) + if not new_permission: + new_permission = Permission(name=permission_name) + session.add(new_permission) + # Check and add the new PVM + new_pvm = ( + session.query(PermissionView) + .filter( + PermissionView.view_menu_id == new_view.id, + PermissionView.permission_id == new_permission.id, + ) + .one_or_none() + ) + if not new_pvm: + new_pvm = PermissionView(view_menu=new_view, permission=new_permission) + session.add(new_pvm) + if commit: + session.commit() + + +def find_pvm(session: Session, view_name: str, permission_name: str) -> PermissionView: + return ( + session.query(PermissionView) + .join(Permission) + .join(ViewMenu) + .filter(ViewMenu.name == view_name, Permission.name == permission_name) + ).one_or_none() + + +def migrate_roles( + session: Session, pvm_key_map: PvmMigrationMapType, commit: bool = False, +) -> None: + from sqlalchemy.orm import Load + + pvm_map: Dict[PermissionView, List[PermissionView]] = {} + for old_pvm_key, new_pvms_ in pvm_key_map.items(): + old_pvm = find_pvm(session, old_pvm_key[0], old_pvm_key[1]) + if old_pvm: + for new_pvm_key in new_pvms_: + new_pvm = find_pvm(session, new_pvm_key[0], new_pvm_key[1]) + if old_pvm not in pvm_map: + pvm_map[old_pvm] = [new_pvm] + else: + pvm_map[old_pvm].append(new_pvm) + + # Replace old permissions by the new ones on all existing roles + roles = session.query(Role).options(Load(Role).joinedload(Role.permissions)).all() + for role in roles: + for old_pvm, new_pvms in pvm_map.items(): + if old_pvm in role.permissions: + print(f"Removing {old_pvm} from {role}") + role.permissions.remove(old_pvm) + for new_pvm in new_pvms: + if new_pvm not in role.permissions: + print(f"Add {new_pvm} to {role}") + role.permissions.append(new_pvm) + session.merge(role) + + # Delete old permissions + for old_pvm, new_pvms in pvm_map.items(): + old_permission_name = old_pvm.permission.name + old_view_name = old_pvm.view_menu.name + print(f"Going to delete pvm: {old_pvm}") + session.delete(old_pvm) + pvms_with_permission = ( + session.query(PermissionView) + .join(Permission) + .filter(Permission.name == old_permission_name) + ).first() + if not pvms_with_permission: + print(f"Going to delete permission: {old_pvm.permission}") + session.delete(old_pvm.permission) + pvms_with_view_menu = ( + session.query(PermissionView) + .join(ViewMenu) + .filter(ViewMenu.name == old_view_name) + ).first() + if not pvms_with_view_menu: + print(f"Going to delete view_menu: {old_pvm.view_menu}") + session.delete(old_pvm.view_menu) + + if commit: + session.commit() + + +def get_reversed_new_pvms(pvm_map: PvmMigrationMapType) -> Dict[str, Tuple[str, ...]]: + reversed_pvms: Dict[str, Tuple[str, ...]] = {} + for old_pvm, new_pvms in pvm_map.items(): + if old_pvm[0] not in reversed_pvms: + reversed_pvms[old_pvm[0]] = (old_pvm[1],) + else: + reversed_pvms[old_pvm[0]] = reversed_pvms[old_pvm[0]] + (old_pvm[1],) + return reversed_pvms + + +def get_reversed_pvm_map(pvm_map: PvmMigrationMapType) -> PvmMigrationMapType: + reversed_pvm_map: PvmMigrationMapType = {} + for old_pvm, new_pvms in pvm_map.items(): + for new_pvm in new_pvms: + if new_pvm not in reversed_pvm_map: + reversed_pvm_map[new_pvm] = (old_pvm,) + else: + reversed_pvm_map[new_pvm] = reversed_pvm_map[new_pvm] + (old_pvm,) + return reversed_pvm_map diff --git a/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py index 1d60c9d16549c..ad34a16ec6039 100644 --- a/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py +++ b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py @@ -26,213 +26,18 @@ revision = "e38177dbf641" down_revision = "49b5a32daba5" -from typing import Dict, List, Tuple from alembic import op -from sqlalchemy import ( - Column, - ForeignKey, - Integer, - Sequence, - String, - Table, - UniqueConstraint, -) from sqlalchemy.exc import SQLAlchemyError -from sqlalchemy.ext.declarative import declarative_base -from sqlalchemy.orm import relationship, Session - -Base = declarative_base() - - -# Partial freeze of the current metadata db schema - - -class Permission(Base): - __tablename__ = "ab_permission" - id = Column(Integer, Sequence("ab_permission_id_seq"), primary_key=True) - name = Column(String(100), unique=True, nullable=False) - - def __repr__(self): - return f"{self.name}" - - -class ViewMenu(Base): - __tablename__ = "ab_view_menu" - id = Column(Integer, Sequence("ab_view_menu_id_seq"), primary_key=True) - name = Column(String(250), unique=True, nullable=False) - - def __repr__(self): - return f"{self.name}" +from sqlalchemy.orm import Session - def __eq__(self, other): - return (isinstance(other, self.__class__)) and (self.name == other.name) - - def __neq__(self, other): - return self.name != other.name - - -assoc_permissionview_role = Table( - "ab_permission_view_role", - Base.metadata, - Column("id", Integer, Sequence("ab_permission_view_role_id_seq"), primary_key=True), - Column("permission_view_id", Integer, ForeignKey("ab_permission_view.id")), - Column("role_id", Integer, ForeignKey("ab_role.id")), - UniqueConstraint("permission_view_id", "role_id"), +from superset.migrations.shared.security_converge import ( + add_pvms, + get_reversed_new_pvms, + get_reversed_pvm_map, + migrate_roles, ) - -class Role(Base): - __tablename__ = "ab_role" - - id = Column(Integer, Sequence("ab_role_id_seq"), primary_key=True) - name = Column(String(64), unique=True, nullable=False) - permissions = relationship( - "PermissionView", secondary=assoc_permissionview_role, backref="role" - ) - - def __repr__(self): - return f"{self.name}" - - -class PermissionView(Base): - __tablename__ = "ab_permission_view" - __table_args__ = (UniqueConstraint("permission_id", "view_menu_id"),) - id = Column(Integer, Sequence("ab_permission_view_id_seq"), primary_key=True) - permission_id = Column(Integer, ForeignKey("ab_permission.id")) - permission = relationship("Permission") - view_menu_id = Column(Integer, ForeignKey("ab_view_menu.id")) - view_menu = relationship("ViewMenu") - - def __repr__(self): - return f"{self.permission} {self.view_menu}" - - -def add_pvms( - session: Session, pvm_data: Dict[str, Tuple[str, str]], commit: bool = False -) -> None: - for view_name, permissions in pvm_data.items(): - # Check and add the new View - new_view = ( - session.query(ViewMenu).filter(ViewMenu.name == view_name).one_or_none() - ) - if not new_view: - new_view = ViewMenu(name=view_name) - session.add(new_view) - for permission_name in permissions: - # Check and add the new Permission - new_permission = ( - session.query(Permission) - .filter(Permission.name == permission_name) - .one_or_none() - ) - if not new_permission: - new_permission = Permission(name=permission_name) - session.add(new_permission) - # Check and add the new PVM - new_pvm = ( - session.query(PermissionView) - .filter( - PermissionView.view_menu_id == new_view.id, - PermissionView.permission_id == new_permission.id, - ) - .one_or_none() - ) - if not new_pvm: - new_pvm = PermissionView(view_menu=new_view, permission=new_permission) - session.add(new_pvm) - if commit: - session.commit() - - -def find_pvm(session: Session, view_name: str, permission_name: str) -> PermissionView: - return ( - session.query(PermissionView) - .join(Permission) - .join(ViewMenu) - .filter(ViewMenu.name == view_name, Permission.name == permission_name) - ).one_or_none() - - -def migrate_roles( - session: Session, - pvm_key_map: Dict[Tuple[str, str], Tuple[Tuple[str, str]]], - commit: bool = False, -) -> None: - from sqlalchemy.orm import Load - - pvm_map: Dict[PermissionView, List[PermissionView]] = {} - for old_pvm_key, new_pvms in pvm_key_map.items(): - old_pvm = find_pvm(session, old_pvm_key[0], old_pvm_key[1]) - if old_pvm: - for new_pvm_key in new_pvms: - new_pvm = find_pvm(session, new_pvm_key[0], new_pvm_key[1]) - if old_pvm not in pvm_map: - pvm_map[old_pvm] = [new_pvm] - else: - pvm_map[old_pvm].append(new_pvm) - - # Replace old permissions by the new ones on all existing roles - roles = session.query(Role).options(Load(Role).joinedload(Role.permissions)).all() - for role in roles: - for old_pvm, new_pvms in pvm_map.items(): - if old_pvm in role.permissions: - print(f"Removing {old_pvm} from {role}") - role.permissions.remove(old_pvm) - for new_pvm in new_pvms: - if new_pvm not in role.permissions: - print(f"Add {new_pvm} to {role}") - role.permissions.append(new_pvm) - session.merge(role) - - # Delete old permissions - for old_pvm, new_pvms in pvm_map.items(): - old_permission_name = old_pvm.permission.name - old_view_name = old_pvm.view_menu.name - print(f"Going to delete pvm: {old_pvm}") - session.delete(old_pvm) - pvms_with_permission = ( - session.query(PermissionView) - .join(Permission) - .filter(Permission.name == old_permission_name) - ).first() - if not pvms_with_permission: - print(f"Going to delete permission: {old_pvm.permission}") - session.delete(old_pvm.permission) - pvms_with_view_menu = ( - session.query(PermissionView) - .join(ViewMenu) - .filter(ViewMenu.name == old_view_name) - ).first() - if not pvms_with_view_menu: - print(f"Going to delete view_menu: {old_pvm.view_menu}") - session.delete(old_pvm.view_menu) - - if commit: - session.commit() - - -def get_reversed_new_pvms() -> Dict[str, Tuple[str, str]]: - reversed_pvms = {} - for old_pvm, new_pvms in PVM_MAP.items(): - if old_pvm[0] not in reversed_pvms: - reversed_pvms[old_pvm[0]] = (old_pvm[1],) - else: - reversed_pvms[old_pvm[0]] = reversed_pvms[old_pvm[0]] + (old_pvm[1],) - return reversed_pvms - - -def get_reversed_pvm_map() -> Dict[Tuple[str, str], Tuple[Tuple[str, str]]]: - pvm_map = {} - for old_pvm, new_pvms in PVM_MAP.items(): - for new_pvm in new_pvms: - if new_pvm not in pvm_map: - pvm_map[new_pvm] = (old_pvm,) - else: - pvm_map[new_pvm] = pvm_map[new_pvm] + (old_pvm,) - return pvm_map - - NEW_PVMS = {"SavedQuery": ("can_read", "can_write",)} PVM_MAP = { ("SavedQueryView", "can_list",): (("SavedQuery", "can_read",),), @@ -269,8 +74,8 @@ def downgrade(): session = Session(bind=bind) # Add the old permissions on the migration itself - add_pvms(session, get_reversed_new_pvms()) - migrate_roles(session, get_reversed_pvm_map()) + add_pvms(session, get_reversed_new_pvms(PVM_MAP)) + migrate_roles(session, get_reversed_pvm_map(PVM_MAP)) try: session.commit() except SQLAlchemyError as ex: From 979defa11b77a9c05a5eaf05b07745e01bbca21e Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 24 Nov 2020 11:21:01 +0000 Subject: [PATCH 10/18] improve code --- .../migrations/shared/security_converge.py | 162 +++++++++++------- 1 file changed, 101 insertions(+), 61 deletions(-) diff --git a/superset/migrations/shared/security_converge.py b/superset/migrations/shared/security_converge.py index 913f1fd9bca1f..d3b24f0842096 100644 --- a/superset/migrations/shared/security_converge.py +++ b/superset/migrations/shared/security_converge.py @@ -15,7 +15,7 @@ # specific language governing permissions and limitations # under the License. -from typing import Dict, Iterable, List, Tuple +from typing import Dict, List, Tuple from sqlalchemy import ( Column, @@ -27,7 +27,7 @@ UniqueConstraint, ) from sqlalchemy.ext.declarative import declarative_base -from sqlalchemy.orm import relationship, Session +from sqlalchemy.orm import Load, relationship, Session Base = declarative_base() @@ -97,63 +97,124 @@ def __repr__(self) -> str: return f"{self.permission} {self.view_menu}" +def _add_view_menu(session: Session, view_name: str) -> ViewMenu: + """ + Check and add the new view menu + """ + new_view = session.query(ViewMenu).filter(ViewMenu.name == view_name).one_or_none() + if not new_view: + new_view = ViewMenu(name=view_name) + session.add(new_view) + return new_view + + +def _add_permission(session: Session, permission_name: str) -> Permission: + """ + Check and add the new Permission + """ + new_permission = ( + session.query(Permission) + .filter(Permission.name == permission_name) + .one_or_none() + ) + if not new_permission: + new_permission = Permission(name=permission_name) + session.add(new_permission) + return new_permission + + +def _add_permission_view( + session: Session, permission: Permission, view_menu: ViewMenu +) -> PermissionView: + """ + Check and add the new Permission View + """ + new_pvm = ( + session.query(PermissionView) + .filter( + PermissionView.view_menu_id == view_menu.id, + PermissionView.permission_id == permission.id, + ) + .one_or_none() + ) + if not new_pvm: + new_pvm = PermissionView(view_menu=view_menu, permission=permission) + session.add(new_pvm) + return new_pvm + + +def _find_pvm(session: Session, view_name: str, permission_name: str) -> PermissionView: + return ( + session.query(PermissionView) + .join(Permission) + .join(ViewMenu) + .filter(ViewMenu.name == view_name, Permission.name == permission_name) + ).one_or_none() + + def add_pvms( session: Session, pvm_data: Dict[str, Tuple[str, ...]], commit: bool = False ) -> None: + """ + Checks if exists and adds new Permissions, Views and PermissionView's + """ for view_name, permissions in pvm_data.items(): # Check and add the new View - new_view = ( - session.query(ViewMenu).filter(ViewMenu.name == view_name).one_or_none() - ) - if not new_view: - new_view = ViewMenu(name=view_name) - session.add(new_view) + new_view = _add_view_menu(session, view_name) for permission_name in permissions: - # Check and add the new Permission - new_permission = ( - session.query(Permission) - .filter(Permission.name == permission_name) - .one_or_none() - ) - if not new_permission: - new_permission = Permission(name=permission_name) - session.add(new_permission) + new_permission = _add_permission(session, permission_name) # Check and add the new PVM - new_pvm = ( - session.query(PermissionView) - .filter( - PermissionView.view_menu_id == new_view.id, - PermissionView.permission_id == new_permission.id, - ) - .one_or_none() - ) - if not new_pvm: - new_pvm = PermissionView(view_menu=new_view, permission=new_permission) - session.add(new_pvm) + _add_permission_view(session, new_permission, new_view) if commit: session.commit() -def find_pvm(session: Session, view_name: str, permission_name: str) -> PermissionView: - return ( - session.query(PermissionView) - .join(Permission) - .join(ViewMenu) - .filter(ViewMenu.name == view_name, Permission.name == permission_name) - ).one_or_none() +def _delete_old_permissions( + session: Session, pvm_map: Dict[PermissionView, List[PermissionView]] +) -> None: + """ + Delete old permissions: + - Delete the PermissionView + - Deletes the Permission if it's an orphan now + - Deletes the ViewMenu if it's an orphan now + """ + # Delete old permissions + for old_pvm, new_pvms in pvm_map.items(): + old_permission_name = old_pvm.permission.name + old_view_name = old_pvm.view_menu.name + print(f"Going to delete pvm: {old_pvm}") + session.delete(old_pvm) + pvms_with_permission = ( + session.query(PermissionView) + .join(Permission) + .filter(Permission.name == old_permission_name) + ).first() + if not pvms_with_permission: + print(f"Going to delete permission: {old_pvm.permission}") + session.delete(old_pvm.permission) + pvms_with_view_menu = ( + session.query(PermissionView) + .join(ViewMenu) + .filter(ViewMenu.name == old_view_name) + ).first() + if not pvms_with_view_menu: + print(f"Going to delete view_menu: {old_pvm.view_menu}") + session.delete(old_pvm.view_menu) def migrate_roles( session: Session, pvm_key_map: PvmMigrationMapType, commit: bool = False, ) -> None: - from sqlalchemy.orm import Load - + """ + Migrates all existing roles that have the permissions to be migrated + """ + # Collect a map of PermissionView objects for migration pvm_map: Dict[PermissionView, List[PermissionView]] = {} for old_pvm_key, new_pvms_ in pvm_key_map.items(): - old_pvm = find_pvm(session, old_pvm_key[0], old_pvm_key[1]) + old_pvm = _find_pvm(session, old_pvm_key[0], old_pvm_key[1]) if old_pvm: for new_pvm_key in new_pvms_: - new_pvm = find_pvm(session, new_pvm_key[0], new_pvm_key[1]) + new_pvm = _find_pvm(session, new_pvm_key[0], new_pvm_key[1]) if old_pvm not in pvm_map: pvm_map[old_pvm] = [new_pvm] else: @@ -173,28 +234,7 @@ def migrate_roles( session.merge(role) # Delete old permissions - for old_pvm, new_pvms in pvm_map.items(): - old_permission_name = old_pvm.permission.name - old_view_name = old_pvm.view_menu.name - print(f"Going to delete pvm: {old_pvm}") - session.delete(old_pvm) - pvms_with_permission = ( - session.query(PermissionView) - .join(Permission) - .filter(Permission.name == old_permission_name) - ).first() - if not pvms_with_permission: - print(f"Going to delete permission: {old_pvm.permission}") - session.delete(old_pvm.permission) - pvms_with_view_menu = ( - session.query(PermissionView) - .join(ViewMenu) - .filter(ViewMenu.name == old_view_name) - ).first() - if not pvms_with_view_menu: - print(f"Going to delete view_menu: {old_pvm.view_menu}") - session.delete(old_pvm.view_menu) - + _delete_old_permissions(session, pvm_map) if commit: session.commit() From 42e58c76f862db956ca9fc8d24d01d4787284e0a Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 25 Nov 2020 18:26:24 +0000 Subject: [PATCH 11/18] add tests for role migration --- .../migrations/shared/security_converge.py | 20 +- tests/security/migrate_roles_tests.py | 194 ++++++++++++++++++ 2 files changed, 206 insertions(+), 8 deletions(-) create mode 100644 tests/security/migrate_roles_tests.py diff --git a/superset/migrations/shared/security_converge.py b/superset/migrations/shared/security_converge.py index d3b24f0842096..9a034e9a93680 100644 --- a/superset/migrations/shared/security_converge.py +++ b/superset/migrations/shared/security_converge.py @@ -14,7 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. - +import logging from typing import Dict, List, Tuple from sqlalchemy import ( @@ -29,6 +29,8 @@ from sqlalchemy.ext.declarative import declarative_base from sqlalchemy.orm import Load, relationship, Session +logger = logging.getLogger(__name__) + Base = declarative_base() PvmType = Tuple[str, str] @@ -154,19 +156,21 @@ def _find_pvm(session: Session, view_name: str, permission_name: str) -> Permiss def add_pvms( session: Session, pvm_data: Dict[str, Tuple[str, ...]], commit: bool = False -) -> None: +) -> List[PermissionView]: """ Checks if exists and adds new Permissions, Views and PermissionView's """ + pvms = [] for view_name, permissions in pvm_data.items(): # Check and add the new View new_view = _add_view_menu(session, view_name) for permission_name in permissions: new_permission = _add_permission(session, permission_name) # Check and add the new PVM - _add_permission_view(session, new_permission, new_view) + pvms.append(_add_permission_view(session, new_permission, new_view)) if commit: session.commit() + return pvms def _delete_old_permissions( @@ -182,7 +186,7 @@ def _delete_old_permissions( for old_pvm, new_pvms in pvm_map.items(): old_permission_name = old_pvm.permission.name old_view_name = old_pvm.view_menu.name - print(f"Going to delete pvm: {old_pvm}") + logger.info(f"Going to delete pvm: {old_pvm}") session.delete(old_pvm) pvms_with_permission = ( session.query(PermissionView) @@ -190,7 +194,7 @@ def _delete_old_permissions( .filter(Permission.name == old_permission_name) ).first() if not pvms_with_permission: - print(f"Going to delete permission: {old_pvm.permission}") + logger.info(f"Going to delete permission: {old_pvm.permission}") session.delete(old_pvm.permission) pvms_with_view_menu = ( session.query(PermissionView) @@ -198,7 +202,7 @@ def _delete_old_permissions( .filter(ViewMenu.name == old_view_name) ).first() if not pvms_with_view_menu: - print(f"Going to delete view_menu: {old_pvm.view_menu}") + logger.info(f"Going to delete view_menu: {old_pvm.view_menu}") session.delete(old_pvm.view_menu) @@ -225,11 +229,11 @@ def migrate_roles( for role in roles: for old_pvm, new_pvms in pvm_map.items(): if old_pvm in role.permissions: - print(f"Removing {old_pvm} from {role}") + logger.info(f"Removing {old_pvm} from {role}") role.permissions.remove(old_pvm) for new_pvm in new_pvms: if new_pvm not in role.permissions: - print(f"Add {new_pvm} to {role}") + logger.info(f"Add {new_pvm} to {role}") role.permissions.append(new_pvm) session.merge(role) diff --git a/tests/security/migrate_roles_tests.py b/tests/security/migrate_roles_tests.py new file mode 100644 index 0000000000000..e2dde466286f7 --- /dev/null +++ b/tests/security/migrate_roles_tests.py @@ -0,0 +1,194 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Unit tests for alerting in Superset""" +import json +import logging +from unittest.mock import patch + +import pytest +from contextlib2 import contextmanager +from flask_appbuilder.security.sqla.models import Role + +from superset.extensions import db, security_manager +from superset.migrations.shared.security_converge import ( + add_pvms, + migrate_roles, + PvmMigrationMapType, +) +from tests.test_app import app + +logging.basicConfig(level=logging.INFO) +logger = logging.getLogger(__name__) + + +@contextmanager +def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): + with app.app_context(): + pvms = [] + for old_pvm, new_pvms in pvm_map.items(): + pvms.append( + security_manager.add_permission_view_menu(old_pvm[1], old_pvm[0]) + ) + for external_pvm in external_pvms: + pvms.append( + security_manager.find_permission_view_menu( + external_pvm[1], external_pvm[0] + ) + ) + + new_role = Role(name="Dummy Role", permissions=pvms) + db.session.add(new_role) + db.session.commit() + + yield new_role + + for old_pvm, new_pvms in pvm_map.items(): + security_manager.del_permission_view_menu(old_pvm[1], old_pvm[0]) + for new_pvm in new_pvms: + security_manager.del_permission_view_menu(new_pvm[1], new_pvm[0]) + + db.session.delete(new_role) + db.session.commit() + + +@pytest.mark.parametrize( + "new_pvms, pvm_map, external_pvms, deleted_views, deleted_permissions", + [ + ( + {"NewDummy": ("can_read",)}, + { + ("DummyView", "can_list",): (("NewDummy", "can_read",),), + ("DummyView", "can_show",): (("NewDummy", "can_read",),), + }, + (), + ("DummyView",), + (), + ), + ( + {"NewDummy": ("can_read", "can_write",)}, + { + ("DummyView", "can_list",): (("NewDummy", "can_read",),), + ("DummyView", "can_show",): (("NewDummy", "can_read",),), + ("DummyView", "can_add",): (("NewDummy", "can_write",),), + ("DummyView", "can_delete",): (("NewDummy", "can_write",),), + }, + (), + ("DummyView",), + (), + ), + ( + {"NewDummy": ("can_read", "can_write",)}, + { + ("DummyView", "can_list",): (("NewDummy", "can_read",),), + ("DummyView", "can_show",): (("NewDummy", "can_read",),), + ("DummyView", "can_add",): (("NewDummy", "can_write",),), + ("DummyView", "can_delete",): (("NewDummy", "can_write",),), + ("DummySecondView", "can_list",): (("NewDummy", "can_read",),), + ("DummySecondView", "can_show",): (("NewDummy", "can_read",),), + ("DummySecondView", "can_add",): (("NewDummy", "can_write",),), + ("DummySecondView", "can_delete",): (("NewDummy", "can_write",),), + }, + (), + ("DummyView", "DummySecondView"), + (), + ), + ( + {"NewDummy": ("can_read", "can_write",)}, + { + ("DummyView", "can_list",): (("NewDummy", "can_read",),), + ("DummyView", "can_add",): (("NewDummy", "can_write",),), + }, + (("UserDBModelView", "can_list"),), + ("DummyView",), + (), + ), + ( + {"NewDummy": ("can_read", "can_write",)}, + { + ("DummyView", "can_list",): (("NewDummy", "can_read",),), + ("DummyView", "can_add",): (("NewDummy", "can_write",),), + ("DummySecondView", "can_list",): (("NewDummy", "can_read",),), + ("DummySecondView", "can_add",): (("NewDummy", "can_write",),), + }, + (("UserDBModelView", "can_list"), ("UserDBModelView", "can_add")), + ("DummyView",), + (), + ), + ( + {"NewDummy": ("can_read", "can_write",)}, + { + ("DummyView", "can_new_perm",): (("NewDummy", "can_read",),), + ("DummyView", "can_add",): (("NewDummy", "can_write",),), + }, + (), + ("DummyView",), + ("can_new_perm",), + ), + ( + {"DummyView": ("can_list", "can_show", "can_add",)}, + { + ("NewDummy", "can_read",): ( + ("DummyView", "can_list",), + ("DummyView", "can_show",), + ), + ("NewDummy", "can_write",): (("DummyView", "can_add",),), + }, + (), + ("NewDummy",), + (), + ), + ], +) +def test_migrate_role_simple( + new_pvms, pvm_map, external_pvms, deleted_views, deleted_permissions +): + """ + Permission migration: Simple base test + """ + with create_old_role(pvm_map, external_pvms) as old_role: + role_name = old_role.name + session = db.session + + # Run migrations + add_pvms(session, new_pvms) + migrate_roles(session, pvm_map) + + role = db.session.query(Role).filter(Role.name == role_name).one_or_none() + for old_pvm, new_pvms in pvm_map.items(): + old_pvm_model = security_manager.find_permission_view_menu( + old_pvm[1], old_pvm[0] + ) + assert old_pvm_model is None + new_pvm_model = security_manager.find_permission_view_menu( + new_pvms[0][1], new_pvms[0][0] + ) + assert new_pvm_model is not None + assert new_pvm_model in role.permissions + # assert deleted view menus + for deleted_view in deleted_views: + assert security_manager.find_view_menu(deleted_view) is None + # assert deleted permissions + for deleted_permission in deleted_permissions: + assert security_manager.find_permission(deleted_permission) is None + # assert externals are still there + for external_pvm in external_pvms: + assert ( + security_manager.find_permission_view_menu( + external_pvm[1], external_pvm[0] + ) + is not None + ) From ef0fe9f71335401c19844c049d218083c010332f Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 25 Nov 2020 18:29:10 +0000 Subject: [PATCH 12/18] change comments --- tests/security/migrate_roles_tests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/security/migrate_roles_tests.py b/tests/security/migrate_roles_tests.py index e2dde466286f7..3395979e76535 100644 --- a/tests/security/migrate_roles_tests.py +++ b/tests/security/migrate_roles_tests.py @@ -153,11 +153,11 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): ), ], ) -def test_migrate_role_simple( +def test_migrate_role( new_pvms, pvm_map, external_pvms, deleted_views, deleted_permissions ): """ - Permission migration: Simple base test + Permission migration: generic tests """ with create_old_role(pvm_map, external_pvms) as old_role: role_name = old_role.name From 644862e172f6391ac25ef15b1e7e413c994c5ec5 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 25 Nov 2020 18:57:40 +0000 Subject: [PATCH 13/18] fix multiple heads --- .../versions/e38177dbf641_security_converge_saved_queries.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py index ad34a16ec6039..20a104fde8881 100644 --- a/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py +++ b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py @@ -17,14 +17,14 @@ """security converge saved queries Revision ID: e38177dbf641 -Revises: 49b5a32daba5 +Revises: a8173232b786 Create Date: 2020-11-20 14:24:03.643031 """ # revision identifiers, used by Alembic. revision = "e38177dbf641" -down_revision = "49b5a32daba5" +down_revision = "a8173232b786" from alembic import op From 4d769ee3ce1e55c5e9c8f89f40a2bd8bd9ba2eb8 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Thu, 26 Nov 2020 10:02:23 +0000 Subject: [PATCH 14/18] more tests and a short description for each one --- tests/security/migrate_roles_tests.py | 40 +++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/tests/security/migrate_roles_tests.py b/tests/security/migrate_roles_tests.py index 3395979e76535..5072488b35b61 100644 --- a/tests/security/migrate_roles_tests.py +++ b/tests/security/migrate_roles_tests.py @@ -56,6 +56,9 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): yield new_role + new_role = db.session.query(Role).filter(Role.name == "Dummy Role").one_or_none() + new_role.permissions = [] + db.session.merge(new_role) for old_pvm, new_pvms in pvm_map.items(): security_manager.del_permission_view_menu(old_pvm[1], old_pvm[0]) for new_pvm in new_pvms: @@ -66,9 +69,10 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): @pytest.mark.parametrize( - "new_pvms, pvm_map, external_pvms, deleted_views, deleted_permissions", + "descriptiom, new_pvms, pvm_map, external_pvms, deleted_views, deleted_permissions", [ ( + "Many to one readonly", {"NewDummy": ("can_read",)}, { ("DummyView", "can_list",): (("NewDummy", "can_read",),), @@ -79,6 +83,18 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): (), ), ( + "Many to one with new permission", + {"NewDummy": ("can_new_perm", "can_write")}, + { + ("DummyView", "can_list",): (("NewDummy", "can_new_perm",),), + ("DummyView", "can_show",): (("NewDummy", "can_write",),), + }, + (), + ("DummyView",), + (), + ), + ( + "Many to one with multiple permissions", {"NewDummy": ("can_read", "can_write",)}, { ("DummyView", "can_list",): (("NewDummy", "can_read",),), @@ -91,6 +107,7 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): (), ), ( + "Many to one with multiple views", {"NewDummy": ("can_read", "can_write",)}, { ("DummyView", "can_list",): (("NewDummy", "can_read",),), @@ -107,6 +124,7 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): (), ), ( + "Many to one with existing permission-view (pvm)", {"NewDummy": ("can_read", "can_write",)}, { ("DummyView", "can_list",): (("NewDummy", "can_read",),), @@ -117,6 +135,7 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): (), ), ( + "Many to one with existing multiple permission-view (pvm)", {"NewDummy": ("can_read", "can_write",)}, { ("DummyView", "can_list",): (("NewDummy", "can_read",),), @@ -129,6 +148,7 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): (), ), ( + "Many to one with with old permission that gets deleted", {"NewDummy": ("can_read", "can_write",)}, { ("DummyView", "can_new_perm",): (("NewDummy", "can_read",),), @@ -139,6 +159,7 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): ("can_new_perm",), ), ( + "Many to Many (normally should be a downgrade)", {"DummyView": ("can_list", "can_show", "can_add",)}, { ("NewDummy", "can_read",): ( @@ -151,14 +172,29 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): ("NewDummy",), (), ), + ( + "Many to Many delete old permissions", + {"DummyView": ("can_list", "can_show", "can_add",)}, + { + ("NewDummy", "can_new_perm1",): ( + ("DummyView", "can_list",), + ("DummyView", "can_show",), + ), + ("NewDummy", "can_new_perm2",): (("DummyView", "can_add",),), + }, + (), + ("NewDummy",), + ("can_new_perm1", "can_new_perm2"), + ), ], ) def test_migrate_role( - new_pvms, pvm_map, external_pvms, deleted_views, deleted_permissions + descriptiom, new_pvms, pvm_map, external_pvms, deleted_views, deleted_permissions ): """ Permission migration: generic tests """ + logger.info(descriptiom) with create_old_role(pvm_map, external_pvms) as old_role: role_name = old_role.name session = db.session From f263177ee20e93a6aa66cb44923f7718263818d5 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Thu, 26 Nov 2020 10:34:31 +0000 Subject: [PATCH 15/18] black --- tests/security/migrate_roles_tests.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/security/migrate_roles_tests.py b/tests/security/migrate_roles_tests.py index 5072488b35b61..d963ee39c36fa 100644 --- a/tests/security/migrate_roles_tests.py +++ b/tests/security/migrate_roles_tests.py @@ -56,7 +56,9 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): yield new_role - new_role = db.session.query(Role).filter(Role.name == "Dummy Role").one_or_none() + new_role = ( + db.session.query(Role).filter(Role.name == "Dummy Role").one_or_none() + ) new_role.permissions = [] db.session.merge(new_role) for old_pvm, new_pvms in pvm_map.items(): From fc067a3b679b5e605f4c93f8b7bccc567dde00b2 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 27 Nov 2020 17:24:29 +0000 Subject: [PATCH 16/18] Improve readability --- .../migrations/shared/security_converge.py | 23 ++- ...7dbf641_security_converge_saved_queries.py | 49 ++++-- tests/security/migrate_roles_tests.py | 142 +++++++++++++----- 3 files changed, 154 insertions(+), 60 deletions(-) diff --git a/superset/migrations/shared/security_converge.py b/superset/migrations/shared/security_converge.py index 9a034e9a93680..e5d3371d4926c 100644 --- a/superset/migrations/shared/security_converge.py +++ b/superset/migrations/shared/security_converge.py @@ -15,6 +15,7 @@ # specific language governing permissions and limitations # under the License. import logging +from dataclasses import dataclass from typing import Dict, List, Tuple from sqlalchemy import ( @@ -33,8 +34,14 @@ Base = declarative_base() -PvmType = Tuple[str, str] -PvmMigrationMapType = Dict[PvmType, Tuple[PvmType, ...]] + +@dataclass(frozen=True) +class Pvm: + permission: str + view: str + + +PvmMigrationMapType = Dict[Pvm, Tuple[Pvm, ...]] # Partial freeze of the current metadata db schema @@ -215,10 +222,10 @@ def migrate_roles( # Collect a map of PermissionView objects for migration pvm_map: Dict[PermissionView, List[PermissionView]] = {} for old_pvm_key, new_pvms_ in pvm_key_map.items(): - old_pvm = _find_pvm(session, old_pvm_key[0], old_pvm_key[1]) + old_pvm = _find_pvm(session, old_pvm_key.view, old_pvm_key.permission) if old_pvm: for new_pvm_key in new_pvms_: - new_pvm = _find_pvm(session, new_pvm_key[0], new_pvm_key[1]) + new_pvm = _find_pvm(session, new_pvm_key.view, new_pvm_key.permission) if old_pvm not in pvm_map: pvm_map[old_pvm] = [new_pvm] else: @@ -246,10 +253,12 @@ def migrate_roles( def get_reversed_new_pvms(pvm_map: PvmMigrationMapType) -> Dict[str, Tuple[str, ...]]: reversed_pvms: Dict[str, Tuple[str, ...]] = {} for old_pvm, new_pvms in pvm_map.items(): - if old_pvm[0] not in reversed_pvms: - reversed_pvms[old_pvm[0]] = (old_pvm[1],) + if old_pvm.view not in reversed_pvms: + reversed_pvms[old_pvm.view] = (old_pvm.view,) else: - reversed_pvms[old_pvm[0]] = reversed_pvms[old_pvm[0]] + (old_pvm[1],) + reversed_pvms[old_pvm.permission] = reversed_pvms[old_pvm.permission] + ( + old_pvm.permission, + ) return reversed_pvms diff --git a/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py index 20a104fde8881..d77278d674ee0 100644 --- a/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py +++ b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py @@ -36,22 +36,47 @@ get_reversed_new_pvms, get_reversed_pvm_map, migrate_roles, + Pvm, ) NEW_PVMS = {"SavedQuery": ("can_read", "can_write",)} PVM_MAP = { - ("SavedQueryView", "can_list",): (("SavedQuery", "can_read",),), - ("SavedQueryView", "can_show",): (("SavedQuery", "can_read",),), - ("SavedQueryView", "can_add",): (("SavedQuery", "can_write",),), - ("SavedQueryView", "can_edit",): (("SavedQuery", "can_write",),), - ("SavedQueryView", "can_delete",): (("SavedQuery", "can_write",),), - ("SavedQueryView", "muldelete",): (("SavedQuery", "can_write",),), - ("SavedQueryView", "can_mulexport",): (("SavedQuery", "can_read",),), - ("SavedQueryViewApi", "can_show",): (("SavedQuery", "can_read",),), - ("SavedQueryViewApi", "can_edit",): (("SavedQuery", "can_write",),), - ("SavedQueryViewApi", "can_list",): (("SavedQuery", "can_read",),), - ("SavedQueryViewApi", "can_add",): (("SavedQuery", "can_write",),), - ("SavedQueryViewApi", "muldelete",): (("SavedQuery", "can_write",),), + Pvm(view="SavedQueryView", permission="can_list"): ( + Pvm(view="SavedQuery", permission="can_read"), + ), + Pvm(view="SavedQueryView", permission="can_show"): ( + Pvm(view="SavedQuery", permission="can_read"), + ), + Pvm(view="SavedQueryView", permission="can_add",): ( + Pvm(view="SavedQuery", permission="can_write"), + ), + Pvm(view="SavedQueryView", permission="can_edit",): ( + Pvm(view="SavedQuery", permission="can_write"), + ), + Pvm(view="SavedQueryView", permission="can_delete",): ( + Pvm(view="SavedQuery", permission="can_write"), + ), + Pvm(view="SavedQueryView", permission="muldelete",): ( + Pvm(view="SavedQuery", permission="can_write"), + ), + Pvm(view="SavedQueryView", permission="can_mulexport",): ( + Pvm(view="SavedQuery", permission="can_read"), + ), + Pvm(view="SavedQueryViewApi", permission="can_show",): ( + Pvm(view="SavedQuery", permission="can_read"), + ), + Pvm(view="SavedQueryViewApi", permission="can_edit",): ( + Pvm(view="SavedQuery", permission="can_write"), + ), + Pvm(view="SavedQueryViewApi", permission="can_list",): ( + Pvm(view="SavedQuery", permission="can_read"), + ), + Pvm(view="SavedQueryViewApi", permission="can_add",): ( + Pvm(view="SavedQuery", permission="can_write"), + ), + Pvm(view="SavedQueryViewApi", permission="muldelete",): ( + Pvm(view="SavedQuery", permission="can_write"), + ), } diff --git a/tests/security/migrate_roles_tests.py b/tests/security/migrate_roles_tests.py index d963ee39c36fa..e3811e3c9e312 100644 --- a/tests/security/migrate_roles_tests.py +++ b/tests/security/migrate_roles_tests.py @@ -27,6 +27,7 @@ from superset.migrations.shared.security_converge import ( add_pvms, migrate_roles, + Pvm, PvmMigrationMapType, ) from tests.test_app import app @@ -41,12 +42,14 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): pvms = [] for old_pvm, new_pvms in pvm_map.items(): pvms.append( - security_manager.add_permission_view_menu(old_pvm[1], old_pvm[0]) + security_manager.add_permission_view_menu( + old_pvm.permission, old_pvm.view + ) ) for external_pvm in external_pvms: pvms.append( security_manager.find_permission_view_menu( - external_pvm[1], external_pvm[0] + external_pvm.permission, external_pvm.view ) ) @@ -62,9 +65,11 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): new_role.permissions = [] db.session.merge(new_role) for old_pvm, new_pvms in pvm_map.items(): - security_manager.del_permission_view_menu(old_pvm[1], old_pvm[0]) + security_manager.del_permission_view_menu(old_pvm.permission, old_pvm.view) for new_pvm in new_pvms: - security_manager.del_permission_view_menu(new_pvm[1], new_pvm[0]) + security_manager.del_permission_view_menu( + new_pvm.permission, new_pvm.view + ) db.session.delete(new_role) db.session.commit() @@ -77,8 +82,12 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to one readonly", {"NewDummy": ("can_read",)}, { - ("DummyView", "can_list",): (("NewDummy", "can_read",),), - ("DummyView", "can_show",): (("NewDummy", "can_read",),), + Pvm(view="DummyView", permission="can_list"): ( + Pvm(view="NewDummy", permission="can_read"), + ), + Pvm(view="DummyView", permission="can_show"): ( + Pvm(view="NewDummy", permission="can_read"), + ), }, (), ("DummyView",), @@ -88,8 +97,12 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to one with new permission", {"NewDummy": ("can_new_perm", "can_write")}, { - ("DummyView", "can_list",): (("NewDummy", "can_new_perm",),), - ("DummyView", "can_show",): (("NewDummy", "can_write",),), + Pvm(view="DummyView", permission="can_list"): ( + Pvm(view="NewDummy", permission="can_new_perm"), + ), + Pvm(view="DummyView", permission="can_show"): ( + Pvm(view="NewDummy", permission="can_write"), + ), }, (), ("DummyView",), @@ -99,10 +112,18 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to one with multiple permissions", {"NewDummy": ("can_read", "can_write",)}, { - ("DummyView", "can_list",): (("NewDummy", "can_read",),), - ("DummyView", "can_show",): (("NewDummy", "can_read",),), - ("DummyView", "can_add",): (("NewDummy", "can_write",),), - ("DummyView", "can_delete",): (("NewDummy", "can_write",),), + Pvm(view="DummyView", permission="can_list"): ( + Pvm(view="NewDummy", permission="can_read"), + ), + Pvm(view="DummyView", permission="can_show"): ( + Pvm(view="NewDummy", permission="can_read"), + ), + Pvm(view="DummyView", permission="can_add"): ( + Pvm(view="NewDummy", permission="can_write"), + ), + Pvm(view="DummyView", permission="can_delete"): ( + Pvm(view="NewDummy", permission="can_write"), + ), }, (), ("DummyView",), @@ -112,14 +133,30 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to one with multiple views", {"NewDummy": ("can_read", "can_write",)}, { - ("DummyView", "can_list",): (("NewDummy", "can_read",),), - ("DummyView", "can_show",): (("NewDummy", "can_read",),), - ("DummyView", "can_add",): (("NewDummy", "can_write",),), - ("DummyView", "can_delete",): (("NewDummy", "can_write",),), - ("DummySecondView", "can_list",): (("NewDummy", "can_read",),), - ("DummySecondView", "can_show",): (("NewDummy", "can_read",),), - ("DummySecondView", "can_add",): (("NewDummy", "can_write",),), - ("DummySecondView", "can_delete",): (("NewDummy", "can_write",),), + Pvm(view="DummyView", permission="can_list"): ( + Pvm(view="NewDummy", permission="can_read"), + ), + Pvm(view="DummyView", permission="can_show"): ( + Pvm(view="NewDummy", permission="can_read"), + ), + Pvm(view="DummyView", permission="can_add"): ( + Pvm(view="NewDummy", permission="can_write"), + ), + Pvm(view="DummyView", permission="can_delete"): ( + Pvm(view="NewDummy", permission="can_write"), + ), + Pvm(view="DummySecondView", permission="can_list"): ( + Pvm(view="NewDummy", permission="can_read"), + ), + Pvm(view="DummySecondView", permission="can_show"): ( + Pvm(view="NewDummy", permission="can_read"), + ), + Pvm(view="DummySecondView", permission="can_add"): ( + Pvm(view="NewDummy", permission="can_write"), + ), + Pvm(view="DummySecondView", permission="can_delete"): ( + Pvm(view="NewDummy", permission="can_write"), + ), }, (), ("DummyView", "DummySecondView"), @@ -129,10 +166,14 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to one with existing permission-view (pvm)", {"NewDummy": ("can_read", "can_write",)}, { - ("DummyView", "can_list",): (("NewDummy", "can_read",),), - ("DummyView", "can_add",): (("NewDummy", "can_write",),), + Pvm(view="DummyView", permission="can_list"): ( + Pvm(view="NewDummy", permission="can_read"), + ), + Pvm(view="DummyView", permission="can_add"): ( + Pvm(view="NewDummy", permission="can_write"), + ), }, - (("UserDBModelView", "can_list"),), + (Pvm(view="UserDBModelView", permission="can_list"),), ("DummyView",), (), ), @@ -140,12 +181,23 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to one with existing multiple permission-view (pvm)", {"NewDummy": ("can_read", "can_write",)}, { - ("DummyView", "can_list",): (("NewDummy", "can_read",),), - ("DummyView", "can_add",): (("NewDummy", "can_write",),), - ("DummySecondView", "can_list",): (("NewDummy", "can_read",),), - ("DummySecondView", "can_add",): (("NewDummy", "can_write",),), + Pvm(view="DummyView", permission="can_list"): ( + Pvm(view="NewDummy", permission="can_read"), + ), + Pvm(view="DummyView", permission="can_add"): ( + Pvm(view="NewDummy", permission="can_write"), + ), + Pvm(view="DummySecondView", permission="can_list"): ( + Pvm(view="NewDummy", permission="can_read"), + ), + Pvm(view="DummySecondView", permission="can_add"): ( + Pvm(view="NewDummy", permission="can_write"), + ), }, - (("UserDBModelView", "can_list"), ("UserDBModelView", "can_add")), + ( + Pvm(view="UserDBModelView", permission="can_list"), + Pvm(view="UserDBModelView", permission="can_add"), + ), ("DummyView",), (), ), @@ -153,8 +205,12 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to one with with old permission that gets deleted", {"NewDummy": ("can_read", "can_write",)}, { - ("DummyView", "can_new_perm",): (("NewDummy", "can_read",),), - ("DummyView", "can_add",): (("NewDummy", "can_write",),), + Pvm(view="DummyView", permission="can_new_perm"): ( + Pvm(view="NewDummy", permission="can_read"), + ), + Pvm(view="DummyView", permission="can_add"): ( + Pvm(view="NewDummy", permission="can_write"), + ), }, (), ("DummyView",), @@ -164,11 +220,13 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to Many (normally should be a downgrade)", {"DummyView": ("can_list", "can_show", "can_add",)}, { - ("NewDummy", "can_read",): ( - ("DummyView", "can_list",), - ("DummyView", "can_show",), + Pvm(view="NewDummy", permission="can_read"): ( + Pvm(view="DummyView", permission="can_list"), + Pvm(view="DummyView", permission="can_show"), + ), + Pvm(view="NewDummy", permission="can_write"): ( + Pvm(view="DummyView", permission="can_add"), ), - ("NewDummy", "can_write",): (("DummyView", "can_add",),), }, (), ("NewDummy",), @@ -178,11 +236,13 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to Many delete old permissions", {"DummyView": ("can_list", "can_show", "can_add",)}, { - ("NewDummy", "can_new_perm1",): ( - ("DummyView", "can_list",), - ("DummyView", "can_show",), + Pvm(view="NewDummy", permission="can_new_perm1"): ( + Pvm(view="DummyView", permission="can_list"), + Pvm(view="DummyView", permission="can_show"), + ), + Pvm(view="NewDummy", permission="can_new_perm2",): ( + Pvm(view="DummyView", permission="can_add"), ), - ("NewDummy", "can_new_perm2",): (("DummyView", "can_add",),), }, (), ("NewDummy",), @@ -208,11 +268,11 @@ def test_migrate_role( role = db.session.query(Role).filter(Role.name == role_name).one_or_none() for old_pvm, new_pvms in pvm_map.items(): old_pvm_model = security_manager.find_permission_view_menu( - old_pvm[1], old_pvm[0] + old_pvm.permission, old_pvm.view ) assert old_pvm_model is None new_pvm_model = security_manager.find_permission_view_menu( - new_pvms[0][1], new_pvms[0][0] + new_pvms[0].permission, new_pvms[0].view ) assert new_pvm_model is not None assert new_pvm_model in role.permissions @@ -226,7 +286,7 @@ def test_migrate_role( for external_pvm in external_pvms: assert ( security_manager.find_permission_view_menu( - external_pvm[1], external_pvm[0] + external_pvm.permission, external_pvm.view ) is not None ) From 2df8b34ff633f09e18d5dc21fcd3e121a5679b19 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 30 Nov 2020 10:39:12 +0000 Subject: [PATCH 17/18] simplify dataclass creation --- ...7dbf641_security_converge_saved_queries.py | 48 ++----- tests/security/migrate_roles_tests.py | 123 +++++------------- 2 files changed, 46 insertions(+), 125 deletions(-) diff --git a/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py index d77278d674ee0..85ce431758dd4 100644 --- a/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py +++ b/superset/migrations/versions/e38177dbf641_security_converge_saved_queries.py @@ -41,42 +41,18 @@ NEW_PVMS = {"SavedQuery": ("can_read", "can_write",)} PVM_MAP = { - Pvm(view="SavedQueryView", permission="can_list"): ( - Pvm(view="SavedQuery", permission="can_read"), - ), - Pvm(view="SavedQueryView", permission="can_show"): ( - Pvm(view="SavedQuery", permission="can_read"), - ), - Pvm(view="SavedQueryView", permission="can_add",): ( - Pvm(view="SavedQuery", permission="can_write"), - ), - Pvm(view="SavedQueryView", permission="can_edit",): ( - Pvm(view="SavedQuery", permission="can_write"), - ), - Pvm(view="SavedQueryView", permission="can_delete",): ( - Pvm(view="SavedQuery", permission="can_write"), - ), - Pvm(view="SavedQueryView", permission="muldelete",): ( - Pvm(view="SavedQuery", permission="can_write"), - ), - Pvm(view="SavedQueryView", permission="can_mulexport",): ( - Pvm(view="SavedQuery", permission="can_read"), - ), - Pvm(view="SavedQueryViewApi", permission="can_show",): ( - Pvm(view="SavedQuery", permission="can_read"), - ), - Pvm(view="SavedQueryViewApi", permission="can_edit",): ( - Pvm(view="SavedQuery", permission="can_write"), - ), - Pvm(view="SavedQueryViewApi", permission="can_list",): ( - Pvm(view="SavedQuery", permission="can_read"), - ), - Pvm(view="SavedQueryViewApi", permission="can_add",): ( - Pvm(view="SavedQuery", permission="can_write"), - ), - Pvm(view="SavedQueryViewApi", permission="muldelete",): ( - Pvm(view="SavedQuery", permission="can_write"), - ), + Pvm("SavedQueryView", "can_list"): (Pvm("SavedQuery", "can_read"),), + Pvm("SavedQueryView", "can_show"): (Pvm("SavedQuery", "can_read"),), + Pvm("SavedQueryView", "can_add",): (Pvm("SavedQuery", "can_write"),), + Pvm("SavedQueryView", "can_edit",): (Pvm("SavedQuery", "can_write"),), + Pvm("SavedQueryView", "can_delete",): (Pvm("SavedQuery", "can_write"),), + Pvm("SavedQueryView", "muldelete",): (Pvm("SavedQuery", "can_write"),), + Pvm("SavedQueryView", "can_mulexport",): (Pvm("SavedQuery", "can_read"),), + Pvm("SavedQueryViewApi", "can_show",): (Pvm("SavedQuery", "can_read"),), + Pvm("SavedQueryViewApi", "can_edit",): (Pvm("SavedQuery", "can_write"),), + Pvm("SavedQueryViewApi", "can_list",): (Pvm("SavedQuery", "can_read"),), + Pvm("SavedQueryViewApi", "can_add",): (Pvm("SavedQuery", "can_write"),), + Pvm("SavedQueryViewApi", "muldelete",): (Pvm("SavedQuery", "can_write"),), } diff --git a/tests/security/migrate_roles_tests.py b/tests/security/migrate_roles_tests.py index e3811e3c9e312..81f067a48e4c5 100644 --- a/tests/security/migrate_roles_tests.py +++ b/tests/security/migrate_roles_tests.py @@ -82,12 +82,8 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to one readonly", {"NewDummy": ("can_read",)}, { - Pvm(view="DummyView", permission="can_list"): ( - Pvm(view="NewDummy", permission="can_read"), - ), - Pvm(view="DummyView", permission="can_show"): ( - Pvm(view="NewDummy", permission="can_read"), - ), + Pvm("DummyView", "can_list"): (Pvm("NewDummy", "can_read"),), + Pvm("DummyView", "can_show"): (Pvm("NewDummy", "can_read"),), }, (), ("DummyView",), @@ -97,12 +93,8 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to one with new permission", {"NewDummy": ("can_new_perm", "can_write")}, { - Pvm(view="DummyView", permission="can_list"): ( - Pvm(view="NewDummy", permission="can_new_perm"), - ), - Pvm(view="DummyView", permission="can_show"): ( - Pvm(view="NewDummy", permission="can_write"), - ), + Pvm("DummyView", "can_list"): (Pvm("NewDummy", "can_new_perm"),), + Pvm("DummyView", "can_show"): (Pvm("NewDummy", "can_write"),), }, (), ("DummyView",), @@ -112,18 +104,10 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to one with multiple permissions", {"NewDummy": ("can_read", "can_write",)}, { - Pvm(view="DummyView", permission="can_list"): ( - Pvm(view="NewDummy", permission="can_read"), - ), - Pvm(view="DummyView", permission="can_show"): ( - Pvm(view="NewDummy", permission="can_read"), - ), - Pvm(view="DummyView", permission="can_add"): ( - Pvm(view="NewDummy", permission="can_write"), - ), - Pvm(view="DummyView", permission="can_delete"): ( - Pvm(view="NewDummy", permission="can_write"), - ), + Pvm("DummyView", "can_list"): (Pvm("NewDummy", "can_read"),), + Pvm("DummyView", "can_show"): (Pvm("NewDummy", "can_read"),), + Pvm("DummyView", "can_add"): (Pvm("NewDummy", "can_write"),), + Pvm("DummyView", "can_delete"): (Pvm("NewDummy", "can_write"),), }, (), ("DummyView",), @@ -133,30 +117,14 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to one with multiple views", {"NewDummy": ("can_read", "can_write",)}, { - Pvm(view="DummyView", permission="can_list"): ( - Pvm(view="NewDummy", permission="can_read"), - ), - Pvm(view="DummyView", permission="can_show"): ( - Pvm(view="NewDummy", permission="can_read"), - ), - Pvm(view="DummyView", permission="can_add"): ( - Pvm(view="NewDummy", permission="can_write"), - ), - Pvm(view="DummyView", permission="can_delete"): ( - Pvm(view="NewDummy", permission="can_write"), - ), - Pvm(view="DummySecondView", permission="can_list"): ( - Pvm(view="NewDummy", permission="can_read"), - ), - Pvm(view="DummySecondView", permission="can_show"): ( - Pvm(view="NewDummy", permission="can_read"), - ), - Pvm(view="DummySecondView", permission="can_add"): ( - Pvm(view="NewDummy", permission="can_write"), - ), - Pvm(view="DummySecondView", permission="can_delete"): ( - Pvm(view="NewDummy", permission="can_write"), - ), + Pvm("DummyView", "can_list"): (Pvm("NewDummy", "can_read"),), + Pvm("DummyView", "can_show"): (Pvm("NewDummy", "can_read"),), + Pvm("DummyView", "can_add"): (Pvm("NewDummy", "can_write"),), + Pvm("DummyView", "can_delete"): (Pvm("NewDummy", "can_write"),), + Pvm("DummySecondView", "can_list"): (Pvm("NewDummy", "can_read"),), + Pvm("DummySecondView", "can_show"): (Pvm("NewDummy", "can_read"),), + Pvm("DummySecondView", "can_add"): (Pvm("NewDummy", "can_write"),), + Pvm("DummySecondView", "can_delete"): (Pvm("NewDummy", "can_write"),), }, (), ("DummyView", "DummySecondView"), @@ -166,14 +134,10 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to one with existing permission-view (pvm)", {"NewDummy": ("can_read", "can_write",)}, { - Pvm(view="DummyView", permission="can_list"): ( - Pvm(view="NewDummy", permission="can_read"), - ), - Pvm(view="DummyView", permission="can_add"): ( - Pvm(view="NewDummy", permission="can_write"), - ), + Pvm("DummyView", "can_list"): (Pvm("NewDummy", "can_read"),), + Pvm("DummyView", "can_add"): (Pvm("NewDummy", "can_write"),), }, - (Pvm(view="UserDBModelView", permission="can_list"),), + (Pvm("UserDBModelView", "can_list"),), ("DummyView",), (), ), @@ -181,23 +145,12 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to one with existing multiple permission-view (pvm)", {"NewDummy": ("can_read", "can_write",)}, { - Pvm(view="DummyView", permission="can_list"): ( - Pvm(view="NewDummy", permission="can_read"), - ), - Pvm(view="DummyView", permission="can_add"): ( - Pvm(view="NewDummy", permission="can_write"), - ), - Pvm(view="DummySecondView", permission="can_list"): ( - Pvm(view="NewDummy", permission="can_read"), - ), - Pvm(view="DummySecondView", permission="can_add"): ( - Pvm(view="NewDummy", permission="can_write"), - ), + Pvm("DummyView", "can_list"): (Pvm("NewDummy", "can_read"),), + Pvm("DummyView", "can_add"): (Pvm("NewDummy", "can_write"),), + Pvm("DummySecondView", "can_list"): (Pvm("NewDummy", "can_read"),), + Pvm("DummySecondView", "can_add"): (Pvm("NewDummy", "can_write"),), }, - ( - Pvm(view="UserDBModelView", permission="can_list"), - Pvm(view="UserDBModelView", permission="can_add"), - ), + (Pvm("UserDBModelView", "can_list"), Pvm("UserDBModelView", "can_add"),), ("DummyView",), (), ), @@ -205,12 +158,8 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to one with with old permission that gets deleted", {"NewDummy": ("can_read", "can_write",)}, { - Pvm(view="DummyView", permission="can_new_perm"): ( - Pvm(view="NewDummy", permission="can_read"), - ), - Pvm(view="DummyView", permission="can_add"): ( - Pvm(view="NewDummy", permission="can_write"), - ), + Pvm("DummyView", "can_new_perm"): (Pvm("NewDummy", "can_read"),), + Pvm("DummyView", "can_add"): (Pvm("NewDummy", "can_write"),), }, (), ("DummyView",), @@ -220,13 +169,11 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to Many (normally should be a downgrade)", {"DummyView": ("can_list", "can_show", "can_add",)}, { - Pvm(view="NewDummy", permission="can_read"): ( - Pvm(view="DummyView", permission="can_list"), - Pvm(view="DummyView", permission="can_show"), - ), - Pvm(view="NewDummy", permission="can_write"): ( - Pvm(view="DummyView", permission="can_add"), + Pvm("NewDummy", "can_read"): ( + Pvm("DummyView", "can_list"), + Pvm("DummyView", "can_show"), ), + Pvm("NewDummy", "can_write"): (Pvm("DummyView", "can_add"),), }, (), ("NewDummy",), @@ -236,13 +183,11 @@ def create_old_role(pvm_map: PvmMigrationMapType, external_pvms): "Many to Many delete old permissions", {"DummyView": ("can_list", "can_show", "can_add",)}, { - Pvm(view="NewDummy", permission="can_new_perm1"): ( - Pvm(view="DummyView", permission="can_list"), - Pvm(view="DummyView", permission="can_show"), - ), - Pvm(view="NewDummy", permission="can_new_perm2",): ( - Pvm(view="DummyView", permission="can_add"), + Pvm("NewDummy", "can_new_perm1"): ( + Pvm("DummyView", "can_list"), + Pvm("DummyView", "can_show"), ), + Pvm("NewDummy", "can_new_perm2",): (Pvm("DummyView", "can_add"),), }, (), ("NewDummy",), From d7c28f624c5d388bd5beec6ce6ce823ba037bdd1 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 30 Nov 2020 11:48:56 +0000 Subject: [PATCH 18/18] fix dataclass --- superset/migrations/shared/security_converge.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/migrations/shared/security_converge.py b/superset/migrations/shared/security_converge.py index e5d3371d4926c..973952653ca13 100644 --- a/superset/migrations/shared/security_converge.py +++ b/superset/migrations/shared/security_converge.py @@ -37,8 +37,8 @@ @dataclass(frozen=True) class Pvm: - permission: str view: str + permission: str PvmMigrationMapType = Dict[Pvm, Tuple[Pvm, ...]]