-
Notifications
You must be signed in to change notification settings - Fork 716
UN-2868 [FIX] Make settings read-only and block deletion on resources shared with a user #2273
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
Open
kirtimanmishrazipstack
wants to merge
29
commits into
main
Choose a base branch
from
UN-2868-sharing-improvements
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
a70584b
UN-2868 [FIX] Restrict workflow connector and tool changes to owners …
kirtimanmishrazipstack ddd04af
UN-2868 [FIX] Show the Prompt Studio project name to shared users ins…
kirtimanmishrazipstack 06198cc
UN-2868 [FIX] Do not close the connector modal on a failed or partial…
kirtimanmishrazipstack e8b07aa
UN-2868 [FIX] Refuse form submits from view-only users, not just clicks
kirtimanmishrazipstack 639baf7
UN-2868 [FIX] Hide edit and delete actions on resources shared with t…
kirtimanmishrazipstack 6feec9d
UN-2868 [FIX] Expose is_owner on the Prompt Studio editor payload
kirtimanmishrazipstack ed1fe98
UN-2868 [FIX] Keep Share available to shared users; gate only edit an…
kirtimanmishrazipstack 8d405fc
UN-2868 [FIX] Make Prompt Studio settings read-only for shared users
kirtimanmishrazipstack 91c8404
UN-2868 [FIX] Restrict the gate to edit and delete only
kirtimanmishrazipstack b812041
UN-2868 [FIX] Show Edit and Delete disabled rather than hiding them
kirtimanmishrazipstack 8a84d68
UN-2868 [FIX] Report one outcome when the connector save also writes …
kirtimanmishrazipstack 8a22d05
Merge branch 'main' into UN-2868-sharing-improvements
kirtimanmishrazipstack f078398
Merge branch 'main' of github.com:Zipstack/unstract into UN-2868-shar…
kirtimanmishrazipstack 3e9156b
UN-2868 [FIX] Prefill the workflow rename form and limit shared-user …
kirtimanmishrazipstack 08d7e4e
Merge remote-tracking branch 'origin/UN-2868-sharing-improvements' in…
kirtimanmishrazipstack bc0d6e3
UN-2868 [FIX] Make Prompt Studio prompts and settings read-only for s…
kirtimanmishrazipstack 14df8ab
UN-2868 [FIX] Share Prompt Studio for collaboration, and gate the API…
kirtimanmishrazipstack 77c055f
Merge branch 'main' into UN-2868-sharing-improvements
kirtimanmishrazipstack d6dbc0e
Merge branch 'main' into UN-2868-sharing-improvements
kirtimanmishrazipstack 91170ec
UN-2868 [FIX] Restrict file history deletion to the workflow owner
kirtimanmishrazipstack 44a06b9
UN-2868 [FIX] Scope the endpoint list, correct the rename status, unl…
kirtimanmishrazipstack 0c9fb44
UN-2868 [FIX] Correct three docstrings in prompt_studio/permission.py
kirtimanmishrazipstack d592324
UN-2868 [FIX] Admit org-shared users to Prompt Studio; make disabled-…
kirtimanmishrazipstack 28cfde6
UN-2868 [FIX] Close the connector-credential path the endpoint-list f…
kirtimanmishrazipstack ac31510
UN-2868 [FIX] Close two more reparenting sites; simplify the endpoint…
kirtimanmishrazipstack 81f2cea
UN-2868 [FIX] Make the disabled-control tooltip work beyond Button, a…
kirtimanmishrazipstack c67c55c
UN-2868 [FIX] Guard the workflow_id alias that bypassed the tool repa…
kirtimanmishrazipstack c349a63
UN-2868 [FIX] Drop the focusable tooltip wrapper SonarCloud flagged
kirtimanmishrazipstack cb266d4
UN-2868 [FIX] Close the ungated tool-instance create, and cover the g…
kirtimanmishrazipstack File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,174 @@ | ||
| """What a shared user may and may not do on someone else's resource (UN-2868). | ||
|
|
||
| Sharing is not one rule. A Prompt Studio project is shared *for | ||
| collaboration* -- prompts, settings and LLM profiles stay editable. Every | ||
| other resource is shared *for use*. On all of them, renaming, deleting and | ||
| changing who else has access stay with the owner. | ||
|
|
||
| These exercise the real viewsets through DRF's request factory, so a gate | ||
| that exists only in a permission class -- and never reaches the route -- is | ||
| still caught. | ||
| """ | ||
|
|
||
| from typing import Any | ||
|
|
||
| from account_v2.models import User | ||
| from django.test import TestCase | ||
| from permissions.roles import ResourceRole | ||
| from permissions.tests.base import CoOwnerOrgTestMixin | ||
| from rest_framework import status | ||
| from rest_framework.response import Response | ||
| from rest_framework.test import APIRequestFactory, force_authenticate | ||
| from tool_instance_v2.views import ToolInstanceViewSet | ||
| from workflow_manager.endpoint_v2.models import WorkflowEndpoint | ||
| from workflow_manager.endpoint_v2.views import WorkflowEndpointViewSet | ||
| from workflow_manager.workflow_v2.models.workflow import Workflow | ||
|
|
||
|
|
||
| class SharedWorkflowEndpointTests(CoOwnerOrgTestMixin, TestCase): | ||
| """A workflow is shared for use: its connector config is owner-only.""" | ||
|
|
||
| def setUp(self) -> None: | ||
| self._seed_org() | ||
| self.workflow = Workflow.objects.create( | ||
| workflow_name="wf-endpoint", organization=self.org, created_by=self.owner | ||
| ) | ||
| self.workflow.memberships.create(user=self.owner, role=ResourceRole.OWNER) | ||
| self.workflow.memberships.create(user=self.viewer, role=ResourceRole.VIEWER) | ||
| self.endpoint = WorkflowEndpoint.objects.create( | ||
| workflow=self.workflow, | ||
| endpoint_type=WorkflowEndpoint.EndpointType.DESTINATION, | ||
| connection_type=WorkflowEndpoint.ConnectionType.FILESYSTEM, | ||
| ) | ||
| self.factory = APIRequestFactory() | ||
|
|
||
| def _patch(self, actor: User) -> Response: | ||
| view = WorkflowEndpointViewSet.as_view({"patch": "partial_update"}) | ||
| request = self.factory.patch( | ||
| "/x/", {"configuration": {"path": "/changed"}}, format="json" | ||
| ) | ||
| force_authenticate(request, user=actor) | ||
| return view(request, pk=str(self.endpoint.pk)) | ||
|
|
||
| def _delete(self, actor: User) -> Response: | ||
| view = WorkflowEndpointViewSet.as_view({"delete": "destroy"}) | ||
| request = self.factory.delete("/x/") | ||
| force_authenticate(request, user=actor) | ||
| return view(request, pk=str(self.endpoint.pk)) | ||
|
|
||
| def _read(self, actor: User) -> Response: | ||
| view = WorkflowEndpointViewSet.as_view({"get": "retrieve"}) | ||
| request = self.factory.get("/x/") | ||
| force_authenticate(request, user=actor) | ||
| return view(request, pk=str(self.endpoint.pk)) | ||
|
|
||
| def test_shared_viewer_cannot_change_connector_config(self) -> None: | ||
| self.assertEqual(self._patch(self.viewer).status_code, status.HTTP_403_FORBIDDEN) | ||
|
|
||
| def test_shared_viewer_cannot_delete_the_endpoint(self) -> None: | ||
| self.assertEqual( | ||
| self._delete(self.viewer).status_code, status.HTTP_403_FORBIDDEN | ||
| ) | ||
| self.assertTrue(WorkflowEndpoint.objects.filter(pk=self.endpoint.pk).exists()) | ||
|
|
||
| def test_shared_viewer_can_still_read_it(self) -> None: | ||
| # Refusing the write must not also hide the resource. | ||
| self.assertEqual(self._read(self.viewer).status_code, status.HTTP_200_OK) | ||
|
|
||
| def test_owner_and_co_owner_can_change_it(self) -> None: | ||
| self.workflow.memberships.create(user=self.coowner, role=ResourceRole.OWNER) | ||
| for actor in (self.owner, self.coowner): | ||
| self.assertEqual(self._patch(actor).status_code, status.HTTP_200_OK) | ||
|
|
||
| def test_a_user_with_no_access_gets_404_not_403(self) -> None: | ||
| # 403 would confirm the endpoint exists to someone who cannot see it. | ||
| self.assertEqual( | ||
| self._patch(self.outsider).status_code, status.HTTP_404_NOT_FOUND | ||
| ) | ||
|
|
||
|
|
||
| class SharedWorkflowToolInstanceTests(CoOwnerOrgTestMixin, TestCase): | ||
| """Attaching a tool mutates the workflow -- and activates it.""" | ||
|
|
||
| def setUp(self) -> None: | ||
| self._seed_org() | ||
| self.workflow = Workflow.objects.create( | ||
| workflow_name="wf-tools", | ||
| organization=self.org, | ||
| created_by=self.owner, | ||
| is_active=False, | ||
| ) | ||
| self.workflow.memberships.create(user=self.owner, role=ResourceRole.OWNER) | ||
| self.workflow.memberships.create(user=self.viewer, role=ResourceRole.VIEWER) | ||
| self.factory = APIRequestFactory() | ||
|
|
||
| def _create(self, actor: User) -> Response: | ||
| view = ToolInstanceViewSet.as_view({"post": "create"}) | ||
| request = self.factory.post( | ||
| "/x/", | ||
| {"workflow_id": str(self.workflow.pk), "tool_id": "tool-uid"}, | ||
| format="json", | ||
| ) | ||
| force_authenticate(request, user=actor) | ||
| return view(request) | ||
|
|
||
| def test_shared_viewer_cannot_add_a_tool(self) -> None: | ||
| response = self._create(self.viewer) | ||
| self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) | ||
|
|
||
| def test_a_user_with_no_access_gets_404(self) -> None: | ||
| self.assertEqual( | ||
| self._create(self.outsider).status_code, status.HTTP_404_NOT_FOUND | ||
| ) | ||
|
|
||
|
|
||
| class SharedPromptStudioProjectTests(CoOwnerOrgTestMixin, TestCase): | ||
| """Prompt Studio is shared for collaboration; only the name is owner-only.""" | ||
|
|
||
| def setUp(self) -> None: | ||
| self._seed_org() | ||
| from prompt_studio.prompt_studio_core_v2.models import CustomTool | ||
|
|
||
| self.tool = CustomTool.objects.create( | ||
| tool_name="ps-project", | ||
| description="collaboration test", | ||
| organization=self.org, | ||
| created_by=self.owner, | ||
| ) | ||
| self.tool.memberships.create(user=self.owner, role=ResourceRole.OWNER) | ||
| self.tool.memberships.create(user=self.viewer, role=ResourceRole.VIEWER) | ||
| self.factory = APIRequestFactory() | ||
|
|
||
| def _patch(self, actor: User, payload: dict[str, Any]) -> Response: | ||
| from prompt_studio.prompt_studio_core_v2.views import PromptStudioCoreView | ||
|
|
||
| view = PromptStudioCoreView.as_view({"patch": "partial_update"}) | ||
| request = self.factory.patch("/x/", payload, format="json") | ||
| force_authenticate(request, user=actor) | ||
| return view(request, pk=str(self.tool.pk)) | ||
|
|
||
| def test_shared_user_cannot_rename_the_project(self) -> None: | ||
| response = self._patch(self.viewer, {"tool_name": "renamed-by-viewer"}) | ||
| self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) | ||
| self.tool.refresh_from_db() | ||
| self.assertEqual(self.tool.tool_name, "ps-project") | ||
|
|
||
| def test_shared_user_can_change_a_settings_field(self) -> None: | ||
| # Same endpoint as the rename, so the gate has to be per-field. | ||
| response = self._patch(self.viewer, {"preamble": "set by a collaborator"}) | ||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
| self.tool.refresh_from_db() | ||
| self.assertEqual(self.tool.preamble, "set by a collaborator") | ||
|
|
||
| def test_owner_can_rename(self) -> None: | ||
| response = self._patch(self.owner, {"tool_name": "renamed-by-owner"}) | ||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
| self.tool.refresh_from_db() | ||
| self.assertEqual(self.tool.tool_name, "renamed-by-owner") | ||
|
|
||
| def test_resending_the_same_name_is_not_a_rename(self) -> None: | ||
| # A settings PATCH that echoes the current name must not be refused. | ||
| response = self._patch( | ||
| self.viewer, {"tool_name": "ps-project", "postamble": "echoed"} | ||
| ) | ||
| self.assertEqual(response.status_code, status.HTTP_200_OK) |
Oops, something went wrong.
Oops, something went wrong.
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.
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 4 — Security] — The gate validates the current parent, never the target parent, and
workflowis writable.IsParentWorkflowOwner.has_object_permissionresolves ownership fromobj.workflow— the parent the row has now.WorkflowEndpointSerializerisfields = "__all__"with onlyworkflow_namemarked read-only, soworkflowitself is a writable FK.Failure mode: the owner of workflow A sends
PATCH /workflow/endpoint/<A-endpoint>/with{"workflow": "<B-id>"}. The object check passes (they do own A), then the save reassigns the row to B, owned by a different user in the same org. B's owner now executes against A's connector instance, bucket, or table.That is the customer-reported bug — a shared user repointing another team's workflow at a different output folder — still reachable after the fix that targets it. The same shape applies to the cloud companion:
RuleEngineSerializer.read_only_fieldsis["id", "created_by", "modified_by"]andSettingsSerializerisfields = "__all__", so both HITL viewsets inherit it.Fix: mark
workflowread-only onWorkflowEndpointSerializer(and on the two cloud HITL serializers), or validate in the mixin thatvalidated_data.get("workflow")equalsinstance.workflowon update.Two notes in this class's favour, verified while checking:
perform_createcorrectly fails closed on a missingworkflow, and the MRO order is right at every current call site. The base-order fragility (a bare mixin overridingget_permissions/perform_create, which DRF also defines) is a trap for the next viewset that copies it, not a live bug — worth an__init_subclass__assertion given this is now a cross-repo contract.Confidence: High on the field config (verified); the reassignment PATCH was not executed against a live DB.
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.
@chandrasekharan-zipstack Half of this held and half didn't — and the half that held is now closed at eight sites, not one.
The OSS half is disconfirmed.
WorkflowEndpointSerializer.workflowis not writable:WorkflowEndpoint.workflowiseditable=False(models.py:34), which DRF honours by forcingread_only=True. I checked by building the serializer rather than readingMeta— the only writable fields areconfiguration,connection_typeandconnector_instance_id. So the reparenting PATCH you describe can't land there. You'd flagged your own uncertainty on exactly this ("the reassignment PATCH was not executed against a live DB"); the field-config half is the part that was off.The cloud half was live and is fixed (1d9c4f6a). Both
RuleEngineSerializer.workflowandSettingsSerializer.workflowbuiltread_only=False. One correction worth recording: onlySettingsSerializerwas actually exploitable —RuleEngineSerializerhas a customupdate()that assigns onlypercentage/rule_string/rule_json/rule_logicand never touchesworkflow. The guard there is defence-in-depth, not a closed hole, and I've said so in the commit so nobody removes it later believing it was load-bearing.The class is wider than either of us listed. Sweeping by gate rather than by field found eight sites: RuleEngine, HITLSettings, ToolInstance, ProfileManager, APIKey (
apiandpipeline), ToolStudioPrompt, and WorkflowEndpoint — the last safe only by thateditable=False. All closed across 1d9c4f6a / 28cfde6 / ac31510 / c67c55c.Two of those are worth your attention:
APIKeySerializer.api/.pipelinewere writable on PATCH.createspends ~60 lines refusing a path-vs-body mismatch;updatehad nothing, so a live key could be retargeted onto a deployment the caller doesn't own.ToolInstance.workflow's attname isworkflow_id, and the serializer declares a writableworkflow_idalias.validate_workflowfires only on the keyworkflow; a body of{"workflow_id": ...}skipped it entirely and DRF'ssetattr(instance, "workflow_id", uuid)wrote the FK column directly.That last one is the argument for your
__init_subclass__suggestion, or better: hoisting the check intoWorkflowOwnerMutationMixinso it covers attname aliases. Enumerating this class by field name provably cannot close it. I did not do that refactor — it touches eight viewsets across both repos and I didn't want it unreviewed at the end of an automated run. Recommended in the commit message.