sadpandajoe commented on code in PR #44849: URL: https://github.com/apache/superset/pull/44849#discussion_r4158713040
########## superset/semantic_layers/metadata_cache.py: ########## @@ -0,0 +1,92 @@ +# 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. + +"""Captured catalog identity for derived caches and read-only inspection.""" + +from __future__ import annotations + +import hashlib +from dataclasses import dataclass, field +from typing import TYPE_CHECKING + +from superset_core.semantic_layers.metadata import CatalogSnapshot, MetadataRefreshError + +from superset.semantic_layers.metadata import ScopedMetadataStore +from superset.semantic_layers.metadata_binding import connection_store +from superset.utils import json + +if TYPE_CHECKING: + from superset.semantic_layers.models import SemanticView + + +@dataclass(frozen=True) +class CompatibilityIdentity: + key: str = field(repr=False) + source_observed_at: str | None + + +def view_cache_token(view: SemanticView, token: str) -> str: + """Include host view configuration without revealing it in cache keys.""" + identity: str = json.dumps( + [token, str(view.uuid), view.name, json.loads(view.configuration)], + sort_keys=True, + separators=(",", ":"), + allow_nan=False, + ) + return hashlib.sha256(identity.encode()).hexdigest() + + +def compatibility_identity( + view: SemanticView, + metrics: list[str], + dimensions: list[str], + *, + inspection: bool = False, +) -> CompatibilityIdentity | None: + """Capture identity before lookup; inspection never discovers or initializes.""" + store: ScopedMetadataStore = connection_store(view.semantic_layer) + token: str + generation: str | None + observed: str | None + if inspection: + snapshot: CatalogSnapshot | None = store.peek() + generation = store.peek_compatibility_generation() + if snapshot is None or generation is None: + return None + token, observed = snapshot.cache_token, snapshot.observed_at + else: + captured: str | None = view.implementation.metadata_cache_token + if not captured: + raise MetadataRefreshError("configuration") + token = captured + observed = store.observed_at(token) + generation = store.compatibility_generation() Review Comment: If a request captures a provider view with the old compatibility rules and a clear happens before this generation read, its old answer is cached under the new generation and can be served to subsequent requests. Should we capture the generation before resolving the provider view so an in-flight pre-clear observation cannot refill the cleared namespace? ########## superset/commands/semantic_layer/refresh_metadata.py: ########## @@ -0,0 +1,332 @@ +# 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. +from __future__ import annotations + +from collections.abc import Callable, Iterator +from contextlib import contextmanager +from datetime import datetime, timezone +from typing import Any, Literal +from uuid import UUID + +from flask import current_app, g +from flask_appbuilder.security.sqla.models import User +from sqlalchemy.orm import Session +from superset_core.semantic_layers.layer import SemanticLayer as SemanticLayerABC +from superset_core.semantic_layers.metadata import ( + MetadataRefreshError, + MetadataRefreshResult, +) +from superset_core.semantic_layers.view import SemanticView as SemanticViewABC + +from superset import cache_manager, security_manager +from superset.commands.base import BaseCommand +from superset.commands.semantic_layer.exceptions import ( + SemanticLayerForbiddenError, + SemanticLayerNotFoundError, + SemanticViewNotFoundError, +) +from superset.commands.utils import current_user_can_modify_object +from superset.coordination.deadline_backend import DeadlineRedisBackend +from superset.daos.semantic_layer import SemanticViewDAO +from superset.exceptions import SupersetSecurityException +from superset.extensions import db +from superset.semantic_layers.cache_inspection import CacheEntryInfo, inspect_data_cache +from superset.semantic_layers.metadata import ScopedMetadataStore +from superset.semantic_layers.metadata_binding import ( + connection_metadata_scope, + metadata_refresh_enabled, + operation_deadline, +) +from superset.semantic_layers.metadata_cache import ( + compatibility_identity, + CompatibilityIdentity, +) +from superset.semantic_layers.models import SemanticLayer, SemanticView +from superset.semantic_layers.registry import registry +from superset.utils import json + + +def authorize_metadata_refresh(view: SemanticView) -> None: + """Share one server policy between the affordance and direct mutation.""" + if not metadata_refresh_enabled(): + raise SemanticViewNotFoundError() + user: User | None = getattr(g, "user", None) + if ( + user is None + or user.is_anonymous + or getattr(user, "is_guest_user", False) + or not user.is_active + ): + raise SemanticLayerForbiddenError() + if not all( + security_manager.can_access(action, resource) + for action, resource in ( + ("can_read", "SemanticView"), + ("can_read", "SemanticLayer"), + ("can_write", "SemanticLayer"), + ) + ): + raise SemanticLayerForbiddenError() + layer: SemanticLayer | None = view.semantic_layer + if layer is None: + raise SemanticLayerNotFoundError() + try: + view.raise_for_access() + layer.raise_for_access() + except SupersetSecurityException: + raise SemanticLayerForbiddenError() from None + if not current_user_can_modify_object(layer): + raise SemanticLayerForbiddenError() + provider: type[SemanticLayerABC[Any, SemanticViewABC]] | None = registry.get( + layer.type + ) + if provider is None: + raise MetadataRefreshError("unsupported") + try: + configuration: dict[str, Any] = json.loads(layer.configuration) + supported: bool = provider.supports_metadata_refresh(configuration) + except (ValueError, TypeError): + raise MetadataRefreshError("configuration") from None + if not supported: + raise MetadataRefreshError("unsupported") + connection_metadata_scope(layer) + + +def can_refresh_metadata(view: SemanticView) -> bool: + """Project policy without constructing a provider or consulting its catalog.""" + try: + authorize_metadata_refresh(view) + except ( + SemanticViewNotFoundError, + SemanticLayerNotFoundError, + SemanticLayerForbiddenError, + MetadataRefreshError, + ): + return False + return True + + +def view_binding(view: SemanticView) -> tuple[str, str, str]: + """Capture immutable provider selection, independent of Details drafts.""" + return ( + str(view.semantic_layer_uuid), + view.name, + json.dumps(json.loads(view.configuration), sort_keys=True), + ) + + +@contextmanager +def fresh_refresh_authority(session: Session) -> Iterator[None]: + """Run existing policy against persisted authority without ending request work. + + Security-manager and subject helpers use the request-scoped session and + principal. Rebind those only for this guard so they cannot reuse an earlier + repeatable-read snapshot or cached role membership. The supplied session + owns no writes; the caller closes it. Always restore the request's objects. + """ + original_user: User = g.user + original_session: Session = db.session() + had_login_user: bool = hasattr(g, "_login_user") + original_login_user: Any = getattr(g, "_login_user", None) + if original_user.is_anonymous or getattr(original_user, "is_guest_user", False): + raise SemanticLayerForbiddenError() + user: User | None = session.get(security_manager.user_model, original_user.id) + if user is None or not user.is_active: + raise SemanticLayerForbiddenError() + try: + db.session.registry.set(session) Review Comment: Rebinding the session and principal leaves `request._user_subject_ids` cached from the initial authorization check. With `EXTRA_EDITORS_RESOLVER` granting layer editorship through a role or group, removing that membership during the provider fetch can therefore still pass `_revalidate()` and publish, even when the user is no longer an editor. Should this guard also bypass or refresh the request-level subject cache for the fresh authority check? ########## tests/unit_tests/semantic_layers/refresh_metadata_command_test.py: ########## @@ -0,0 +1,418 @@ +# 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. +from __future__ import annotations + +from collections.abc import Callable, Iterator +from typing import cast +from unittest.mock import MagicMock, Mock, patch +from uuid import UUID + +import pytest +from flask import Flask, g +from superset_core.semantic_layers.metadata import ( + MetadataRefreshError, + MetadataRefreshResult, +) + +MODULE: str = "superset.commands.semantic_layer.refresh_metadata" +VIEW_UUID: UUID = UUID("bd2f07da-c65e-40da-b75e-c62b7cdd67f1") + + [email protected] +def refresh_context(app: Flask) -> Iterator[tuple[Mock, Mock, Mock]]: + """Authorize synthetic records; provider construction is always observable.""" + from superset.commands.semantic_layer import refresh_metadata as module + + layer: Mock = Mock(uuid="connection", type="test", configuration='{"token":"test"}') + view: Mock = Mock( + uuid=VIEW_UUID, semantic_layer_uuid="connection", configuration="{}" + ) + view.name = "full" + view.semantic_layer = layer + provider: Mock = Mock() + provider.supports_metadata_refresh.return_value = True + manager: Mock = Mock() + with ( + app.app_context(), + patch.object( + g, + "user", + Mock(id=1, is_anonymous=False, is_guest_user=False), + create=True, + ), + patch.object(module, "Session", return_value=MagicMock()), + patch.object(module, "operation_deadline", return_value=130.0), + patch.object(module, "metadata_refresh_enabled", return_value=True), + patch.object(module, "security_manager", manager), + patch.object(module, "current_user_can_modify_object", return_value=True), + patch.object(module, "connection_metadata_scope", return_value="scope"), + patch.dict(module.registry, {"test": provider}), + patch.object(module.SemanticViewDAO, "find_by_uuid", return_value=view), + patch.object(module, "guarded_store"), + patch.object(module.db, "session", MagicMock()), + ): + cast( + Mock, module.Session + ).return_value.__enter__.return_value.get.return_value = Mock( + id=1, is_anonymous=False, is_guest_user=False, is_active=True + ) + yield view, provider, manager + + +def test_refresh_uses_stored_connection_after_all_authority_checks( + refresh_context: tuple[Mock, Mock, Mock], +) -> None: + from superset.commands.semantic_layer import refresh_metadata as module + from superset.commands.semantic_layer.refresh_metadata import RefreshMetadataCommand + + view: Mock + provider: Mock + manager: Mock + view, provider, manager = refresh_context + result: MetadataRefreshResult = RefreshMetadataCommand(VIEW_UUID).run() + provider.from_configuration.assert_called_once_with({"token": "test"}) + adapter: Mock = provider.from_configuration.return_value.metadata_refresh + assert result is adapter.refresh.return_value + adapter.bind.assert_called_once_with( + cast(Mock, module.guarded_store).return_value, + deadline=130.0, + ) + adapter.refresh.assert_called_once_with(deadline=130.0) + manager.can_access.assert_any_call("can_read", "SemanticView") + manager.can_access.assert_any_call("can_read", "SemanticLayer") + manager.can_access.assert_any_call("can_write", "SemanticLayer") + view.raise_for_access.assert_called_once() + view.semantic_layer.raise_for_access.assert_called_once() + + [email protected]( + "permission", + [ + ("can_read", "SemanticView"), + ("can_read", "SemanticLayer"), + ("can_write", "SemanticLayer"), + ], +) +def test_denied_permissions_do_no_provider_or_cache_work( + refresh_context: tuple[Mock, Mock, Mock], + permission: tuple[str, str], +) -> None: + from superset.commands.semantic_layer import refresh_metadata as module + from superset.commands.semantic_layer.exceptions import SemanticLayerForbiddenError + + view: Mock + provider: Mock + manager: Mock + view, provider, manager = refresh_context + manager.can_access.side_effect = ( + lambda action, resource: (action, resource) != permission + ) + with pytest.raises(SemanticLayerForbiddenError): + module.RefreshMetadataCommand(VIEW_UUID).run() + assert module.can_refresh_metadata(view) is False + provider.from_configuration.assert_not_called() + cast(Mock, module.guarded_store).assert_not_called() + + +def test_creator_without_edit_authority_cannot_refresh( + refresh_context: tuple[Mock, Mock, Mock], +) -> None: + from superset.commands.semantic_layer import refresh_metadata as module + from superset.commands.semantic_layer.exceptions import SemanticLayerForbiddenError + + view: Mock + provider: Mock + view, provider, _ = refresh_context + with patch.object(module, "current_user_can_modify_object", return_value=False): + with pytest.raises(SemanticLayerForbiddenError): + module.RefreshMetadataCommand(VIEW_UUID).run() + assert module.can_refresh_metadata(view) is False + provider.from_configuration.assert_not_called() + + +def test_disabled_refresh_is_unavailable_without_discovery( + refresh_context: tuple[Mock, Mock, Mock], +) -> None: + from superset.commands.semantic_layer import refresh_metadata as module + from superset.commands.semantic_layer.exceptions import SemanticViewNotFoundError + + view: Mock + provider: Mock + view, provider, _ = refresh_context + with patch.object(module, "metadata_refresh_enabled", return_value=False): + with pytest.raises(SemanticViewNotFoundError): + module.RefreshMetadataCommand(VIEW_UUID).run() + assert module.can_refresh_metadata(view) is False + provider.from_configuration.assert_not_called() + + +def test_unsupported_provider_is_not_constructed( + refresh_context: tuple[Mock, Mock, Mock], +) -> None: + from superset.commands.semantic_layer import refresh_metadata as module + + view: Mock + provider: Mock + view, provider, _ = refresh_context + provider.supports_metadata_refresh.return_value = False + with pytest.raises(MetadataRefreshError, match="unsupported"): + module.RefreshMetadataCommand(VIEW_UUID).run() + assert module.can_refresh_metadata(view) is False + provider.from_configuration.assert_not_called() + + [email protected]( + "changed", ["missing", "connection", "configuration", "name", "authority"] +) +def test_publication_revalidates_view_in_supplied_fresh_session( + refresh_context: tuple[Mock, Mock, Mock], + changed: str, +) -> None: + from superset.commands.semantic_layer import refresh_metadata as module + from superset.commands.semantic_layer.exceptions import SemanticLayerForbiddenError + + view: Mock + provider: Mock + manager: Mock + view, provider, manager = refresh_context + module.RefreshMetadataCommand(VIEW_UUID).run() + guard: Callable[[], None] = cast(Mock, module.guarded_store).call_args.kwargs[ + "before_publish" + ] + fresh: Mock = Mock( + uuid=VIEW_UUID, semantic_layer_uuid="connection", configuration="{}" + ) + fresh.name = "full" + fresh.semantic_layer = view.semantic_layer + session: Mock = cast(Mock, module.Session).return_value.__enter__.return_value + cast(Mock, module.SemanticViewDAO.find_by_uuid).return_value = fresh + if changed == "missing": + cast(Mock, module.SemanticViewDAO.find_by_uuid).return_value = None + elif changed == "connection": + fresh.semantic_layer_uuid = "another" + elif changed == "configuration": + fresh.configuration = '{"metrics": ["old"]}' + elif changed == "name": + fresh.name = "another" + else: + manager.can_access.return_value = False + with pytest.raises((MetadataRefreshError, SemanticLayerForbiddenError)): + guard() + session.commit.assert_not_called() + session.rollback.assert_not_called() + + +def test_incomplete_configuration_is_sanitized_before_construction( + refresh_context: tuple[Mock, Mock, Mock], +) -> None: + from superset.commands.semantic_layer import refresh_metadata as module + + view: Mock + provider: Mock + manager: Mock + view, provider, manager = refresh_context + provider.supports_metadata_refresh.side_effect = ValueError("private configuration") + with pytest.raises(MetadataRefreshError, match="^configuration$"): + module.RefreshMetadataCommand(VIEW_UUID).run() + assert module.can_refresh_metadata(view) is False + provider.from_configuration.assert_not_called() + cast(Mock, module.guarded_store).assert_not_called() + + [email protected]("resource", ["view", "layer"]) +def test_datasource_access_denial_matches_capability_and_prevents_construction( + refresh_context: tuple[Mock, Mock, Mock], resource: str +) -> None: + from superset.commands.semantic_layer import refresh_metadata as module + from superset.commands.semantic_layer.exceptions import SemanticLayerForbiddenError + from superset.errors import ErrorLevel, SupersetError, SupersetErrorType + from superset.exceptions import SupersetSecurityException + + view: Mock + provider: Mock + manager: Mock + view, provider, manager = refresh_context + target: Mock = view if resource == "view" else view.semantic_layer + target.raise_for_access.side_effect = SupersetSecurityException( + SupersetError( + message="controlled denial", + error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR, + level=ErrorLevel.ERROR, + ) + ) + with pytest.raises(SemanticLayerForbiddenError): + module.RefreshMetadataCommand(VIEW_UUID).run() + assert module.can_refresh_metadata(view) is False + provider.from_configuration.assert_not_called() + cast(Mock, module.guarded_store).assert_not_called() + + [email protected]("target", ["view", "layer"]) +def test_missing_stored_target_does_not_construct_provider( + refresh_context: tuple[Mock, Mock, Mock], target: str +) -> None: + from superset.commands.semantic_layer import refresh_metadata as module + from superset.commands.semantic_layer.exceptions import ( + SemanticLayerNotFoundError, + SemanticViewNotFoundError, + ) + + view: Mock + provider: Mock + manager: Mock + view, provider, manager = refresh_context + if target == "view": + cast(Mock, module.SemanticViewDAO.find_by_uuid).return_value = None + else: + view.semantic_layer = None + with pytest.raises((SemanticViewNotFoundError, SemanticLayerNotFoundError)): + module.RefreshMetadataCommand(VIEW_UUID).run() + provider.from_configuration.assert_not_called() + cast(Mock, module.guarded_store).assert_not_called() + + [email protected]("principal", ["anonymous", "guest", "inactive"]) +def test_ineligible_principal_has_no_capability_or_provider_work( + refresh_context: tuple[Mock, Mock, Mock], principal: str +) -> None: + from superset.commands.semantic_layer import refresh_metadata as module + from superset.commands.semantic_layer.exceptions import SemanticLayerForbiddenError + + view: Mock + provider: Mock + manager: Mock + view, provider, manager = refresh_context + user: Mock = Mock( + id=1, + is_anonymous=principal == "anonymous", + is_guest_user=principal == "guest", + is_active=principal != "inactive", + ) + cast( + Mock, module.Session + ).return_value.__enter__.return_value.get.return_value = user + with patch.object(g, "user", user): + with pytest.raises(SemanticLayerForbiddenError): + module.RefreshMetadataCommand(VIEW_UUID).run() + assert module.can_refresh_metadata(view) is False + provider.from_configuration.assert_not_called() + cast(Mock, module.guarded_store).assert_not_called() + + +def test_unregistered_provider_has_no_capability_or_construction( + refresh_context: tuple[Mock, Mock, Mock], +) -> None: + from superset.commands.semantic_layer import refresh_metadata as module + + view: Mock + provider: Mock + manager: Mock + view, provider, manager = refresh_context + with patch.dict(module.registry, {}, clear=True): + with pytest.raises(MetadataRefreshError, match="^unsupported$"): + module.RefreshMetadataCommand(VIEW_UUID).run() + assert module.can_refresh_metadata(view) is False + provider.from_configuration.assert_not_called() + cast(Mock, module.guarded_store).assert_not_called() + + [email protected]("namespace", [None, "", 7]) +def test_missing_trusted_namespace_denies_before_provider_construction( + refresh_context: tuple[Mock, Mock, Mock], namespace: object +) -> None: + from flask import current_app + + from superset.commands.semantic_layer import refresh_metadata as module + from superset.semantic_layers.metadata_binding import connection_metadata_scope + + view: Mock + provider: Mock + manager: Mock + view, provider, manager = refresh_context + with ( + patch.object(module, "connection_metadata_scope", connection_metadata_scope), + patch.dict( + current_app.config, {"SEMANTIC_LAYER_METADATA_NAMESPACE": namespace} + ), + ): + with pytest.raises(MetadataRefreshError, match="^configuration$"): + module.RefreshMetadataCommand(VIEW_UUID).run() + assert module.can_refresh_metadata(view) is False + provider.from_configuration.assert_not_called() + cast(Mock, module.guarded_store).assert_not_called() + + +def test_capability_projection_does_not_construct_or_acquire_metadata( + refresh_context: tuple[Mock, Mock, Mock], +) -> None: + from superset.commands.semantic_layer import refresh_metadata as module + + view: Mock + provider: Mock + manager: Mock + view, provider, manager = refresh_context + assert module.can_refresh_metadata(view) is True + provider.supports_metadata_refresh.assert_called_once_with({"token": "test"}) + provider.from_configuration.assert_not_called() + cast(Mock, module.guarded_store).assert_not_called() + + [email protected]( + "command_name,method", + [ + ("InvalidateCatalogCommand", "invalidate_catalog"), + ("InvalidateCompatibilityCommand", "invalidate_compatibility"), + ("InspectCatalogCommand", "inspect_catalog"), + ], +) +def test_separate_controls_never_construct_a_provider( + refresh_context: tuple[Mock, Mock, Mock], + command_name: str, + method: str, +) -> None: + from superset.commands.semantic_layer import refresh_metadata as module + + view: Mock + provider: Mock + manager: Mock + view, provider, manager = refresh_context + getattr(module, command_name)(VIEW_UUID).run() Review Comment: This checks that the clear methods run, but would still pass if either clear command stopped revalidating authority after initial validation; the database race tests exercise refresh only. Could we cover a role revocation or binding change between validation and each clear, asserting that the command is rejected and neither the catalog nor compatibility generation is mutated? -- 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]
