UN-3315 [FIX] Honour shared_to_org for Prompt Studio prompt edits - #2259
Open
hari-kuriakose wants to merge 19 commits into
Open
UN-3315 [FIX] Honour shared_to_org for Prompt Studio prompt edits#2259hari-kuriakose wants to merge 19 commits into
hari-kuriakose wants to merge 19 commits into
Conversation
Projects shared via "Share with everyone" set shared_to_org on the parent CustomTool. IsOwnerOrSharedUserOrSharedToOrg already honours that flag, so such a project was visible to the whole org -- but PromptAcesssToUser, which guards prompt/note CRUD, never checked it. The result was that only the owner could edit prompts in a project shared with everyone. Adds the shared_to_org check to PromptAcesssToUser so prompt access matches the project access the share already granted. UN-3542 (last org admin demotion) needs no change: the _ensure_not_last_admin_demotion guard already landed on main in 2f996d3 (#2048) and is wired into both add_user_role and remove_user_role. The ticket is stale and should be closed rather than reimplemented. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sXFBEu2GHatXq2CV2ShPF
Follow-up to 79c985b, which made shared_to_org grant access to a project's prompts. PromptAcesssToUser is the sole permission class on ToolStudioPromptView, which routes delete->destroy (prompt_studio_v2/urls.py:13), so that grant covered deletion too. The parent CustomTool's own destroy is owner-only (IsOwner in CustomToolViewSet.get_permissions), which left any org member able to delete every prompt inside a project they could not themselves delete. Product decision: "share with everyone" means view + edit, not delete. Adds IsPromptParentToolOwner and splits get_permissions so only destroy uses it. Reads and edits keep the widened class, matching CustomToolViewSet, which already routes update/partial_update on the tool itself through IsOwnerOrSharedUserOrSharedToOrg. Kept as a separate class rather than teaching IsParentToolOwner to read both prompt_studio_tool and tool_id: a shared authorization class that accumulates per-caller special cases is how these gates drift apart. Also addresses two review findings on 79c985b: - getattr(tool, "shared_to_org", False) -> tool.shared_to_org. tool is always a CustomTool, where the field is a non-nullable BooleanField, so the default was unreachable and would only mask a renamed field by silently denying. Matches IsOwnerOrSharedUserOrSharedToOrg, which reads it directly. - The inline comment claimed prompts "stayed read-only for everyone except the owner". That was imprecise: VIEWER and group-share already granted write. Narrowed to the shared_to_org-only user, who genuinely had no access. Known gap, unchanged by this commit: reorder_prompts is a collection-level POST, so get_object() never runs and neither permission class gates it (see prompt_studio_v2/helper.py:28). tool_instance_v2/views.py:196-205 has the pattern that closes it. Needs its own change. Untested: the backend suite does not run in this checkout -- settings import fails at backend/settings/base.py:63 on CELERY_BROKER_BASE_URL=None, and conftest.py notes backend tests do not run under tox in CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
Follow-up to 943b3a0, whose comments overstated what the destroy split achieves and asserted an invariant the models contradict. 1. "Deletion is not included" / "not deletable by it" was false. The bulk sync_prompts route on PromptStudioCoreView rip-and-replaces every prompt in a project and admits org-shared users -- only destroy and the co-owner actions are IsOwner-gated there, and CustomTool.objects.for_user admits shared_to_org=True so no 404 shields it. The split closes per-prompt DELETE and nothing else. Both comments now say so and point at the surviving route rather than implying it does not exist. 2. "tool is always a CustomTool" contradicted the model (tool_id is a nullable SET_NULL FK) and the IsPromptParentToolOwner docstring 25 lines below, which correctly documents the orphan case. A maintainer trusting it would drop the `tool is not None` guard and turn a clean 403 into an AttributeError 500. Dropped the sentence; the "a default masks a renamed field" rationale is true on its own and is what the direct read actually rests on. 3. The divergence docstring cited only the precedent that supports the widening (CustomToolViewSet) and omitted the nearer sibling that chose the other way -- ProfileManagerView gates every mutation behind IsParentToolOwner. Two sub-resources of one parent answer "does a share grant edit?" differently; the docstring now names that rather than reading as though it were settled. No behaviour change: comments and docstrings only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
4d8a381 fixed one over-broad closing claim and introduced another. It said ProfileManagerView "gates every mutation behind IsParentToolOwner, so a shared user can edit a project's prompts but not its profiles". Both halves are false in the direction that stops someone hardening those routes: - IsParentToolOwner implements only has_object_permission. DRF never calls it for create (there is no object yet), and ProfileManagerView.create never calls check_object_permissions or get_object, so that route is ungated. The sibling IsParentDeploymentOwner docstring documents this exact DRF gap and notes its view compensates by handing the parent to check_object_permissions -- ProfileManagerView does not. - create_profile_manager and make_profile_default on PromptStudioCoreView both fall through to IsOwnerOrSharedUserOrSharedToOrg, which admits shared_to_org. Narrowed to the three routes IsParentToolOwner actually gates, and named the gap rather than implying profiles are locked down. No behaviour change: docstring only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
sync_prompts is a rip-and-replace that deletes every prompt in a project
before importing, and PromptStudioCoreView.get_permissions sent only
destroy/add_co_owner/remove_co_owner to IsOwner(). Everything else, this
route included, fell through to IsOwnerOrSharedUserOrSharedToOrg, which
admits shared_to_org -- and CustomTool.objects.for_user admits those tools
too, so no 404 shielded it. Any org member of a shared project could wipe
every prompt in it, outputs riding along via CASCADE.
Two changes, closing two different things:
1. sync_prompts joins the IsOwner() list. Deleting every prompt is
owner-level destruction; UN-3315 settled that a share grants view + edit,
not delete.
2. An empty-payload guard in PromptStudioHelper.sync_prompts, ahead of the
transaction. This one is a correctness fix as much as a security one: the
docstring says "deletes all existing prompts and creates new ones", but
the create half is a loop over prompts_data that never runs on an empty
list. So {"prompts": []} deleted everything, imported nothing, and
returned success -- the code did not do what it documented. Raises
ValueError to match the default_profile guard immediately below it.
The guard sits before `with transaction.atomic()` deliberately: a
rollback is not a refusal, and the point is that the delete never
executes. Rejects empty only -- a non-empty list is a legitimate replace
and still works.
Honest scope: this closes the session-user path via IsOwner(), and the
empty-payload wipe on both paths via the guard. A read_write platform API
key can still call sync_prompts and replace prompts wholesale with a
non-empty list -- service accounts short-circuit ahead of every check, and
this route declares no required_method tier the way mcp_server does. Known
and accepted; deliberately not addressed here.
Also updates the two comments 0a96979 left behind, which said the
sync_prompts hole was open. It is not, for a session user, as of this
commit.
Untested: the backend suite does not run in this checkout (settings import
fails on CELERY_BROKER_BASE_URL=None).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
75fd2ac added a guard refusing {"prompts": []} on the premise that an empty rip-and-replace deleting everything and importing nothing was the code failing its own contract. That premise was wrong. test_prompt_studio_author.py:216 -- test_sync_prompts_clear_bumps_tool_ modified_at -- calls sync_prompts(tool, {"prompts": []}, user) and asserts prompts_deleted == 1, with a docstring describing a prompts-clearing sync as behaviour that must bump modified_at. Clearing every prompt by syncing an empty list is existing, intentional, test-asserted behaviour, not an accident. The guard broke a published capability and would have failed that test the moment anyone could run the suite. Reverts the guard only. The IsOwner() gating from 75fd2ac stands: the exposure was always a permissions question, and clearing a project's prompts is now owner-only like every other deletion path. Docstring updated to record the empty-list clear as supported, so the next reader does not re-derive it as a defect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
reorder_prompts is declared @action(detail=True) but routed at the collection path (prompt/reorder/, urls.py:22) with a signature taking no pk -- the target comes from prompt_id in the request body. DRF therefore never calls get_object(), so has_object_permission never fires, and neither permission class defines has_permission, so BasePermission's default True applied. The helper then fetched on the raw manager (helper.py:28, no for_user scoping) and mutated every sibling row sharing the derived tool_id. Net: any authenticated user could renumber the prompts of any project in any organization, shared or not. Resolves the prompt and calls check_object_permissions explicitly, the same shape ToolInstanceViewSet.reorder uses for the identical collection-POST problem (tool_instance_v2/views.py:196-205). Two details worth stating: - Gated as an EDIT, not a deletion. reorder_prompts resolves to PromptAcesssToUser, so org-shared members can reorder, per UN-3315's view + edit ruling. Routing it to owner-only would have over-restricted. - Org scoping goes through the parent tool. ToolStudioPrompt is a plain BaseModel with no organization field and no for_user manager, so filtering on CustomTool.objects.for_user(...) is what makes a cross-org prompt_id 404 before the permission check rather than after it. A missing prompt_id now raises a 400 rather than reaching the controller, which previously surfaced it as a serializer error further in. Untested: the backend suite does not run in this checkout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
ProfileManagerView.get_permissions already listed create under IsParentToolOwner(), but that class defined only has_object_permission, which DRF never calls for create -- there is no object yet. So the docstring's guarantee, "Mutations require ownership of the parent tool", was false for exactly one action: any org member could create a ProfileManager against any tool by naming it in the payload. Adds has_permission, reading the parent from the request payload (prompt_studio_tool; ProfileManagerSerializer uses fields = "__all__", so it is present on create) and applying the same ownership test as has_object_permission. Service-account and org-admin fallbacks preserved in both. has_object_permission is untouched, so update/partial_update/ destroy keep resolving through get_object() as before. Non-create actions return True from has_permission and are still decided by has_object_permission -- the collection gate must not double-gate an action whose object check already covers it. Malformed input denies rather than crashes: request.data may be a QueryDict, a list, or unparsed garbage, and the pk is a UUID, so a non-dict body, an absent prompt_studio_tool, or an unparseable id returns False and lets the serializer raise its own 400. Django's ValidationError is what a bad UUID raises here, hence the import. Deliberately extends the shared class rather than adding a sibling, which is the opposite of the IsPromptParentToolOwner call two commits back. Not a contradiction: there, the shared class would have had to juggle two different parent FK names (prompt_studio_tool vs tool_id) for two consumers. Here there is one consumer -- ProfileManagerView is the only non-docstring reference to IsParentToolOwner in the tree -- and what is being added is a method the class was always missing. Untested: the backend suite does not run in this checkout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
…aces Three prose defects from 5bc4ee9 and c35fbe7, all in the closing direction -- each told a reader a surface was shut when it is not. 1. "ProfileManagerView routes every mutation through IsParentToolOwner, so a shared user can edit a project's prompts but not its profiles." False twice over. ProfileManagerView routes no create at all (its urls.py has a single detail path), and profiles are created via PromptStudioCoreView.create_profile_manager, which falls through to IsOwnerOrSharedUserOrSharedToOrg and admits org-shared users -- as does make_profile_default. 75fd2ac said exactly this and was correct; 5bc4ee9 deleted it and replaced it with the false claim. Restored. 2. "A read_write API key still reaches both." Only one. Per the tier table in _is_service_account, read_write covers POST/PUT/PATCH and full_access adds DELETE, so a read_write key is refused on per-prompt destroy (an HTTP DELETE) and reaches only sync_prompts (a POST). 3. "Two deletion paths remain open to a non-owner" then listed one route twice, the second entry concluding it is in fact owner-gated. Now states the one path, with the empty-list clear recorded separately as supported behaviour rather than as a hole. No behaviour change: comments and docstrings only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
Fourteen tests over the three mechanisms enforcing "a share grants view and edit, not delete". Every one is mutation-checked: the fix was reverted, the test observed to fail, then restored. 1. destroy split reverted to a flat permission_classes -> fails 2. split widened to gate update/partial_update too -> fails (x2) 3. check_object_permissions deleted from reorder_prompts -> fails (x2) 4. "sync_prompts" removed from the IsOwner list -> fails 5. the UN-3315 shared_to_org branch removed -> fails Mutation 2 is the over-restriction guard. UN-3315 grants edit; a split that routed every mutation to the owner-only class would pass the deletion tests while silently removing the capability this work exists to add. Mutation 5 covers the same axis from the other side. Imports the real modules rather than slicing bodies out with tests_common.source_extraction, as the sibling registry suite does. That technique's own docstring records why it cannot serve here: bodies are exec-ed out of context, so unreachable code is indistinguishable from wired code -- and "the hook is never reached" IS the reorder_prompts defect. A source-extracted test of PromptAcesssToUser.has_object_permission would have passed both before and after that fix. The same docstring notes the premise behind extraction no longer holds (Django is importable in this tier), and importing also avoids its other two sharp edges. Correcting the record: four earlier commits in this series say "the backend suite does not run in this checkout". That is true only of the DB-backed tier. The permission tier is deliberately DB-free and runs in about a second; the blocker was that settings vars must be exported into the environment, not merely written to test.env. Those notes were wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
…eachable 70ff027 added has_permission to close what looked like an ungated create: ProfileManagerView.get_permissions lists "create" under IsParentToolOwner, and that class defined only has_object_permission, which DRF never calls for create. The second half is true. The first is not. ProfileManagerView exposes no create route. prompt_profile_manager_v2/ urls.py binds exactly one path -- profile-manager/<uuid:pk>/ -- to {get: retrieve, put: update, patch: partial_update, delete: destroy}. No collection POST, and no router registration anywhere in the tree; the only other references to the viewset are that import and a docstring. So self.action is never "create", the method always took its non-create early return, and it closed nothing. The "create" entry in that get_permissions list is itself dead, but it predates this PR. The real gap is on PromptStudioCoreView: create_profile_manager (views.py:958) and make_profile_default both fall through get_permissions to IsOwnerOrSharedUserOrSharedToOrg, so an org-shared member can create a profile on someone else's project and change which profile is default. Deliberately not addressed here -- gating them narrows a shipped capability on a viewset outside this PR's scope, which is a product decision. make_profile_default carries a second, separate defect worth its own ticket: at views.py:401-407 it clears is_default across the tool's profiles and then resolves the promoted profile with ProfileManager.objects.get(pk=request.data["default_profile"]) on the raw manager, so the profile being promoted is never checked against the tool it is being made default for. No test accompanied 70ff027 and none is removed here. A test would have constructed a view with action="create", passed, and mutation-checked correctly while the production path stayed ungated -- vacuous in exactly the way that hides this class of defect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
Behaviour-preserving cleanup after the fixes settled. Tests re-run and the
mutation checks re-confirmed: removing the shared_to_org branch still fails
1 test, removing check_object_permissions still fails 2.
Prose: the PromptAcesssToUser docstring had accumulated a routing table
stating, from inside a permission class, which actions two other viewsets
gate -- facts already stated at both enforcement sites, so three copies
that drift independently. Cut to the one line a reader of this class needs.
Same for the ProfileManagerView rebuttal and the empty-prompts note, whose
subject is payload validation on a route this class does not guard. The
read_write API-key gap stays: it is a load-bearing accepted-risk warning,
not restatement. The duplicate of it in views.py becomes a pointer.
Queries: hoisted `tool.shared_to_org` above the owner and viewer checks. It
is a free attribute read and each branch it now precedes runs an .exists()
query, so the org-share path -- the one UN-3315 exists to serve -- saves up
to two. Order is not otherwise observable: the method is a plain OR of
side-effect-free predicates. Added select_related("tool_id") to the reorder
lookup, since the permission class dereferences that FK immediately and the
parent is already joined by the filter.
Not taken, deliberately:
- Threading the fetched prompt through PromptStudioController into
reorder_prompts_helper to kill its duplicate SELECT. Real (one wasted
round-trip per reorder) but it changes two signatures outside this diff
and makes the controller's DoesNotExist branch dead.
- Extracting a parent-owner base class over IsParentToolOwner /
IsRegistryToolOwner / IsPromptParentToolOwner. The duplication is real
and predates this PR; consolidating touches two unchanged auth classes,
which is not scope for a permissions fix.
- Switching the inline service-account/admin checks to the shared
_is_service_account / _is_organization_admin helpers. Right in principle
and would pick up the per-request admin cache, but all three classes in
this file hand-roll them; changing one creates divergence rather than
removing it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
make_profile_default cleared is_default across the target tool's profiles and then resolved the promoted profile with ProfileManager.objects.get(pk=request.data["default_profile"]) -- the raw manager, with no check that the profile belongs to that tool. Promoting a profile from another tool therefore succeeded, and because the clear had already run, the target was left with NO default of its own and a foreign profile marked default for it. Resolves the profile first, scoped to prompt_studio_tool, and only then clears. Order matters as much as the scoping: 404-ing after the clear would still leave the tool without a default, so a refused promotion must modify nothing. The test pins the ordering, not just the filter -- moving the clear back ahead of the lookup fails it. Also replaces the bare request.data["default_profile"] KeyError (a 500 when the field is absent) with a 400. No capability change: who may call this route is unchanged. Gating it and create_profile_manager to IsOwner was proposed and reversed by the user; org-shared members keep both, as they ship today. Restores two docstring passages that 7d4f281 cut as redundant. They are not: that sync_prompts with an empty prompts list clears a project BY DESIGN is the conclusion that cost a wrong guard, a commit and a forward revert, and the next reader will re-propose that guard without it. Now says plainly not to. The note that sync_prompts is IsOwner-gated is likewise true and load-bearing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
Three tests, each mutation-checked:
A1 drop `prompt_studio_tool=` from the lookup -> fails
A2 restore the bare request.data[...] KeyError -> fails
A3 move the is_default clear back ahead of the
lookup (the original bug's ordering) -> fails
A3 is the one worth having. A test that only asserted the filter would pass
against a version that 404s after wiping the tool's default -- which is the
same broken end state, reached a different way. It asserts the clear did not
run when the promotion is refused.
Extends the over-restriction tripwire to create_profile_manager and
make_profile_default, asserting they still resolve the share-aware class.
Gating them was proposed and reversed by the user, so this pins the
reversal: current behaviour, not the abandoned change. A future edit that
sweeps every action to owner-only now fails here.
19 pass in the file; 129 across the permission tier, unchanged from before
these commits apart from the 3 added here plus the 4 extra parametrized
cases. The 53 Postgres errors in the wider run are pre-existing and
identical at the pre-PR baseline.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
PromptAcesssToUser admits org-shared members to update/partial_update while
IsPromptParentToolOwner denies them destroy -- but that gate reads the
STORED parent, which DRF loads before the update is applied. tool_id is
client-writable (fields="__all__", and the model FK leaves editable=True
unlike created_by/modified_by), so a denied user could:
PATCH /prompt/<id>/ {"tool_id": "<a tool they own>"} -> 200
DELETE /prompt/<id>/ -> 200
Two requests, each passing every check, and Alice's prompt is gone. Every
test this PR added passes throughout.
Moving a prompt out of a project removes it from that project -- a deletion
from the losing side -- so it now requires what destroy requires, checked
against the EXISTING parent. Checking the NEW parent would pass trivially:
the attacker's destination is a tool they already own, and the harm is the
prompt leaving the original project regardless of where it lands.
Three behaviours preserved deliberately, each with a test:
- An unchanged tool_id is a no-op, not a reparent. The Prompt Studio UI
PATCHes one field at a time (DocumentParser.jsx builds {[name]: value}),
so nothing in-tree is affected either way, but a payload echoing the
parent back must not 403.
- An owner reparenting between tools they own still works.
- null is a reparent, not a no-op. Orphaning the row hides it from
everyone, its owner and org admins included, once the org filter's INNER
JOIN excludes it.
A deliberate tightening, not a bug fix: a request that succeeds today will
403. Verified no legitimate caller does this -- the only two PATCH callers
in frontend/src hit tool_instance/ and workflow/endpoint/, and the prompt
PATCH never carries tool_id.
This NARROWS the bypass; it does not make tool_id unwritable. The complete
fix is a read-only field on update, declined on API-contract grounds
because DRF silently ignores read-only input -- a legitimate reparent would
get 200 and no effect. Preserved at:
unstract-pr2259-pending/fix1-tool_id-readonly-DECLINED.patch
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
get_queryset returned ToolStudioPrompt.objects.all() when unfiltered. list never calls get_object(), and neither permission class defines has_permission, so BasePermission's default True applied and nothing object-level fired: any org member could enumerate every prompt in the organization via GET /prompt/. OrganizationFilterBackend still blocked cross-org access, so the exposure was intra-org, and the list serializer limits it to prompt keys and sequence numbers -- but CustomTool.for_user deliberately hides those projects, and this route handed back their contents anyway. Both branches were open, not just the fallback: the filtered branch takes tool_id straight from the query string with no ownership check of its own, so scoping only the .all() path would have left the branch the UI actually uses exactly as it was. A test pins each. Scoped through the parent because ToolStudioPrompt has no organization field and no for_user manager -- the same route reorder_prompts already takes. This matches the sibling PromptStudioCoreView.get_queryset, which has always scoped with CustomTool.objects.for_user. Not a contract change: the response shape is identical and only the row set narrows. It is a deliberate tightening rather than a bug fix -- a caller listing prompts of a project they cannot reach stops seeing them. Note this also narrows get_object() for the detail routes, turning a 403 into a 404 for unreachable tools. That is an improvement (less enumeration) and does not weaken the deletion gate: for_user ORs in shared_to_org, so a shared member still reaches the prompt and still gets a real 403 from IsPromptParentToolOwner on destroy -- which the existing tests pin. Also corrects the get_permissions comment, which claimed reads honour project sharing. That was true for retrieve and false for list until this change; it now names both levels that enforce it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
format_suffix_patterns generates a `.json` variant of every route in this URLconf, and DRF forwards the captured `format` kwarg to the handler. reorder_prompts took only `request`, so the suffixed route raised TypeError -- a 500 raised during dispatch, before the permission check was reached. Absorbs the kwarg in the signature, which is how the DRF mixins handle the same thing (they take *args, **kwargs). Dropping the route from format_suffix_patterns was the alternative, but that call wraps every pattern in the file, so it would have changed the URLconf for routes this PR has no business touching. Fixes a broken contract rather than changing a working one: the route returns 200 where it previously 500'd, and the unsuffixed route is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
Fifteen tests across the three fixes, every one mutation-checked -- the fix
reverted, the failure observed, the file verified restored:
neuter the reparent gate (never deny) -> 2 failed
treat null tool_id as a no-op -> 1 failed
unscope get_queryset entirely -> 2 failed
scope ONLY the unfiltered branch -> 1 failed
remove **kwargs from reorder_prompts -> 1 failed
Two are over-restriction guards rather than under-restriction ones, which
is the half that is easy to omit: an owner must still be able to reparent,
and an unchanged tool_id must stay a no-op so the UI's field-level PATCH
keeps working. Both fail if the gate is widened to catch every update.
The null case earns its own test. Treating {"tool_id": null} as "unchanged"
looks reasonable and orphans the row, which the org filter's INNER JOIN
then hides from everyone including org admins -- a delete by another name.
The two list tests cover the branches separately on purpose: scoping the
.all() fallback alone leaves the filtered branch -- the one the UI uses --
exactly as open as before, and a single test over both would not have
caught that.
Direct imports rather than tests_common.source_extraction. That technique
execs method bodies out of context, so unreachable code is indistinguishable
from wired code -- and "the hook never fires" is precisely the class of bug
these fixes address.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NQhqNqCwFcXZZ7cQU6HxUE
Contributor
Author
Follow-up raised: UN-4054 —
|
|
hari-kuriakose
marked this pull request as ready for review
August 31, 2026 20:52
Contributor
|
| Filename | Overview |
|---|---|
| backend/prompt_studio/permission.py | Adds organization-sharing support to prompt read/edit authorization and introduces a parent-owner gate for deletion without exposing a reachable cross-organization path. |
| backend/prompt_studio/prompt_studio_core_v2/views.py | Makes destructive prompt synchronization owner-gated and validates default-profile ownership before changing persisted defaults. |
| backend/prompt_studio/prompt_studio_v2/views.py | Splits edit and delete permissions, scopes prompt resolution to reachable tools, explicitly authorizes reorder targets, and prevents shared users from reparenting prompts out of projects. |
| backend/prompt_studio/prompt_studio_v2/tests/test_prompt_permission_guards.py | Adds comprehensive regression coverage for the changed authorization and object-scoping behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
R[Prompt request] --> Q{Action}
Q -->|Read or edit| V[Resolve through requester-visible parent tools]
V --> S{Owner, viewer, group share, org share, or org admin?}
S -->|Yes| A[Allow]
S -->|No| D[Deny]
Q -->|Delete prompt| O{Parent owner or org admin?}
O -->|Yes| A
O -->|No| D
Q -->|Bulk sync| B{IsOwner permission passes?}
B -->|Yes| A
B -->|No| D
Reviews (1): Last reviewed commit: "Merge branch 'main' into un-sprint4-B-pe..." | Re-trigger Greptile
hari-kuriakose
requested review from
chandrasekharan-zipstack and
muhammad-ali-e
August 31, 2026 20:59
Contributor
Unstract test resultsPer-group results
Critical paths
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
UN-3315 — prompts stayed read-only in org-shared Prompt Studio projects
"Share with everyone" sets
shared_to_orgon the parentCustomTool.IsOwnerOrSharedUserOrSharedToOrgalready honoured that flag, so the project was visible to the org — butPromptAcesssToUserdid not check it, so editing a prompt inside that project stayed blocked for everyone except the owner.One file, +9/−2: adds the
shared_to_orgcheck alongside the existing owner / direct-viewer / group-share / org-admin paths.Reviewer note — this widens access
Please sanity-check the intent: after this change, any org member can edit prompts in a project shared with the whole org. That is what "share with everyone" reads as, and it matches the visibility rule already in place, but it is a genuine broadening rather than a pure bug fix — worth a second opinion before merge.
The check reads
tool.shared_to_orgdirectly, matching the sibling gate atpermissions/permission.py:248. (An earlier revision usedgetattr(tool, "shared_to_org", False)and this section claimed it "degrades safely if the attribute is absent" — that was inaccurate:shared_to_orgis a non-null model field, so the default could never apply.)Update: deletion is no longer included in the widened access.
get_permissionsonToolStudioPromptViewnow routesdestroytoIsPromptParentToolOwner, so an org-shared member can view and edit prompts but not delete them, matching the parentCustomTool.destroybeingIsOwner-gated. Reads and edits are unchanged by that split.Known gap, not closed here: the bulk
sync_promptsroute onPromptStudioCoreViewstill lets an org-shared member remove every prompt in the project (pre-existing, UN-3318). Tracked separately.🤖 Generated with Claude Code
https://claude.ai/code/session_0197gRFp4QFxu2VzxhTEozGn