sadpandajoe commented on code in PR #45894: URL: https://github.com/apache/superset/pull/45894#discussion_r4450928005
########## superset/migrations/versions/2026-09-28_00-00_4c8f2a1d9b73_add_extension_storage_access.py: ########## @@ -0,0 +1,95 @@ +# 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. +"""Add browser access classification to extension storage. + +Revision ID: 4c8f2a1d9b73 +Revises: f72b6e9c104a +Create Date: 2026-09-28 00:00:00.000000 +""" + +import sqlalchemy as sa +from alembic import op + +revision = "4c8f2a1d9b73" +down_revision = "f72b6e9c104a" Review Comment: This revision still names `f72b6e9c104a` as its parent, but master's current head is `b8f3a2c91d64` (`migrate_databricks_connector_uri`), which also revises `f72b6e9c104a`. Merged as-is there are two Alembic heads and `superset db upgrade` fails with a multiple-heads error. Can this be rebased onto `b8f3a2c91d64` (or given a merge revision) before it lands? ########## superset/extensions/storage/secrets.py: ########## @@ -0,0 +1,78 @@ +# 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. + +"""Host implementation of backend-only extension secrets storage.""" + +from __future__ import annotations + +from typing import Any + +from superset_core.extensions.storage.models import StorageAccess +from superset_core.extensions.storage.secrets import SecretsState as CoreSecretsState + +from superset.extensions.storage.codecs import DEFAULT_CODEC, get_codec +from superset.extensions.storage.persistent_dao import ExtensionStorageDAO +from superset.extensions.storage.utils import ( + get_current_extension_id, + get_current_user_id, +) +from superset.utils.decorators import transaction + + +class SecretsState(CoreSecretsState): + """Encrypted persistent secrets scoped to the current extension and user.""" + + @staticmethod + def get(key: str) -> Any: + """Return the current user's decoded secret, or ``None`` when absent.""" + extension_id = get_current_extension_id("secrets_state") + user_id = get_current_user_id("secrets_state") + return ExtensionStorageDAO.get_decoded_value( + extension_id, + key, + user_fk=user_id, + access=StorageAccess.BACKEND, + ) + + @staticmethod + @transaction() Review Comment: `@transaction()` joins an already-open transaction rather than committing on its own, so a token saved here lives or dies with the caller's unit of work. `CreateSemanticViewCommand.run()` is one such transaction: if provider discovery in `validate()` rotates a single-use refresh token, saves it via `secrets.set`, and the lookup then fails (or the post-lookup uniqueness check raises), the rollback discards the new token while the provider has already invalidated the old one, so the user has to reconnect. `refresh_oauth2_token` in `superset/utils/oauth2.py` uses an isolated session for this reason. Should the secret write be independently durable for rotated tokens? ########## superset/semantic_layers/registry.py: ########## @@ -17,8 +17,264 @@ from __future__ import annotations -from typing import Any +from contextlib import contextmanager, nullcontext +from functools import wraps +from typing import Any, Iterator +from superset_core.extensions.types import Manifest from superset_core.semantic_layers.layer import SemanticLayer +from superset.extensions.context import ( + extension_context, + get_current_extension_context, +) +from superset.semantic_layers.oauth2 import check_for_oauth2 +from superset.utils.core import get_user_id + registry: dict[str, type[SemanticLayer[Any, Any]]] = {} +_extension_manifests: dict[str, Manifest] = {} + + +def register_semantic_layer( + semantic_layer_id: str, + implementation: type[SemanticLayer[Any, Any]], + manifest: Manifest | None = None, +) -> None: + """Register a provider and retain the extension that owns it.""" + registry[semantic_layer_id] = implementation + if manifest is None: + _extension_manifests.pop(semantic_layer_id, None) + else: + _extension_manifests[semantic_layer_id] = manifest + + +def unregister_semantic_layer(semantic_layer_id: str) -> None: + """Remove a provider and any retained extension metadata.""" + registry.pop(semantic_layer_id, None) + _extension_manifests.pop(semantic_layer_id, None) + + +@contextmanager +def semantic_layer_context( + semantic_layer_id: str, + layer_uuid: str | None = None, + view_uuid: str | None = None, +) -> Iterator[None]: + """Run provider code in its owning extension and principal context.""" + manifest = _extension_manifests.get(semantic_layer_id) + current_context = get_current_extension_context() + user_id = ( + current_context.user_id + if current_context is not None and current_context.user_id is not None + else get_user_id() + ) + context = ( + extension_context(manifest, user_id=user_id) + if manifest is not None + else nullcontext() + ) + with check_for_oauth2(layer_uuid, view_uuid), context: + yield + + +def _contextual_attribute( + semantic_layer_id: str, + implementation: Any, + name: str, + layer_uuid: str | None, + view_uuid: str | None, +) -> Any: + """Keep both property access and fallback method calls in provider context.""" + with semantic_layer_context(semantic_layer_id, layer_uuid, view_uuid): + attribute = getattr(implementation, name) + if not callable(attribute): + return attribute + + @wraps(attribute) + def contextualized(*args: Any, **kwargs: Any) -> Any: + with semantic_layer_context(semantic_layer_id, layer_uuid, view_uuid): + return attribute(*args, **kwargs) + + return contextualized + + +class ContextualSemanticView: + """Forward semantic view operations inside their extension context.""" + + def __init__( + self, + semantic_layer_id: str, + implementation: Any, + layer_uuid: str | None = None, + view_uuid: str | None = None, + ): + self._semantic_layer_id = semantic_layer_id + self._implementation = implementation + self._layer_uuid = layer_uuid + self._view_uuid = view_uuid + + def __getattr__(self, name: str) -> Any: + return _contextual_attribute( + self._semantic_layer_id, + self._implementation, + name, + self._layer_uuid, + self._view_uuid, + ) + + def __hash__(self) -> int: + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): + return hash(self._implementation) + + def __eq__(self, other: object) -> bool: + if isinstance(other, ContextualSemanticView): + other = other._implementation + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): + return self._implementation == other + + def uid(self) -> str: + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): + return self._implementation.uid() + + def get_dimensions(self) -> Any: + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): + return self._implementation.get_dimensions() + + def get_metrics(self) -> Any: + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): + return self._implementation.get_metrics() + + def get_values(self, dimension: Any, filters: Any = None) -> Any: Review Comment: `get_values` now raises `SemanticLayerOAuth2RedirectError` when the personal grant is missing, but `DatasourceRestApi.get_column_values` only catches the security and execution errors under FAB's `@safe`, so on a cache miss this surfaces as a generic HTTP 500 and the authorization URL and `tab_id` are lost. Opening filter suggestions on a semantic view the user hasn't authorized then shows an error instead of the authorize prompt; `/compatible` has the same gap. Should those endpoints return the structured 403 the way Explore and discovery do? ########## superset/extensions/storage/persistent_dao.py: ########## @@ -292,11 +312,13 @@ def _scope_filter( user_fk: int | None = None, resource_type: str | None = None, resource_uuid: str | None = None, + access: StorageAccess = StorageAccess.FRONTEND, ) -> list[object]: """Build the SQLAlchemy filter list for a scoped lookup.""" filters: list[object] = [ ExtensionStorage.extension_id == extension_id, ExtensionStorage.key == key, + ExtensionStorage.access == access.value, Review Comment: The isolation between a backend and a frontend row with the same extension, user, scope and key rests on this new `access` predicate, but nothing exercises it: the `get`/`set`/`delete` tests in `test_persistent_dao.py` mock `db.session.query(...).filter(...).first()`, and `test_secrets.py` mocks the DAO, while the only real-query test (`test_list_scopes_by_access`) covers `_list_scope_filter`, not `_scope_filter`. If this line were dropped, a frontend write or delete would hit the backend token row and no test would fail. Could a test on the existing SQLite `list_session` fixture seed both rows and assert that frontend get/set/delete leave the backend row, its value and its classification untouched? ########## superset/semantic_layers/registry.py: ########## @@ -17,8 +17,264 @@ from __future__ import annotations -from typing import Any +from contextlib import contextmanager, nullcontext +from functools import wraps +from typing import Any, Iterator +from superset_core.extensions.types import Manifest from superset_core.semantic_layers.layer import SemanticLayer +from superset.extensions.context import ( + extension_context, + get_current_extension_context, +) +from superset.semantic_layers.oauth2 import check_for_oauth2 +from superset.utils.core import get_user_id + registry: dict[str, type[SemanticLayer[Any, Any]]] = {} +_extension_manifests: dict[str, Manifest] = {} + + +def register_semantic_layer( + semantic_layer_id: str, + implementation: type[SemanticLayer[Any, Any]], + manifest: Manifest | None = None, +) -> None: + """Register a provider and retain the extension that owns it.""" + registry[semantic_layer_id] = implementation + if manifest is None: + _extension_manifests.pop(semantic_layer_id, None) + else: + _extension_manifests[semantic_layer_id] = manifest + + +def unregister_semantic_layer(semantic_layer_id: str) -> None: + """Remove a provider and any retained extension metadata.""" + registry.pop(semantic_layer_id, None) + _extension_manifests.pop(semantic_layer_id, None) + + +@contextmanager +def semantic_layer_context( + semantic_layer_id: str, + layer_uuid: str | None = None, + view_uuid: str | None = None, +) -> Iterator[None]: + """Run provider code in its owning extension and principal context.""" + manifest = _extension_manifests.get(semantic_layer_id) + current_context = get_current_extension_context() + user_id = ( + current_context.user_id + if current_context is not None and current_context.user_id is not None + else get_user_id() + ) + context = ( + extension_context(manifest, user_id=user_id) + if manifest is not None + else nullcontext() + ) + with check_for_oauth2(layer_uuid, view_uuid), context: + yield + + +def _contextual_attribute( + semantic_layer_id: str, + implementation: Any, + name: str, + layer_uuid: str | None, + view_uuid: str | None, +) -> Any: + """Keep both property access and fallback method calls in provider context.""" + with semantic_layer_context(semantic_layer_id, layer_uuid, view_uuid): + attribute = getattr(implementation, name) + if not callable(attribute): + return attribute + + @wraps(attribute) + def contextualized(*args: Any, **kwargs: Any) -> Any: + with semantic_layer_context(semantic_layer_id, layer_uuid, view_uuid): + return attribute(*args, **kwargs) + + return contextualized + + +class ContextualSemanticView: + """Forward semantic view operations inside their extension context.""" + + def __init__( + self, + semantic_layer_id: str, + implementation: Any, + layer_uuid: str | None = None, + view_uuid: str | None = None, + ): + self._semantic_layer_id = semantic_layer_id + self._implementation = implementation + self._layer_uuid = layer_uuid + self._view_uuid = view_uuid + + def __getattr__(self, name: str) -> Any: + return _contextual_attribute( + self._semantic_layer_id, + self._implementation, + name, + self._layer_uuid, + self._view_uuid, + ) + + def __hash__(self) -> int: + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): + return hash(self._implementation) + + def __eq__(self, other: object) -> bool: + if isinstance(other, ContextualSemanticView): + other = other._implementation + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): + return self._implementation == other + + def uid(self) -> str: + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): + return self._implementation.uid() + + def get_dimensions(self) -> Any: + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): + return self._implementation.get_dimensions() + + def get_metrics(self) -> Any: + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): + return self._implementation.get_metrics() + + def get_values(self, dimension: Any, filters: Any = None) -> Any: + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): + return self._implementation.get_values(dimension, filters) + + def get_table(self, query: Any) -> Any: + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): + return self._implementation.get_table(query) + + def get_row_count(self, query: Any) -> Any: + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): + return self._implementation.get_row_count(query) + + def get_compatible_metrics( Review Comment: These compatibility calls now run under the requesting user's personal credentials, but the `/compatible` response cache in `superset/datasource/api.py` (the `compatible:` key) is still keyed only on datasource uid, `changed_on`, metadata version and selections, so it never gets the user partition added to the chart and column-values caches. If Alice and Bob have different members under their personal grants, Alice's empty-selection request warms the cache and Bob gets Alice's metric/dimension lists without his own provider ever running (an empty selection also skips member validation). Should the requesting user be part of that key, or should caching be skipped for semantic views with personal auth? -- 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]
