From 5be7b4357b41211f7205a592f08da3d70d993982 Mon Sep 17 00:00:00 2001 From: Athul Date: Thu, 16 Jul 2026 14:01:37 +0530 Subject: [PATCH 1/5] UN-3739 [FIX] Validate requester, not just profile creator, in owner-access check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prompt Studio's validate_profile_manager_owner_access() only checked the profile creator's adapter access — the requesting user was never passed in. Org admins were denied on any project whose profile creator lost adapter access (share revoked, member offboarded, or the profile was created by a service account via the platform API), with an error that blamed the requester. - Plumb request_user_id into the check from all call sites (including the single-pass and summarize paths, which previously dropped user_id) - Pass when the requester is an org admin (implicit access) - Pass when the profile creator is a service account (platform-API / org-migration projects hold no adapter shares by design) - Keep the revocation guard: non-admin requesters are still denied when the creator lacks adapter access (delegated use unchanged) - Error message now names the profile creator who lacks access (and flags offboarded creators) instead of misattributing to "You"; also fixes the message accidentally being raised as a 1-tuple Co-Authored-By: Claude Fable 5 --- .../prompt_studio_helper.py | 218 ++++++++++------- .../tests/test_build_index_payload.py | 36 ++- ...t_validate_profile_manager_owner_access.py | 230 ++++++++++++++++++ 3 files changed, 386 insertions(+), 98 deletions(-) create mode 100644 backend/prompt_studio/prompt_studio_core_v2/tests/test_validate_profile_manager_owner_access.py diff --git a/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py b/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py index 91515db118..9bc4b9e353 100644 --- a/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py +++ b/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py @@ -183,105 +183,111 @@ def validate_adapter_status( if not adapter.is_usable: raise PermissionError(error_msg) + @staticmethod + def _is_org_admin_by_user_id(user_id: str | None) -> bool: + """Return True if ``user_id`` resolves to an org member with admin role.""" + if not user_id: + return False + member = OrganizationMemberService.get_user_by_user_id(user_id) + return bool( + member and OrganizationMemberService.is_user_organization_admin(member.user) + ) + + @staticmethod + def _user_has_adapter_access(user: Any, adapter: Any) -> bool: + return ( + adapter.shared_to_org + or adapter.created_by == user + or adapter.shared_users.filter(pk=user.pk).exists() + or has_group_access(user, adapter) + ) + @staticmethod def validate_profile_manager_owner_access( profile_manager: ProfileManager, + request_user_id: str | None = None, ) -> None: - """Helper method to validate the owner's access to the profile manager. + """Validate adapter access for a profile before using its adapters. + + This is a revocation guard on the profile *creator*, not a + requester ACL: users with project access deliberately piggyback on + the creator's adapter access. Org admins and service-account + creators bypass it — admins have implicit access to all adapters, + and platform-API/org-migration profiles are created by service + accounts that hold no adapter shares by design (UN-3739). Args: - profile_manager (ProfileManager): The profile manager instance to - validate. + profile_manager: The profile whose adapters will be used. + request_user_id: ``user_id`` of the user triggering the action. Raises: - PermissionError: If the owner does not have permission to perform - the action. + PermissionError: If the profile creator no longer has access to + one or more adapters and no bypass applies. """ - profile_manager_owner = profile_manager.created_by - if profile_manager_owner is None: + if PromptStudioHelper._is_org_admin_by_user_id(request_user_id): return - if OrganizationMemberService.is_user_organization_admin(profile_manager_owner): + owner = profile_manager.created_by + if owner is None: return - is_llm_owned = ( - profile_manager.llm.shared_to_org - or profile_manager.llm.created_by == profile_manager_owner - or profile_manager.llm.shared_users.filter( - pk=profile_manager_owner.pk - ).exists() - or has_group_access(profile_manager_owner, profile_manager.llm) - ) - is_vector_store_owned = ( - profile_manager.vector_store.shared_to_org - or profile_manager.vector_store.created_by == profile_manager_owner - or profile_manager.vector_store.shared_users.filter( - pk=profile_manager_owner.pk - ).exists() - or has_group_access(profile_manager_owner, profile_manager.vector_store) - ) - is_embedding_model_owned = ( - profile_manager.embedding_model.shared_to_org - or profile_manager.embedding_model.created_by == profile_manager_owner - or profile_manager.embedding_model.shared_users.filter( - pk=profile_manager_owner.pk - ).exists() - or has_group_access(profile_manager_owner, profile_manager.embedding_model) - ) - is_x2text_owned = ( - profile_manager.x2text.shared_to_org - or profile_manager.x2text.created_by == profile_manager_owner - or profile_manager.x2text.shared_users.filter( - pk=profile_manager_owner.pk - ).exists() - or has_group_access(profile_manager_owner, profile_manager.x2text) - ) - - if not ( - is_llm_owned - and is_vector_store_owned - and is_embedding_model_owned - and is_x2text_owned - ): - adapter_names = set() - if not is_llm_owned: - logger.error( - ERROR_MSG, - profile_manager_owner.user_id, - profile_manager.llm.id, - ) - adapter_names.add(profile_manager.llm.adapter_name) - if not is_vector_store_owned: - logger.error( - ERROR_MSG, - profile_manager_owner.user_id, - profile_manager.vector_store.id, - ) - adapter_names.add(profile_manager.vector_store.adapter_name) - if not is_embedding_model_owned: - logger.error( - ERROR_MSG, - profile_manager_owner.user_id, - profile_manager.embedding_model.id, - ) - adapter_names.add(profile_manager.embedding_model.adapter_name) - if not is_x2text_owned: - logger.error( - ERROR_MSG, - profile_manager_owner.user_id, - profile_manager.x2text.id, - ) - adapter_names.add(profile_manager.x2text.adapter_name) - if len(adapter_names) > 1: - error_msg = ( - f"Multiple permission errors were encountered with {', '.join(adapter_names)}", # noqa: E501 - ) - else: - error_msg = ( - f"Permission Error: You do not have access to {adapter_names.pop()}", # noqa: E501 - ) + if getattr(owner, "is_service_account", False): + return - raise PermissionError(error_msg) + if OrganizationMemberService.is_user_organization_admin(owner): + return + + adapters = [ + profile_manager.llm, + profile_manager.vector_store, + profile_manager.embedding_model, + profile_manager.x2text, + ] + denied = [ + adapter + for adapter in adapters + if not PromptStudioHelper._user_has_adapter_access(owner, adapter) + ] + if not denied: + return + + for adapter in denied: + logger.error(ERROR_MSG, owner.user_id, adapter.id) + logger.error( + "Adapter access denied for profile '%s': creator %s lacks access," + " requester %s", + profile_manager.profile_name, + owner.user_id, + request_user_id, + ) + + denied_names = ", ".join(adapter.adapter_name for adapter in denied) + profile_ref = ( + f"This project's LLM profile '{profile_manager.profile_name}' was" + f" created by {owner.email}" + ) + if not OrganizationMemberService.get_user_by_id(owner.id): + error_msg = ( + f"Permission Error: {profile_ref}, who is no longer a member of" + f" this organization. Recreate the default profile, or ask an" + f" admin to share these adapters with everyone: {denied_names}." + ) + elif len(denied) > 1: + error_msg = ( + f"Permission Error: {profile_ref}, who no longer has access to" + f" these adapters: {denied_names}. Re-share them with the" + f" creator, share them with everyone, or recreate the profile" + f" using adapters you have access to." + ) + else: + error_msg = ( + f"Permission Error: {profile_ref}, who no longer has access to" + f" the adapter '{denied_names}'. Re-share the adapter with" + f" them, share it with everyone, or recreate the profile using" + f" adapters you have access to." + ) + + raise PermissionError(error_msg) @staticmethod def _publish_log( @@ -321,6 +327,7 @@ def _build_summarize_params( stem: str, extract_file_path: str, platform_api_key: str, + request_user_id: str | None = None, ) -> tuple[dict[str, Any] | None, str, "ProfileManager"]: """Build summarize_params dict if summarization is enabled. @@ -344,7 +351,9 @@ def _build_summarize_params( if summary_profile != default_profile: PromptStudioHelper.validate_adapter_status(summary_profile) - PromptStudioHelper.validate_profile_manager_owner_access(summary_profile) + PromptStudioHelper.validate_profile_manager_owner_access( + summary_profile, request_user_id=request_user_id + ) llm_adapter_id = ( str(tool.summarize_llm_adapter.id) @@ -508,7 +517,9 @@ def build_index_payload( raise DefaultProfileError() PromptStudioHelper.validate_adapter_status(default_profile) - PromptStudioHelper.validate_profile_manager_owner_access(default_profile) + PromptStudioHelper.validate_profile_manager_owner_access( + default_profile, request_user_id=user_id + ) # Common path decomposition used by extract, summarize, and index directory, filename = os.path.split(file_path) @@ -525,6 +536,7 @@ def build_index_payload( stem, extract_file_path, platform_api_key, + request_user_id=user_id, ) ) @@ -727,7 +739,9 @@ def build_fetch_response_payload( monitor_llm, challenge_llm = PromptStudioHelper._resolve_llm_ids(tool) PromptStudioHelper.validate_adapter_status(profile_manager) - PromptStudioHelper.validate_profile_manager_owner_access(profile_manager) + PromptStudioHelper.validate_profile_manager_owner_access( + profile_manager, request_user_id=user_id + ) vector_db = str(profile_manager.vector_store.id) embedding_model = str(profile_manager.embedding_model.id) @@ -945,7 +959,9 @@ def build_bulk_fetch_response_payload( raise DefaultProfileError() PromptStudioHelper.validate_adapter_status(profile_manager) - PromptStudioHelper.validate_profile_manager_owner_access(profile_manager) + PromptStudioHelper.validate_profile_manager_owner_access( + profile_manager, request_user_id=user_id + ) monitor_llm, challenge_llm = PromptStudioHelper._resolve_llm_ids(tool) @@ -1136,7 +1152,9 @@ def build_single_pass_payload( challenge_llm = str(default_profile.llm.id) PromptStudioHelper.validate_adapter_status(default_profile) - PromptStudioHelper.validate_profile_manager_owner_access(default_profile) + PromptStudioHelper.validate_profile_manager_owner_access( + default_profile, request_user_id=user_id + ) default_profile.chunk_size = 0 if prompt_grammar: @@ -1373,12 +1391,16 @@ def index_document( PromptStudioHelper.validate_adapter_status(default_profile) # Need to check the user who created profile manager # has access to adapters configured in profile manager - PromptStudioHelper.validate_profile_manager_owner_access(default_profile) + PromptStudioHelper.validate_profile_manager_owner_access( + default_profile, request_user_id=user_id + ) # Also validate summary profile if it's different from default if tool.summarize_context and summary_profile != default_profile: PromptStudioHelper.validate_adapter_status(summary_profile) - PromptStudioHelper.validate_profile_manager_owner_access(summary_profile) + PromptStudioHelper.validate_profile_manager_owner_access( + summary_profile, request_user_id=user_id + ) fs_instance = EnvHelper.get_storage( storage_type=StorageType.PERMANENT, @@ -1550,6 +1572,7 @@ def prompt_responder( org_id=org_id, document_id=document_id, run_id=run_id, + user_id=user_id, ) @staticmethod @@ -1668,6 +1691,7 @@ def _execute_prompts_in_single_pass( org_id, document_id, run_id, + user_id=None, ): prompts = PromptStudioHelper.fetch_prompt_from_tool(tool_id) prompts = [ @@ -1698,6 +1722,7 @@ def _execute_prompts_in_single_pass( org_id=org_id, document_id=document_id, run_id=run_id, + user_id=user_id, ) return PromptStudioHelper._handle_response( response=response, @@ -1837,7 +1862,9 @@ def _fetch_response( PromptStudioHelper.validate_adapter_status(profile_manager) # Need to check the user who created profile manager # has access to adapters - PromptStudioHelper.validate_profile_manager_owner_access(profile_manager) + PromptStudioHelper.validate_profile_manager_owner_access( + profile_manager, request_user_id=user_id + ) # Not checking reindex here as there might be # change in Profile Manager vector_db = str(profile_manager.vector_store.id) @@ -2218,6 +2245,7 @@ def _fetch_single_pass_response( org_id: str, document_id: str, run_id: str = None, + user_id: str | None = None, ) -> Any: tool_id: str = str(tool.tool_id) outputs: list[dict[str, Any]] = [] @@ -2237,7 +2265,9 @@ def _fetch_single_pass_response( # Need to check the user who created profile manager PromptStudioHelper.validate_adapter_status(default_profile) # has access to adapters configured in profile manager - PromptStudioHelper.validate_profile_manager_owner_access(default_profile) + PromptStudioHelper.validate_profile_manager_owner_access( + default_profile, request_user_id=user_id + ) default_profile.chunk_size = 0 # To retrive full context if prompt_grammar: for word, synonyms in prompt_grammar.items(): diff --git a/backend/prompt_studio/prompt_studio_core_v2/tests/test_build_index_payload.py b/backend/prompt_studio/prompt_studio_core_v2/tests/test_build_index_payload.py index d82156b140..a7aea0118e 100644 --- a/backend/prompt_studio/prompt_studio_core_v2/tests/test_build_index_payload.py +++ b/backend/prompt_studio/prompt_studio_core_v2/tests/test_build_index_payload.py @@ -55,6 +55,7 @@ def _dispatch_build( read_return: str | Exception, tool: Any = None, profile: Any = None, + validate_owner_mock: Any = None, ): """Run ``build_index_payload`` with all collaborators patched. @@ -69,6 +70,7 @@ def _dispatch_build( """ tool = tool or _make_tool() profile = profile or _make_profile() + validate_owner_mock = validate_owner_mock or MagicMock(return_value=None) fs_instance = MagicMock(name="fs_instance") if isinstance(read_return, Exception): @@ -90,14 +92,22 @@ def _dispatch_build( "get_or_create_prompt_studio_subdirectory", MagicMock(return_value="/prompt-studio/org/user/tool"), ), - (_psh_mod.ProfileManager, "get_default_llm_profile", MagicMock(return_value=profile)), + ( + _psh_mod.ProfileManager, + "get_default_llm_profile", + MagicMock(return_value=profile), + ), (PromptStudioHelper, "validate_adapter_status", MagicMock(return_value=None)), ( PromptStudioHelper, "validate_profile_manager_owner_access", - MagicMock(return_value=None), + validate_owner_mock, + ), + ( + PromptStudioHelper, + "_get_platform_api_key", + MagicMock(return_value="pk-test"), ), - (PromptStudioHelper, "_get_platform_api_key", MagicMock(return_value="pk-test")), ( PromptStudioHelper, "_build_summarize_params", @@ -105,7 +115,11 @@ def _dispatch_build( ), (_psh_mod.EnvHelper, "get_storage", MagicMock(return_value=fs_instance)), (_psh_mod.PromptStudioIndexHelper, "check_extraction_status", check_mock), - (_psh_mod.IndexingUtils, "generate_index_key", MagicMock(return_value="doc-key-1")), + ( + _psh_mod.IndexingUtils, + "generate_index_key", + MagicMock(return_value="doc-key-1"), + ), (_psh_mod, "PromptIdeBaseTool", MagicMock(return_value=MagicMock())), (_psh_mod.StateStore, "get", MagicMock(return_value="")), ): @@ -173,3 +187,17 @@ def test_check_extraction_status_raises_is_swallowed(self, caplog) -> None: "falling back to full extraction" in rec.getMessage() for rec in caplog.records ) + + +class TestOwnerAccessPlumbing: + """UN-3739: the requesting user must reach the owner-access check.""" + + def test_owner_access_receives_requesting_user(self) -> None: + validate_owner_mock = MagicMock(return_value=None) + _dispatch_build( + check_return=True, + read_return="extracted text", + validate_owner_mock=validate_owner_mock, + ) + _args, kwargs = validate_owner_mock.call_args + assert kwargs.get("request_user_id") == "user-1" diff --git a/backend/prompt_studio/prompt_studio_core_v2/tests/test_validate_profile_manager_owner_access.py b/backend/prompt_studio/prompt_studio_core_v2/tests/test_validate_profile_manager_owner_access.py new file mode 100644 index 0000000000..9ed3e3458a --- /dev/null +++ b/backend/prompt_studio/prompt_studio_core_v2/tests/test_validate_profile_manager_owner_access.py @@ -0,0 +1,230 @@ +"""Tests for ``PromptStudioHelper.validate_profile_manager_owner_access``. + +UN-3739: the check used to validate only the profile *creator*'s adapter +access — the requesting user was never passed in, so org admins were +denied on any project whose profile creator lost adapter access (share +revoked, member offboarded, or profile created by a service account via +the platform API). + +Contract pinned here (evaluation order): + + 1. Requester is an org admin → pass (implicit access) + 2. ``created_by`` is None → pass (unchanged) + 3. Creator is a service account → pass (platform-API projects) + 4. Creator is an org admin → pass (unchanged) + 5. Creator has access to all 4 adapters → pass (delegated use kept) + 6. Otherwise → PermissionError naming the + CREATOR (not "You"), with remediation, as a plain string. + +Unit tests: the real helper module is imported and collaborators are +patched per-test, so no database is touched. +""" + +from __future__ import annotations + +from contextlib import ExitStack +from unittest.mock import MagicMock, patch + +import pytest + +from prompt_studio.prompt_studio_core_v2 import prompt_studio_helper as _psh_mod +from prompt_studio.prompt_studio_core_v2.exceptions import ( + PermissionError as PSPermissionError, +) + +PromptStudioHelper = _psh_mod.PromptStudioHelper + + +def _user(user_id: str, email: str, *, service_account: bool = False) -> MagicMock: + user = MagicMock(name=f"user-{user_id}") + user.id = f"pk-{user_id}" + user.user_id = user_id + user.email = email + user.is_service_account = service_account + return user + + +def _adapter(name: str, created_by: MagicMock) -> MagicMock: + adapter = MagicMock(name=f"adapter-{name}") + adapter.adapter_name = name + adapter.created_by = created_by + adapter.shared_to_org = False + adapter.shared_users.filter.return_value.exists.return_value = False + return adapter + + +def _profile(creator: MagicMock | None, adapter_owner: MagicMock) -> MagicMock: + """Profile whose 4 adapters are all owned by ``adapter_owner``.""" + profile = MagicMock(name="ProfileManager") + profile.profile_name = "Default" + profile.created_by = creator + profile.llm = _adapter("llm-1", adapter_owner) + profile.vector_store = _adapter("vdb-1", adapter_owner) + profile.embedding_model = _adapter("emb-1", adapter_owner) + profile.x2text = _adapter("x2t-1", adapter_owner) + return profile + + +def _run_check( + profile: MagicMock, + *, + request_user_id: str | None = None, + admin_users: tuple[MagicMock, ...] = (), + members_by_user_id: dict[str, MagicMock] | None = None, + creator_is_member: bool = True, +) -> None: + """Invoke the validator with OrganizationMemberService / group access patched. + + ``members_by_user_id`` maps a request_user_id to an OrganizationMember + mock (``.user`` set); missing ids resolve to None. ``admin_users`` + are Users for whom ``is_user_organization_admin`` returns True. + """ + members_by_user_id = members_by_user_id or {} + + oms = MagicMock(name="OrganizationMemberService") + oms.is_user_organization_admin.side_effect = lambda u: u in admin_users + oms.get_user_by_user_id.side_effect = lambda uid: members_by_user_id.get(uid) + oms.get_user_by_id.return_value = ( + MagicMock(name="creator-membership") if creator_is_member else None + ) + + with ExitStack() as stack: + stack.enter_context(patch.object(_psh_mod, "OrganizationMemberService", oms)) + stack.enter_context( + patch.object(_psh_mod, "has_group_access", MagicMock(return_value=False)) + ) + PromptStudioHelper.validate_profile_manager_owner_access( + profile, request_user_id=request_user_id + ) + + +def _member_for(user: MagicMock) -> MagicMock: + member = MagicMock(name=f"member-{user.user_id}") + member.user = user + return member + + +class TestRequesterBypass: + """Cases 1 and 7 of the UN-3739 matrix: the reported bug.""" + + def test_admin_requester_passes_when_creator_lost_access(self) -> None: + """Admin indexing a project whose creator lost adapter access → pass.""" + admin = _user("admin-1", "admin@org.com") + creator = _user("userb", "userb@org.com") + other = _user("userx", "userx@org.com") + profile = _profile(creator, adapter_owner=other) + + _run_check( + profile, + request_user_id="admin-1", + admin_users=(admin,), + members_by_user_id={"admin-1": _member_for(admin)}, + ) + + def test_non_admin_requester_with_lapsed_creator_is_still_blocked(self) -> None: + """Case 2: the revocation guard must survive the fix.""" + requester = _user("userc", "userc@org.com") + creator = _user("userb", "userb@org.com") + other = _user("userx", "userx@org.com") + profile = _profile(creator, adapter_owner=other) + + with pytest.raises(PSPermissionError): + _run_check( + profile, + request_user_id="userc", + members_by_user_id={"userc": _member_for(requester)}, + ) + + def test_unknown_request_user_id_falls_through_to_creator_checks(self) -> None: + """A user_id with no membership row must not crash the check.""" + creator = _user("userb", "userb@org.com") + profile = _profile(creator, adapter_owner=creator) + + _run_check(profile, request_user_id="ghost-user") + + +class TestCreatorBypasses: + def test_service_account_creator_passes(self) -> None: + """Case 4: platform-API / org-migration projects (the customer's setup).""" + sa_creator = _user("sa-1", "sa@org.com", service_account=True) + other = _user("userx", "userx@org.com") + profile = _profile(sa_creator, adapter_owner=other) + + _run_check(profile) + + def test_admin_creator_passes(self) -> None: + """Case 3: existing behavior kept — creator-admin bypass.""" + admin_creator = _user("admin-1", "admin@org.com") + other = _user("userx", "userx@org.com") + profile = _profile(admin_creator, adapter_owner=other) + + _run_check(profile, admin_users=(admin_creator,)) + + def test_none_creator_passes(self) -> None: + """Case 8: SET_NULL creator skips the check (unchanged).""" + other = _user("userx", "userx@org.com") + profile = _profile(None, adapter_owner=other) + + _run_check(profile) + + def test_creator_with_access_passes_for_any_requester(self) -> None: + """Cases 5/6: delegated use — creator owns the adapters.""" + creator = _user("userb", "userb@org.com") + requester = _user("userc", "userc@org.com") + profile = _profile(creator, adapter_owner=creator) + + _run_check( + profile, + request_user_id="userc", + members_by_user_id={"userc": _member_for(requester)}, + ) + + +class TestDenialMessage: + """The message must name the creator, not 'You', and be a plain string.""" + + def _denied(self, *, creator_is_member: bool) -> PSPermissionError: + creator = _user("userb", "userb@org.com") + other = _user("userx", "userx@org.com") + profile = _profile(creator, adapter_owner=other) + # Creator has access to all but the LLM adapter — mirrors the + # customer's single-adapter denial. + for attr in ("vector_store", "embedding_model", "x2text"): + getattr(profile, attr).created_by = creator + + with pytest.raises(PSPermissionError) as exc_info: + _run_check(profile, creator_is_member=creator_is_member) + return exc_info.value + + def test_message_names_creator_and_adapter(self) -> None: + exc = self._denied(creator_is_member=True) + + # A tuple detail (the old ``error_msg = (f"...",)`` bug) becomes a + # list in DRF's APIException — detail must stay a plain string. + assert isinstance(exc.detail, str), "error detail must be a string" + message = str(exc) + assert "userb@org.com" in message + assert "llm-1" in message + assert "You do not have access" not in message + + def test_message_flags_former_member_creator(self) -> None: + """Case 7: offboarded creator — say so instead of a bare denial.""" + exc = self._denied(creator_is_member=False) + message = str(exc) + + assert isinstance(exc.detail, str) + assert "userb@org.com" in message + assert "no longer a member" in message + + def test_message_lists_all_inaccessible_adapters(self) -> None: + creator = _user("userb", "userb@org.com") + other = _user("userx", "userx@org.com") + profile = _profile(creator, adapter_owner=other) + + with pytest.raises(PSPermissionError) as exc_info: + _run_check(profile) + message = str(exc_info.value) + + assert isinstance(exc_info.value.detail, str) + for adapter_name in ("llm-1", "vdb-1", "emb-1", "x2t-1"): + assert adapter_name in message From fcf48ba572b670a6e385418541edca68b6d6589c Mon Sep 17 00:00:00 2001 From: Athul Date: Thu, 16 Jul 2026 19:00:34 +0530 Subject: [PATCH 2/5] =?UTF-8?q?UN-3739=20[FIX]=20Pass=20the=20real=20reque?= =?UTF-8?q?ster=20from=20views=20=E2=80=94=20user=5Fid=20is=20the=20file-p?= =?UTF-8?q?ath=20owner?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Devil's-advocate review of the previous commit found it defeated itself: every Prompt Studio run/index view passes user_id=tool.created_by.user_id (the project creator, who owns the document directory), so forwarding user_id as the requester re-validated the creator, not the person clicking Index. - Add an explicit request_user_id parameter through all four view entry points (index, fetch, bulk fetch, single pass) and their internal chains; views now pass request.user.user_id - Keep user_id untouched everywhere — it addresses the project creator's file storage path, not an identity to authorize - Plumbing regression test now pins request_user_id != user_id so the two identities can't be conflated again Co-Authored-By: Claude Fable 5 --- .../prompt_studio_helper.py | 36 ++++++++++++------- .../tests/test_build_index_payload.py | 13 +++++-- .../prompt_studio_core_v2/views.py | 6 ++++ 3 files changed, 40 insertions(+), 15 deletions(-) diff --git a/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py b/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py index 9bc4b9e353..fbc121d00b 100644 --- a/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py +++ b/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py @@ -496,6 +496,7 @@ def build_index_payload( user_id: str, document_id: str, run_id: str, + request_user_id: str | None = None, ) -> tuple[ExecutionContext, dict[str, Any]]: """Build ide_index ExecutionContext for fire-and-forget dispatch. @@ -518,7 +519,7 @@ def build_index_payload( PromptStudioHelper.validate_adapter_status(default_profile) PromptStudioHelper.validate_profile_manager_owner_access( - default_profile, request_user_id=user_id + default_profile, request_user_id=request_user_id ) # Common path decomposition used by extract, summarize, and index @@ -536,7 +537,7 @@ def build_index_payload( stem, extract_file_path, platform_api_key, - request_user_id=user_id, + request_user_id=request_user_id, ) ) @@ -718,6 +719,7 @@ def build_fetch_response_payload( document_id: str, run_id: str, profile_manager_id: str | None = None, + request_user_id: str | None = None, ) -> tuple[ExecutionContext | None, dict[str, Any]]: """Build answer_prompt ExecutionContext for fire-and-forget dispatch. @@ -740,7 +742,7 @@ def build_fetch_response_payload( PromptStudioHelper.validate_adapter_status(profile_manager) PromptStudioHelper.validate_profile_manager_owner_access( - profile_manager, request_user_id=user_id + profile_manager, request_user_id=request_user_id ) vector_db = str(profile_manager.vector_store.id) @@ -938,6 +940,7 @@ def build_bulk_fetch_response_payload( document_id: str, run_id: str, profile_manager_id: str | None = None, + request_user_id: str | None = None, ) -> tuple[ExecutionContext | None, dict[str, Any]]: """Build answer_prompt payload for multiple prompts in one task. @@ -960,7 +963,7 @@ def build_bulk_fetch_response_payload( PromptStudioHelper.validate_adapter_status(profile_manager) PromptStudioHelper.validate_profile_manager_owner_access( - profile_manager, request_user_id=user_id + profile_manager, request_user_id=request_user_id ) monitor_llm, challenge_llm = PromptStudioHelper._resolve_llm_ids(tool) @@ -1129,6 +1132,7 @@ def build_single_pass_payload( user_id: str, document_id: str, run_id: str, + request_user_id: str | None = None, ) -> tuple[ExecutionContext, dict[str, Any]]: """Build single_pass_extraction ExecutionContext. @@ -1153,7 +1157,7 @@ def build_single_pass_payload( PromptStudioHelper.validate_adapter_status(default_profile) PromptStudioHelper.validate_profile_manager_owner_access( - default_profile, request_user_id=user_id + default_profile, request_user_id=request_user_id ) default_profile.chunk_size = 0 @@ -1335,6 +1339,7 @@ def index_document( user_id: str, document_id: str, run_id: str = None, + request_user_id: str | None = None, ) -> Any: """Method to index a document. @@ -1392,14 +1397,14 @@ def index_document( # Need to check the user who created profile manager # has access to adapters configured in profile manager PromptStudioHelper.validate_profile_manager_owner_access( - default_profile, request_user_id=user_id + default_profile, request_user_id=request_user_id ) # Also validate summary profile if it's different from default if tool.summarize_context and summary_profile != default_profile: PromptStudioHelper.validate_adapter_status(summary_profile) PromptStudioHelper.validate_profile_manager_owner_access( - summary_profile, request_user_id=user_id + summary_profile, request_user_id=request_user_id ) fs_instance = EnvHelper.get_storage( @@ -1528,6 +1533,7 @@ def prompt_responder( id: str | None = None, run_id: str = None, profile_manager_id: str | None = None, + request_user_id: str | None = None, ) -> Any: """Execute chain/single run of the prompts. Makes a call to prompt service and returns the dict of response. @@ -1563,6 +1569,7 @@ def prompt_responder( document_id=document_id, run_id=run_id, profile_manager_id=profile_manager_id, + request_user_id=request_user_id, ) else: return PromptStudioHelper._execute_prompts_in_single_pass( @@ -1572,7 +1579,7 @@ def prompt_responder( org_id=org_id, document_id=document_id, run_id=run_id, - user_id=user_id, + request_user_id=request_user_id, ) @staticmethod @@ -1586,6 +1593,7 @@ def _execute_single_prompt( document_id, run_id, profile_manager_id, + request_user_id=None, ): prompt_instance = PromptStudioHelper._fetch_prompt_from_id(id) @@ -1652,6 +1660,7 @@ def _execute_single_prompt( run_id=run_id, profile_manager_id=profile_manager_id, user_id=user_id, + request_user_id=request_user_id, ) return PromptStudioHelper._handle_response( response=response, @@ -1691,7 +1700,7 @@ def _execute_prompts_in_single_pass( org_id, document_id, run_id, - user_id=None, + request_user_id=None, ): prompts = PromptStudioHelper.fetch_prompt_from_tool(tool_id) prompts = [ @@ -1722,7 +1731,7 @@ def _execute_prompts_in_single_pass( org_id=org_id, document_id=document_id, run_id=run_id, - user_id=user_id, + request_user_id=request_user_id, ) return PromptStudioHelper._handle_response( response=response, @@ -1809,6 +1818,7 @@ def _fetch_response( run_id: str, user_id: str, profile_manager_id: str | None = None, + request_user_id: str | None = None, ) -> Any: """Utility function to invoke prompt service. Used internally. @@ -1863,7 +1873,7 @@ def _fetch_response( # Need to check the user who created profile manager # has access to adapters PromptStudioHelper.validate_profile_manager_owner_access( - profile_manager, request_user_id=user_id + profile_manager, request_user_id=request_user_id ) # Not checking reindex here as there might be # change in Profile Manager @@ -2245,7 +2255,7 @@ def _fetch_single_pass_response( org_id: str, document_id: str, run_id: str = None, - user_id: str | None = None, + request_user_id: str | None = None, ) -> Any: tool_id: str = str(tool.tool_id) outputs: list[dict[str, Any]] = [] @@ -2266,7 +2276,7 @@ def _fetch_single_pass_response( PromptStudioHelper.validate_adapter_status(default_profile) # has access to adapters configured in profile manager PromptStudioHelper.validate_profile_manager_owner_access( - default_profile, request_user_id=user_id + default_profile, request_user_id=request_user_id ) default_profile.chunk_size = 0 # To retrive full context if prompt_grammar: diff --git a/backend/prompt_studio/prompt_studio_core_v2/tests/test_build_index_payload.py b/backend/prompt_studio/prompt_studio_core_v2/tests/test_build_index_payload.py index a7aea0118e..a0be0af703 100644 --- a/backend/prompt_studio/prompt_studio_core_v2/tests/test_build_index_payload.py +++ b/backend/prompt_studio/prompt_studio_core_v2/tests/test_build_index_payload.py @@ -132,6 +132,7 @@ def _dispatch_build( user_id="user-1", document_id="doc-1", run_id="run-1", + request_user_id="requester-1", ) return context, cb_kwargs, fs_instance, check_mock @@ -190,7 +191,14 @@ def test_check_extraction_status_raises_is_swallowed(self, caplog) -> None: class TestOwnerAccessPlumbing: - """UN-3739: the requesting user must reach the owner-access check.""" + """UN-3739: the REQUESTER must reach the owner-access check. + + ``user_id`` in these helpers is the file-path owner (the project + creator) — views pass ``tool.created_by.user_id`` there. The + requesting user travels separately as ``request_user_id``; asserting + it differs from ``user_id`` pins that the two identities are never + conflated again. + """ def test_owner_access_receives_requesting_user(self) -> None: validate_owner_mock = MagicMock(return_value=None) @@ -200,4 +208,5 @@ def test_owner_access_receives_requesting_user(self) -> None: validate_owner_mock=validate_owner_mock, ) _args, kwargs = validate_owner_mock.call_args - assert kwargs.get("request_user_id") == "user-1" + assert kwargs.get("request_user_id") == "requester-1" + assert kwargs.get("request_user_id") != "user-1" diff --git a/backend/prompt_studio/prompt_studio_core_v2/views.py b/backend/prompt_studio/prompt_studio_core_v2/views.py index 001d63a838..c032997e8a 100644 --- a/backend/prompt_studio/prompt_studio_core_v2/views.py +++ b/backend/prompt_studio/prompt_studio_core_v2/views.py @@ -446,9 +446,12 @@ def index_document(self, request: HttpRequest, pk: Any = None) -> Response: tool_id=str(tool.tool_id), file_name=file_name, org_id=UserSessionUtils.get_organization_id(request), + # user_id is the file-path owner (project creator); + # request_user_id is who triggered the action (UN-3739) user_id=tool.created_by.user_id, document_id=document_id, run_id=run_id, + request_user_id=request.user.user_id, ) dispatcher = PromptStudioHelper._get_dispatcher() @@ -607,6 +610,7 @@ def fetch_response(self, request: HttpRequest, pk: Any = None) -> Response: document_id=document_id, run_id=run_id, profile_manager_id=profile_manager_id, + request_user_id=request.user.user_id, ) # If document is being indexed, return pending status @@ -719,6 +723,7 @@ def bulk_fetch_response(self, request: HttpRequest, pk: Any = None) -> Response: document_id=document_id, run_id=run_id, profile_manager_id=profile_manager_id, + request_user_id=request.user.user_id, ) if context is None: @@ -825,6 +830,7 @@ def single_pass_extraction(self, request: HttpRequest, pk: uuid) -> Response: user_id=user_id, document_id=document_id, run_id=run_id, + request_user_id=request.user.user_id, ) dispatcher = PromptStudioHelper._get_dispatcher() From 9d33951e496e9d1359a8be314fb7150f8f3a60c3 Mon Sep 17 00:00:00 2001 From: Athul Date: Thu, 16 Jul 2026 19:37:14 +0530 Subject: [PATCH 3/5] UN-3739 [FIX] Address review: no PII in denial messages, annotate request_user_id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Denial messages no longer include the profile creator's email — after the requester-admin bypass, the only audience for these errors is non-admin collaborators, so remediation now points at an org admin and the creator's identity stays in server logs only (CodeRabbit) - Add missing str | None annotations on the two private single-pass helpers (Greptile) Co-Authored-By: Claude Fable 5 --- .../prompt_studio_helper.py | 31 ++++++++++--------- ...t_validate_profile_manager_owner_access.py | 15 ++++++--- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py b/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py index fbc121d00b..309ed50d40 100644 --- a/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py +++ b/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py @@ -261,30 +261,33 @@ def validate_profile_manager_owner_access( request_user_id, ) + # Creator identity stays in server logs only — this error is shown + # to non-admin collaborators (admins bypass above), so no PII here. denied_names = ", ".join(adapter.adapter_name for adapter in denied) profile_ref = ( f"This project's LLM profile '{profile_manager.profile_name}' was" - f" created by {owner.email}" + f" created by another user" ) if not OrganizationMemberService.get_user_by_id(owner.id): error_msg = ( - f"Permission Error: {profile_ref}, who is no longer a member of" - f" this organization. Recreate the default profile, or ask an" - f" admin to share these adapters with everyone: {denied_names}." + f"Permission Error: {profile_ref} who is no longer a member of" + f" this organization. Ask an org admin to recreate the default" + f" profile, or to share these adapters with everyone:" + f" {denied_names}." ) elif len(denied) > 1: error_msg = ( - f"Permission Error: {profile_ref}, who no longer has access to" - f" these adapters: {denied_names}. Re-share them with the" - f" creator, share them with everyone, or recreate the profile" - f" using adapters you have access to." + f"Permission Error: {profile_ref} who no longer has access to" + f" these adapters: {denied_names}. Ask an org admin to re-share" + f" them with the profile's creator, share them with everyone," + f" or recreate the profile." ) else: error_msg = ( - f"Permission Error: {profile_ref}, who no longer has access to" - f" the adapter '{denied_names}'. Re-share the adapter with" - f" them, share it with everyone, or recreate the profile using" - f" adapters you have access to." + f"Permission Error: {profile_ref} who no longer has access to" + f" the adapter '{denied_names}'. Ask an org admin to re-share" + f" it with the profile's creator, share it with everyone, or" + f" recreate the profile." ) raise PermissionError(error_msg) @@ -1593,7 +1596,7 @@ def _execute_single_prompt( document_id, run_id, profile_manager_id, - request_user_id=None, + request_user_id: str | None = None, ): prompt_instance = PromptStudioHelper._fetch_prompt_from_id(id) @@ -1700,7 +1703,7 @@ def _execute_prompts_in_single_pass( org_id, document_id, run_id, - request_user_id=None, + request_user_id: str | None = None, ): prompts = PromptStudioHelper.fetch_prompt_from_tool(tool_id) prompts = [ diff --git a/backend/prompt_studio/prompt_studio_core_v2/tests/test_validate_profile_manager_owner_access.py b/backend/prompt_studio/prompt_studio_core_v2/tests/test_validate_profile_manager_owner_access.py index 9ed3e3458a..26151ac8f2 100644 --- a/backend/prompt_studio/prompt_studio_core_v2/tests/test_validate_profile_manager_owner_access.py +++ b/backend/prompt_studio/prompt_studio_core_v2/tests/test_validate_profile_manager_owner_access.py @@ -181,7 +181,12 @@ def test_creator_with_access_passes_for_any_requester(self) -> None: class TestDenialMessage: - """The message must name the creator, not 'You', and be a plain string.""" + """The message must blame the creator role (never 'You'), carry no PII, + and be a plain string. + + Post-fix the only audience for these denials is non-admin + collaborators, so the creator's email stays in server logs only. + """ def _denied(self, *, creator_is_member: bool) -> PSPermissionError: creator = _user("userb", "userb@org.com") @@ -196,16 +201,18 @@ def _denied(self, *, creator_is_member: bool) -> PSPermissionError: _run_check(profile, creator_is_member=creator_is_member) return exc_info.value - def test_message_names_creator_and_adapter(self) -> None: + def test_message_blames_creator_without_pii(self) -> None: exc = self._denied(creator_is_member=True) # A tuple detail (the old ``error_msg = (f"...",)`` bug) becomes a # list in DRF's APIException — detail must stay a plain string. assert isinstance(exc.detail, str), "error detail must be a string" message = str(exc) - assert "userb@org.com" in message + assert "userb@org.com" not in message, "no PII in user-facing errors" + assert "created" in message assert "llm-1" in message assert "You do not have access" not in message + assert "admin" in message, "remediation must point at an org admin" def test_message_flags_former_member_creator(self) -> None: """Case 7: offboarded creator — say so instead of a bare denial.""" @@ -213,7 +220,7 @@ def test_message_flags_former_member_creator(self) -> None: message = str(exc) assert isinstance(exc.detail, str) - assert "userb@org.com" in message + assert "userb@org.com" not in message assert "no longer a member" in message def test_message_lists_all_inaccessible_adapters(self) -> None: From 153e4dccb72c76fcce7e681a0e1657f8a33dff91 Mon Sep 17 00:00:00 2001 From: Athul Date: Fri, 17 Jul 2026 08:59:08 +0530 Subject: [PATCH 4/5] UN-3739 [FIX] Address review: pass request.user object, harden plumbing contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Pass the requester as a User object (request_user) instead of a user_id string: deletes the re-resolution helper (3 queries → 1), stops keying an authz decision on the non-unique User.user_id CharField, and uses is_user_organization_admin directly so the service-account exclusion stays in one place - Make request_user keyword-only with NO default on the four view-facing builders — a dropped plumb is now a loud TypeError instead of a silent revert to pre-fix behavior; signature test pins it - Add forwarding tests for the three previously-untested builders (parametrized, sentinel-abort pattern) - Pin the three non-ownership access disjuncts (org-share, user-share, group-share) that a sabotage-check showed were untested - Document the created_by-None bypass honestly in the docstring; the platform-API label-set gap it interacts with is filed as UN-3750 Sonar: dedup forwarding tests (new-code duplication gate) and keep a single throwing invocation inside pytest.raises blocks. Co-Authored-By: Claude Fable 5 --- .../prompt_studio_helper.py | 86 ++++---- .../tests/test_build_index_payload.py | 13 +- ...t_validate_profile_manager_owner_access.py | 185 +++++++++++++----- .../prompt_studio_core_v2/views.py | 10 +- 4 files changed, 197 insertions(+), 97 deletions(-) diff --git a/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py b/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py index 309ed50d40..516b7bc008 100644 --- a/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py +++ b/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py @@ -183,16 +183,6 @@ def validate_adapter_status( if not adapter.is_usable: raise PermissionError(error_msg) - @staticmethod - def _is_org_admin_by_user_id(user_id: str | None) -> bool: - """Return True if ``user_id`` resolves to an org member with admin role.""" - if not user_id: - return False - member = OrganizationMemberService.get_user_by_user_id(user_id) - return bool( - member and OrganizationMemberService.is_user_organization_admin(member.user) - ) - @staticmethod def _user_has_adapter_access(user: Any, adapter: Any) -> bool: return ( @@ -205,26 +195,32 @@ def _user_has_adapter_access(user: Any, adapter: Any) -> bool: @staticmethod def validate_profile_manager_owner_access( profile_manager: ProfileManager, - request_user_id: str | None = None, + request_user: User | None = None, ) -> None: """Validate adapter access for a profile before using its adapters. This is a revocation guard on the profile *creator*, not a requester ACL: users with project access deliberately piggyback on - the creator's adapter access. Org admins and service-account - creators bypass it — admins have implicit access to all adapters, - and platform-API/org-migration profiles are created by service - accounts that hold no adapter shares by design (UN-3739). + the creator's adapter access. Bypasses (UN-3739): an org-admin + requester (implicit access to all adapters); a None creator — + profiles whose creating user was deleted, which includes + platform-API users removed on key deletion since ``created_by`` + is SET_NULL; a service-account creator (platform-API profiles + hold no adapter shares by design); and an org-admin creator. Args: profile_manager: The profile whose adapters will be used. - request_user_id: ``user_id`` of the user triggering the action. + request_user: The user triggering the action, if in a request + context. Worker paths pass None and rely on creator checks. Raises: PermissionError: If the profile creator no longer has access to one or more adapters and no bypass applies. """ - if PromptStudioHelper._is_org_admin_by_user_id(request_user_id): + if ( + request_user is not None + and OrganizationMemberService.is_user_organization_admin(request_user) + ): return owner = profile_manager.created_by @@ -258,7 +254,7 @@ def validate_profile_manager_owner_access( " requester %s", profile_manager.profile_name, owner.user_id, - request_user_id, + getattr(request_user, "user_id", None), ) # Creator identity stays in server logs only — this error is shown @@ -330,7 +326,7 @@ def _build_summarize_params( stem: str, extract_file_path: str, platform_api_key: str, - request_user_id: str | None = None, + request_user: User | None = None, ) -> tuple[dict[str, Any] | None, str, "ProfileManager"]: """Build summarize_params dict if summarization is enabled. @@ -355,7 +351,7 @@ def _build_summarize_params( if summary_profile != default_profile: PromptStudioHelper.validate_adapter_status(summary_profile) PromptStudioHelper.validate_profile_manager_owner_access( - summary_profile, request_user_id=request_user_id + summary_profile, request_user=request_user ) llm_adapter_id = ( @@ -499,7 +495,8 @@ def build_index_payload( user_id: str, document_id: str, run_id: str, - request_user_id: str | None = None, + *, + request_user: User | None, ) -> tuple[ExecutionContext, dict[str, Any]]: """Build ide_index ExecutionContext for fire-and-forget dispatch. @@ -522,7 +519,7 @@ def build_index_payload( PromptStudioHelper.validate_adapter_status(default_profile) PromptStudioHelper.validate_profile_manager_owner_access( - default_profile, request_user_id=request_user_id + default_profile, request_user=request_user ) # Common path decomposition used by extract, summarize, and index @@ -540,7 +537,7 @@ def build_index_payload( stem, extract_file_path, platform_api_key, - request_user_id=request_user_id, + request_user=request_user, ) ) @@ -722,7 +719,8 @@ def build_fetch_response_payload( document_id: str, run_id: str, profile_manager_id: str | None = None, - request_user_id: str | None = None, + *, + request_user: User | None, ) -> tuple[ExecutionContext | None, dict[str, Any]]: """Build answer_prompt ExecutionContext for fire-and-forget dispatch. @@ -745,7 +743,7 @@ def build_fetch_response_payload( PromptStudioHelper.validate_adapter_status(profile_manager) PromptStudioHelper.validate_profile_manager_owner_access( - profile_manager, request_user_id=request_user_id + profile_manager, request_user=request_user ) vector_db = str(profile_manager.vector_store.id) @@ -943,7 +941,8 @@ def build_bulk_fetch_response_payload( document_id: str, run_id: str, profile_manager_id: str | None = None, - request_user_id: str | None = None, + *, + request_user: User | None, ) -> tuple[ExecutionContext | None, dict[str, Any]]: """Build answer_prompt payload for multiple prompts in one task. @@ -966,7 +965,7 @@ def build_bulk_fetch_response_payload( PromptStudioHelper.validate_adapter_status(profile_manager) PromptStudioHelper.validate_profile_manager_owner_access( - profile_manager, request_user_id=request_user_id + profile_manager, request_user=request_user ) monitor_llm, challenge_llm = PromptStudioHelper._resolve_llm_ids(tool) @@ -1135,7 +1134,8 @@ def build_single_pass_payload( user_id: str, document_id: str, run_id: str, - request_user_id: str | None = None, + *, + request_user: User | None, ) -> tuple[ExecutionContext, dict[str, Any]]: """Build single_pass_extraction ExecutionContext. @@ -1160,7 +1160,7 @@ def build_single_pass_payload( PromptStudioHelper.validate_adapter_status(default_profile) PromptStudioHelper.validate_profile_manager_owner_access( - default_profile, request_user_id=request_user_id + default_profile, request_user=request_user ) default_profile.chunk_size = 0 @@ -1342,7 +1342,7 @@ def index_document( user_id: str, document_id: str, run_id: str = None, - request_user_id: str | None = None, + request_user: User | None = None, ) -> Any: """Method to index a document. @@ -1400,14 +1400,14 @@ def index_document( # Need to check the user who created profile manager # has access to adapters configured in profile manager PromptStudioHelper.validate_profile_manager_owner_access( - default_profile, request_user_id=request_user_id + default_profile, request_user=request_user ) # Also validate summary profile if it's different from default if tool.summarize_context and summary_profile != default_profile: PromptStudioHelper.validate_adapter_status(summary_profile) PromptStudioHelper.validate_profile_manager_owner_access( - summary_profile, request_user_id=request_user_id + summary_profile, request_user=request_user ) fs_instance = EnvHelper.get_storage( @@ -1536,7 +1536,7 @@ def prompt_responder( id: str | None = None, run_id: str = None, profile_manager_id: str | None = None, - request_user_id: str | None = None, + request_user: User | None = None, ) -> Any: """Execute chain/single run of the prompts. Makes a call to prompt service and returns the dict of response. @@ -1572,7 +1572,7 @@ def prompt_responder( document_id=document_id, run_id=run_id, profile_manager_id=profile_manager_id, - request_user_id=request_user_id, + request_user=request_user, ) else: return PromptStudioHelper._execute_prompts_in_single_pass( @@ -1582,7 +1582,7 @@ def prompt_responder( org_id=org_id, document_id=document_id, run_id=run_id, - request_user_id=request_user_id, + request_user=request_user, ) @staticmethod @@ -1596,7 +1596,7 @@ def _execute_single_prompt( document_id, run_id, profile_manager_id, - request_user_id: str | None = None, + request_user: User | None = None, ): prompt_instance = PromptStudioHelper._fetch_prompt_from_id(id) @@ -1663,7 +1663,7 @@ def _execute_single_prompt( run_id=run_id, profile_manager_id=profile_manager_id, user_id=user_id, - request_user_id=request_user_id, + request_user=request_user, ) return PromptStudioHelper._handle_response( response=response, @@ -1703,7 +1703,7 @@ def _execute_prompts_in_single_pass( org_id, document_id, run_id, - request_user_id: str | None = None, + request_user: User | None = None, ): prompts = PromptStudioHelper.fetch_prompt_from_tool(tool_id) prompts = [ @@ -1734,7 +1734,7 @@ def _execute_prompts_in_single_pass( org_id=org_id, document_id=document_id, run_id=run_id, - request_user_id=request_user_id, + request_user=request_user, ) return PromptStudioHelper._handle_response( response=response, @@ -1821,7 +1821,7 @@ def _fetch_response( run_id: str, user_id: str, profile_manager_id: str | None = None, - request_user_id: str | None = None, + request_user: User | None = None, ) -> Any: """Utility function to invoke prompt service. Used internally. @@ -1876,7 +1876,7 @@ def _fetch_response( # Need to check the user who created profile manager # has access to adapters PromptStudioHelper.validate_profile_manager_owner_access( - profile_manager, request_user_id=request_user_id + profile_manager, request_user=request_user ) # Not checking reindex here as there might be # change in Profile Manager @@ -2258,7 +2258,7 @@ def _fetch_single_pass_response( org_id: str, document_id: str, run_id: str = None, - request_user_id: str | None = None, + request_user: User | None = None, ) -> Any: tool_id: str = str(tool.tool_id) outputs: list[dict[str, Any]] = [] @@ -2279,7 +2279,7 @@ def _fetch_single_pass_response( PromptStudioHelper.validate_adapter_status(default_profile) # has access to adapters configured in profile manager PromptStudioHelper.validate_profile_manager_owner_access( - default_profile, request_user_id=request_user_id + default_profile, request_user=request_user ) default_profile.chunk_size = 0 # To retrive full context if prompt_grammar: diff --git a/backend/prompt_studio/prompt_studio_core_v2/tests/test_build_index_payload.py b/backend/prompt_studio/prompt_studio_core_v2/tests/test_build_index_payload.py index a0be0af703..234de45e12 100644 --- a/backend/prompt_studio/prompt_studio_core_v2/tests/test_build_index_payload.py +++ b/backend/prompt_studio/prompt_studio_core_v2/tests/test_build_index_payload.py @@ -29,6 +29,10 @@ PromptStudioHelper = _psh_mod.PromptStudioHelper IKeys = _psh_mod.IKeys +# Sentinel requester object handed to build_index_payload by every test — +# distinct from the file-path ``user_id`` ("user-1") by construction. +_REQUEST_USER = MagicMock(name="request-user") + def _make_tool(enable_highlight: bool = False, summarize_context: bool = False): tool = MagicMock(name="CustomTool") @@ -132,7 +136,7 @@ def _dispatch_build( user_id="user-1", document_id="doc-1", run_id="run-1", - request_user_id="requester-1", + request_user=_REQUEST_USER, ) return context, cb_kwargs, fs_instance, check_mock @@ -195,8 +199,8 @@ class TestOwnerAccessPlumbing: ``user_id`` in these helpers is the file-path owner (the project creator) — views pass ``tool.created_by.user_id`` there. The - requesting user travels separately as ``request_user_id``; asserting - it differs from ``user_id`` pins that the two identities are never + requesting user travels separately as the ``request_user`` object; + asserting the sentinel arrives pins that the two identities are never conflated again. """ @@ -208,5 +212,4 @@ def test_owner_access_receives_requesting_user(self) -> None: validate_owner_mock=validate_owner_mock, ) _args, kwargs = validate_owner_mock.call_args - assert kwargs.get("request_user_id") == "requester-1" - assert kwargs.get("request_user_id") != "user-1" + assert kwargs.get("request_user") is _REQUEST_USER diff --git a/backend/prompt_studio/prompt_studio_core_v2/tests/test_validate_profile_manager_owner_access.py b/backend/prompt_studio/prompt_studio_core_v2/tests/test_validate_profile_manager_owner_access.py index 26151ac8f2..88b0dfd382 100644 --- a/backend/prompt_studio/prompt_studio_core_v2/tests/test_validate_profile_manager_owner_access.py +++ b/backend/prompt_studio/prompt_studio_core_v2/tests/test_validate_profile_manager_owner_access.py @@ -8,13 +8,13 @@ Contract pinned here (evaluation order): - 1. Requester is an org admin → pass (implicit access) - 2. ``created_by`` is None → pass (unchanged) - 3. Creator is a service account → pass (platform-API projects) - 4. Creator is an org admin → pass (unchanged) - 5. Creator has access to all 4 adapters → pass (delegated use kept) - 6. Otherwise → PermissionError naming the - CREATOR (not "You"), with remediation, as a plain string. + 1. Requester (a ``User`` object) is an org admin → pass + 2. ``created_by`` is None → pass (unchanged) + 3. Creator is a service account → pass (platform-API) + 4. Creator is an org admin → pass (unchanged) + 5. Creator has access to all 4 adapters → pass (delegated use) + 6. Otherwise → PermissionError blaming the creator role (never "You"), + PII-free, as a plain string. Unit tests: the real helper module is imported and collaborators are patched per-test, so no database is touched. @@ -22,6 +22,7 @@ from __future__ import annotations +import inspect from contextlib import ExitStack from unittest.mock import MagicMock, patch @@ -68,22 +69,18 @@ def _profile(creator: MagicMock | None, adapter_owner: MagicMock) -> MagicMock: def _run_check( profile: MagicMock, *, - request_user_id: str | None = None, + request_user: MagicMock | None = None, admin_users: tuple[MagicMock, ...] = (), - members_by_user_id: dict[str, MagicMock] | None = None, + group_access: bool = False, creator_is_member: bool = True, ) -> None: """Invoke the validator with OrganizationMemberService / group access patched. - ``members_by_user_id`` maps a request_user_id to an OrganizationMember - mock (``.user`` set); missing ids resolve to None. ``admin_users`` - are Users for whom ``is_user_organization_admin`` returns True. + ``admin_users`` are Users for whom ``is_user_organization_admin`` + returns True. """ - members_by_user_id = members_by_user_id or {} - oms = MagicMock(name="OrganizationMemberService") oms.is_user_organization_admin.side_effect = lambda u: u in admin_users - oms.get_user_by_user_id.side_effect = lambda uid: members_by_user_id.get(uid) oms.get_user_by_id.return_value = ( MagicMock(name="creator-membership") if creator_is_member else None ) @@ -91,19 +88,15 @@ def _run_check( with ExitStack() as stack: stack.enter_context(patch.object(_psh_mod, "OrganizationMemberService", oms)) stack.enter_context( - patch.object(_psh_mod, "has_group_access", MagicMock(return_value=False)) + patch.object( + _psh_mod, "has_group_access", MagicMock(return_value=group_access) + ) ) PromptStudioHelper.validate_profile_manager_owner_access( - profile, request_user_id=request_user_id + profile, request_user=request_user ) -def _member_for(user: MagicMock) -> MagicMock: - member = MagicMock(name=f"member-{user.user_id}") - member.user = user - return member - - class TestRequesterBypass: """Cases 1 and 7 of the UN-3739 matrix: the reported bug.""" @@ -114,12 +107,7 @@ def test_admin_requester_passes_when_creator_lost_access(self) -> None: other = _user("userx", "userx@org.com") profile = _profile(creator, adapter_owner=other) - _run_check( - profile, - request_user_id="admin-1", - admin_users=(admin,), - members_by_user_id={"admin-1": _member_for(admin)}, - ) + _run_check(profile, request_user=admin, admin_users=(admin,)) def test_non_admin_requester_with_lapsed_creator_is_still_blocked(self) -> None: """Case 2: the revocation guard must survive the fix.""" @@ -129,23 +117,19 @@ def test_non_admin_requester_with_lapsed_creator_is_still_blocked(self) -> None: profile = _profile(creator, adapter_owner=other) with pytest.raises(PSPermissionError): - _run_check( - profile, - request_user_id="userc", - members_by_user_id={"userc": _member_for(requester)}, - ) + _run_check(profile, request_user=requester) - def test_unknown_request_user_id_falls_through_to_creator_checks(self) -> None: - """A user_id with no membership row must not crash the check.""" + def test_none_requester_falls_through_to_creator_checks(self) -> None: + """Worker paths pass no requester — creator checks still decide.""" creator = _user("userb", "userb@org.com") profile = _profile(creator, adapter_owner=creator) - _run_check(profile, request_user_id="ghost-user") + _run_check(profile, request_user=None) class TestCreatorBypasses: def test_service_account_creator_passes(self) -> None: - """Case 4: platform-API / org-migration projects (the customer's setup).""" + """Case 4: platform-API projects referencing others' adapters.""" sa_creator = _user("sa-1", "sa@org.com", service_account=True) other = _user("userx", "userx@org.com") profile = _profile(sa_creator, adapter_owner=other) @@ -173,11 +157,41 @@ def test_creator_with_access_passes_for_any_requester(self) -> None: requester = _user("userc", "userc@org.com") profile = _profile(creator, adapter_owner=creator) - _run_check( - profile, - request_user_id="userc", - members_by_user_id={"userc": _member_for(requester)}, - ) + _run_check(profile, request_user=requester) + + +class TestCreatorAccessDisjuncts: + """Each non-ownership access path must independently satisfy the guard. + + Review sabotage-check: reducing ``_user_has_adapter_access`` to + ``created_by == user`` must fail these. + """ + + def _lapsed_profile(self) -> MagicMock: + creator = _user("userb", "userb@org.com") + other = _user("userx", "userx@org.com") + return _profile(creator, adapter_owner=other) + + def test_org_shared_adapters_pass(self) -> None: + profile = self._lapsed_profile() + for attr in ("llm", "vector_store", "embedding_model", "x2text"): + getattr(profile, attr).shared_to_org = True + + _run_check(profile) + + def test_user_shared_adapters_pass(self) -> None: + profile = self._lapsed_profile() + for attr in ("llm", "vector_store", "embedding_model", "x2text"): + getattr( + profile, attr + ).shared_users.filter.return_value.exists.return_value = True + + _run_check(profile) + + def test_group_shared_adapters_pass(self) -> None: + profile = self._lapsed_profile() + + _run_check(profile, group_access=True) class TestDenialMessage: @@ -193,7 +207,7 @@ def _denied(self, *, creator_is_member: bool) -> PSPermissionError: other = _user("userx", "userx@org.com") profile = _profile(creator, adapter_owner=other) # Creator has access to all but the LLM adapter — mirrors the - # customer's single-adapter denial. + # reported single-adapter denial. for attr in ("vector_store", "embedding_model", "x2text"): getattr(profile, attr).created_by = creator @@ -235,3 +249,86 @@ def test_message_lists_all_inaccessible_adapters(self) -> None: assert isinstance(exc_info.value.detail, str) for adapter_name in ("llm-1", "vdb-1", "emb-1", "x2t-1"): assert adapter_name in message + + +class TestBuilderSignatures: + """``request_user`` must be keyword-only with NO default on the four + view-facing builders — a dropped plumb must fail loudly (TypeError), + never silently revert to pre-fix behavior.""" + + @pytest.mark.parametrize( + "builder", + [ + PromptStudioHelper.build_index_payload, + PromptStudioHelper.build_fetch_response_payload, + PromptStudioHelper.build_bulk_fetch_response_payload, + PromptStudioHelper.build_single_pass_payload, + ], + ) + def test_request_user_is_required_keyword_only(self, builder) -> None: + param = inspect.signature(builder).parameters["request_user"] + assert param.kind is inspect.Parameter.KEYWORD_ONLY + assert param.default is inspect.Parameter.empty + + +class _Forwarded(Exception): + """Sentinel raised by the patched validator to abort the builder.""" + + +class TestBuilderForwarding: + """Each builder must hand the requester object to the validator.""" + + REQUEST_USER = MagicMock(name="request-user") + + @pytest.mark.parametrize( + "builder_name, extra_kwargs_factory", + [ + ( + "build_fetch_response_payload", + lambda: {"prompt": MagicMock(name="prompt")}, + ), + ( + "build_bulk_fetch_response_payload", + lambda: {"prompts": [MagicMock(name="prompt")]}, + ), + ( + "build_single_pass_payload", + lambda: {"prompts": [MagicMock(name="prompt")]}, + ), + ], + ) + def test_builder_forwards_requester(self, builder_name, extra_kwargs_factory) -> None: + validator = MagicMock(side_effect=_Forwarded()) + builder = getattr(PromptStudioHelper, builder_name) + call_kwargs = { + "tool": MagicMock(name="tool"), + "doc_path": "/doc", + "doc_name": "doc.pdf", + "org_id": "org-1", + "user_id": "owner-1", + "document_id": "doc-1", + "run_id": "run-1", + "request_user": self.REQUEST_USER, + **extra_kwargs_factory(), + } + with ExitStack() as stack: + for target, attr, value in ( + ( + _psh_mod.ProfileManager, + "get_default_llm_profile", + MagicMock(return_value=MagicMock(name="profile")), + ), + ( + PromptStudioHelper, + "_resolve_llm_ids", + MagicMock(return_value=("m", "c")), + ), + (PromptStudioHelper, "validate_adapter_status", MagicMock()), + (PromptStudioHelper, "validate_profile_manager_owner_access", validator), + ): + stack.enter_context(patch.object(target, attr, value)) + with pytest.raises(_Forwarded): + builder(**call_kwargs) + + _args, kwargs = validator.call_args + assert kwargs.get("request_user") is self.REQUEST_USER diff --git a/backend/prompt_studio/prompt_studio_core_v2/views.py b/backend/prompt_studio/prompt_studio_core_v2/views.py index c032997e8a..1f28a66bea 100644 --- a/backend/prompt_studio/prompt_studio_core_v2/views.py +++ b/backend/prompt_studio/prompt_studio_core_v2/views.py @@ -447,11 +447,11 @@ def index_document(self, request: HttpRequest, pk: Any = None) -> Response: file_name=file_name, org_id=UserSessionUtils.get_organization_id(request), # user_id is the file-path owner (project creator); - # request_user_id is who triggered the action (UN-3739) + # request_user is who triggered the action (UN-3739) user_id=tool.created_by.user_id, document_id=document_id, run_id=run_id, - request_user_id=request.user.user_id, + request_user=request.user, ) dispatcher = PromptStudioHelper._get_dispatcher() @@ -610,7 +610,7 @@ def fetch_response(self, request: HttpRequest, pk: Any = None) -> Response: document_id=document_id, run_id=run_id, profile_manager_id=profile_manager_id, - request_user_id=request.user.user_id, + request_user=request.user, ) # If document is being indexed, return pending status @@ -723,7 +723,7 @@ def bulk_fetch_response(self, request: HttpRequest, pk: Any = None) -> Response: document_id=document_id, run_id=run_id, profile_manager_id=profile_manager_id, - request_user_id=request.user.user_id, + request_user=request.user, ) if context is None: @@ -830,7 +830,7 @@ def single_pass_extraction(self, request: HttpRequest, pk: uuid) -> Response: user_id=user_id, document_id=document_id, run_id=run_id, - request_user_id=request.user.user_id, + request_user=request.user, ) dispatcher = PromptStudioHelper._get_dispatcher() From ba313f4f73a53350aab41b992687632f572f5deb Mon Sep 17 00:00:00 2001 From: Athul Date: Fri, 17 Jul 2026 15:26:09 +0530 Subject: [PATCH 5/5] UN-3739 [FIX] Address review round 3: creator-requester message, summarize hop contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Denial message no longer claims "created by another user" when the requester IS the creator (share revoked on their own profile) — they are addressed directly, still PII-free - _build_summarize_params: request_user is now required keyword-only — it guards a live second validator call, so a dropped plumb must TypeError, not silently revert (added to the signature test; direct forwarding test added for the hop) - Denial logging collapsed to one record carrying profile, creator, denied adapter ids, and requester; ERROR_MSG constant deleted - Adapter names deduped in messages (adapters of different types can share a name); single-vs-multi message branches pinned by tests - Docstring no longer claims nonexistent "worker paths"; test case citations renumbered to the module docstring's contract; stale access-model comment reduced to a pointer; redundant None guard dropped (callee handles None) Co-Authored-By: Claude Fable 5 --- .../prompt_studio_helper.py | 51 ++++++--- ...t_validate_profile_manager_owner_access.py | 103 ++++++++++++++++-- 2 files changed, 128 insertions(+), 26 deletions(-) diff --git a/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py b/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py index 3dfae2a326..ff46d14a31 100644 --- a/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py +++ b/backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py @@ -88,7 +88,6 @@ logger = logging.getLogger(__name__) CHOICES_JSON = "/static/select_choices.json" -ERROR_MSG = "User %s doesn't have access to adapter %s" logger = logging.getLogger(__name__) @@ -220,17 +219,16 @@ def validate_profile_manager_owner_access( Args: profile_manager: The profile whose adapters will be used. - request_user: The user triggering the action, if in a request - context. Worker paths pass None and rely on creator checks. + request_user: The user triggering the action. Defaults to None + only for the legacy, currently-uncalled ``index_document`` / + ``prompt_responder`` chain (UN-3756); all live callers pass + a requester. Raises: PermissionError: If the profile creator no longer has access to one or more adapters and no bypass applies. """ - if ( - request_user is not None - and OrganizationMemberService.is_user_organization_admin(request_user) - ): + if OrganizationMemberService.is_user_organization_admin(request_user): return owner = profile_manager.created_by @@ -249,27 +247,45 @@ def validate_profile_manager_owner_access( profile_manager.embedding_model, profile_manager.x2text, ] - # Access resolution via the UN-2202 membership bridges — created_by - # is audit-only for adapters; owner/viewer roles carry access. + # Access = org share, owner/viewer role, or group (UN-2202); + # see _adapter_accessible_by. denied = [ adapter for adapter in adapters if not _adapter_accessible_by(adapter, owner) ] if not denied: return - for adapter in denied: - logger.error(ERROR_MSG, owner.user_id, adapter.id) logger.error( - "Adapter access denied for profile '%s': creator %s lacks access," - " requester %s", + "Adapter access denied for profile '%s': creator %s lacks access" + " to adapters %s, requester %s", profile_manager.profile_name, owner.user_id, + [str(adapter.id) for adapter in denied], getattr(request_user, "user_id", None), ) - # Creator identity stays in server logs only — this error is shown - # to non-admin collaborators (admins bypass above), so no PII here. - denied_names = ", ".join(adapter.adapter_name for adapter in denied) + # Third-party identity stays in server logs only — user-facing text + # carries no PII. + denied_names = ", ".join( + dict.fromkeys(adapter.adapter_name for adapter in denied) + ) + if request_user is not None and owner.pk == request_user.pk: + # The requester IS the creator — "created by another user" + # would be false, and they can be addressed directly. + adapter_ref = ( + f"the adapter '{denied_names}', which you no longer have" f" access to" + if len(denied) == 1 + else f"adapters you no longer have access to: {denied_names}" + ) + error_msg = ( + f"Permission Error: This project's LLM profile" + f" '{profile_manager.profile_name}' uses {adapter_ref}." + f" Ask an org admin to re-share access with you or share with" + f" everyone, or recreate the profile with adapters you can" + f" access." + ) + raise PermissionError(error_msg) + profile_ref = ( f"This project's LLM profile '{profile_manager.profile_name}' was" f" created by another user" @@ -336,7 +352,8 @@ def _build_summarize_params( stem: str, extract_file_path: str, platform_api_key: str, - request_user: User | None = None, + *, + request_user: User | None, ) -> tuple[dict[str, Any] | None, str, "ProfileManager"]: """Build summarize_params dict if summarization is enabled. diff --git a/backend/prompt_studio/prompt_studio_core_v2/tests/test_validate_profile_manager_owner_access.py b/backend/prompt_studio/prompt_studio_core_v2/tests/test_validate_profile_manager_owner_access.py index 27ab9e8d87..086a0d600d 100644 --- a/backend/prompt_studio/prompt_studio_core_v2/tests/test_validate_profile_manager_owner_access.py +++ b/backend/prompt_studio/prompt_studio_core_v2/tests/test_validate_profile_manager_owner_access.py @@ -114,7 +114,7 @@ def _run_check( class TestRequesterBypass: - """Cases 1 and 7 of the UN-3739 matrix: the reported bug.""" + """Contract case 1: the reported bug (and its case-6 guard).""" def test_admin_requester_passes_when_creator_lost_access(self) -> None: """Admin indexing a project whose creator lost adapter access → pass.""" @@ -126,7 +126,7 @@ def test_admin_requester_passes_when_creator_lost_access(self) -> None: _run_check(profile, request_user=admin, admin_users=(admin,)) def test_non_admin_requester_with_lapsed_creator_is_still_blocked(self) -> None: - """Case 2: the revocation guard must survive the fix.""" + """Contract case 6: the revocation guard must survive the fix.""" requester = _user("userc", "userc@org.com") creator = _user("userb", "userb@org.com") other = _user("userx", "userx@org.com") @@ -145,7 +145,7 @@ def test_none_requester_falls_through_to_creator_checks(self) -> None: class TestCreatorBypasses: def test_service_account_creator_passes(self) -> None: - """Case 4: platform-API projects referencing others' adapters.""" + """Contract case 3: platform-API projects referencing others' adapters.""" sa_creator = _user("sa-1", "sa@org.com", service_account=True) other = _user("userx", "userx@org.com") profile = _profile(sa_creator, adapter_owner=other) @@ -153,7 +153,7 @@ def test_service_account_creator_passes(self) -> None: _run_check(profile) def test_admin_creator_passes(self) -> None: - """Case 3: existing behavior kept — creator-admin bypass.""" + """Contract case 4: existing behavior kept — creator-admin bypass.""" admin_creator = _user("admin-1", "admin@org.com") other = _user("userx", "userx@org.com") profile = _profile(admin_creator, adapter_owner=other) @@ -161,14 +161,14 @@ def test_admin_creator_passes(self) -> None: _run_check(profile, admin_users=(admin_creator,)) def test_none_creator_passes(self) -> None: - """Case 8: SET_NULL creator skips the check (unchanged).""" + """Contract case 2: SET_NULL creator skips the check (unchanged).""" other = _user("userx", "userx@org.com") profile = _profile(None, adapter_owner=other) _run_check(profile) def test_creator_with_access_passes_for_any_requester(self) -> None: - """Cases 5/6: delegated use — creator owns the adapters.""" + """Contract case 5: delegated use — creator owns the adapters.""" creator = _user("userb", "userb@org.com") requester = _user("userc", "userc@org.com") profile = _profile(creator, adapter_owner=creator) @@ -241,9 +241,43 @@ def test_message_blames_creator_without_pii(self) -> None: assert "llm-1" in message assert "You do not have access" not in message assert "admin" in message, "remediation must point at an org admin" + # Single-denial branch must use singular copy, not the multi branch. + assert "the adapter 'llm-1'" in message + assert "these adapters" not in message + + def test_message_addresses_creator_requester_directly(self) -> None: + """When the requester IS the creator, 'created by another user' is + false — the message must speak to them directly (still PII-free).""" + creator = _user("userb", "userb@org.com") + other = _user("userx", "userx@org.com") + profile = _profile(creator, adapter_owner=other) + for attr in ("vector_store", "embedding_model", "x2text"): + getattr(profile, attr).owner_set.add(creator) + + with pytest.raises(PSPermissionError) as exc_info: + _run_check(profile, request_user=creator) + message = str(exc_info.value) + + assert "another user" not in message, "requester is the creator" + assert "you no longer have access" in message + assert "llm-1" in message + assert "userb@org.com" not in message + + def test_message_dedupes_adapter_names(self) -> None: + """Adapters of different types can share a name — list it once.""" + creator = _user("userb", "userb@org.com") + other = _user("userx", "userx@org.com") + profile = _profile(creator, adapter_owner=other) + for attr in ("llm", "vector_store", "embedding_model", "x2text"): + getattr(profile, attr).adapter_name = "shared-name" + + with pytest.raises(PSPermissionError) as exc_info: + _run_check(profile) + + assert str(exc_info.value).count("shared-name") == 1 def test_message_flags_former_member_creator(self) -> None: - """Case 7: offboarded creator — say so instead of a bare denial.""" + """Contract case 6, former-member sub-branch: offboarded creator — say so.""" exc = self._denied(creator_is_member=False) message = str(exc) @@ -263,12 +297,15 @@ def test_message_lists_all_inaccessible_adapters(self) -> None: assert isinstance(exc_info.value.detail, str) for adapter_name in ("llm-1", "vdb-1", "emb-1", "x2t-1"): assert adapter_name in message + # Multi-denial branch must use plural copy, not the single branch. + assert "these adapters:" in message + assert "the adapter '" not in message class TestBuilderSignatures: """``request_user`` must be keyword-only with NO default on the four - view-facing builders — a dropped plumb must fail loudly (TypeError), - never silently revert to pre-fix behavior.""" + view-facing builders and the live summarize hop — a dropped plumb must + fail loudly (TypeError), never silently revert to pre-fix behavior.""" @pytest.mark.parametrize( "builder", @@ -277,6 +314,7 @@ class TestBuilderSignatures: PromptStudioHelper.build_fetch_response_payload, PromptStudioHelper.build_bulk_fetch_response_payload, PromptStudioHelper.build_single_pass_payload, + PromptStudioHelper._build_summarize_params, ], ) def test_request_user_is_required_keyword_only(self, builder) -> None: @@ -285,6 +323,53 @@ def test_request_user_is_required_keyword_only(self, builder) -> None: assert param.default is inspect.Parameter.empty +class TestSummarizeHopForwarding: + """The summarize-profile validation inside ``_build_summarize_params`` + must receive the requester — it guards a live second validator call.""" + + def test_summarize_profile_check_receives_requesting_user(self) -> None: + request_user = MagicMock(name="request-user") + validator = MagicMock(side_effect=_Forwarded()) + tool = MagicMock(name="tool") + tool.summarize_context = True + tool.summarize_llm_adapter = None + summary_profile = MagicMock(name="summary-profile") + + with ExitStack() as stack: + for target, attr, value in ( + ( + _psh_mod.SummarizeMigrationUtils, + "migrate_tool_to_adapter_based", + MagicMock(), + ), + ( + _psh_mod.ProfileManager, + "objects", + MagicMock(get=MagicMock(return_value=summary_profile)), + ), + (PromptStudioHelper, "validate_adapter_status", MagicMock()), + ( + PromptStudioHelper, + "validate_profile_manager_owner_access", + validator, + ), + ): + stack.enter_context(patch.object(target, attr, value)) + with pytest.raises(_Forwarded): + PromptStudioHelper._build_summarize_params( + tool, + MagicMock(name="default-profile"), + "/dir", + "stem", + "/dir/extract/stem.txt", + "pk-test", + request_user=request_user, + ) + + _args, kwargs = validator.call_args + assert kwargs.get("request_user") is request_user + + class _Forwarded(Exception): """Sentinel raised by the patched validator to abort the builder."""