You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by dp...@apache.org on 2020/09/29 11:33:23 UTC
[incubator-superset] branch master updated: fix(api): unable to
delete virtual dataset, wrong permission name (#11019)
This is an automated email from the ASF dual-hosted git repository.
dpgaspar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git
The following commit(s) were added to refs/heads/master by this push:
new 5d08a42 fix(api): unable to delete virtual dataset, wrong permission name (#11019)
5d08a42 is described below
commit 5d08a426d30e7e3b32e09541937f466889c2319b
Author: Daniel Vaz Gaspar <da...@gmail.com>
AuthorDate: Tue Sep 29 12:33:07 2020 +0100
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 <33...@users.noreply.github.com>
Co-authored-by: Ville Brofeldt <33...@users.noreply.github.com>
---
superset/datasets/commands/delete.py | 31 +--
...654_fix_data_access_permissions_for_virtual_.py | 224 +++++++++++++++++++++
superset/views/core.py | 12 +-
tests/sqllab_tests.py | 21 ++
4 files changed, 271 insertions(+), 17 deletions(-)
diff --git a/superset/datasets/commands/delete.py b/superset/datasets/commands/delete.py
index 236fadd..f538415 100644
--- a/superset/datasets/commands/delete.py
+++ b/superset/datasets/commands/delete.py
@@ -46,27 +46,30 @@ class DeleteDatasetCommand(BaseCommand):
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 0000000..a6db4c2
--- /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 be07365..7108042 100755
--- a/superset/views/core.py
+++ b/superset/views/core.py
@@ -1820,8 +1820,14 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods
@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 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods
)
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 20a3382..cff4796 100644
--- a/tests/sqllab_tests.py
+++ b/tests/sqllab_tests.py
@@ -381,6 +381,27 @@ class TestSqlLab(SupersetTestCase):
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")