gabotorresruiz commented on code in PR #44142: URL: https://github.com/apache/superset/pull/44142#discussion_r4020495273
########## superset/migrations/versions/2026-09-10_00-01_3ce9a4572f8a_rename_deprecated_permissions_33272.py: ########## @@ -0,0 +1,206 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""migrate roles off deprecated, renamed permissions (#33272) + +Companion to the previous migration (which deletes deprecated permissions +confirmed to have no successor). This one handles the other half of +apache/superset#33272: permissions that were deprecated but effectively +renamed/consolidated onto a permission that already exists on current master, +almost always as part of the API-ification of the old ``Superset`` monolithic +view (``superset/views/core.py``) into per-resource ``ModelRestApi``s. + +``PVM_MAP`` started as an 11-entry list. Two follow-up investigation passes +found: (1) 19 permissions originally believed to be pure deletions (in the +delete migration's earlier draft) actually have a verified live successor and +belong here instead; (2) the original ``can_get_or_create_table`` key was +itself a phantom -- no method named ``get_or_create_table`` was ever +generated by FAB, so it never matched any real historical permission and +would have silently no-op'd, leaving its intended target +(``can_sqllab_table_viz``, the real permission for ``def sqllab_table_viz`` / +route ``/get_or_create_table/``) to be deleted with no migration; and (3) two +more genuinely-real permissions (``can_annotation_json``, +``can_fave_dashboards_by_username``) were missing from both lists entirely. +All three are fixed below. + +Two entries rest on strong-but-not-decorator-level evidence rather than a +formal ``@deprecated(new_target=...)`` marker: ``can_my_queries`` -> both Review Comment: Just a small NIT: the docstrings here and in the delete migration narrate this PR's own draft evolution (the 25-entry earlier draft, the 11-entry original map). Since no migration with those lists ever shipped, a future reader may go hunting for one. Consider trimming to just the verified-evidence summary; the per-entry comments in `PVM_LIST`/`PVM_MAP` already carry the important context. ########## superset/migrations/versions/2026-09-10_00-01_3ce9a4572f8a_rename_deprecated_permissions_33272.py: ########## @@ -0,0 +1,206 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""migrate roles off deprecated, renamed permissions (#33272) + +Companion to the previous migration (which deletes deprecated permissions +confirmed to have no successor). This one handles the other half of +apache/superset#33272: permissions that were deprecated but effectively +renamed/consolidated onto a permission that already exists on current master, +almost always as part of the API-ification of the old ``Superset`` monolithic +view (``superset/views/core.py``) into per-resource ``ModelRestApi``s. + +``PVM_MAP`` started as an 11-entry list. Two follow-up investigation passes +found: (1) 19 permissions originally believed to be pure deletions (in the +delete migration's earlier draft) actually have a verified live successor and +belong here instead; (2) the original ``can_get_or_create_table`` key was +itself a phantom -- no method named ``get_or_create_table`` was ever +generated by FAB, so it never matched any real historical permission and +would have silently no-op'd, leaving its intended target +(``can_sqllab_table_viz``, the real permission for ``def sqllab_table_viz`` / +route ``/get_or_create_table/``) to be deleted with no migration; and (3) two +more genuinely-real permissions (``can_annotation_json``, +``can_fave_dashboards_by_username``) were missing from both lists entirely. +All three are fixed below. + +Two entries rest on strong-but-not-decorator-level evidence rather than a +formal ``@deprecated(new_target=...)`` marker: ``can_my_queries`` -> both +``SavedQuery.can_list`` (the old endpoint's redirect target, gating the page +shell) and ``SavedQuery.can_read`` (gating the ``/api/v1/saved_query/`` list +call the page actually renders -- without it, a migrated role gets a page +with no visible data) -- and +``can_import_dashboards`` -> ``Dashboard.can_write`` (a same-commit +functional replacement) -- flagged here for reviewer awareness, not hidden. + +Each mapping was verified against a genuine live ``superset init`` DB (not +just ``UPDATING.md`` wording, which is wrong for the ``can_sql_json`` and +``can_results`` rows -- it names a non-existent "can_execute"/"can_results" +permission; the correct live successors, confirmed by querying +``ab_permission``/``ab_permission_view``/``ab_view_menu``, are +``can_execute_sql_query`` and ``can_get_results`` on the ``SQLLab`` view +menu). + +This uses the existing, unmodified ``migrate_roles``/``add_pvms`` helpers: +``add_pvms`` first guarantees every successor PVM exists so ``migrate_roles`` +can never resolve a target to ``None``, then ``migrate_roles`` moves any role +holding an old PVM onto its successor(s) and deletes the old PVM (and its +Permission/ViewMenu, if now orphaned). Chained after the deletion migration, +though the two touch disjoint PVMs and their relative order does not matter. + +Revision ID: 3ce9a4572f8a +Revises: 1f5f4fb8bfc1 +Create Date: 2026-09-10 00:01:00.000000 + +""" + +# revision identifiers, used by Alembic. +revision = "3ce9a4572f8a" +down_revision = "1f5f4fb8bfc1" + +from alembic import op # noqa: E402 +from sqlalchemy.exc import SQLAlchemyError # noqa: E402 +from sqlalchemy.orm import Session # noqa: E402 + +from superset.migrations.shared.security_converge import ( # noqa: E402 + add_pvms, + migrate_roles, + Pvm, +) + +OLD_VIEW_MENU = "Superset" + +# The live permissions these old ones are being folded into. Ensured present +# before `migrate_roles` runs so lookups can't resolve to None (they normally +# already exist -- FAB creates them on startup -- this just makes the +# migration self-contained). +NEW_PVMS = { + "Chart": ("can_read", "can_write"), + "Query": ("can_read",), + "Database": ("can_read", "can_write"), + "Datasource": ("can_get_column_values",), + "Log": ("can_recent_activity",), + "Dataset": ("can_get_or_create_dataset", "can_read", "can_write"), + "SQLLab": ("can_estimate_query_cost", "can_execute_sql_query", "can_get_results"), + "Dashboard": ("can_read", "can_write"), + "AvailableDomains": ("can_read",), + "SecurityRestApi": ("can_read",), + "SavedQuery": ("can_list", "can_read"), +} + +# Map each deprecated (view, permission) to its live successor(s). +# `migrate_roles` will, for every role holding an old PVM: add the successor +# (if missing), remove the old PVM, then delete the old PVM row -- and the +# underlying Permission/ViewMenu too, if they become orphans. +PVM_MAP = { + # Unchanged from the original migration -- already correct. + Pvm(OLD_VIEW_MENU, "can_explore_json"): (Pvm("Chart", "can_read"),), + Pvm(OLD_VIEW_MENU, "can_slice_json"): (Pvm("Chart", "can_read"),), + Pvm(OLD_VIEW_MENU, "can_search_queries"): (Pvm("Query", "can_read"),), + Pvm(OLD_VIEW_MENU, "can_tables"): (Pvm("Database", "can_read"),), + Pvm(OLD_VIEW_MENU, "can_queries"): (Pvm("Query", "can_read"),), + Pvm(OLD_VIEW_MENU, "can_filter"): (Pvm("Datasource", "can_get_column_values"),), + Pvm(OLD_VIEW_MENU, "can_recent_activity"): (Pvm("Log", "can_recent_activity"),), + Pvm(OLD_VIEW_MENU, "can_stop_query"): (Pvm("Query", "can_read"),), + Pvm(OLD_VIEW_MENU, "can_estimate_query_cost"): ( + Pvm("SQLLab", "can_estimate_query_cost"), + ), + Pvm(OLD_VIEW_MENU, "can_sql_json"): (Pvm("SQLLab", "can_execute_sql_query"),), + Pvm(OLD_VIEW_MENU, "can_results"): (Pvm("SQLLab", "can_get_results"),), + # Corrected: the old key here was "can_get_or_create_table", a permission + # name that never actually existed (no method named get_or_create_table + # was ever generated by FAB) -- the real historical permission for this + # capability was "can_sqllab_table_viz" (def sqllab_table_viz, route + # /get_or_create_table/, PR #22931/#24375). + Pvm(OLD_VIEW_MENU, "can_sqllab_table_viz"): ( + Pvm("Dataset", "can_get_or_create_dataset"), + ), + # Reclassified from the delete migration: each of these was originally + # believed to have no successor, but actually has a verified live one + # (PR body / @deprecated(new_target=...) evidence -- see file docstring). + Pvm(OLD_VIEW_MENU, "can_select_star"): (Pvm("Database", "can_read"),), + Pvm(OLD_VIEW_MENU, "can_testconn"): (Pvm("Database", "can_write"),), Review Comment: A security-awareness question, not a blocker: for the three write-level successors (`can_testconn` to `Database.can_write` here, plus `can_sqllab_viz` to `Dataset.can_write` and `can_import_dashboards` to `Dashboard.can_write` below), the migration grants a strictly broader capability than the role has actually had since 3.0. I verified it on this branch: a custom role holding only the dead `can_testconn` (no live capability at all today, the endpoint is gone) comes out of the migration holding `Database.can_write`, which covers creating, editing and deleting database connections, not just testing one. The `@deprecated(new_target="/api/v1/database/test_connection/")` decorator does sanction the mapping, and the read-level rows all look clearly right to me. Are we comfortable resurrecting years-dead grants as live write permissions, or would moving these three rows to the delete migration be the safer default? UPDATING.md already told operators to re-add the successors to custom ro les for these endpoints, so deletion would not strand anyone who followed it. -- 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]
