Ujjwaljain16 opened a new pull request, #44142: URL: https://github.com/apache/superset/pull/44142
### SUMMARY Deprecated permissions are never cleaned up after an upgrade (#33272). `superset init` → `sync_role_definitions` creates missing permissions and runs `clean_perms`, but `clean_perms` only ever deletes `PermissionView` rows whose `Permission`/`ViewMenu` foreign keys are `NULL` — it never removes well-formed PVMs whose underlying endpoint/method has since been deleted from the codebase. `can_select_star` (the reporter's example) is one of many such permissions that silently accumulate on every upgraded install while never appearing on a fresh one. The maintainer thread on the issue correctly flagged the risk here: a naive "delete everything not in a hardcoded current-permissions allow-list" approach could delete permissions a custom role or an installed extension still legitimately needs, since Superset has no built-in way to distinguish "genuinely dead" from "just not used by a built-in role." This PR deliberately avoids that failure mode: - **No generic sweep, no allow-list.** Both migrations target an explicit, individually-verified list of `(view_menu, permission)` pairs. There is no "find by permission name" logic anywhere, so the per-object `database_access`/`datasource_access`/`schema_access`/`catalog_access` permissions — which are legitimately reused across many view menus (one per Database/SqlaTable/schema/catalog) — are structurally unreachable by this code, not just excluded by convention. - **Every candidate was individually verified**, not inferred from "absent from a live DB." Each of the 25 pure-deletion candidates was cross-checked against current `superset/views`/API code, the entire `superset/migrations/` history (to rule out double-handling), and `UPDATING.md`'s actual PR history for the underlying feature removal. Anything without affirmative "removed, not renamed" evidence was left out, even when it looked plausible. - **Renames preserve role assignments.** 12 permissions were deprecated by being consolidated onto a permission that already exists on current master (mostly the `ModelRestApi` migration of the old monolithic `Superset` view — e.g. `can_explore_json` → `can_read` on `Chart`). These use the existing `migrate_roles()` machinery unmodified, so a role holding the old permission is moved onto the verified live successor rather than just losing it. Two of these mappings also correct an actual inaccuracy in `UPDATING.md` itself (`can_sql_json`/`can_results`'s stated successors don't exist in a live DB; the real ones, confirmed by querying `ab_permission`/`ab_permission_view`, are `can_execute_sql_query`/`can_get_results` on `SQLLab`). Implementation-wise, `migrate_roles()` can't express a pure deletion with no successor (an empty replacement tuple is silently never processed — confirmed by reading the function), so this adds one small new helper, `delete_pvms()`, to the existing `superset/migrations/shared/security_converge.py` module. It reuses `_delete_old_permissions()`'s existing orphan-safe deletion logic (delete the `PermissionView`, then the `Permission`/`ViewMenu` only if no other PVM still references them) rather than duplicating it, and does its own role-unassignment pass first. `migrate_roles()`, `clean_perms()`, and FAB's `security_cleanup()` are all untouched. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF Not applicable — this is a backend/migration-only change with no UI impact. ### TESTING INSTRUCTIONS - `pytest tests/unit_tests/migrations/shared/security_converge_test.py` — 7 tests covering `delete_pvms()` directly: removal + role-association cleanup, orphan deletion, shared-permission preservation, idempotency (running twice is a safe no-op), and that a dynamic/object-style permission is left untouched because it's simply never in the explicit input list. - `pytest tests/unit_tests/migrations/test_deprecated_permissions_33272.py` — 4 tests that import the two actual migration modules and exercise their real `PVM_LIST`/`PVM_MAP` (not hand-copied subsets), so editing either list in the future automatically re-exercises the updated data. - `pytest tests/unit_tests/migrations/` — full suite, 154 passed, no regressions. - Manually verified end-to-end on a disposable database: `superset db upgrade` from a fresh install runs both migrations cleanly and lands on a single new head with no branching; `superset db downgrade` back past both migrations also completes without error (both `downgrade()`s are intentionally documented no-ops, since there's nothing meaningful to restore for a pure deletion, and reversing the renames would strip active permissions from roles). - `ruff format --check` / `ruff check` clean on all changed files. ### ADDITIONAL INFORMATION - [x] Has associated issue: Fixes #33272 - [ ] Required feature flags: - [ ] Changes UI - [x] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [x] Migration is atomic, supports rollback & is backwards-compatible - [x] Confirm DB migration upgrade and downgrade tested - [x] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API Runtime/downtime note: both migrations only touch `ab_permission`/`ab_view_menu`/`ab_permission_view`/`ab_permission_view_role` rows for a fixed, small (37 total) set of named permissions — not a table scan — so runtime is negligible regardless of instance size, and no downtime is expected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
