-
Notifications
You must be signed in to change notification settings - Fork 716
UN-3315 [FIX] Honour shared_to_org for Prompt Studio prompt edits #2259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
79c985b
943b3a0
4d8a381
0a96979
75fd2ac
5bc4ee9
c35fbe7
70ff027
7e6b1cd
6c7c186
54f5fa3
7d4f281
c3a0316
615b16e
636cedf
4fdc528
6d0a5bb
4274af6
ca40729
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,18 +12,55 @@ | |
|
|
||
|
|
||
| class PromptAcesssToUser(permissions.BasePermission): | ||
| """Is the crud to Prompt/Notes allowed to user. | ||
| """Read and edit access to a Prompt/Note, inherited from the parent tool. | ||
|
|
||
| A user qualifies when they own the parent ``CustomTool``, are a direct | ||
| viewer (VIEWER membership, UN-2202), reach the project via group sharing | ||
| (``ResourceGroupShare`` on the parent tool), or are an org admin | ||
| (org-wide admin override, UN-3479). | ||
| (``ResourceGroupShare`` on the parent tool), reach it because the parent | ||
| tool is shared with the whole org (``shared_to_org``, UN-3315), or are an | ||
| org admin (org-wide admin override, UN-3479). | ||
|
|
||
| Deliberately broader than the workflow rule stated in | ||
| ``permissions.permission.is_workflow_mutator`` ("shared access grants read | ||
| only, never mutate"): a Prompt Studio share confers *edit* rights on the | ||
| project's prompts, matching ``CustomToolViewSet``, which already routes | ||
| ``update``/``partial_update`` on the tool itself through | ||
| ``IsOwnerOrSharedUserOrSharedToOrg``. ``ProfileManagerView`` looks like a | ||
| counterexample -- it routes ``update``/``partial_update``/``destroy`` | ||
| through ``IsParentToolOwner`` -- but it is not: profiles are *created* | ||
| through ``PromptStudioCoreView.create_profile_manager``, which admits | ||
| org-shared users, as does ``make_profile_default``. That surface is | ||
| share-permissive and unaddressed here. | ||
|
|
||
| This class does not confer deletion; ``destroy`` is gated by | ||
| :class:`IsPromptParentToolOwner`, and the bulk ``sync_prompts`` route on | ||
| ``PromptStudioCoreView`` is likewise ``IsOwner``-gated. | ||
|
|
||
| One deletion path remains open to a non-owner, known and accepted | ||
| (UN-3315): a ``read_write`` platform API key reaches ``sync_prompts``. | ||
| Service accounts short-circuit ahead of every check here, and being a POST | ||
| that route is not covered by the DELETE tier that guards per-prompt | ||
| ``destroy``. | ||
|
|
||
| Separately, and not a hole: ``sync_prompts`` with an empty ``prompts`` | ||
| list clears a project's prompts by design -- supported behaviour, asserted | ||
| by ``test_sync_prompts_clear_bumps_tool_modified_at``. Do not "fix" it with | ||
| a payload guard; that breaks a published contract and its own test. The | ||
| owner gate, not payload validation, is what stands between a share and | ||
| that wipe. | ||
| """ | ||
|
|
||
| def has_object_permission(self, request: Request, view: APIView, obj: Any) -> bool: | ||
| if getattr(request.user, "is_service_account", False): | ||
| return True | ||
| tool = obj.tool_id | ||
| # UN-3315: "Share with everyone" sets shared_to_org on the parent tool. | ||
| # Checked first among the grant paths because it is a free attribute | ||
| # read, while every branch below it runs a query -- and it is the path | ||
| # UN-3315 exists to serve. Order is not otherwise observable: these are | ||
| # side-effect-free predicates OR'd together. | ||
| if tool.shared_to_org: | ||
| return True | ||
| if _is_resource_owner(request.user, tool): | ||
| return True | ||
| if _is_resource_viewer(request.user, tool): | ||
|
|
@@ -33,6 +70,35 @@ def has_object_permission(self, request: Request, view: APIView, obj: Any) -> bo | |
| return OrganizationMemberService.is_user_organization_admin(request.user) | ||
|
|
||
|
|
||
| class IsPromptParentToolOwner(permissions.BasePermission): | ||
| """Deletion gate for Prompt Studio prompts/notes. | ||
|
|
||
| Mirrors ``permissions.permission.IsParentToolOwner``, which does the same | ||
| for ``ProfileManager``, but reads the parent through ``ToolStudioPrompt``'s | ||
| own FK name (``tool_id``) rather than ``prompt_studio_tool``. Kept as a | ||
| separate class rather than teaching the shared one to juggle both attribute | ||
| names: a shared authorization class that accumulates per-caller special | ||
| cases is how these gates drift apart. | ||
|
|
||
| Exists because the parent ``CustomTool``'s own ``destroy`` is owner-only | ||
| (``IsOwner`` in ``CustomToolViewSet.get_permissions``). Without this, | ||
| UN-3315's org-wide share would let any org member delete every prompt | ||
| inside a project they cannot themselves delete. | ||
|
|
||
| ``tool_id`` is nullable (``SET_NULL``), so an orphaned prompt whose parent | ||
| tool was deleted falls back to the org-admin check -- it has no owner to | ||
| inherit from. | ||
|
Comment on lines
+76
to
+90
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [High] [Lens 16 — Doc accuracy] — This docstring claims parity with
|
||
| """ | ||
|
|
||
| def has_object_permission(self, request: Request, view: APIView, obj: Any) -> bool: | ||
| if getattr(request.user, "is_service_account", False): | ||
| return True | ||
| tool = obj.tool_id | ||
| if tool is not None and _is_resource_owner(request.user, tool): | ||
| return True | ||
| return OrganizationMemberService.is_user_organization_admin(request.user) | ||
|
Comment on lines
+73
to
+99
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [High] [Lens 2 — Architectural fit] — Fifth near-clone parent-owner permission class, justified by an anti-drift argument the existing four already falsifyThe docstring argues the clone exists so a shared class won't "drift apart". But the four pre-existing clones have already drifted, on three separate axes — so a future security fix lands in only some of them:
The count, for the record: Fix: parameterise — Confidence: High. |
||
|
|
||
|
|
||
| class IsRegistryToolOwner(permissions.BasePermission): | ||
| """Is unpublishing an exported tool allowed to user. | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[High] [Lens 16 — Doc accuracy] — Three comments cite
CustomToolViewSet, a class that does not existAlso at
permission.py:84andprompt_studio_v2/views.py:44.All three carry the entire justification for this PR's central decision — "edits honour sharing, deletes do not, because the parent tool viewset does the same." A maintainer auditing that premise greps
CustomToolViewSet, finds only these three comments referencing each other, and cannot verify it. One who concludes the class was deleted may decide the parity argument no longer holds and "re-align" the gate.grep -rn "CustomToolViewSet" backend --include="*.py"returns exactly those three comment lines. The real class isPromptStudioCoreView(prompt_studio_core_v2/views.py:131) — the class whoseget_permissionsthis PR edits at:157-170.Fix: replace all three with
PromptStudioCoreView. The substantive claims are correct once the name is fixed.Confidence: High.