From 247b67a894658d7b107a96c4b71b7a846cb04d33 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 23 Sep 2020 16:23:37 +0100 Subject: [PATCH 1/9] fix(api): unable to delete virtual dataset because of wrong permission name --- superset/connectors/sqla/models.py | 2 +- superset/views/core.py | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 49663d21421ab..a7c6cd18186cc 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -551,7 +551,7 @@ def datasource_type(self) -> str: @property def database_name(self) -> str: - return self.database.name + return self.database.database_name @classmethod def get_datasource_by_name( 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 From 8add5145ab65c39b4e5d0c9388eb8d0da14c03dc Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Thu, 24 Sep 2020 11:29:03 +0100 Subject: [PATCH 2/9] Still delete the dataset even when no permission was found --- superset/datasets/commands/delete.py | 30 +++++++++++++++------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/superset/datasets/commands/delete.py b/superset/datasets/commands/delete.py index 236fadd0055e2..ee95111115021 100644 --- a/superset/datasets/commands/delete.py +++ b/superset/datasets/commands/delete.py @@ -46,27 +46,29 @@ 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" + if view_menu: + permission_views = ( + db.session.query(security_manager.permissionview_model) + .filter_by(view_menu=view_menu) + .all() ) - 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) + 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) From ea6b11146fee76ce2723d761f80ff81a7e167696 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Thu, 24 Sep 2020 12:43:23 +0100 Subject: [PATCH 3/9] migration script to fix possible existing faulty permissions on the db --- ...ix_data_access_permissions_for_virtual_.py | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py 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..fee85de594ee3 --- /dev/null +++ b/superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py @@ -0,0 +1,71 @@ +# 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 orm +from sqlalchemy.exc import SQLAlchemyError + + +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 + """ + from flask_appbuilder.security.sqla.models import ViewMenu + from superset.connectors.sqla.models import SqlaTable + + bind = op.get_bind() + session = orm.Session(bind=bind) + + faulty_perms = ( + session.query(ViewMenu).filter(ViewMenu.name.ilike("[None].%(id:%)")).all() + ) + for faulty_perm in faulty_perms: + match_ds_id = re.match("\[None\]\.\[.*\]\(id:(.*)\)", faulty_perm.name) + if match_ds_id: + try: + dataset_id = int(match_ds_id.group(1)) + except ValueError: + continue + dataset = session.query(SqlaTable).get(dataset_id) + if dataset: + faulty_perm.name = dataset.get_perm() + try: + session.commit() + except SQLAlchemyError: + session.rollback() + + +def downgrade(): + pass From 34d5c2511bf95a17dbedc46a9d2b52d1fbe57ea3 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Thu, 24 Sep 2020 13:34:32 +0100 Subject: [PATCH 4/9] black --- superset/datasets/commands/delete.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/superset/datasets/commands/delete.py b/superset/datasets/commands/delete.py index ee95111115021..f538415768115 100644 --- a/superset/datasets/commands/delete.py +++ b/superset/datasets/commands/delete.py @@ -53,11 +53,12 @@ def run(self) -> Model: if self._model else None ) + if view_menu: permission_views = ( db.session.query(security_manager.permissionview_model) - .filter_by(view_menu=view_menu) - .all() + .filter_by(view_menu=view_menu) + .all() ) for permission_view in permission_views: From 60fa6c37dc3bfb4bc772c9c031fcc70b00f998e0 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Thu, 24 Sep 2020 15:34:24 +0100 Subject: [PATCH 5/9] fix db migration and one more test --- ...ix_data_access_permissions_for_virtual_.py | 59 +++++++++++++++++-- tests/sqllab_tests.py | 21 +++++++ 2 files changed, 74 insertions(+), 6 deletions(-) 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 index fee85de594ee3..c648ddc55f346 100644 --- a/superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py +++ b/superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py @@ -42,17 +42,28 @@ def upgrade(): Only fixes permissions that still have an associated dataset (fetch by id) and replaces them with the current (correct) permission name """ - from flask_appbuilder.security.sqla.models import ViewMenu + from flask_appbuilder.security.sqla.models import ( + ViewMenu, + PermissionView, + Role, + Permission, + ) from superset.connectors.sqla.models import SqlaTable bind = op.get_bind() session = orm.Session(bind=bind) - faulty_perms = ( - session.query(ViewMenu).filter(ViewMenu.name.ilike("[None].%(id:%)")).all() + faulty_view_menus = ( + session.query(ViewMenu) + .join(PermissionView) + .join(Permission) + .filter(ViewMenu.name.ilike("[None].[%](id:%)")) + .filter(Permission.name == "datasource_access") + .all() ) - for faulty_perm in faulty_perms: - match_ds_id = re.match("\[None\]\.\[.*\]\(id:(.*)\)", faulty_perm.name) + orphaned_faulty_view_menus = [] + for faulty_view_menu in faulty_view_menus: + match_ds_id = re.match("\[None\]\.\[.*\]\(id:(.*)\)", faulty_view_menu.name) if match_ds_id: try: dataset_id = int(match_ds_id.group(1)) @@ -60,7 +71,43 @@ def upgrade(): continue dataset = session.query(SqlaTable).get(dataset_id) if dataset: - faulty_perm.name = dataset.get_perm() + 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 permission with the right name already exists, + # so delete the faulty one later + if existing_view_menu: + orphaned_faulty_view_menus.append(existing_view_menu) + else: + faulty_view_menu.name = new_view_menu + # Commit all possible changes + 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) + session.delete(pvm) + session.delete(orphaned_faulty_view_menu) + try: session.commit() except SQLAlchemyError: 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") From 7a4a450196922d6a7c146821a7fc7dc513932c22 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Thu, 24 Sep 2020 16:08:28 +0100 Subject: [PATCH 6/9] add more comments to the migration script --- ...bbc6e8d654_fix_data_access_permissions_for_virtual_.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) 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 index c648ddc55f346..adcf1f321f7f3 100644 --- a/superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py +++ b/superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py @@ -63,6 +63,7 @@ def upgrade(): ) 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:(.*)\)", faulty_view_menu.name) if match_ds_id: try: @@ -81,10 +82,12 @@ def upgrade(): .filter(ViewMenu.name == new_view_menu) .one_or_none() ) - # A permission with the right name already exists, + # 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(existing_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 possible changes @@ -92,6 +95,7 @@ def upgrade(): session.commit() except SQLAlchemyError: session.rollback() + # Delete all orphaned faulty permissions for orphaned_faulty_view_menu in orphaned_faulty_view_menus: pvm = ( @@ -105,7 +109,9 @@ def upgrade(): 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: From 61fd2de25db0f5b2259e243166dcc75dcab97732 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 25 Sep 2020 11:30:22 +0100 Subject: [PATCH 7/9] freeze a partial schema of the model on the migration step --- superset/connectors/sqla/models.py | 2 +- ...ix_data_access_permissions_for_virtual_.py | 119 ++++++++++++++++-- 2 files changed, 112 insertions(+), 9 deletions(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index a7c6cd18186cc..49663d21421ab 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -551,7 +551,7 @@ def datasource_type(self) -> str: @property def database_name(self) -> str: - return self.database.database_name + return self.database.name @classmethod def get_datasource_by_name( 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 index adcf1f321f7f3..9854530864062 100644 --- a/superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py +++ b/superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py @@ -29,8 +29,118 @@ import re from alembic import op -from sqlalchemy import orm +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(): @@ -42,13 +152,6 @@ def upgrade(): Only fixes permissions that still have an associated dataset (fetch by id) and replaces them with the current (correct) permission name """ - from flask_appbuilder.security.sqla.models import ( - ViewMenu, - PermissionView, - Role, - Permission, - ) - from superset.connectors.sqla.models import SqlaTable bind = op.get_bind() session = orm.Session(bind=bind) From 182a11d6b2361ed865642710d6011a7665d48dbd Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 25 Sep 2020 12:18:17 +0100 Subject: [PATCH 8/9] fix mig script --- .../3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 index 9854530864062..6688cdb708536 100644 --- a/superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py +++ b/superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py @@ -188,12 +188,12 @@ def upgrade(): # 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(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 possible changes + # Commit all view_menu updates try: session.commit() except SQLAlchemyError: From 32acdb15a5e2fdb008dc660e108f77045b16286e Mon Sep 17 00:00:00 2001 From: Daniel Vaz Gaspar Date: Tue, 29 Sep 2020 09:44:28 +0100 Subject: [PATCH 9/9] Update superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> --- ...fbbc6e8d654_fix_data_access_permissions_for_virtual_.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) 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 index 6688cdb708536..a6db4c2cb6153 100644 --- a/superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py +++ b/superset/migrations/versions/3fbbc6e8d654_fix_data_access_permissions_for_virtual_.py @@ -167,12 +167,9 @@ def upgrade(): 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:(.*)\)", faulty_view_menu.name) + match_ds_id = re.match("\[None\]\.\[.*\]\(id:(\d+)\)", faulty_view_menu.name) if match_ds_id: - try: - dataset_id = int(match_ds_id.group(1)) - except ValueError: - continue + dataset_id = int(match_ds_id.group(1)) dataset = session.query(SqlaTable).get(dataset_id) if dataset: try: