Summary by CodeRabbit

• Bug Fixes
  • Improved access controls for workflow execution logs.
  • Prevented unauthorized users from viewing logs or discovering restricted executions.
  • Ensured organization and group sharing permissions are consistently respected across executions, workflows, deployments, and pipelines.
  • Added coverage for shared, restricted, and inaccessible execution scenarios.

Walkthrough

The change updates execution visibility to use user-scoped resource querysets. Execution log access now checks execution visibility before querying logs. Tests cover organization sharing, group sharing, unshared deployments, and denied or allowed log access.

Changes

Execution access control

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant RequestingUser
  participant WorkflowExecutionLogViewSet
  participant WorkflowExecutionManager
  participant ExecutionLogs
  RequestingUser->>WorkflowExecutionLogViewSet: Request execution logs
  WorkflowExecutionLogViewSet->>WorkflowExecutionManager: Check execution access for user
  WorkflowExecutionManager-->>WorkflowExecutionLogViewSet: Return accessible execution or no match
  WorkflowExecutionLogViewSet->>ExecutionLogs: Query logs for accessible execution
  WorkflowExecutionLogViewSet-->>RequestingUser: Return logs or PermissionDenied

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 @coderabbitai help to get the list of available commands.}

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    U[Authenticated user] --> E[Execution endpoint]
    E --> G[Shared execution access gate]
    G --> Q[WorkflowExecution.for_user]
    Q --> W[Workflow.for_user]
    Q --> A[API deployment.for_user]
    Q --> P[Pipeline.for_user]
    W --> V{Execution visible?}
    A --> V
    P --> V
    V -->|Yes| D[Return logs, export, files, or execution list]
    V -->|No| X[Return permission denied]

_{Reviews (5): Last reviewed commit: "UN-2651 [MISC] Trim the comments added b..." | Re-trigger Greptile}

Standardized review — the 3 Low findings

Split out from the review above so they don't compete with the High/Medium ones. None of these block anything.

[Low] [Lens 3 — Correctness] — for_user now returns empty rather than membership-scoped results outside request context

backend/workflow_manager/workflow_v2/models/execution.py:66-78

The removed helper documented an explicit organization argument for exactly this case:

Falls back to UserContext so request paths need no change; pass organization explicitly on worker/management paths where UserContext is empty.
— tenant_account_v2/sharing_helpers.py:208-213

The replacements go through DefaultOrganizationManagerMixin.get_queryset, which does filter(organization=UserContext.get_organization()) (utils/models/organization_mixin.py:27-30). With no org context that becomes organization_id IS NULL, so a non-admin for_user call from Celery or a management command silently returns nothing.

That is fail-closed, so it is not a security issue, and I confirmed every current caller is a request-context viewset (workflow_manager/execution/views/execution.py:30,44 and execution_log_view.py:46). But the escape hatch the old helper deliberately provided is gone, and the new comment at :68 — "Those managers are org-scoped, so no explicit org arg" — reads as unconditional when it really means "correct inside a request".

Suggested fix: narrow the comment to something like "org-scoped via UserContext; request paths only", so a future worker-side caller is warned rather than surprised.

Confidence: Medium — would rise to High if a worker or management path is later shown to call WorkflowExecution.objects.for_user.

[Low] [Lens 13 — Testing] — test_logs_denied_when_the_execution_is_unknown cannot fail independently

backend/workflow_manager/execution/tests/test_shared_execution_access.py:104-108

The property being claimed is that an unknown id and an inaccessible id are indistinguishable to an attacker probing for valid ids. The test asserts only that PermissionDenied is raised — from the single raise at execution_log_view.py:50 that both cases reach identically. It is true by construction and can only fail in lockstep with its sibling at :99-102. It never compares status code, body, or headers, which are what an enumerator actually observes.

There is also a real distinction at the HTTP layer that this test structurally cannot see: execution/urls.py:22 uses <uuid:pk>, so a malformed id 404s from URL resolution while an unknown-but-valid UUID 403s.

Suggested fix: drive both cases through as_view({"get": "list"}) and assert the two responses have equal status_code and equal .data. That is the actual property, and it can then fail on its own.

Confidence: High.

[Low] [Lens 13 — Testing] — Randomised fixture value makes a failure non-reproducible from CI output

backend/workflow_manager/execution/tests/test_shared_execution_access.py:43

api_name=f"api-{secrets.token_hex(4)}"

