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

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

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
 

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

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

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