diff --git a/api/services/app_dsl_service.py b/api/services/app_dsl_service.py index 87998b7a0a5..de501b83ca2 100644 --- a/api/services/app_dsl_service.py +++ b/api/services/app_dsl_service.py @@ -244,8 +244,11 @@ class AppDslService: # If major version mismatch, store import info in Redis if status == ImportStatus.PENDING: + tenant_id = account.current_tenant_id + if tenant_id is None: + raise ValueError("Current tenant is not set") pending_data = PendingData( - tenant_id=account.current_tenant_id, + tenant_id=tenant_id, account_id=account.id, import_mode=import_mode, yaml_content=content, diff --git a/api/services/entities/dsl_entities.py b/api/services/entities/dsl_entities.py index 2bff3fcac24..67bada2207e 100644 --- a/api/services/entities/dsl_entities.py +++ b/api/services/entities/dsl_entities.py @@ -1,7 +1,7 @@ from enum import StrEnum from typing import Any -from pydantic import BaseModel, Field +from pydantic import BaseModel, ConfigDict, Field from core.plugin.entities.plugin import PluginDependency @@ -19,15 +19,13 @@ class ImportStatus(StrEnum): class PendingImportOwner(BaseModel): - tenant_id: str | None = None - account_id: str | None = None + model_config = ConfigDict(hide_input_in_errors=True) + + tenant_id: str + account_id: str def is_accessible_by(self, *, tenant_id: str | None, account_id: str) -> bool: - if tenant_id is None: - return False - owner = (self.tenant_id, self.account_id) - # Ownerless payloads come from older pods and expire after 10 minutes; #40106 removes this bridge. - return owner in ((None, None), (tenant_id, account_id)) + return tenant_id is not None and (self.tenant_id, self.account_id) == (tenant_id, account_id) class DslImportWarning(BaseModel): diff --git a/api/services/rag_pipeline/rag_pipeline_dsl_service.py b/api/services/rag_pipeline/rag_pipeline_dsl_service.py index 6977c28b7a6..6ee64efc9d5 100644 --- a/api/services/rag_pipeline/rag_pipeline_dsl_service.py +++ b/api/services/rag_pipeline/rag_pipeline_dsl_service.py @@ -221,8 +221,11 @@ class RagPipelineDslService: # If major version mismatch, store import info in Redis if status == ImportStatus.PENDING: + tenant_id = account.current_tenant_id + if tenant_id is None: + raise ValueError("Current tenant is not set") pending_data = RagPipelinePendingData( - tenant_id=account.current_tenant_id, + tenant_id=tenant_id, account_id=account.id, import_mode=import_mode, yaml_content=content, diff --git a/api/services/snippet_dsl_service.py b/api/services/snippet_dsl_service.py index 78361700ad4..a6e03021d55 100644 --- a/api/services/snippet_dsl_service.py +++ b/api/services/snippet_dsl_service.py @@ -235,8 +235,11 @@ class SnippetDslService: # If major version mismatch, store import info in Redis if status == ImportStatus.PENDING: + tenant_id = account.current_tenant_id + if tenant_id is None: + raise ValueError("Current tenant is not set") pending_data = SnippetPendingData( - tenant_id=account.current_tenant_id, + tenant_id=tenant_id, account_id=account.id, import_mode=import_mode, yaml_content=content, diff --git a/api/tests/test_containers_integration_tests/services/test_app_dsl_service.py b/api/tests/test_containers_integration_tests/services/test_app_dsl_service.py index eea8ab9c2a8..05ea4111378 100644 --- a/api/tests/test_containers_integration_tests/services/test_app_dsl_service.py +++ b/api/tests/test_containers_integration_tests/services/test_app_dsl_service.py @@ -600,6 +600,8 @@ class TestAppDslService: redis_key = f"{IMPORT_INFO_REDIS_KEY_PREFIX}{import_id}" pending = PendingData( + tenant_id=_DEFAULT_TENANT_ID, + account_id=_DEFAULT_ACCOUNT_ID, import_mode=ImportMode.YAML_CONTENT, yaml_content=_workflow_yaml(), name="name", @@ -609,11 +611,7 @@ class TestAppDslService: icon_background="#fff", app_id=None, ) - redis_client.setex( - redis_key, - IMPORT_INFO_REDIS_EXPIRY, - pending.model_dump_json(exclude={"tenant_id", "account_id"}), - ) + redis_client.setex(redis_key, IMPORT_INFO_REDIS_EXPIRY, pending.model_dump_json()) created_app = SimpleNamespace( id=str(uuid4()), diff --git a/api/tests/unit_tests/controllers/console/app/test_app_import_api.py b/api/tests/unit_tests/controllers/console/app/test_app_import_api.py index c2d6a756062..96739b27060 100644 --- a/api/tests/unit_tests/controllers/console/app/test_app_import_api.py +++ b/api/tests/unit_tests/controllers/console/app/test_app_import_api.py @@ -343,7 +343,8 @@ class TestAppImportConfirmApi: ) redis_get = MagicMock( return_value=( - b'{"import_mode":"yaml-content","yaml_content":"app: {}","app_id":null,' + b'{"tenant_id":"tenant-1","account_id":"u1","import_mode":"yaml-content",' + b'"yaml_content":"app: {}","app_id":null,' b'"name":null,"description":null,"icon_type":null,"icon":null,"icon_background":null}' ) ) @@ -388,7 +389,8 @@ class TestAppImportConfirmApi: app_import_module.redis_client, "get", lambda *_args, **_kwargs: ( - b'{"import_mode":"yaml-content","yaml_content":"app: {}","app_id":"existing-app",' + b'{"tenant_id":"tenant-1","account_id":"u1","import_mode":"yaml-content",' + b'"yaml_content":"app: {}","app_id":"existing-app",' b'"name":null,"description":null,"icon_type":null,"icon":null,"icon_background":null}' ), ) diff --git a/api/tests/unit_tests/services/rag_pipeline/test_rag_pipeline_dsl_service.py b/api/tests/unit_tests/services/rag_pipeline/test_rag_pipeline_dsl_service.py index f7f8e377f41..75a45c8436c 100644 --- a/api/tests/unit_tests/services/rag_pipeline/test_rag_pipeline_dsl_service.py +++ b/api/tests/unit_tests/services/rag_pipeline/test_rag_pipeline_dsl_service.py @@ -643,6 +643,15 @@ def test_confirm_import_rejects_non_serialized_pending_data( def test_import_pending_version_stores_redis(monkeypatch: pytest.MonkeyPatch, service: RagPipelineDslService) -> None: setex = Mock() monkeypatch.setattr(module.redis_client, "setex", setex) + missing_tenant = service.import_rag_pipeline( + account=Mock(id="account-1", current_tenant_id=None), + import_mode=ImportMode.YAML_CONTENT.value, + yaml_content=_valid_dsl(version="1.0.0"), + ) + assert missing_tenant.status == ImportStatus.FAILED + assert missing_tenant.error == "Current tenant is not set" + setex.assert_not_called() + result = service.import_rag_pipeline( account=_account(), import_mode=ImportMode.YAML_CONTENT.value, yaml_content=_valid_dsl(version="1.0.0") ) @@ -813,14 +822,27 @@ def test_confirm_import_updates_tenant_pipeline_and_dataset( pipeline_id=pipeline.id, ) redis_key = "app_import_info:import-1" + pending_json = pending.model_dump_json(exclude={"tenant_id", "account_id"}) monkeypatch.setattr( module.redis_client, "get", - Mock(side_effect=lambda key: pending.model_dump_json() if key == redis_key else None), + Mock(side_effect=lambda key: pending_json if key == redis_key else None), ) delete = Mock() monkeypatch.setattr(module.redis_client, "delete", delete) + load = Mock(wraps=module.yaml.safe_load) + monkeypatch.setattr(module.yaml, "safe_load", load) + create_or_update = Mock(wraps=service._create_or_update_pipeline) + monkeypatch.setattr(service, "_create_or_update_pipeline", create_or_update) monkeypatch.setattr(module.KnowledgeConfiguration, "model_validate", Mock(return_value=_knowledge_configuration())) + + assert service.confirm_import(import_id="import-1", account=_account()).status == ImportStatus.FAILED + load.assert_not_called() + create_or_update.assert_not_called() + delete.assert_not_called() + assert pipeline.name == "Pipeline" + pending_json = pending.model_dump_json() + for foreign_account in (_account(tenant_id="tenant-2"), _account(account_id="account-2")): assert service.confirm_import(import_id="import-1", account=foreign_account).status == ImportStatus.FAILED delete.assert_not_called() diff --git a/api/tests/unit_tests/services/test_app_dsl_service.py b/api/tests/unit_tests/services/test_app_dsl_service.py index af4c7c1ee7d..da7efbe5acf 100644 --- a/api/tests/unit_tests/services/test_app_dsl_service.py +++ b/api/tests/unit_tests/services/test_app_dsl_service.py @@ -348,11 +348,17 @@ def test_pending_import_is_scoped_to_its_owner(monkeypatch: pytest.MonkeyPatch, monkeypatch.setattr("services.app_dsl_service.redis_client.get", pending_imports.get) monkeypatch.setattr("services.app_dsl_service.redis_client.delete", pending_imports.pop) - monkeypatch.setattr( - service, - "_create_or_update_app", - Mock(return_value=_app(app_id="app-1", mode=AppMode.WORKFLOW)), - ) + create_or_update = Mock(return_value=_app(app_id="app-1", mode=AppMode.WORKFLOW)) + monkeypatch.setattr(service, "_create_or_update_app", create_or_update) + load = Mock(wraps=yaml.safe_load) + monkeypatch.setattr("services.app_dsl_service.yaml.safe_load", load) + + pending_imports[redis_key] = pending_data.model_dump_json(exclude={"tenant_id", "account_id"}) + assert service.confirm_import(import_id=pending.id, account=creator).status == ImportStatus.FAILED + load.assert_not_called() + create_or_update.assert_not_called() + assert redis_key in pending_imports + pending_imports[redis_key] = pending_data.model_dump_json() for other_account in ( _account(tenant_id="tenant-2"), @@ -364,36 +370,60 @@ def test_pending_import_is_scoped_to_its_owner(monkeypatch: pytest.MonkeyPatch, assert redis_key not in pending_imports +def test_pending_import_requires_current_tenant(monkeypatch: pytest.MonkeyPatch, unbound_session: Session) -> None: + setex = Mock() + monkeypatch.setattr("services.app_dsl_service.redis_client.setex", setex) + account = _account() + account._current_tenant = None + + result = AppDslService(session=unbound_session).import_app( + account=account, + import_mode="yaml-content", + yaml_content="version: 99.0.0\nkind: app\napp: {name: Test, mode: workflow}\n", + ) + + assert result.status == ImportStatus.FAILED + assert result.error == "Current tenant is not set" + setex.assert_not_called() + + @pytest.mark.parametrize( - ("tenant_id", "account_id", "expected"), + ("caller_tenant_id", "caller_account_id", "expected"), [ ("tenant-1", "account-1", True), (None, "account-1", False), - ("tenant-1", None, False), ("tenant-2", "account-1", False), ("tenant-1", "account-2", False), ], ) def test_pending_import_owner_access( - tenant_id: str | None, - account_id: str | None, + caller_tenant_id: str | None, + caller_account_id: str, expected: bool, ) -> None: pending = PendingData( - tenant_id=tenant_id, - account_id=account_id, + tenant_id="tenant-1", + account_id="account-1", import_mode="yaml-content", yaml_content="", ) - assert pending.is_accessible_by(tenant_id="tenant-1", account_id="account-1") is expected + assert pending.is_accessible_by(tenant_id=caller_tenant_id, account_id=caller_account_id) is expected -def test_pending_import_owner_access_accepts_legacy_json() -> None: - pending = PendingData.model_validate_json('{"import_mode":"yaml-content","yaml_content":""}') +@pytest.mark.parametrize( + "payload", + [ + '{"import_mode":"yaml-content","yaml_content":"secret-token-123"}', + '{"tenant_id":"tenant-1","import_mode":"yaml-content","yaml_content":"secret-token-123"}', + '{"account_id":"account-1","import_mode":"yaml-content","yaml_content":"secret-token-123"}', + ], +) +def test_pending_import_owner_is_required(payload: str) -> None: + with pytest.raises(ValueError) as exc_info: + PendingData.model_validate_json(payload) - assert pending.is_accessible_by(tenant_id="tenant-1", account_id="account-1") - assert not pending.is_accessible_by(tenant_id=None, account_id="account-1") + assert "secret-token-123" not in str(exc_info.value) def test_create_or_update_app_loads_existing_model_config_with_service_session( diff --git a/api/tests/unit_tests/services/test_snippet_dsl_service.py b/api/tests/unit_tests/services/test_snippet_dsl_service.py index b1318b1edce..900f321c43b 100644 --- a/api/tests/unit_tests/services/test_snippet_dsl_service.py +++ b/api/tests/unit_tests/services/test_snippet_dsl_service.py @@ -3,6 +3,7 @@ from types import SimpleNamespace from unittest.mock import Mock import pytest +import yaml from sqlalchemy import event from sqlalchemy.orm import Session @@ -318,6 +319,17 @@ workflow: edges: [] """ + account_without_tenant = _account() + account_without_tenant._current_tenant = None + missing_tenant = service.import_snippet( + account=account_without_tenant, + import_mode=ImportMode.YAML_CONTENT.value, + yaml_content=yaml_content, + ) + assert missing_tenant.status == ImportStatus.FAILED + assert missing_tenant.error == "Current tenant is not set" + setex.assert_not_called() + result = service.import_snippet( account=_account(), import_mode=ImportMode.YAML_CONTENT.value, @@ -459,12 +471,21 @@ workflow: create_or_update = Mock(return_value=snippet) monkeypatch.setattr(service, "_create_or_update_snippet", create_or_update) redis_key = "snippet_import_info:import-1" + pending_json = pending.model_dump_json(exclude={"tenant_id", "account_id"}) monkeypatch.setattr( "services.snippet_dsl_service.redis_client.get", - Mock(side_effect=lambda key: pending.model_dump_json() if key == redis_key else None), + Mock(side_effect=lambda key: pending_json if key == redis_key else None), ) redis_delete = Mock() monkeypatch.setattr("services.snippet_dsl_service.redis_client.delete", redis_delete) + load = Mock(wraps=yaml.safe_load) + monkeypatch.setattr("services.snippet_dsl_service.yaml.safe_load", load) + + assert service.confirm_import(import_id="import-1", account=account).status == ImportStatus.FAILED + load.assert_not_called() + create_or_update.assert_not_called() + redis_delete.assert_not_called() + pending_json = pending.model_dump_json() for other_account in ( _account(tenant_id="tenant-2"), @@ -491,6 +512,8 @@ def test_confirm_import_returns_failed_for_non_mapping_yaml( service: SnippetDslService, monkeypatch: pytest.MonkeyPatch ): pending = SnippetPendingData( + tenant_id="tenant-1", + account_id="account-1", import_mode="yaml-content", yaml_content="- item", snippet_id=None, @@ -509,6 +532,8 @@ def test_confirm_import_returns_failed_when_create_or_update_raises( rollback_events: list[str] = [] event.listen(sqlite_session, "after_rollback", lambda _session: rollback_events.append("rollback")) pending = SnippetPendingData( + tenant_id="tenant-1", + account_id="account-1", import_mode="yaml-content", yaml_content="version: 0.1.0\nkind: snippet\nsnippet:\n name: Bad\n", snippet_id="snippet-1",