rebenitez1802 commented on PR #44142:
URL: https://github.com/apache/superset/pull/44142#issuecomment-5677373630
Request changes: genuinely high-quality migration — helper logic is sound,
the security model is intact (no privilege escalation/loss in any of the 12
renames), the alembic chain is clean (single head, chains correctly), and every
one of the 25 deletions I could confirm dead *is* dead. But two entries carry
the wrong permission name and silently no-op, and one of those loses a
custom-role grant it was meant to preserve. Note: despite the `fix(security)`
title, none of these are security-boundary issues — the worst real impact is
incomplete cleanup + one dropped grant.
🟡 Medium — `can_sqllab_table_viz` is deleted, but the rename that should
preserve it targets a phantom name
In the delete migration, `Pvm("Superset", "can_sqllab_table_viz")` (line
~107) is treated as a no-successor deletion. But that permission was generated
by `def sqllab_table_viz` (route `/get_or_create_table/`), which was
`@deprecated(new_target="api/v1/dataset/get_or_create/")` — its capability
*moved* to the live `("Dataset","can_get_or_create_dataset")`, it didn't
disappear. Meanwhile the rename migration maps
`("Superset","can_get_or_create_table") →
("Dataset","can_get_or_create_dataset")`, but no method named
`get_or_create_table` ever existed (the method was always `sqllab_table_viz`),
so `can_get_or_create_table` was never a real PVM and that rename is a no-op on
every real DB. Net effect: custom roles granted `can_sqllab_table_viz` silently
lose it instead of being migrated to `can_get_or_create_dataset`. Fix: remove
`can_sqllab_table_viz` from `PVM_LIST` and replace the phantom
`can_get_or_create_table` rename entry with
`Pvm("Superset","can_sqllab_table_viz"): (Pvm("
Dataset","can_get_or_create_dataset"),)`.
🟡 Medium — `can_test_conn` deletion is a phantom; the real leftover
permission is `can_testconn`
`PVM_LIST` deletes `Pvm("Superset", "can_test_conn")` (line ~87), but the
old method was `def testconn` (route `/testconn`, `@deprecated`, no
`@permission_name` override — verified across history; `def test_conn` never
existed), so FAB generated `("Superset","can_testconn")`, without the
underscore. The `can_test_conn` spelling only ever lived as a hardcoded string
in `SQLLAB_ONLY_PERMISSIONS` (which this PR correctly removes) and appears
never to have matched a real PVM. So on upgraded DBs this entry deletes nothing
while the genuinely-dead `can_testconn` — the exact kind of leftover #33272 is
about — survives. Fix: change the entry to `Pvm("Superset", "can_testconn")`.
More broadly, these two phantoms suggest some names were derived from route
paths / the manager.py string rather than the actual FAB method names — worth
re-deriving `PVM_LIST`/`PVM_MAP` against a real pre-3.0 `ab_permission` dump.
🟢 Low — Cleanup is incomplete; docstring over-claims an exhaustive sweep
Four other permissions from `@expose`'d methods on the same monolithic
`Superset` view, removed in the same 3.0-era API-ification, are in neither list
and will persist exactly like the targeted ones:
`can_override_role_permissions` and `can_theme` (pure deletes — `theme` is
superseded by the live `Theme` view menu), and `can_annotation_json` and
`can_fave_dashboards_by_username` (renames paralleling the already-handled
`can_slice_json` and `can_fave_dashboards`/favorite_status → `can_read`). The
delete migration's docstring claims each entry was confirmed "by cross-checking
current views/API code, the full migrations history, and UPDATING.md," implying
completeness. Fix: add these four (or, at minimum, soften the docstring so it
doesn't assert an exhaustive sweep).
🟢 Low — Stale MCP doc reference to a removed permission
`superset/mcp_service/SECURITY.md:405` documents `execute_sql` as requiring
`can_sql_json`, which this PR renames to `can_execute_sql_query`. The file
isn't touched by the PR (and the row was already partly wrong — it says view
menu "Database"), but after this migration the reference is fully dangling; the
code already gates on `execute_sql_query`/`SQLLab`. Fix: update the row to
`can_execute_sql_query` on `SQLLab`.
🟢 Low — Missing regression guard for the PVM_LIST / PVM_MAP disjointness
invariant
The whole two-migration design relies on the 25 deleted pairs and 12 rename
keys being disjoint (an overlap would be order-dependent and could permanently
drop grants). It currently holds, and both constants are already imported
side-by-side in `test_deprecated_permissions_33272.py`, so the guard is one
line. Fix: add `assert set(PVM_LIST).isdisjoint(PVM_MAP.keys())` (and
optionally that no `PVM_LIST` entry is a `NEW_PVMS` successor).
🟢 Low — No test that a role keeps its *other* permissions after a
delete/rename
`delete_pvms`/`migrate_roles` do a targeted
`role.permissions.remove(old_pvm)`, but every role-bearing test seeds the role
with only the single targeted PVM, so `assert role.permissions == []` can't
distinguish "removed the target" from "cleared everything." Fix: add a case
seeding a role with `[targeted_pvm, untargeted_pvm]` and assert the untargeted
one survives (and, for rename, that the successor was added alongside it).
🟢 Low — Test polish (nits)
Two symmetry gaps, both cheap: no end-to-end test runs *both* chained
migrations on one DB (each test exercises one migration in isolation — the
migrations are provably non-interacting, so this is completeness only), and
rename-path idempotency is untested while the delete path has an explicit
twice-run test. Fix: add a chained-run test and a rename idempotency test
mirroring `test_delete_migration_is_idempotent_with_real_pvm_list`.
--
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]