Only uniqueness is required, and per-test rollback makes collisions a non-issue — so this is not a flake source. But nothing seeds or logs the value, so a failure whose cause depended on the name could not be reproduced from the CI output.

Suggested fix: derive the name from the test method name or a per-instance counter. Equivalent uniqueness, reproducible.

Confidence: High.

@chandrasekharan-zipstack On the three Lows — two fixed in 8e2649c, one I think is not worth acting on.

L1 (comment reads unconditional) — agreed, fixed. models/execution.py:66-70 now reads "org-scope themselves via UserContext, so this is correct on request paths only — a worker or management command with no org context gets an empty queryset (fail-closed)."

L2 (unknown-id test cannot fail independently) — agreed, and your framing is the right one: the property is about what an enumerator observes. Replaced it with test_unknown_execution_is_indistinguishable_from_an_inaccessible_one, which drives both ids through as_view({"get": "list"}) and asserts equal status_code and equal .data. It fails on its own now.

L3 (randomised api_name) — not taking this one. API_NAME_MAX_LENGTH is 30 and the method names here run past 50, so deriving the name from _testMethodName overflows the column. More fundamentally, no assertion reads api_name, so a failure cannot depend on its value — and secrets.token_hex is already the module’s idiom for throwaway uniqueness (tenant_account_v2/tests.py:44). Happy to revisit if you have a case where the name reaches an assertion.

Self-review — high + medium findings

Ran a multi-pass review over this branch (correctness, tests, error handling, comment accuracy). Every claim below was verified against the code. Lows are listed at the bottom without detail.

HIGH

H1 — The fix is incomplete: /execution/<id>/files/ takes the same id and is still ungated within the org

backend/workflow_manager/file_execution/views.py:14-35

class FileCentricExecutionViewSet(viewsets.ReadOnlyModelViewSet):
    permission_classes = [IsAuthenticated]
    def get_queryset(self):
        return FileExecution.objects.filter(workflow_execution_id=execution_id)...

Same <uuid:pk> — a WorkflowExecution id — with no for_user check. OrganizationFilterBackend still applies (this viewset does not override filter_backends), so cross-org is blocked, but nothing scopes it within the org.

The payload is not metadata-only. FileCentricExecutionSerializer uses exclude = ["file_hash"], so the response carries file_name, file_path, execution_error, provider_file_uuid, fs_metadata — plus status_msg, which get_status_msg resolves from the latest non-DEBUG/WARN ExecutionLog.data["log"]. That is log text.

Both endpoints back the same screen: DetailedLogs.jsx:177 fetches /execution/${id}/files/ and LogModal.jsx:47 fetches /execution/${executionId}/logs/, for the same id. After this PR a de-shared user gets 403 on one and filenames + per-file error text + a log line on the other.

Pre-existing, but same threat model, same id space, same page — and this PR's own rationale ("unknown ids are denied like inaccessible ones so the response does not reveal which ids exist") applies verbatim. Suggest extracting the gate into a shared assert_execution_accessible(user, execution_id) helper and calling it from both get_querysets so the two cannot drift again.

H2 — execution.py:69-73: the "empty queryset (fail-closed)" claim is false

DefaultOrganizationManagerMixin.get_queryset (backend/utils/models/organization_mixin.py:27-30) is:

organization = UserContext.get_organization()
return super().get_queryset().filter(organization=organization)

With no org context that is filter(organization=None), which Django compiles to organization_id IS NULL — orphan rows, not zero rows. organization is null=True on the mixin, and DefaultOrganizationMixin.save() assigns it from UserContext, so rows created off the request path are born with NULL org — exactly what this branch would return.

The genuine fail-closed mechanism is OrganizationFilterBackend (utils/filters/organization_filter.py:52-60, explicit .none() plus a warning) — which ExecutionViewSet drops by overriding filter_backends. Either reword the comment to what the code does, or make the mixin .none() on a missing org.

H3 — execution_log_view.py:31-33: the read-only rationale is inverted, and the editable=False half is wrong

Read-only on purpose: the access gate lives in get_queryset, which write handlers never call. ExecutionLog rows are written by workers and their fields are editable=False, so there is nothing to expose.

