aminghadersohi commented on code in PR #43783: URL: https://github.com/apache/superset/pull/43783#discussion_r3926690837
########## superset/migrations/versions/2026-09-03_00-00_b3e9c1a75d24_retire_semantic_layer_views_connections_pvms.py: ########## @@ -0,0 +1,107 @@ +# 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. +"""retire stale can_views / can_connections PVMs on the SemanticLayer view menu + +The ``views`` (POST ``/<uuid>/views``) and ``connections`` (GET +``/connections/``) methods on ``SemanticLayerRestApi`` are now mapped to +``can_read`` in ``method_permission_name``. Earlier builds left those methods +unmapped, so Flask-AppBuilder derived ``can_views`` and ``can_connections`` +permission-view-menus (PVMs) on the ``SemanticLayer`` view menu. Current code +can no longer create them, and once ``SemanticLayer`` joined +``READ_ONLY_MODEL_VIEWS`` those two permissions would have been withheld from +everyone but Admin anyway. + +This migration migrates any role holding the stale PVMs onto the live +``can_read`` PVM, then removes the stale rows. On a clean install (or one where +the default-off ``SEMANTIC_LAYERS`` flag was never enabled, so the API was +never registered and the PVMs never existed) ``migrate_roles`` resolves the old +PVMs to ``None`` and this is a no-op, so it is safe to run everywhere. + +Revision ID: b3e9c1a75d24 +Revises: 8f31c5d726ab +Create Date: 2026-09-03 00:00:00.000000 + +""" + +# revision identifiers, used by Alembic. +revision = "b3e9c1a75d24" +down_revision = "8f31c5d726ab" + +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, +) + +VIEW_MENU = "SemanticLayer" + +# The live permission the two read endpoints now use. Ensure it exists before +# reassigning roles to it so the lookup in ``migrate_roles`` cannot resolve to +# ``None`` (FAB normally creates it on startup once the API is registered). +NEW_PVMS = {VIEW_MENU: ("can_read",)} + +# Map each stale PVM to the live ``can_read``. ``migrate_roles`` will, for every +# role holding a stale PVM: add ``can_read`` (if missing), remove the stale PVM, +# then delete the stale PVM row. The stale permission rows and the view menu are +# only deleted by the helper if they become orphans afterwards. +PVM_MAP = { + Pvm(VIEW_MENU, "can_views"): (Pvm(VIEW_MENU, "can_read"),), Review Comment: This remap grants `can_read` to every role that held `can_views` or `can_connections`, including roles that deliberately did **not** have `can_read`. That's a small permission widening, and UPDATING.md describes the migration only as retiring "now-unused" permissions. Executed, with a custom role holding `can_views` alone: ``` before: role ViewsOnly: ['can_views'] after : role ViewsOnly: ['can_read'] ``` `can_read` on `SemanticLayer` gates `GET /semantic_layer/` and `GET /semantic_layer/<uuid>` — the layer list and detail, including the masked configuration — which `ViewsOnly` could not previously reach. This is inherent to the `security_converge` rename pattern and I think it's the right trade (the alternative strands those roles with no access at all), so I'd not change the code. But it is a role gaining a permission during an upgrade, and the UPDATING.md entry should say so rather than implying the migration only deletes rows — operators who hand-rolled semantic-layer roles will want to re-audit them. ########## superset/migrations/versions/2026-09-03_00-00_b3e9c1a75d24_retire_semantic_layer_views_connections_pvms.py: ########## @@ -0,0 +1,107 @@ +# 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. +"""retire stale can_views / can_connections PVMs on the SemanticLayer view menu + +The ``views`` (POST ``/<uuid>/views``) and ``connections`` (GET +``/connections/``) methods on ``SemanticLayerRestApi`` are now mapped to +``can_read`` in ``method_permission_name``. Earlier builds left those methods +unmapped, so Flask-AppBuilder derived ``can_views`` and ``can_connections`` +permission-view-menus (PVMs) on the ``SemanticLayer`` view menu. Current code +can no longer create them, and once ``SemanticLayer`` joined +``READ_ONLY_MODEL_VIEWS`` those two permissions would have been withheld from +everyone but Admin anyway. + +This migration migrates any role holding the stale PVMs onto the live +``can_read`` PVM, then removes the stale rows. On a clean install (or one where +the default-off ``SEMANTIC_LAYERS`` flag was never enabled, so the API was +never registered and the PVMs never existed) ``migrate_roles`` resolves the old +PVMs to ``None`` and this is a no-op, so it is safe to run everywhere. + +Revision ID: b3e9c1a75d24 +Revises: 8f31c5d726ab +Create Date: 2026-09-03 00:00:00.000000 + +""" + +# revision identifiers, used by Alembic. +revision = "b3e9c1a75d24" +down_revision = "8f31c5d726ab" + +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, +) + +VIEW_MENU = "SemanticLayer" + +# The live permission the two read endpoints now use. Ensure it exists before +# reassigning roles to it so the lookup in ``migrate_roles`` cannot resolve to +# ``None`` (FAB normally creates it on startup once the API is registered). +NEW_PVMS = {VIEW_MENU: ("can_read",)} + +# Map each stale PVM to the live ``can_read``. ``migrate_roles`` will, for every +# role holding a stale PVM: add ``can_read`` (if missing), remove the stale PVM, +# then delete the stale PVM row. The stale permission rows and the view menu are +# only deleted by the helper if they become orphans afterwards. +PVM_MAP = { + Pvm(VIEW_MENU, "can_views"): (Pvm(VIEW_MENU, "can_read"),), + Pvm(VIEW_MENU, "can_connections"): (Pvm(VIEW_MENU, "can_read"),), +} + + +def do_upgrade(session: Session) -> None: + add_pvms(session, NEW_PVMS) Review Comment: `add_pvms` runs unconditionally, so this migration is **not** a no-op on a clean install — which is what the docstring above (lines 29-32) rests its "safe to run everywhere" argument on. Executed against SQLite with an empty permission table (no `SemanticLayer` view menu, no PVMs — i.e. a fresh install, or any install where the default-off `SEMANTIC_LAYERS` flag meant the API was never registered): ``` before: ViewMenu SemanticLayer exists: False after : ViewMenu SemanticLayer exists: True after : PVMs = ['can_read'] ``` `migrate_roles` *is* a no-op there, exactly as the docstring says. `add_pvms` isn't: it creates the view menu and the `can_read` PVM. `sync_role_definitions` will then grant that to Gamma for a feature whose routes aren't registered. Harmless for access — the endpoints 404 with the flag off — but these are the rows FAB's `security_cleanup` treats as stale, and the docstring's safety claim isn't quite true as written. `add_pvms` only exists here to give `migrate_roles` a resolvable target, so guarding it on the stale PVMs actually being present would make the no-op claim literally correct — something like `if _find_pvm(session, VIEW_MENU, "can_views") or _find_pvm(session, VIEW_MENU, "can_connections"):` around both calls, adding `_find_pvm` to the existing `security_converge` import. Or just reword the docstring: the behaviour is benign, it's the stated justification that's off. -- 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]
