From b9a98aae79705b4db2dab94f1a5fafcf8b821a8b Mon Sep 17 00:00:00 2001 From: Daniel Vaz Gaspar Date: Fri, 20 May 2022 09:40:10 +0100 Subject: [PATCH] fix: None dataset and schema permissions (#20108) * fix: None dataset and schema permissions * fix pylint * add migration and test * fix migration --- superset/connectors/sqla/models.py | 8 + superset/exceptions.py | 6 + .../e786798587de_delete_none_permissions.py | 145 ++++++++++++++++++ superset/security/manager.py | 20 ++- tests/integration_tests/security_tests.py | 29 ++++ 5 files changed, 202 insertions(+), 6 deletions(-) create mode 100644 superset/migrations/versions/e786798587de_delete_none_permissions.py diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 6dde259f98..60eff5e630 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -88,6 +88,7 @@ from superset.datasets.models import Dataset as NewDataset from superset.db_engine_specs.base import BaseEngineSpec, CTE_ALIAS, TimestampExpression from superset.exceptions import ( AdvancedDataTypeResponseError, + DatasetInvalidPermissionEvaluationException, QueryClauseValidationException, QueryObjectValidationError, ) @@ -785,6 +786,13 @@ class SqlaTable(Model, BaseDatasource): # pylint: disable=too-many-public-metho return security_manager.get_schema_perm(self.database, self.schema) def get_perm(self) -> str: + """ + Return this dataset permission name + :return: dataset permission name + :raises DatasetInvalidPermissionEvaluationException: When database is missing + """ + if self.database is None: + raise DatasetInvalidPermissionEvaluationException() return f"[{self.database}].[{self.table_name}](id:{self.id})" @property diff --git a/superset/exceptions.py b/superset/exceptions.py index 758a026a21..07bedfa2db 100644 --- a/superset/exceptions.py +++ b/superset/exceptions.py @@ -216,6 +216,12 @@ class DashboardImportException(SupersetException): pass +class DatasetInvalidPermissionEvaluationException(SupersetException): + """ + When a dataset can't compute its permission name + """ + + class SerializationError(SupersetException): pass diff --git a/superset/migrations/versions/e786798587de_delete_none_permissions.py b/superset/migrations/versions/e786798587de_delete_none_permissions.py new file mode 100644 index 0000000000..e79c7a4370 --- /dev/null +++ b/superset/migrations/versions/e786798587de_delete_none_permissions.py @@ -0,0 +1,145 @@ +# 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. +"""Delete None permissions + +Revision ID: e786798587de +Revises: 6f139c533bea +Create Date: 2022-05-18 16:07:47.648514 + +""" + +# revision identifiers, used by Alembic. +revision = "e786798587de" +down_revision = "6f139c533bea" + +from alembic import op +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() + + +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 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) -> str: + return self.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" + ) + + def __repr__(self) -> str: + 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) -> str: + return f"{self.permission.name} on {self.view_menu.name}" + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + bind = op.get_bind() + session = Session(bind=bind) + + pvms = ( + session.query(PermissionView) + .join(ViewMenu) + .join(Permission) + .filter( + Permission.name.in_(("datasource_access", "schema_access")), + ViewMenu.name.like("[None].%"), + ) + .all() + ) + + roles = ( + session.query(Role) + .outerjoin(Role.permissions) + .join(ViewMenu) + .join(Permission) + .filter( + Permission.name.in_(("datasource_access", "schema_access")), + ViewMenu.name.like("[None].%"), + ) + .all() + ) + + for pvm in pvms: + for role in roles: + if pvm in role.permissions: + print( + f"Going to delete a data access permission [{pvm}] on Role [{role}]" + ) + role.permissions.remove(pvm) + print(f"Going to delete a data access permission [{pvm}]") + session.delete(pvm) + session.delete(pvm.view_menu) + session.commit() + session.close() + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + ... + # ### end Alembic commands ### diff --git a/superset/security/manager.py b/superset/security/manager.py index 44c53329ad..b231b93b48 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -64,7 +64,10 @@ from superset import sql_parse from superset.connectors.connector_registry import ConnectorRegistry from superset.constants import RouteMethod from superset.errors import ErrorLevel, SupersetError, SupersetErrorType -from superset.exceptions import SupersetSecurityException +from superset.exceptions import ( + DatasetInvalidPermissionEvaluationException, + SupersetSecurityException, +) from superset.security.guest_token import ( GuestToken, GuestTokenResources, @@ -934,14 +937,19 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods :param connection: The DB-API connection :param target: The mapped instance being persisted """ + try: + target_get_perm = target.get_perm() + except DatasetInvalidPermissionEvaluationException: + logger.warning("Dataset has no database refusing to set permission") + return link_table = target.__table__ - if target.perm != target.get_perm(): + if target.perm != target_get_perm: connection.execute( link_table.update() .where(link_table.c.id == target.id) - .values(perm=target.get_perm()) + .values(perm=target_get_perm) ) - target.perm = target.get_perm() + target.perm = target_get_perm if ( hasattr(target, "schema_perm") @@ -956,9 +964,9 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods pvm_names = [] if target.__tablename__ in {"dbs", "clusters"}: - pvm_names.append(("database_access", target.get_perm())) + pvm_names.append(("database_access", target_get_perm)) else: - pvm_names.append(("datasource_access", target.get_perm())) + pvm_names.append(("datasource_access", target_get_perm)) if target.schema: pvm_names.append(("schema_access", target.get_schema_perm())) diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index 1b6d1318db..c44335552b 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -272,6 +272,35 @@ class TestRolePermission(SupersetTestCase): session.delete(stored_table) session.commit() + def test_set_perm_sqla_table_none(self): + session = db.session + table = SqlaTable( + schema="tmp_schema", + table_name="tmp_perm_table", + # Setting database_id instead of database will skip permission creation + database_id=get_example_database().id, + ) + session.add(table) + session.commit() + + stored_table = ( + session.query(SqlaTable).filter_by(table_name="tmp_perm_table").one() + ) + # Assert no permission is created + self.assertIsNone( + security_manager.find_permission_view_menu( + "datasource_access", stored_table.perm + ) + ) + # Assert no bogus permission is created + self.assertIsNone( + security_manager.find_permission_view_menu( + "datasource_access", f"[None].[tmp_perm_table](id:{stored_table.id})" + ) + ) + session.delete(table) + session.commit() + def test_set_perm_database(self): session = db.session database = Database(database_name="tmp_database", sqlalchemy_uri="sqlite://")