UN-3815 [FIX] Apply organization scoping to prompt-studio child models - #2213
UN-3815 [FIX] Apply organization scoping to prompt-studio child models#2213athul-rs wants to merge 6 commits into
Conversation
Summary by CodeRabbit
WalkthroughThe changes enforce organization-aware querying, constrain Prompt Studio lookups, remove file deletion routes, narrow row-locking behavior, and add organization-path and cross-organization isolation tests. ChangesOrganization isolation and access control
Endpoint and transaction changes
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| backend/utils/models/org_path_discovery.py | Pins stable organization relationship paths for the five newly scoped Prompt Studio child models. |
| backend/prompt_studio/prompt_profile_manager_v2/models.py | Makes profile queries organization-aware while preserving the existing user-sharing manager behavior. |
| backend/prompt_studio/prompt_studio_core_v2/views.py | Tool-scopes request-supplied document and profile identifiers and makes default-profile replacement atomic. |
| backend/utils/organization_utils.py | Changes the organization-scoping helper to fail closed when organization context is absent or unresolvable. |
| backend/prompt_studio/tests/test_cross_org_isolation.py | Adds regression coverage for cross-organization manager access, explicit parent scoping, worker context, and the removed route. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Request[Request or worker context] --> Org[Current organization]
Org --> Manager[OrgAwareManager]
Manager --> Path[get_org_path]
Path --> Override[Stable pinned FK path]
Override --> Children[Prompt Studio child queryset]
Children --> Tool[Organization-owned CustomTool or adapter]
Action[Custom action with request-supplied ID] --> ParentScope[Explicit tool or organization predicate]
ParentScope --> Children
Reviews (6): Last reviewed commit: "Merge branch 'main' into UN-3794-org-sco..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/file_management/views.py (1)
28-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale docstring still advertises DELETE.
The class docstring still says the viewset "Handles GET,POST,PUT,PATCH and DELETE" but the delete action (and its URL route) is now gone.
✏️ Proposed docstring fix
"""FileManagement view. - Handles GET,POST,PUT,PATCH and DELETE + Handles GET, POST, PUT, and PATCH """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/file_management/views.py` around lines 28 - 33, Update the FileManagementViewSet class docstring to remove DELETE from the listed supported operations, leaving only the methods and actions still exposed by the viewset.backend/prompt_studio/tests/test_cross_org_isolation.py (1)
100-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo
tearDownto resetUserContextafter mutating tests.
test_no_org_context_is_unfiltered(Line 143) sets the org identifier toNone, andtest_worker_context_sees_its_own_org(Line 151) sets it to org B; neither is restored. SinceUserContextlooks like process-level/thread-local state (not something Django's transactionalTestCaserolls back), whichever of these runs last leaves stale org context for the next test class in the same run.♻️ Proposed fix
def setUp(self) -> None: self.a = OrgFixture(f"org-a-{secrets.token_hex(3)}") self.b = OrgFixture(f"org-b-{secrets.token_hex(3)}") # End state: acting as org A, as a request would. UserContext.set_organization_identifier(self.a.org.organization_id) + + def tearDown(self) -> None: + UserContext.set_organization_identifier(None)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/prompt_studio/tests/test_cross_org_isolation.py` around lines 100 - 104, Update the test fixture class containing setUp, OrgFixture, and the affected isolation tests with a tearDown method that clears or restores UserContext’s organization identifier after every test. Ensure tests that mutate the context, including test_no_org_context_is_unfiltered and test_worker_context_sees_its_own_org, cannot leak state into subsequent tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/prompt_studio/prompt_studio_core_v2/views.py`:
- Around line 446-453: The default-profile update flow should resolve the target
ProfileManager before clearing the current default, so invalid or cross-tool IDs
leave existing state unchanged. In the relevant view method, move the
get_object_or_404 lookup for prompt_tool and request.data["default_profile"]
ahead of the reset, then wrap target validation and both default updates in
transaction.atomic().
In `@backend/prompt_studio/prompt_studio_output_manager_v2/views.py`:
- Around line 127-132: Update fetch_default_output_response() after the
organization-scoped ToolStudioPrompt.objects.filter() lookup to explicitly
detect an empty queryset and raise the existing tool-not-found error. Preserve
the scoped tool_id and organization filters, and continue using the queryset for
valid tools.
---
Nitpick comments:
In `@backend/file_management/views.py`:
- Around line 28-33: Update the FileManagementViewSet class docstring to remove
DELETE from the listed supported operations, leaving only the methods and
actions still exposed by the viewset.
In `@backend/prompt_studio/tests/test_cross_org_isolation.py`:
- Around line 100-104: Update the test fixture class containing setUp,
OrgFixture, and the affected isolation tests with a tearDown method that clears
or restores UserContext’s organization identifier after every test. Ensure tests
that mutate the context, including test_no_org_context_is_unfiltered and
test_worker_context_sees_its_own_org, cannot leak state into subsequent tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 99c5a213-933f-481f-a966-d43ed583fd72
📒 Files selected for processing (15)
backend/file_management/urls.pybackend/file_management/views.pybackend/prompt_studio/prompt_profile_manager_v2/models.pybackend/prompt_studio/prompt_studio_core_v2/migration_utils.pybackend/prompt_studio/prompt_studio_core_v2/views.pybackend/prompt_studio/prompt_studio_document_manager_v2/models.pybackend/prompt_studio/prompt_studio_index_manager_v2/models.pybackend/prompt_studio/prompt_studio_index_manager_v2/prompt_studio_index_helper.pybackend/prompt_studio/prompt_studio_output_manager_v2/models.pybackend/prompt_studio/prompt_studio_output_manager_v2/views.pybackend/prompt_studio/prompt_studio_v2/models.pybackend/prompt_studio/tests/__init__.pybackend/prompt_studio/tests/test_cross_org_isolation.pybackend/utils/models/org_path_discovery.pybackend/utils/tests/test_org_path_discovery.py
💤 Files with no reviewable changes (1)
- backend/file_management/urls.py
athul-rs
changed the title
get_org_path resolves the shortest FK chain from a model to Organization and breaks ties by field declaration order. Reordering two fields can therefore swap in a different path of the same length, and if that path runs through a nullable FK the org filter becomes an INNER JOIN that silently drops every row with a NULL — which reads as missing records rather than as an error. Pin the five prompt-studio models to their currently resolved paths so both consumers (OrgAwareManager and OrganizationFilterBackend) are frozen on the same value, and add tests that fail if a pin drifts from discovery or starts traversing a nullable FK. ProfileManager resolves to vector_store__organization rather than prompt_studio_tool__organization: BFS reaches AdapterInstance (which carries the organization FK) before CustomTool, and prompt_studio_tool is nullable, so pinning there would drop tool-less profiles.
Custom DRF @action methods never call filter_queryset(), so OrganizationFilterBackend does not run on them and a raw .objects lookup inside one carries no organization predicate. Five prompt-studio models have no organization FK and used a plain manager, leaving roughly 44 such call sites relying on the caller to pass a correct id. - Scope at the model layer: OrgAwareManager on DocumentManager, IndexManager, PromptStudioOutputManager, ToolStudioPrompt and ProfileManager. No migration — no manager sets use_in_migrations, so swapping objects serializes nothing. - Scope the lookups that take an id straight from the request: delete_for_ide now requires the document to belong to the tool the caller already passed authz on, get_output_for_tool_default filters prompts by organization, and make_profile_default constrains its secondary lookup to the same tool. All three use get_object_or_404 so a non-matching id is a 404 rather than an unhandled DoesNotExist, which the DRF handler would turn into a 500. - Drop the file/delete route and action: it has no caller, and it deleted a document over GET. - select_for_update(of=("self",)) where the org filter now adds joins, so Postgres does not also lock rows in DocumentManager, CustomTool or AdapterInstance. Tests cover the org isolation matrix, same-org access, worker context (org is set there, so the manager filters) and the no-org fail-open path.
…aults make_profile_default cleared is_default across every profile on the tool and only then resolved the id from the request body. A non-matching id left the tool with no default at all, and the two writes were not in a transaction. Resolve first, then clear and set inside a single transaction, so a rejected id changes nothing. Adds a regression test for that, plus a tearDown resetting the thread-local UserContext (TestCase rollback does not clear it, so the org-switching tests leaked into later classes) and drops DELETE from the FileManagement docstring now the route is gone.
65613a0 to
14f94cd
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/prompt_studio/tests/test_cross_org_isolation.py (1)
163-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the actions, not only their ORM predicates.
These tests recreate the intended lookups directly, so they cannot catch a regression in
delete_for_ideormake_profile_default’s HTTP 404 mapping or mutation order. Add authenticated action requests that assert 404 and preserve the original default profile.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/prompt_studio/tests/test_cross_org_isolation.py` around lines 163 - 207, Add authenticated HTTP action tests covering delete_for_ide and make_profile_default, rather than only direct DocumentManager/ProfileManager lookups. Use cross-organization or cross-tool IDs, assert each endpoint returns 404, and verify the target tool’s existing default profile remains unchanged after each rejected request.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/prompt_studio/tests/test_cross_org_isolation.py`:
- Around line 163-207: Add authenticated HTTP action tests covering
delete_for_ide and make_profile_default, rather than only direct
DocumentManager/ProfileManager lookups. Use cross-organization or cross-tool
IDs, assert each endpoint returns 404, and verify the target tool’s existing
default profile remains unchanged after each rejected request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b3f6192-48dd-4517-b09f-875acf509d01
📒 Files selected for processing (15)
backend/file_management/urls.pybackend/file_management/views.pybackend/prompt_studio/prompt_profile_manager_v2/models.pybackend/prompt_studio/prompt_studio_core_v2/migration_utils.pybackend/prompt_studio/prompt_studio_core_v2/views.pybackend/prompt_studio/prompt_studio_document_manager_v2/models.pybackend/prompt_studio/prompt_studio_index_manager_v2/models.pybackend/prompt_studio/prompt_studio_index_manager_v2/prompt_studio_index_helper.pybackend/prompt_studio/prompt_studio_output_manager_v2/models.pybackend/prompt_studio/prompt_studio_output_manager_v2/views.pybackend/prompt_studio/prompt_studio_v2/models.pybackend/prompt_studio/tests/__init__.pybackend/prompt_studio/tests/test_cross_org_isolation.pybackend/utils/models/org_path_discovery.pybackend/utils/tests/test_org_path_discovery.py
💤 Files with no reviewable changes (1)
- backend/file_management/urls.py
🚧 Files skipped from review as they are similar to previous changes (9)
- backend/prompt_studio/prompt_studio_output_manager_v2/views.py
- backend/prompt_studio/prompt_studio_core_v2/migration_utils.py
- backend/prompt_studio/prompt_studio_document_manager_v2/models.py
- backend/prompt_studio/prompt_profile_manager_v2/models.py
- backend/prompt_studio/prompt_studio_index_manager_v2/prompt_studio_index_helper.py
- backend/prompt_studio/prompt_studio_index_manager_v2/models.py
- backend/utils/tests/test_org_path_discovery.py
- backend/file_management/views.py
- backend/prompt_studio/prompt_studio_v2/models.py
…ol_default filter() does not raise ObjectDoesNotExist, so the except branch could never fire and the tool-not-found message was dead. Empty is the right result here anyway: it covers a missing tool, an out-of-org tool, and a newly created project that has no prompts yet, which is a normal state that must not 400.
filter_queryset_by_organization returned the queryset unfiltered when the request carried no organization context, which is the opposite of what a scoping helper should do — and its own docstring already claimed it returned an empty queryset. Six internal viewsets set skip_org_filter = True, which disables OrganizationFilterBackend and leaves this helper as their only tenant boundary across roughly 39 call sites. The internal auth middleware logs a warning and continues when X-Organization-ID is missing, so any caller holding the internal service key reached those endpoints without context by omitting the header, reading across every organization — and through the file-execution viewset, writing and deleting too. Return none() instead, and log loudly, so a caller that legitimately has no context is visible rather than silently served everything. Deliberately not rejecting header-less /internal/ requests in the middleware: the leader-elected reaper calls without the header on purpose, to scan across organizations. It queries the model directly rather than through this helper, so failing closed leaves it working.
|
@greptileai please review this |
|
@greptileai re-review this PR |
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/prompt_studio/prompt_studio_core_v2/views.py (1)
453-456: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReturn 400 when
default_profileis missing.
request.data["default_profile"]raisesKeyErrorwhen the field is omitted. Django REST Framework then returns a 500 response. Validate the field with a serializer or use.get()and return a 400 response before the scoped lookup.Proposed fix
+ default_profile_id = request.data.get("default_profile") + if default_profile_id is None: + return Response( + {"detail": "default_profile is required."}, + status=status.HTTP_400_BAD_REQUEST, + ) + profile_manager = get_object_or_404( ProfileManager, - pk=request.data["default_profile"], + pk=default_profile_id, prompt_studio_tool=prompt_tool, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/prompt_studio/prompt_studio_core_v2/views.py` around lines 453 - 456, Update the view logic around the ProfileManager lookup to validate that default_profile is present before accessing request.data["default_profile"]. Return a 400 response when it is omitted, while preserving the existing scoped lookup through prompt_studio_tool for valid values; use the view’s established validation or error-response pattern.
🧹 Nitpick comments (1)
backend/prompt_studio/prompt_studio_core_v2/views.py (1)
137-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark the class configuration as
ClassVar.Ruff reports RUF012 for both mutable class attributes. Add
ClassVarannotations to make the shared viewset configuration explicit without changing behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/prompt_studio/prompt_studio_core_v2/views.py` around lines 137 - 139, Annotate the mutable ordering and ordering_fields class attributes in the surrounding viewset with ClassVar, importing ClassVar from typing if needed. Preserve their existing list values and behavior while satisfying Ruff RUF012.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/prompt_studio/prompt_studio_core_v2/views.py`:
- Around line 453-456: Update the view logic around the ProfileManager lookup to
validate that default_profile is present before accessing
request.data["default_profile"]. Return a 400 response when it is omitted, while
preserving the existing scoped lookup through prompt_studio_tool for valid
values; use the view’s established validation or error-response pattern.
---
Nitpick comments:
In `@backend/prompt_studio/prompt_studio_core_v2/views.py`:
- Around line 137-139: Annotate the mutable ordering and ordering_fields class
attributes in the surrounding viewset with ClassVar, importing ClassVar from
typing if needed. Preserve their existing list values and behavior while
satisfying Ruff RUF012.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ae00466-ec2f-47b0-a872-58ea3d565c73
📒 Files selected for processing (1)
backend/prompt_studio/prompt_studio_core_v2/views.py
Unstract test resultsPer-group results
Critical paths
|
chandrasekharan-zipstack
left a comment
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Standardized review — PR #2213
Verdict: REQUEST CHANGES
Summary — Critical: 0 · High: 4 · Medium: 9 · Low: 9 · Lenses run: 16/16
Reviewed under the unstract:standard-review 16-lens rubric. Findings below are deduplicated against the existing CodeRabbit and Greptile threads — anything already raised there is not repeated. Specifically not re-raised:
- CodeRabbit's "exercise the actions, not only their ORM predicates" on
test_cross_org_isolation.py— I agree with it and rate it higher than Trivial; only the part it did not cover (themake_profile_defaultordering fix having vacuous coverage) is filed below. - Greptile's "shared adapters hide tool profiles" on
org_path_discovery.py:47— the author's rebuttal is correct;AdapterInstanceModelManagerdoes scope every sharing path to the org. Closed on the merits. - CodeRabbit's
get_output_for_tool_defaultempty-200 thread, which the author answered and CodeRabbit accepted. Only the third cause of empty that the thread never discussed is filed below. - CodeRabbit's stale-DELETE docstring on
file_management/views.py. Residual nit: the replacement line now readsHandles GET, POST, PUT and PATCH, buturls.pyroutes only GET and POST — noupdate/partial_updateexists on the viewset.
Lens checklist
| # | Lens | Result |
|---|---|---|
| 1 | Spec & intent | Clean |
| 2 | Architectural fit | See H2, M7 |
| 3 | Correctness & edge cases | See H4, M1, M3, M5, M6, M9 |
| 4 | Security | See H2 |
| 5 | Data integrity & migrations | See M1, M2 |
| 6 | Concurrency | Clean — of=("self",) rationale verified correct at both sites; positive filters give INNER JOINs, so no nullable-outer-join hazard |
| 7 | API & contract compatibility | See H1 |
| 8 | Reliability & resilience | See H1 |
| 9 | Performance & cost | Clean |
| 10 | Observability | See H4 |
| 11 | Operational safety | See H1 — no flag, no deploy gate |
| 12 | LLM/agent | N/A — no model calls touched |
| 13 | Testing | See H3, M2 |
| 14 | Dependencies & build | N/A — none changed. Confirmed no migration needed: no manager sets use_in_migrations |
| 15 | Code quality | Low only |
| 16 | Doc & comment accuracy | See M8, M9, and Lows |
Unanchored findings (outside the diff hunks)
[High] [Lens 8, 11] — validate_tool_instances_internal returns success: true having validated nothing. backend/tool_instance_v2/internal_views.py:337-397. Function-based @api_view, no filter backend, so filter_queryset_by_organization is its only scoping. Header-less, tool_instances is now empty, the loop never runs, validation_errors stays empty, and it returns HTTP 200 {"success": true, "errors": []}. The adapter-ID migration inside that loop (:355-360) is skipped too. Worker side, workers/shared/workflow/execution/tool_validation.py:120-133 then logs ✅ Validated N tool instances successfully using the requested count, not len(validated_instances). This is the sharpest instance of H1 and the reason I would argue H1 up to Critical if any deployed worker can omit the header.
[Medium] [Lens 3] — import_prompts attaches profile_manager=None to every imported prompt. backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py:3014-3016 → :3062. ProfileManager.objects.filter(...).first() is now org-scoped and returns None rather than raising when the filter empties; there is no None check before :3062 passes it into every ToolStudioPrompt.objects.create(...). The sibling sync_prompts at :3146-3153 does raise on exactly this — the omission looks accidental rather than deliberate.
[Medium] [Lens 5] — Org-scoped .delete() in sync_prompts can leave survivors. prompt_studio_helper.py:3159-3161. ToolStudioPrompt.objects.filter(tool_id=tool).delete() is now filtered by tool_id__organization; prompts the scope misses survive alongside their recreated replacements inside the same transaction.atomic(). deleted_count is only used to decide whether to bump modified_at, never to verify the delete was complete.
[Medium] [Lens 3] — check_files_history reads org from the header but sets it from the body. backend/workflow_manager/internal_views.py:2584-2595 installs request.data["organization_id"] into StateStore, but filter_queryset_by_organization reads request.organization_id, populated only from the header. A body-only caller previously worked and now gets .none() → Workflow.DoesNotExist → 404 "not found or access denied", which blames authorization for a context-plumbing mismatch inside one function.
Low (9)
backend/prompt_studio/tests/test_cross_org_isolation.py:112, :156, :163, :208— review-artifact tags (A-1,A-3,A-4,A-5,B1, "the reported call sites") resolve to nothing in the repo. Same for "pinned as-is rather than changed under a security fix" atorg_path_discovery.py:41-42. RepoCLAUDE.mdasks that comments read correctly without the authoring session's context.backend/file_management/views.py:31— "Handles GET, POST, PUT and PATCH"; only GET and POST are routed.- Three symbols orphaned by the route removal, each had exactly one caller and this PR deleted it:
file_management/serializer.py:53(FileInfoIdeSerializer),file_management/file_management_helper.py:229(delete_file),prompt_studio_output_manager_v2/constants.py:12(TOOL_NOT_FOUND). backend/utils/tests/test_org_path_discovery.py:27-30—test_pin_is_returnedreduces tod.get(k) == d[k]; it can only fail if the short-circuit is deleted outright.backend/utils/tests/test_organization_scoping.py:23-26, :59-62—_Request.__init__guards onis not None, so theNoneleg offor falsy in ("", None)produces an object with no attribute at all — byte-identical totest_missing_org_context_returns_nothing.test_cross_org_isolation.py:96,test_organization_scoping.py:29—@pytest.mark.django_dbis a no-op onTestCasesubclasses and does not drive tier selection (backend/conftest.py:38-44marks on either signal).test_cross_org_isolation.py:100-110—OrgFixture.__init__sets thread-local org context as a construction side effect, andunittestskipstearDownwhensetUpraises.self.addCleanup(UserContext.set_organization_identifier, None)as the first statement ofsetUpruns even on failure.test_organization_scoping.py:47-53— depends on Django's private_base_managerMRO resolution; abase_manager_nameadded toBaseModellater would silently re-scope the queryset and point the failure at the helper.- Django admin changelists for all five models now use
OrgAwareManagervia_default_manager, and/admin/is not matched byOrganizationMiddleware, so the list depends on whateverStateStoreholds on that thread.
Verified clean, for the record
All five pins match what _discover_org_path returns today (BFS field ordering traced per model). _base_manager stays a plain unfiltered Manager (no base_manager_name on BaseModel), so cascade deletes, forward-FK descriptors and the pre_delete receiver at prompt_studio_index_manager_v2/models.py:122-141 are unaffected. Zero references to file/delete across unstract, unstract-cloud and unstract-docs — the UI deletes via DELETE /prompt-studio/file/<tool_id> (ManageDocsModal.jsx:674-687), so the route removal is correct, and the sibling-route guard in test_file_delete_route_removed is a nice touch. tests/groups.yaml collects both new test directories and CI runs them. The CONCURRENCY_MODE → RuntimeError → fail-open path in StateStore is real but latent — the env var is set in no compose, helm or env file in either repo.
Open questions
- Can any currently deployed worker call an internal endpoint without
X-Organization-ID? Three in-repo comments say yes during rolling deploys. That answer decides whether H1 is High or Critical. - Is
OrgAwareManager's fail-open deliberate policy, or an artifact of it predating the fail-closed backend? This PR pins it in a test and argues the opposite in a docstring, in the same diff. SELECT count(*) FROM custom_tool WHERE organization_id IS NULL(andadapter_instance) — several Mediums collapse to nothing if that is zero.
Reviewed with unstract:standard-review v0.18.1 (16-lens rubric, 4 specialist agents). Comments are advisory; event: COMMENT, no merge gate.
| if not org_id: | ||
| logger.warning( | ||
| "Organization scoping requested without organization context on %s; " | ||
| "returning no rows. A caller that must span organizations should " | ||
| "query the model directly instead of using this helper.", | ||
| getattr(request, "path", "<unknown path>"), | ||
| ) | ||
| return queryset.none() | ||
|
|
||
| organization = resolve_organization(org_id, raise_on_not_found=False) | ||
| if not organization: |
There was a problem hiding this comment.
[High] [Lens 7, 8, 11] — Fail-closing this helper breaks a documented worker contract, with no rollout gate
Failing closed is the right end state — I am not arguing against the change itself. The problem is shipping it without a deploy gate while the codebase still documents the header as optional.
Failure mode: six viewsets set skip_org_filter = True, so this helper is their only tenant boundary. A worker that omits X-Organization-ID previously got the unfiltered queryset (the leak being fixed); it now gets zero rows. retrieve() becomes 404, list() becomes empty, and metrics endpoints return a well-formed 200 with all counters at zero — indistinguishable from a genuinely idle system.
Evidence that the header is genuinely optional today:
backend/middleware/internal_api_auth.py:157-164returns{"warning": ..., "context_set": False}and the request proceeds.workers/shared/clients/base_client.py:158initialisesself.organization_id = None, clears it again at:589and:602, and:315-317only attaches the headerif current_org_id:.- Two in-repo comments state the contract this diff breaks and are now stale:
workflow_manager/workflow_v2/views.py:405-408("workers may call without X-Organization-ID during rolling deployments") andnotification_v2/internal_views.py:44("Backward compat: remove once all workers pass X-Organization-ID").
The sharpest consequence is in tool_instance_v2/internal_views.py — see "Unanchored findings" in the summary for that one, since it is outside this diff.
Suggested fix: either (a) land the fail-closed switch behind a settings flag defaulted to the old behaviour for one release while still emitting the warning, or (b) make InternalAPIAuthMiddleware reject internal requests without a resolvable X-Organization-ID (400/401) so the failure is explicit rather than an empty 200/404 — and update or delete the three backward-compat comments in the same PR.
Confidence: High that the contract changes and the comments are now false. Medium that live workers hit it — confirming every internal client call site passes organization_id would raise this to High.
Lens 7 · 8 · 11
|
|
||
| Fails closed. Six internal viewsets set ``skip_org_filter = True``, which | ||
| disables OrganizationFilterBackend, leaving this function as their only | ||
| tenant boundary — so returning the queryset unfiltered when there is no |
There was a problem hiding this comment.
[High] [Lens 2, 4] — The two scoping layers now disagree on the same condition, and the PR pins the disagreement in a test
This docstring states the policy plainly: "A scoping helper returns nothing when it cannot scope, never everything" — and justifies it with the header-less internal path, which is correct reasoning.
But backend/utils/models/org_aware_manager.py:73-75 does the opposite on that exact condition:
if org is None:
# No request context (Celery, management commands, shell)
return qsSo on the very path this docstring names, all five models this PR "protects" still hand back every organization's rows, silently, with no log line. backend/prompt_studio/tests/test_cross_org_isolation.py:147-152 (test_no_org_context_is_unfiltered) codifies that as intended behaviour.
Rated High rather than Critical because it is not tenant-reachable — tenant requests always carry org context, so the manager does filter for them. The gap is on the internal-service-key path. But the manager is the defence this PR advertises for actions that bypass the filter backend, and on that path it does not hold.
Compounding it, org_aware_manager.py:59-71 catches bare RuntimeError and returns unfiltered. StateStore.get raises RuntimeError for any mode that is not the enum, and local_context.py:17 reads os.environ.get("CONCURRENCY_MODE", ConcurrencyMode.THREAD) — so setting that env var at all, even to the string "thread", makes cls.mode == ConcurrencyMode.THREAD false and silently disables org scoping on all five models. I checked: the var is set in no compose, helm or env file in either repo, so this is latent, not live. Worth narrowing the catch anyway.
Suggested fix: pick one policy and state it in both places. If the manager must stay fail-open for shell and management-command use, say so here rather than asserting the opposite rule, and log at WARNING on every unfiltered return so the fail-open is at least observable.
Lens 2 · 4
| """Filter a Django queryset by organization context from request. | ||
| """Filter a Django queryset by the request's organization context. | ||
|
|
||
| Fails closed. Six internal viewsets set ``skip_org_filter = True``, which |
There was a problem hiding this comment.
[Medium] [Lens 16] — "Six internal viewsets" undercounts the caller surface in the direction that matters
The count is literally correct — there are exactly six skip_org_filter = True assignments (pipeline_v2/internal_api_views.py:17, tool_instance_v2/internal_views.py:27, workflow_manager/internal_views.py:56, workflow_manager/workflow_v2/views.py:409, workflow_manager/file_execution/internal_views.py:36, notification_v2/internal_views.py:46), and all six do route through here.
But this helper is also called from function-based @api_view / @require_http_methods handlers that have no filter backend at all and therefore never set skip_org_filter: tool_instance_v2/internal_views.py:343, notification_v2/internal_api_views.py:140/166/209/251/287, workflow_manager/internal_views.py:1916/2588/2729.
Failure mode: a maintainer auditing "which callers does the fail-closed change affect?" by grepping skip_org_filter misses exactly the handlers with the worst failure shape — including the one that returns success: true after validating nothing.
Suggested fix: "every caller of this helper", and drop the count — it will rot on the next viewset added anyway. Same wording appears in backend/utils/tests/test_organization_scoping.py:3-8.
Lens 16
| def test_rejected_default_leaves_the_existing_default_intact(self): | ||
| """A non-matching id must not clear the tool's current default. | ||
|
|
||
| The de-dup update runs against every profile on the tool, so resolving | ||
| the target after it would leave the tool with no default at all when the | ||
| id turns out to be someone else's. | ||
| """ | ||
| assert ProfileManager.objects.get(pk=self.a.profile.profile_id).is_default | ||
|
|
||
| with self.assertRaises(ProfileManager.DoesNotExist): | ||
| ProfileManager.objects.get( | ||
| pk=self.b.profile.profile_id, prompt_studio_tool=self.a.tool | ||
| ) | ||
|
|
||
| self.a.profile.refresh_from_db() | ||
| assert self.a.profile.is_default, ( | ||
| "the tool lost its default profile while rejecting another org's id" | ||
| ) |
There was a problem hiding this comment.
[High] [Lens 13] — The ordering fix in make_profile_default has vacuous coverage; reverting it leaves the suite green
CodeRabbit already raised "exercise the actions, not only their ORM predicates" on this file and I agree with it — rated higher than Trivial, in my view. Not repeating that. This is the part it did not cover.
The headline correctness fix in this PR is the ordering change at prompt_studio_core_v2/views.py:449-466: resolve the target first, then clear, both inside one transaction, so a bad default_profile id cannot leave the tool with zero defaults. This test is named after that property and does not test it.
The body asserts ProfileManager.objects.get(pk=b.profile, prompt_studio_tool=a.tool) raises DoesNotExist, then asserts self.a.profile.is_default is still True. Nothing in the test ever ran the update(is_default=False) sweep — so the "intact" assertion is vacuous. Concretely: revert views.py:449-463 to the original clear-then-resolve order, or delete the with transaction.atomic(): wrapper entirely, and this test still passes.
Its docstring states the mechanism correctly — "The de-dup update runs against every profile on the tool, so resolving the target after it would leave the tool with no default at all" — while the test body never executes that update. test_make_profile_default_lookup_is_tool_scoped at :197-206 is a near-duplicate: :189-192 and :200-203 are the same three-line assertion.
Suggested fix: one view-level test. Org A POSTs make_profile_default on tool A with a second same-org profile (expect 200, old default cleared, new one set), then a second POST with an out-of-tool id (expect 404, and refresh_from_db() shows tool A still has exactly one is_default=True). The second case fails on the pre-fix ordering; the current test cannot.
backend/prompt_studio/prompt_studio_core_v2/tests/test_prompt_studio_author.py:20-53 already drives this viewset with APIRequestFactory + force_authenticate and an identical org setUp — the harness exists in this app.
Lens 13
| ).get_or_create( | ||
| document_manager=document, | ||
| profile_manager=profile_manager, | ||
| defaults={"extraction_status": {}}, |
There was a problem hiding this comment.
[High] [Lens 3, 10] — mark_extraction_status swallows the newly-reachable failures, and nothing downstream reads the result
Making this manager org-scoped adds new failure modes to this function, and the existing error handling converts all of them into a silent no-op. Three stages:
DocumentManager.objects.get(pk=document_id)at:98is now org-filtered, so a mismatch raisesDoesNotExist→ caught at:152→return False. Anything else — including anIntegrityErrorfrom thisget_or_createracing the unique constraint atmodels.py:97-102— is caught by the bareexcept Exceptionat:156→return False.prompt_studio_core_v2/internal_views.py:205-213wraps that asJsonResponse({"success": success})with HTTP 200.workers/ide_callback/tasks.py:236-252only wraps the call intry/exceptand never inspects the body, so a200 {"success": false}sails through as a success.
Net effect: extraction_status is never persisted, check_extraction_status (:184-190) returns False forever, and every subsequent Answer Prompt re-runs the full X2Text extraction — a recurring cost and latency regression with no error anywhere in the system.
The bare except Exception at :156 also hides DatabaseError/OperationalError, malformed-JSON TypeError, and the ImproperlyConfigured that OrgAwareManager itself raises when a pin is wrong — all indistinguishable from "document not found".
Suggested fix: return 500 (or 404 for the DoesNotExist case) from the extraction_status endpoint when success is falsy instead of 200; have the worker check response.get("success") and log at ERROR. Narrow :156 to the exception types actually expected. The synchronous caller already does this correctly — prompt_studio_helper.py:2556-2559 and :2571-2575 both check if not success — so the internal path is the weaker of the two for no stated reason.
Lens 3 · 10
| # Org scoping lives here because custom @action methods never call | ||
| # filter_queryset(), so OrganizationFilterBackend does not run on them. |
There was a problem hiding this comment.
[Medium] [Lens 16] — "custom @action methods never call filter_queryset()" is not what is actually happening
This claim is the PR's stated premise and is repeated at seven sites: here, prompt_studio_index_manager_v2/models.py:25-26, prompt_studio_output_manager_v2/models.py:20-21, prompt_studio_v2/models.py:19-20, prompt_studio_output_manager_v2/views.py:125-126, prompt_studio_core_v2/views.py:1201-1204 ("this action never runs filter_queryset()"), and the test_cross_org_isolation.py:3-4 module docstring.
The two actions this PR fixes do run the filter backend. delete_for_ide (prompt_studio_core_v2/views.py:1176) opens with custom_tool = self.get_object(), and make_profile_default (:437-439) does the same. DRF's GenericAPIView.get_object() is queryset = self.filter_queryset(self.get_queryset()) — so OrganizationFilterBackend runs in both.
What actually bypasses the backend is the raw DocumentManager.objects.get(...) / ProfileManager.objects.get(...) call inside the action, not the action itself. The vulnerability is real and the fix is right — the explanation attached to it is not.
Failure mode: a maintainer reads this as "the filter backend does not apply inside any @action" and either adds redundant scoping to actions that are already scoped, or concludes self.get_object() inside an action is unscoped and "fixes" something that is not broken.
Suggested fix: reword once and reference it from the other six sites — something like "the filter backend only scopes querysets routed through filter_queryset(); raw Model.objects lookups inside a view bypass it, so scope at the manager."
Lens 16
| # No exception handling here: filter() does not raise for a missing or | ||
| # out-of-org tool, it returns empty. Empty is also the correct result | ||
| # for a tool that simply has no prompts yet, which is the normal state | ||
| # of a newly created project — so this stays a 200 with an empty body | ||
| # rather than a validation error. | ||
| tool_studio_prompts = ToolStudioPrompt.objects.filter( | ||
| tool_id=tool_id, | ||
| tool_id__organization=UserContext.get_organization(), | ||
| ).order_by("sequence_number") |
There was a problem hiding this comment.
[Medium] [Lens 3, 16] — Two claims in this comment do not hold
CodeRabbit already ran the empty-200 discussion on this endpoint and you answered it; CodeRabbit agreed and I am not reopening that. These are the two parts the thread did not cover.
1. "filter() does not raise for a missing or out-of-org tool" is false for the input this endpoint actually receives. tool_id comes straight off the query string at :110 (request.GET.get("tool_id")), and CustomTool.tool_id is a UUIDField primary key. A non-UUID value — ?tool_id=abc — makes UUIDField.to_python raise Django's ValidationError while the query is being built, before any row lookup. drf_standardized_errors.handler.ExceptionHandler.convert_known_exceptions maps only Http404 and Django's PermissionDenied; everything else becomes APIException("Server Error (500)"). So the un-wrapped filter() returns a 500, not the 200-with-empty-body this comment promises.
This is not a regression — the old except ObjectDoesNotExist did not catch it either. The defect is the comment asserting a safety the code does not have, which is what will stop the next maintainer from adding validation. It also contradicts your reply on CodeRabbit's thread ("filter() never raises it") in the one case that matters.
2. There is a third cause of empty that the comment does not list. UserContext.get_organization() returns None on both Organization.DoesNotExist and ProgrammingError (backend/utils/user_context.py:26-34), both swallowed without a log. When it does, this filter compiles to custom_tool.organization_id IS NULL and matches nothing regardless of the tool id. Downstream, output_manager_helper.py:317-340 renders each missing output as "", so the user sees blank extraction results for a project with real persisted outputs — no error, no toast, nothing to correlate in logs.
Endpoint is only reachable under /api/v1/unstract/<org>/, so a null org here is a bug rather than a state worth serving.
Suggested fix: narrow claim 1 to "does not raise for a valid UUID that matches no row", or validate tool_id as a UUID up front and return 400. For claim 2, resolve the organization once and fail loudly when it is None. The identical pattern is at latest_outputs_by_keys (views.py:80-92).
Lens 3 · 16
| profile_manager = get_object_or_404( | ||
| ProfileManager, | ||
| pk=request.data["default_profile"], | ||
| prompt_studio_tool=prompt_tool, |
There was a problem hiding this comment.
[Medium] [Lens 3] — The two malformed-input cases next to the hardened one are still 500s, and the full-object save() can clobber a concurrent write
(a) request.data["default_profile"] raises KeyError when the key is absent, and Django's ValidationError ("badly formed hexadecimal UUID string") when the value is not a UUID. Neither is mapped by drf_standardized_errors, so both surface as 500s. This diff deliberately hardened the adjacent case — a valid id that does not match becomes a 404 via get_object_or_404 — and left the two malformed-input siblings as server errors.
(b) profile_manager is fetched before transaction.atomic() opens and then written with a bare save(), which writes every column from the pre-transaction snapshot. Any concurrent edit to that profile between the fetch and the save is silently reverted. The transaction added here guarantees the two writes are atomic with respect to each other, but not that the second one is a narrow write.
Suggested fix: validate with a small serializer (or request.data.get(...) plus an explicit ValidationError) so missing and malformed ids are 400s; and use profile_manager.save(update_fields=["is_default"]) so only the intended column is written.
Lens 3
| # Scope to the tool the caller already passed authz on — tighter than | ||
| # org scope, and this action never runs filter_queryset(). | ||
| # get_object_or_404 keeps a non-matching id a 404 rather than an | ||
| # unhandled DoesNotExist, which the DRF handler turns into a 500. | ||
| document: DocumentManager = get_object_or_404( | ||
| DocumentManager, pk=document_id, tool=custom_tool | ||
| ) |
There was a problem hiding this comment.
[Medium] [Lens 3, 10] — delete_for_ide reports success while silently leaving Redis indexing flags behind
The scoping tightening on this lookup is correct. The issue is the code immediately after it.
IndexManager.objects.filter(document_manager=document_id) at :1187 is now org-scoped. If it comes back empty because the filter hid the rows rather than because none exist, the for loop body never runs, DocumentIndexingService.remove_document_indexing is never called, and execution proceeds straight to document.delete() and a 200 "File deleted succesfully."
The document row and the file are gone, but the Redis indexing flags persist — so a re-upload of the same file is treated as already-indexed. The user is told the delete succeeded, and nothing distinguishes "this document had no index managers" from "the filter hid them".
The except Exception at :1207-1212 compounds it: connector errors, storage errors, Redis errors and ORM errors all collapse into one 400 {"data": "File deletion failed."} with the detail only in logger.error. Worth noting this PR did correctly delete an identical swallow-everything handler over in file_management/views.py — this one, in the surviving path the same diff edits, was left in place.
Suggested fix: log at WARNING when index_managers is empty, including the resolved org, before proceeding. Split the except Exception into the specific failures (ConnectorError, storage exceptions, IntegrityError) with distinct messages and let unexpected types propagate to the DRF handler.
Lens 3 · 10
| # of=("self",): the org-scoped manager joins through | ||
| # AdapterInstance, which would otherwise be locked too. | ||
| summarize_profile = ProfileManager.objects.select_for_update( | ||
| of=("self",) | ||
| ).get(prompt_studio_tool=tool_instance, is_summarize_llm=True) |
There was a problem hiding this comment.
[Medium] [Lens 3, 10] — The except below this now conflates "row is filtered out" with "row does not exist", and never self-heals
The of=("self",) change is correct, and its comment correctly identifies that the org-scoped manager joins through AdapterInstance. But the handler immediately below was not revisited for the same reason.
ProfileManager.objects is now scoped through vector_store__organization. A summarize profile that exists but falls outside that scope raises ObjectDoesNotExist, which is caught at :68 and reported as:
logger.info(f"No summarize profile found for tool {tool_instance.tool_id}, skipping migration")
return False"Filtered out" and "does not exist" become the same INFO line and the same silent no-op. The tool keeps summarize_llm_adapter = NULL, so summarization keeps using the deprecated profile path — and because this lazy migration re-runs and re-skips on every invocation, it never self-heals and never escalates.
The outer except Exception at :93-99 is worse: it logs at WARNING and says "Continuing with the deprecated approach for now" — an explicit, undocumented fallback to legacy behaviour on any error, now including the ImproperlyConfigured that OrgAwareManager raises when a pin is wrong.
Suggested fix: distinguish the two cases — check ProfileManager._base_manager.filter(prompt_studio_tool=tool_instance, is_summarize_llm=True).exists() before concluding "none found", and log at ERROR with the tool id and current org context when the row exists but is not visible.
Lens 3 · 10
3 participants
What
DocumentManager,IndexManager,PromptStudioOutputManager,ToolStudioPrompt,ProfileManager— via the existingOrgAwareManager.delete_for_ide,get_output_for_tool_default,make_profile_default.Organizationinstead of re-deriving it by BFS on every fresh process.file/deleteroute and action.select_for_update(of=("self",))where the new org filter introduces joins.Why
OrganizationFilterBackendruns infilter_queryset(), which custom DRF@actionmethods never call. These five models have noorganizationFK and used a plainBaseModelManager, so a raw.objectslookup inside a custom action carried no organization predicate at all — roughly 44 call sites relying on the caller to pass a correct id.OrgAwareManageralready existed for exactly this shape but was only wired to one model (ExecutionLog).get_org_pathreturns the shortest FK chain toOrganizationand breaks ties by field declaration order. Reordering two fields on a model can swap in a different path of the same length. If that path runs through a nullable FK, Django turns the filter into an INNER JOIN and silently drops every row with a NULL — data loss that presents as missing records, not as an error. This applies toOrganizationFilterBackendin production today, independent of anything else in this PR.file/deleteis dead and shaped wrong. No caller anywhere in the frontend or backend;prompt-studio/file/<tool_id>(DELETE →delete_for_ide) is the live path. It also performed a delete over GET, which makes it prefetchable.How
objects = OrgAwareManager()on the four models with no custom manager;ProfileManagerModelManagernow extendsOrgAwareManagerinstead ofBaseModelManager. No migration — no manager setsuse_in_migrations, so swappingobjectsserializes nothing (makemigrations --check --dry-runis clean).ORG_PATH_OVERRIDESinorg_path_discovery.py, keyed by model label and checked before BFS, so both consumers (OrgAwareManagerandOrganizationFilterBackend) are frozen on the same value. Each pin is set to the path BFS resolves today, so this changes no behaviour on its own.ProfileManagerpins tovector_store__organization, notprompt_studio_tool__organization: BFS reachesAdapterInstance(which carries the organization FK) beforeCustomTool, andprompt_studio_toolis nullable, so pinning there would drop tool-less profiles.AdapterInstanceis org-owned and the serializer's FK queryset uses the org-scoped default manager, so this scopes to the same organization.get_object_or_404, so a non-matching id is a 404 rather than an unhandledModel.DoesNotExist— whichmiddleware.exception.drf_logging_exc_handlerdoes not map, and would surface as a 500.select_for_update(of=("self",))inprompt_studio_index_helperandmigration_utils: the org filter adds INNER JOINs, and PostgresFOR UPDATEwithoutof=locks rows in every joined table (DocumentManager,CustomTool,AdapterInstance).Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Yes — three areas, each covered by a test:
internal_api_auth.py,scheduler/tasks.py,workflow_helper.py), soOrgAwareManagerfilters there too — it is not a no-op outside requests. Indexing and execution pass because the worker's org matches the data's org.test_worker_context_sees_its_own_orgcovers this. Any path that legitimately spans organizations would now return empty; none was found.get_or_createunder a filtering manager. If thegethalf is filtered out while the row exists, thecreatehalf hits the unique constraint. Only reachable across organizations, but the failure mode changes from silently-wrong toIntegrityError.select_for_updatelock scope. Addressed withof=("self",); without it the joins would widen the lock.ProfileManagerandIndexManagerare the two affected call sites.Management commands and shell keep full access:
UserContext.get_organization()returnsNoneoutside a request and the manager fails open, unchanged.test_no_org_context_is_unfilteredpins that.file/deleteremoval is the one behaviour change with no in-repo caller to break. Any external API consumer of that endpoint is unknowable from this repo — worth a release note.Database Migrations
None.
makemigrations --check --dry-runis clean; no manager setsuse_in_migrations, so replacingobjectsdoes not produce a migration.Env Config
None.
Relevant Docs
None.
Related Issues or PRs
UN-3815
Dependencies Versions
Unchanged.
Notes on Testing
backend/prompt_studio/tests/test_cross_org_isolation.py— two fully populated organizations, then per-model checks that org A cannot reach org B's rows, that org A's own rows stay visible, that worker context still sees its own org, and that the no-org path stays unfiltered. Every isolation assertion was confirmed to fail againstmainbefore the fix, so the tests actually bite.backend/utils/tests/test_org_path_discovery.py— asserts each pin still matches what BFS resolves, and that no pin traverses a nullable FK (with one documented exception,ToolStudioPrompt.tool_id, which is the path already in force).main: identical failure sets (36, all pre-existing inworkflow_manager/execution/tests/test_pg_finalization_fixes.py), zero new.Not covered by automation: a real two-org Prompt Studio cycle (upload → index → run → delete) in a compose stack. Worth doing manually before merge.
Screenshots
Checklist
I have read and understood the Contribution Guidelines.