From a589e253e9f1ac776c01f655dbb1f3a3d92caca8 Mon Sep 17 00:00:00 2001 From: Daniel Vaz Gaspar Date: Tue, 29 Sep 2020 12:33:07 +0100 Subject: [PATCH] fix(api): unable to delete virtual dataset, wrong permission name (#11019) * fix(api): unable to delete virtual dataset because of wrong permission name * Still delete the dataset even when no permission was found * migration script to fix possible existing faulty permissions on the db * black * fix db migration and one more test * add more comments to the migration script * freeze a partial schema of the model on the migration step * fix mig script * Update superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> --- superset/datasets/commands/delete.py | 31 +-- ...ix_data_access_permissions_for_virtual_.py | 224 ++++++++++++++++++ superset/views/core.py | 12 +- tests/sqllab_tests.py | 21 ++ 4 files changed, 271 insertions(+), 17 deletions(-) create mode 100644 superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py diff --git a/superset/datasets/commands/delete.py b/superset/datasets/commands/delete.py index 236fadd0055e2..f538415768115 100644 --- a/superset/datasets/commands/delete.py +++ b/superset/datasets/commands/delete.py @@ -46,27 +46,30 @@ def __init__(self, user: User, model_id: int): def run(self) -> Model: self.validate() try: + dataset = DatasetDAO.delete(self._model, commit=False) + view_menu = ( security_manager.find_view_menu(self._model.get_perm()) if self._model else None ) - if not view_menu: - logger.error( - "Could not find the data access permission for the dataset" - ) - raise DatasetDeleteFailedError() - permission_views = ( - db.session.query(security_manager.permissionview_model) - .filter_by(view_menu=view_menu) - .all() - ) - dataset = DatasetDAO.delete(self._model, commit=False) - for permission_view in permission_views: - db.session.delete(permission_view) if view_menu: - db.session.delete(view_menu) + permission_views = ( + db.session.query(security_manager.permissionview_model) + .filter_by(view_menu=view_menu) + .all() + ) + + for permission_view in permission_views: + db.session.delete(permission_view) + if view_menu: + db.session.delete(view_menu) + else: + if not view_menu: + logger.error( + "Could not find the data access permission for the dataset" + ) db.session.commit() except (SQLAlchemyError, DAODeleteFailedError) as ex: logger.exception(ex) diff --git a/superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py b/superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py new file mode 100644 index 0000000000000..a6db4c2cb6153 --- /dev/null +++ b/superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py @@ -0,0 +1,224 @@ +# 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. +"""fix data access permissions for virtual datasets + +Revision ID: 3fbbc6e8d654 +Revises: e5ef6828ac4e +Create Date: 2020-09-24 12:04:33.827436 + +""" + +# revision identifiers, used by Alembic. +revision = "3fbbc6e8d654" +down_revision = "e5ef6828ac4e" + +import re + +from alembic import op +from sqlalchemy import ( + Column, + ForeignKey, + Integer, + orm, + Sequence, + String, + Table, + UniqueConstraint, +) +from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.ext.declarative import declarative_base +from sqlalchemy.orm import backref, relationship + +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) + + +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 __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") + + +sqlatable_user = Table( + "sqlatable_user", + Base.metadata, + Column("id", Integer, primary_key=True), + Column("user_id", Integer, ForeignKey("ab_user.id")), + Column("table_id", Integer, ForeignKey("tables.id")), +) + + +class Database(Base): # pylint: disable=too-many-public-methods + + """An ORM object that stores Database related information""" + + __tablename__ = "dbs" + __table_args__ = (UniqueConstraint("database_name"),) + + id = Column(Integer, primary_key=True) + verbose_name = Column(String(250), unique=True) + # short unique name, used in permissions + database_name = Column(String(250), unique=True, nullable=False) + + def __repr__(self) -> str: + return self.name + + @property + def name(self) -> str: + return self.verbose_name if self.verbose_name else self.database_name + + +class SqlaTable(Base): + __tablename__ = "tables" + __table_args__ = (UniqueConstraint("database_id", "table_name"),) + + # Base columns from Basedatasource + id = Column(Integer, primary_key=True) + table_name = Column(String(250), nullable=False) + database_id = Column(Integer, ForeignKey("dbs.id"), nullable=False) + database = relationship( + "Database", + backref=backref("tables", cascade="all, delete-orphan"), + foreign_keys=[database_id], + ) + + def get_perm(self) -> str: + return f"[{self.database}].[{self.table_name}](id:{self.id})" + + +def upgrade(): + """ + Previous sqla_viz behaviour when creating a virtual dataset was faulty + by creating an associated data access permission with [None] on the database name. + + This migration revision, fixes all faulty permissions that may exist on the db + Only fixes permissions that still have an associated dataset (fetch by id) + and replaces them with the current (correct) permission name + """ + + bind = op.get_bind() + session = orm.Session(bind=bind) + + faulty_view_menus = ( + session.query(ViewMenu) + .join(PermissionView) + .join(Permission) + .filter(ViewMenu.name.ilike("[None].[%](id:%)")) + .filter(Permission.name == "datasource_access") + .all() + ) + orphaned_faulty_view_menus = [] + for faulty_view_menu in faulty_view_menus: + # Get the dataset id from the view_menu name + match_ds_id = re.match("\[None\]\.\[.*\]\(id:(\d+)\)", faulty_view_menu.name) + if match_ds_id: + dataset_id = int(match_ds_id.group(1)) + dataset = session.query(SqlaTable).get(dataset_id) + if dataset: + try: + new_view_menu = dataset.get_perm() + except Exception: + # This can fail on differing SECRET_KEYS + return + existing_view_menu = ( + session.query(ViewMenu) + .filter(ViewMenu.name == new_view_menu) + .one_or_none() + ) + # A view_menu permission with the right name already exists, + # so delete the faulty one later + if existing_view_menu: + orphaned_faulty_view_menus.append(faulty_view_menu) + # No view_menu permission with this name exists + # so safely change this one + else: + faulty_view_menu.name = new_view_menu + # Commit all view_menu updates + try: + session.commit() + except SQLAlchemyError: + session.rollback() + + # Delete all orphaned faulty permissions + for orphaned_faulty_view_menu in orphaned_faulty_view_menus: + pvm = ( + session.query(PermissionView) + .filter(PermissionView.view_menu == orphaned_faulty_view_menu) + .one_or_none() + ) + if pvm: + # Removes orphaned pvm from all roles + roles = session.query(Role).filter(Role.permissions.contains(pvm)).all() + for role in roles: + if pvm in role.permissions: + role.permissions.remove(pvm) + # Now it's safe to remove the pvm pair + session.delete(pvm) + # finally remove the orphaned view_menu permission + session.delete(orphaned_faulty_view_menu) + + try: + session.commit() + except SQLAlchemyError: + session.rollback() + + +def downgrade(): + pass diff --git a/superset/views/core.py b/superset/views/core.py index be07365b7f3f3..71080425b47d8 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -1820,8 +1820,14 @@ def sqllab_table_viz(self) -> FlaskResponse: # pylint: disable=no-self-use @event_logger.log_this def sqllab_viz(self) -> FlaskResponse: # pylint: disable=no-self-use data = json.loads(request.form["data"]) - table_name = data["datasourceName"] - database_id = data["dbId"] + try: + table_name = data["datasourceName"] + database_id = data["dbId"] + except KeyError: + return json_error_response("Missing required fields", status=400) + database = db.session.query(Database).get(database_id) + if not database: + return json_error_response("Database not found", status=400) table = ( db.session.query(SqlaTable) .filter_by(database_id=database_id, table_name=table_name) @@ -1829,7 +1835,7 @@ def sqllab_viz(self) -> FlaskResponse: # pylint: disable=no-self-use ) if not table: table = SqlaTable(table_name=table_name, owners=[g.user]) - table.database_id = database_id + table.database = database table.schema = data.get("schema") table.template_params = data.get("templateParams") table.is_sqllab_view = True diff --git a/tests/sqllab_tests.py b/tests/sqllab_tests.py index 20a3382452ab6..cff4796a58bfb 100644 --- a/tests/sqllab_tests.py +++ b/tests/sqllab_tests.py @@ -381,6 +381,27 @@ def test_sqllab_viz(self): table_id = resp["table_id"] table = db.session.query(SqlaTable).filter_by(id=table_id).one() self.assertEqual([owner.username for owner in table.owners], ["admin"]) + view_menu = security_manager.find_view_menu(table.get_perm()) + assert view_menu is not None + + def test_sqllab_viz_bad_payload(self): + self.login("admin") + payload = { + "chartType": "dist_bar", + "schema": "superset", + "columns": [ + {"is_date": False, "type": "STRING", "name": f"viz_type_{random()}"}, + {"is_date": False, "type": "OBJECT", "name": f"ccount_{random()}"}, + ], + "sql": """\ + SELECT * + FROM birth_names + LIMIT 10""", + } + data = {"data": json.dumps(payload)} + url = "/superset/sqllab_viz/" + response = self.client.post(url, data=data, follow_redirects=True) + assert response.status_code == 400 def test_sqllab_table_viz(self): self.login("admin")