aminghadersohi opened a new pull request, #45874: URL: https://github.com/apache/superset/pull/45874
### SUMMARY The permission event hooks in `superset/security/manager.py` rewrite `perm`, `schema_perm` and `catalog_perm` of datasets, semantic views and charts with Core `UPDATE`s on the flush connection. A datasource instance that is already loaded in the session keeps the previous strings. `set_related_perm` (the chart `before_update` hook) copies a chart's perm strings from that instance. So a chart written later in the same transaction writes the previous strings back over the Core values. A log probe in `set_related_perm` during `PUT /api/v1/database/<id>` showed this. The dataset was already loaded in the session, and the chart was flushed when the database permission sync set its `schema_perm`. At that point the dataset instance held `perm='[old_db].[t1](id:1)'`, the dataset row held `[new_db].[t1](id:1)`, and the chart already held the new perm before the hook overwrote it. Charts of datasets without a schema are not touched by that sync, so they kept the right perm. A viewer whose access comes from the dataset grant then gets 404 on `GET /api/v1/chart/<id>/data/` until the chart is saved again. Where it happens: - **Database rename** (`database_after_update`): only when the datasets are already loaded in the session before the rename, e.g. a long-lived app context or a script. A rename from a fresh request session is not affected; `test_rename_database_keeps_chart_perms_in_sync` passes before and after this change. - **Dataset table rename, schema change or database change** (`dataset_before_update`): the dataset being flushed is always loaded. This is reachable through asset import: re-importing a bundle whose dataset has a new table name or schema leaves its chart with the old perm and no schema perm (`test_import_assets_dataset_change_keeps_chart_perms`). - **Semantic layer rename** (`semantic_layer_before_update`): the same pattern for loaded semantic views. The fix records the values the Core `UPDATE` wrote on the loaded instances with `set_committed_value`. Attributes that are not loaded, or that carry a pending change, are left alone. `_update_vm_datasources_access` is not changed: the database rename path reads the rewritten perms back from the table after it runs. Not changed here: a database renamed through asset import keeps the old database name in the `schema_perm` of its datasets and charts, because import does not run the schema permission sync. That is a separate gap, not this stale-instance problem. Related: https://github.com/apache/superset/pull/45864 changes `_update_vm_datasources_access`; this PR does not touch those lines. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF Not applicable (backend only). ### TESTING INSTRUCTIONS New tests, each failing before this change (commit 841336653cbf2cd01220e4de9a5ddf070b51200e with `superset/security/manager.py` reverted) and passing after: - `tests/integration_tests/security/perm_sync_after_rename_tests.py::TestDatabaseRenamePermissions::test_rename_database_with_loaded_datasets_keeps_chart_perms_in_sync`: real `PUT /api/v1/database/<id>` with a dataset with a schema and one without; asserts every chart's `perm`/`schema_perm` uses the new name and that a viewer with only the dataset grant gets 200 on `GET /api/v1/chart/<id>/data/`. - `TestDatasetChangePermissions::test_table_rename_then_chart_write`, `test_schema_change_then_chart_write`, `test_database_change_then_chart_write` (same file). - `tests/integration_tests/commands_test.py::TestImportAssetsCommand::test_import_assets_dataset_change_keeps_chart_perms`. - `tests/unit_tests/semantic_layers/models_test.py::test_semantic_layer_rename_then_chart_write_keeps_slice_perm`. `TestDatabaseRenamePermissions::test_rename_database_keeps_chart_perms_in_sync` covers the same PUT from a fresh session and passes before and after. ``` pytest tests/integration_tests/security/perm_sync_after_rename_tests.py \ tests/integration_tests/commands_test.py::TestImportAssetsCommand pytest tests/unit_tests/semantic_layers/models_test.py -k rename ``` ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- 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]
