fix(api): unable to delete virtual dataset, wrong permission name (ap…

…ache#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

@@ -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)

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

superset/views/core.py

@@ -1820,16 +1820,22 @@ 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)
  .one_or_none()
  )
  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

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")