• DRF's update / partial_update / destroy all call get_object() → filter_queryset(self.get_queryset()). They are precisely the handlers that do run the gate. create is the only one that skips it.
• Only id, execution_id, wf_execution, file_execution carry editable=False. data (JSONField) and event_time (DateTimeField) do not, and the serializer is fields = "__all__" (workflow_v2/serializers.py:154-157), so DRF would expose both as writable on a PATCH.
• Rows are written by the backend Celery task execution_log_utils.consume_log_history, not by the workers/ processes.

The decision to go read-only is right. The stated reason is the inverse of the mechanism, and would justify someone later adding a write handler on the belief it bypasses the gate. Worth also noting that the reason no write verb is reachable today is that both URLconfs bind GET only — not anything in this class.

H4 — Test gap: the pipeline_id__isnull=True conjunct is unpinned

execution.py:88:

final_filter = (workflow_filter & Q(pipeline_id__isnull=True)) | deployment_filter

Deleting & Q(pipeline_id__isnull=True) breaks no test. Every fixture deployment hangs off self.workflow (test file lines 48, 61), and self.workflow is reachable only by self.owner — so the assertFalse in test_group_share_exposes_the_deployment_executions and test_pipeline_execution_follows_the_pipeline_share holds with or without the conjunct. The mutation survives.

That conjunct is what stops workflow access from leaking into runs of deployments deliberately not shared — a privilege widening on the endpoint this PR is hardening. It is also the behaviour the PR description calls out as counter-intuitive ("every path has to be revoked"), so it is the line most likely to be "simplified" later.

def test_workflow_share_does_not_expose_unshared_deployment_runs(self) -> None:
    deployment = self._api_deployment()            # shared with nobody
    execution = self._execution(deployment)
    self._share_with_group(self.workflow)          # member now sees the workflow
    self.assertIn(self.workflow, Workflow.objects.for_user(self.member))  # control
    self.assertFalse(self._visible_to(self.member, execution))

def test_deployment_share_does_not_expose_workflow_level_runs(self) -> None:
    deployment = self._api_deployment(shared_to_org=True)
    self.assertTrue(self._visible_to(self.outsider, self._execution(deployment)))  # control
    self.assertFalse(self._visible_to(self.outsider, self._execution()))

H5 — The admin memo is not keyed by organization, but the predicate it caches is org-dependent

tenant_account_v2/organization_member_service.py:10-14, 41-44

OrganizationMember.objects is OrganizationMemberModelManager(DefaultOrganizationManagerMixin, ...) (tenant_account_v2/models.py:12), so OrganizationMember.objects.get(user=user.id) resolves against UserContext. The answer is a function of (user, current org); the memo stores only the bool.

The justifying comment — "Django rebuilds that instance per request, so the memo lives exactly one request" — asserts an invariant nothing enforces. authentication_controller.set_user_organization:178 flips UserContext mid-request while holding the same request.user, and the predicate is a public static called with non-request users (prompt_studio_helper.py:241 passes profile_manager.created_by; tool_instance_helper.py:525 takes an arbitrary user).

I traced the mid-request switch: that view is a bare @api_view with no permission class that would seed the memo beforehand, so there is no live bug today. But a stale True in for_user means "see every resource in the org" — the widest possible answer — and the guard is currently a comment rather than code. One-line fix:

org_id = UserContext.get_organization_identifier()
memo = getattr(user, _ADMIN_MEMO_ATTR, None)
if memo is not None and memo[0] == org_id:
    return memo[1]
...
setattr(user, _ADMIN_MEMO_ATTR, (org_id, is_admin))

Separately worth asking whether the memo belongs in this PR. Measured saving is exactly 3 single-row indexed lookups per for_user call, non-admins only (admins short-circuit before the three delegates). That is 22 lines and a new staleness mode, in a module with 20+ call sites, riding along in an access-control fix. Hoisting the predicate to a local in for_user and passing it down saves the same 3 queries with no lifetime question at all.

MEDIUM

M1 — organization_member_service.py:62-65: except AttributeError: pass guards an unreachable case and silently disables the memo

AnonymousUser cannot reach that line (line 38 returns first — is_authenticated is False), and it is not immutable anyway (plain __dict__, setattr succeeds). No user type in either repo raises here. If a future cloud proxy ever wraps User with a custom __setattr__, this pass silently restores the 4x query amplification the hunk exists to remove, with zero diagnostic signal. Drop the try/except, or log a warning instead of passing. It is also the only bare except: pass in the diff.

M2 — The org-admin and service-account branches have zero test coverage on a cross-tenant boundary

