You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/08/26 13:40:27 UTC
[GitHub] [superset] geido commented on a diff in pull request #21161: fix: dataset name change and permission change
geido commented on code in PR #21161:
URL: https://github.com/apache/superset/pull/21161#discussion_r956055000
##########
superset/security/manager.py:
##########
@@ -1109,80 +1173,412 @@ def _update_vm_datasources_access( # pylint: disable=too-many-locals
updated_view_menus.append(self.find_view_menu(new_dataset_view_menu))
return updated_view_menus
- def database_after_update(
+ def dataset_after_insert(
self,
mapper: Mapper,
connection: Connection,
- target: "Database",
+ target: "SqlaTable",
) -> None:
- # Check if database name has changed
+ """
+ Handles permission creation when a dataset is inserted.
+ Triggered by a SQLAlchemy after_insert event.
+
+ We need to create:
+ - The dataset PVM and set local and schema perm
+
+ :param mapper: The SQLA mapper
+ :param connection: The SQLA connection
+ :param target: The changed dataset object
+ :return:
+ """
+ try:
+ dataset_perm = target.get_perm()
+ except DatasetInvalidPermissionEvaluationException:
+ logger.warning("Dataset has no database refusing to set permission")
+ return
+ dataset_table = target.__table__
+
+ self._insert_pvm_on_sqla_event(
+ mapper, connection, "datasource_access", dataset_perm
+ )
+ if target.perm != dataset_perm:
+ target.perm = dataset_perm
+ connection.execute(
+ dataset_table.update()
+ .where(dataset_table.c.id == target.id)
+ .values(perm=dataset_perm)
+ )
+
+ if target.schema:
+ dataset_schema_perm = self.get_schema_perm(
+ target.database.database_name, target.schema
+ )
+ self._insert_pvm_on_sqla_event(
+ mapper, connection, "schema_access", dataset_schema_perm
+ )
+ target.schema_perm = dataset_schema_perm
+ connection.execute(
+ dataset_table.update()
+ .where(dataset_table.c.id == target.id)
+ .values(schema_perm=dataset_schema_perm)
+ )
+
+ def dataset_after_delete(
+ self,
+ mapper: Mapper,
+ connection: Connection,
+ target: "SqlaTable",
+ ) -> None:
+ """
+ Handles permissions update when a dataset is deleted.
+ Triggered by a SQLAlchemy after_delete event.
+
+ We need to delete:
+ - The dataset PVM
+
+ :param mapper: The SQLA mapper
+ :param connection: The SQLA connection
+ :param target: The changed dataset object
+ :return:
+ """
+ dataset_vm_name = self.get_dataset_perm(
+ target.id, target.table_name, target.database.database_name
+ )
+ self._delete_pvm_on_sqla_event(
+ mapper, connection, "datasource_access", dataset_vm_name
+ )
+
+ def dataset_after_update(
+ self,
+ mapper: Mapper,
+ connection: Connection,
+ target: "SqlaTable",
+ ) -> None:
+ """
+ Handles all permissions update when a dataset is changed.
+ Triggered by a SQLAlchemy after_update event.
+
+ We need to update:
+ - The dataset PVM and local perm
+ - All charts local perm related with said datasets
+ - All charts local schema perm related with said datasets
+
+ :param mapper: The SQLA mapper
+ :param connection: The SQLA connection
+ :param target: The changed dataset object
+ :return:
+ """
+ # Check if watched fields have changed
state = inspect(target)
- history = state.get_history("database_name", True)
- if not history.has_changes() or not history.deleted:
+ history_database = state.get_history("database_id", True)
+ history_table_name = state.get_history("table_name", True)
+ history_schema = state.get_history("schema", True)
+
+ # When database name changes
+ if history_database.has_changes() and history_database.deleted:
+ new_dataset_vm_name = self.get_dataset_perm(
+ target.id, target.table_name, target.database.database_name
+ )
+ self._update_dataset_perm(
+ mapper, connection, target.perm, new_dataset_vm_name, target
+ )
+
+ # Updates schema permissions
+ new_dataset_schema_name = self.get_schema_perm(
+ target.database.database_name, target.schema
+ )
+ self._update_dataset_schema_perm(
+ mapper,
+ connection,
+ new_dataset_schema_name,
+ target,
+ )
+
+ # When table name changes
+ if history_table_name.has_changes() and history_table_name.deleted:
+ old_dataset_name = history_table_name.deleted[0]
+ new_dataset_vm_name = self.get_dataset_perm(
+ target.id, target.table_name, target.database.database_name
+ )
+ old_dataset_vm_name = self.get_dataset_perm(
+ target.id, old_dataset_name, target.database.database_name
+ )
+ self._update_dataset_perm(
+ mapper, connection, old_dataset_vm_name, new_dataset_vm_name, target
+ )
+
+ # When schema changes
+ if history_schema.has_changes() and history_schema.deleted:
+ new_dataset_schema_name = self.get_schema_perm(
+ target.database.database_name, target.schema
+ )
+ self._update_dataset_schema_perm(
+ mapper,
+ connection,
+ new_dataset_schema_name,
+ target,
+ )
+
+ def _update_dataset_schema_perm(
+ self,
+ mapper: Mapper,
+ connection: Connection,
+ new_schema_permission_name: Optional[str],
+ target: "SqlaTable",
+ ) -> None:
+ """
+ Helper method that is called by SQLAlchemy events on datasets to update
+ a new schema permission name, propagates the name change to datasets and charts.
+
+ If the schema permission name does not exist already has a PVM,
+ creates a new one.
+
+ :param mapper: The SQLA event mapper
+ :param connection: The SQLA connection
+ :param new_schema_permission_name: The new schema permission name that changed
+ :param target: Dataset that was updated
+ :return:
+ """
+ from superset.connectors.sqla.models import ( # pylint: disable=import-outside-toplevel
+ SqlaTable,
+ )
+ from superset.models.slice import ( # pylint: disable=import-outside-toplevel
+ Slice,
+ )
+
+ sqlatable_table = SqlaTable.__table__ # pylint: disable=no-member
+ chart_table = Slice.__table__ # pylint: disable=no-member
+
+ # insert new schema PVM if it does not exist
+ self._insert_pvm_on_sqla_event(
+ mapper, connection, "schema_access", new_schema_permission_name
+ )
+
+ # Update dataset (SqlaTable schema_perm field)
+ connection.execute(
+ sqlatable_table.update()
+ .where(
+ sqlatable_table.c.id == target.id,
+ )
+ .values(schema_perm=new_schema_permission_name)
+ )
+
+ # Update charts (Slice schema_perm field)
+ connection.execute(
+ chart_table.update()
+ .where(
+ chart_table.c.datasource_id == target.id,
+ chart_table.c.datasource_type == DatasourceType.TABLE,
+ )
+ .values(schema_perm=new_schema_permission_name)
+ )
+
+ def _update_dataset_perm( # pylint: disable=too-many-arguments
+ self,
+ mapper: Mapper,
+ connection: Connection,
+ old_permission_name: Optional[str],
+ new_permission_name: Optional[str],
+ target: "SqlaTable",
+ ) -> None:
+ """
+ Helper method that is called by SQLAlchemy events on datasets to update
+ a permission name change, propagates the name change to VM, datasets and charts.
+
+ :param mapper:
+ :param connection:
+ :param old_permission_name
+ :param new_permission_name:
+ :param target:
+ :return:
+ """
+ from superset.connectors.sqla.models import ( # pylint: disable=import-outside-toplevel
+ SqlaTable,
+ )
+ from superset.models.slice import ( # pylint: disable=import-outside-toplevel
+ Slice,
+ )
+
+ view_menu_table = self.viewmenu_model.__table__ # pylint: disable=no-member
+ sqlatable_table = SqlaTable.__table__ # pylint: disable=no-member
+ chart_table = Slice.__table__ # pylint: disable=no-member
+
+ new_dataset_view_menu = self.find_view_menu(new_permission_name)
+ if new_dataset_view_menu:
Review Comment:
I am not sure if I am missing something here you are returning if `new_dataset_view_menu` exists. A few lines later in the code at [this line](https://github.com/apache/superset/blob/94b53f0247e732f54caa75b85eed7019c1bd72e3/superset/security/manager.py#L1327) you are using that `new_dataset_view_menu` but the code will never reach that point due to the return above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org