From ff1362ad9edfbdfc6002e03e3f2ac19a26e789c2 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Mon, 28 Sep 2026 18:37:22 -0400 Subject: [PATCH 01/13] WIP --- QUIVER_SEMANTIC_LAYER_OAUTH_PLAN.md | 582 ++++++++++++++++++ docs/developer_docs/extensions/storage.md | 50 +- .../src/superset_core/extensions/context.py | 6 + .../extensions/storage/__init__.py | 2 + .../extensions/storage/models.py | 9 + .../extensions/storage/persistent.py | 2 +- .../extensions/storage/secrets.py | 41 ++ .../superset_core/semantic_layers/layer.py | 46 +- .../SemanticLayerModal.test.tsx | 54 ++ .../semanticLayers/SemanticLayerModal.tsx | 111 +++- superset/commands/semantic_layer/create.py | 5 +- superset/commands/semantic_layer/update.py | 13 +- superset/core/api/core_api_injection.py | 49 +- superset/extensions/context.py | 21 +- superset/extensions/storage/__init__.py | 1 + superset/extensions/storage/api.py | 17 +- superset/extensions/storage/persistent_dao.py | 93 ++- .../extensions/storage/persistent_model.py | 9 + superset/extensions/storage/secrets.py | 78 +++ superset/extensions/storage/utils.py | 5 +- ...8f2a1d9b73_add_extension_storage_access.py | 95 +++ superset/semantic_layers/api.py | 216 ++++++- superset/semantic_layers/masking.py | 5 +- superset/semantic_layers/models.py | 6 +- superset/semantic_layers/registry.py | 146 ++++- .../unit_tests/extensions/api_context_test.py | 52 ++ .../unit_tests/extensions/storage/test_api.py | 5 + .../extensions/storage/test_context.py | 12 +- .../extensions/storage/test_persistent_dao.py | 89 ++- .../extensions/storage/test_secrets.py | 112 ++++ tests/unit_tests/semantic_layers/api_test.py | 141 +++++ .../semantic_layers/decorators_test.py | 8 +- .../semantic_layers/registry_test.py | 69 +++ 33 files changed, 2067 insertions(+), 83 deletions(-) create mode 100644 QUIVER_SEMANTIC_LAYER_OAUTH_PLAN.md create mode 100644 superset-core/src/superset_core/extensions/storage/secrets.py create mode 100644 superset/extensions/storage/secrets.py create mode 100644 superset/migrations/versions/2026-09-28_00-00_4c8f2a1d9b73_add_extension_storage_access.py create mode 100644 tests/unit_tests/extensions/api_context_test.py create mode 100644 tests/unit_tests/extensions/storage/test_secrets.py create mode 100644 tests/unit_tests/semantic_layers/registry_test.py diff --git a/QUIVER_SEMANTIC_LAYER_OAUTH_PLAN.md b/QUIVER_SEMANTIC_LAYER_OAUTH_PLAN.md new file mode 100644 index 000000000000..4ff61ab1d3fa --- /dev/null +++ b/QUIVER_SEMANTIC_LAYER_OAUTH_PLAN.md @@ -0,0 +1,582 @@ +# Quiver Semantic Layer OAuth Implementation Plan + +## Objective + +Enable semantic layer extensions such as Quiver to store and use OAuth2 tokens +without exposing those tokens to browser code and without requiring a semantic +layer row to exist before authorization begins. + +The design uses `extension_storage` as the permanent token store. Tokens are +written directly during the OAuth callback for every user, including the admin +creating the semantic layer. It does not use a temporary KV escrow. + +## Target Architecture + +```text +Semantic Layer configuration + | + +-- Quiver settings produce stable credential_key + | + +-- "Connect to Quiver" + | +-- OAuth2 + PKCE + | +-- callback + | +-- extension_storage + | extension = netflix.quiver + | user = current user + | access = backend + | key = oauth2:v1: + | encrypted = true + | + +-- Create Semantic Layer + +-- uses the same credential_key +``` + +The admin creating the semantic layer follows the same OAuth path as every +other user. The credential is associated with the extension, current user, and +a stable Quiver credential identity rather than a semantic layer UUID. + +## 1. Add a Backend-Only Secrets Namespace + +Expose a dedicated Python-only API: + +```python +ctx.storage.secrets.get(key) +ctx.storage.secrets.set(key, value) +ctx.storage.secrets.remove(key) +``` + +This is preferable to adding `backend_only=True` to the ordinary persistent +accessor because it provides safer defaults: + +- It exists only in Python. +- It always encrypts values. +- It is always user-scoped. +- It has no `.shared` accessor. +- It does not support listing. +- It cannot accidentally become available in the TypeScript SDK. +- Ordinary `ctx.storage.persistent` behavior remains unchanged. + +### Storage Model + +Modify `superset/extensions/storage/persistent_model.py` and the corresponding +core model declaration to add: + +```python +access: str # "frontend" or "backend" +``` + +Existing rows receive `access="frontend"`. + +The logical identity becomes: + +```text +(extension_id, user_fk, resource_type, resource_uuid, access, key) +``` + +Including `access` in the lookup identity allows a browser-visible value and a +backend secret to use the same key without allowing frontend `PUT`, `GET`, or +`DELETE` requests to reveal whether the secret exists. + +Use an application-level enum backed by a portable string column rather than a +database enum: + +```python +class StorageAccess(str, Enum): + FRONTEND = "frontend" + BACKEND = "backend" +``` + +Update: + +- `superset/extensions/storage/persistent_model.py` +- `superset-core/src/superset_core/extensions/storage/models.py` +- `superset/extensions/storage/persistent_dao.py` +- Extension storage lookup indexes and unique constraint +- A new migration under `superset/migrations/versions/` + +The migration should: + +1. Add `access`, non-null, with a server default of `frontend`. +2. Replace the scoped lookup index with one containing `access`. +3. Replace the scoped unique constraint with one containing `access`. +4. Preserve every existing row as frontend-visible. +5. On downgrade, delete backend-only entries before removing the column; + converting them to frontend-visible entries would expose secrets. + +### Secrets API + +Add a new core interface, likely at: + +```text +superset-core/src/superset_core/extensions/storage/secrets.py +``` + +Add `secrets` to: + +- `superset-core/src/superset_core/extensions/context.py` +- `superset/extensions/context.py` +- The storage injection in `superset/core/api/core_api_injection.py` + +The host implementation should call the DAO with: + +```python +access=StorageAccess.BACKEND +encrypt=True +user_fk=current_user_id +codec="json" +``` + +Do not expose backend access in: + +- `superset-frontend/packages/superset-core/src/storage/index.ts` +- REST request parameters +- REST request bodies + +### DAO Invariants + +DAO methods should always receive an access class, defaulting to `FRONTEND` +for backward compatibility. + +The secrets accessor must enforce encryption internally. It must not accept an +option that permits `encrypt=False`. + +Quota accounting should continue to include both frontend and backend entries. + +## 2. Make the Browser API Blind to Backend Entries + +Update `superset/extensions/storage/api.py` so every persistent REST operation +is hard-coded to `access=FRONTEND`. + +Required behavior: + +- `GET`: a backend-only entry behaves as if it does not exist. +- `list`: backend entries are excluded before count, pagination, and payload + size calculation. +- `PUT`: writes only to the frontend namespace. +- `DELETE`: removes only from the frontend namespace. +- No response reveals the backend key, codec, size, encryption flag, or + existence. +- Supplying `access`, `backend_only`, or similar fields must not change the + namespace. + +Because access is part of the logical key, frontend `PUT` does not need to +reject a colliding backend key. It creates or updates an independent frontend +entry. + +### Storage Tests + +Extend: + +- `tests/unit_tests/extensions/storage/test_api.py` +- `tests/unit_tests/extensions/storage/test_persistent.py` +- `tests/unit_tests/extensions/storage/test_persistent_dao.py` +- `tests/unit_tests/extensions/storage/test_context.py` + +Cover: + +- Secret round-trip for the current user. +- Separation between two users. +- Separation between two extensions. +- Forced encryption. +- Absence of a shared secret accessor. +- Browser `GET` cannot read a secret. +- Browser `list` omits secrets and reports the filtered count. +- Browser `DELETE` does not remove a secret. +- Browser `PUT` cannot overwrite a secret. +- Frontend and backend entries can coexist at the same key. +- Unauthenticated use fails closed. +- Existing and migrated rows remain frontend-visible. + +## 3. Restore Extension Context During Semantic Layer Execution + +Storage depends on ambient extension context, but semantic layer providers are +registered during extension loading and invoked later, outside that loading +context. + +The registration process must retain the owning extension manifest. + +Update: + +- `superset/semantic_layers/registry.py` +- Semantic layer decorator injection in + `superset/core/api/core_api_injection.py` +- `superset/semantic_layers/models.py` +- Class-level registry consumers in `superset/semantic_layers/api.py` +- Create and update commands under `superset/commands/semantic_layer/` + +A registry entry should retain: + +```python +@dataclass(frozen=True) +class RegisteredSemanticLayer: + implementation: type[SemanticLayer] + extension_manifest: Manifest | None +``` + +Host semantic layers have `extension_manifest=None`. + +### Contextual Adapters + +Avoid relying on scattered `with extension_context(...)` calls. Introduce +explicit adapters for: + +- Semantic layer class operations +- Semantic layer instances +- Semantic view instances returned by a layer + +Each adapter enters the saved extension context before invoking the provider. + +Explicit adapters implementing the semantic layer protocols are preferable to +a generic `__getattr__` proxy. When a new protocol method is added, typing and +tests will then show that its context forwarding is missing. + +The wrapper must cover at least: + +- `from_configuration` +- Configuration schema generation +- Runtime schema generation +- Semantic view discovery +- `get_semantic_view` +- Metrics and dimensions +- Compatible metrics and dimensions +- Values lookup +- Query and table generation +- Row count +- Feature or property access that can call extension code + +Tests belong in: + +- `tests/unit_tests/semantic_layers/decorators_test.py` +- `tests/unit_tests/semantic_layers/models_test.py` +- A focused new contextual execution test module + +The test extension should call `get_context()` and `ctx.storage.secrets` from +both a layer and a returned view. + +## 4. Carry the Initiating User Into Background Execution + +Extension identity alone is insufficient because personal tokens require the +initiating user. + +The execution context should support an explicit principal ID: + +```python +extension_context(manifest, user_id=user_id) +``` + +`get_current_user_id()` should: + +1. Prefer the explicit execution principal. +2. Fall back to the authenticated Flask user during request execution. +3. Fail if neither exists. + +For async chart and query paths, propagate the initiating user already present +in query or task metadata into the semantic layer execution context. Never +fall back to: + +- The semantic layer creator +- The admin who configured it +- The Celery worker or service account + +This is required by Superset's async security model: a worker is a continuation +of the initiating principal. + +Embedded guests and anonymous or Public users have no personal Quiver token +and should fail closed unless Quiver later defines a separate service credential +design. + +## 5. Add Generic Semantic Layer Connection Actions + +The semantic layer modal needs a way to initiate OAuth before a `SemanticLayer` +row exists. This should be a generic contribution rather than a Quiver-specific +button in Superset. + +Add a typed semantic layer action contract to `superset-core`, for example: + +```python +@dataclass(frozen=True) +class SemanticLayerAction: + id: str + label: str + status: Literal["required", "connected", "expired", "error"] + required: bool + message: str | None = None +``` + +Provider hooks could be: + +```python +get_configuration_actions(configuration) +execute_configuration_action(action_id, configuration, return_url) +``` + +The default implementation returns no actions, preserving existing providers. + +### API Shape + +Extend the existing configuration schema response: + +```json +{ + "result": { "...": "JSON schema" }, + "actions": [ + { + "id": "oauth2", + "label": "Connect to Quiver", + "status": "required", + "required": true + } + ] +} +``` + +Add protected endpoints for executing actions: + +```text +POST /api/v1/semantic_layer/type//actions/ +POST /api/v1/semantic_layer//actions/ +GET /api/v1/semantic_layer//actions +``` + +The type endpoint supports pre-creation setup and receives partial +configuration. It should require semantic layer write permission. + +The UUID endpoint supports existing users connecting their own account. It +should require read access to the semantic layer or data source, rather than +edit permission, because connecting a personal token does not modify the +semantic layer. + +The action returns an OAuth authorization URL or a same-origin redirect +response, never a token. + +Class hooks and action execution must run through the extension context +adapter. + +## 6. Update the Semantic Layer Frontend + +Update +`superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx` to: + +1. Read `actions` from the schema response. +2. Render a standard `@superset-ui/core/components` button and status. +3. Send the partial configuration when starting the action. +4. Open the OAuth flow in a popup or redirect. +5. Poll or refetch schema and action status after completion. +6. Disable creation while a required action is not connected. +7. Never receive or store access or refresh tokens. + +Update +`superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx` +for: + +- Providers without actions. +- A required disconnected action. +- Successful connection and schema refresh. +- Popup cancellation. +- OAuth failure. +- A required action blocking creation. +- Edit-mode reconnection. + +For existing users, add a reusable Connect action surfaced when the semantic +layer reports that authentication is required. A typed +`SemanticLayerAuthenticationRequiredError` should carry only: + +- Semantic layer UUID +- Action ID +- Human-readable message + +The frontend can then call the UUID action endpoint. It must not receive +credential keys or token metadata beyond connected or expired status. + +## 7. Define the Quiver Credential Identity + +Tokens must be addressable before semantic layer creation and reusable by +multiple Quiver semantic layers. + +Build a canonical non-secret identity from fields such as: + +```json +{ + "quiver_url": "...", + "issuer": "...", + "client_id": "...", + "audience": "...", + "scopes": ["..."] +} +``` + +Normalize URLs and sort scopes, then hash the canonical JSON: + +```text +oauth2:v1: +``` + +Do not include: + +- Client secret +- Access token +- Refresh token +- Semantic layer UUID +- Semantic layer name + +The resulting record is: + +```text +extension_id = netflix.quiver +user_fk = authenticated user +access = backend +key = oauth2:v1: +value = { + access_token, + refresh_token, + token_type, + expires_at, + granted_scopes +} +is_encrypted = true +``` + +Deleting one semantic layer must not delete this entry because several layers +may share it. Provide an explicit Disconnect Quiver action instead. + +Changing identity fields produces a new credential key. Old credentials can +be removed through Disconnect or eventual extension cleanup. + +## 8. Implement OAuth in the Quiver Extension + +Superset provides storage, execution context, and action UI. The Quiver +extension remains responsible for OAuth protocol behavior. + +### Start + +The start handler should: + +- Require an authenticated user. +- Derive the credential key from submitted non-secret configuration. +- Generate state, nonce, and a PKCE verifier and challenge. +- Bind signed state to the user ID, credential key, return location, nonce, + and expiry. +- Store the PKCE verifier in a short-lived backend cache inaccessible through + extension browser storage. +- Redirect to the configured issuer. + +The PKCE verifier is temporary protocol state, not token escrow. + +### Callback + +The callback should: + +- Validate the state signature and age. +- Require the same authenticated user recorded in state. +- Consume the nonce and verifier exactly once. +- Exchange the authorization code server-side. +- Write the resulting token directly through `ctx.storage.secrets`. +- Return a minimal success page or redirect. +- Never include tokens in URLs, HTML, `postMessage`, logs, or frontend + responses. + +### Refresh + +Before calling Quiver: + +1. Load the user's token. +2. Refresh if it is near expiry. +3. Acquire a distributed lock scoped to extension, user, and credential key. +4. Reread after acquiring the lock. +5. Refresh only if it is still necessary. +6. Persist the new access token and any rotated refresh token. + +On `invalid_grant`, remove the stored secret and raise the typed authentication +required error. + +## 9. Update Documentation + +Update `docs/developer_docs/extensions/storage.md` to make the distinction +explicit: + +- `encrypt=True` means encryption at rest. +- Ordinary persistent values remain readable by the extension frontend. +- Tokens and backend credentials belong in `ctx.storage.secrets`. +- Secrets are encrypted, user-scoped, backend-only, non-listable, and + unavailable in TypeScript. +- `.shared` is intentionally unavailable for secrets. +- Secret values remain subject to extension storage quotas. +- User deletion cascades to personal secrets. + +Also add semantic layer extension documentation covering: + +- Runtime extension context +- Configuration actions +- Pre-creation OAuth +- Personal token behavior in background queries +- Disconnect and reconnect behavior + +The existing `api_token` example under ordinary persistent storage should be +changed because it suggests a browser-readable location for bearer +credentials. + +## 10. Suggested Pull Request Sequence + +### PR 1: Backend-Only Extension Secrets + +- Migration and model changes +- DAO access namespace +- `ctx.storage.secrets` +- REST isolation +- Storage tests +- Storage documentation + +This PR is independently useful to all backend extensions. + +### PR 2: Semantic Layer Runtime Context + +- Registered provider metadata +- Contextual layer and view adapters +- Explicit user principal propagation +- Synchronous and asynchronous execution tests + +This makes extension storage usable reliably from semantic layer providers. + +### PR 3: Semantic Layer Connection Actions + +- Core action types and hooks +- Protected action endpoints +- Modal integration +- Authentication-required error contract +- Frontend and API tests +- Developer documentation + +### PR 4: Quiver Extension + +- Credential key derivation +- OAuth start and callback +- Token storage and refresh locking +- Disconnect and reconnect +- Quiver-specific tests + +## Explicit Non-Goals + +- Do not refactor `database_user_oauth2_tokens`. +- Do not migrate existing database OAuth flows. +- Do not use `localStorage`, `sessionStorage`, cookies, or frontend Redux state + for tokens. +- Do not use KV escrow for the initial admin. +- Do not key tokens to semantic layer UUIDs. +- Do not expose a general browser flag for creating backend-only entries. +- Do not introduce shared or service-account credentials as part of the + personal OAuth implementation. + +## Completion Criteria + +The work is complete when: + +- An admin can authorize Quiver before creating the semantic layer. +- No semantic layer row is required to store that token. +- Another user sees a Connect action and stores a separate personal token. +- Synchronous and asynchronous queries use the initiating user's token. +- Multiple semantic layers can reuse the same user credential. +- Browser storage APIs cannot read, enumerate, overwrite, or delete the token. +- Refresh token rotation is persisted safely. +- Revoked credentials produce a reconnect action. +- Existing Tier 3 storage behavior remains backward compatible. +- Targeted tests and staged-file `pre-commit run` pass. diff --git a/docs/developer_docs/extensions/storage.md b/docs/developer_docs/extensions/storage.md index 93c13b07ff00..d1e633f621fb 100644 --- a/docs/developer_docs/extensions/storage.md +++ b/docs/developer_docs/extensions/storage.md @@ -438,8 +438,8 @@ import { extensions } from '@apache-superset/core'; const ctx = extensions.getContext(); -await ctx.storage.persistent.set('api_token', 'sk-...', { encrypt: true }); -const token = await ctx.storage.persistent.get('api_token'); +await ctx.storage.persistent.set('private_note', 'draft', { encrypt: true }); +const note = await ctx.storage.persistent.get('private_note'); ``` ```python @@ -448,10 +448,52 @@ from superset_core.extensions.storage.persistent import PersistentSetOptions ctx = get_context() -ctx.storage.persistent.set('api_token', 'sk-...', PersistentSetOptions(encrypt=True)) -token = ctx.storage.persistent.get('api_token') +ctx.storage.persistent.set( + 'private_note', + 'draft', + PersistentSetOptions(encrypt=True), +) +note = ctx.storage.persistent.get('private_note') ``` +Encryption protects a value at rest; it does not make an ordinary persistent +value backend-only. The persistent API is intentionally available to frontend +extensions, so the authenticated user's browser can read values written with +`encrypt=True`. Do not use `persistent` for OAuth tokens, private keys, or +other credentials that must stay on the server. + +### Backend-only secrets + +Backend extensions can store user-scoped credentials through the Python-only +secrets accessor: + +```python +from superset_core.extensions.context import get_context + +ctx = get_context() +ctx.storage.secrets.set( + "oauth2:quiver", + { + "access_token": access_token, + "refresh_token": refresh_token, + "expires_at": expires_at, + }, +) +token = ctx.storage.secrets.get("oauth2:quiver") +ctx.storage.secrets.remove("oauth2:quiver") +``` + +Secrets are always encrypted at rest and scoped to the current authenticated +user and extension. They cannot be read, listed, overwritten, or deleted +through the extension storage REST API and are not exposed by the TypeScript +SDK. The accessor intentionally has no `list()` or `.shared` operation. Use +deployment configuration for extension-wide client secrets or service +credentials. + +Secret operations require an authenticated principal. Background work must +execute with the identity of the user who initiated it; the accessor fails +closed when no user identity is available. + Encryption reuses Superset's existing `EncryptedType` (from `sqlalchemy-utils`) rather than a separate mechanism — the same infrastructure used for database connection credentials. The key is not configured directly: user-scoped values are encrypted with a key derived per-user via HMAC-SHA256 from the deployment's `SECRET_KEY`, so ciphertext for one user cannot be decrypted as another's; shared/global values (written via `.shared`) use `SECRET_KEY` directly. The encryption engine (AES-CBC by default) can be changed for the whole deployment via the existing `SQLALCHEMY_ENCRYPTED_FIELD_ENGINE` config, the same setting that controls encryption for database credentials elsewhere in Superset — there is no separate key list or rotation mechanism specific to extension storage. When `SECRET_KEY` is rotated, extension storage rows are re-encrypted by the existing `SecretsMigrator` tooling alongside database credentials, with no extension-specific steps required. ### Quotas diff --git a/superset-core/src/superset_core/extensions/context.py b/superset-core/src/superset_core/extensions/context.py index 63c8ea35e78c..a9a3a45a488e 100644 --- a/superset-core/src/superset_core/extensions/context.py +++ b/superset-core/src/superset_core/extensions/context.py @@ -46,6 +46,7 @@ def setup(): if TYPE_CHECKING: from superset_core.extensions.storage.ephemeral import EphemeralStateAccessor from superset_core.extensions.storage.persistent import PersistentStateAccessor + from superset_core.extensions.storage.secrets import SecretsState from superset_core.extensions.types import BaseExtension @@ -62,6 +63,11 @@ def persistent(self) -> "PersistentStateAccessor": """Database-backed persistent storage.""" ... + @property + def secrets(self) -> "SecretsState": + """Backend-only encrypted storage for user-scoped secrets.""" + ... + class ExtensionContext(Protocol): """ diff --git a/superset-core/src/superset_core/extensions/storage/__init__.py b/superset-core/src/superset_core/extensions/storage/__init__.py index 095b1b9c9cb1..e0c4359e5fff 100644 --- a/superset-core/src/superset_core/extensions/storage/__init__.py +++ b/superset-core/src/superset_core/extensions/storage/__init__.py @@ -35,6 +35,7 @@ - persistent: Durable KV storage backed by database table - Survives server restarts, supports encryption and resource linking - Use for user preferences, extension config, per-resource settings + - secrets: Python-only, encrypted user credentials unavailable to browsers All tiers follow the same API pattern: - User-scoped by default (private to current user) @@ -76,4 +77,5 @@ ephemeral, # noqa: F401 models, # noqa: F401 persistent, # noqa: F401 + secrets, # noqa: F401 ) diff --git a/superset-core/src/superset_core/extensions/storage/models.py b/superset-core/src/superset_core/extensions/storage/models.py index 847052ed021f..64384edba0ee 100644 --- a/superset-core/src/superset_core/extensions/storage/models.py +++ b/superset-core/src/superset_core/extensions/storage/models.py @@ -30,6 +30,7 @@ from __future__ import annotations +from enum import Enum from typing import TYPE_CHECKING from uuid import UUID @@ -39,6 +40,13 @@ from datetime import datetime +class StorageAccess(str, Enum): + """Controls whether a persistent storage entry is browser-accessible.""" + + FRONTEND = "frontend" + BACKEND = "backend" + + class ExtensionStorageEntry(CoreModel): """ Abstract model interface for a single Tier 3 persistent storage row. @@ -66,6 +74,7 @@ class ExtensionStorageEntry(CoreModel): value_size: int codec: str is_encrypted: bool + access: str # Audit fields created_on: "datetime | None" diff --git a/superset-core/src/superset_core/extensions/storage/persistent.py b/superset-core/src/superset_core/extensions/storage/persistent.py index fa2b7e5ad312..8f3f9ffb1718 100644 --- a/superset-core/src/superset_core/extensions/storage/persistent.py +++ b/superset-core/src/superset_core/extensions/storage/persistent.py @@ -46,7 +46,7 @@ # Encrypted at rest ctx.storage.persistent.set( - 'api_token', 'sk-...', PersistentSetOptions(encrypt=True) + 'private_note', 'draft', PersistentSetOptions(encrypt=True) ) # Listing entries (page and page_size are required) diff --git a/superset-core/src/superset_core/extensions/storage/secrets.py b/superset-core/src/superset_core/extensions/storage/secrets.py new file mode 100644 index 000000000000..c4053775b68b --- /dev/null +++ b/superset-core/src/superset_core/extensions/storage/secrets.py @@ -0,0 +1,41 @@ +# 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. + +"""Backend-only, encrypted storage for user-scoped extension secrets.""" + +from __future__ import annotations + +from typing import Any + + +class SecretsState: + """User-scoped persistent secrets unavailable to extension frontend code.""" + + @staticmethod + def get(key: str) -> Any: + """Return the current user's secret, or ``None`` when absent.""" + raise NotImplementedError("Class will be replaced during initialization") + + @staticmethod + def set(key: str, value: Any) -> None: + """Store an encrypted secret for the current user.""" + raise NotImplementedError("Class will be replaced during initialization") + + @staticmethod + def remove(key: str) -> None: + """Remove the current user's secret.""" + raise NotImplementedError("Class will be replaced during initialization") diff --git a/superset-core/src/superset_core/semantic_layers/layer.py b/superset-core/src/superset_core/semantic_layers/layer.py index 8e6b02ef7314..ff62268c5eaf 100644 --- a/superset-core/src/superset_core/semantic_layers/layer.py +++ b/superset-core/src/superset_core/semantic_layers/layer.py @@ -20,7 +20,7 @@ import enum from abc import ABC, abstractmethod from dataclasses import dataclass -from typing import Any, ClassVar, Generic, Mapping, TypeVar +from typing import Any, ClassVar, Generic, Literal, Mapping, TypeVar from pydantic import BaseModel @@ -74,6 +74,32 @@ class SemanticCacheIdentityMaterial: values: Mapping[str, object] +@dataclass(frozen=True) +class SemanticLayerAction: + """A user action required to configure or authenticate a provider.""" + + id: str + label: str + status: Literal["required", "connected", "expired", "error"] + required: bool = False + message: str | None = None + + +@dataclass(frozen=True) +class SemanticLayerActionResult: + """Result of starting a semantic layer configuration action.""" + + redirect_url: str + + +class SemanticLayerAuthenticationRequiredError(Exception): + """Raised when a provider needs a personal authentication action.""" + + def __init__(self, action_id: str, message: str = "Authentication required"): + super().__init__(message) + self.action_id = action_id + self.message = message + class SemanticLayer(ABC, Generic[ConfigT, SemanticViewT]): """ @@ -114,6 +140,24 @@ def get_semantic_cache_context_identity( # None preserves legacy result-cache keys without constructing the provider. result_cache_version: ClassVar[str | None] = None + @classmethod + def get_configuration_actions( + cls, + configuration: ConfigT | None = None, + ) -> list[SemanticLayerAction]: + """Return actions associated with the current user's configuration.""" + return [] + + @classmethod + def execute_configuration_action( + cls, + action_id: str, + configuration: ConfigT | None, + return_url: str | None = None, + ) -> SemanticLayerActionResult: + """Start an action and return the URL to which the user should navigate.""" + raise ValueError(f"Unknown semantic layer action: {action_id}") + @classmethod @abstractmethod def from_configuration( diff --git a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx index 67f278dbaa3f..855f1458190c 100644 --- a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx +++ b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx @@ -745,3 +745,57 @@ test('A pending clear prevents edits and a closed editor ignores its late respon await act(async () => finish({ json: {} })); expect(props.addSuccessToast).not.toHaveBeenCalled(); }); + +test('starts a required semantic layer configuration action', async () => { + mockedPost + .mockResolvedValueOnce({ + json: { + result: schemaWithExternalDeps, + actions: [ + { + id: 'oauth2', + label: 'Connect to Quiver', + status: 'required', + required: true, + }, + ], + }, + }) + .mockResolvedValueOnce({ + json: { + result: { redirect_url: 'https://auth.example.test/authorize' }, + }, + }); + const popup = { + close: jest.fn(), + location: { href: '' }, + } as unknown as Window; + const open = jest.spyOn(window, 'open').mockReturnValue(popup); + + render(); + + const connect = await screen.findByRole('button', { + name: 'Connect to Quiver', + }); + expect(screen.getByRole('button', { name: 'Save' })).toBeDisabled(); + fireEvent.click(connect); + + await waitFor(() => { + expect(mockedPost).toHaveBeenNthCalledWith(2, { + endpoint: '/api/v1/semantic_layer/type/snowflake/actions/oauth2', + jsonPayload: { + configuration: expect.any(Object), + return_url: window.location.href, + semantic_layer_uuid: props.semanticLayerUuid, + }, + }); + expect(open).toHaveBeenCalledWith( + '', + 'semantic-layer-action-oauth2', + 'popup,width=720,height=800', + ); + expect(popup.location.href).toBe('https://auth.example.test/authorize'); + }); + + open.mockRestore(); +}); diff --git a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx index 301a2900c97f..f5651cc22462 100644 --- a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx +++ b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx @@ -79,6 +79,14 @@ interface SemanticLayerType { description: string; } +interface SemanticLayerAction { + id: string; + label: string; + status: 'required' | 'connected' | 'expired' | 'error'; + required: boolean; + message?: string | null; +} + interface SemanticLayerModalProps { show: boolean; onHide: () => void; @@ -101,6 +109,10 @@ export default function SemanticLayerModal({ const [types, setTypes] = useState([]); const [loading, setLoading] = useState(false); const [configSchema, setConfigSchema] = useState(null); + const [configurationActions, setConfigurationActions] = useState< + SemanticLayerAction[] + >([]); + const [runningActionId, setRunningActionId] = useState(null); const [uiSchema, setUiSchema] = useState( undefined, ); @@ -132,6 +144,7 @@ export default function SemanticLayerModal({ useState('ValidateAndHide'); const errorsRef = useRef([]); const debounceTimerRef = useRef | null>(null); + const oauthFocusListenerRef = useRef<(() => void) | null>(null); const lastDepSnapshotRef = useRef(''); const dynamicDepsRef = useRef>({}); const schemaRequestRef = useRef(0); @@ -179,6 +192,7 @@ export default function SemanticLayerModal({ }); if (request !== schemaRequestRef.current) return; applySchema(json.result); + setConfigurationActions(json.actions ?? []); if (json.warning) { addDangerToast(configurationErrorMessage()); } @@ -227,6 +241,8 @@ export default function SemanticLayerModal({ if (request !== schemaRequestRef.current) return; applySchema(schemaJson.result); if (schemaJson.warning) addDangerToast(configurationErrorMessage()); + + setConfigurationActions(schemaJson.actions ?? []); setStep('config'); } catch { if (request !== schemaRequestRef.current) return; @@ -249,11 +265,19 @@ export default function SemanticLayerModal({ fetchTypes(); } } else { + useEffect( + () => () => { + if (oauthFocusListenerRef.current) { + window.removeEventListener('focus', oauthFocusListenerRef.current); + oauthFocusListenerRef.current = null; + } setStep('type'); setName(''); setSelectedType(null); setTypes([]); setConfigSchema(null); + setConfigurationActions([]); + setRunningActionId(null); setUiSchema(undefined); setFormData({}); setSavedForm(null); @@ -304,6 +328,14 @@ export default function SemanticLayerModal({ } }; + + if (oauthFocusListenerRef.current) { + window.removeEventListener('focus', oauthFocusListenerRef.current); + } + }, + [], + ); + const handleStepAdvance = () => { if (selectedType) { // Pre-fill the Name with the type's display name so a user who just @@ -323,6 +355,7 @@ export default function SemanticLayerModal({ setRefreshingSchema(false); setStep('type'); setConfigSchema(null); + setConfigurationActions([]); setUiSchema(undefined); setFormData({}); setValidationMode('ValidateAndHide'); @@ -362,6 +395,62 @@ export default function SemanticLayerModal({ } }; + const handleConfigurationAction = async (action: SemanticLayerAction) => { + if (!selectedType) return; + const popup = window.open( + '', + `semantic-layer-action-${action.id}`, + 'popup,width=720,height=800', + ); + if (!popup) { + addDangerToast(t('Allow popups to connect this semantic layer')); + return; + } + + setRunningActionId(action.id); + try { + const { json } = await SupersetClient.post({ + endpoint: `/api/v1/semantic_layer/type/${encodeURIComponent( + selectedType, + )}/actions/${encodeURIComponent(action.id)}`, + jsonPayload: { + configuration: formData, + return_url: window.location.href, + ...(semanticLayerUuid + ? { semantic_layer_uuid: semanticLayerUuid } + : {}), + }, + }); + const redirectUrl = json.result?.redirect_url; + if (typeof redirectUrl !== 'string') { + throw new Error(t('The connection action did not return a URL')); + } + popup.location.href = redirectUrl; + + const refreshAfterOAuth = () => { + window.removeEventListener('focus', refreshAfterOAuth); + oauthFocusListenerRef.current = null; + fetchConfigSchema(selectedType, formData); + }; + if (oauthFocusListenerRef.current) { + window.removeEventListener('focus', oauthFocusListenerRef.current); + } + oauthFocusListenerRef.current = refreshAfterOAuth; + window.addEventListener('focus', refreshAfterOAuth); + } catch (error) { + popup.close(); + const clientError = await getClientErrorObject(error); + addDangerToast( + clientError.error || + (error instanceof Error + ? error.message + : t('An error occurred while starting the connection')), + ); + } finally { + setRunningActionId(null); + } + }; + // Edit mode skips the type-picker step. Gating on this prevents the brief // flash of the Create modal's first step while the existing layer is being // fetched. @@ -468,6 +557,9 @@ export default function SemanticLayerModal({ : isTypeStep ? t('New Semantic Layer') : t('Configure %s', selectedTypeName); + const requiredActionPending = configurationActions.some( + action => action.required && action.status !== 'connected', + ); return ( + {configurationActions.map(action => ( + + + {action.message &&
{action.message}
} + + ))} {configSchema && ( // Wrap in a form with autocomplete="off" so browsers do not // autofill credential fields (service token, account, etc.). diff --git a/superset/commands/semantic_layer/create.py b/superset/commands/semantic_layer/create.py index c32f479912d8..a80b51204503 100644 --- a/superset/commands/semantic_layer/create.py +++ b/superset/commands/semantic_layer/create.py @@ -38,7 +38,7 @@ from superset.commands.utils import current_user_can_modify_object from superset.daos.semantic_layer import SemanticLayerDAO, SemanticViewDAO from superset.exceptions import SupersetSecurityException -from superset.semantic_layers.registry import registry +from superset.semantic_layers.registry import registry, semantic_layer_context from superset.utils import json from superset.utils.decorators import on_error, transaction @@ -75,7 +75,8 @@ def validate(self) -> None: # Validate configuration against the plugin cls = registry[sl_type] - validate_configuration(cls, self._properties["configuration"]) + with semantic_layer_context(sl_type): + validate_configuration(cls, self._properties["configuration"]) class CreateSemanticViewCommand(BaseCommand): diff --git a/superset/commands/semantic_layer/update.py b/superset/commands/semantic_layer/update.py index 3689ca315d93..b5bef3383b6a 100644 --- a/superset/commands/semantic_layer/update.py +++ b/superset/commands/semantic_layer/update.py @@ -45,7 +45,7 @@ unmask_configuration, ) from superset.semantic_layers.models import SemanticLayer, SemanticView -from superset.semantic_layers.registry import registry +from superset.semantic_layers.registry import registry, semantic_layer_context from superset.utils import json from superset.utils.decorators import on_error, transaction @@ -56,7 +56,7 @@ _MISSING = object() -def _unmask_configuration( +def unmask_stored_configuration( existing_raw_configuration: str | None, new_configuration: dict[str, Any], layer_type: str, @@ -136,6 +136,10 @@ def _unmask_configuration( raise SemanticLayerInvalidError(str(ex)) from None +# Kept while callers transition to the descriptive shared name above. +_unmask_configuration = unmask_stored_configuration + + class UpdateSemanticViewCommand(BaseCommand): def __init__(self, model_id: int, data: dict[str, Any]): self._model_id = model_id @@ -230,7 +234,7 @@ def validate(self) -> None: raise SemanticLayerInvalidError(f"Name already exists: {name}") if isinstance(self._properties.get("configuration"), dict): - self._properties["configuration"] = _unmask_configuration( + self._properties["configuration"] = unmask_stored_configuration( self._model.configuration, self._properties["configuration"], self._model.type, @@ -241,7 +245,8 @@ def validate(self) -> None: sl_type: str = self._model.type if sl_type not in registry: raise SemanticLayerInvalidError(f"Unknown type: {sl_type}") - validate_configuration(registry[sl_type], configuration) + with semantic_layer_context(sl_type): + validate_configuration(registry[sl_type], configuration) class ClearSemanticLayerCacheCommand(BaseCommand): diff --git a/superset/core/api/core_api_injection.py b/superset/core/api/core_api_injection.py index f2d48f31853a..c42c68290d1e 100644 --- a/superset/core/api/core_api_injection.py +++ b/superset/core/api/core_api_injection.py @@ -23,17 +23,52 @@ to be used with direct imports while maintaining loose coupling. """ +from functools import wraps from typing import Any, Callable, TYPE_CHECKING, TypeVar from sqlalchemy.orm import scoped_session -from superset.extensions.context import get_current_extension_context +from superset.extensions.context import ( + extension_context, + get_current_extension_context, +) +from superset.utils.core import get_user_id if TYPE_CHECKING: from superset_core.common.models import Database + from superset_core.extensions.types import Manifest from superset_core.rest_api.api import RestApi +def _contextualize_extension_api( + api_class: type["RestApi"], + manifest: "Manifest", +) -> None: + """Execute exposed extension API methods with extension and user context.""" + for attribute_name, attribute in vars(api_class).items(): + if not callable(attribute) or not getattr(attribute, "_urls", None): + continue + setattr( + api_class, + attribute_name, + _contextualize_extension_api_method(attribute, manifest), + ) + + +def _contextualize_extension_api_method( + method: Callable[..., Any], + manifest: "Manifest", +) -> Callable[..., Any]: + """Wrap one API method in its extension's execution context.""" + + @wraps(method) + def contextualized(*args: Any, **kwargs: Any) -> Any: + with extension_context(manifest, user_id=get_user_id()): + return method(*args, **kwargs) + + return contextualized + + def inject_dao_implementations() -> None: """ Replace abstract DAO classes in superset_core common/queries/tasks daos with @@ -196,6 +231,7 @@ def decorator(api_class: T) -> T: f"extensions.{context.extension.publisher}." f"{context.extension.name}.{id}" ) + _contextualize_extension_api(api_class, context.extension) else: # HOST CONTEXT @@ -254,7 +290,7 @@ def inject_semantic_layer_implementations() -> None: import superset_core.semantic_layers.decorators as core_sl_module import superset.extensions.context as context_module - from superset.semantic_layers.registry import registry + from superset.semantic_layers.registry import register_semantic_layer def semantic_layer_impl( id: str, @@ -273,7 +309,11 @@ def decorator(cls: Any) -> Any: cls.name = name cls.description = description cls._semantic_layer_id = prefixed_id - registry[prefixed_id] = cls + register_semantic_layer( + prefixed_id, + cls, + context.extension if context is not None else None, + ) return cls return decorator @@ -290,15 +330,18 @@ def inject_storage_implementations() -> None: import superset_core.extensions.storage.ephemeral as core_ephemeral_state import superset_core.extensions.storage.models as core_storage_models import superset_core.extensions.storage.persistent as core_persistent_state + import superset_core.extensions.storage.secrets as core_secrets_state from superset.extensions.storage.ephemeral import EphemeralState from superset.extensions.storage.persistent import PersistentState from superset.extensions.storage.persistent_dao import ExtensionStorageDAO from superset.extensions.storage.persistent_model import ExtensionStorage + from superset.extensions.storage.secrets import SecretsState # Replace abstract classes with concrete implementations core_ephemeral_state.EphemeralState = EphemeralState # type: ignore[misc,assignment] core_persistent_state.PersistentState = PersistentState # type: ignore[misc,assignment] + core_secrets_state.SecretsState = SecretsState # type: ignore[misc,assignment] core_storage_models.ExtensionStorageEntry = ExtensionStorage # type: ignore[misc,assignment] core_storage_dao.ExtensionStorageDAO = ExtensionStorageDAO # type: ignore[misc,assignment] diff --git a/superset/extensions/context.py b/superset/extensions/context.py index 7d11822657c0..7ba9c1d56351 100644 --- a/superset/extensions/context.py +++ b/superset/extensions/context.py @@ -50,12 +50,19 @@ def persistent(self) -> Any: return PersistentState + @property + def secrets(self) -> Any: + from superset.extensions.storage.secrets import SecretsState + + return SecretsState + class ConcreteExtensionContext: """Concrete implementation of ExtensionContext for the host.""" - def __init__(self, manifest: Manifest): + def __init__(self, manifest: Manifest, user_id: int | None = None): self._manifest = manifest + self._user_id = user_id self._storage = ExtensionStorage() @property @@ -67,6 +74,11 @@ def extension(self) -> BaseExtension: def storage(self) -> ExtensionStorage: return self._storage + @property + def user_id(self) -> int | None: + """Explicit principal for extension execution outside a request.""" + return self._user_id + # Context variable for ambient extension context pattern. # Thread-safe and async-safe via Python's contextvars. @@ -119,7 +131,10 @@ def use_context(ctx: ConcreteExtensionContext) -> Iterator[None]: @contextmanager -def extension_context(manifest: Manifest) -> Iterator[ConcreteExtensionContext]: +def extension_context( + manifest: Manifest, + user_id: int | None = None, +) -> Iterator[ConcreteExtensionContext]: """ Context manager for setting extension context during loading. @@ -129,6 +144,6 @@ def extension_context(manifest: Manifest) -> Iterator[ConcreteExtensionContext]: :param manifest: The extension manifest :yields: The created ExtensionContext """ - ctx = ConcreteExtensionContext(manifest) + ctx = ConcreteExtensionContext(manifest, user_id=user_id) with use_context(ctx): yield ctx diff --git a/superset/extensions/storage/__init__.py b/superset/extensions/storage/__init__.py index 8f8a59b45c2e..b1b83f04acde 100644 --- a/superset/extensions/storage/__init__.py +++ b/superset/extensions/storage/__init__.py @@ -26,3 +26,4 @@ EphemeralState, SharedEphemeralStateAccessor, ) +from superset.extensions.storage.secrets import SecretsState # noqa: F401 diff --git a/superset/extensions/storage/api.py b/superset/extensions/storage/api.py index 8bee0e55a363..64853a904608 100644 --- a/superset/extensions/storage/api.py +++ b/superset/extensions/storage/api.py @@ -33,6 +33,7 @@ from flask import g, request from flask.wrappers import Response from flask_appbuilder.api import expose, protect, safe +from superset_core.extensions.storage.models import StorageAccess from superset.extensions.storage.codecs import DEFAULT_CODEC, get_codec, SAFE_CODECS from superset.extensions.storage.ephemeral_dao import ( @@ -478,6 +479,7 @@ def list_persistent(self, publisher: str, name: str, **kwargs: Any) -> Response: resource_uuid=resource_uuid, page=page, page_size=page_size, + access=StorageAccess.FRONTEND, ) except ExtensionStorageListPayloadTooLarge as ex: return self.response(ex.status, message=ex.message) @@ -567,7 +569,12 @@ def get_persistent( shared = request.args.get("shared", "false").lower() == "true" user_fk = None if shared else g.user.id - entry = ExtensionStorageDAO.get(extension_id, key, user_fk=user_fk) + entry = ExtensionStorageDAO.get( + extension_id, + key, + user_fk=user_fk, + access=StorageAccess.FRONTEND, + ) if entry is None: return self.response(200, result=None) if entry.codec not in SAFE_CODECS: @@ -693,6 +700,7 @@ def set_persistent( codec=codec, user_fk=user_fk, encrypt=encrypt, + access=StorageAccess.FRONTEND, ) except ( ExtensionStorageKeyTooLong, @@ -752,6 +760,11 @@ def delete_persistent( shared = request.args.get("shared", "false").lower() == "true" user_fk = None if shared else g.user.id - ExtensionStorageDAO.delete_by_key(extension_id, key, user_fk=user_fk) + ExtensionStorageDAO.delete_by_key( + extension_id, + key, + user_fk=user_fk, + access=StorageAccess.FRONTEND, + ) return self.response(200, message="Value deleted successfully") diff --git a/superset/extensions/storage/persistent_dao.py b/superset/extensions/storage/persistent_dao.py index 52e625c3b6e8..b473a3cb6549 100644 --- a/superset/extensions/storage/persistent_dao.py +++ b/superset/extensions/storage/persistent_dao.py @@ -29,7 +29,9 @@ import backoff from flask import current_app from flask_babel import gettext as _ -from sqlalchemy import and_, desc, func, LargeBinary +from sqlalchemy import and_, desc, func, String +from sqlalchemy.engine import Dialect +from superset_core.extensions.storage.models import StorageAccess from superset import db from superset.daos.base import BaseDAO @@ -163,6 +165,8 @@ def __init__(self, max_length: int) -> None: #: a crashed holder doesn't block the same key for long. SET_LOCK_TTL_SECONDS = 5 +ENCRYPTED_VALUE_PREFIX = "superset-extension-storage:v1:" + def _validate_key_length(key: str) -> None: """Validate key length against MAX_KEY_LENGTH. Raises if over.""" @@ -202,6 +206,36 @@ def _enc_type(user_fk: int | None) -> EncryptedBytesType: ) +def _encrypt_value(value: bytes, user_fk: int | None, dialect: Dialect) -> bytes: + """Encrypt arbitrary bytes using a versioned, text-safe representation.""" + plaintext = ENCRYPTED_VALUE_PREFIX + base64.b64encode(value).decode("ascii") + encrypted = _enc_type(user_fk).process_bind_param(plaintext, dialect) + if not isinstance(encrypted, bytes): + raise TypeError("Encrypted extension storage values must be bytes") + return encrypted + + +def _decrypt_value(value: bytes, user_fk: int | None, dialect: Dialect) -> bytes: + """Decrypt bytes written by ``_encrypt_value`` or the legacy implementation.""" + plaintext = _enc_type(user_fk).process_result_value(value, dialect) + if not isinstance(plaintext, str): + raise TypeError("Decrypted extension storage values must be strings") + if plaintext.startswith(ENCRYPTED_VALUE_PREFIX): + return base64.b64decode( + plaintext.removeprefix(ENCRYPTED_VALUE_PREFIX), + validate=True, + ) + + # The initial implementation passed bytes directly to EncryptedType. The + # resulting plaintext is the Python bytes representation (for example, + # ``b'{\"token\": \"...\"}'``). Recover those rows so deployments that + # wrote secrets before this encoding was introduced do not lose them. + legacy_value = ast.literal_eval(plaintext) + if not isinstance(legacy_value, bytes): + raise TypeError("Legacy encrypted extension storage value is not bytes") + return legacy_value + + def _get_quota() -> int | None: """Return the configured per-extension persistent storage quota in bytes. @@ -273,9 +307,7 @@ def _decrypt_if_needed( if not entry.is_encrypted: return entry.value try: - return _enc_type(entry.user_fk).process_result_value( - entry.value, db.engine.dialect - ) + return _decrypt_value(entry.value, entry.user_fk, db.engine.dialect) except Exception: # noqa: BLE001 logger.error( "Failed to decrypt extension storage value for " @@ -292,11 +324,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, ] if user_fk is None: filters.append(ExtensionStorage.user_fk.is_(None)) @@ -318,13 +352,17 @@ def _list_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 `list()` call. Same scope semantics as `_scope_filter` (an explicit None means "match the global/unset scope"), minus the `key` filter. """ - filters: list[object] = [ExtensionStorage.extension_id == extension_id] + filters: list[object] = [ + ExtensionStorage.extension_id == extension_id, + ExtensionStorage.access == access.value, + ] if user_fk is None: filters.append(ExtensionStorage.user_fk.is_(None)) else: @@ -396,6 +434,7 @@ def get( user_fk: int | None = None, resource_type: str | None = None, resource_uuid: str | None = None, + access: StorageAccess = StorageAccess.FRONTEND, ) -> ExtensionStorage | None: """Return the raw storage entry. The value field may be encrypted.""" entry = ( @@ -403,7 +442,12 @@ def get( .filter( and_( *_scope_filter( - extension_id, key, user_fk, resource_type, resource_uuid + extension_id, + key, + user_fk, + resource_type, + resource_uuid, + access, ) ) ) @@ -418,10 +462,11 @@ def get_value( user_fk: int | None = None, resource_type: str | None = None, resource_uuid: str | None = None, + access: StorageAccess = StorageAccess.FRONTEND, ) -> bytes | None: """Return the raw (decrypted) value bytes, or None if not found.""" entry = ExtensionStorageDAO.get( - extension_id, key, user_fk, resource_type, resource_uuid + extension_id, key, user_fk, resource_type, resource_uuid, access ) if entry is None: return None @@ -434,13 +479,14 @@ def get_decoded_value( user_fk: int | None = None, resource_type: str | None = None, resource_uuid: str | None = None, + access: StorageAccess = StorageAccess.FRONTEND, ) -> Any: """Return the value decoded with the codec it was written with. :returns: The decoded value, or None if not found. """ entry = ExtensionStorageDAO.get( - extension_id, key, user_fk, resource_type, resource_uuid + extension_id, key, user_fk, resource_type, resource_uuid, access ) if entry is None: return None @@ -471,6 +517,7 @@ def list_entries( resource_uuid: str | None = None, page: int = 0, page_size: int = 10, + access: StorageAccess = StorageAccess.FRONTEND, ) -> tuple[list[ExtensionStorageListEntry], int]: """List entries in the given scope, most recently changed first. @@ -493,7 +540,7 @@ def list_entries( combined value size exceeds MAX_LIST_PAYLOAD_SIZE. """ filters = _list_scope_filter( - extension_id, user_fk, resource_type, resource_uuid + extension_id, user_fk, resource_type, resource_uuid, access ) base_query = db.session.query(ExtensionStorage).filter(and_(*filters)) total_count = base_query.count() @@ -549,8 +596,12 @@ def set( resource_type: str | None = None, resource_uuid: str | None = None, encrypt: bool = False, + access: StorageAccess = StorageAccess.FRONTEND, ) -> ExtensionStorage: - """Upsert a storage entry. Encrypts value when encrypt=True. + """Upsert a storage entry. + + Values are encrypted when ``encrypt`` is true. Backend-only values + are always encrypted regardless of the caller-supplied flag. The select-then-insert/update below is guarded by a distributed lock scoped to the whole extension, for two independent reasons: @@ -582,10 +633,9 @@ def set( """ _validate_key_length(key) _validate_value_size(value) + encrypt = encrypt or access is StorageAccess.BACKEND stored_value = ( - _enc_type(user_fk).process_bind_param(value, db.engine.dialect) - if encrypt - else value + _encrypt_value(value, user_fk, db.engine.dialect) if encrypt else value ) with DistributedLock( @@ -598,7 +648,12 @@ def set( .filter( and_( *_scope_filter( - extension_id, key, user_fk, resource_type, resource_uuid + extension_id, + key, + user_fk, + resource_type, + resource_uuid, + access, ) ) ) @@ -612,6 +667,7 @@ def set( entry.value_size = new_size entry.codec = codec entry.is_encrypted = encrypt + entry.access = access.value else: entry = ExtensionStorage( extension_id=extension_id, @@ -623,6 +679,7 @@ def set( resource_type=resource_type, resource_uuid=resource_uuid, is_encrypted=encrypt, + access=access.value, ) db.session.add(entry) db.session.flush() @@ -668,6 +725,7 @@ def delete_by_key( user_fk: int | None = None, resource_type: str | None = None, resource_uuid: str | None = None, + access: StorageAccess = StorageAccess.FRONTEND, ) -> bool: """Delete a single entry by key. Returns True if a row was removed. @@ -681,7 +739,12 @@ def delete_by_key( .filter( and_( *_scope_filter( - extension_id, key, user_fk, resource_type, resource_uuid + extension_id, + key, + user_fk, + resource_type, + resource_uuid, + access, ) ) ) diff --git a/superset/extensions/storage/persistent_model.py b/superset/extensions/storage/persistent_model.py index 1adc15db719c..f71282a18e42 100644 --- a/superset/extensions/storage/persistent_model.py +++ b/superset/extensions/storage/persistent_model.py @@ -31,6 +31,7 @@ from sqlalchemy_utils import UUIDType from superset_core.extensions.storage.models import ( ExtensionStorageEntry as CoreExtensionStorageEntry, + StorageAccess, ) from superset.models.helpers import AuditMixinNullable @@ -103,6 +104,12 @@ class ExtensionStorage(CoreExtensionStorageEntry, AuditMixinNullable, Model): value_size = Column(Integer, nullable=False) codec = Column(String(255), nullable=False, default="json") is_encrypted = Column(Boolean, nullable=False, default=False) + access = Column( + String(32), + nullable=False, + default=StorageAccess.FRONTEND.value, + server_default=StorageAccess.FRONTEND.value, + ) __table_args__ = ( # user_fk, resource_type, and resource_uuid are nullable, and @@ -114,6 +121,7 @@ class ExtensionStorage(CoreExtensionStorageEntry, AuditMixinNullable, Model): "user_fk", "resource_type", "resource_uuid", + "access", "key", name="uq_extension_storage_scoped_key", ), @@ -124,6 +132,7 @@ class ExtensionStorage(CoreExtensionStorageEntry, AuditMixinNullable, Model): "user_fk", "resource_type", "resource_uuid", + "access", "key", ), # Covers both extension_id-only lookups (leftmost prefix) and the diff --git a/superset/extensions/storage/secrets.py b/superset/extensions/storage/secrets.py new file mode 100644 index 000000000000..35398fdd8c0a --- /dev/null +++ b/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() + def set(key: str, value: Any) -> None: + """Store a JSON-encoded secret encrypted at rest.""" + extension_id = get_current_extension_id("secrets_state") + user_id = get_current_user_id("secrets_state") + ExtensionStorageDAO.set( + extension_id, + key, + get_codec(DEFAULT_CODEC).encode(value), + codec=DEFAULT_CODEC, + user_fk=user_id, + encrypt=True, + access=StorageAccess.BACKEND, + ) + + @staticmethod + @transaction() + def remove(key: str) -> None: + """Remove the current user's secret.""" + extension_id = get_current_extension_id("secrets_state") + user_id = get_current_user_id("secrets_state") + ExtensionStorageDAO.delete_by_key( + extension_id, + key, + user_fk=user_id, + access=StorageAccess.BACKEND, + ) diff --git a/superset/extensions/storage/utils.py b/superset/extensions/storage/utils.py index bdd9a537a9dd..57cb37529b4e 100644 --- a/superset/extensions/storage/utils.py +++ b/superset/extensions/storage/utils.py @@ -103,7 +103,10 @@ def get_current_user_id(caller: str) -> int: :param caller: Name of the calling accessor (e.g. "ephemeral_state", "persistent_state"), used in the error message. """ - user_id = get_user_id() + context = get_current_extension_context() + user_id = context.user_id if context is not None else None + if user_id is None: + user_id = get_user_id() if user_id is None: raise RuntimeError( f"{caller} requires an authenticated user. " diff --git a/superset/migrations/versions/2026-09-28_00-00_4c8f2a1d9b73_add_extension_storage_access.py b/superset/migrations/versions/2026-09-28_00-00_4c8f2a1d9b73_add_extension_storage_access.py new file mode 100644 index 000000000000..58ca5cddc85c --- /dev/null +++ b/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: 95d8a99c822e +Create Date: 2026-09-28 00:00:00.000000 +""" + +import sqlalchemy as sa +from alembic import op + +revision = "4c8f2a1d9b73" +down_revision = "95d8a99c822e" + +_LOOKUP_COLUMNS = [ + "extension_id", + "user_fk", + "resource_type", + "resource_uuid", + "access", + "key", +] +_LEGACY_LOOKUP_COLUMNS = [ + "extension_id", + "user_fk", + "resource_type", + "resource_uuid", + "key", +] + + +def upgrade() -> None: + with op.batch_alter_table("extension_storage") as batch_op: + batch_op.add_column( + sa.Column( + "access", + sa.String(length=32), + server_default="frontend", + nullable=False, + ) + ) + batch_op.drop_constraint( + "uq_extension_storage_scoped_key", + type_="unique", + ) + batch_op.create_unique_constraint( + "uq_extension_storage_scoped_key", + _LOOKUP_COLUMNS, + ) + batch_op.drop_index("ix_ext_storage_lookup") + batch_op.create_index( + "ix_ext_storage_lookup", + _LOOKUP_COLUMNS, + unique=False, + ) + + +def downgrade() -> None: + # Backend entries must never become browser-readable after a downgrade. + op.execute( + sa.delete(sa.table("extension_storage", sa.column("access"))).where( + sa.column("access") == "backend" + ) + ) + with op.batch_alter_table("extension_storage") as batch_op: + batch_op.drop_constraint( + "uq_extension_storage_scoped_key", + type_="unique", + ) + batch_op.create_unique_constraint( + "uq_extension_storage_scoped_key", + _LEGACY_LOOKUP_COLUMNS, + ) + batch_op.drop_index("ix_ext_storage_lookup") + batch_op.create_index( + "ix_ext_storage_lookup", + _LEGACY_LOOKUP_COLUMNS, + unique=False, + ) + batch_op.drop_column("access") diff --git a/superset/semantic_layers/api.py b/superset/semantic_layers/api.py index d37b3c891dbf..a695af7fd680 100644 --- a/superset/semantic_layers/api.py +++ b/superset/semantic_layers/api.py @@ -18,6 +18,8 @@ import logging import time + +from dataclasses import asdict from typing import Any from flask import Blueprint, current_app, make_response, request, Response @@ -29,6 +31,7 @@ from marshmallow import ValidationError from pydantic import ValidationError as PydanticValidationError from sqlalchemy.orm import load_only, Query +from superset_core.semantic_layers.layer import SemanticLayerAuthenticationRequiredError from superset import db, event_logger, is_feature_enabled, security_manager from superset.commands.semantic_layer.create import ( @@ -57,6 +60,8 @@ ) from superset.commands.semantic_layer.update import ( ClearSemanticLayerCacheCommand, + + unmask_stored_configuration, UpdateSemanticLayerCommand, UpdateSemanticViewCommand, ) @@ -69,7 +74,7 @@ from superset.semantic_layers.access import is_semantic_layers_enabled from superset.semantic_layers.masking import mask_configuration from superset.semantic_layers.models import SemanticLayer, SemanticView -from superset.semantic_layers.registry import registry +from superset.semantic_layers.registry import registry, semantic_layer_context from superset.semantic_layers.schemas import ( SemanticLayerPostSchema, SemanticLayerPutSchema, @@ -760,6 +765,9 @@ class SemanticLayerRestApi(_RuntimeGuardedApiMixin, BaseSupersetApi): "types": "read", "clear_cache": "write", "configuration_schema": "read", + "configuration_action": "write", + "configuration_actions": "read", + "semantic_layer_action": "read", "runtime_schema": "read", # ``read`` (not the default ``can_views`` / ``can_connections``) so # these stay broadly accessible: ``SemanticLayer`` is in @@ -830,43 +838,173 @@ def configuration_schema(self) -> FlaskResponse: if not cls: return self.response_400(message=f"Unknown type: {sl_type}") - # Enriching the schema hands the submitted configuration to the - # provider, which may contact the hosts it names. That is part of - # configuring a layer, so it requires write access; read-only callers - # get the static schema. - parsed_config = None - if (config := body.get("configuration")) and security_manager.can_access( - "can_write", "SemanticLayer" - ): - parsed_config = _parse_partial_config(cls, config) - - warning: str | None = None - try: - schema = cls.get_configuration_schema(parsed_config) - except Exception: # pylint: disable=broad-except - # Show a stable, user-friendly message in the UI; the exception - # detail goes to the server log below. - warning = str( - t( - "Could not load metadata for this configuration; " - "showing the default form. See the server logs for details." + with semantic_layer_context(sl_type): # type: ignore[arg-type] + # Enriching the schema hands the submitted configuration to the + # provider, which may contact the hosts it names. That is part of + # configuring a layer, so it requires write access; read-only callers + # get the static schema. + parsed_config = None + if (config := body.get("configuration")) and security_manager.can_access( + "can_write", "SemanticLayer" + ): + parsed_config = _parse_partial_config(cls, config) + + warning: str | None = None + try: + schema = cls.get_configuration_schema(parsed_config) + except Exception: # pylint: disable=broad-except + # Show a stable, user-friendly message in the UI; the exception + # detail goes to the server log below. + warning = str( + t( + "Could not load metadata for this configuration; " + "showing the default form. See the server logs for details." + ) ) - ) - logger.exception( - "Error enriching semantic layer configuration schema for type %s", - sl_type, - ) - # Connection or query failures during schema enrichment should not - # prevent the form from rendering — return the base schema instead. - schema = cls.get_configuration_schema(None) + logger.exception( + "Error enriching semantic layer configuration schema for type %s", + sl_type, + ) + # Connection or query failures during schema enrichment should not + # prevent the form from rendering — return the base schema instead. + schema = cls.get_configuration_schema(None) + + actions = [ + asdict(action) + for action in cls.get_configuration_actions(parsed_config) + ] - payload: dict[str, Any] = {"result": schema} + payload: dict[str, Any] = {"result": schema, "actions": actions} if warning: payload["warning"] = warning resp = make_response(json.dumps(payload, sort_keys=False), 200) resp.headers["Content-Type"] = "application/json; charset=utf-8" return resp + @expose( + "/type//actions/", + methods=("POST",), + ) + @protect() + @safe + @statsd_metrics + @requires_json + def configuration_action( + self, + sl_type: str, + action_id: str, + ) -> FlaskResponse: + """Start a configuration action before a semantic layer exists.""" + cls = registry.get(sl_type) + if not cls: + return self.response_400(message=f"Unknown type: {sl_type}") + + body = request.json or {} + configuration = body.get("configuration") or {} + if not isinstance(configuration, dict): + return self.response_400(message="Configuration must be an object") + + if semantic_layer_uuid := body.get("semantic_layer_uuid"): + layer = SemanticLayerDAO.find_by_uuid(semantic_layer_uuid) + if not layer: + return self.response_404() + try: + layer.raise_for_access() + except SupersetSecurityException as ex: + return self.response(403, message=ex.message) + if layer.type != sl_type: + return self.response_400( + message="Semantic layer type does not match the requested type" + ) + try: + configuration = unmask_stored_configuration( + layer.configuration, + configuration, + layer.type, + ) + except SemanticLayerInvalidError as ex: + return self.response_400(message=str(ex)) + + with semantic_layer_context(sl_type): + parsed_config = _parse_partial_config(cls, configuration) + try: + result = cls.execute_configuration_action( + action_id, + parsed_config, + body.get("return_url"), + ) + except ValueError as ex: + return self.response_400(message=str(ex)) + + return self.response(200, result=asdict(result)) + + @expose("//actions", methods=("GET",)) + @protect() + @safe + @statsd_metrics + def configuration_actions(self, uuid: str) -> FlaskResponse: + """Return configuration actions for an existing semantic layer.""" + layer = SemanticLayerDAO.find_by_uuid(uuid) + if not layer: + return self.response_404() + try: + layer.raise_for_access() + except SupersetSecurityException as ex: + return self.response(403, message=ex.message) + + cls = registry.get(layer.type) + if not cls: + return self.response_400(message=f"Unknown type: {layer.type}") + configuration = layer.configuration + if isinstance(configuration, str): + configuration = json.loads(configuration) + + with semantic_layer_context(layer.type): + parsed_config = _parse_partial_config(cls, configuration or {}) + actions = [ + asdict(action) + for action in cls.get_configuration_actions(parsed_config) + ] + return self.response(200, result=actions) + + @expose("//actions/", methods=("POST",)) + @protect() + @safe + @statsd_metrics + def semantic_layer_action( + self, + uuid: str, + action_id: str, + ) -> FlaskResponse: + """Start a personal configuration action for an existing layer.""" + layer = SemanticLayerDAO.find_by_uuid(uuid) + if not layer: + return self.response_404() + try: + layer.raise_for_access() + except SupersetSecurityException as ex: + return self.response(403, message=ex.message) + + cls = registry.get(layer.type) + if not cls: + return self.response_400(message=f"Unknown type: {layer.type}") + configuration = layer.configuration + if isinstance(configuration, str): + configuration = json.loads(configuration) + body = request.get_json(silent=True) or {} + + with semantic_layer_context(layer.type): + parsed_config = _parse_partial_config(cls, configuration or {}) + try: + result = cls.execute_configuration_action( + action_id, + parsed_config, + body.get("return_url"), + ) + except ValueError as ex: + return self.response_400(message=str(ex)) + return self.response(200, result=asdict(result)) + @expose("//schema/runtime", methods=("POST",)) @protect() @safe @@ -919,10 +1057,11 @@ def runtime_schema(self, uuid: str) -> FlaskResponse: return self.response_400(message=f"Unknown type: {layer.type}") try: - schema = cls.get_runtime_schema( - layer.implementation.configuration, # type: ignore[attr-defined] - runtime_data, - ) + with semantic_layer_context(layer.type): + schema = cls.get_runtime_schema( + layer.implementation.configuration, + runtime_data, + ) except SupersetSecurityException as ex: return self.response(403, message=ex.message, errors=[ex.to_dict()]) except Exception: # pylint: disable=broad-except @@ -986,6 +1125,15 @@ def views(self, uuid: str) -> FlaskResponse: views = layer.implementation.get_semantic_views(runtime_data) except SupersetSecurityException as ex: return self.response(403, message=ex.message, errors=[ex.to_dict()]) + + except SemanticLayerAuthenticationRequiredError as ex: + return self.response( + 428, + message=ex.message, + error_type="SEMANTIC_LAYER_AUTHENTICATION_REQUIRED", + action_id=ex.action_id, + semantic_layer_uuid=str(layer.uuid), + ) except Exception as ex: # pylint: disable=broad-except logger.error( "Error fetching semantic views for layer %s: %s", diff --git a/superset/semantic_layers/masking.py b/superset/semantic_layers/masking.py index b0c07f30ea6c..5bc957586a41 100644 --- a/superset/semantic_layers/masking.py +++ b/superset/semantic_layers/masking.py @@ -66,7 +66,7 @@ from superset_core.semantic_layers.layer import SemanticLayer as CoreSemanticLayer from superset.constants import PASSWORD_MASK -from superset.semantic_layers.registry import registry +from superset.semantic_layers.registry import registry, semantic_layer_context logger: logging.Logger = logging.getLogger(__name__) @@ -361,7 +361,8 @@ def mask_configuration(layer_type: str, configuration: Any) -> dict[str, Any]: # The connector's own published shape (the same source #43474's # top-level masker used); this walker extends it to nested/union # secrets rather than only top-level ``writeOnly`` properties. - schema: JsonSchema = cls.get_configuration_schema() + with semantic_layer_context(layer_type): + schema: JsonSchema = cls.get_configuration_schema() except Exception: # pylint: disable=broad-except logger.warning( "Masking semantic layer type %s: schema generation failed", layer_type diff --git a/superset/semantic_layers/models.py b/superset/semantic_layers/models.py index bb04feb76318..46000e83dd7a 100644 --- a/superset/semantic_layers/models.py +++ b/superset/semantic_layers/models.py @@ -81,7 +81,11 @@ SemanticBindingMemo, ) from superset.semantic_layers.mapper import get_results -from superset.semantic_layers.registry import registry +from superset.semantic_layers.registry import ( + contextualize_semantic_layer, + registry, + semantic_layer_context, +) from superset.utils import json from superset.utils.core import GenericDataType, TIME_COMPARISON diff --git a/superset/semantic_layers/registry.py b/superset/semantic_layers/registry.py index 076d1aa87de3..81e3c5d59172 100644 --- a/superset/semantic_layers/registry.py +++ b/superset/semantic_layers/registry.py @@ -17,8 +17,152 @@ from __future__ import annotations -from typing import Any +from contextlib import contextmanager, nullcontext +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.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) -> 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 context: + yield + + +class ContextualSemanticView: + """Forward semantic view operations inside their extension context.""" + + def __init__(self, semantic_layer_id: str, implementation: Any): + self._semantic_layer_id = semantic_layer_id + self._implementation = implementation + + def __getattr__(self, name: str) -> Any: + with semantic_layer_context(self._semantic_layer_id): + return getattr(self._implementation, name) + + def uid(self) -> str: + with semantic_layer_context(self._semantic_layer_id): + return self._implementation.uid() + + def get_dimensions(self) -> Any: + with semantic_layer_context(self._semantic_layer_id): + return self._implementation.get_dimensions() + + def get_metrics(self) -> Any: + with semantic_layer_context(self._semantic_layer_id): + return self._implementation.get_metrics() + + def get_values(self, dimension: Any, filters: Any = None) -> Any: + with semantic_layer_context(self._semantic_layer_id): + return self._implementation.get_values(dimension, filters) + + def get_table(self, query: Any) -> Any: + with semantic_layer_context(self._semantic_layer_id): + return self._implementation.get_table(query) + + def get_row_count(self, query: Any) -> Any: + with semantic_layer_context(self._semantic_layer_id): + return self._implementation.get_row_count(query) + + def get_compatible_metrics( + self, + selected_metrics: Any, + selected_dimensions: Any, + ) -> Any: + with semantic_layer_context(self._semantic_layer_id): + return self._implementation.get_compatible_metrics( + selected_metrics, + selected_dimensions, + ) + + def get_compatible_dimensions( + self, + selected_metrics: Any, + selected_dimensions: Any, + ) -> Any: + with semantic_layer_context(self._semantic_layer_id): + return self._implementation.get_compatible_dimensions( + selected_metrics, + selected_dimensions, + ) + + +class ContextualSemanticLayer: + """Forward semantic layer operations inside their extension context.""" + + def __init__(self, semantic_layer_id: str, implementation: Any): + self._semantic_layer_id = semantic_layer_id + self._implementation = implementation + + def __getattr__(self, name: str) -> Any: + with semantic_layer_context(self._semantic_layer_id): + return getattr(self._implementation, name) + + def get_semantic_views(self, runtime_configuration: dict[str, Any]) -> set[Any]: + with semantic_layer_context(self._semantic_layer_id): + views = self._implementation.get_semantic_views(runtime_configuration) + return {ContextualSemanticView(self._semantic_layer_id, view) for view in views} + + def get_semantic_view( + self, + name: str, + additional_configuration: dict[str, Any], + ) -> ContextualSemanticView: + with semantic_layer_context(self._semantic_layer_id): + view = self._implementation.get_semantic_view( + name, + additional_configuration, + ) + return ContextualSemanticView(self._semantic_layer_id, view) + + +def contextualize_semantic_layer( + semantic_layer_id: str, + implementation: Any, +) -> Any: + """Wrap extension providers while leaving host providers unchanged.""" + if semantic_layer_id not in _extension_manifests: + return implementation + return ContextualSemanticLayer(semantic_layer_id, implementation) diff --git a/tests/unit_tests/extensions/api_context_test.py b/tests/unit_tests/extensions/api_context_test.py new file mode 100644 index 000000000000..058146e41083 --- /dev/null +++ b/tests/unit_tests/extensions/api_context_test.py @@ -0,0 +1,52 @@ +# 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 unittest.mock import MagicMock, patch + +from flask_appbuilder.api import expose + +from superset.core.api.core_api_injection import _contextualize_extension_api + + +def test_extension_api_methods_run_in_extension_context() -> None: + manifest = MagicMock() + + class TestApi: + @expose("/callback", methods=("GET",)) + def callback(self) -> str: + return "ok" + + def helper(self) -> str: + return "helper" + + original_helper = TestApi.helper + with ( + patch( + "superset.core.api.core_api_injection.extension_context" + ) as extension_context, + patch( + "superset.core.api.core_api_injection.get_user_id", + return_value=42, + ), + ): + _contextualize_extension_api(TestApi, manifest) + + assert TestApi().callback() == "ok" + + extension_context.assert_called_once_with(manifest, user_id=42) + assert TestApi.helper is original_helper + assert TestApi.callback._urls == [("/callback", ("GET",))] diff --git a/tests/unit_tests/extensions/storage/test_api.py b/tests/unit_tests/extensions/storage/test_api.py index e835fb8443d1..e3f7a1586031 100644 --- a/tests/unit_tests/extensions/storage/test_api.py +++ b/tests/unit_tests/extensions/storage/test_api.py @@ -32,6 +32,7 @@ from flask import Flask, g from flask_babel import Babel +from superset_core.extensions.storage.models import StorageAccess from superset.extensions.storage.api import ExtensionStorageRestApi from superset.extensions.storage.codecs import get_codec @@ -733,6 +734,7 @@ def test_persistent_set_defaults_codec_to_json( codec="json", user_fk=42, encrypt=False, + access=StorageAccess.FRONTEND, ) @@ -773,6 +775,7 @@ def test_persistent_set_with_binary_flag_decodes_base64( codec="binary", user_fk=42, encrypt=False, + access=StorageAccess.FRONTEND, ) @@ -943,6 +946,7 @@ def test_list_persistent_returns_decoded_entries_and_count( resource_uuid=None, page=0, page_size=10, + access=StorageAccess.FRONTEND, ) @@ -1063,6 +1067,7 @@ def test_list_persistent_passes_query_params_to_dao( resource_uuid="uuid-1", page=2, page_size=25, + access=StorageAccess.FRONTEND, ) diff --git a/tests/unit_tests/extensions/storage/test_context.py b/tests/unit_tests/extensions/storage/test_context.py index 732b08ae09df..f3ffaebf6ac4 100644 --- a/tests/unit_tests/extensions/storage/test_context.py +++ b/tests/unit_tests/extensions/storage/test_context.py @@ -98,7 +98,7 @@ def test_context_provides_extension_property(): def test_context_provides_storage_property(): - """Context provides storage property with ephemeral tier.""" + """Context provides browser storage and backend secrets accessors.""" manifest = create_manifest() ctx = ConcreteExtensionContext(manifest) @@ -106,6 +106,16 @@ def test_context_provides_storage_property(): storage = get_context().storage assert storage is not None assert hasattr(storage, "ephemeral") + assert hasattr(storage, "persistent") + assert hasattr(storage, "secrets") + + +def test_extension_context_carries_explicit_user_id(): + """Background execution can carry the initiating principal explicitly.""" + manifest = create_manifest() + + with extension_context(manifest, user_id=42) as ctx: + assert ctx.user_id == 42 def test_context_exception_still_restores(): diff --git a/tests/unit_tests/extensions/storage/test_persistent_dao.py b/tests/unit_tests/extensions/storage/test_persistent_dao.py index 07bda4968b93..98c720e36158 100644 --- a/tests/unit_tests/extensions/storage/test_persistent_dao.py +++ b/tests/unit_tests/extensions/storage/test_persistent_dao.py @@ -26,13 +26,17 @@ import pytest from flask import Flask from flask_babel import Babel +from sqlalchemy.dialects.sqlite import dialect from sqlalchemy.orm.session import Session +from superset_core.extensions.storage.models import StorageAccess from superset.extensions.storage.codecs import get_codec from superset.extensions.storage.filters import ExtensionStorageFilter from superset.extensions.storage.persistent_dao import ( + _decrypt_value, _derive_key, _enc_type, + _encrypt_value, ExtensionStorageDAO, ExtensionStorageKeyTooLong, ExtensionStorageListPayloadTooLarge, @@ -126,6 +130,7 @@ def _create_entry( resource_type: str | None = None, resource_uuid: str | None = None, is_encrypted: bool = False, + access: StorageAccess = StorageAccess.FRONTEND, ) -> ExtensionStorage: """Insert an ExtensionStorage row directly, bypassing DAO.set() (whose own validation isn't what these `list()` tests are exercising).""" @@ -139,6 +144,7 @@ def _create_entry( resource_type=resource_type, resource_uuid=resource_uuid, is_encrypted=is_encrypted, + access=access.value, ) session.add(entry) session.flush() @@ -233,7 +239,7 @@ def test_dao_get_value_decrypts_encrypted_entry( entry.value = b"encrypted-bytes" mock_db.session.query.return_value.filter.return_value.first.return_value = entry mock_enc_type.return_value.process_result_value.return_value = ( - b'{"decrypted": true}' + "superset-extension-storage:v1:eyJkZWNyeXB0ZWQiOiB0cnVlfQ==" ) with app.app_context(): @@ -377,7 +383,7 @@ def test_dao_set_encrypts_value_when_requested( mock_enc_type.assert_called_once_with(1) mock_enc_type.return_value.process_bind_param.assert_called_once_with( - b"plaintext", mock_db.engine.dialect + "superset-extension-storage:v1:cGxhaW50ZXh0", mock_db.engine.dialect ) added_entry = mock_db.session.add.call_args[0][0] assert added_entry.value == b"ciphertext" @@ -385,6 +391,34 @@ def test_dao_set_encrypts_value_when_requested( assert added_entry.is_encrypted is True +@patch("superset.extensions.storage.persistent_dao._enc_type") +@patch("superset.extensions.storage.persistent_dao.db") +def test_dao_set_forces_encryption_for_backend_access( + mock_db: MagicMock, + mock_enc_type: MagicMock, + app: Flask, +) -> None: + """Backend-only values are encrypted even when encrypt is omitted.""" + mock_db.session.query.return_value.filter.return_value.first.return_value = None + mock_enc_type.return_value.process_bind_param.return_value = b"ciphertext" + + with app.app_context(): + ExtensionStorageDAO.set( + "my-ext", + "token", + b"plaintext", + user_fk=1, + access=StorageAccess.BACKEND, + ) + + added_entry = mock_db.session.add.call_args[0][0] + assert added_entry.is_encrypted is True + assert added_entry.access == StorageAccess.BACKEND.value + mock_enc_type.return_value.process_bind_param.assert_called_once_with( + "superset-extension-storage:v1:cGxhaW50ZXh0", mock_db.engine.dialect + ) + + # ── value size ──────────────────────────────────────────────────────────────── @@ -792,6 +826,31 @@ def test_enc_type_rejects_plaintext_that_is_neither_base64_nor_bytes( with pytest.raises(binascii.Error): enc.process_result_value(ciphertext, None) +@pytest.mark.parametrize("engine", ["aes", "aes-gcm"]) +def test_encrypted_value_round_trip_preserves_arbitrary_bytes( + app: Flask, engine: str +) -> None: + """The real encryption types round-trip binary codec output.""" + value = b"\x00\x80binary\xff" + app.config["SQLALCHEMY_ENCRYPTED_FIELD_ENGINE"] = engine + + with app.app_context(): + encrypted = _encrypt_value(value, user_fk=42, dialect=dialect()) + assert _decrypt_value(encrypted, user_fk=42, dialect=dialect()) == value + + +def test_decrypt_value_reads_legacy_bytes_representation(app: Flask) -> None: + """Secrets written by the initial byte-based implementation remain readable.""" + with app.app_context(): + encrypted = _enc_type(user_fk=42).process_bind_param( + b'{"access_token":"token"}', + dialect(), + ) + + assert _decrypt_value(encrypted, user_fk=42, dialect=dialect()) == ( + b'{"access_token":"token"}' + ) + # ── list ────────────────────────────────────────────────────────────────────── @@ -841,6 +900,28 @@ def test_list_scopes_by_user_fk(app: Flask, list_session: Session) -> None: assert entries[0].key == "mine" +def test_list_scopes_by_access(app: Flask, list_session: Session) -> None: + """Frontend listing cannot count or return backend-only entries.""" + _create_entry(list_session, "my-ext", "preference", user_fk=1) + _create_entry( + list_session, + "my-ext", + "oauth2", + user_fk=1, + access=StorageAccess.BACKEND, + ) + + with app.app_context(): + entries, total_count = ExtensionStorageDAO.list_entries( + "my-ext", + user_fk=1, + access=StorageAccess.FRONTEND, + ) + + assert total_count == 1 + assert entries[0].key == "preference" + + def test_list_scopes_by_resource(app: Flask, list_session: Session) -> None: """list() only returns entries matching the given resource_type/uuid.""" _create_entry( @@ -1018,7 +1099,9 @@ def test_list_decrypts_encrypted_entries( mock_db.session.query.return_value.filter.return_value.order_by.return_value ) page_query.offset.return_value.limit.return_value.all.return_value = [entry] - mock_enc_type.return_value.process_result_value.return_value = b"plaintext" + mock_enc_type.return_value.process_result_value.return_value = ( + "superset-extension-storage:v1:cGxhaW50ZXh0" + ) with app.app_context(): entries, total_count = ExtensionStorageDAO.list_entries("my-ext", user_fk=1) diff --git a/tests/unit_tests/extensions/storage/test_secrets.py b/tests/unit_tests/extensions/storage/test_secrets.py new file mode 100644 index 000000000000..1f56e010a42e --- /dev/null +++ b/tests/unit_tests/extensions/storage/test_secrets.py @@ -0,0 +1,112 @@ +# 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. + +"""Tests for backend-only extension secrets storage.""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest +from flask import Flask +from superset_core.extensions.storage.models import StorageAccess + +from superset.extensions.context import use_context +from superset.extensions.storage.secrets import SecretsState +from superset.utils import json +from tests.unit_tests.extensions.storage.conftest import create_context, set_user + + +def test_secrets_state_requires_extension_context(app: Flask) -> None: + """Secrets cannot be accessed outside extension execution.""" + with app.app_context(): + set_user(42) + with pytest.raises(RuntimeError, match="within an extension context"): + SecretsState.get("oauth2") + + +def test_secrets_state_requires_authenticated_user(app: Flask) -> None: + """Secrets cannot be accessed without an authenticated principal.""" + with app.app_context(), use_context(create_context()): + with pytest.raises(RuntimeError, match="requires an authenticated user"): + SecretsState.get("oauth2") + + +@patch("superset.extensions.storage.secrets.ExtensionStorageDAO") +def test_secrets_state_get_is_user_scoped( + mock_dao: MagicMock, + app: Flask, +) -> None: + """Secret reads use backend access and the authenticated user.""" + mock_dao.get_decoded_value.return_value = {"access_token": "token"} + + with app.app_context(), use_context(create_context()): + set_user(42) + result = SecretsState.get("oauth2") + + mock_dao.get_decoded_value.assert_called_once_with( + "test-org.test-ext", + "oauth2", + user_fk=42, + access=StorageAccess.BACKEND, + ) + assert result == {"access_token": "token"} + + +@patch("superset.db") +@patch("superset.extensions.storage.secrets.ExtensionStorageDAO") +def test_secrets_state_set_forces_encryption( + mock_dao: MagicMock, + mock_db: MagicMock, + app: Flask, +) -> None: + """Secret writes are JSON encoded, encrypted, and backend-only.""" + value = {"access_token": "token", "refresh_token": "refresh"} + + with app.app_context(), use_context(create_context()): + set_user(42) + SecretsState.set("oauth2", value) + + mock_dao.set.assert_called_once_with( + "test-org.test-ext", + "oauth2", + json.dumps(value).encode(), + codec="json", + user_fk=42, + encrypt=True, + access=StorageAccess.BACKEND, + ) + + +@patch("superset.db") +@patch("superset.extensions.storage.secrets.ExtensionStorageDAO") +def test_secrets_state_remove_is_backend_only( + mock_dao: MagicMock, + mock_db: MagicMock, + app: Flask, +) -> None: + """Removing a secret cannot remove a frontend entry with the same key.""" + with app.app_context(), use_context(create_context()): + set_user(42) + SecretsState.remove("oauth2") + + mock_dao.delete_by_key.assert_called_once_with( + "test-org.test-ext", + "oauth2", + user_fk=42, + access=StorageAccess.BACKEND, + ) diff --git a/tests/unit_tests/semantic_layers/api_test.py b/tests/unit_tests/semantic_layers/api_test.py index 349dbf46bfc9..e1f2ffc3d2ef 100644 --- a/tests/unit_tests/semantic_layers/api_test.py +++ b/tests/unit_tests/semantic_layers/api_test.py @@ -25,6 +25,10 @@ from flask.testing import FlaskClient from pydantic import BaseModel, Field, model_validator, SecretStr from pytest_mock import MockerFixture +from superset_core.semantic_layers.layer import ( + SemanticLayerAction, + SemanticLayerActionResult, +) from werkzeug.test import TestResponse from superset.commands.semantic_layer.exceptions import ( @@ -362,9 +366,146 @@ def test_configuration_schema( assert response.status_code == 200 assert response.json["result"] == {"type": "object"} + assert response.json["actions"] == [] mock_cls.get_configuration_schema.assert_called_once_with(None) +@SEMANTIC_LAYERS_APP +def test_configuration_schema_returns_actions( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """Configuration schema reports the current user's provider actions.""" + mock_cls = MagicMock() + mock_cls.get_configuration_schema.return_value = {"type": "object"} + mock_cls.get_configuration_actions.return_value = [ + SemanticLayerAction( + id="oauth2", + label="Connect to Quiver", + status="required", + required=True, + ) + ] + mocker.patch.dict( + "superset.semantic_layers.api.registry", + {"quiver": mock_cls}, + clear=True, + ) + + response = client.post( + "/api/v1/semantic_layer/schema/configuration", + json={"type": "quiver"}, + ) + + assert response.status_code == 200 + assert response.json["actions"] == [ + { + "id": "oauth2", + "label": "Connect to Quiver", + "status": "required", + "required": True, + "message": None, + } + ] + + +@SEMANTIC_LAYERS_APP +def test_execute_configuration_action( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """A provider can start OAuth before a semantic layer row exists.""" + parsed_config = MagicMock() + mock_cls = MagicMock() + mock_cls.configuration_class.model_json_schema.return_value = { + "type": "object", + "properties": {}, + } + mock_cls.configuration_class.model_validate.return_value = parsed_config + mock_cls.execute_configuration_action.return_value = SemanticLayerActionResult( + redirect_url="https://auth.example.test/authorize" + ) + mocker.patch.dict( + "superset.semantic_layers.api.registry", + {"quiver": mock_cls}, + clear=True, + ) + + response = client.post( + "/api/v1/semantic_layer/type/quiver/actions/oauth2", + json={ + "configuration": {"host": "quiver.example.test"}, + "return_url": "https://superset.example.test/semantic-layers", + }, + ) + + assert response.status_code == 200 + assert response.json["result"] == { + "redirect_url": "https://auth.example.test/authorize" + } + mock_cls.execute_configuration_action.assert_called_once_with( + "oauth2", + parsed_config, + "https://superset.example.test/semantic-layers", + ) + + +@SEMANTIC_LAYERS_APP +def test_execute_configuration_action_unmasks_existing_layer( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """An edit action resolves masks against the authorized stored layer.""" + parsed_config = MagicMock() + mock_cls = MagicMock() + mock_cls.configuration_class.model_json_schema.return_value = { + "type": "object", + "properties": {}, + } + mock_cls.configuration_class.model_validate.return_value = parsed_config + mock_cls.execute_configuration_action.return_value = SemanticLayerActionResult( + redirect_url="https://auth.example.test/authorize" + ) + mocker.patch.dict( + "superset.semantic_layers.api.registry", + {"quiver": mock_cls}, + clear=True, + ) + layer = MagicMock() + layer.type = "quiver" + layer.configuration = '{"client_secret": "stored-secret"}' + mocker.patch( + "superset.semantic_layers.api.SemanticLayerDAO.find_by_uuid", + return_value=layer, + ) + unmask = mocker.patch( + "superset.semantic_layers.api.unmask_stored_configuration", + return_value={"client_secret": "stored-secret"}, + ) + + response = client.post( + "/api/v1/semantic_layer/type/quiver/actions/oauth2", + json={ + "configuration": {"client_secret": PASSWORD_MASK}, + "semantic_layer_uuid": "11111111-1111-1111-1111-111111111111", + }, + ) + + assert response.status_code == 200 + layer.raise_for_access.assert_called_once_with() + unmask.assert_called_once_with( + layer.configuration, + {"client_secret": PASSWORD_MASK}, + "quiver", + ) + mock_cls.execute_configuration_action.assert_called_once_with( + "oauth2", parsed_config, None + ) + + @SEMANTIC_LAYERS_APP def test_configuration_schema_with_partial_config( client: Any, diff --git a/tests/unit_tests/semantic_layers/decorators_test.py b/tests/unit_tests/semantic_layers/decorators_test.py index 9edd4957db57..b3a0eb9d0ba1 100644 --- a/tests/unit_tests/semantic_layers/decorators_test.py +++ b/tests/unit_tests/semantic_layers/decorators_test.py @@ -40,7 +40,7 @@ def test_inject_semantic_layer_host_context() -> None: from superset.core.api.core_api_injection import ( inject_semantic_layer_implementations, ) - from superset.semantic_layers.registry import registry + from superset.semantic_layers.registry import registry, unregister_semantic_layer # Clear registry for test isolation registry.clear() @@ -65,7 +65,7 @@ class FakeLayer: assert FakeLayer.description == "A test" # type: ignore[attr-defined] # Cleanup - registry.pop("test_layer", None) + unregister_semantic_layer("test_layer") def test_inject_semantic_layer_extension_context() -> None: @@ -73,7 +73,7 @@ def test_inject_semantic_layer_extension_context() -> None: from superset.core.api.core_api_injection import ( inject_semantic_layer_implementations, ) - from superset.semantic_layers.registry import registry + from superset.semantic_layers.registry import registry, unregister_semantic_layer registry.clear() @@ -100,4 +100,4 @@ class ExtLayer: assert registry[expected_id] is ExtLayer # Cleanup - registry.pop(expected_id, None) + unregister_semantic_layer(expected_id) diff --git a/tests/unit_tests/semantic_layers/registry_test.py b/tests/unit_tests/semantic_layers/registry_test.py new file mode 100644 index 000000000000..07116eabb416 --- /dev/null +++ b/tests/unit_tests/semantic_layers/registry_test.py @@ -0,0 +1,69 @@ +# 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. + +"""Tests for contextual semantic layer extension execution.""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +from flask import Flask + +from superset.extensions.context import get_context +from superset.semantic_layers.registry import ( + contextualize_semantic_layer, + register_semantic_layer, + semantic_layer_context, + unregister_semantic_layer, +) +from tests.unit_tests.extensions.storage.conftest import create_manifest + + +def test_semantic_layer_context_restores_extension_and_user(app: Flask) -> None: + """Registered extension providers receive their manifest and principal.""" + manifest = create_manifest("acme", "semantic") + implementation_class = MagicMock() + semantic_layer_id = "extensions.acme.semantic.test" + register_semantic_layer(semantic_layer_id, implementation_class, manifest) + + try: + with app.app_context(): + with semantic_layer_context(semantic_layer_id): + assert get_context().extension is manifest + finally: + unregister_semantic_layer(semantic_layer_id) + + +def test_contextual_layer_wraps_returned_view(app: Flask) -> None: + """Calls on views returned by an extension retain extension context.""" + manifest = create_manifest("acme", "semantic") + semantic_layer_id = "extensions.acme.semantic.test" + implementation_class = MagicMock() + register_semantic_layer(semantic_layer_id, implementation_class, manifest) + + view = MagicMock() + view.uid.side_effect = lambda: get_context().extension.id + layer = MagicMock() + layer.get_semantic_view.return_value = view + + try: + contextual_layer = contextualize_semantic_layer(semantic_layer_id, layer) + with app.app_context(): + contextual_view = contextual_layer.get_semantic_view("orders", {}) + assert contextual_view.uid() == "acme.semantic" + finally: + unregister_semantic_layer(semantic_layer_id) From 1cfbc415126cf32e5cbf56f95ffa4366aace1032 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Thu, 29 Oct 2026 13:37:13 -0400 Subject: [PATCH 02/13] Implement OAUTH2_REDIRECT and per-user cache --- .../extensions/contribution-types.md | 49 ++++ .../superset_core/semantic_layers/layer.py | 7 +- .../src/superset_core/tasks/types.py | 2 + .../ErrorMessage/OAuth2RedirectMessage.tsx | 227 ++++++++---------- .../SemanticLayerModal.test.tsx | 68 +++++- .../semanticLayers/SemanticLayerModal.tsx | 53 ++-- .../AddSemanticViewModal.test.tsx | 69 ++++++ .../semanticViews/AddSemanticViewModal.tsx | 109 ++++++++- .../src/hooks/useOAuth2Completion.ts | 70 ++++++ .../src/middleware/asyncEvent.test.ts | 23 ++ .../src/middleware/asyncEvent.ts | 2 + superset/semantic_layers/api.py | 155 +++++++++--- superset/semantic_layers/models.py | 4 +- superset/semantic_layers/oauth2.py | 67 ++++++ superset/semantic_layers/registry.py | 98 ++++++-- superset/tasks/utils.py | 7 + superset/utils/encrypt.py | 38 ++- tests/unit_tests/semantic_layers/api_test.py | 154 +++++++++++- .../unit_tests/semantic_layers/models_test.py | 18 ++ .../semantic_layers/registry_test.py | 54 +++++ tests/unit_tests/tasks/test_utils.py | 15 ++ tests/unit_tests/utils/encrypt_test.py | 61 +++++ 22 files changed, 1121 insertions(+), 229 deletions(-) create mode 100644 superset-frontend/src/hooks/useOAuth2Completion.ts create mode 100644 superset/semantic_layers/oauth2.py diff --git a/docs/developer_docs/extensions/contribution-types.md b/docs/developer_docs/extensions/contribution-types.md index 563aa8ae540a..98d3b49948a8 100644 --- a/docs/developer_docs/extensions/contribution-types.md +++ b/docs/developer_docs/extensions/contribution-types.md @@ -266,3 +266,52 @@ class MySemanticLayer(SemanticLayer[MyConfig, MySemanticView]): ``` See [Semantic Layers Extension Point](./extension-points/semantic-layers.md) for implementation details. + +#### Personal OAuth2 connections + +Providers can expose a personal connection action by implementing +`get_configuration_actions(configuration)` and +`execute_configuration_action(action_id, configuration, return_url)`. The first +hook returns `SemanticLayerAction` entries describing whether the authenticated +user is connected. The second returns a `SemanticLayerActionResult` containing +the provider's authorization URL. Providers without connection actions can keep +the default implementations. + +Store access and refresh tokens using the Python-only +[`get_context().storage.secrets` accessor](./storage.md#backend-only-secrets). +Never put personal credentials in the shared layer configuration, action results, +query results, logs, or exceptions. The host executes provider methods under the +owning extension and requesting user's context, including background chart work. +Semantic View query results and shared async query tasks are partitioned by user. + +When any provider operation requires authentication, raise the core exception: + +```python +from superset_core.semantic_layers.layer import SemanticLayerAuthenticationRequiredError + +raise SemanticLayerAuthenticationRequiredError("oauth2") +``` + +Superset translates this into the existing `OAUTH2_REDIRECT` error contract for +view discovery, metadata, filter values, and chart queries. The authorization +link opens a protected start route, which checks access to the originating layer +or specific view before invoking the provider's action. A grant on one view is +sufficient to connect a personal account; it does not grant access to sibling +views or permission to edit the layer. + +The host supplies a completion URL as `return_url`. The provider must preserve +that URL in its server-side OAuth state and redirect there **after** validating +the callback and durably saving the authenticated user's tokens. OAuth state, +PKCE validation, token exchange, refresh, and revocation remain the provider's +responsibility. The host completion page sends a correlation ID through +`BroadcastChannel` and the storage-event fallback; it never sends credentials. +The original tab then refreshes the configuration form or retries the failed +operation. Returning browser focus before authorization completes does not +consume the completion notification. Closing an unfinished connection window +allows the user to start another attempt. + +Action POST responses contain `result.redirect_url` and a host-generated +`result.tab_id`. Both the configuration modal and the generic OAuth error UI +use the same completion listener. No access or refresh token is returned in +these responses. + diff --git a/superset-core/src/superset_core/semantic_layers/layer.py b/superset-core/src/superset_core/semantic_layers/layer.py index ff62268c5eaf..bc247a1b7053 100644 --- a/superset-core/src/superset_core/semantic_layers/layer.py +++ b/superset-core/src/superset_core/semantic_layers/layer.py @@ -155,7 +155,12 @@ def execute_configuration_action( configuration: ConfigT | None, return_url: str | None = None, ) -> SemanticLayerActionResult: - """Start an action and return the URL to which the user should navigate.""" + """Start a personal action and return its authorization URL. + + Preserve the host-supplied ``return_url`` in server-side OAuth state. + After validating the callback and durably storing tokens, redirect there + to notify the initiating tab. Never include credentials in the result. + """ raise ValueError(f"Unknown semantic layer action: {action_id}") @classmethod diff --git a/superset-core/src/superset_core/tasks/types.py b/superset-core/src/superset_core/tasks/types.py index 39b2edbcf4b3..fa4db4ac68ce 100644 --- a/superset-core/src/superset_core/tasks/types.py +++ b/superset-core/src/superset_core/tasks/types.py @@ -87,6 +87,8 @@ class TaskProperties(TypedDict, total=False): # failure reason (public); the exception class and traceback are internal # debug detail and live under ``private["framework"]`` instead. error_message: str + # Actionable OAuth errors; credentials and provider exceptions are excluded. + errors: list[dict[str, Any]] # Internal runtime state, surfaced to user-facing API payloads only in debug # mode (the Task REST API strips this key otherwise). Holds framework/task diff --git a/superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx b/superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx index 4642fb23c3a6..b3dcc079e3c7 100644 --- a/superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx +++ b/superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx @@ -16,8 +16,6 @@ * specific language governing permissions and limitations * under the License. */ -import { useEffect, useRef } from 'react'; - import { useDispatch, useSelector } from 'react-redux'; import { QueryEditor, SqlLabRootState } from 'src/SqlLab/types'; import { ExplorePageState } from 'src/explore/types'; @@ -27,50 +25,63 @@ import { triggerQuery } from 'src/components/Chart/chartAction'; import { onRefresh } from 'src/dashboard/actions/dashboardState'; import { UNSAVED_CHART_ID } from 'src/explore/constants'; import { api } from 'src/hooks/apiResources/queryApi'; +import useOAuth2Completion from 'src/hooks/useOAuth2Completion'; import { t } from '@apache-superset/core/translation'; import { QueryResponse } from '@superset-ui/core'; import type { ErrorMessageComponentProps } from './types'; import { ErrorAlert } from './ErrorAlert'; -const OAUTH_CHANNEL_NAME = 'oauth'; -const OAUTH_STORAGE_EVENT_KEY = 'oauth2_auth_complete'; - interface OAuth2RedirectExtra { url: string; tab_id: string; } -/* - * Component for starting OAuth2 dance. - * - * When a user without credentials tries to access a database that supports OAuth2, the - * backend will raise an exception with the custom error `OAUTH2_REDIRECT`. This will - * cause the frontend to display this component, which informs the user that they need - * to authenticate in order to access the data. - * - * The component has a URL that is used to start the OAuth2 dance for the given - * database. When the user clicks the link a new browser tab will open, where they can - * authorize Superset to access the data. Once authorization is successful the user will - * be redirected back to Superset, and their personal access token is stored, so it can - * be used in subsequent connections. If a refresh token is also present in the response, - * it will also be stored. - * - * After the token has been stored, the opened tab will broadcast a message to the - * original tab and close itself. This component, running on the original tab, listens - * for same-origin BroadcastChannel and storage notifications and re-runs the query - * for the user once it receives the success message — be it in SQL Lab, Explore, or - * a dashboard. Both tabs share a "tab ID" (a UUID generated by the backend) which is - * echoed back so the original tab only reacts to its own OAuth2 flow. - */ -export function OAuth2RedirectMessage({ +function OAuth2RedirectPrompt({ + error, + closable, + onComplete, +}: ErrorMessageComponentProps & { + onComplete: () => void | boolean; +}) { + const { extra, level } = error; + useOAuth2Completion(extra.tab_id, onComplete); + + const body = ( +

+ {t( + 'This data source uses OAuth2 for authentication. Please click the link above to grant Apache Superset permission to access the data. Your personal access token will be stored encrypted and used only for queries run by you.', + )} +

+ ); + const subtitle = ( + <> + {t('You need to')}{' '} + + {t('provide authorization')} + {' '} + {t('in order to run this operation.')} + + ); + + return ( + + ); +} + +function OAuth2RedirectMessageWithRedux({ error, source, closable, errorMitigationFunction, }: ErrorMessageComponentProps) { - const { extra, level } = error; - // state needed for re-running the SQL Lab query const queries = useSelector< SqlLabRootState, @@ -104,104 +115,70 @@ export function OAuth2RedirectMessage({ ); const dispatch = useDispatch(); - const lastHandledTabIdRef = useRef(); - - useEffect(() => { - const handleOAuthComplete = (tabId?: string) => { - if (tabId !== extra.tab_id || tabId === lastHandledTabIdRef.current) { - return; - } - - if (errorMitigationFunction) { - errorMitigationFunction(); - } else if (source === 'sqllab' && query) { - dispatch(reRunQuery(query)); - } else if (source === 'explore') { - dispatch(triggerQuery(true, chartId)); - } else if (source === 'dashboard') { - dispatch(onRefresh(chartList.map(Number), true, 0, dashboardId)); - } else if (source === 'crud') { - dispatch( - api.util.invalidateTags([ - { type: 'Schemas', id: 'LIST' }, - { type: 'Catalogs', id: 'LIST' }, - 'Tables', - 'TableMetadatas', - ]), - ); - } else { - return; - } - - lastHandledTabIdRef.current = tabId; - }; - - const channel = - typeof BroadcastChannel !== 'undefined' - ? new BroadcastChannel(OAUTH_CHANNEL_NAME) - : null; - - if (channel) { - channel.onmessage = event => { - handleOAuthComplete(event.data?.tabId); - }; + const handleComplete = () => { + if (errorMitigationFunction) { + errorMitigationFunction(); + } else if (source === 'sqllab' && query) { + dispatch(reRunQuery(query)); + } else if (source === 'explore') { + dispatch(triggerQuery(true, chartId)); + } else if (source === 'dashboard') { + dispatch(onRefresh(chartList.map(Number), true, 0, dashboardId)); + } else if (source === 'crud') { + dispatch( + api.util.invalidateTags([ + { type: 'Schemas', id: 'LIST' }, + { type: 'Catalogs', id: 'LIST' }, + 'Tables', + 'TableMetadatas', + ]), + ); + } else { + return false; } - - const handleStorage = (event: StorageEvent) => { - if (event.key !== OAUTH_STORAGE_EVENT_KEY || !event.newValue) { - return; - } - - try { - const message = JSON.parse(event.newValue) as { tabId?: string }; - handleOAuthComplete(message.tabId); - } catch { - // ignore malformed storage payloads - } - }; - - window.addEventListener('storage', handleStorage); - - return () => { - window.removeEventListener('storage', handleStorage); - channel?.close(); - }; - }, [ - source, - extra.tab_id, - dispatch, - query, - chartId, - chartList, - dashboardId, - errorMitigationFunction, - ]); - - const body = ( -

- {t( - 'This database uses OAuth2 for authentication. Please click the link above to grant Apache Superset permission to access the data. Your personal access token will be stored encrypted and used only for queries run by you.', - )} -

- ); - const subtitle = ( - <> - {t('You need to')}{' '} - - {t('provide authorization')} - {' '} - {t('in order to run this operation.')} - - ); - + return true; + }; return ( - ); } + +/* + * Component for starting OAuth2 dance. + * + * When a user without credentials tries to access a database that supports OAuth2, the + * backend will raise an exception with the custom error `OAUTH2_REDIRECT`. This will + * cause the frontend to display this component, which informs the user that they need + * to authenticate in order to access the data. + * + * The component has a URL that is used to start the OAuth2 dance for the given + * database. When the user clicks the link a new browser tab will open, where they can + * authorize Superset to access the data. Once authorization is successful the user will + * be redirected back to Superset, and their personal access token is stored, so it can + * be used in subsequent connections. If a refresh token is also present in the response, + * it will also be stored. + * + * After the token has been stored, the opened tab will broadcast a message to the + * original tab and close itself. This component, running on the original tab, listens + * for same-origin BroadcastChannel and storage notifications and re-runs the query + * for the user once it receives the success message — be it in SQL Lab, Explore, or + * a dashboard. Both tabs share a "tab ID" (a UUID generated by the backend) which is + * echoed back so the original tab only reacts to its own OAuth2 flow. + */ +export function OAuth2RedirectMessage( + props: ErrorMessageComponentProps, +) { + if (props.errorMitigationFunction) { + return ( + + ); + } + return ; +} diff --git a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx index 855f1458190c..50018c65c41a 100644 --- a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx +++ b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx @@ -763,7 +763,10 @@ test('starts a required semantic layer configuration action', async () => { }) .mockResolvedValueOnce({ json: { - result: { redirect_url: 'https://auth.example.test/authorize' }, + result: { + redirect_url: 'https://auth.example.test/authorize', + tab_id: 'oauth-tab', + }, }, }); const popup = { @@ -785,7 +788,6 @@ test('starts a required semantic layer configuration action', async () => { endpoint: '/api/v1/semantic_layer/type/snowflake/actions/oauth2', jsonPayload: { configuration: expect.any(Object), - return_url: window.location.href, semantic_layer_uuid: props.semanticLayerUuid, }, }); @@ -797,5 +799,67 @@ test('starts a required semantic layer configuration action', async () => { expect(popup.location.href).toBe('https://auth.example.test/authorize'); }); + const callsBeforeCompletion = mockedPost.mock.calls.length; + fireEvent.focus(window); + expect(mockedPost).toHaveBeenCalledTimes(callsBeforeCompletion); + fireEvent( + window, + new StorageEvent('storage', { + key: 'oauth2_auth_complete', + newValue: JSON.stringify({ tabId: 'unrelated-flow' }), + }), + ); + expect(mockedPost).toHaveBeenCalledTimes(callsBeforeCompletion); + fireEvent( + window, + new StorageEvent('storage', { + key: 'oauth2_auth_complete', + newValue: JSON.stringify({ tabId: 'oauth-tab' }), + }), + ); + await waitFor(() => + expect(mockedPost).toHaveBeenCalledTimes(callsBeforeCompletion + 1), + ); + open.mockRestore(); +}); + +test('allows another connection attempt after the popup is cancelled', async () => { + mockedPost + .mockResolvedValueOnce({ + json: { + result: schemaWithExternalDeps, + actions: [ + { + id: 'oauth2', + label: 'Connect', + status: 'required', + required: true, + }, + ], + }, + }) + .mockResolvedValueOnce({ + json: { + result: { + redirect_url: 'https://auth.example.test/authorize', + tab_id: 'cancelled-flow', + }, + }, + }); + const popup = { close: jest.fn(), closed: false, location: { href: '' } }; + const open = jest + .spyOn(window, 'open') + .mockReturnValue(popup as unknown as Window); + render(); + const connect = await screen.findByRole('button', { name: 'Connect' }); + fireEvent.click(connect); + await waitFor(() => expect(popup.location.href).toContain('/authorize')); + expect(connect).toBeDisabled(); + popup.closed = true; + await act(async () => { + jest.advanceTimersByTime(500); + }); + expect(connect).toBeEnabled(); + expect(screen.getByRole('button', { name: 'Save' })).toBeDisabled(); open.mockRestore(); }); diff --git a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx index f5651cc22462..45b160376b42 100644 --- a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx +++ b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx @@ -33,6 +33,7 @@ import { MODAL_MEDIUM_WIDTH, } from 'src/components/Modal'; import { styled } from '@apache-superset/core/theme'; +import useOAuth2Completion from 'src/hooks/useOAuth2Completion'; import { renderers, sanitizeSchema, @@ -144,7 +145,8 @@ export default function SemanticLayerModal({ useState('ValidateAndHide'); const errorsRef = useRef([]); const debounceTimerRef = useRef | null>(null); - const oauthFocusListenerRef = useRef<(() => void) | null>(null); + const [oauthTabId, setOauthTabId] = useState(); + const oauthPopupRef = useRef(null); const lastDepSnapshotRef = useRef(''); const dynamicDepsRef = useRef>({}); const schemaRequestRef = useRef(0); @@ -265,12 +267,8 @@ export default function SemanticLayerModal({ fetchTypes(); } } else { - useEffect( - () => () => { - if (oauthFocusListenerRef.current) { - window.removeEventListener('focus', oauthFocusListenerRef.current); - oauthFocusListenerRef.current = null; - } + setOauthTabId(undefined); + oauthPopupRef.current = null; setStep('type'); setName(''); setSelectedType(null); @@ -329,12 +327,24 @@ export default function SemanticLayerModal({ }; - if (oauthFocusListenerRef.current) { - window.removeEventListener('focus', oauthFocusListenerRef.current); + useOAuth2Completion(oauthTabId, () => { + setOauthTabId(undefined); + setRunningActionId(null); + oauthPopupRef.current = null; + if (selectedType) fetchConfigSchema(selectedType, formData); + }); + + useEffect(() => { + if (!oauthTabId) return undefined; + const timer = window.setInterval(() => { + if (oauthPopupRef.current?.closed) { + setOauthTabId(undefined); + setRunningActionId(null); + oauthPopupRef.current = null; } - }, - [], - ); + }, 500); + return () => window.clearInterval(timer); + }, [oauthTabId]); const handleStepAdvance = () => { if (selectedType) { @@ -415,7 +425,6 @@ export default function SemanticLayerModal({ )}/actions/${encodeURIComponent(action.id)}`, jsonPayload: { configuration: formData, - return_url: window.location.href, ...(semanticLayerUuid ? { semantic_layer_uuid: semanticLayerUuid } : {}), @@ -425,18 +434,13 @@ export default function SemanticLayerModal({ if (typeof redirectUrl !== 'string') { throw new Error(t('The connection action did not return a URL')); } - popup.location.href = redirectUrl; - - const refreshAfterOAuth = () => { - window.removeEventListener('focus', refreshAfterOAuth); - oauthFocusListenerRef.current = null; - fetchConfigSchema(selectedType, formData); - }; - if (oauthFocusListenerRef.current) { - window.removeEventListener('focus', oauthFocusListenerRef.current); + const tabId = json.result?.tab_id; + if (typeof tabId !== 'string') { + throw new Error(t('The connection action did not return a tab ID')); } - oauthFocusListenerRef.current = refreshAfterOAuth; - window.addEventListener('focus', refreshAfterOAuth); + oauthPopupRef.current = popup; + setOauthTabId(tabId); + popup.location.href = redirectUrl; } catch (error) { popup.close(); const clientError = await getClientErrorObject(error); @@ -446,7 +450,6 @@ export default function SemanticLayerModal({ ? error.message : t('An error occurred while starting the connection')), ); - } finally { setRunningActionId(null); } }; diff --git a/superset-frontend/src/features/semanticViews/AddSemanticViewModal.test.tsx b/superset-frontend/src/features/semanticViews/AddSemanticViewModal.test.tsx index eac84aaa3d0d..a4e2d60b15eb 100644 --- a/superset-frontend/src/features/semanticViews/AddSemanticViewModal.test.tsx +++ b/superset-frontend/src/features/semanticViews/AddSemanticViewModal.test.tsx @@ -756,6 +756,10 @@ test('the save payload carries every selected metric', async () => { await waitFor( () => { + expect(mockedPost).toHaveBeenCalledWith({ + endpoint: '/api/v1/semantic_layer/layer-1/views', + jsonPayload: { runtime_data: { metrics: ['m1', 'm2', 'm3'] } }, + }); expect( screen.getByRole('combobox', { name: 'Semantic views' }), ).toBeEnabled(); @@ -1677,3 +1681,68 @@ test('a superseded refresh failure cannot toast or clobber a newer response', as fireEvent.mouseDown(schema); expect(await screen.findByText('third_only')).toBeInTheDocument(); }); + +test.each(['runtime', 'views'])( + 'offers OAuth and retries when %s discovery needs a personal token', + async stage => { + mockedGet.mockResolvedValue({ + json: { result: [{ uuid: 'layer-1', name: 'Personal layer' }] }, + }); + let needsAuthentication = true; + const authorizationUrl = '/api/v1/semantic_layer/layer-1/oauth2/connect'; + mockedPost.mockImplementation(({ endpoint }: { endpoint: string }) => { + const isRuntime = endpoint.endsWith('/schema/runtime'); + if ( + needsAuthentication && + (isRuntime ? stage === 'runtime' : stage === 'views') + ) { + return Promise.reject({ + response: new Response( + JSON.stringify({ + errors: [ + { + error_type: 'OAUTH2_REDIRECT', + message: 'Authorization needed', + level: 'warning', + extra: { url: authorizationUrl, tab_id: 'personal-flow' }, + }, + ], + }), + { status: 403, headers: { 'Content-Type': 'application/json' } }, + ), + }); + } + return Promise.resolve({ + json: { + result: isRuntime + ? { properties: {} } + : [{ name: 'orders', already_added: false }], + }, + }); + }); + const props = createProps(); + render(); + await selectOption('Semantic layer', 'Personal layer'); + const authorize = await screen.findByRole('link', { + name: 'provide authorization', + }); + expect(authorize).toHaveAttribute('href', authorizationUrl); + expect(props.addDangerToast).not.toHaveBeenCalled(); + needsAuthentication = false; + await act(async () => { + window.dispatchEvent( + new StorageEvent('storage', { + key: 'oauth2_auth_complete', + newValue: JSON.stringify({ tabId: 'personal-flow' }), + }), + ); + }); + await waitFor(() => + expect( + screen.queryByRole('link', { name: 'provide authorization' }), + ).not.toBeInTheDocument(), + ); + await selectOption('Semantic views', 'orders'); + expect(screen.getByRole('button', { name: 'Add 1 view(s)' })).toBeEnabled(); + }, +); diff --git a/superset-frontend/src/features/semanticViews/AddSemanticViewModal.tsx b/superset-frontend/src/features/semanticViews/AddSemanticViewModal.tsx index 7db276d8f272..e0a807b5af45 100644 --- a/superset-frontend/src/features/semanticViews/AddSemanticViewModal.tsx +++ b/superset-frontend/src/features/semanticViews/AddSemanticViewModal.tsx @@ -20,7 +20,13 @@ import { useState, useEffect, useCallback, useMemo, useRef } from 'react'; import { t } from '@apache-superset/core/translation'; import { logging } from '@apache-superset/core/utils'; import { styled } from '@apache-superset/core/theme'; -import { SupersetClient } from '@superset-ui/core'; +import { + ErrorTypeEnum, + getClientErrorObject, + SupersetClient, + SupersetError, +} from '@superset-ui/core'; +import { OAuth2RedirectMessage } from 'src/components/ErrorMessage'; import { List, Select } from '@superset-ui/core/components'; import { Alert } from '@apache-superset/core/components'; import { Spin } from '@superset-ui/core/components/Spin'; @@ -205,6 +211,43 @@ export default function AddSemanticViewModal({ // flight for a previous dependency state never land. const viewsGenRef = useRef(0); + const [authentication, setAuthentication] = useState<{ + error: SupersetError<{ url: string; tab_id: string }>; + retry: () => void; + }>(); + const captureAuthenticationError = useCallback( + async ( + error: Parameters[0], + retry: () => void, + isCurrent: () => boolean, + ): Promise => { + const parsed = await getClientErrorObject(error); + if (!isCurrent()) return true; + const oauthError = parsed.errors?.find( + item => item.error_type === ErrorTypeEnum.OAUTH2_REDIRECT, + ); + if ( + oauthError && + typeof oauthError.extra?.url === 'string' && + typeof oauthError.extra?.tab_id === 'string' + ) { + setAuthentication({ + error: { + ...oauthError, + extra: { + url: oauthError.extra.url, + tab_id: oauthError.extra.tab_id, + }, + }, + retry, + }); + return true; + } + return false; + }, + [], + ); + // ========================================================================= // Fetch helpers // ========================================================================= @@ -243,14 +286,24 @@ export default function AddSemanticViewModal({ }); if (!isCurrent()) return; setAvailableViews(json.result ?? []); - } catch { + } catch (error) { if (!isCurrent()) return; + if ( + await captureAuthenticationError( + error, + () => { + fetchViews(uuid, rData, gen); + }, + isCurrent, + ) + ) + return; addDangerToast(t('An error occurred while fetching available views')); } finally { if (isCurrent()) setLoadingViews(false); } }, - [addDangerToast], + [addDangerToast, captureAuthenticationError], ); const lastSchemaJsonRef = useRef(''); @@ -318,12 +371,14 @@ export default function AddSemanticViewModal({ // ========================================================================= const handleLayerChange = useCallback( - async (uuid: string) => { + async (uuid: string): Promise => { fetchGenRef.current += 1; const gen = fetchGenRef.current; setSelectedLayerUuid(uuid); setCreationErrors([]); + + setAuthentication(undefined); if (schemaTimerRef.current) clearTimeout(schemaTimerRef.current); if (viewsTimerRef.current) clearTimeout(viewsTimerRef.current); setRuntimeSchema(null); @@ -362,8 +417,18 @@ export default function AddSemanticViewModal({ } else { applyRuntimeSchema(schema); } - } catch { + } catch (error) { if (gen !== fetchGenRef.current) return; + if ( + await captureAuthenticationError( + error, + () => { + handleLayerChange(uuid); + }, + () => gen === fetchGenRef.current, + ) + ) + return; addDangerToast( t('An error occurred while fetching the runtime schema'), ); @@ -371,7 +436,12 @@ export default function AddSemanticViewModal({ if (gen === fetchGenRef.current) setLoadingRuntime(false); } }, - [applyRuntimeSchema, fetchViews, addDangerToast], + [ + applyRuntimeSchema, + fetchViews, + addDangerToast, + captureAuthenticationError, + ], ); // ========================================================================= @@ -465,6 +535,22 @@ export default function AddSemanticViewModal({ refreshGen !== schemaRefreshGenRef.current ) return; + if ( + await captureAuthenticationError( + error, + () => { + lastDepSnapshotRef.current = ''; + handleRuntimeFormChange({ + data, + errors: errorsRef.current, + }); + }, + () => + gen === fetchGenRef.current && + refreshGen === schemaRefreshGenRef.current, + ) + ) + return; logging.error('Runtime schema refresh failed', error); // Allow exactly one retry per dependency state: clearing the // committed snapshot lets the next change event re-attempt @@ -524,6 +610,7 @@ export default function AddSemanticViewModal({ addDangerToast, staleSchemaOptions, runtimeData, + captureAuthenticationError, ], ); @@ -586,6 +673,7 @@ export default function AddSemanticViewModal({ if (schemaTimerRef.current) clearTimeout(schemaTimerRef.current); if (viewsTimerRef.current) clearTimeout(viewsTimerRef.current); setLayers([]); + setAuthentication(undefined); setSelectedLayerUuid(null); setLoadingLayers(false); setRuntimeSchema(null); @@ -793,6 +881,15 @@ export default function AddSemanticViewModal({ /> } + )} + {authentication && ( + { + setAuthentication(undefined); + authentication.retry(); + }} /> )} {/* Semantic Layer */} diff --git a/superset-frontend/src/hooks/useOAuth2Completion.ts b/superset-frontend/src/hooks/useOAuth2Completion.ts new file mode 100644 index 000000000000..3829e25190c4 --- /dev/null +++ b/superset-frontend/src/hooks/useOAuth2Completion.ts @@ -0,0 +1,70 @@ +/** + * 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. + */ +import { useEffect, useRef } from 'react'; + +const OAUTH_CHANNEL_NAME = 'oauth'; +const OAUTH_STORAGE_EVENT_KEY = 'oauth2_auth_complete'; + +/** Listen for a matching OAuth completion, once per connection attempt. */ +export default function useOAuth2Completion( + tabId: string | undefined, + onComplete: () => void | boolean, +) { + const onCompleteRef = useRef(onComplete); + onCompleteRef.current = onComplete; + const lastHandledTabIdRef = useRef(); + + useEffect(() => { + if (!tabId) return undefined; + const handleCompletion = (message: unknown) => { + if ( + typeof message !== 'object' || + message === null || + !('tabId' in message) || + message.tabId !== tabId || + lastHandledTabIdRef.current === tabId + ) { + return; + } + if (onCompleteRef.current() !== false) { + lastHandledTabIdRef.current = tabId; + } + }; + const channel = + typeof BroadcastChannel !== 'undefined' + ? new BroadcastChannel(OAUTH_CHANNEL_NAME) + : null; + if (channel) { + channel.onmessage = event => handleCompletion(event.data); + } + const handleStorage = (event: StorageEvent) => { + if (event.key !== OAUTH_STORAGE_EVENT_KEY || !event.newValue) return; + try { + handleCompletion(JSON.parse(event.newValue)); + } catch { + // Ignore malformed notifications from other tabs. + } + }; + window.addEventListener('storage', handleStorage); + return () => { + window.removeEventListener('storage', handleStorage); + channel?.close(); + }; + }, [tabId]); +} diff --git a/superset-frontend/src/middleware/asyncEvent.test.ts b/superset-frontend/src/middleware/asyncEvent.test.ts index 6872fe35da2d..2a14450b0e38 100644 --- a/superset-frontend/src/middleware/asyncEvent.test.ts +++ b/superset-frontend/src/middleware/asyncEvent.test.ts @@ -1191,3 +1191,26 @@ test('a stalled status detail does not prevent completeness guidance', async () await rejection; expect(refetch).not.toHaveBeenCalled(); }); + +test('preserves an OAuth authorization prompt from a failed query task', async () => { + const oauthError = { + error_type: 'OAUTH2_REDIRECT', + level: 'warning', + message: 'Authorization needed', + extra: { + url: '/api/v1/semantic_layer/layer/oauth2/connect', + tab_id: 'flow-id', + }, + }; + fetchMock.get('glob:*/api/v1/task/oauth-task', { + status: 200, + body: { result: { properties: { errors: [oauthError] } } }, + }); + queueStatuses({ 'oauth-task': { status: 'failure' } }); + asyncEvent.init(config); + const refetch = jest.fn(); + await expect( + asyncEvent.waitForAsyncData({ task_ids: ['oauth-task'] }, refetch), + ).rejects.toEqual({ errors: [oauthError] }); + expect(refetch).not.toHaveBeenCalled(); +}); diff --git a/superset-frontend/src/middleware/asyncEvent.ts b/superset-frontend/src/middleware/asyncEvent.ts index 42849ab127ae..060b4e598ecb 100644 --- a/superset-frontend/src/middleware/asyncEvent.ts +++ b/superset-frontend/src/middleware/asyncEvent.ts @@ -37,6 +37,8 @@ import { FeatureFlag, makeApi, SupersetClient, + ErrorTypeEnum, + SupersetError, } from '@superset-ui/core'; import { logging } from '@apache-superset/core/utils'; import getBootstrapData from 'src/utils/getBootstrapData'; diff --git a/superset/semantic_layers/api.py b/superset/semantic_layers/api.py index a695af7fd680..3f9b7c2cc56d 100644 --- a/superset/semantic_layers/api.py +++ b/superset/semantic_layers/api.py @@ -21,8 +21,9 @@ from dataclasses import asdict from typing import Any +from uuid import UUID, uuid4 -from flask import Blueprint, current_app, make_response, request, Response +from flask import Blueprint, current_app, make_response, redirect, render_template, request, Response, url_for from flask_appbuilder import AppBuilder from flask_appbuilder.api import BaseApi, expose, protect, rison, safe from flask_appbuilder.api.schemas import get_list_schema @@ -31,7 +32,6 @@ from marshmallow import ValidationError from pydantic import ValidationError as PydanticValidationError from sqlalchemy.orm import load_only, Query -from superset_core.semantic_layers.layer import SemanticLayerAuthenticationRequiredError from superset import db, event_logger, is_feature_enabled, security_manager from superset.commands.semantic_layer.create import ( @@ -69,7 +69,7 @@ from superset.daos.semantic_layer import SemanticLayerDAO from superset.databases.filters import DatabaseFilter from superset.datasets.schemas import get_delete_ids_schema -from superset.exceptions import SupersetSecurityException +from superset.exceptions import OAuth2RedirectError, SupersetSecurityException from superset.models.core import Database from superset.semantic_layers.access import is_semantic_layers_enabled from superset.semantic_layers.masking import mask_configuration @@ -83,6 +83,7 @@ ) from superset.superset_typing import FlaskResponse from superset.utils import json +from superset.utils.core import get_user_id from superset.views.base_api import ( BaseSupersetApi, BaseSupersetModelRestApi, @@ -90,6 +91,7 @@ requires_json, statsd_metrics, ) +from superset.views.error_handling import handle_api_exception logger = logging.getLogger(__name__) @@ -274,6 +276,7 @@ class SemanticViewRestApi(_RuntimeGuardedApiMixin, BaseSupersetModelRestApi): @expose("//structure", methods=("GET",)) @protect() @safe + @handle_api_exception @statsd_metrics @event_logger.log_this_with_context( action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.structure", @@ -341,6 +344,8 @@ def structure(self, pk: int) -> Response: view.implementation.get_metrics(), key=lambda m: m.name ) ] + except OAuth2RedirectError: + raise except SupersetSecurityException as ex: return self.response(403, message=ex.message, errors=[ex.to_dict()]) except Exception: # pylint: disable=broad-except @@ -768,6 +773,8 @@ class SemanticLayerRestApi(_RuntimeGuardedApiMixin, BaseSupersetApi): "configuration_action": "write", "configuration_actions": "read", "semantic_layer_action": "read", + "oauth2_start": "read", + "oauth2_complete": "read", "runtime_schema": "read", # ``read`` (not the default ``can_views`` / ``can_connections``) so # these stay broadly accessible: ``SemanticLayer`` is in @@ -887,6 +894,7 @@ def configuration_schema(self) -> FlaskResponse: ) @protect() @safe + @handle_api_exception @statsd_metrics @requires_json def configuration_action( @@ -925,32 +933,32 @@ def configuration_action( except SemanticLayerInvalidError as ex: return self.response_400(message=str(ex)) + tab_id = str(uuid4()) + completion_url = self._oauth2_completion_url(tab_id) with semantic_layer_context(sl_type): parsed_config = _parse_partial_config(cls, configuration) try: result = cls.execute_configuration_action( action_id, parsed_config, - body.get("return_url"), + completion_url, ) except ValueError as ex: return self.response_400(message=str(ex)) - return self.response(200, result=asdict(result)) + return self.response(200, result={**asdict(result), "tab_id": tab_id}) @expose("//actions", methods=("GET",)) - @protect() + @protect_read("SemanticLayer", "SemanticView") @safe + @handle_api_exception @statsd_metrics def configuration_actions(self, uuid: str) -> FlaskResponse: """Return configuration actions for an existing semantic layer.""" - layer = SemanticLayerDAO.find_by_uuid(uuid) - if not layer: - return self.response_404() - try: - layer.raise_for_access() - except SupersetSecurityException as ex: - return self.response(403, message=ex.message) + layer, error = self._action_layer(uuid, request.args.get("view_uuid")) + if error is not None: + return error + assert layer is not None cls = registry.get(layer.type) if not cls: @@ -968,8 +976,9 @@ def configuration_actions(self, uuid: str) -> FlaskResponse: return self.response(200, result=actions) @expose("//actions/", methods=("POST",)) - @protect() + @protect_read("SemanticLayer", "SemanticView") @safe + @handle_api_exception @statsd_metrics def semantic_layer_action( self, @@ -977,13 +986,10 @@ def semantic_layer_action( action_id: str, ) -> FlaskResponse: """Start a personal configuration action for an existing layer.""" - layer = SemanticLayerDAO.find_by_uuid(uuid) - if not layer: - return self.response_404() - try: - layer.raise_for_access() - except SupersetSecurityException as ex: - return self.response(403, message=ex.message) + layer, error = self._action_layer(uuid, request.args.get("view_uuid")) + if error is not None: + return error + assert layer is not None cls = registry.get(layer.type) if not cls: @@ -991,23 +997,108 @@ def semantic_layer_action( configuration = layer.configuration if isinstance(configuration, str): configuration = json.loads(configuration) - body = request.get_json(silent=True) or {} + tab_id = str(uuid4()) + completion_url = self._oauth2_completion_url(tab_id) with semantic_layer_context(layer.type): parsed_config = _parse_partial_config(cls, configuration or {}) try: result = cls.execute_configuration_action( action_id, parsed_config, - body.get("return_url"), + completion_url, ) except ValueError as ex: return self.response_400(message=str(ex)) - return self.response(200, result=asdict(result)) + return self.response(200, result={**asdict(result), "tab_id": tab_id}) + + @staticmethod + def _oauth2_completion_url(tab_id: str) -> str: + """Return the same-origin page that signals a completed connection.""" + return url_for( + "SemanticLayerRestApi.oauth2_complete", tab_id=tab_id, _external=True + ) + + def _action_layer( + self, uuid: str, view_uuid: str | None = None + ) -> tuple[SemanticLayer | None, FlaskResponse | None]: + """Authorize a personal action using its originating data resource.""" + if not is_feature_enabled("SEMANTIC_LAYERS"): + return None, self.response_404() + if get_user_id() is None: + return None, self.response_401() + layer = SemanticLayerDAO.find_by_uuid(uuid) + if layer is None: + return None, self.response_404() + try: + if view_uuid is None: + layer.raise_for_access() + else: + try: + parsed_uuid = UUID(view_uuid) + except ValueError: + return None, self.response_400(message="Invalid semantic view UUID") + view = ( + db.session.query(SemanticView) + .filter( + SemanticView.uuid == parsed_uuid, + SemanticView.semantic_layer_uuid == layer.uuid, + ) + .one_or_none() + ) + if view is None: + return None, self.response_404() + view.raise_for_access() + except SupersetSecurityException as ex: + return None, self.response(403, message=ex.message) + return layer, None + + @expose("//oauth2/", methods=("GET",)) + @protect_read("SemanticLayer", "SemanticView") + @safe + @handle_api_exception + @statsd_metrics + def oauth2_start(self, uuid: str, action_id: str) -> FlaskResponse: + """Start a personal connection after an OAuth2 error link is clicked.""" + layer, error = self._action_layer(uuid, request.args.get("view_uuid")) + if error is not None: + return error + assert layer is not None + try: + tab_id = str(UUID(request.args.get("tab_id", ""))) + except ValueError: + return self.response_400(message="Invalid OAuth2 tab ID") + cls = registry.get(layer.type) + if cls is None: + return self.response_400(message=f"Unknown type: {layer.type}") + configuration = json.loads(layer.configuration or "{}") + with semantic_layer_context(layer.type): + parsed_config = _parse_partial_config(cls, configuration) + try: + result = cls.execute_configuration_action( + action_id, parsed_config, self._oauth2_completion_url(tab_id) + ) + except ValueError: + return self.response_400(message="Unable to start the connection") + return redirect(result.redirect_url) + + @expose("/oauth2/complete/", methods=("GET",)) + @protect_read("SemanticLayer", "SemanticView") + @safe + def oauth2_complete(self, tab_id: UUID) -> FlaskResponse: + """Notify the initiating tab without exposing credentials to the browser.""" + if not is_feature_enabled("SEMANTIC_LAYERS"): + return self.response_404() + if get_user_id() is None: + return self.response_401() + return make_response( + render_template("superset/oauth2.html", tab_id=str(tab_id)) + ) @expose("//schema/runtime", methods=("POST",)) @protect() @safe + @handle_api_exception @statsd_metrics def runtime_schema(self, uuid: str) -> FlaskResponse: """Get runtime schema for a stored semantic layer. @@ -1057,11 +1148,13 @@ def runtime_schema(self, uuid: str) -> FlaskResponse: return self.response_400(message=f"Unknown type: {layer.type}") try: - with semantic_layer_context(layer.type): + with semantic_layer_context(layer.type, str(layer.uuid)): schema = cls.get_runtime_schema( layer.implementation.configuration, runtime_data, ) + except OAuth2RedirectError: + raise except SupersetSecurityException as ex: return self.response(403, message=ex.message, errors=[ex.to_dict()]) except Exception: # pylint: disable=broad-except @@ -1077,6 +1170,7 @@ def runtime_schema(self, uuid: str) -> FlaskResponse: @expose("//views", methods=("POST",)) @protect() @safe + @handle_api_exception @statsd_metrics def views(self, uuid: str) -> FlaskResponse: """List available views from a semantic layer. @@ -1123,17 +1217,10 @@ def views(self, uuid: str) -> FlaskResponse: try: views = layer.implementation.get_semantic_views(runtime_data) + except OAuth2RedirectError: + raise except SupersetSecurityException as ex: return self.response(403, message=ex.message, errors=[ex.to_dict()]) - - except SemanticLayerAuthenticationRequiredError as ex: - return self.response( - 428, - message=ex.message, - error_type="SEMANTIC_LAYER_AUTHENTICATION_REQUIRED", - action_id=ex.action_id, - semantic_layer_uuid=str(layer.uuid), - ) except Exception as ex: # pylint: disable=broad-except logger.error( "Error fetching semantic views for layer %s: %s", diff --git a/superset/semantic_layers/models.py b/superset/semantic_layers/models.py index 46000e83dd7a..802fd0157da5 100644 --- a/superset/semantic_layers/models.py +++ b/superset/semantic_layers/models.py @@ -57,6 +57,7 @@ from superset.common.utils.dataframe_utils import df_columns_to_num from superset.exceptions import ( InvalidPostProcessingError, + OAuth2RedirectError, QueryObjectValidationError, ) from superset.explorables.base import TimeGrainDict @@ -87,7 +88,7 @@ semantic_layer_context, ) from superset.utils import json -from superset.utils.core import GenericDataType, TIME_COMPARISON +from superset.utils.core import GenericDataType, get_user_id, TIME_COMPARISON if TYPE_CHECKING: from superset.superset_typing import ExplorableData, QueryObjectDict @@ -909,6 +910,7 @@ def get_extra_cache_keys(self, query_obj: QueryObjectDict) -> list[Hashable]: keys: list[Hashable] = [self.metadata_generation, "semantic-null-filters-v1"] if discriminator is not None: keys.append(("semantic-result-version", *discriminator)) + keys.append(("semantic_layer_user", get_user_id())) return keys @property diff --git a/superset/semantic_layers/oauth2.py b/superset/semantic_layers/oauth2.py new file mode 100644 index 000000000000..df325cb1178e --- /dev/null +++ b/superset/semantic_layers/oauth2.py @@ -0,0 +1,67 @@ +# 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. +"""Bridge provider authentication requests to Superset's OAuth2 error UI.""" + +from __future__ import annotations + +from contextlib import contextmanager, nullcontext +from typing import Iterator +from uuid import uuid4 + +from flask import current_app, has_request_context, url_for +from superset_core.semantic_layers.layer import SemanticLayerAuthenticationRequiredError + +from superset.exceptions import OAuth2RedirectError + + +class SemanticLayerOAuth2RedirectError(OAuth2RedirectError): + """An actionable OAuth error from a user-partitioned semantic query.""" + + +@contextmanager +def check_for_oauth2( + layer_uuid: str | None, + view_uuid: str | None = None, +) -> Iterator[None]: + """Translate authentication failures without starting a provider exchange.""" + try: + yield + except SemanticLayerAuthenticationRequiredError as ex: + if layer_uuid is None: + raise + tab_id = str(uuid4()) + in_request = has_request_context() + # Workers need only a browser-relative start URL. Resolve it without + # requiring SERVER_NAME or inventing a public hostname for the worker. + context = nullcontext() if in_request else current_app.test_request_context() + with context: + completion_url = url_for( + "SemanticLayerRestApi.oauth2_complete", + tab_id=tab_id, + _external=in_request, + ) + start_url = url_for( + "SemanticLayerRestApi.oauth2_start", + uuid=layer_uuid, + action_id=ex.action_id, + tab_id=tab_id, + _external=False, + **({"view_uuid": view_uuid} if view_uuid else {}), + ) + raise SemanticLayerOAuth2RedirectError( + start_url, tab_id, completion_url + ) from None diff --git a/superset/semantic_layers/registry.py b/superset/semantic_layers/registry.py index 81e3c5d59172..67d26c13f6b7 100644 --- a/superset/semantic_layers/registry.py +++ b/superset/semantic_layers/registry.py @@ -27,6 +27,7 @@ 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]]] = {} @@ -53,7 +54,11 @@ def unregister_semantic_layer(semantic_layer_id: str) -> None: @contextmanager -def semantic_layer_context(semantic_layer_id: str) -> Iterator[None]: +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() @@ -67,43 +72,65 @@ def semantic_layer_context(semantic_layer_id: str) -> Iterator[None]: if manifest is not None else nullcontext() ) - with context: + with check_for_oauth2(layer_uuid, view_uuid), context: yield class ContextualSemanticView: """Forward semantic view operations inside their extension context.""" - def __init__(self, semantic_layer_id: str, implementation: Any): + 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: - with semantic_layer_context(self._semantic_layer_id): + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): return getattr(self._implementation, name) def uid(self) -> str: - with semantic_layer_context(self._semantic_layer_id): + 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): + 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): + 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): + 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): + 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): + 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( @@ -111,7 +138,9 @@ def get_compatible_metrics( selected_metrics: Any, selected_dimensions: Any, ) -> Any: - with semantic_layer_context(self._semantic_layer_id): + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): return self._implementation.get_compatible_metrics( selected_metrics, selected_dimensions, @@ -122,7 +151,9 @@ def get_compatible_dimensions( selected_metrics: Any, selected_dimensions: Any, ) -> Any: - with semantic_layer_context(self._semantic_layer_id): + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): return self._implementation.get_compatible_dimensions( selected_metrics, selected_dimensions, @@ -132,37 +163,62 @@ def get_compatible_dimensions( class ContextualSemanticLayer: """Forward semantic layer operations inside their extension context.""" - def __init__(self, semantic_layer_id: str, implementation: Any): + 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: - with semantic_layer_context(self._semantic_layer_id): + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): return getattr(self._implementation, name) def get_semantic_views(self, runtime_configuration: dict[str, Any]) -> set[Any]: - with semantic_layer_context(self._semantic_layer_id): + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): views = self._implementation.get_semantic_views(runtime_configuration) - return {ContextualSemanticView(self._semantic_layer_id, view) for view in views} + return { + ContextualSemanticView( + self._semantic_layer_id, view, self._layer_uuid, self._view_uuid + ) + for view in views + } def get_semantic_view( self, name: str, additional_configuration: dict[str, Any], ) -> ContextualSemanticView: - with semantic_layer_context(self._semantic_layer_id): + with semantic_layer_context( + self._semantic_layer_id, self._layer_uuid, self._view_uuid + ): view = self._implementation.get_semantic_view( name, additional_configuration, ) - return ContextualSemanticView(self._semantic_layer_id, view) + return ContextualSemanticView( + self._semantic_layer_id, view, self._layer_uuid, self._view_uuid + ) def contextualize_semantic_layer( semantic_layer_id: str, implementation: Any, + layer_uuid: str | None = None, + view_uuid: str | None = None, ) -> Any: - """Wrap extension providers while leaving host providers unchanged.""" - if semantic_layer_id not in _extension_manifests: + """Wrap providers to preserve extension and OAuth resource context.""" + if semantic_layer_id not in _extension_manifests and layer_uuid is None: return implementation - return ContextualSemanticLayer(semantic_layer_id, implementation) + return ContextualSemanticLayer( + semantic_layer_id, implementation, layer_uuid, view_uuid + ) diff --git a/superset/tasks/utils.py b/superset/tasks/utils.py index 8cf97bf0d0d0..896cc8a07dc6 100644 --- a/superset/tasks/utils.py +++ b/superset/tasks/utils.py @@ -19,6 +19,7 @@ import logging import traceback +from dataclasses import asdict from datetime import datetime from http.client import HTTPResponse from typing import Any, cast, Final, TYPE_CHECKING @@ -29,6 +30,7 @@ from flask import current_app, g from superset_core.tasks.types import TaskProperties, TaskScope +from superset.semantic_layers.oauth2 import SemanticLayerOAuth2RedirectError from superset.tasks.exceptions import ExecutorNotFoundError, InvalidExecutorError from superset.tasks.types import ( ChosenExecutor, @@ -344,6 +346,11 @@ def error_update(exception: BaseException) -> TaskProperties: TaskProperties, { "error_message": str(exception), + **( + {"errors": [asdict(exception.error)]} + if isinstance(exception, SemanticLayerOAuth2RedirectError) + else {} + ), "private": { "framework": { "exception_type": type(exception).__name__, diff --git a/superset/utils/encrypt.py b/superset/utils/encrypt.py index b17a7a02d973..5ddcebe1b044 100644 --- a/superset/utils/encrypt.py +++ b/superset/utils/encrypt.py @@ -17,12 +17,13 @@ import base64 import logging from abc import ABC, abstractmethod +from copy import copy from dataclasses import dataclass from typing import Any, Optional from flask import current_app, Flask from flask_babel import lazy_gettext as _ -from sqlalchemy import Table, text, TypeDecorator +from sqlalchemy import String, Table, text, TypeDecorator from sqlalchemy.engine import Connection, Dialect, Row from sqlalchemy_utils import EncryptedType as SqlaEncryptedType from sqlalchemy_utils.types.encrypted.encrypted_type import ( @@ -631,20 +632,39 @@ def _re_encrypt_conditional_table( "SQLALCHEMY_ENCRYPTED_FIELD_ENGINE", DEFAULT_ENCRYPTION_ENGINE_NAME ) current_engine = resolve_encryption_engine(engine_name) - enc_type = EncryptedType( - table.columns[value_col].type, - key=self._secret_key, - engine=current_engine, - ) - - cols = ", ".join(pk_columns + [value_col]) + extra_columns = ["user_fk"] if table_name == "extension_storage" else [] + cols = ", ".join(pk_columns + [value_col] + extra_columns) rows = conn.execute( text( f"SELECT {cols} FROM {table_name} WHERE {flag_col} = true" # noqa: S608 ) ) for row in rows: - self._re_encrypt_row( + migrator = self + value_type = table.columns[value_col].type + if table_name == "extension_storage": + # Storage encrypts a text envelope with a per-user key. Keep + # that envelope intact and use the same derivation for both keys. + from superset.extensions.storage.persistent_dao import ( # pylint: disable=import-outside-toplevel + _derive_key, + ) + + value_type = String() + user_fk = row._mapping["user_fk"] + if user_fk is not None: + migrator = copy(self) + migrator._secret_key = _derive_key(self._secret_key, user_fk) + migrator._previous_secret_key = ( + _derive_key(self._previous_secret_key, user_fk) + if self._previous_secret_key + else None + ) + enc_type = EncryptedType( + value_type, + key=migrator._secret_key, + engine=current_engine, + ) + migrator._re_encrypt_row( conn, row, table_name, diff --git a/tests/unit_tests/semantic_layers/api_test.py b/tests/unit_tests/semantic_layers/api_test.py index e1f2ffc3d2ef..0f2cf070d171 100644 --- a/tests/unit_tests/semantic_layers/api_test.py +++ b/tests/unit_tests/semantic_layers/api_test.py @@ -28,9 +28,11 @@ from superset_core.semantic_layers.layer import ( SemanticLayerAction, SemanticLayerActionResult, + SemanticLayerAuthenticationRequiredError, ) from werkzeug.test import TestResponse +from superset import security_manager from superset.commands.semantic_layer.exceptions import ( SemanticDeleteDependentsError, SemanticLayerCreateFailedError, @@ -442,13 +444,13 @@ def test_execute_configuration_action( ) assert response.status_code == 200 - assert response.json["result"] == { - "redirect_url": "https://auth.example.test/authorize" - } + result = response.json["result"] + assert result["redirect_url"] == "https://auth.example.test/authorize" + uuid_lib.UUID(result["tab_id"]) mock_cls.execute_configuration_action.assert_called_once_with( "oauth2", parsed_config, - "https://superset.example.test/semantic-layers", + f"http://example.com/api/v1/semantic_layer/oauth2/complete/{result['tab_id']}", ) @@ -501,8 +503,11 @@ def test_execute_configuration_action_unmasks_existing_layer( {"client_secret": PASSWORD_MASK}, "quiver", ) + result = response.json["result"] mock_cls.execute_configuration_action.assert_called_once_with( - "oauth2", parsed_config, None + "oauth2", + parsed_config, + f"http://example.com/api/v1/semantic_layer/oauth2/complete/{result['tab_id']}", ) @@ -3799,3 +3804,142 @@ def test_post_semantic_view_provider_validation( ) else: layer.implementation.get_semantic_view.assert_not_called() + + +@SEMANTIC_LAYERS_APP +@pytest.mark.parametrize("view_granted", [True, False]) +def test_oauth2_start_checks_originating_view( + client: FlaskClient, + full_api_access: None, + mocker: MockerFixture, + view_granted: bool, +) -> None: + """A view grant permits personal OAuth without granting its sibling views.""" + from superset.semantic_layers.models import SemanticView + + layer = SemanticLayer( + uuid=uuid_lib.uuid4(), + type="personal", + configuration='{"host":"provider"}', + perm="layer-permission", + ) + view = SemanticView( + uuid=uuid_lib.uuid4(), semantic_layer=layer, perm="view-permission" + ) + mocker.patch("superset.semantic_layers.api.get_user_id", return_value=42) + mocker.patch.object( + security_manager, "can_access_all_datasources", return_value=False + ) + mocker.patch.object( + security_manager, + "can_access", + side_effect=lambda permission, value: view_granted and value == view.perm, + ) + mocker.patch( + "superset.semantic_layers.api.SemanticLayerDAO.find_by_uuid", return_value=layer + ) + query = mocker.patch("superset.semantic_layers.api.db.session.query") + query.return_value.filter.return_value.one_or_none.return_value = view + provider = MagicMock() + provider.execute_configuration_action.return_value = SemanticLayerActionResult( + "https://provider.example/authorize" + ) + mocker.patch.dict("superset.semantic_layers.api.registry", {"personal": provider}) + tab_id = uuid_lib.uuid4() + response = client.get( + f"/api/v1/semantic_layer/{layer.uuid}/oauth2/connect" + f"?view_uuid={view.uuid}&tab_id={tab_id}" + ) + if view_granted: + assert response.status_code == 302 + assert response.location == "https://provider.example/authorize" + assert provider.execute_configuration_action.call_args.args[2] == ( + f"http://example.com/api/v1/semantic_layer/oauth2/complete/{tab_id}" + ) + # The database lookup must bind the view to this layer. + filters = query.return_value.filter.call_args.args + assert filters[1].right.value == layer.uuid + else: + assert response.status_code == 403 + provider.execute_configuration_action.assert_not_called() + + +@SEMANTIC_LAYERS_APP +def test_oauth2_start_rejects_view_from_another_layer( + client: FlaskClient, full_api_access: None, mocker: MockerFixture +) -> None: + """A missing view/layer pair must not start an action using stored config.""" + mocker.patch("superset.semantic_layers.api.get_user_id", return_value=42) + mocker.patch( + "superset.semantic_layers.api.SemanticLayerDAO.find_by_uuid", + return_value=MagicMock(uuid=uuid_lib.uuid4()), + ) + query = mocker.patch("superset.semantic_layers.api.db.session.query") + query.return_value.filter.return_value.one_or_none.return_value = None + response = client.get( + f"/api/v1/semantic_layer/{uuid_lib.uuid4()}/oauth2/connect" + f"?view_uuid={uuid_lib.uuid4()}&tab_id={uuid_lib.uuid4()}" + ) + assert response.status_code == 404 + + +@SEMANTIC_LAYERS_APP +def test_oauth2_completion_contains_only_correlation_id( + app: Any, client: FlaskClient, full_api_access: None, mocker: MockerFixture +) -> None: + """The existing completion page notifies both supported transports.""" + from pathlib import Path + + from jinja2 import FileSystemLoader + + mocker.patch.object( + app, + "jinja_loader", + FileSystemLoader(Path(__file__).resolve().parents[3] / "superset/templates"), + ) + mocker.patch("superset.semantic_layers.api.get_user_id", return_value=42) + tab_id = uuid_lib.uuid4() + response = client.get(f"/api/v1/semantic_layer/oauth2/complete/{tab_id}") + assert response.status_code == 200 + assert str(tab_id) in response.text + assert "BroadcastChannel" in response.text + assert "oauth2_auth_complete" in response.text + assert "access_token" not in response.text + + +@SEMANTIC_LAYERS_APP +@pytest.mark.parametrize("endpoint", ["schema/runtime", "views"]) +def test_discovery_authentication_error( + client: FlaskClient, + full_api_access: None, + mocker: MockerFixture, + endpoint: str, +) -> None: + """Provider auth failures reach discovery clients as actionable OAuth errors.""" + layer = SemanticLayer( + uuid=uuid_lib.uuid4(), + type="personal", + configuration="{}", + ) + mocker.patch.object(SemanticLayer, "raise_for_access") + mocker.patch( + "superset.semantic_layers.api.SemanticLayerDAO.find_by_uuid", return_value=layer + ) + provider = MagicMock() + implementation = provider.from_configuration.return_value + implementation.configuration = {} + auth_error = SemanticLayerAuthenticationRequiredError("connect", "Connect first") + provider.get_runtime_schema.side_effect = auth_error + implementation.get_semantic_views.side_effect = auth_error + mocker.patch.dict("superset.semantic_layers.api.registry", {"personal": provider}) + + response = client.post( + f"/api/v1/semantic_layer/{layer.uuid}/{endpoint}", json={"runtime_data": {}} + ) + + assert response.status_code == 403 + error = response.json["errors"][0] + assert error["error_type"] == "OAUTH2_REDIRECT" + assert f"/{layer.uuid}/oauth2/connect?" in error["extra"]["url"] + assert uuid_lib.UUID(error["extra"]["tab_id"]) + provider.execute_configuration_action.assert_not_called() diff --git a/tests/unit_tests/semantic_layers/models_test.py b/tests/unit_tests/semantic_layers/models_test.py index 7e0c6d04eaf8..65c6159a8f41 100644 --- a/tests/unit_tests/semantic_layers/models_test.py +++ b/tests/unit_tests/semantic_layers/models_test.py @@ -3352,3 +3352,21 @@ def test_values_rejection_logs_view_and_dimension( assert "INVALID_FILTER" in caplog.text assert "private-search" not in caplog.text mock_implementation.get_values.assert_called_once() + + + +def test_personal_results_have_distinct_query_cache_keys() -> None: + """Two users cannot reuse each other's provider data or shared query task.""" + from superset.common.query_object import QueryObject + + view = SemanticView(id=1) + view.__dict__["implementation"] = MagicMock() + view.implementation.uid.return_value = "semantic-view-1" + query = QueryObject(datasource=view, columns=["country"]) + with patch("superset.semantic_layers.models.get_user_id", return_value=42): + first = query.cache_key(extra_cache_keys=view.get_extra_cache_keys({})) + repeated = query.cache_key(extra_cache_keys=view.get_extra_cache_keys({})) + with patch("superset.semantic_layers.models.get_user_id", return_value=43): + second = query.cache_key(extra_cache_keys=view.get_extra_cache_keys({})) + assert first == repeated + assert first != second diff --git a/tests/unit_tests/semantic_layers/registry_test.py b/tests/unit_tests/semantic_layers/registry_test.py index 07116eabb416..4293d9dbdbb4 100644 --- a/tests/unit_tests/semantic_layers/registry_test.py +++ b/tests/unit_tests/semantic_layers/registry_test.py @@ -20,9 +20,13 @@ from __future__ import annotations from unittest.mock import MagicMock +from urllib.parse import parse_qs, urlparse +import pytest from flask import Flask +from superset_core.semantic_layers.layer import SemanticLayerAuthenticationRequiredError +from superset.exceptions import OAuth2RedirectError from superset.extensions.context import get_context from superset.semantic_layers.registry import ( contextualize_semantic_layer, @@ -67,3 +71,53 @@ def test_contextual_layer_wraps_returned_view(app: Flask) -> None: assert contextual_view.uid() == "acme.semantic" finally: unregister_semantic_layer(semantic_layer_id) + + +@pytest.mark.parametrize( + "method,args", + [ + ("get_dimensions", ()), + ("get_metrics", ()), + ("get_values", (None,)), + ("get_table", (None,)), + ("get_row_count", (None,)), + ("get_compatible_metrics", (set(), set())), + ("get_compatible_dimensions", (set(), set())), + ], +) +@pytest.mark.parametrize("in_request", [True, False]) +def test_view_authentication_uses_shared_oauth_error( + app: Flask, method: str, args: tuple[object, ...], in_request: bool +) -> None: + """Provider failures retain the originating view and shared UI contract.""" + app.add_url_rule( + "/api/v1/semantic_layer//oauth2/", + endpoint="SemanticLayerRestApi.oauth2_start", + ) + app.add_url_rule( + "/api/v1/semantic_layer/oauth2/complete/", + endpoint="SemanticLayerRestApi.oauth2_complete", + ) + layer = MagicMock() + view = MagicMock() + getattr(view, method).side_effect = SemanticLayerAuthenticationRequiredError( + "connect", "Provider details must stay on the server" + ) + layer.get_semantic_view.return_value = view + contextual_layer = contextualize_semantic_layer( + "provider", layer, "layer-uuid", "view-uuid" + ) + context = app.test_request_context() if in_request else app.app_context() + with context: + contextual_view = contextual_layer.get_semantic_view("orders", {}) + with pytest.raises(OAuth2RedirectError) as raised: + getattr(contextual_view, method)(*args) + error = raised.value.error + assert error.error_type.value == "OAUTH2_REDIRECT" + assert "Provider details" not in error.message + url = urlparse(error.extra["url"]) + assert url.path == "/api/v1/semantic_layer/layer-uuid/oauth2/connect" + query = parse_qs(url.query) + assert query["view_uuid"] == ["view-uuid"] + assert query["tab_id"] == [error.extra["tab_id"]] + assert error.extra["redirect_uri"].endswith(error.extra["tab_id"]) diff --git a/tests/unit_tests/tasks/test_utils.py b/tests/unit_tests/tasks/test_utils.py index 197de6649928..c9f4576b6f53 100644 --- a/tests/unit_tests/tasks/test_utils.py +++ b/tests/unit_tests/tasks/test_utils.py @@ -996,3 +996,18 @@ def test_floored_status_cursor_drops_subsecond_precision() -> None: cursor = floored_status_cursor() assert cursor.microsecond == 0 + + +def test_oauth_failure_preserves_actionable_error_for_async_clients() -> None: + """Task consumers can authorize and retry without receiving provider secrets.""" + from superset.semantic_layers.oauth2 import SemanticLayerOAuth2RedirectError + + exception = SemanticLayerOAuth2RedirectError( + "/oauth2/start", "flow-id", "/oauth2/complete" + ) + properties = error_update(exception) + error = properties["errors"][0] + assert error["error_type"] == "OAUTH2_REDIRECT" + assert error["extra"]["url"] == "/oauth2/start" + assert error["extra"]["tab_id"] == "flow-id" + assert "errors" not in error_update(ValueError("ordinary failure")) diff --git a/tests/unit_tests/utils/encrypt_test.py b/tests/unit_tests/utils/encrypt_test.py index 8bd84202ecd1..d49fe60217e1 100644 --- a/tests/unit_tests/utils/encrypt_test.py +++ b/tests/unit_tests/utils/encrypt_test.py @@ -444,3 +444,64 @@ def test_engine_migration_unreadable_value_counts_as_failure() -> None: assert stats == ReEncryptStats(failed=1) assert conn.execute.call_count == 0 + + +@pytest.mark.parametrize("user_fk", [None, 42, 43]) +@pytest.mark.parametrize( + "rotate_key,source_engine,target_engine", + [ + (True, "aes", "aes"), + (True, "aes-gcm", "aes-gcm"), + (False, "aes", "aes-gcm"), + (True, "aes", "aes-gcm"), + (False, "aes-gcm", "aes"), + ], +) +def test_extension_storage_rotation_preserves_user_keys( + user_fk: int | None, + rotate_key: bool, + source_engine: str, + target_engine: str, +) -> None: + """Rotation and engine changes preserve the storage envelope and user scope.""" + from flask import Flask + + from superset.extensions.storage.persistent_dao import ( + _decrypt_value, + _encrypt_value, + ) + from superset.extensions.storage.persistent_model import ExtensionStorage + + app = Flask(__name__) + app.config["SECRET_KEY"] = "old-key" if rotate_key else SECRET_KEY + app.config["SQLALCHEMY_ENCRYPTED_FIELD_ENGINE"] = source_engine + value = b'{"access_token":"private-token"}' + with app.app_context(): + encrypted = _encrypt_value(value, user_fk, DIALECT) + migrator = _engine_migrator(resolve_encryption_engine(target_engine)) + migrator._previous_secret_key = app.config["SECRET_KEY"] + migrator._db = MagicMock() + migrator._db.metadata.tables = {"extension_storage": ExtensionStorage.__table__} + conn = MagicMock() + row = _Row({"id": 1, "value": encrypted, "user_fk": user_fk}) + conn.execute.return_value = [row] + app.config["SECRET_KEY"] = SECRET_KEY + stats = ReEncryptStats() + migrator._re_encrypt_conditional_table( + conn, "extension_storage", "value", "is_encrypted", stats + ) + assert stats == ReEncryptStats(re_encrypted=1) + migrated_value = conn.execute.call_args.args[1]["value"] + app.config["SQLALCHEMY_ENCRYPTED_FIELD_ENGINE"] = target_engine + assert _decrypt_value(migrated_value, user_fk, DIALECT) == value + # Repeating the migration must leave the row unchanged. + conn.reset_mock() + conn.execute.return_value = [ + _Row({"id": 1, "value": migrated_value, "user_fk": user_fk}) + ] + stats = ReEncryptStats() + migrator._re_encrypt_conditional_table( + conn, "extension_storage", "value", "is_encrypted", stats + ) + assert stats == ReEncryptStats(skipped=1) + assert conn.execute.call_count == 1 From c9cc6f2121fe611d50b4842e992cd9a03f11f348 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Thu, 29 Oct 2026 14:03:35 -0400 Subject: [PATCH 03/13] Cleanup --- QUIVER_SEMANTIC_LAYER_OAUTH_PLAN.md | 582 ------------------ UPDATING.md | 10 + .../extensions/contribution-types.md | 10 +- superset/commands/semantic_layer/update.py | 4 - superset/core/api/core_api_injection.py | 2 +- superset/extensions/context.py | 15 +- superset/extensions/storage/persistent_dao.py | 4 +- superset/semantic_layers/registry.py | 52 +- superset/utils/encrypt.py | 6 +- .../commands/semantic_layer/update_test.py | 24 +- .../unit_tests/extensions/api_context_test.py | 81 ++- .../extensions/storage/test_persistent_dao.py | 4 +- .../semantic_layers/registry_test.py | 80 +++ tests/unit_tests/tasks/test_async_queries.py | 70 +++ 14 files changed, 324 insertions(+), 620 deletions(-) delete mode 100644 QUIVER_SEMANTIC_LAYER_OAUTH_PLAN.md diff --git a/QUIVER_SEMANTIC_LAYER_OAUTH_PLAN.md b/QUIVER_SEMANTIC_LAYER_OAUTH_PLAN.md deleted file mode 100644 index 4ff61ab1d3fa..000000000000 --- a/QUIVER_SEMANTIC_LAYER_OAUTH_PLAN.md +++ /dev/null @@ -1,582 +0,0 @@ -# Quiver Semantic Layer OAuth Implementation Plan - -## Objective - -Enable semantic layer extensions such as Quiver to store and use OAuth2 tokens -without exposing those tokens to browser code and without requiring a semantic -layer row to exist before authorization begins. - -The design uses `extension_storage` as the permanent token store. Tokens are -written directly during the OAuth callback for every user, including the admin -creating the semantic layer. It does not use a temporary KV escrow. - -## Target Architecture - -```text -Semantic Layer configuration - | - +-- Quiver settings produce stable credential_key - | - +-- "Connect to Quiver" - | +-- OAuth2 + PKCE - | +-- callback - | +-- extension_storage - | extension = netflix.quiver - | user = current user - | access = backend - | key = oauth2:v1: - | encrypted = true - | - +-- Create Semantic Layer - +-- uses the same credential_key -``` - -The admin creating the semantic layer follows the same OAuth path as every -other user. The credential is associated with the extension, current user, and -a stable Quiver credential identity rather than a semantic layer UUID. - -## 1. Add a Backend-Only Secrets Namespace - -Expose a dedicated Python-only API: - -```python -ctx.storage.secrets.get(key) -ctx.storage.secrets.set(key, value) -ctx.storage.secrets.remove(key) -``` - -This is preferable to adding `backend_only=True` to the ordinary persistent -accessor because it provides safer defaults: - -- It exists only in Python. -- It always encrypts values. -- It is always user-scoped. -- It has no `.shared` accessor. -- It does not support listing. -- It cannot accidentally become available in the TypeScript SDK. -- Ordinary `ctx.storage.persistent` behavior remains unchanged. - -### Storage Model - -Modify `superset/extensions/storage/persistent_model.py` and the corresponding -core model declaration to add: - -```python -access: str # "frontend" or "backend" -``` - -Existing rows receive `access="frontend"`. - -The logical identity becomes: - -```text -(extension_id, user_fk, resource_type, resource_uuid, access, key) -``` - -Including `access` in the lookup identity allows a browser-visible value and a -backend secret to use the same key without allowing frontend `PUT`, `GET`, or -`DELETE` requests to reveal whether the secret exists. - -Use an application-level enum backed by a portable string column rather than a -database enum: - -```python -class StorageAccess(str, Enum): - FRONTEND = "frontend" - BACKEND = "backend" -``` - -Update: - -- `superset/extensions/storage/persistent_model.py` -- `superset-core/src/superset_core/extensions/storage/models.py` -- `superset/extensions/storage/persistent_dao.py` -- Extension storage lookup indexes and unique constraint -- A new migration under `superset/migrations/versions/` - -The migration should: - -1. Add `access`, non-null, with a server default of `frontend`. -2. Replace the scoped lookup index with one containing `access`. -3. Replace the scoped unique constraint with one containing `access`. -4. Preserve every existing row as frontend-visible. -5. On downgrade, delete backend-only entries before removing the column; - converting them to frontend-visible entries would expose secrets. - -### Secrets API - -Add a new core interface, likely at: - -```text -superset-core/src/superset_core/extensions/storage/secrets.py -``` - -Add `secrets` to: - -- `superset-core/src/superset_core/extensions/context.py` -- `superset/extensions/context.py` -- The storage injection in `superset/core/api/core_api_injection.py` - -The host implementation should call the DAO with: - -```python -access=StorageAccess.BACKEND -encrypt=True -user_fk=current_user_id -codec="json" -``` - -Do not expose backend access in: - -- `superset-frontend/packages/superset-core/src/storage/index.ts` -- REST request parameters -- REST request bodies - -### DAO Invariants - -DAO methods should always receive an access class, defaulting to `FRONTEND` -for backward compatibility. - -The secrets accessor must enforce encryption internally. It must not accept an -option that permits `encrypt=False`. - -Quota accounting should continue to include both frontend and backend entries. - -## 2. Make the Browser API Blind to Backend Entries - -Update `superset/extensions/storage/api.py` so every persistent REST operation -is hard-coded to `access=FRONTEND`. - -Required behavior: - -- `GET`: a backend-only entry behaves as if it does not exist. -- `list`: backend entries are excluded before count, pagination, and payload - size calculation. -- `PUT`: writes only to the frontend namespace. -- `DELETE`: removes only from the frontend namespace. -- No response reveals the backend key, codec, size, encryption flag, or - existence. -- Supplying `access`, `backend_only`, or similar fields must not change the - namespace. - -Because access is part of the logical key, frontend `PUT` does not need to -reject a colliding backend key. It creates or updates an independent frontend -entry. - -### Storage Tests - -Extend: - -- `tests/unit_tests/extensions/storage/test_api.py` -- `tests/unit_tests/extensions/storage/test_persistent.py` -- `tests/unit_tests/extensions/storage/test_persistent_dao.py` -- `tests/unit_tests/extensions/storage/test_context.py` - -Cover: - -- Secret round-trip for the current user. -- Separation between two users. -- Separation between two extensions. -- Forced encryption. -- Absence of a shared secret accessor. -- Browser `GET` cannot read a secret. -- Browser `list` omits secrets and reports the filtered count. -- Browser `DELETE` does not remove a secret. -- Browser `PUT` cannot overwrite a secret. -- Frontend and backend entries can coexist at the same key. -- Unauthenticated use fails closed. -- Existing and migrated rows remain frontend-visible. - -## 3. Restore Extension Context During Semantic Layer Execution - -Storage depends on ambient extension context, but semantic layer providers are -registered during extension loading and invoked later, outside that loading -context. - -The registration process must retain the owning extension manifest. - -Update: - -- `superset/semantic_layers/registry.py` -- Semantic layer decorator injection in - `superset/core/api/core_api_injection.py` -- `superset/semantic_layers/models.py` -- Class-level registry consumers in `superset/semantic_layers/api.py` -- Create and update commands under `superset/commands/semantic_layer/` - -A registry entry should retain: - -```python -@dataclass(frozen=True) -class RegisteredSemanticLayer: - implementation: type[SemanticLayer] - extension_manifest: Manifest | None -``` - -Host semantic layers have `extension_manifest=None`. - -### Contextual Adapters - -Avoid relying on scattered `with extension_context(...)` calls. Introduce -explicit adapters for: - -- Semantic layer class operations -- Semantic layer instances -- Semantic view instances returned by a layer - -Each adapter enters the saved extension context before invoking the provider. - -Explicit adapters implementing the semantic layer protocols are preferable to -a generic `__getattr__` proxy. When a new protocol method is added, typing and -tests will then show that its context forwarding is missing. - -The wrapper must cover at least: - -- `from_configuration` -- Configuration schema generation -- Runtime schema generation -- Semantic view discovery -- `get_semantic_view` -- Metrics and dimensions -- Compatible metrics and dimensions -- Values lookup -- Query and table generation -- Row count -- Feature or property access that can call extension code - -Tests belong in: - -- `tests/unit_tests/semantic_layers/decorators_test.py` -- `tests/unit_tests/semantic_layers/models_test.py` -- A focused new contextual execution test module - -The test extension should call `get_context()` and `ctx.storage.secrets` from -both a layer and a returned view. - -## 4. Carry the Initiating User Into Background Execution - -Extension identity alone is insufficient because personal tokens require the -initiating user. - -The execution context should support an explicit principal ID: - -```python -extension_context(manifest, user_id=user_id) -``` - -`get_current_user_id()` should: - -1. Prefer the explicit execution principal. -2. Fall back to the authenticated Flask user during request execution. -3. Fail if neither exists. - -For async chart and query paths, propagate the initiating user already present -in query or task metadata into the semantic layer execution context. Never -fall back to: - -- The semantic layer creator -- The admin who configured it -- The Celery worker or service account - -This is required by Superset's async security model: a worker is a continuation -of the initiating principal. - -Embedded guests and anonymous or Public users have no personal Quiver token -and should fail closed unless Quiver later defines a separate service credential -design. - -## 5. Add Generic Semantic Layer Connection Actions - -The semantic layer modal needs a way to initiate OAuth before a `SemanticLayer` -row exists. This should be a generic contribution rather than a Quiver-specific -button in Superset. - -Add a typed semantic layer action contract to `superset-core`, for example: - -```python -@dataclass(frozen=True) -class SemanticLayerAction: - id: str - label: str - status: Literal["required", "connected", "expired", "error"] - required: bool - message: str | None = None -``` - -Provider hooks could be: - -```python -get_configuration_actions(configuration) -execute_configuration_action(action_id, configuration, return_url) -``` - -The default implementation returns no actions, preserving existing providers. - -### API Shape - -Extend the existing configuration schema response: - -```json -{ - "result": { "...": "JSON schema" }, - "actions": [ - { - "id": "oauth2", - "label": "Connect to Quiver", - "status": "required", - "required": true - } - ] -} -``` - -Add protected endpoints for executing actions: - -```text -POST /api/v1/semantic_layer/type//actions/ -POST /api/v1/semantic_layer//actions/ -GET /api/v1/semantic_layer//actions -``` - -The type endpoint supports pre-creation setup and receives partial -configuration. It should require semantic layer write permission. - -The UUID endpoint supports existing users connecting their own account. It -should require read access to the semantic layer or data source, rather than -edit permission, because connecting a personal token does not modify the -semantic layer. - -The action returns an OAuth authorization URL or a same-origin redirect -response, never a token. - -Class hooks and action execution must run through the extension context -adapter. - -## 6. Update the Semantic Layer Frontend - -Update -`superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx` to: - -1. Read `actions` from the schema response. -2. Render a standard `@superset-ui/core/components` button and status. -3. Send the partial configuration when starting the action. -4. Open the OAuth flow in a popup or redirect. -5. Poll or refetch schema and action status after completion. -6. Disable creation while a required action is not connected. -7. Never receive or store access or refresh tokens. - -Update -`superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx` -for: - -- Providers without actions. -- A required disconnected action. -- Successful connection and schema refresh. -- Popup cancellation. -- OAuth failure. -- A required action blocking creation. -- Edit-mode reconnection. - -For existing users, add a reusable Connect action surfaced when the semantic -layer reports that authentication is required. A typed -`SemanticLayerAuthenticationRequiredError` should carry only: - -- Semantic layer UUID -- Action ID -- Human-readable message - -The frontend can then call the UUID action endpoint. It must not receive -credential keys or token metadata beyond connected or expired status. - -## 7. Define the Quiver Credential Identity - -Tokens must be addressable before semantic layer creation and reusable by -multiple Quiver semantic layers. - -Build a canonical non-secret identity from fields such as: - -```json -{ - "quiver_url": "...", - "issuer": "...", - "client_id": "...", - "audience": "...", - "scopes": ["..."] -} -``` - -Normalize URLs and sort scopes, then hash the canonical JSON: - -```text -oauth2:v1: -``` - -Do not include: - -- Client secret -- Access token -- Refresh token -- Semantic layer UUID -- Semantic layer name - -The resulting record is: - -```text -extension_id = netflix.quiver -user_fk = authenticated user -access = backend -key = oauth2:v1: -value = { - access_token, - refresh_token, - token_type, - expires_at, - granted_scopes -} -is_encrypted = true -``` - -Deleting one semantic layer must not delete this entry because several layers -may share it. Provide an explicit Disconnect Quiver action instead. - -Changing identity fields produces a new credential key. Old credentials can -be removed through Disconnect or eventual extension cleanup. - -## 8. Implement OAuth in the Quiver Extension - -Superset provides storage, execution context, and action UI. The Quiver -extension remains responsible for OAuth protocol behavior. - -### Start - -The start handler should: - -- Require an authenticated user. -- Derive the credential key from submitted non-secret configuration. -- Generate state, nonce, and a PKCE verifier and challenge. -- Bind signed state to the user ID, credential key, return location, nonce, - and expiry. -- Store the PKCE verifier in a short-lived backend cache inaccessible through - extension browser storage. -- Redirect to the configured issuer. - -The PKCE verifier is temporary protocol state, not token escrow. - -### Callback - -The callback should: - -- Validate the state signature and age. -- Require the same authenticated user recorded in state. -- Consume the nonce and verifier exactly once. -- Exchange the authorization code server-side. -- Write the resulting token directly through `ctx.storage.secrets`. -- Return a minimal success page or redirect. -- Never include tokens in URLs, HTML, `postMessage`, logs, or frontend - responses. - -### Refresh - -Before calling Quiver: - -1. Load the user's token. -2. Refresh if it is near expiry. -3. Acquire a distributed lock scoped to extension, user, and credential key. -4. Reread after acquiring the lock. -5. Refresh only if it is still necessary. -6. Persist the new access token and any rotated refresh token. - -On `invalid_grant`, remove the stored secret and raise the typed authentication -required error. - -## 9. Update Documentation - -Update `docs/developer_docs/extensions/storage.md` to make the distinction -explicit: - -- `encrypt=True` means encryption at rest. -- Ordinary persistent values remain readable by the extension frontend. -- Tokens and backend credentials belong in `ctx.storage.secrets`. -- Secrets are encrypted, user-scoped, backend-only, non-listable, and - unavailable in TypeScript. -- `.shared` is intentionally unavailable for secrets. -- Secret values remain subject to extension storage quotas. -- User deletion cascades to personal secrets. - -Also add semantic layer extension documentation covering: - -- Runtime extension context -- Configuration actions -- Pre-creation OAuth -- Personal token behavior in background queries -- Disconnect and reconnect behavior - -The existing `api_token` example under ordinary persistent storage should be -changed because it suggests a browser-readable location for bearer -credentials. - -## 10. Suggested Pull Request Sequence - -### PR 1: Backend-Only Extension Secrets - -- Migration and model changes -- DAO access namespace -- `ctx.storage.secrets` -- REST isolation -- Storage tests -- Storage documentation - -This PR is independently useful to all backend extensions. - -### PR 2: Semantic Layer Runtime Context - -- Registered provider metadata -- Contextual layer and view adapters -- Explicit user principal propagation -- Synchronous and asynchronous execution tests - -This makes extension storage usable reliably from semantic layer providers. - -### PR 3: Semantic Layer Connection Actions - -- Core action types and hooks -- Protected action endpoints -- Modal integration -- Authentication-required error contract -- Frontend and API tests -- Developer documentation - -### PR 4: Quiver Extension - -- Credential key derivation -- OAuth start and callback -- Token storage and refresh locking -- Disconnect and reconnect -- Quiver-specific tests - -## Explicit Non-Goals - -- Do not refactor `database_user_oauth2_tokens`. -- Do not migrate existing database OAuth flows. -- Do not use `localStorage`, `sessionStorage`, cookies, or frontend Redux state - for tokens. -- Do not use KV escrow for the initial admin. -- Do not key tokens to semantic layer UUIDs. -- Do not expose a general browser flag for creating backend-only entries. -- Do not introduce shared or service-account credentials as part of the - personal OAuth implementation. - -## Completion Criteria - -The work is complete when: - -- An admin can authorize Quiver before creating the semantic layer. -- No semantic layer row is required to store that token. -- Another user sees a Connect action and stores a separate personal token. -- Synchronous and asynchronous queries use the initiating user's token. -- Multiple semantic layers can reuse the same user credential. -- Browser storage APIs cannot read, enumerate, overwrite, or delete the token. -- Refresh token rotation is persisted safely. -- Revoked credentials produce a reconnect action. -- Existing Tier 3 storage behavior remains backward compatible. -- Targeted tests and staged-file `pre-commit run` pass. diff --git a/UPDATING.md b/UPDATING.md index 3e564449ed8d..614f4bab66ac 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -734,6 +734,16 @@ floats), and non-finite Decimals in those columns become null. Other chart result serialization and chart number formatting are unchanged. Re-run queries whose cached JSON results were produced before upgrading all workers. + +### Downgrading backend-only extension secrets + +Migration `4c8f2a1d9b73` adds backend-only extension storage for personal secrets, +including OAuth tokens. Downgrading past this migration permanently deletes all +storage rows marked `access=backend` before removing the access classification. +This prevents secrets from becoming readable through the older browser storage +API. Back up the metadata database before downgrading. Users must reconnect their +personal accounts if they upgrade again after the downgrade. + ### Version history retention setting Use `VERSION_HISTORY_RETENTION_DAYS` for both the application setting and diff --git a/docs/developer_docs/extensions/contribution-types.md b/docs/developer_docs/extensions/contribution-types.md index 98d3b49948a8..5b765ebbe424 100644 --- a/docs/developer_docs/extensions/contribution-types.md +++ b/docs/developer_docs/extensions/contribution-types.md @@ -282,7 +282,10 @@ Store access and refresh tokens using the Python-only Never put personal credentials in the shared layer configuration, action results, query results, logs, or exceptions. The host executes provider methods under the owning extension and requesting user's context, including background chart work. -Semantic View query results and shared async query tasks are partitioned by user. +Semantic View query results and shared async query tasks are partitioned by user, +including providers without connection actions. This reduces cache sharing for +providers with identical results for all users, but declaring no actions does not +establish that a provider returns user-independent data. When any provider operation requires authentication, raise the core exception: @@ -310,6 +313,11 @@ operation. Returning browser focus before authorization completes does not consume the completion notification. Closing an unfinished connection window allows the user to start another attempt. +The completion URL uses the request's externally visible host and scheme. Operators +must configure their reverse proxy, forwarded headers, and allowed hosts so +Superset sees its trusted public HTTPS origin. See the +[reverse proxy configuration](../../admin_docs/configuration/configuring-superset.mdx#configuration-behind-a-load-balancer). + Action POST responses contain `result.redirect_url` and a host-generated `result.tab_id`. Both the configuration modal and the generic OAuth error UI use the same completion listener. No access or refresh token is returned in diff --git a/superset/commands/semantic_layer/update.py b/superset/commands/semantic_layer/update.py index b5bef3383b6a..129da403824b 100644 --- a/superset/commands/semantic_layer/update.py +++ b/superset/commands/semantic_layer/update.py @@ -136,10 +136,6 @@ def unmask_stored_configuration( raise SemanticLayerInvalidError(str(ex)) from None -# Kept while callers transition to the descriptive shared name above. -_unmask_configuration = unmask_stored_configuration - - class UpdateSemanticViewCommand(BaseCommand): def __init__(self, model_id: int, data: dict[str, Any]): self._model_id = model_id diff --git a/superset/core/api/core_api_injection.py b/superset/core/api/core_api_injection.py index c42c68290d1e..c55666e30ccd 100644 --- a/superset/core/api/core_api_injection.py +++ b/superset/core/api/core_api_injection.py @@ -63,7 +63,7 @@ def _contextualize_extension_api_method( @wraps(method) def contextualized(*args: Any, **kwargs: Any) -> Any: - with extension_context(manifest, user_id=get_user_id()): + with extension_context(manifest, user_id=get_user_id): return method(*args, **kwargs) return contextualized diff --git a/superset/extensions/context.py b/superset/extensions/context.py index 7ba9c1d56351..2a0f57e6146a 100644 --- a/superset/extensions/context.py +++ b/superset/extensions/context.py @@ -30,7 +30,7 @@ from contextlib import contextmanager from contextvars import ContextVar -from typing import Any, Iterator +from typing import Any, Callable, Iterator from superset_core.extensions.types import BaseExtension, Manifest @@ -60,7 +60,11 @@ def secrets(self) -> Any: class ConcreteExtensionContext: """Concrete implementation of ExtensionContext for the host.""" - def __init__(self, manifest: Manifest, user_id: int | None = None): + def __init__( + self, + manifest: Manifest, + user_id: int | None | Callable[[], int | None] = None, + ) -> None: self._manifest = manifest self._user_id = user_id self._storage = ExtensionStorage() @@ -76,8 +80,8 @@ def storage(self) -> ExtensionStorage: @property def user_id(self) -> int | None: - """Explicit principal for extension execution outside a request.""" - return self._user_id + """Resolve request principals after authentication, or use a fixed worker ID.""" + return self._user_id() if callable(self._user_id) else self._user_id # Context variable for ambient extension context pattern. @@ -133,7 +137,7 @@ def use_context(ctx: ConcreteExtensionContext) -> Iterator[None]: @contextmanager def extension_context( manifest: Manifest, - user_id: int | None = None, + user_id: int | None | Callable[[], int | None] = None, ) -> Iterator[ConcreteExtensionContext]: """ Context manager for setting extension context during loading. @@ -142,6 +146,7 @@ def extension_context( the current context. Supports nested contexts via ContextVar tokens. :param manifest: The extension manifest + :param user_id: A fixed principal or a resolver called when the ID is accessed :yields: The created ExtensionContext """ ctx = ConcreteExtensionContext(manifest, user_id=user_id) diff --git a/superset/extensions/storage/persistent_dao.py b/superset/extensions/storage/persistent_dao.py index b473a3cb6549..63ff3ddfa8ae 100644 --- a/superset/extensions/storage/persistent_dao.py +++ b/superset/extensions/storage/persistent_dao.py @@ -174,7 +174,7 @@ def _validate_key_length(key: str) -> None: raise ExtensionStorageKeyTooLong(MAX_KEY_LENGTH) -def _derive_key(secret: str, user_fk: int) -> str: +def derive_user_key(secret: str, user_fk: int) -> str: """Derive a per-user encryption key from the master secret and user ID. Uses HMAC-SHA256 so the derived key is cryptographically bound to both. @@ -195,7 +195,7 @@ def _enc_type(user_fk: int | None) -> EncryptedBytesType: Shared rows (user_fk=None) use SECRET_KEY directly. """ secret = current_app.config["SECRET_KEY"] - key = _derive_key(secret, user_fk) if user_fk is not None else secret + key = derive_user_key(secret, user_fk) if user_fk is not None else secret engine_name = current_app.config.get( "SQLALCHEMY_ENCRYPTED_FIELD_ENGINE", DEFAULT_ENCRYPTION_ENGINE_NAME ) diff --git a/superset/semantic_layers/registry.py b/superset/semantic_layers/registry.py index 67d26c13f6b7..8e8d5bd7b9bf 100644 --- a/superset/semantic_layers/registry.py +++ b/superset/semantic_layers/registry.py @@ -18,6 +18,7 @@ from __future__ import annotations from contextlib import contextmanager, nullcontext +from functools import wraps from typing import Any, Iterator from superset_core.extensions.types import Manifest @@ -76,6 +77,27 @@ def semantic_layer_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.""" @@ -92,10 +114,27 @@ def __init__( 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 getattr(self._implementation, name) + 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( @@ -176,10 +215,13 @@ def __init__( self._view_uuid = view_uuid def __getattr__(self, name: str) -> Any: - with semantic_layer_context( - self._semantic_layer_id, self._layer_uuid, self._view_uuid - ): - return getattr(self._implementation, name) + return _contextual_attribute( + self._semantic_layer_id, + self._implementation, + name, + self._layer_uuid, + self._view_uuid, + ) def get_semantic_views(self, runtime_configuration: dict[str, Any]) -> set[Any]: with semantic_layer_context( diff --git a/superset/utils/encrypt.py b/superset/utils/encrypt.py index 5ddcebe1b044..ea5c0e115ada 100644 --- a/superset/utils/encrypt.py +++ b/superset/utils/encrypt.py @@ -646,16 +646,16 @@ def _re_encrypt_conditional_table( # Storage encrypts a text envelope with a per-user key. Keep # that envelope intact and use the same derivation for both keys. from superset.extensions.storage.persistent_dao import ( # pylint: disable=import-outside-toplevel - _derive_key, + derive_user_key, ) value_type = String() user_fk = row._mapping["user_fk"] if user_fk is not None: migrator = copy(self) - migrator._secret_key = _derive_key(self._secret_key, user_fk) + migrator._secret_key = derive_user_key(self._secret_key, user_fk) migrator._previous_secret_key = ( - _derive_key(self._previous_secret_key, user_fk) + derive_user_key(self._previous_secret_key, user_fk) if self._previous_secret_key else None ) diff --git a/tests/unit_tests/commands/semantic_layer/update_test.py b/tests/unit_tests/commands/semantic_layer/update_test.py index 2f2e3d995478..9b1d07e347c6 100644 --- a/tests/unit_tests/commands/semantic_layer/update_test.py +++ b/tests/unit_tests/commands/semantic_layer/update_test.py @@ -29,7 +29,7 @@ SemanticViewNotFoundError, ) from superset.commands.semantic_layer.update import ( - _unmask_configuration, + unmask_stored_configuration, UpdateSemanticLayerCommand, UpdateSemanticViewCommand, ) @@ -484,14 +484,14 @@ def test_update_uniqueness_same_config_same_name_fails( # ============================================================================= -# _unmask_configuration tests +# unmask_stored_configuration tests # ============================================================================= def test_unmask_configuration_restores_masked_secret() -> None: """A masked write-only field in the payload is replaced by the stored value rather than overwriting the real credential with the mask.""" - result: dict[str, Any] = _unmask_configuration( + result: dict[str, Any] = unmask_stored_configuration( '{"account": "test", "password": "hunter2"}', {"account": "test", "password": PASSWORD_MASK}, "test_provider", @@ -503,7 +503,7 @@ def test_unmask_configuration_restores_masked_secret() -> None: def test_unmask_configuration_keeps_fresh_secret() -> None: """A genuinely new secret value (not the mask sentinel) passes through unchanged.""" - result: dict[str, Any] = _unmask_configuration( + result: dict[str, Any] = unmask_stored_configuration( '{"account": "test", "password": "old-secret"}', {"account": "test", "password": "new-secret"}, "test_provider", @@ -518,7 +518,7 @@ def test_unmask_configuration_restores_fail_closed_masked_fields() -> None: round-trip, not just write-only ones -- otherwise a name-only save persists the literal mask into non-secret fields like ``account`` once the schema becomes available again.""" - result: dict[str, Any] = _unmask_configuration( + result: dict[str, Any] = unmask_stored_configuration( '{"account": "test", "database": "prod", "password": "hunter2"}', { "account": PASSWORD_MASK, @@ -538,7 +538,7 @@ def test_unmask_configuration_restores_fail_closed_masked_fields() -> None: def test_unmask_configuration_missing_existing_key() -> None: """A masked field with no corresponding stored value passes through unchanged rather than raising.""" - result: dict[str, Any] = _unmask_configuration( + result: dict[str, Any] = unmask_stored_configuration( '{"account": "test"}', {"account": "test", "password": PASSWORD_MASK}, "test_provider", @@ -558,7 +558,7 @@ def test_unmask_configuration_rejects_secret_reveal_with_changed_field() -> None config, silently leaking it on the next legitimate use of this layer. """ with pytest.raises(SemanticLayerInvalidError): - _unmask_configuration( + unmask_stored_configuration( '{"account": "prod-account", "password": "hunter2"}', {"account": "attacker-account", "password": PASSWORD_MASK}, "test_provider", @@ -575,7 +575,7 @@ def test_unmask_configuration_rejects_secret_reveal_with_new_none_valued_key() - it. """ with pytest.raises(SemanticLayerInvalidError): - _unmask_configuration( + unmask_stored_configuration( '{"account": "prod-account", "password": "hunter2"}', { "account": "prod-account", @@ -594,7 +594,7 @@ def test_unmask_configuration_rejects_secret_reveal_with_removed_key() -> None: Comparing only the submitted keys would let that through. """ with pytest.raises(SemanticLayerInvalidError): - _unmask_configuration( + unmask_stored_configuration( '{"account": "prod-account", "region": "eu-west-1", "password": "hunter2"}', {"account": "prod-account", "password": PASSWORD_MASK}, "test_provider", @@ -606,7 +606,7 @@ def test_unmask_configuration_allows_removed_key_with_fresh_secret() -> None: Dropping a stored key is still allowed when a genuinely fresh secret is supplied alongside it -- only the masked round-trip is restricted. """ - result = _unmask_configuration( + result = unmask_stored_configuration( '{"account": "prod-account", "region": "eu-west-1", "password": "hunter2"}', {"account": "prod-account", "password": "fresh-secret"}, "test_provider", @@ -620,7 +620,7 @@ def test_unmask_configuration_allows_fresh_secret_with_changed_field() -> None: A deliberate configuration change is still possible when a genuinely fresh (non-masked) secret is supplied alongside it. """ - result = _unmask_configuration( + result = unmask_stored_configuration( '{"account": "prod-account", "password": "hunter2"}', {"account": "new-account", "password": "fresh-secret"}, "test_provider", @@ -634,7 +634,7 @@ def test_unmask_configuration_allows_unrelated_field_addition_with_no_mask() -> Adding/changing fields with no masked value present at all is unaffected -- there's no secret being reused, so nothing to protect. """ - result = _unmask_configuration( + result = unmask_stored_configuration( '{"account": "prod-account"}', {"account": "new-account", "extra_option": "value"}, "test_provider", diff --git a/tests/unit_tests/extensions/api_context_test.py b/tests/unit_tests/extensions/api_context_test.py index 058146e41083..5a34db3a13a3 100644 --- a/tests/unit_tests/extensions/api_context_test.py +++ b/tests/unit_tests/extensions/api_context_test.py @@ -15,11 +15,20 @@ # specific language governing permissions and limitations # under the License. +from secrets import token_hex +from types import SimpleNamespace from unittest.mock import MagicMock, patch -from flask_appbuilder.api import expose +import pytest +from flask import Flask, g +from flask_appbuilder.api import BaseApi, expose, protect +from flask_appbuilder.security.sqla.manager import SecurityManager +from flask_jwt_extended import create_access_token, JWTManager +from pytest_mock import MockerFixture from superset.core.api.core_api_injection import _contextualize_extension_api +from superset.extensions.context import get_context, get_current_extension_context +from tests.unit_tests.extensions.storage.conftest import create_manifest def test_extension_api_methods_run_in_extension_context() -> None: @@ -41,12 +50,78 @@ def helper(self) -> str: patch( "superset.core.api.core_api_injection.get_user_id", return_value=42, - ), + ) as user_id_resolver, ): _contextualize_extension_api(TestApi, manifest) assert TestApi().callback() == "ok" + user_id_resolver.assert_not_called() - extension_context.assert_called_once_with(manifest, user_id=42) + extension_context.assert_called_once_with(manifest, user_id=user_id_resolver) assert TestApi.helper is original_helper assert TestApi.callback._urls == [("/callback", ("GET",))] + + +@pytest.mark.parametrize("authentication", ["jwt", "api_key", "unauthenticated"]) +def test_extension_api_resolves_principal_after_protect( + mocker: MockerFixture, authentication: str +) -> None: + """FAB authentication must finish before context and secrets resolve the user.""" + app = Flask(__name__) + app.config["JWT_SECRET_KEY"] = token_hex(32) + app.config["FAB_API_KEY_ENABLED"] = authentication == "api_key" + jwt = JWTManager(app) + user = SimpleNamespace(id=42, is_active=True) + manager = MagicMock() + manager.is_item_public.return_value = False + manager.has_access.return_value = True + manager.load_user.return_value = user + jwt.user_lookup_loader( + lambda headers, data: SecurityManager.load_user_jwt(manager, headers, data) + ) + + def authenticate_api_key(key: str) -> SimpleNamespace: + assert key == "personal-api-key" + assert get_context().user_id is None + g.user = user + return user + + manager.extract_api_key_from_request.return_value = "personal-api-key" + manager.validate_api_key.side_effect = authenticate_api_key + mocker.patch.object(app, "appbuilder", MagicMock(sm=manager), create=True) + manifest = create_manifest("acme", "semantic") + get_secret = mocker.patch( + "superset.extensions.storage.secrets.ExtensionStorageDAO.get_decoded_value", + return_value="personal-token", + ) + + class TestApi(BaseApi): + class_permission_name = "ExtensionCallback" + base_permissions = ["can_callback"] + allow_browser_login = False + + @expose("/callback", methods=("GET",)) + @protect() + def callback(self) -> dict[str, int | None]: + assert get_context().storage.secrets.get("token") == "personal-token" + return {"user_id": get_context().user_id} + + _contextualize_extension_api(TestApi, manifest) + app.add_url_rule("/callback", view_func=TestApi().callback) + headers = {} + if authentication == "jwt": + with app.app_context(): + headers["Authorization"] = f"Bearer {create_access_token(identity='42')}" + response = app.test_client().get("/callback", headers=headers) + if authentication == "unauthenticated": + assert response.status_code == 401 + get_secret.assert_not_called() + else: + from superset_core.extensions.storage.models import StorageAccess + + assert response.status_code == 200 + assert response.json == {"user_id": 42} + get_secret.assert_called_once_with( + "acme.semantic", "token", user_fk=42, access=StorageAccess.BACKEND + ) + assert get_current_extension_context() is None diff --git a/tests/unit_tests/extensions/storage/test_persistent_dao.py b/tests/unit_tests/extensions/storage/test_persistent_dao.py index 98c720e36158..d8f87674af05 100644 --- a/tests/unit_tests/extensions/storage/test_persistent_dao.py +++ b/tests/unit_tests/extensions/storage/test_persistent_dao.py @@ -34,9 +34,9 @@ from superset.extensions.storage.filters import ExtensionStorageFilter from superset.extensions.storage.persistent_dao import ( _decrypt_value, - _derive_key, _enc_type, _encrypt_value, + derive_user_key, ExtensionStorageDAO, ExtensionStorageKeyTooLong, ExtensionStorageListPayloadTooLarge, @@ -717,7 +717,7 @@ def test_enc_type_user_scoped_uses_derived_key(app: Flask) -> None: with app.app_context(): secret = app.config["SECRET_KEY"] enc = _enc_type(user_fk=42) - expected_key = _derive_key(secret, 42) + expected_key = derive_user_key(secret, 42) # The key lambda on the EncryptedType resolves to the derived key. assert enc.key == expected_key diff --git a/tests/unit_tests/semantic_layers/registry_test.py b/tests/unit_tests/semantic_layers/registry_test.py index 4293d9dbdbb4..948c9b435b1d 100644 --- a/tests/unit_tests/semantic_layers/registry_test.py +++ b/tests/unit_tests/semantic_layers/registry_test.py @@ -30,6 +30,8 @@ from superset.extensions.context import get_context from superset.semantic_layers.registry import ( contextualize_semantic_layer, + ContextualSemanticLayer, + ContextualSemanticView, register_semantic_layer, semantic_layer_context, unregister_semantic_layer, @@ -121,3 +123,81 @@ def test_view_authentication_uses_shared_oauth_error( assert query["view_uuid"] == ["view-uuid"] assert query["tab_id"] == [error.extra["tab_id"]] assert error.extra["redirect_uri"].endswith(error.extra["tab_id"]) + + +@pytest.mark.parametrize("wrapper", [ContextualSemanticLayer, ContextualSemanticView]) +def test_fallback_method_executes_in_provider_context( + app: Flask, wrapper: type[ContextualSemanticLayer] | type[ContextualSemanticView] +) -> None: + """Fallback callables must retain context after attribute lookup returns.""" + manifest = create_manifest("acme", "semantic") + provider_id = "extensions.acme.semantic.fallback" + register_semantic_layer(provider_id, MagicMock(), manifest) + implementation = MagicMock() + implementation.custom_operation.side_effect = lambda: get_context().extension.id + try: + with app.app_context(): + proxy = wrapper(provider_id, implementation) + operation = proxy.custom_operation + assert operation() == manifest.id + finally: + unregister_semantic_layer(provider_id) + + +@pytest.mark.parametrize("wrapper", [ContextualSemanticLayer, ContextualSemanticView]) +def test_fallback_method_translates_oauth_failure( + app: Flask, wrapper: type[ContextualSemanticLayer] | type[ContextualSemanticView] +) -> None: + """Provider methods outside the explicit forwarders use the OAuth bridge.""" + app.add_url_rule( + "/layers//oauth2/", + endpoint="SemanticLayerRestApi.oauth2_start", + ) + app.add_url_rule( + "/oauth2/complete/", endpoint="SemanticLayerRestApi.oauth2_complete" + ) + implementation = MagicMock() + implementation.custom_operation.side_effect = ( + SemanticLayerAuthenticationRequiredError("connect") + ) + proxy = wrapper("personal", implementation, "layer-id", "view-id") + with app.test_request_context(): + with pytest.raises(OAuth2RedirectError) as raised: + proxy.custom_operation() + assert "view_uuid=view-id" in raised.value.error.extra["url"] + + +def test_wrapped_view_sets_preserve_provider_equality(app: Flask) -> None: + """Repeated discovery keeps provider equality and hashing across proxy sets.""" + + class ProviderView: + def __init__(self, name: str) -> None: + self.name = name + + def __hash__(self) -> int: + assert get_context().extension.id == "acme.semantic" + return hash(self.name) + + def __eq__(self, other: object) -> bool: + assert get_context().extension.id == "acme.semantic" + return isinstance(other, ProviderView) and self.name == other.name + + manifest = create_manifest("acme", "semantic") + provider_id = "extensions.acme.semantic.equality" + layer = MagicMock() + layer.get_semantic_views.side_effect = lambda _: { + ProviderView("orders"), + ProviderView("customers"), + ProviderView("orders"), + } + register_semantic_layer(provider_id, MagicMock(), manifest) + try: + with app.app_context(): + proxy = contextualize_semantic_layer(provider_id, layer) + first = proxy.get_semantic_views({}) + second = proxy.get_semantic_views({}) + assert len(first) == 2 + assert first == second + assert len(first | second) == 2 + finally: + unregister_semantic_layer(provider_id) diff --git a/tests/unit_tests/tasks/test_async_queries.py b/tests/unit_tests/tasks/test_async_queries.py index 068b6803059d..a8c4479305ad 100644 --- a/tests/unit_tests/tasks/test_async_queries.py +++ b/tests/unit_tests/tasks/test_async_queries.py @@ -23,6 +23,7 @@ import pandas as pd import pytest +from flask import Flask, g, has_request_context from pytest_mock import MockerFixture from superset_core.tasks.types import TaskStatus @@ -730,3 +731,72 @@ def test_completeness_failure_does_not_publish_task_success( task_context.update_task.assert_called_once_with( payload={"semantic_result_error": reason}, immediate=True ) + + +@pytest.mark.parametrize("user_id", [42, 43]) +def test_semantic_query_worker_installs_requesting_principal( + app: Flask, mocker: MockerFixture, user_id: int +) -> None: + """Celery execution scopes provider secrets and result cache keys to its user.""" + from superset.extensions.context import get_context as get_extension_context + from superset.extensions.storage.secrets import SecretsState + from superset.semantic_layers.models import SemanticView + from superset.semantic_layers.registry import ( + contextualize_semantic_layer, + register_semantic_layer, + unregister_semantic_layer, + ) + from superset.tasks.async_queries import execute_chart_query + from tests.unit_tests.extensions.storage.conftest import create_manifest + + manifest = create_manifest("acme", "semantic") + provider_id = "extensions.acme.semantic.worker" + user = mocker.MagicMock(id=user_id) + mocker.patch( + "superset.tasks.async_queries.security_manager.get_user_by_id", + return_value=user, + ) + get_secret = mocker.patch( + "superset.extensions.storage.secrets.ExtensionStorageDAO.get_decoded_value", + return_value="personal-token", + ) + provider = mocker.MagicMock() + + def provider_operation() -> str: + assert get_extension_context().user_id == user_id + assert SecretsState.get("token") == "personal-token" + return "orders" + + provider.get_semantic_view.return_value.uid.side_effect = provider_operation + register_semantic_layer(provider_id, mocker.MagicMock(), manifest) + proxy = contextualize_semantic_layer(provider_id, provider) + query_context = _fake_query_context(num_queries=1) + query_context.datasource = SemanticView(id=1) + query_context.datasource.__dict__["implementation"] = proxy.get_semantic_view( + "orders", {} + ) + + def execute_query(query: object) -> mock.MagicMock: + assert not has_request_context() + assert query_context.datasource.implementation.uid() == "orders" + assert query_context.datasource.get_extra_cache_keys({}) == [ + ("semantic_layer_user", user_id) + ] + return mocker.MagicMock(payload={}) + + query_context.get_df_payload_result.side_effect = execute_query + mocker.patch( + "superset.tasks.async_queries.load_serialized_query", return_value=query_context + ) + mocker.patch( + "superset.tasks.async_queries.get_context", return_value=mocker.MagicMock() + ) + try: + with app.app_context(): + g.user = None + execute_chart_query.func(_serialized_query(), user_id=user_id) + assert g.user is None + query_context.get_df_payload_result.assert_called_once() + assert get_secret.call_args.kwargs["user_fk"] == user_id + finally: + unregister_semantic_layer(provider_id) From 2585c25ae560457be52b29894703719a9e73f59a Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Fri, 30 Oct 2026 10:50:51 -0400 Subject: [PATCH 04/13] Fix OAuth2 --- .../src/pages/Chart/Chart.test.tsx | 66 +++++++++++++++++++ superset-frontend/src/pages/Chart/index.tsx | 46 +++++++++++++ superset/commands/explore/get.py | 4 +- superset/explore/api.py | 2 + tests/unit_tests/explore/api_test.py | 47 ++++++++++++- 5 files changed, 163 insertions(+), 2 deletions(-) diff --git a/superset-frontend/src/pages/Chart/Chart.test.tsx b/superset-frontend/src/pages/Chart/Chart.test.tsx index 3ed0de3dc631..fa65415a4486 100644 --- a/superset-frontend/src/pages/Chart/Chart.test.tsx +++ b/superset-frontend/src/pages/Chart/Chart.test.tsx @@ -788,4 +788,70 @@ describe('ChartPage', () => { addDangerToastSpy.mockRestore(); }); + + test('authorizes before loading semantic datasource metadata and retries the chart', async () => { + const exploreApiRoute = 'glob:*/api/v1/explore/*'; + const hydrateSpy = jest.spyOn(hydrateExploreActions, 'hydrateExplore'); + const toastSpy = jest.spyOn(messageToastActions, 'addDangerToast'); + (getParsedExploreURLParams as jest.Mock).mockReturnValue( + new Map([['slice_id', 106]]), + ); + const formData = getExploreFormData({ viz_type: VizType.Table }); + let requests = 0; + fetchMock.get(exploreApiRoute, () => { + requests += 1; + if (requests === 1) { + return { + status: 403, + body: { + errors: [ + { + error_type: 'OAUTH2_REDIRECT', + level: 'warning', + message: "You don't have permission to access the data.", + extra: { url: '/oauth2/start', tab_id: 'metadata-flow' }, + }, + ], + }, + }; + } + return { result: { dataset: { id: 7 }, form_data: formData } }; + }); + render(, { useRouter: true, useRedux: true, useDnd: true }); + const link = await screen.findByRole('link', { + name: 'provide authorization', + }); + expect(link).toHaveAttribute('href', '/oauth2/start'); + expect(hydrateSpy).not.toHaveBeenCalled(); + expect(toastSpy).not.toHaveBeenCalled(); + expect( + screen.queryByTestId('mock-explore-chart-panel'), + ).not.toBeInTheDocument(); + + act(() => { + window.dispatchEvent( + new StorageEvent('storage', { + key: 'oauth2_auth_complete', + newValue: JSON.stringify({ tabId: 'other-flow' }), + }), + ); + }); + expect(requests).toBe(1); + act(() => { + window.dispatchEvent( + new StorageEvent('storage', { + key: 'oauth2_auth_complete', + newValue: JSON.stringify({ tabId: 'metadata-flow' }), + }), + ); + }); + await screen.findByTestId('mock-explore-chart-panel'); + expect(requests).toBe(2); + expect(hydrateSpy).toHaveBeenCalledTimes(1); + expect( + screen.queryByRole('link', { name: 'provide authorization' }), + ).not.toBeInTheDocument(); + hydrateSpy.mockRestore(); + toastSpy.mockRestore(); + }); }); diff --git a/superset-frontend/src/pages/Chart/index.tsx b/superset-frontend/src/pages/Chart/index.tsx index f573b10ea396..152dddb6c093 100644 --- a/superset-frontend/src/pages/Chart/index.tsx +++ b/superset-frontend/src/pages/Chart/index.tsx @@ -28,8 +28,12 @@ import { makeApi, LabelsColorMapSource, getClientErrorObject, + ErrorTypeEnum, + SupersetError, + SupersetApiError, } from '@superset-ui/core'; import { Loading } from '@superset-ui/core/components'; +import { OAuth2RedirectMessage } from 'src/components/ErrorMessage'; import { addDangerToast } from 'src/components/MessageToasts/actions'; import { getUrlParam } from 'src/utils/urlUtils'; import { URL_PARAMS } from 'src/constants'; @@ -144,6 +148,8 @@ const getDashboardContextFormData = (search: string) => { export default function ExplorePage() { const [isLoaded, setIsLoaded] = useState(false); + const [authenticationError, setAuthenticationError] = + useState>(); const fetchGeneration = useRef(0); const abortControllerRef = useRef(null); const dispatch = useDispatch(); @@ -155,6 +161,7 @@ export default function ExplorePage() { loc: { search: string; pathname: string }, saveAction?: SaveActionType | null, ) => { + setAuthenticationError(undefined); // Abort any in-flight request before starting a new one abortControllerRef.current?.abort(); const controller = new AbortController(); @@ -231,6 +238,33 @@ export default function ExplorePage() { if (!err) { return Promise.resolve(); } + const oauthError: SupersetError | undefined = + clientError?.errors?.find( + error => error.error_type === ErrorTypeEnum.OAUTH2_REDIRECT, + ) ?? + (err instanceof SupersetApiError && + String(err.errorType) === ErrorTypeEnum.OAUTH2_REDIRECT + ? { + error_type: ErrorTypeEnum.OAUTH2_REDIRECT, + level: 'warning', + message: err.message, + extra: err.extra, + } + : undefined); + if ( + oauthError && + typeof oauthError.extra?.url === 'string' && + typeof oauthError.extra?.tab_id === 'string' + ) { + setAuthenticationError({ + ...oauthError, + extra: { + url: oauthError.extra.url, + tab_id: oauthError.extra.tab_id, + }, + }); + return Promise.resolve(); + } const errorMesage = clientError?.message || clientError?.error || @@ -379,5 +413,17 @@ export default function ExplorePage() { if (!isLoaded) { return ; } + if (authenticationError) { + return ( + { + setIsLoaded(false); + loadExploreData(history.location); + }} + /> + ); + } return ; } diff --git a/superset/commands/explore/get.py b/superset/commands/explore/get.py index b7f22876cc96..bd7a9c8f6e2f 100644 --- a/superset/commands/explore/get.py +++ b/superset/commands/explore/get.py @@ -34,7 +34,7 @@ from superset.daos.dataset import DatasetDAO from superset.daos.datasource import DatasourceDAO from superset.daos.exceptions import DatasourceNotFound -from superset.exceptions import SupersetException +from superset.exceptions import OAuth2RedirectError, SupersetException from superset.explore.exceptions import WrongEndpointError from superset.explore.permalink.exceptions import ExplorePermalinkGetFailedError from superset.extensions import security_manager @@ -208,6 +208,8 @@ def run(self) -> Optional[dict[str, Any]]: # noqa: C901 try: if datasource: datasource_data = datasource.data + except OAuth2RedirectError: + raise except SupersetException as ex: message = ex.message except SQLAlchemyError: diff --git a/superset/explore/api.py b/superset/explore/api.py index dea284fe43ea..883a74d9eb2f 100644 --- a/superset/explore/api.py +++ b/superset/explore/api.py @@ -36,6 +36,7 @@ SemanticLayersDisabledError, ) from superset.views.base_api import BaseSupersetApi, statsd_metrics +from superset.views.error_handling import handle_api_exception logger = logging.getLogger(__name__) @@ -51,6 +52,7 @@ class ExploreRestApi(BaseSupersetApi): @expose("/", methods=("GET",)) @protect() @safe + @handle_api_exception @statsd_metrics @event_logger.log_this_with_context( action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.get", diff --git a/tests/unit_tests/explore/api_test.py b/tests/unit_tests/explore/api_test.py index 9852def0086e..82838fa9c358 100644 --- a/tests/unit_tests/explore/api_test.py +++ b/tests/unit_tests/explore/api_test.py @@ -16,7 +16,7 @@ # under the License. from typing import Any -from unittest.mock import patch +from unittest.mock import MagicMock, patch, PropertyMock from superset.commands.temporary_cache.exceptions import ( TemporaryCacheAccessDeniedError, @@ -70,3 +70,48 @@ def test_temporary_cache_access_denied_omits_inert_access_flag( response = client.get("/api/v1/explore/?form_data_key=abc") assert "extra" not in response.json + + +def test_explore_preserves_oauth_error_during_metadata_load( + client: Any, full_api_access: None +) -> None: + """Initial chart loading must retain the authorization link and correlation ID.""" + from superset.exceptions import OAuth2RedirectError + + datasource = MagicMock() + error = OAuth2RedirectError("/oauth2/start", "flow-id", "/oauth2/complete") + with ( + patch( + "superset.commands.explore.get.get_form_data", + return_value=( + {"datasource": "7__semantic_view", "viz_type": "table"}, + None, + ), + ), + patch( + "superset.commands.explore.get.get_datasource_info", + return_value=(7, "semantic_view"), + ), + patch( + "superset.commands.explore.get.DatasourceDAO.get_datasource", + return_value=datasource, + ), + patch("superset.commands.explore.get._authorize_datasource") as authorize, + patch.object( + type(datasource), + "data", + new_callable=PropertyMock, + side_effect=error, + create=True, + ), + ): + response = client.get( + "/api/v1/explore/?datasource_id=7&datasource_type=semantic_view" + ) + + authorize.assert_called_once_with(datasource, None) + assert response.status_code == 403 + assert response.json["errors"][0]["error_type"] == "OAUTH2_REDIRECT" + assert response.json["errors"][0]["extra"]["url"] == "/oauth2/start" + assert response.json["errors"][0]["extra"]["tab_id"] == "flow-id" + assert "result" not in response.json From 76ee7e96d48feac50728f71c227fc358d7b0f00b Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Fri, 30 Oct 2026 13:25:58 -0400 Subject: [PATCH 05/13] fix(semantic-layer): integrate OAuth with updated provider bindings --- .../extensions/contribution-types.md | 1 - .../superset_core/semantic_layers/layer.py | 1 + .../SemanticLayerModal.test.tsx | 2 +- .../semanticLayers/SemanticLayerModal.tsx | 7 +- .../semanticViews/AddSemanticViewModal.tsx | 31 ++++----- .../src/middleware/asyncEvent.test.ts | 8 ++- .../src/middleware/asyncEvent.ts | 3 +- .../src/middleware/asyncQueryError.ts | 64 +++++++++++++------ superset/extensions/storage/persistent_dao.py | 28 +++----- ...8f2a1d9b73_add_extension_storage_access.py | 4 +- superset/semantic_layers/api.py | 13 +++- superset/semantic_layers/execution.py | 9 ++- superset/semantic_layers/models.py | 24 +++++-- superset/semantic_layers/registry.py | 14 ++++ .../extensions/storage/test_persistent_dao.py | 20 +++--- tests/unit_tests/semantic_layers/api_test.py | 4 +- .../semantic_layers/execution_context_test.py | 37 +++++++---- .../unit_tests/semantic_layers/models_test.py | 60 +++++++++++------ tests/unit_tests/tasks/test_async_queries.py | 25 ++++++-- 19 files changed, 234 insertions(+), 121 deletions(-) diff --git a/docs/developer_docs/extensions/contribution-types.md b/docs/developer_docs/extensions/contribution-types.md index 5b765ebbe424..e868d9393cad 100644 --- a/docs/developer_docs/extensions/contribution-types.md +++ b/docs/developer_docs/extensions/contribution-types.md @@ -322,4 +322,3 @@ Action POST responses contain `result.redirect_url` and a host-generated `result.tab_id`. Both the configuration modal and the generic OAuth error UI use the same completion listener. No access or refresh token is returned in these responses. - diff --git a/superset-core/src/superset_core/semantic_layers/layer.py b/superset-core/src/superset_core/semantic_layers/layer.py index bc247a1b7053..76eb0bd48532 100644 --- a/superset-core/src/superset_core/semantic_layers/layer.py +++ b/superset-core/src/superset_core/semantic_layers/layer.py @@ -74,6 +74,7 @@ class SemanticCacheIdentityMaterial: values: Mapping[str, object] + @dataclass(frozen=True) class SemanticLayerAction: """A user action required to configure or authenticate a provider.""" diff --git a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx index 50018c65c41a..bfffc04a8b3b 100644 --- a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx +++ b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx @@ -785,7 +785,7 @@ test('starts a required semantic layer configuration action', async () => { await waitFor(() => { expect(mockedPost).toHaveBeenNthCalledWith(2, { - endpoint: '/api/v1/semantic_layer/type/snowflake/actions/oauth2', + endpoint: '/api/v1/semantic_layer/type/example/actions/oauth2', jsonPayload: { configuration: expect.any(Object), semantic_layer_uuid: props.semanticLayerUuid, diff --git a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx index 45b160376b42..ef92071a4492 100644 --- a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx +++ b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx @@ -326,7 +326,6 @@ export default function SemanticLayerModal({ } }; - useOAuth2Completion(oauthTabId, () => { setOauthTabId(undefined); setRunningActionId(null); @@ -575,7 +574,11 @@ export default function SemanticLayerModal({ saveDisabled={ isTypeStep ? !selectedType - : saving || clearing || !name.trim() || hasErrors || requiredActionPending + : saving || + clearing || + !name.trim() || + hasErrors || + requiredActionPending } saveText={isTypeStep ? undefined : isEditMode ? t('Save') : t('Create')} saveLoading={saving} diff --git a/superset-frontend/src/features/semanticViews/AddSemanticViewModal.tsx b/superset-frontend/src/features/semanticViews/AddSemanticViewModal.tsx index e0a807b5af45..a727d67d12ff 100644 --- a/superset-frontend/src/features/semanticViews/AddSemanticViewModal.tsx +++ b/superset-frontend/src/features/semanticViews/AddSemanticViewModal.tsx @@ -535,22 +535,22 @@ export default function AddSemanticViewModal({ refreshGen !== schemaRefreshGenRef.current ) return; - if ( - await captureAuthenticationError( - error, - () => { - lastDepSnapshotRef.current = ''; - handleRuntimeFormChange({ - data, - errors: errorsRef.current, - }); - }, - () => - gen === fetchGenRef.current && - refreshGen === schemaRefreshGenRef.current, - ) + if ( + await captureAuthenticationError( + error, + () => { + lastDepSnapshotRef.current = ''; + handleRuntimeFormChange({ + data, + errors: errorsRef.current, + }); + }, + () => + gen === fetchGenRef.current && + refreshGen === schemaRefreshGenRef.current, ) - return; + ) + return; logging.error('Runtime schema refresh failed', error); // Allow exactly one retry per dependency state: clearing the // committed snapshot lets the next change event re-attempt @@ -881,6 +881,7 @@ export default function AddSemanticViewModal({ /> } + /> )} {authentication && ( ( if (signal?.aborted || generation !== pollingGeneration) { throw new DOMException('Aborted', 'AbortError'); } + if (queryError.errors?.length) throw { errors: queryError.errors }; throw Object.assign(queryError, { error: queryError.message }); }); diff --git a/superset-frontend/src/middleware/asyncQueryError.ts b/superset-frontend/src/middleware/asyncQueryError.ts index 2b73d6044e85..26e6e0de15ad 100644 --- a/superset-frontend/src/middleware/asyncQueryError.ts +++ b/superset-frontend/src/middleware/asyncQueryError.ts @@ -16,16 +16,24 @@ * specific language governing permissions and limitations * under the License. */ -import { SupersetClient } from '@superset-ui/core'; +import { + ErrorTypeEnum, + SupersetClient, + SupersetError, +} from '@superset-ui/core'; import { t } from '@apache-superset/core/translation'; const ERROR_DETAIL_TIMEOUT_MS = 5000; type CompletenessReason = 'incomplete' | 'unverified'; +type TaskFailureDetail = { + reason: CompletenessReason | null; + errors: SupersetError>[]; +}; -const fetchCompletenessReason = async ( +const fetchFailureDetail = async ( taskId: string, signal: AbortSignal, -): Promise => { +): Promise => { try { // Use the protected detail route; status notifications carry no error data. const { json } = await SupersetClient.get({ @@ -39,19 +47,34 @@ const fetchCompletenessReason = async ( !('status' in task) || task.status !== 'failure' || !('task_type' in task) || - task.task_type !== 'superset.query_object_v1' || - !('payload' in task) - ) - return null; - const { payload } = task; - if ( - !payload || - typeof payload !== 'object' || - !('semantic_result_error' in payload) + task.task_type !== 'superset.query_object_v1' ) return null; - const reason = payload.semantic_result_error; - return reason === 'incomplete' || reason === 'unverified' ? reason : null; + const payload = 'payload' in task ? task.payload : null; + const reason = + payload && + typeof payload === 'object' && + 'semantic_result_error' in payload + ? payload.semantic_result_error + : null; + const properties = 'properties' in task ? task.properties : null; + const errors: SupersetError>[] = + properties && + typeof properties === 'object' && + 'errors' in properties && + Array.isArray(properties.errors) + ? properties.errors.filter( + error => + error?.error_type === ErrorTypeEnum.OAUTH2_REDIRECT && + typeof error.extra?.url === 'string' && + typeof error.extra?.tab_id === 'string', + ) + : []; + return { + reason: + reason === 'incomplete' || reason === 'unverified' ? reason : null, + errors, + }; } catch { // Access revocation, unavailable details and malformed responses all fall back. return null; @@ -62,7 +85,7 @@ export const getAsyncQueryError = async ( taskIds: string[], signal?: AbortSignal, failureMessage?: string, -): Promise => { +): Promise>[] }> => { const controller = new AbortController(); let abort!: () => void; const cancelled = new Promise(resolve => { @@ -75,14 +98,17 @@ export const getAsyncQueryError = async ( if (signal?.aborted) abort(); const timeout = window.setTimeout(abort, ERROR_DETAIL_TIMEOUT_MS); try { - const reasons = await Promise.race([ + const details = await Promise.race([ Promise.all( - taskIds.map(taskId => - fetchCompletenessReason(taskId, controller.signal), - ), + taskIds.map(taskId => fetchFailureDetail(taskId, controller.signal)), ), cancelled, ]); + const errors = details?.flatMap(detail => detail?.errors ?? []) ?? []; + if (errors.length) { + return Object.assign(new Error(t('Authorization needed')), { errors }); + } + const reasons = details?.map(detail => detail?.reason ?? null); // Keep this fixed guidance aligned with SemanticResultCompletenessError // in superset/exceptions.py; never substitute text from task details. // A mixed failure must not imply that this guidance fixes every failed query. diff --git a/superset/extensions/storage/persistent_dao.py b/superset/extensions/storage/persistent_dao.py index 63ff3ddfa8ae..ff0450db8f9d 100644 --- a/superset/extensions/storage/persistent_dao.py +++ b/superset/extensions/storage/persistent_dao.py @@ -29,7 +29,7 @@ import backoff from flask import current_app from flask_babel import gettext as _ -from sqlalchemy import and_, desc, func, String +from sqlalchemy import and_, desc, func, LargeBinary from sqlalchemy.engine import Dialect from superset_core.extensions.storage.models import StorageAccess @@ -78,6 +78,8 @@ def process_result_value(self, value: Any, dialect: Any) -> bytes | None: value = value.decode() self._update_key() plaintext = self.engine.decrypt(value) + if plaintext.startswith(ENCRYPTED_VALUE_PREFIX): + plaintext = plaintext.removeprefix(ENCRYPTED_VALUE_PREFIX) try: return base64.b64decode(plaintext, validate=True) except binascii.Error: @@ -207,9 +209,8 @@ def _enc_type(user_fk: int | None) -> EncryptedBytesType: def _encrypt_value(value: bytes, user_fk: int | None, dialect: Dialect) -> bytes: - """Encrypt arbitrary bytes using a versioned, text-safe representation.""" - plaintext = ENCRYPTED_VALUE_PREFIX + base64.b64encode(value).decode("ascii") - encrypted = _enc_type(user_fk).process_bind_param(plaintext, dialect) + """Encrypt arbitrary bytes using the shared storage encoding.""" + encrypted = _enc_type(user_fk).process_bind_param(value, dialect) if not isinstance(encrypted, bytes): raise TypeError("Encrypted extension storage values must be bytes") return encrypted @@ -218,22 +219,9 @@ def _encrypt_value(value: bytes, user_fk: int | None, dialect: Dialect) -> bytes def _decrypt_value(value: bytes, user_fk: int | None, dialect: Dialect) -> bytes: """Decrypt bytes written by ``_encrypt_value`` or the legacy implementation.""" plaintext = _enc_type(user_fk).process_result_value(value, dialect) - if not isinstance(plaintext, str): - raise TypeError("Decrypted extension storage values must be strings") - if plaintext.startswith(ENCRYPTED_VALUE_PREFIX): - return base64.b64decode( - plaintext.removeprefix(ENCRYPTED_VALUE_PREFIX), - validate=True, - ) - - # The initial implementation passed bytes directly to EncryptedType. The - # resulting plaintext is the Python bytes representation (for example, - # ``b'{\"token\": \"...\"}'``). Recover those rows so deployments that - # wrote secrets before this encoding was introduced do not lose them. - legacy_value = ast.literal_eval(plaintext) - if not isinstance(legacy_value, bytes): - raise TypeError("Legacy encrypted extension storage value is not bytes") - return legacy_value + if not isinstance(plaintext, bytes): + raise TypeError("Decrypted extension storage values must be bytes") + return plaintext def _get_quota() -> int | None: diff --git a/superset/migrations/versions/2026-09-28_00-00_4c8f2a1d9b73_add_extension_storage_access.py b/superset/migrations/versions/2026-09-28_00-00_4c8f2a1d9b73_add_extension_storage_access.py index 58ca5cddc85c..7e651ab5f735 100644 --- a/superset/migrations/versions/2026-09-28_00-00_4c8f2a1d9b73_add_extension_storage_access.py +++ b/superset/migrations/versions/2026-09-28_00-00_4c8f2a1d9b73_add_extension_storage_access.py @@ -17,7 +17,7 @@ """Add browser access classification to extension storage. Revision ID: 4c8f2a1d9b73 -Revises: 95d8a99c822e +Revises: f72b6e9c104a Create Date: 2026-09-28 00:00:00.000000 """ @@ -25,7 +25,7 @@ from alembic import op revision = "4c8f2a1d9b73" -down_revision = "95d8a99c822e" +down_revision = "f72b6e9c104a" _LOOKUP_COLUMNS = [ "extension_id", diff --git a/superset/semantic_layers/api.py b/superset/semantic_layers/api.py index 3f9b7c2cc56d..64a24ed4a6e9 100644 --- a/superset/semantic_layers/api.py +++ b/superset/semantic_layers/api.py @@ -18,12 +18,20 @@ import logging import time - from dataclasses import asdict from typing import Any from uuid import UUID, uuid4 -from flask import Blueprint, current_app, make_response, redirect, render_template, request, Response, url_for +from flask import ( + Blueprint, + current_app, + make_response, + redirect, + render_template, + request, + Response, + url_for, +) from flask_appbuilder import AppBuilder from flask_appbuilder.api import BaseApi, expose, protect, rison, safe from flask_appbuilder.api.schemas import get_list_schema @@ -60,7 +68,6 @@ ) from superset.commands.semantic_layer.update import ( ClearSemanticLayerCacheCommand, - unmask_stored_configuration, UpdateSemanticLayerCommand, UpdateSemanticViewCommand, diff --git a/superset/semantic_layers/execution.py b/superset/semantic_layers/execution.py index f1ec2cebb454..3a901dcd4a9b 100644 --- a/superset/semantic_layers/execution.py +++ b/superset/semantic_layers/execution.py @@ -51,7 +51,7 @@ SEMANTIC_INSTALL_ID_KEY, ) from superset.semantic_layers.exceptions import provider_code -from superset.semantic_layers.registry import registry +from superset.semantic_layers.registry import registry, semantic_layer_context from superset.utils import json if TYPE_CHECKING: @@ -317,7 +317,7 @@ def forget(self, layer: SemanticLayer) -> None: del self.bindings[key] def bind( - self, layer: SemanticLayer + self, layer: SemanticLayer, view_uuid: str | None = None ) -> tuple[ProviderLayer[Any, Any], dict[tuple[str, str], ProviderView]]: """Construct once after policy validation, including on a warm memo hit.""" context: SemanticExecutionContext = current_host_execution_context() @@ -331,7 +331,10 @@ def bind( provider: type[ProviderLayer[Any, Any]] = registry[layer.type] configuration: dict[str, Any] = json.loads(layer.configuration) cache_token: str = layer.metadata_generation - with provider_code(): + with ( + provider_code(), + semantic_layer_context(layer.type, str(layer.uuid), view_uuid), + ): bound: ProviderLayer[Any, Any] = provider.from_configuration( configuration, cache_token=cache_token, diff --git a/superset/semantic_layers/models.py b/superset/semantic_layers/models.py index 802fd0157da5..94de2e9a3000 100644 --- a/superset/semantic_layers/models.py +++ b/superset/semantic_layers/models.py @@ -57,7 +57,6 @@ from superset.common.utils.dataframe_utils import df_columns_to_num from superset.exceptions import ( InvalidPostProcessingError, - OAuth2RedirectError, QueryObjectValidationError, ) from superset.explorables.base import TimeGrainDict @@ -84,6 +83,7 @@ from superset.semantic_layers.mapper import get_results from superset.semantic_layers.registry import ( contextualize_semantic_layer, + contextualize_semantic_view, registry, semantic_layer_context, ) @@ -332,7 +332,9 @@ def clear_metadata_cache(self) -> None: @property def implementation(self) -> SemanticLayerABC[Any, SemanticViewABC]: """Return the operation's guarded, identity-bound provider layer.""" - return SemanticBindingMemo.for_layer(self).bind(self)[0] + return contextualize_semantic_layer( + self.type, SemanticBindingMemo.for_layer(self).bind(self)[0], str(self.uuid) + ) class SemanticView(AuditMixinNullable, Model): @@ -429,15 +431,27 @@ def implementation(self) -> SemanticViewABC: layer: SemanticLayerABC[Any, Any] views: dict[tuple[str, str], SemanticViewABC] layer, views = SemanticBindingMemo.for_layer(self.semantic_layer).bind( - self.semantic_layer + self.semantic_layer, str(self.uuid) ) self._capture_metadata_generation() configuration: dict[str, Any] = json.loads(self.configuration) key: tuple[str, str] = (self.name, json.dumps(configuration, sort_keys=True)) if key not in views: - with provider_code(): + with ( + provider_code(), + semantic_layer_context( + self.semantic_layer.type, + str(self.semantic_layer.uuid), + str(self.uuid), + ), + ): views[key] = layer.get_semantic_view(self.name, configuration) - return views[key] + return contextualize_semantic_view( + self.semantic_layer.type, + views[key], + str(self.semantic_layer.uuid), + str(self.uuid), + ) # ========================================================================= # Explorable protocol implementation diff --git a/superset/semantic_layers/registry.py b/superset/semantic_layers/registry.py index 8e8d5bd7b9bf..c5ed1c0e7cb3 100644 --- a/superset/semantic_layers/registry.py +++ b/superset/semantic_layers/registry.py @@ -264,3 +264,17 @@ def contextualize_semantic_layer( return ContextualSemanticLayer( semantic_layer_id, implementation, layer_uuid, view_uuid ) + + +def contextualize_semantic_view( + semantic_layer_id: str, + implementation: Any, + layer_uuid: str | None = None, + view_uuid: str | None = None, +) -> Any: + """Attach the requesting resource to a session-memoized provider view.""" + if semantic_layer_id not in _extension_manifests and layer_uuid is None: + return implementation + return ContextualSemanticView( + semantic_layer_id, implementation, layer_uuid, view_uuid + ) diff --git a/tests/unit_tests/extensions/storage/test_persistent_dao.py b/tests/unit_tests/extensions/storage/test_persistent_dao.py index d8f87674af05..a4f7453bee3f 100644 --- a/tests/unit_tests/extensions/storage/test_persistent_dao.py +++ b/tests/unit_tests/extensions/storage/test_persistent_dao.py @@ -239,7 +239,7 @@ def test_dao_get_value_decrypts_encrypted_entry( entry.value = b"encrypted-bytes" mock_db.session.query.return_value.filter.return_value.first.return_value = entry mock_enc_type.return_value.process_result_value.return_value = ( - "superset-extension-storage:v1:eyJkZWNyeXB0ZWQiOiB0cnVlfQ==" + b'{"decrypted": true}' ) with app.app_context(): @@ -383,7 +383,7 @@ def test_dao_set_encrypts_value_when_requested( mock_enc_type.assert_called_once_with(1) mock_enc_type.return_value.process_bind_param.assert_called_once_with( - "superset-extension-storage:v1:cGxhaW50ZXh0", mock_db.engine.dialect + b"plaintext", mock_db.engine.dialect ) added_entry = mock_db.session.add.call_args[0][0] assert added_entry.value == b"ciphertext" @@ -415,7 +415,7 @@ def test_dao_set_forces_encryption_for_backend_access( assert added_entry.is_encrypted is True assert added_entry.access == StorageAccess.BACKEND.value mock_enc_type.return_value.process_bind_param.assert_called_once_with( - "superset-extension-storage:v1:cGxhaW50ZXh0", mock_db.engine.dialect + b"plaintext", mock_db.engine.dialect ) @@ -826,6 +826,7 @@ def test_enc_type_rejects_plaintext_that_is_neither_base64_nor_bytes( with pytest.raises(binascii.Error): enc.process_result_value(ciphertext, None) + @pytest.mark.parametrize("engine", ["aes", "aes-gcm"]) def test_encrypted_value_round_trip_preserves_arbitrary_bytes( app: Flask, engine: str @@ -842,10 +843,11 @@ def test_encrypted_value_round_trip_preserves_arbitrary_bytes( def test_decrypt_value_reads_legacy_bytes_representation(app: Flask) -> None: """Secrets written by the initial byte-based implementation remain readable.""" with app.app_context(): - encrypted = _enc_type(user_fk=42).process_bind_param( - b'{"access_token":"token"}', - dialect(), - ) + enc = _enc_type(user_fk=42) + enc._update_key() + encrypted = enc.engine.encrypt( + "superset-extension-storage:v1:eyJhY2Nlc3NfdG9rZW4iOiJ0b2tlbiJ9" + ).encode() assert _decrypt_value(encrypted, user_fk=42, dialect=dialect()) == ( b'{"access_token":"token"}' @@ -1099,9 +1101,7 @@ def test_list_decrypts_encrypted_entries( mock_db.session.query.return_value.filter.return_value.order_by.return_value ) page_query.offset.return_value.limit.return_value.all.return_value = [entry] - mock_enc_type.return_value.process_result_value.return_value = ( - "superset-extension-storage:v1:cGxhaW50ZXh0" - ) + mock_enc_type.return_value.process_result_value.return_value = b"plaintext" with app.app_context(): entries, total_count = ExtensionStorageDAO.list_entries("my-ext", user_fk=1) diff --git a/tests/unit_tests/semantic_layers/api_test.py b/tests/unit_tests/semantic_layers/api_test.py index 0f2cf070d171..86fb3a8c9961 100644 --- a/tests/unit_tests/semantic_layers/api_test.py +++ b/tests/unit_tests/semantic_layers/api_test.py @@ -3925,7 +3925,9 @@ def test_discovery_authentication_error( mocker.patch( "superset.semantic_layers.api.SemanticLayerDAO.find_by_uuid", return_value=layer ) - provider = MagicMock() + from superset_core.semantic_layers.context import ExecutionMethod + + provider = MagicMock(supported_execution_methods={ExecutionMethod.CONNECTION}) implementation = provider.from_configuration.return_value implementation.configuration = {} auth_error = SemanticLayerAuthenticationRequiredError("connect", "Connect first") diff --git a/tests/unit_tests/semantic_layers/execution_context_test.py b/tests/unit_tests/semantic_layers/execution_context_test.py index 1fafe9d6067a..f0c6175ec80f 100644 --- a/tests/unit_tests/semantic_layers/execution_context_test.py +++ b/tests/unit_tests/semantic_layers/execution_context_test.py @@ -29,6 +29,7 @@ from superset.common.query_context import QueryContext from superset.common.query_object import QueryObject from superset.semantic_layers.models import SemanticLayer, SemanticView +from superset.semantic_layers.registry import ContextualSemanticLayer from tests.unit_tests.semantic_layers.conftest import LegacyLayer @@ -61,13 +62,13 @@ def execution_view(app: Any, mocker: MockerFixture) -> SemanticView: return view -def test_connection_credentials_keys_unchanged( +def test_connection_credentials_metadata_keys_unchanged( client: Any, full_api_access: None, execution_view: SemanticView, mocker: MockerFixture, ) -> None: - """Connection credentials preserve upstream result-cache identity.""" + """Metadata keys stay stable while chart results gain user partitioning.""" cache: MagicMock = mocker.patch("superset.datasource.api.cache_manager").data_cache cache.get.return_value = ["cached"] keys: list[str] = [] @@ -102,9 +103,8 @@ def test_connection_credentials_keys_unchanged( "col_values:eb5d43a7322c2a03556fc53bc62d54fbbe0b3d26d561d18a0e385c7457abd822", # Compatibility identity includes changed_on="None" for this view. "compatible:a2e526c4cd1db4f68fd34cc7dffe06cc4fbd029975f2147b4b11bb55179795b8", - # Recorded on master ffee0a843f: #45133 adds semantic-null-filters-v1 - # to chart keys; the execution-context contract adds no discriminator. - "80b906df8cf7f4837b95d21c32c6f71bd6c810fcf7c1896dbeca74a5023ea961", + # Chart keys also include the requesting user's OAuth cache partition. + "14ee069dbffd0f7ca50bb720f7d2b9b1d2d4ade37cf50661e33562913dba0e6a", ] @@ -817,20 +817,33 @@ def test_one_session_two_principals_never_share_instances( assert first_layer.cache_token == "golden-generation" # noqa: S105 assert first_layer.credentials is None with override_user(bob): - assert other.implementation is not first_other + assert ( + other.implementation._implementation is not first_other._implementation + ) assert ( cast( LegacyLayer, execution_view.semantic_layer.implementation ).context.principal_id == "2" ) - assert execution_view.implementation is not first - assert execution_view.semantic_layer.implementation is not first_layer - assert execution_view.implementation is first - assert other.implementation is first_other + assert ( + execution_view.implementation._implementation + is not first._implementation + ) + assert ( + cast( + ContextualSemanticLayer, + execution_view.semantic_layer.implementation, + )._implementation + is not first_layer._implementation + ) + assert execution_view.implementation._implementation is first._implementation + assert other.implementation._implementation is first_other._implementation execution_view.forget_metadata() - assert other.implementation is not first_other - assert execution_view.implementation is not first + assert other.implementation._implementation is not first_other._implementation + assert ( + execution_view.implementation._implementation is not first._implementation + ) def test_policy_is_checked_on_warm_binding( diff --git a/tests/unit_tests/semantic_layers/models_test.py b/tests/unit_tests/semantic_layers/models_test.py index 65c6159a8f41..1b6e1e2663d2 100644 --- a/tests/unit_tests/semantic_layers/models_test.py +++ b/tests/unit_tests/semantic_layers/models_test.py @@ -63,6 +63,7 @@ SemanticLayer, SemanticView, ) +from superset.semantic_layers.registry import ContextualSemanticLayer from superset.superset_typing import ExplorableData from superset.utils.core import GenericDataType @@ -361,14 +362,23 @@ def test_semantic_layer_implementation_is_scoped_to_owning_session( assert first_layer is not None assert second_layer is not None assert first_layer is not second_layer - first: LegacyLayer = cast(LegacyLayer, first_layer.implementation) - assert first_layer.implementation is first - second: LegacyLayer = cast(LegacyLayer, second_layer.implementation) - assert second_layer.implementation is second - assert first_layer.implementation is first + first: Any = first_layer.implementation + assert ( + cast(ContextualSemanticLayer, first_layer.implementation)._implementation + is first._implementation + ) + second: Any = second_layer.implementation + assert ( + cast(ContextualSemanticLayer, second_layer.implementation)._implementation + is second._implementation + ) + assert ( + cast(ContextualSemanticLayer, first_layer.implementation)._implementation + is first._implementation + ) assert first.context == second.context assert first.context.principal_id == "7" - assert first is not second + assert first._implementation is not second._implementation assert factory.call_count == 2 @@ -391,8 +401,14 @@ def test_semantic_layer_implementation( with patch.dict( "superset.semantic_layers.models.registry", {"test_type": mock_class} ): - assert layer.implementation is mock_impl - assert layer.implementation is mock_impl + assert ( + cast(ContextualSemanticLayer, layer.implementation)._implementation + is mock_impl + ) + assert ( + cast(ContextualSemanticLayer, layer.implementation)._implementation + is mock_impl + ) mock_class.from_configuration.assert_called_once_with( {"key": "value"}, cache_token=layer.metadata_generation, @@ -646,6 +662,7 @@ def test_semantic_view_get_extra_cache_keys( ] if version is not None: expected.append(("semantic-result-version", "fixture", version)) + expected.append(("semantic_layer_user", None)) assert view.get_extra_cache_keys({}) == expected assert composed_key != query.cache_key(extra_cache_keys=expected[1:]) assert composed_key != query.cache_key( @@ -1491,14 +1508,14 @@ def test_semantic_view_implementation( memo: SemanticBindingMemo = SemanticBindingMemo() mocker.patch.object(SemanticBindingMemo, "for_layer", return_value=memo) mocker.patch.object(memo, "bind", return_value=(provider, {})) - assert view.implementation is first - assert view.implementation is first - assert equivalent.implementation is first + assert view.implementation._implementation is first + assert view.implementation._implementation is first + assert equivalent.implementation._implementation is first provider.get_semantic_view.assert_called_once_with( "Test View", {"tenant": "A", "key": "value"} ) - assert different.implementation is second - assert different.implementation is not first + assert different.implementation._implementation is second + assert different.implementation._implementation is not first assert provider.get_semantic_view.call_count == 2 provider.get_semantic_view.assert_called_with( "Test View", {"tenant": "B", "key": "value"} @@ -2917,6 +2934,7 @@ def test_result_generation_reads_class_without_provider_construction( ] if version is not None: expected.append(("semantic-result-version", "fixture", version)) + expected.append(("semantic_layer_user", None)) assert view.get_extra_cache_keys({}) == expected provider.assert_not_called() @@ -3354,15 +3372,19 @@ def test_values_rejection_logs_view_and_dimension( mock_implementation.get_values.assert_called_once() - -def test_personal_results_have_distinct_query_cache_keys() -> None: +def test_personal_results_have_distinct_query_cache_keys( + mocker: MockerFixture, explicit_model_namespace: None +) -> None: """Two users cannot reuse each other's provider data or shared query task.""" from superset.common.query_object import QueryObject - view = SemanticView(id=1) - view.__dict__["implementation"] = MagicMock() - view.implementation.uid.return_value = "semantic-view-1" - query = QueryObject(datasource=view, columns=["country"]) + provider = MagicMock() + provider.result_cache_version = None + mocker.patch.dict( + "superset.semantic_layers.models.registry", {"personal": provider} + ) + view = SemanticView(id=1, semantic_layer=SemanticLayer(type="personal")) + query = QueryObject(columns=["country"]) with patch("superset.semantic_layers.models.get_user_id", return_value=42): first = query.cache_key(extra_cache_keys=view.get_extra_cache_keys({})) repeated = query.cache_key(extra_cache_keys=view.get_extra_cache_keys({})) diff --git a/tests/unit_tests/tasks/test_async_queries.py b/tests/unit_tests/tasks/test_async_queries.py index a8c4479305ad..2958cbe3ab77 100644 --- a/tests/unit_tests/tasks/test_async_queries.py +++ b/tests/unit_tests/tasks/test_async_queries.py @@ -772,16 +772,31 @@ def provider_operation() -> str: proxy = contextualize_semantic_layer(provider_id, provider) query_context = _fake_query_context(num_queries=1) query_context.datasource = SemanticView(id=1) - query_context.datasource.__dict__["implementation"] = proxy.get_semantic_view( - "orders", {} + mocker.patch.object( + SemanticView, + "implementation", + new_callable=mock.PropertyMock, + return_value=proxy.get_semantic_view("orders", {}), + ) + mocker.patch.object( + SemanticView, + "result_cache_discriminator", + new_callable=mock.PropertyMock, + return_value=None, + ) + mocker.patch.object( + SemanticView, + "metadata_generation", + new_callable=mock.PropertyMock, + return_value="generation", ) def execute_query(query: object) -> mock.MagicMock: assert not has_request_context() assert query_context.datasource.implementation.uid() == "orders" - assert query_context.datasource.get_extra_cache_keys({}) == [ - ("semantic_layer_user", user_id) - ] + assert ("semantic_layer_user", user_id) in ( + query_context.datasource.get_extra_cache_keys({}) + ) return mocker.MagicMock(payload={}) query_context.get_df_payload_result.side_effect = execute_query From cae4cefb49dcadc50044fa66ac4490f0090abe9a Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Fri, 30 Oct 2026 13:51:04 -0400 Subject: [PATCH 06/13] fix(extensions): preserve context and deferred OAuth completion --- .../src/hooks/useOAuth2Completion.test.ts | 126 ++++++++++++++++++ .../src/hooks/useOAuth2Completion.ts | 24 +++- superset/core/api/core_api_injection.py | 3 +- .../unit_tests/extensions/api_context_test.py | 42 +++++- 4 files changed, 186 insertions(+), 9 deletions(-) create mode 100644 superset-frontend/src/hooks/useOAuth2Completion.test.ts diff --git a/superset-frontend/src/hooks/useOAuth2Completion.test.ts b/superset-frontend/src/hooks/useOAuth2Completion.test.ts new file mode 100644 index 000000000000..1d10b6697a24 --- /dev/null +++ b/superset-frontend/src/hooks/useOAuth2Completion.test.ts @@ -0,0 +1,126 @@ +/** + * 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. + */ +import { act, renderHook } from '@testing-library/react'; +import useOAuth2Completion from './useOAuth2Completion'; + +let channel: { + onmessage: ((event: { data: unknown }) => void) | null; + close: jest.Mock; +}; +const originalBroadcastChannel = global.BroadcastChannel; + +beforeEach(() => { + global.BroadcastChannel = jest.fn().mockImplementation(() => { + channel = { onmessage: null, close: jest.fn() }; + return channel; + }); +}); + +afterEach(() => { + global.BroadcastChannel = originalBroadcastChannel; +}); + +function notify(transport: string, tabId = 'attempt') { + act(() => { + if (transport === 'broadcast') { + channel.onmessage?.({ data: { tabId } }); + } else { + window.dispatchEvent( + new StorageEvent('storage', { + key: 'oauth2_auth_complete', + newValue: JSON.stringify({ tabId }), + }), + ); + } + }); +} + +test.each(['broadcast', 'storage'])( + 'retries a deferred %s completion after query state loads, once across transports', + transport => { + const onComplete = jest.fn().mockReturnValue(false); + const { rerender } = renderHook(() => + useOAuth2Completion('attempt', onComplete), + ); + + notify(transport); + expect(onComplete).toHaveBeenCalledTimes(1); + + const onQueryLoaded = jest.fn(); + rerender(); + expect(onComplete).toHaveBeenCalledTimes(2); + onComplete.mockImplementation(onQueryLoaded); + rerender(); + expect(onQueryLoaded).toHaveBeenCalledTimes(1); + + notify('broadcast'); + notify('storage'); + rerender(); + expect(onQueryLoaded).toHaveBeenCalledTimes(1); + }, +); + +test('uses the latest callback when retrying a deferred completion', () => { + const deferred = jest.fn().mockReturnValue(false); + const ready = jest.fn(); + const { rerender } = renderHook( + ({ onComplete }) => useOAuth2Completion('attempt', onComplete), + { initialProps: { onComplete: deferred } }, + ); + notify('broadcast'); + rerender({ onComplete: ready }); + expect(deferred).toHaveBeenCalledTimes(1); + expect(ready).toHaveBeenCalledTimes(1); +}); + +test('discards pending completions when the connection attempt changes', () => { + const onComplete = jest.fn().mockReturnValue(false); + const { rerender, unmount } = renderHook( + ({ tabId }: { tabId: string | undefined }) => + useOAuth2Completion(tabId, onComplete), + { initialProps: { tabId: 'attempt' } }, + ); + notify('storage'); + const previousChannel = channel; + onComplete.mockReturnValue(true); + rerender({ tabId: 'next-attempt' }); + expect(previousChannel.close).toHaveBeenCalledTimes(1); + notify('broadcast'); + expect(onComplete).toHaveBeenCalledTimes(1); + + notify('storage', 'next-attempt'); + expect(onComplete).toHaveBeenCalledTimes(2); + rerender({ tabId: undefined }); + expect(channel.close).toHaveBeenCalledTimes(1); + notify('storage', 'next-attempt'); + unmount(); + expect(onComplete).toHaveBeenCalledTimes(2); +}); + +test('does not retry a deferred completion after unmount', () => { + const onComplete = jest.fn().mockReturnValue(false); + const { unmount } = renderHook(() => + useOAuth2Completion('attempt', onComplete), + ); + notify('broadcast'); + unmount(); + expect(channel.close).toHaveBeenCalledTimes(1); + notify('storage'); + expect(onComplete).toHaveBeenCalledTimes(1); +}); diff --git a/superset-frontend/src/hooks/useOAuth2Completion.ts b/superset-frontend/src/hooks/useOAuth2Completion.ts index 3829e25190c4..cdabfe839401 100644 --- a/superset-frontend/src/hooks/useOAuth2Completion.ts +++ b/superset-frontend/src/hooks/useOAuth2Completion.ts @@ -28,23 +28,31 @@ export default function useOAuth2Completion( ) { const onCompleteRef = useRef(onComplete); onCompleteRef.current = onComplete; - const lastHandledTabIdRef = useRef(); + const retryCompletionRef = useRef<() => void>(); useEffect(() => { if (!tabId) return undefined; + let pending = false; + let handled = false; + const retryCompletion = () => { + if (pending && !handled && onCompleteRef.current() !== false) { + handled = true; + pending = false; + } + }; + retryCompletionRef.current = retryCompletion; const handleCompletion = (message: unknown) => { if ( typeof message !== 'object' || message === null || !('tabId' in message) || message.tabId !== tabId || - lastHandledTabIdRef.current === tabId + handled ) { return; } - if (onCompleteRef.current() !== false) { - lastHandledTabIdRef.current = tabId; - } + pending = true; + retryCompletion(); }; const channel = typeof BroadcastChannel !== 'undefined' @@ -63,8 +71,14 @@ export default function useOAuth2Completion( }; window.addEventListener('storage', handleStorage); return () => { + retryCompletionRef.current = undefined; window.removeEventListener('storage', handleStorage); channel?.close(); }; }, [tabId]); + + // A query can finish loading after its OAuth notification arrives. + useEffect(() => { + retryCompletionRef.current?.(); + }); } diff --git a/superset/core/api/core_api_injection.py b/superset/core/api/core_api_injection.py index c55666e30ccd..b6d2e7a4aa13 100644 --- a/superset/core/api/core_api_injection.py +++ b/superset/core/api/core_api_injection.py @@ -45,7 +45,8 @@ def _contextualize_extension_api( manifest: "Manifest", ) -> None: """Execute exposed extension API methods with extension and user context.""" - for attribute_name, attribute in vars(api_class).items(): + for attribute_name in dir(api_class): + attribute = getattr(api_class, attribute_name) if not callable(attribute) or not getattr(attribute, "_urls", None): continue setattr( diff --git a/tests/unit_tests/extensions/api_context_test.py b/tests/unit_tests/extensions/api_context_test.py index 5a34db3a13a3..5482d2bab769 100644 --- a/tests/unit_tests/extensions/api_context_test.py +++ b/tests/unit_tests/extensions/api_context_test.py @@ -62,9 +62,41 @@ def helper(self) -> str: assert TestApi.callback._urls == [("/callback", ("GET",))] +def test_inherited_api_routes_use_each_extensions_context() -> None: + """Wrapping subclasses must leave shared handlers and overrides untouched.""" + + class SharedApi: + @expose("/callback", methods=("GET",)) + def callback(self) -> str: + return get_context().extension.name + + class FirstApi(SharedApi): + pass + + class SecondApi(SharedApi): + pass + + class OverriddenApi(SharedApi): + def callback(self) -> str: + return "not exposed" + + original_callback = SharedApi.callback + original_override = OverriddenApi.callback + _contextualize_extension_api(FirstApi, create_manifest("acme", "first")) + _contextualize_extension_api(SecondApi, create_manifest("acme", "second")) + _contextualize_extension_api(OverriddenApi, create_manifest("acme", "third")) + + assert FirstApi().callback() == "first" + assert SecondApi().callback() == "second" + assert SharedApi.callback is original_callback + assert OverriddenApi.callback is original_override + assert get_current_extension_context() is None + + @pytest.mark.parametrize("authentication", ["jwt", "api_key", "unauthenticated"]) +@pytest.mark.parametrize("inherited", [False, True]) def test_extension_api_resolves_principal_after_protect( - mocker: MockerFixture, authentication: str + mocker: MockerFixture, authentication: str, inherited: bool ) -> None: """FAB authentication must finish before context and secrets resolve the user.""" app = Flask(__name__) @@ -106,8 +138,12 @@ def callback(self) -> dict[str, int | None]: assert get_context().storage.secrets.get("token") == "personal-token" return {"user_id": get_context().user_id} - _contextualize_extension_api(TestApi, manifest) - app.add_url_rule("/callback", view_func=TestApi().callback) + api_class = type("InheritedApi", (TestApi,), {}) if inherited else TestApi + original_callback = TestApi.callback + _contextualize_extension_api(api_class, manifest) + if inherited: + assert TestApi.callback is original_callback + app.add_url_rule("/callback", view_func=api_class().callback) headers = {} if authentication == "jwt": with app.app_context(): From c92c3998a84d4edeb8c92d4f6baf3f465d3ad755 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Fri, 30 Oct 2026 13:57:30 -0400 Subject: [PATCH 07/13] fix(semantic-layer): isolate filter values and verify storage migration --- .../extensions/contribution-types.md | 3 +- superset/datasource/api.py | 10 +- .../test_extension_storage_access.py | 121 ++++++++++++++++++ .../simple_cache_clear_test.py | 23 +++- .../semantic_layers/values_endpoint_test.py | 58 ++++++++- 5 files changed, 205 insertions(+), 10 deletions(-) create mode 100644 tests/unit_tests/migrations/test_extension_storage_access.py diff --git a/docs/developer_docs/extensions/contribution-types.md b/docs/developer_docs/extensions/contribution-types.md index e868d9393cad..f88a09bab70a 100644 --- a/docs/developer_docs/extensions/contribution-types.md +++ b/docs/developer_docs/extensions/contribution-types.md @@ -282,7 +282,8 @@ Store access and refresh tokens using the Python-only Never put personal credentials in the shared layer configuration, action results, query results, logs, or exceptions. The host executes provider methods under the owning extension and requesting user's context, including background chart work. -Semantic View query results and shared async query tasks are partitioned by user, +Semantic View query results, cached dimension values, and shared async query tasks +are partitioned by user, including providers without connection actions. This reduces cache sharing for providers with identical results for all users, but declaring no actions does not establish that a provider returns user-independent data. diff --git a/superset/datasource/api.py b/superset/datasource/api.py index a207204f0522..69eb0111a768 100644 --- a/superset/datasource/api.py +++ b/superset/datasource/api.py @@ -62,6 +62,7 @@ apply_max_row_limit, DatasourceType, FilterOperator, + get_user_id, parse_boolean_string, SqlExpressionType, ) @@ -95,6 +96,7 @@ def _column_values_cache_key( material: dict[str, Any] = dict(query) if isinstance(datasource, SemanticView): material["metadata_version"] = datasource.metadata_generation + material["semantic_layer_user"] = get_user_id() discriminator: tuple[str, str] | None = datasource.result_cache_discriminator if discriminator is not None: material["semantic_result_version"] = discriminator @@ -138,8 +140,8 @@ def get_cached_column_values( Callers must have authorized the datasource (``raise_for_access``) first: the key partitions entries by effective RLS (the datasource's own rules and - those of the tables its SQL reads), not by user, so the cache itself is not - an access check. ``search`` must already be stripped, with + those of the tables its SQL reads) and, for semantic views, the requesting + user. The cache itself is not an access check. ``search`` must be stripped, with ``None`` for "no search", so equal queries produce equal keys. Returns ``(values, cache_hit)``. ``force`` skips the read and refreshes @@ -382,7 +384,9 @@ def _column_values_response( # - ``uid`` / ``col`` / ``limit`` / ``denorm`` — basic query-shape # isolation so different inputs never collide. # - ``semantic_result_version`` — opt-in producer guarantee, omitted - # for SDK-default providers to preserve their legacy key bytes. + # for SDK-default providers. + # - ``semantic_layer_user`` — personal OAuth grants can return different + # values for each user, even when they have the same Superset access. force: bool = parse_boolean_string(request.args.get("force")) try: payload, cache_hit = get_cached_column_values( diff --git a/tests/unit_tests/migrations/test_extension_storage_access.py b/tests/unit_tests/migrations/test_extension_storage_access.py new file mode 100644 index 000000000000..74fe8dc4b6fd --- /dev/null +++ b/tests/unit_tests/migrations/test_extension_storage_access.py @@ -0,0 +1,121 @@ +# 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. +"""Exercise the access classification migration against a legacy storage table.""" + +from importlib import import_module + +import pytest +import sqlalchemy as sa +from alembic.migration import MigrationContext +from alembic.operations import Operations + +migration = import_module( + "superset.migrations.versions." + "2026-09-28_00-00_4c8f2a1d9b73_add_extension_storage_access" +) + + +@pytest.mark.parametrize("resource_scoped", [False, True]) +def test_access_migration_preserves_frontend_rows_and_removes_secrets( + resource_scoped: bool, +) -> None: + """Upgrade classifies legacy rows; downgrade never exposes backend secrets.""" + engine = sa.create_engine("sqlite:///:memory:") + metadata = sa.MetaData() + scoped_key = [ + "extension_id", + "user_fk", + "resource_type", + "resource_uuid", + "key", + ] + table = sa.Table( + "extension_storage", + metadata, + sa.Column("id", sa.Integer, primary_key=True), + sa.Column("extension_id", sa.String(255), nullable=False), + sa.Column("user_fk", sa.Integer), + sa.Column("resource_type", sa.String(64)), + sa.Column("resource_uuid", sa.String(36)), + sa.Column("key", sa.String(255), nullable=False), + sa.Column("value", sa.LargeBinary, nullable=False), + sa.UniqueConstraint(*scoped_key, name="uq_extension_storage_scoped_key"), + ) + sa.Index("ix_ext_storage_lookup", *(table.c[name] for name in scoped_key)) + metadata.create_all(engine) + scope = { + "extension_id": "acme.semantic", + "user_fk": 42, + "resource_type": "semantic_layer" if resource_scoped else None, + "resource_uuid": "00000000-0000-0000-0000-000000000001" + if resource_scoped + else None, + "key": "token", + } + legacy_value = b"browser-visible-setting" + try: + with engine.begin() as connection: + connection.execute(table.insert().values(id=1, value=legacy_value, **scope)) + with Operations.context(MigrationContext.configure(connection)): + migration.upgrade() + upgraded = sa.Table( + "extension_storage", sa.MetaData(), autoload_with=connection + ) + row = connection.execute(sa.select(upgraded)).mappings().one() + assert row["access"] == "frontend" + assert row["value"] == legacy_value + + # Both access classes must coexist under the same scoped key. + connection.execute( + upgraded.insert().values( + id=2, + access="backend", + value=b"encrypted-personal-token", + **scope, + ) + ) + connection.execute( + upgraded.insert().values( + **{**scope, "key": "refresh_token"}, + id=3, + access="backend", + value=b"encrypted-refresh-token", + ) + ) + migration.downgrade() + + downgraded = sa.Table( + "extension_storage", sa.MetaData(), autoload_with=connection + ) + assert "access" not in downgraded.c + assert connection.execute(sa.select(downgraded)).mappings().all() == [ + {"id": 1, "value": legacy_value, **scope} + ] + inspector = sa.inspect(connection) + assert inspector.get_unique_constraints("extension_storage") == [ + { + "name": "uq_extension_storage_scoped_key", + "column_names": scoped_key, + } + ] + indexes = { + index["name"]: index["column_names"] + for index in inspector.get_indexes("extension_storage") + } + assert indexes["ix_ext_storage_lookup"] == scoped_key + finally: + engine.dispose() diff --git a/tests/unit_tests/semantic_layers/simple_cache_clear_test.py b/tests/unit_tests/semantic_layers/simple_cache_clear_test.py index 8b71bf732314..3afbd31a4f82 100644 --- a/tests/unit_tests/semantic_layers/simple_cache_clear_test.py +++ b/tests/unit_tests/semantic_layers/simple_cache_clear_test.py @@ -18,7 +18,7 @@ from __future__ import annotations from collections.abc import Hashable -from typing import Any +from typing import Any, cast from unittest.mock import ANY, call, Mock, patch, PropertyMock from uuid import uuid4 @@ -32,6 +32,10 @@ from superset.commands.semantic_layer.exceptions import SemanticLayerUpdateFailedError from superset.commands.semantic_layer.update import UpdateSemanticLayerCommand from superset.semantic_layers.models import SemanticLayer, SemanticView +from superset.semantic_layers.registry import ( + ContextualSemanticLayer, + ContextualSemanticView, +) pytestmark: pytest.MarkDecorator = pytest.mark.parametrize( "app", [{"FEATURE_FLAGS": {"SEMANTIC_LAYERS": True}}], indirect=True @@ -151,7 +155,10 @@ def test_generation_binding_precedes_provider_construction( app.app_context(), patch.dict("superset.semantic_layers.models.registry", {"test": factory}), ): - assert layer.implementation is factory.from_configuration.return_value + assert ( + cast(ContextualSemanticLayer, layer.implementation)._implementation + is factory.from_configuration.return_value + ) factory.from_configuration.assert_called_once_with( {}, cache_token=layer.metadata_generation, @@ -163,6 +170,7 @@ def test_generation_binding_precedes_provider_construction( assert view.get_extra_cache_keys({}) == [ layer.metadata_generation, "semantic-null-filters-v1", + ("semantic_layer_user", None), ] @@ -180,6 +188,7 @@ def test_host_generation_is_named_apart_from_provider_token( assert view.get_extra_cache_keys({}) == [ layer.metadata_generation, "semantic-null-filters-v1", + ("semantic_layer_user", None), ] @@ -623,10 +632,15 @@ def test_clear_preserves_loaded_views_of_other_layers( from tests.unit_tests.semantic_layers.conftest import LegacyLayer with patch.dict("superset.semantic_layers.models.registry", {"test": LegacyLayer}): - implementation: Any = view.implementation + implementation = cast( + ContextualSemanticView, view.implementation + )._implementation before: str = view.metadata_generation layer.clear_metadata_cache() - assert view.implementation is implementation + assert ( + cast(ContextualSemanticView, view.implementation)._implementation + is implementation + ) assert view.metadata_generation == before @@ -669,6 +683,7 @@ def test_cache_keys_compose_metadata_and_producer_versions( first_generation, "semantic-null-filters-v1", ("semantic-result-version", "test", "v1"), + ("semantic_layer_user", None), ] assert first_annotation["source_versions"] == {"7": first_generation} assert first_annotation["semantic_result_versions"] == {"7": ("test", "v1")} diff --git a/tests/unit_tests/semantic_layers/values_endpoint_test.py b/tests/unit_tests/semantic_layers/values_endpoint_test.py index 43bcbfce6501..488b3c17114b 100644 --- a/tests/unit_tests/semantic_layers/values_endpoint_test.py +++ b/tests/unit_tests/semantic_layers/values_endpoint_test.py @@ -379,12 +379,12 @@ def test_unregistered_provider_values_return_safe_error_before_cache_lookup( @pytest.mark.parametrize("provider_type", ["sql", "cube", "snowflake", "metricflow"]) -def test_default_provider_value_cache_key_matches_legacy_bytes( +def test_default_provider_value_cache_key_preserves_scope( semantic_view_datasource: SemanticView, mocker: MockerFixture, provider_type: str, ) -> None: - """An SDK-default provider adds no producer field to metadata-versioned keys.""" + """Default providers retain RLS scope and add user scope for semantic views.""" import hashlib from superset_core.semantic_layers.layer import SemanticLayer as ProviderLayer @@ -392,6 +392,7 @@ def test_default_provider_value_cache_key_matches_legacy_bytes( from superset.datasource.api import _column_values_cache_key from superset.utils import json + mocker.patch("superset.datasource.api.get_user_id", return_value=42) datasource: Any = semantic_view_datasource if provider_type == "sql": datasource = MagicMock(uid="semantic_view_uid_123", changed_on=None) @@ -419,6 +420,7 @@ def test_default_provider_value_cache_key_matches_legacy_bytes( } if isinstance(datasource, SemanticView): legacy["metadata_version"] = datasource.metadata_generation + legacy["semantic_layer_user"] = 42 expected: str = ( "col_values:" + hashlib.sha256(json.dumps(legacy, sort_keys=True).encode()).hexdigest() @@ -427,6 +429,58 @@ def test_default_provider_value_cache_key_matches_legacy_bytes( assert "semantic_result_version" not in query +@pytest.mark.parametrize("authorized", [True, False]) +def test_column_values_cache_isolates_personal_grants( + semantic_view_datasource: SemanticView, + mocker: MockerFixture, + authorized: bool, +) -> None: + """A warm cache cannot bypass another user's narrower or missing OAuth grant.""" + from superset.datasource.api import get_cached_column_values + from superset.exceptions import OAuth2RedirectError + + user = mocker.patch("superset.datasource.api.get_user_id", return_value=42) + mocker.patch( + "superset.datasource.api.security_manager.get_rls_cache_key", return_value=[] + ) + cache = mocker.patch("superset.datasource.api.cache_manager").data_cache + stored: dict[str, Any] = {} + cache.get.side_effect = stored.get + cache.set.side_effect = lambda key, value, **kwargs: stored.__setitem__(key, value) + implementation = cast(MagicMock, semantic_view_datasource.implementation) + + def values() -> tuple[list[Any], bool]: + return get_cached_column_values( + semantic_view_datasource, + column_name="category", + limit=500, + denormalize_column=False, + ) + + assert values() == (["Books", "Clothing"], False) + assert values() == (["Books", "Clothing"], True) + implementation.get_values.assert_called_once() + + user.return_value = 43 + if authorized: + implementation.get_values.return_value = SemanticResult( + requests=[], results=pa.table({"category": ["Books"]}) + ) + assert values() == (["Books"], False) + assert values() == (["Books"], True) + else: + implementation.get_values.side_effect = OAuth2RedirectError( + "https://example.com/oauth", "tab-id", "https://example.com/complete" + ) + with pytest.raises(OAuth2RedirectError): + values() + assert cache.set.call_count == 1 + + assert implementation.get_values.call_count == 2 + user.return_value = 42 + assert values() == (["Books", "Clothing"], True) + + def test_versioned_suggestions_are_unavailable_before_cache( client: Any, full_api_access: None, From c12e6eb98f8c5fb40824db4e5398431edded5d2a Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Fri, 30 Oct 2026 14:11:11 -0400 Subject: [PATCH 08/13] docs(extensions): fix cross-plugin proxy configuration link --- docs/developer_docs/extensions/contribution-types.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/developer_docs/extensions/contribution-types.md b/docs/developer_docs/extensions/contribution-types.md index f88a09bab70a..88c7df9598f1 100644 --- a/docs/developer_docs/extensions/contribution-types.md +++ b/docs/developer_docs/extensions/contribution-types.md @@ -317,7 +317,7 @@ allows the user to start another attempt. The completion URL uses the request's externally visible host and scheme. Operators must configure their reverse proxy, forwarded headers, and allowed hosts so Superset sees its trusted public HTTPS origin. See the -[reverse proxy configuration](../../admin_docs/configuration/configuring-superset.mdx#configuration-behind-a-load-balancer). +[reverse proxy configuration](/admin-docs/configuration/configuring-superset#configuration-behind-a-load-balancer). Action POST responses contain `result.redirect_url` and a host-generated `result.tab_id`. Both the configuration modal and the generic OAuth error UI From 97a314315fbcae10d1d02e0344990c1ea3b81d22 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Fri, 30 Oct 2026 14:48:39 -0400 Subject: [PATCH 09/13] chore(semantic-layer): refresh API spec and translation template --- docs/static/resources/openapi.json | 35 ++++++++++++++++++++++++++++++ superset/translations/messages.pot | 27 ++++++++++++++++++----- 2 files changed, 56 insertions(+), 6 deletions(-) diff --git a/docs/static/resources/openapi.json b/docs/static/resources/openapi.json index 028af7aa04bb..a4acba7ce0d5 100644 --- a/docs/static/resources/openapi.json +++ b/docs/static/resources/openapi.json @@ -35169,6 +35169,13 @@ ] } }, + "/api/v1/semantic_layer/oauth2/complete/{tab_id}": { + "get": { + "tags": [ + "Semantic Layers" + ] + } + }, "/api/v1/semantic_layer/schema/configuration": { "post": { "requestBody": { @@ -35214,6 +35221,13 @@ ] } }, + "/api/v1/semantic_layer/type/{sl_type}/actions/{action_id}": { + "post": { + "tags": [ + "Semantic Layers" + ] + } + }, "/api/v1/semantic_layer/types": { "get": { "responses": { @@ -35448,6 +35462,20 @@ ] } }, + "/api/v1/semantic_layer/{uuid}/actions": { + "get": { + "tags": [ + "Semantic Layers" + ] + } + }, + "/api/v1/semantic_layer/{uuid}/actions/{action_id}": { + "post": { + "tags": [ + "Semantic Layers" + ] + } + }, "/api/v1/semantic_layer/{uuid}/clear_cache": { "post": { "parameters": [ @@ -35505,6 +35533,13 @@ ] } }, + "/api/v1/semantic_layer/{uuid}/oauth2/{action_id}": { + "get": { + "tags": [ + "Semantic Layers" + ] + } + }, "/api/v1/semantic_layer/{uuid}/schema/runtime": { "post": { "parameters": [ diff --git a/superset/translations/messages.pot b/superset/translations/messages.pot index f4555d86d762..d1f16aa8f9bc 100644 --- a/superset/translations/messages.pot +++ b/superset/translations/messages.pot @@ -25,7 +25,7 @@ msgid "" msgstr "" "Project-Id-Version: Superset VERSION\n" "Report-Msgid-Bugs-To: EMAIL@ADDRESS\n" -"POT-Creation-Date: 2026-10-27 21:53-0300\n" +"POT-Creation-Date: 2026-10-30 18:46+0000\n" "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n" "Last-Translator: FULL NAME \n" "Language-Team: LANGUAGE \n" @@ -1769,6 +1769,9 @@ msgstr "" msgid "Allow node selections" msgstr "" +msgid "Allow popups to connect this semantic layer" +msgstr "" + msgid "Allow the execution of DDL (Data Definition Language: CREATE, DROP, TRUNCATE, etc.) and DML (Data Modification Language: INSERT, UPDATE, DELETE, etc)" msgstr "" @@ -2071,6 +2074,9 @@ msgstr "" msgid "An error occurred while starring this chart" msgstr "" +msgid "An error occurred while starting the connection" +msgstr "" + msgid "An error occurred while storing your query in the backend. To avoid losing your changes, please save your query using the \"Save Query\" button." msgstr "" @@ -4085,6 +4091,9 @@ msgstr "" msgid "Connect to engine" msgstr "" +msgid "Connected" +msgstr "" + msgid "Connection" msgstr "" @@ -6359,11 +6368,11 @@ msgid "Error in jinja expression in WHERE clause: %(msg)s" msgstr "" #, python-format -msgid "Error in jinja expression in adhoc column: %(msg)s" +msgid "Error in jinja expression in adhoc SQL expression: %(msg)s" msgstr "" #, python-format -msgid "Error in jinja expression in adhoc SQL expression: %(msg)s" +msgid "Error in jinja expression in adhoc column: %(msg)s" msgstr "" #, python-format @@ -14241,6 +14250,12 @@ msgstr "" msgid "The configuration for the map layers" msgstr "" +msgid "The connection action did not return a URL" +msgstr "" + +msgid "The connection action did not return a tab ID" +msgstr "" + msgid "The corner radius of the chart background" msgstr "" @@ -15404,6 +15419,9 @@ msgstr "" msgid "This dashboard will be restored to the following version:" msgstr "" +msgid "This data source uses OAuth2 for authentication. Please click the link above to grant Apache Superset permission to access the data. Your personal access token will be stored encrypted and used only for queries run by you." +msgstr "" + msgid "This database does not allow creating tables from queries (CTAS). Please contact your administrator for more assistance." msgstr "" @@ -15419,9 +15437,6 @@ msgstr "" msgid "This database table does not contain any data. Please select a different table." msgstr "" -msgid "This database uses OAuth2 for authentication. Please click the link above to grant Apache Superset permission to access the data. Your personal access token will be stored encrypted and used only for queries run by you." -msgstr "" - msgid "This dataset is managed externally, and can't be edited in Superset" msgstr "" From c0a149f816d5159ad5842c88d5ed7fd43af67410 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Fri, 30 Oct 2026 15:22:42 -0400 Subject: [PATCH 10/13] fix(semantic-layer): use guarded navigation for OAuth tabs --- .../SemanticLayerModal.test.tsx | 24 ++++++---- .../semanticLayers/SemanticLayerModal.tsx | 9 ++-- .../src/utils/navigationUtils.test.ts | 47 ++++++++++--------- .../src/utils/navigationUtils.ts | 27 +++-------- 4 files changed, 52 insertions(+), 55 deletions(-) diff --git a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx index bfffc04a8b3b..00da8be5dd6b 100644 --- a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx +++ b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx @@ -771,7 +771,7 @@ test('starts a required semantic layer configuration action', async () => { }); const popup = { close: jest.fn(), - location: { href: '' }, + location: { replace: jest.fn() }, } as unknown as Window; const open = jest.spyOn(window, 'open').mockReturnValue(popup); @@ -791,12 +791,12 @@ test('starts a required semantic layer configuration action', async () => { semantic_layer_uuid: props.semanticLayerUuid, }, }); - expect(open).toHaveBeenCalledWith( - '', - 'semantic-layer-action-oauth2', - 'popup,width=720,height=800', + expect(open).toHaveBeenCalledWith('', '_blank'); + expect(popup.opener).toBeNull(); + expect(popup.location.replace).toHaveBeenCalledWith( + 'https://auth.example.test/authorize', ); - expect(popup.location.href).toBe('https://auth.example.test/authorize'); + expect(open).toHaveBeenCalledTimes(1); }); const callsBeforeCompletion = mockedPost.mock.calls.length; @@ -846,14 +846,22 @@ test('allows another connection attempt after the popup is cancelled', async () }, }, }); - const popup = { close: jest.fn(), closed: false, location: { href: '' } }; + const popup = { + close: jest.fn(), + closed: false, + location: { replace: jest.fn() }, + }; const open = jest .spyOn(window, 'open') .mockReturnValue(popup as unknown as Window); render(); const connect = await screen.findByRole('button', { name: 'Connect' }); fireEvent.click(connect); - await waitFor(() => expect(popup.location.href).toContain('/authorize')); + await waitFor(() => + expect(popup.location.replace).toHaveBeenCalledWith( + 'https://auth.example.test/authorize', + ), + ); expect(connect).toBeDisabled(); popup.closed = true; await act(async () => { diff --git a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx index ef92071a4492..2409371a372f 100644 --- a/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx +++ b/superset-frontend/src/features/semanticLayers/SemanticLayerModal.tsx @@ -34,6 +34,7 @@ import { } from 'src/components/Modal'; import { styled } from '@apache-superset/core/theme'; import useOAuth2Completion from 'src/hooks/useOAuth2Completion'; +import { navigateOpenedTab, openBlankTab } from 'src/utils/navigationUtils'; import { renderers, sanitizeSchema, @@ -406,11 +407,7 @@ export default function SemanticLayerModal({ const handleConfigurationAction = async (action: SemanticLayerAction) => { if (!selectedType) return; - const popup = window.open( - '', - `semantic-layer-action-${action.id}`, - 'popup,width=720,height=800', - ); + const popup = openBlankTab(); if (!popup) { addDangerToast(t('Allow popups to connect this semantic layer')); return; @@ -439,7 +436,7 @@ export default function SemanticLayerModal({ } oauthPopupRef.current = popup; setOauthTabId(tabId); - popup.location.href = redirectUrl; + navigateOpenedTab(popup, redirectUrl); } catch (error) { popup.close(); const clientError = await getClientErrorObject(error); diff --git a/superset-frontend/src/utils/navigationUtils.test.ts b/superset-frontend/src/utils/navigationUtils.test.ts index d866613a1370..8d7ca18761e4 100644 --- a/superset-frontend/src/utils/navigationUtils.test.ts +++ b/superset-frontend/src/utils/navigationUtils.test.ts @@ -836,6 +836,7 @@ describe('openBlankTab / navigateOpenedTab / closeOpenedTab', () => { expect(tab.location.replace as jest.Mock).toHaveBeenCalledWith( '/dashboard/9/', ); + expect(tab.opener).toBeNull(); // The live-handle branch must NOT fall through to a second window.open, // which by the time it runs has lost user activation and is popup-blocked. expect(openSpy).not.toHaveBeenCalled(); @@ -887,38 +888,42 @@ describe('openBlankTab / navigateOpenedTab / closeOpenedTab', () => { }); }); - test('navigateOpenedTab does not expose the opener-connected tab to an external URL', async () => { + test('navigateOpenedTab detaches the opener before navigating to an external URL', async () => { await withApplicationRoot('', async () => { const tab = makeTab(); const { navigateOpenedTab } = await import('src/utils/navigationUtils'); - // A safe-but-absolute URL is permitted by assertSafeNavigationUrl, but - // must not be navigated on the noopener-less claimed tab: it is closed - // and reopened through the noopener fallback instead. + const replace = tab.location.replace as jest.Mock; + replace.mockImplementation(() => expect(tab.opener).toBeNull()); navigateOpenedTab(tab, 'https://example.com/'); - expect(tab.location.replace as jest.Mock).not.toHaveBeenCalled(); - expect(tab.close as jest.Mock).toHaveBeenCalledTimes(1); - expect(openSpy).toHaveBeenCalledWith( - 'https://example.com/', - '_blank', - 'noopener noreferrer', - ); - }); - }); - - test('navigateOpenedTab validates the URL before touching a claimed tab', async () => { - await withApplicationRoot('', async () => { - const tab = makeTab(); - const { navigateOpenedTab } = await import('src/utils/navigationUtils'); - - expect(() => navigateOpenedTab(tab, '//evil.com')).toThrow(); - expect(tab.location.replace as jest.Mock).not.toHaveBeenCalled(); + expect(replace).toHaveBeenCalledWith('https://example.com/'); expect(tab.close as jest.Mock).not.toHaveBeenCalled(); expect(openSpy).not.toHaveBeenCalled(); }); }); + test.each([ + '//evil.com', + 'https://trusted.example@evil.com/', + '/\\evil.com', + '/bad\nurl', + ])( + 'navigateOpenedTab rejects %j before touching a claimed tab', + async url => { + await withApplicationRoot('', async () => { + const tab = makeTab(); + const { navigateOpenedTab } = await import('src/utils/navigationUtils'); + + expect(() => navigateOpenedTab(tab, url)).toThrow(); + expect(tab.opener).toBeUndefined(); + expect(tab.location.replace as jest.Mock).not.toHaveBeenCalled(); + expect(tab.close as jest.Mock).not.toHaveBeenCalled(); + expect(openSpy).not.toHaveBeenCalled(); + }); + }, + ); + test('closeOpenedTab closes a live tab and no-ops on null or already-closed', async () => { const { closeOpenedTab } = await import('src/utils/navigationUtils'); const tab = makeTab(); diff --git a/superset-frontend/src/utils/navigationUtils.ts b/superset-frontend/src/utils/navigationUtils.ts index 1719082eae7a..7f165a395dce 100644 --- a/superset-frontend/src/utils/navigationUtils.ts +++ b/superset-frontend/src/utils/navigationUtils.ts @@ -471,8 +471,8 @@ export function openInNewTab(path: string): void { * `window.open(..., 'noopener')` always returns null, which would discard the * very handle this function exists to return and leave the caller with a * stranded `about:blank` tab it can never navigate. The destination is always - * a same-origin app route (`ensureAppRoot` in `navigateOpenedTab`), so the - * opener relationship carries no cross-origin tabnabbing risk. The one-shot + * a validated URL (`ensureAppRoot` in `navigateOpenedTab`), and the opener is + * detached before navigation to prevent cross-origin tabnabbing. The one-shot * `window.open(url, ...)` fallback in `navigateOpenedTab` keeps `noopener`, * since it passes the real URL and never needs the handle. */ @@ -487,30 +487,17 @@ export function openBlankTab(): Window | null { * The URL is validated before either branch, so an unsafe path cannot reach * a pre-opened tab any more than it could reach `openInNewTab`. * - * The external-URL branch (a safe absolute URL on a claimed tab) is defensive: - * no current caller passes one, and by the time it would run the click's - * activation has lapsed, so the reopen is popup-blocked and the placeholder - * closes with nothing opening. It exists only to keep an absolute URL off the - * opener-connected tab; a caller needing an external target after an await - * should not claim a tab up front. + * The opener is detached before navigating, including for app routes that may + * redirect to an external OAuth provider. Reusing the tab preserves the handle + * and avoids attempting to open a second window after user activation expires. */ export function navigateOpenedTab(tab: Window | null, path: string): void { const url = assertSafeNavigationUrl(ensureAppRoot(path)); - // Only a same-origin app route may reuse the opener-connected placeholder - // from `openBlankTab` (which drops `noopener` to keep its handle). - // `ensureAppRoot` leaves an absolute URL untouched, so anything not starting - // with a single `/` is external: it must not ride the opener chain. Route it - // through the `noopener` fallback instead — closing the claimed tab first so - // no `about:blank` is stranded — which makes the same-origin guarantee - // structural rather than a caller convention. - const isSameOriginRoute = url.startsWith('/') && !url.startsWith('//'); - if (isSameOriginRoute && tab && !tab.closed) { + if (tab && !tab.closed) { + tab.opener = null; tab.location.replace(url); return; } - if (tab && !tab.closed) { - tab.close(); - } window.open(url, '_blank', NEW_TAB_FEATURES); } From 5d41d7f4666bea9445fbf0daf9c2fdbdada4718d Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Fri, 30 Oct 2026 16:14:49 -0400 Subject: [PATCH 11/13] fix(semantic-layer): document OAuth endpoint response contracts --- docs/static/resources/openapi.json | 376 +++++++++++++++++++++++++++++ superset/semantic_layers/api.py | 240 +++++++++++++++++- 2 files changed, 611 insertions(+), 5 deletions(-) diff --git a/docs/static/resources/openapi.json b/docs/static/resources/openapi.json index a4acba7ce0d5..e8b1ff657a07 100644 --- a/docs/static/resources/openapi.json +++ b/docs/static/resources/openapi.json @@ -35171,6 +35171,50 @@ }, "/api/v1/semantic_layer/oauth2/complete/{tab_id}": { "get": { + "parameters": [ + { + "in": "path", + "name": "tab_id", + "required": true, + "schema": { + "format": "uuid", + "type": "string" + } + } + ], + "responses": { + "200": { + "content": { + "text/html": { + "schema": { + "type": "string" + } + } + }, + "description": "HTML page that signals completion without exposing tokens" + }, + "401": { + "$ref": "#/components/responses/401" + }, + "403": { + "$ref": "#/components/responses/403" + }, + "404": { + "$ref": "#/components/responses/404" + }, + "500": { + "$ref": "#/components/responses/500" + } + }, + "security": [ + { + "jwt": [] + }, + { + "api_key": [] + } + ], + "summary": "Notify the initiating tab that OAuth authorization completed", "tags": [ "Semantic Layers" ] @@ -35223,6 +35267,95 @@ }, "/api/v1/semantic_layer/type/{sl_type}/actions/{action_id}": { "post": { + "parameters": [ + { + "in": "path", + "name": "sl_type", + "required": true, + "schema": { + "type": "string" + } + }, + { + "in": "path", + "name": "action_id", + "required": true, + "schema": { + "type": "string" + } + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "properties": { + "configuration": { + "type": "object" + }, + "semantic_layer_uuid": { + "description": "Existing layer whose masked configuration to resolve", + "format": "uuid", + "type": "string" + } + }, + "type": "object" + } + } + }, + "required": true + }, + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "properties": { + "result": { + "properties": { + "redirect_url": { + "format": "uri", + "type": "string" + }, + "tab_id": { + "format": "uuid", + "type": "string" + } + }, + "type": "object" + } + }, + "type": "object" + } + } + }, + "description": "Authorization URL and connection correlation ID" + }, + "400": { + "$ref": "#/components/responses/400" + }, + "401": { + "$ref": "#/components/responses/401" + }, + "403": { + "$ref": "#/components/responses/403" + }, + "404": { + "$ref": "#/components/responses/404" + }, + "500": { + "$ref": "#/components/responses/500" + } + }, + "security": [ + { + "jwt": [] + }, + { + "api_key": [] + } + ], + "summary": "Start a semantic layer configuration action", "tags": [ "Semantic Layers" ] @@ -35464,6 +35597,94 @@ }, "/api/v1/semantic_layer/{uuid}/actions": { "get": { + "parameters": [ + { + "in": "path", + "name": "uuid", + "required": true, + "schema": { + "format": "uuid", + "type": "string" + } + }, + { + "description": "Semantic view through which the user accesses the layer", + "in": "query", + "name": "view_uuid", + "schema": { + "format": "uuid", + "type": "string" + } + } + ], + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "properties": { + "result": { + "items": { + "properties": { + "id": { + "type": "string" + }, + "label": { + "type": "string" + }, + "message": { + "nullable": true, + "type": "string" + }, + "required": { + "type": "boolean" + }, + "status": { + "enum": [ + "required", + "connected", + "expired", + "error" + ], + "type": "string" + } + }, + "type": "object" + }, + "type": "array" + } + }, + "type": "object" + } + } + }, + "description": "Available configuration actions and their connection status" + }, + "400": { + "$ref": "#/components/responses/400" + }, + "401": { + "$ref": "#/components/responses/401" + }, + "403": { + "$ref": "#/components/responses/403" + }, + "404": { + "$ref": "#/components/responses/404" + }, + "500": { + "$ref": "#/components/responses/500" + } + }, + "security": [ + { + "jwt": [] + }, + { + "api_key": [] + } + ], + "summary": "List personal configuration actions for a semantic layer", "tags": [ "Semantic Layers" ] @@ -35471,6 +35692,85 @@ }, "/api/v1/semantic_layer/{uuid}/actions/{action_id}": { "post": { + "parameters": [ + { + "in": "path", + "name": "uuid", + "required": true, + "schema": { + "format": "uuid", + "type": "string" + } + }, + { + "in": "path", + "name": "action_id", + "required": true, + "schema": { + "type": "string" + } + }, + { + "description": "Semantic view through which the user accesses the layer", + "in": "query", + "name": "view_uuid", + "schema": { + "format": "uuid", + "type": "string" + } + } + ], + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "properties": { + "result": { + "properties": { + "redirect_url": { + "format": "uri", + "type": "string" + }, + "tab_id": { + "format": "uuid", + "type": "string" + } + }, + "type": "object" + } + }, + "type": "object" + } + } + }, + "description": "Authorization URL and connection correlation ID" + }, + "400": { + "$ref": "#/components/responses/400" + }, + "401": { + "$ref": "#/components/responses/401" + }, + "403": { + "$ref": "#/components/responses/403" + }, + "404": { + "$ref": "#/components/responses/404" + }, + "500": { + "$ref": "#/components/responses/500" + } + }, + "security": [ + { + "jwt": [] + }, + { + "api_key": [] + } + ], + "summary": "Start a personal configuration action for a semantic layer", "tags": [ "Semantic Layers" ] @@ -35535,6 +35835,82 @@ }, "/api/v1/semantic_layer/{uuid}/oauth2/{action_id}": { "get": { + "parameters": [ + { + "in": "path", + "name": "uuid", + "required": true, + "schema": { + "format": "uuid", + "type": "string" + } + }, + { + "in": "path", + "name": "action_id", + "required": true, + "schema": { + "type": "string" + } + }, + { + "description": "Correlation ID for the initiating browser tab", + "in": "query", + "name": "tab_id", + "required": true, + "schema": { + "format": "uuid", + "type": "string" + } + }, + { + "description": "Semantic view through which the user accesses the layer", + "in": "query", + "name": "view_uuid", + "schema": { + "format": "uuid", + "type": "string" + } + } + ], + "responses": { + "302": { + "description": "Redirect to the authorization provider", + "headers": { + "Location": { + "description": "Provider authorization URL", + "schema": { + "format": "uri", + "type": "string" + } + } + } + }, + "400": { + "$ref": "#/components/responses/400" + }, + "401": { + "$ref": "#/components/responses/401" + }, + "403": { + "$ref": "#/components/responses/403" + }, + "404": { + "$ref": "#/components/responses/404" + }, + "500": { + "$ref": "#/components/responses/500" + } + }, + "security": [ + { + "jwt": [] + }, + { + "api_key": [] + } + ], + "summary": "Redirect to the semantic layer's authorization provider", "tags": [ "Semantic Layers" ] diff --git a/superset/semantic_layers/api.py b/superset/semantic_layers/api.py index 64a24ed4a6e9..3f8c727bf52e 100644 --- a/superset/semantic_layers/api.py +++ b/superset/semantic_layers/api.py @@ -909,7 +909,62 @@ def configuration_action( sl_type: str, action_id: str, ) -> FlaskResponse: - """Start a configuration action before a semantic layer exists.""" + """Start a configuration action before a semantic layer exists. + --- + post: + summary: Start a semantic layer configuration action + parameters: + - in: path + name: sl_type + required: true + schema: + type: string + - in: path + name: action_id + required: true + schema: + type: string + requestBody: + required: true + content: + application/json: + schema: + type: object + properties: + configuration: + type: object + semantic_layer_uuid: + type: string + format: uuid + description: Existing layer whose masked configuration to resolve + responses: + 200: + description: Authorization URL and connection correlation ID + content: + application/json: + schema: + type: object + properties: + result: + type: object + properties: + redirect_url: + type: string + format: uri + tab_id: + type: string + format: uuid + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 500: + $ref: '#/components/responses/500' + """ cls = registry.get(sl_type) if not cls: return self.response_400(message=f"Unknown type: {sl_type}") @@ -961,7 +1016,59 @@ def configuration_action( @handle_api_exception @statsd_metrics def configuration_actions(self, uuid: str) -> FlaskResponse: - """Return configuration actions for an existing semantic layer.""" + """Return configuration actions for an existing semantic layer. + --- + get: + summary: List personal configuration actions for a semantic layer + parameters: + - in: path + name: uuid + required: true + schema: + type: string + format: uuid + - in: query + name: view_uuid + description: Semantic view through which the user accesses the layer + schema: + type: string + format: uuid + responses: + 200: + description: Available configuration actions and their connection status + content: + application/json: + schema: + type: object + properties: + result: + type: array + items: + type: object + properties: + id: + type: string + label: + type: string + status: + type: string + enum: [required, connected, expired, error] + required: + type: boolean + message: + type: string + nullable: true + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 500: + $ref: '#/components/responses/500' + """ layer, error = self._action_layer(uuid, request.args.get("view_uuid")) if error is not None: return error @@ -992,7 +1099,56 @@ def semantic_layer_action( uuid: str, action_id: str, ) -> FlaskResponse: - """Start a personal configuration action for an existing layer.""" + """Start a personal configuration action for an existing layer. + --- + post: + summary: Start a personal configuration action for a semantic layer + parameters: + - in: path + name: uuid + required: true + schema: + type: string + format: uuid + - in: path + name: action_id + required: true + schema: + type: string + - in: query + name: view_uuid + description: Semantic view through which the user accesses the layer + schema: + type: string + format: uuid + responses: + 200: + description: Authorization URL and connection correlation ID + content: + application/json: + schema: + type: object + properties: + result: + type: object + properties: + redirect_url: + type: string + format: uri + tab_id: + type: string + format: uuid + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 500: + $ref: '#/components/responses/500' + """ layer, error = self._action_layer(uuid, request.args.get("view_uuid")) if error is not None: return error @@ -1066,7 +1222,55 @@ def _action_layer( @handle_api_exception @statsd_metrics def oauth2_start(self, uuid: str, action_id: str) -> FlaskResponse: - """Start a personal connection after an OAuth2 error link is clicked.""" + """Start a personal connection after an OAuth2 error link is clicked. + --- + get: + summary: Redirect to the semantic layer's authorization provider + parameters: + - in: path + name: uuid + required: true + schema: + type: string + format: uuid + - in: path + name: action_id + required: true + schema: + type: string + - in: query + name: tab_id + required: true + description: Correlation ID for the initiating browser tab + schema: + type: string + format: uuid + - in: query + name: view_uuid + description: Semantic view through which the user accesses the layer + schema: + type: string + format: uuid + responses: + 302: + description: Redirect to the authorization provider + headers: + Location: + description: Provider authorization URL + schema: + type: string + format: uri + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 500: + $ref: '#/components/responses/500' + """ layer, error = self._action_layer(uuid, request.args.get("view_uuid")) if error is not None: return error @@ -1093,7 +1297,33 @@ def oauth2_start(self, uuid: str, action_id: str) -> FlaskResponse: @protect_read("SemanticLayer", "SemanticView") @safe def oauth2_complete(self, tab_id: UUID) -> FlaskResponse: - """Notify the initiating tab without exposing credentials to the browser.""" + """Notify the initiating tab without exposing credentials to the browser. + --- + get: + summary: Notify the initiating tab that OAuth authorization completed + parameters: + - in: path + name: tab_id + required: true + schema: + type: string + format: uuid + responses: + 200: + description: HTML page that signals completion without exposing tokens + content: + text/html: + schema: + type: string + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 500: + $ref: '#/components/responses/500' + """ if not is_feature_enabled("SEMANTIC_LAYERS"): return self.response_404() if get_user_id() is None: From 0d0d7a3a539fb0840928332078bd2b4291c2ea83 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Fri, 30 Oct 2026 17:03:56 -0400 Subject: [PATCH 12/13] test(semantic-layer): update user-scoped values cache expectation --- tests/unit_tests/semantic_layers/execution_context_test.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/unit_tests/semantic_layers/execution_context_test.py b/tests/unit_tests/semantic_layers/execution_context_test.py index f0c6175ec80f..3b88c6bdb972 100644 --- a/tests/unit_tests/semantic_layers/execution_context_test.py +++ b/tests/unit_tests/semantic_layers/execution_context_test.py @@ -62,13 +62,13 @@ def execution_view(app: Any, mocker: MockerFixture) -> SemanticView: return view -def test_connection_credentials_metadata_keys_unchanged( +def test_connection_credentials_cache_keys( client: Any, full_api_access: None, execution_view: SemanticView, mocker: MockerFixture, ) -> None: - """Metadata keys stay stable while chart results gain user partitioning.""" + """Dimension values and chart results include the requesting user's partition.""" cache: MagicMock = mocker.patch("superset.datasource.api.cache_manager").data_cache cache.get.return_value = ["cached"] keys: list[str] = [] @@ -100,7 +100,8 @@ def test_connection_credentials_metadata_keys_unchanged( assert chart_key is not None keys.append(chart_key) assert keys == [ - "col_values:eb5d43a7322c2a03556fc53bc62d54fbbe0b3d26d561d18a0e385c7457abd822", + # Dimension values use the requesting user's OAuth cache partition. + "col_values:ef5f1012d43907f9aeed4b06bc9f847dfe2d6bf2117d7ecaf806d1a484df5bff", # Compatibility identity includes changed_on="None" for this view. "compatible:a2e526c4cd1db4f68fd34cc7dffe06cc4fbd029975f2147b4b11bb55179795b8", # Chart keys also include the requesting user's OAuth cache partition. From 61fde49f2f30d72bb3ea9535639f4d2c2a32f92c Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Sat, 31 Oct 2026 09:58:56 -0400 Subject: [PATCH 13/13] test(semantic-layer): cover OAuth action endpoints and failure paths --- tests/unit_tests/semantic_layers/api_test.py | 275 +++++++++++++++++- .../semantic_layers/registry_test.py | 26 ++ 2 files changed, 300 insertions(+), 1 deletion(-) diff --git a/tests/unit_tests/semantic_layers/api_test.py b/tests/unit_tests/semantic_layers/api_test.py index 86fb3a8c9961..a7d3df367fc9 100644 --- a/tests/unit_tests/semantic_layers/api_test.py +++ b/tests/unit_tests/semantic_layers/api_test.py @@ -50,7 +50,7 @@ ) from superset.constants import PASSWORD_MASK from superset.errors import ErrorLevel, SupersetError, SupersetErrorType -from superset.exceptions import SupersetSecurityException +from superset.exceptions import OAuth2RedirectError, SupersetSecurityException from superset.models.core import Database from superset.semantic_layers.api import ( _mask_configuration, @@ -66,6 +66,279 @@ ) +@pytest.fixture +def action_provider(mocker: MockerFixture) -> tuple[MagicMock, MagicMock]: + """Provide an accessible stored layer and its configuration action provider.""" + layer = MagicMock(type="personal", configuration='{"host":"provider"}') + provider = MagicMock() + provider.configuration_class.model_json_schema.return_value = { + "type": "object", + "properties": {}, + } + provider.execute_configuration_action.return_value = SemanticLayerActionResult( + "https://provider.example/authorize" + ) + provider.get_configuration_actions.return_value = [ + SemanticLayerAction( + id="connect", label="Connect", status="required", required=True + ) + ] + mocker.patch("superset.semantic_layers.api.get_user_id", return_value=42) + mocker.patch( + "superset.semantic_layers.api.SemanticLayerDAO.find_by_uuid", return_value=layer + ) + mocker.patch.dict( + "superset.semantic_layers.api.registry", {"personal": provider}, clear=True + ) + return layer, provider + + +@SEMANTIC_LAYERS_APP +@pytest.mark.parametrize("configuration", [{"host": "provider"}, '{"host":"provider"}']) +@pytest.mark.parametrize("execute", [False, True]) +def test_stored_layer_personal_actions( + client: FlaskClient, + full_api_access: None, + action_provider: tuple[MagicMock, MagicMock], + configuration: dict[str, str] | str, + execute: bool, +) -> None: + """Personal actions use authorized stored configuration and host completion URLs.""" + layer, provider = action_provider + layer.configuration = configuration + path = f"/api/v1/semantic_layer/{uuid_lib.uuid4()}/actions" + response = client.post(f"{path}/connect") if execute else client.get(path) + assert response.status_code == 200 + layer.raise_for_access.assert_called_once_with() + provider.configuration_class.model_validate.assert_called_once_with( + {"host": "provider"} + ) + parsed_config = provider.configuration_class.model_validate.return_value + if execute: + result = response.json["result"] + uuid_lib.UUID(result["tab_id"]) + assert result["redirect_url"] == "https://provider.example/authorize" + provider.execute_configuration_action.assert_called_once_with( + "connect", + parsed_config, + f"http://example.com/api/v1/semantic_layer/oauth2/complete/{result['tab_id']}", + ) + else: + assert response.json["result"] == [ + { + "id": "connect", + "label": "Connect", + "status": "required", + "required": True, + "message": None, + } + ] + provider.get_configuration_actions.assert_called_once_with(parsed_config) + + +@SEMANTIC_LAYERS_APP +@pytest.mark.parametrize("endpoint", ["actions", "actions/connect", "oauth2/connect"]) +@pytest.mark.parametrize( + "failure,status", + [ + ("disabled", 404), + ("anonymous", 401), + ("missing", 404), + ("forbidden", 403), + ("invalid_view", 400), + ("unknown_type", 400), + ], +) +def test_personal_actions_reject_inaccessible_layers( + client: FlaskClient, + full_api_access: None, + action_provider: tuple[MagicMock, MagicMock], + mocker: MockerFixture, + endpoint: str, + failure: str, + status: int, +) -> None: + """Action entry points check user and resource access before calling providers.""" + layer, provider = action_provider + if failure == "disabled": + mocker.patch( + "superset.semantic_layers.api.is_feature_enabled", return_value=False + ) + elif failure == "anonymous": + mocker.patch("superset.semantic_layers.api.get_user_id", return_value=None) + elif failure == "missing": + mocker.patch( + "superset.semantic_layers.api.SemanticLayerDAO.find_by_uuid", + return_value=None, + ) + elif failure == "forbidden": + layer.raise_for_access.side_effect = SupersetSecurityException( + SupersetError( + error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR, + message="Access denied", + level=ErrorLevel.ERROR, + ) + ) + elif failure == "unknown_type": + layer.type = "unregistered" + query = {"tab_id": str(uuid_lib.uuid4())} + if failure == "invalid_view": + query["view_uuid"] = "not-a-uuid" + path = f"/api/v1/semantic_layer/{uuid_lib.uuid4()}/{endpoint}" + response = ( + client.post(path, query_string=query) + if endpoint == "actions/connect" + else client.get(path, query_string=query) + ) + assert response.status_code == status + provider.get_configuration_actions.assert_not_called() + provider.execute_configuration_action.assert_not_called() + + +@SEMANTIC_LAYERS_APP +@pytest.mark.parametrize("tab_id", [None, "invalid"]) +def test_oauth2_start_rejects_invalid_tab_id( + client: FlaskClient, + full_api_access: None, + action_provider: tuple[MagicMock, MagicMock], + tab_id: str | None, +) -> None: + """OAuth redirects require a valid initiating tab correlation ID.""" + response = client.get( + f"/api/v1/semantic_layer/{uuid_lib.uuid4()}/oauth2/connect", + query_string={} if tab_id is None else {"tab_id": tab_id}, + ) + assert response.status_code == 400 + assert response.json["message"] == "Invalid OAuth2 tab ID" + action_provider[1].execute_configuration_action.assert_not_called() + + +@SEMANTIC_LAYERS_APP +@pytest.mark.parametrize( + "endpoint", ["actions/connect", "oauth2/connect", "type/personal/actions/connect"] +) +def test_action_provider_rejects_operation( + client: FlaskClient, + full_api_access: None, + action_provider: tuple[MagicMock, MagicMock], + endpoint: str, +) -> None: + """Invalid actions return 400; redirect errors keep provider details private.""" + action_provider[1].execute_configuration_action.side_effect = ValueError( + "private-detail" + ) + prefix = "" if endpoint.startswith("type/") else f"{uuid_lib.uuid4()}/" + path = f"/api/v1/semantic_layer/{prefix}{endpoint}" + response = ( + client.get(path, query_string={"tab_id": str(uuid_lib.uuid4())}) + if endpoint.startswith("oauth2") + else client.post(path, json={}) + ) + assert response.status_code == 400 + expected = ( + "Unable to start the connection" + if endpoint.startswith("oauth2") + else "private-detail" + ) + assert response.json["message"] == expected + + +@SEMANTIC_LAYERS_APP +@pytest.mark.parametrize( + "failure,status", + [ + ("unknown_type", 400), + ("configuration", 400), + ("missing", 404), + ("forbidden", 403), + ("type_mismatch", 400), + ("unmask", 400), + ], +) +def test_configuration_action_rejects_invalid_input( + client: FlaskClient, + full_api_access: None, + action_provider: tuple[MagicMock, MagicMock], + mocker: MockerFixture, + failure: str, + status: int, +) -> None: + """Editing actions check configuration and access before resolving secrets.""" + layer, provider = action_provider + body: dict[str, Any] = { + "configuration": {}, + "semantic_layer_uuid": str(uuid_lib.uuid4()), + } + sl_type = "personal" + if failure == "unknown_type": + sl_type = "unregistered" + elif failure == "configuration": + body["configuration"] = ["invalid"] + elif failure == "missing": + mocker.patch( + "superset.semantic_layers.api.SemanticLayerDAO.find_by_uuid", + return_value=None, + ) + elif failure == "forbidden": + layer.raise_for_access.side_effect = SupersetSecurityException( + SupersetError( + error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR, + message="Access denied", + level=ErrorLevel.ERROR, + ) + ) + elif failure == "type_mismatch": + layer.type = "another-provider" + else: + mocker.patch( + "superset.semantic_layers.api.unmask_stored_configuration", + side_effect=SemanticLayerInvalidError("Invalid stored configuration"), + ) + response = client.post( + f"/api/v1/semantic_layer/type/{sl_type}/actions/connect", json=body + ) + assert response.status_code == status + provider.execute_configuration_action.assert_not_called() + + +@SEMANTIC_LAYERS_APP +@pytest.mark.parametrize("disabled,status", [(True, 404), (False, 401)]) +def test_oauth2_completion_requires_enabled_feature_and_user( + client: FlaskClient, + full_api_access: None, + mocker: MockerFixture, + disabled: bool, + status: int, +) -> None: + """Completion pages enforce the feature flag and authenticated principal.""" + mocker.patch( + "superset.semantic_layers.api.is_feature_enabled", return_value=not disabled + ) + mocker.patch("superset.semantic_layers.api.get_user_id", return_value=None) + response = client.get(f"/api/v1/semantic_layer/oauth2/complete/{uuid_lib.uuid4()}") + assert response.status_code == status + + +@SEMANTIC_LAYERS_APP +@pytest.mark.parametrize("method", ["get_dimensions", "get_metrics"]) +def test_semantic_view_structure_preserves_oauth_redirect( + client: FlaskClient, + full_api_access: None, + mocker: MockerFixture, + method: str, +) -> None: + """Structure failures retain the OAuth contract needed to authorize from Explore.""" + view = MagicMock() + error = OAuth2RedirectError("/connect", str(uuid_lib.uuid4()), "/complete") + getattr(view.implementation, method).side_effect = error + session = mocker.patch("superset.semantic_layers.api.db.session") + session.query.return_value.filter_by.return_value.first.return_value = view + response = client.get("/api/v1/semantic_view/1/structure") + assert response.status_code == 403 + assert response.json["errors"][0]["error_type"] == "OAUTH2_REDIRECT" + assert response.json["errors"][0]["extra"] == error.error.extra + + @SEMANTIC_LAYERS_APP def test_put_semantic_view( client: Any, diff --git a/tests/unit_tests/semantic_layers/registry_test.py b/tests/unit_tests/semantic_layers/registry_test.py index 948c9b435b1d..b77804926678 100644 --- a/tests/unit_tests/semantic_layers/registry_test.py +++ b/tests/unit_tests/semantic_layers/registry_test.py @@ -19,6 +19,7 @@ from __future__ import annotations +from collections.abc import Callable from unittest.mock import MagicMock from urllib.parse import parse_qs, urlparse @@ -28,8 +29,10 @@ from superset.exceptions import OAuth2RedirectError from superset.extensions.context import get_context +from superset.semantic_layers.oauth2 import check_for_oauth2 from superset.semantic_layers.registry import ( contextualize_semantic_layer, + contextualize_semantic_view, ContextualSemanticLayer, ContextualSemanticView, register_semantic_layer, @@ -199,5 +202,28 @@ def __eq__(self, other: object) -> bool: assert len(first) == 2 assert first == second assert len(first | second) == 2 + for view in first: + assert view == ProviderView(view.name) + assert view != ProviderView("unrelated") finally: unregister_semantic_layer(provider_id) + + +@pytest.mark.parametrize( + "contextualize", [contextualize_semantic_layer, contextualize_semantic_view] +) +def test_unbound_host_provider_preserves_identity( + contextualize: Callable[..., object], +) -> None: + """Host providers without a persisted layer need no context proxy.""" + implementation = MagicMock() + assert contextualize("host-provider", implementation) is implementation + + +def test_unpersisted_layer_preserves_authentication_error() -> None: + """An unpersisted layer cannot supply a browser action URL.""" + error = SemanticLayerAuthenticationRequiredError("connect") + with pytest.raises(SemanticLayerAuthenticationRequiredError) as raised: + with check_for_oauth2(None): + raise error + assert raised.value is error