codeant-ai-for-open-source[bot] commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3510001047
##########
superset/commands/database/sync_permissions.py:
##########
@@ -283,17 +286,29 @@ def _rename_database_in_permissions(
if existing_pvm:
existing_pvm.view_menu.name = new_schema_perm_name
- # rename permissions on datasets and charts
- for dataset in DatabaseDAO.get_datasets(
- self.db_connection_id,
- catalog=catalog,
- schema=schema,
- ):
- dataset.catalog_perm = new_catalog_perm_name
- dataset.schema_perm = new_schema_perm_name
- for chart in
DatasetDAO.get_related_objects(dataset.id)["charts"]:
- chart.catalog_perm = new_catalog_perm_name
- chart.schema_perm = new_schema_perm_name
+ # Rename permissions on datasets and charts. Bypass the
+ # soft-delete visibility filter: a soft-deleted dataset's
+ # ``schema_perm``/``catalog_perm`` (and its charts') must still be
+ # rewritten on a database rename — otherwise restoring the dataset
+ # later brings back stale perm strings referencing the old
+ # database name (fail-closed for legitimate users, and matchable
+ # by a schema_access grant on a new database reusing the old
+ # name). Mirrors the same bypass in
+ # ``security_manager._update_vm_datasources_access``, which
+ # maintains the dataset-level ``perm`` string. ``Slice`` is
+ # included so chart perm rewrites keep working once charts gain
+ # soft delete (a no-op until then).
+ with skip_visibility_filter(db.session, SqlaTable, Slice):
+ for dataset in DatabaseDAO.get_datasets(
+ self.db_connection_id,
+ catalog=catalog,
+ schema=schema,
+ ):
+ dataset.catalog_perm = new_catalog_perm_name
+ dataset.schema_perm = new_schema_perm_name
Review Comment:
**Suggestion:** The dataset rewrite loop is catalog-strict, so when the
renamed database has a non-NULL default catalog, legacy datasets stored with
`catalog = NULL` are skipped and keep stale permission strings. Use a
null-aware catalog match for this rename path (default catalog should match
both explicit default and NULL) so restored datasets and their charts do not
retain old database-based perms. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Default-catalog datasets retain stale schema/catalog permission strings.
- ⚠️ Restored datasets fail for authorized users post-database rename.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a database that supports catalogs with a non-NULL default
catalog, so
`SyncPermissionsCommand._get_catalog_names()` in
`superset/commands/database/sync_permissions.py:188-207` returns
`{self.db_connection.get_default_catalog()}` instead of `{None}`.
2. Ensure there is at least one legacy dataset row for this database whose
physical table
is in the default catalog but whose `SqlaTable.catalog` column is `NULL`
(semantics
documented in `DatasetDAO._catalog_identity_filter()` at
`superset/daos/dataset.py:159-176`, which treats `catalog = NULL` as the
default catalog).
3. Rename the database connection (changing `Database.database_name`) so that
`SyncPermissionsCommand.sync_database_permissions()` at
`superset/commands/database/sync_permissions.py:138-187` runs and enters
`_rename_database_in_permissions()` when `self.old_db_connection_name !=
self.db_connection.database_name`.
4. During `_rename_database_in_permissions()` the loop at
`superset/commands/database/sync_permissions.py:301-308` calls
`DatabaseDAO.get_datasets(self.db_connection_id, catalog=catalog,
schema=schema)`, which
filters with strict equality on `SqlaTable.catalog == catalog`
(`superset/daos/database.py:255-259`). The legacy dataset with `catalog =
NULL` is not
returned, so its `catalog_perm` and `schema_perm` fields are not updated to
the new
database name and remain stale, causing restored datasets/charts to carry
old permission
identifiers.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7d3d209db65a426daadb06c5fafc0a64&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7d3d209db65a426daadb06c5fafc0a64&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/database/sync_permissions.py
**Line:** 301:308
**Comment:**
*Logic Error: The dataset rewrite loop is catalog-strict, so when the
renamed database has a non-NULL default catalog, legacy datasets stored with
`catalog = NULL` are skipped and keep stale permission strings. Use a
null-aware catalog match for this rename path (default catalog should match
both explicit default and NULL) so restored datasets and their charts do not
retain old database-based perms.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=a93b6c2f0b695bd1d43725b45258abe37551bcb3e4872a3881d8209423c949ef&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=a93b6c2f0b695bd1d43725b45258abe37551bcb3e4872a3881d8209423c949ef&reaction=dislike'>👎</a>
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