execution.py:57-61 and :63-67 — 4 lines each, 0 hits in the integration coverage report for this branch's own CI run. The setUp patch (test file lines 40-42) pins is_user_organization_admin to False for the whole class, so neither branch ever executes.

These branches are the only tenant boundary for admins on /execution/, per this PR's own docstring. GroupSharingTestBase already creates self.admin with role "admin" and it is unused in this file. A sibling class without the patch closes both. Note the patch buys nothing for the current assertions either — every fixture user has role "user", so real admin resolution already returns False.

M3 — execution.py:61, 67: return self.all() is a cross-org fallback under a docstring that says it cannot happen

Pre-existing and unchanged by this PR. But this PR's new docstring promotes this manager to the tenant boundary ("That org scoping is enforced here, not by the view"), and these two lines return every execution in every organization when UserContext.get_organization() is None — which UserContext returns on three silent paths (no StateStore key, Organization.DoesNotExist, ProgrammingError), none of them logged.

I confirmed all three non-test callers of WorkflowExecution.objects.for_user are request-path, so self.none() would break nothing. Either make it self.none() (2 lines, arguably in scope since the PR is claiming the guarantee) or soften the docstring so the claim does not outrun the code.

M4 — execution_log_view.py:57-59: the backward-compat Q(execution_id=...) branch is dead in request context

ExecutionLog.objects is an OrgAwareManager (models/execution_log.py:42) that BFS-resolves the org path to wf_execution__workflow__organization. That is an INNER JOIN, so any legacy row with wf_execution IS NULL — the only rows the second Q can match — is dropped before the OR is evaluated.

Pre-existing, but this PR makes it more confusing: the gate now explicitly confirms "you have access to this execution", and the body then returns {"count": 0, "results": []} with no error and no log line. Since this method's contract is being rewritten anyway, worth answering the question — delete the branch and the TODO, or make it reachable via an unfiltered manager plus an explicit org check.

M5 — The denial is not logged with the user id

execution_log_view.py:53. The global handler (middleware.exception.drf_logging_exc_handler) does log method, full URI and request id at ERROR, so this is not silent. What is missing is who — the field needed to answer "is someone enumerating execution ids?". Since this PR deliberately makes unknown and inaccessible ids indistinguishable to the client (correct), the server is now the only place that can tell them apart, and that distinction is worth recording:

logger.warning(
    "Execution log access denied: user=%s execution=%s org=%s exists=%s",
    getattr(self.request.user, "id", None), execution_id,
    UserContext.get_organization_identifier(),
    WorkflowExecution.objects.filter(pk=execution_id).exists(),
)

M6 — The PR description's test table is inaccurate

It claims "6 / 6 pass (4 of them fail on main)". The file has 13 tests, and 8 fail on main. Two listed names do not exist in the file (test_unshared_deployment_stays_invisible, test_logs_denied_when_the_execution_is_unknown), and seven real tests are missing from the table. Reviewers calibrate on that table. A table mirroring a test file also rots on every edit — one sentence naming the file is more durable.

Lows, not detailed here

.values("pk") redundant on pk__in=<queryset>; .distinct() vestigial (all terms are IN (subquery), no outer join); _log_queryset redundant now that _call drives the real endpoint, and two of its three uses are strict subsets of the _call tests; _share_with_group hand-writes the ContentType row instead of using the production set_resource_share_groups; JSON export (the default file_format) untested; group-share and direct-viewer tested for APIDeployment but not Pipeline; export 400/413 paths uncovered; cross-org test has no positive control; UserContext never torn down in GroupSharingTestBase; comment cites APIDeploymentViewSet.perform_create, which does not exist (the OWNER row is created in create()); class docstring says self.owner "owns every fixture here", not true of the cross-org fixtures; six intermediate Q locals in for_user collapse to one expression.

Holding up well

The gate itself is real and pinned — delete the raise PermissionDenied block and four tests fail. Dropping IsOwner genuinely widens nothing: it implements only has_object_permission, and all four routes bind list/export, neither of which calls get_object(). test_unknown_execution_is_indistinguishable_from_an_inaccessible_one asserting on the response (status and body) rather than the exception type is the right call — the usual assertRaises version ships an enumeration oracle. And test_unshared_deployment_is_visible_only_to_its_owner asserting its own positive control is the discipline H4 and the cross-org test are missing.

Addressed in 92a06516. All five Highs and five of six Mediums fixed; M4 got a comment rather than a deletion. Also fixed @chandrasekharan-zipstack's second unanchored High from the earlier review, which this self-review did not repeat.

H1 — <id>/files/ ungated. New workflow_manager/execution/access.py holds assert_execution_accessible(user, execution_id); WorkflowExecutionLogViewSet.get_queryset and FileCentricExecutionViewSet.get_queryset both call it. One gate, so the two routes on the same id cannot drift apart again.

H2 — the fail-closed claim. Confirmed: DefaultOrganizationManagerMixin compiles filter(organization=None) to organization_id IS NULL, which matches orphan rows rather than nothing. Comment now states that, and points the reader at the request path. I did not change the mixin — that is a repo-wide behavioural change well outside this PR.

H3 — the read-only rationale. Both halves confirmed. update/partial_update/destroy do run the gate via get_object(); create is the single handler that does not. Only id, execution_id, wf_execution and file_execution are editable=False — data and event_time are writable under fields = "__all__". Writer is the backend consume_log_history Celery task, not workers/. Comment rewritten to the actual mechanism.

H4 — the pipeline_id__isnull=True conjunct. Both suggested tests added verbatim in intent. Verified by mutation: deleting the conjunct now fails test_workflow_share_does_not_expose_unshared_deployment_runs.

H5 — memo staleness. Memo is now (organization_identifier, is_admin) and the comment no longer asserts an instance-lifetime invariant. Kept the memo rather than hoisting the predicate into for_user: the three delegates would each need a new parameter in their public signature, which is a wider change than the one it replaces.

M1 — except AttributeError: pass deleted; plain setattr.

M2 — new ExecutionBypassRoleTests: org admin sees unshared runs, service account sees them without any membership, both stay inside the current org, and no-org-in-context returns nothing. One correction to the finding, though: leaving the predicate to resolve for real does not work. AuthenticationController resolves auth_service from the active auth plugin, and the auth0 plugin's UserRole.ADMIN is "unstract_admin" where OSS's is "admin" — so is_admin_by_role("admin") returns False on any checkout carrying the plugin and True in OSS CI. A test depending on that is environment-dependent by construction. Patched deterministically instead (only self.admin), same idiom as ShareAuthorizationServiceTests, which pins what for_user does once the predicate is True — the role string belongs to the plugin, not to this PR.

M3 — extracted _org_scoped(); self.none() plus a warning when UserContext has no org. Confirmed all three non-test callers are request-path.

M4 — not deleting the branch. The reachability analysis is right — the OrgAwareManager join through wf_execution drops legacy-only rows before the OR is evaluated — but "delete it" turns on whether rows with wf_execution IS NULL still exist in production, which I cannot verify from here, and the branch still functions in the unscoped contexts (Celery, shell) where the org filter is absent. Replaced the stale TODO with the actual reason it matches nothing in request context.

M5 — the denial now logs user, execution, org and exists=. It sits in the shared helper, so both routes get it, and it runs only on the denial path.

M6 — PR body updated. Dropped the table for one sentence naming the file, as suggested.

Four mutation checks run against the new tests, all caught, files restored: gate removed from files/ → test_file_executions_follow_the_same_gate_as_the_logs; for_user removed from WorkflowExecutionViewSet → test_workflow_execution_list_is_scoped_to_accessible_workflows; self.none() → self.all() → test_no_organization_in_context_returns_nothing; conjunct removed → test_workflow_share_does_not_expose_unshared_deployment_runs.

Lows not acted on beyond the three I corrected in passing (the perform_create citation, the class docstring's ownership claim, and the missing positive control on the cross-org test).

@chandrasekharan-zipstack Fair — done in c2e28a98 (7 files, 42 insertions / 83 deletions, comments only).

What I cut: line-by-line retellings of the mechanism, file.py:NN citations, and "this used to be X" history. Those are the parts that rot on the next refactor and that a whole-file read has to wade through.

What I kept: the reason a thing is the way it is, where it isn't inferable from the code — why the log viewset is read-only, why the compat Q term still exists, why the admin memo is keyed by org, and why the bypass tests patch the predicate instead of resolving it. One or two lines each.

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Unstract test results

Per-group results

Critical paths

⚠️ Critical paths not yet covered

• workflow-execution-fan-out — Multi-file workflow execution fans out to file-processing workers and rejoins. (declared coverage: no groups declared)