UN-3853 [FIX] Attribute platform-key-created resources to the API key's creator - #2274
kirtimanmishrazipstack merged 16 commits into
Conversation
|
Merge this before the cloud PRZipstack/unstract-cloud#1753 imports |
…reator A platform API key authenticates as a service account, and every resource create path granted the OWNER membership row to that machine identity. Service accounts are filtered out of every owner surface (HasMembersMixin), so such a resource ended up with no human owner: invisible to its creator in list views, manageable only through the org-admin fallback, and rendered in "Owned By" as a synthetic @platform.internal address dressed up as a colleague. Record the key's creator as owner instead — the same successor delete_api_user_for_key already hands ownership to when a key is deleted, now applied at creation rather than only at deletion. The service account loses nothing: permission classes and for_user() short-circuit on is_service_account. Where no human can be named (the key's creator has since been deleted), the resource stays deliberately ownerless and the table labels it "Platform key" rather than naming a machine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JHnDZZWGhsevUdwgMyR2ai
…s too The ticket asks for Owned By to be correct on every resource type. The API deployment and ETL pipeline card views were still wrong: OwnerFieldRow read created_by_email only, and their serializers never exposed owner_emails — so those cards named the audit creator, which on a platform-key create is the service account. The backend fix alone could not reach them. Expose owner_emails on both serializers (their querysets already prefetch memberships__user, so it costs no extra query), and move the owner-label rule into one resolveOwnerDisplay helper shared by the table and the cards. The two had already drifted on both the source field and the "Me" rule — the card said "Me" to any owner, which is the co-owner bug the table's comment warns about. Cards now match the table: "Me" tracks the displayed owner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JHnDZZWGhsevUdwgMyR2ai
90013a8 to
b818d85
Compare
…platform-key Both conflicts were adjacency-only in backend/api_v2 — each side kept: - api_deployment_views.py: main's PlatformKeyAutoSchema import landed on the line owner_user_for occupies. - serializers.py: main's get_run_count comment abuts get_owner_emails. Regenerated specs/docstudio-oss.json: #2278 publishes the deployment listing, so owner_emails now belongs in APIDeploymentSummary and the committed spec must byte-match a fresh render (test_docstudio_spec). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BAfubrF8kR2rKewMSjXagg
`owner_user_for` had no coverage. Adds the resolver's own branches (normal user early-returns with no query, service account resolves to the key's creator, a deleted creator or a missing key leaves the resource ownerless) and one case per OSS resource that grants an OWNER row on create: workflow, prompt studio, ETL pipeline, API deployment, connector, adapter. The resource cases drive the real URLconf and middleware chain with a key minted in the test, so the service-account swap that caused the bug is exercised rather than simulated. Verified by mutation: reverting each call site to the raw request user fails the matching case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BAfubrF8kR2rKewMSjXagg
F1: owner_user_for granted an OWNER membership row to the key's creator without checking they are still a member of the key's organization. _is_resource_owner grants on any surviving OWNER row with no live-membership check, which is why cleanup_user_org_access purges those rows on departure; minting a new one afterwards reopened that rejoin backdoor for the key's lifetime. AddOwnerSerializer already refuses both non-members and service accounts, so these sites were the only OWNER writes without the gate. The check reads _base_manager: the default manager is org-scoped by UserContext, which is unset outside a request, so the plain manager would return empty and silently strip every resource of its owner. F6: the fallback arms now log, naming the key id (not the key itself). F7, F8: the docstring claimed service accounts are filtered from "every" owner surface and that the residual resource "stays deliberately ownerless". Neither held -- owner_memberships() and is_owner() apply no such filter, and a real OWNER row is written that the last-owner guard counts as live. Tests: the fixtures created a key creator with no OrganizationMember row, a user who could never have created a key through IsOrganizationAdmin. They now build the member production guarantees, and two cases pin the new guard (creator left the org; creator belongs to a different org). Both fail when the guard is neutered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tm83ikxv1euWWQQGLMBcjC
…er the gaps F5: the create response is a seven-field summary carrying none of the owner fields the list renders, and the modal spliced it straight into the table, so a just-created API deployment showed no owner until the next fetch. It now refetches; the response is still handed to the code modal, which is the only thing that needs the API key. The edit path already merged over the existing row, so it kept its owner fields and is unchanged. F3: reverting the seventh grant site (create_tool_from_import_data, reached by project-transfer/ and sync-prompts/ with create_copy) left the whole backend suite green -- six cases covered seven sites. A case now pins it, and fails when that site is reverted. F9: the comment called the "Platform key" branch reachable only via a deleted creator. With no backfill it is also every resource created through a key before this change; the accurate statement already sits on the fallback above. F11: resolveOwnerDisplay decides the Owned By cell on every list and card and had no test -- the only suite rendering it asserts nothing about that cell. Six cases now cover its four branches; they fail when the platform-key suppression or the "Me" rule is broken. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tm83ikxv1euWWQQGLMBcjC
…he field F10: resources created through a platform key before this change hold an OWNER row naming the service account, which every owner surface filters out, so they show no owner and only an org admin can manage them. Nothing moved them -- the only re-pointer runs from delete_api_user_for_key. A data migration now hands each to the same successor owner_user_for picks, under the same membership rule: a creator who has left the org is skipped rather than handed a fresh OWNER row, which would reopen the rejoin backdoor. Where the creator already holds a row on that resource the stronger role is kept and the service account's is dropped, so the unique triple is never violated. Verified on a disposable database seeded with all three cases -- live creator, departed creator, and a creator already holding a VIEWER row: forward applies, reverse unapplies, and a re-apply is a no-op. F4: APIDeploymentSummary subclasses the list serializer and declares no fields of its own, so owner_emails reached the published platform-key contract with no description. Stripping it there would only desynchronise the spec from the response, and the UI reads the same endpoint, so the field stays and the docstring now documents what a caller receives. Spec regenerated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tm83ikxv1euWWQQGLMBcjC
F12: resolving the acting user was a rule every create site had to remember, applied at seven sites here and four in cloud. That is how the agentic sample-project site came to be missed. HasMembersMixin is already on every one of these models, so grant_owner() lives there and each site calls it -- the rule now has one enforcement point, and all seven ownership cases fail when it is broken, instead of one case per site. Also drops seven imports of platform_api from the business apps, for an operation permissions/ owns; the resolver import is now lazy and in one place. Out of scope, checked: sharing_helpers grants VIEWER rather than create-time OWNER, and platform_admin's two onboarding grants take the human from signup rather than request.user, so no bearer session reaches them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tm83ikxv1euWWQQGLMBcjC
…ill paths Found by adversarial verification of the previous commits. F21: closing the create path left delete_api_user_for_key still handing every OWNER row to the key's creator with no membership check, so deleting a key re-granted a departed creator the rows the create path had just refused -- the same rejoin backdoor, one step later. The membership question is now live_key_creator(), asked by both paths instead of stated twice, and transfer_ownership already short-circuits on None. Two cases pin it. F22: migration 0006 declared no edge to UN-2202's *_absorb_shared_users backfills, which write an OWNER row from created_by -- the service account on a key-created resource. Six of the eight sorted before 0006 by alphabetical accident and workflow_v2.0022 sorted after it, so workflows would have been backfilled ownerless and left that way. The edges are now declared; workflow_v2.0022 moves from plan index 289 to 183, immediately before 0006. F23: the create-modal refresh called the fetch with its own defaults, so it dropped any active search and reset to page 1 -- and a never-run deployment sorts into the tail, so it was usually not on the page it landed on. It now uses handleListRefresh, which the hook documents for exactly this and which the sibling delete and co-owner callbacks already use. Also corrects two comments this branch introduced: the owner-display pointer named a fallback that states something else, and the lazy-import note named a cycle that does not exist (the real constraint is import-time model loading). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tm83ikxv1euWWQQGLMBcjC
The red CI here is an infra issue, unrelated to this change
|
… tree Sonar flagged all three `apps.get_model` bindings in the backfill (python:S117). `0005_add_reconciliation_task` already uses the `*_model` form, so this matches the migration the repo most recently reviewed rather than the older PascalCase one in `0004_pg_periodic_tasks`. The quoted model names are untouched -- those are lookup keys, not identifiers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BAfubrF8kR2rKewMSjXagg
The comments had grown into mechanism retellings and change history -- why a site was missed, what the two views used to disagree on, which surface filters what. That rots as soon as the code moves and costs every later reader. Keeps the contract (what a function returns, why a lazy import, why _base_manager) and drops the narration. No code changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BAfubrF8kR2rKewMSjXagg
Deepak-Kesavan
left a comment
There was a problem hiding this comment.
🤖 Automated review
Automated review by Unstract PR review kit (Claude Code). Each finding below was reproduced against the code rather than inferred, so treat it as something to resolve before merge. If one is wrong, disagree on the thread and close it — that is the expected way to clear a finding. Anything tagged [unverified] was not reproduced and is flagged for your judgement instead.
4 inline comment(s) · 1 finding(s) not on changed lines (below).
Findings off the diff
backend/api_v2/api_deployment_views.py:406
[minor]by_prompt_studio_toolserializes withAPIDeploymentListSerializeron a queryset that has neitherselect_related("created_by")norprefetch_related("memberships__user"), so the newowner_emailsSerializerMethodField adds one membership query plus one user query per membership row, per deployment.
GET /api/v1/unstract//api/deployment/by_prompt_studio_tool/?tool_id= with N matching deployments. get_owner_emails (backend/api_v2/serializers.py:547-551) calls obj.owner_emails(), which iterates self.memberships.all() and dereferences m.user.email (backend/permissions/models.py:60-70). With no prefetch cache, memberships.all() issues a fresh SELECT per row and each m.user issues another, so the endpoint gains 1 + (#owners) queries per deployment on top of the pre-existing per-row cost. The two list viewsets are prefetched (api_deployment_views.py:300, pipeline_v2/views.py:86); this caller was missed.
Suggested fix: Add the same eager loading at backend/api_v2/api_deployment_views.py:406: APIDeployment.objects.for_user(request.user).select_related("created_by").prefetch_related("memberships__user").filter(workflow_id__in=workflow_ids).
🤖 Unstract PR review kit (Claude Code) · review-pr-bot:165a1da9d29f
review-pr-bot:review
… intact Review findings from #2274. Moving the OWNER row off the service account broke two gates that authorize by OWNER membership and have no service-account bypass, so a workflow provisioned through a platform key failed its own execution: `validate_adapter_access` and `IsWorkflowOwnerOrShared.has_permission`. Both now short-circuit on a service account, matching every other access surface. On key deletion the audit half and the ownership half are now separate. `created_by`/`modified_by` follow the key's creator even if they have left the org -- deleting the account is SET_NULL on those FKs, and a null `created_by` breaks `CustomTool.delete()`. The OWNER row still goes only to a live member. Also: eager-load `by_prompt_studio_tool` so `owner_emails` does not fan out, write `modified_at` in the backfill (historical models skip BaseModel.save), and return to page 1 after creating a deployment, since never-run rows sort last and the new one could land off-page. Three regression tests, each verified by reverting its fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BAfubrF8kR2rKewMSjXagg
|
@Deepak-Kesavan Thanks — all five findings reproduced against the code, all five fixed in a57f99b (OSS) and 402f4ea1 (cloud). Replies are on each thread; this one covers the off-diff finding.
Two notes on the process, since they affected the outcome: One of my regression tests was initially vacuous. The test for the critical adapter-access finding passed with the fix reverted, because The cloud storage finding was invisible locally. That test passed on my machine because my dev Full suite after the fixes: 382 passed, spec check clean, hooks clean on both repos. 🤖 Generated with Claude Code |
Rewiring the create modal to the page-1 refresh left `handleListRefresh` destructured but unread here. The hook still exports it and Workflows.jsx still uses it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BAfubrF8kR2rKewMSjXagg
…apps tenant_account_v2's backfill migration can't depend on cloud-only apps' absorb_shared_users migrations, so their service-account OWNER rows could land after the repair and never get revisited. Extract the repair into _membership_backfill so a cloud migration can import and re-run it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Deepak-Kesavan
left a comment
There was a problem hiding this comment.
🤖 Automated review (re-review)
Automated review by Unstract PR review kit (Claude Code). Each finding below was reproduced against the code rather than inferred, so treat it as something to resolve before merge. If one is wrong, disagree on the thread and close it — that is the expected way to clear a finding. Anything tagged [unverified] was not reproduced and is flagged for your judgement instead.
1 inline comment(s) · 1 finding(s) not on changed lines (below).
Findings off the diff
backend/permissions/permission.py:285
[major] The fix patched the two authz gates that were reported, butIsFrictionLessAdapterDeleteis a third ownership gate with nois_service_accountbypass — so a full_access platform key can no longer delete an adapter it created through that same key.
- Admin mints a
full_accessplatform key. POST /api/v1/unstract/<org>/adapter/withAuthorization: Bearer <key>creates an LLM adapter — the exact flow this PR's owntest_platform_key_resource_ownership.test_adapterasserts.grant_owner(request.user)resolves the service account to the human creator, so the only OWNER row names the human.- The same key issues
DELETE /api/v1/unstract/<org>/adapter/<id>/.AdapterInstance.objects.for_user(SA)returns.all(), so the object resolves; thenIsFrictionLessAdapterDelete.has_object_permissionruns (adapter_processor_v2/views.py:170-171routesdestroyto it):obj.is_friction_less-> False_is_resource_owner(SA, adapter)-> False (post-PR the row is the human's; pre-PR it was the SA's and this returned True)_is_organization_admin(SA)-> False by design (organization_member_service.py:39short-circuits service accounts)
=> 403, where main returned 204.
permissions/permission.py:274-287 is the only _is_resource_owner call site in the tree without a service-account short-circuit. Every sibling in the same file has one (:114, :131, :163, :194, :229, :243, :268), including IsFrictionLessAdapter immediately above it, and prompt_studio/permission.py:24, :51 have one too. The asymmetry is right next to the fix.
It ships green: the new ServiceAccountStaysAuthorizedTest covers exactly the two sites that were reported, and test_platform_key_resource_ownership.test_adapter asserts the create but never the subsequent delete.
This also still falsifies the PR body's claim that "the robot account keeps full access to everything in its org".
Suggested fix: Add the bypass the same way every sibling in the file does, after the frictionless carve-out:
def has_object_permission(self, request, view, obj):
if obj.is_friction_less:
return True
if _is_service_account(request):
return True
if _is_resource_owner(request.user, obj):
return True
return _is_organization_admin(request)Worth adding a DELETE .../adapter/<id>/ case to ServiceAccountStaysAuthorizedTest — the two cases it has would not have caught this one.
🤖 Unstract PR review kit (Claude Code) · review-pr-bot:d82dbd8bdf70
review-pr-bot:review
Re-review findings from #2274. `IsFrictionLessAdapterDelete` is a third gate that authorizes by OWNER membership with no service-account bypass, so a full_access key could no longer delete an adapter it had just created. Every sibling in the file has the bypass, including `IsFrictionLessAdapter` directly above it. Swept the rest rather than wait for a fourth: all 13 `_is_resource_owner` call sites across both repos now either carry the bypass or are unreachable by a service account -- `prompt_studio_helper._adapter_accessible_by` is guarded by an `is_service_account` early return at its only caller. The page-1 refresh is reverted. The ordering is `last_run_time DESC NULLS LAST`, so a never-run deployment sorts to the END -- page 1 is the wrong target, and jumping there also threw the user off the page the row was actually on. `handleListRefresh` at least keeps them there. Making the new row visible needs an ordering change, which is a product decision, not this PR. Regression test for the adapter delete gate, verified by reverting the bypass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BAfubrF8kR2rKewMSjXagg
|
@Deepak-Kesavan Both re-review findings fixed in 040f205.
I swept the rest rather than wait for a fourth round. All 13
So the PR body's claim about the service account keeping org-wide access is now true rather than aspirational. On the page-1 revert — reply is on that thread. Short version: you were right to correct it, and the contradiction was visible in my own comment, which said never-run rows sort last and then jumped to page 1 anyway. Reverted to Regression test added for the delete gate, verified by reverting the bypass. One caveat on verification: my local full-suite runs are unreliable right now — the Docker environment was wiped and the test-database lifecycle is thrashing, with the same failure profile on a clean baseline as with my changes. Per-app runs are clean and the new test is mutation-verified, but I am treating CI as the authority for the full suite rather than claiming a green local run. 🤖 Generated with Claude Code |
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
Unstract test resultsPer-group results
Critical paths
|
🤖 Automated re-review (pass 3) — cleanRe-reviewed Verified:
One candidate raised and refuted, recording it so it does not come back: creating an adapter through a platform key then deleting it returns 500 Nothing blocking from this pass. Worth remembering 🤖 Unstract PR review kit (Claude Code) · automated pass · findings are starting points, not verdicts |
UN-3853 (#2274) extracted the "Owned By" label into the shared ``resolveOwnerDisplay`` helper. This branch had rewritten the same block in the card view to read ``is_owner``, so the two collided in ``CardFieldComponents`` and on the import line in ``ResourceTable``. Took the helper. It reads ``owner_emails``, so it sees co-owners -- the gap this branch's version was written to close -- and it additionally labels platform-key service accounts and resolves "Me" against the owner actually shown rather than the viewer's own membership. ``OwnerFieldRow`` therefore takes ``sessionDetails`` again, and both card configs pass it. ``is_owner`` is untouched elsewhere: it drives editability via ``canEditResource``, which is a separate question from whose name is on the row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UwnxaoVTsxbzufUixPGFQ9



What
Why
UN / Unknownin Owned By.How
owner_user_for(user)inbackend/platform_api/services.py. If the acting user is a platform key's service account it returns the key'screated_by; otherwise it returns the user unchanged, before any query.delete_api_user_for_keyalready hands a key's resources tocreated_bywhen the key is deleted. This applies the same successor at creation time instead of only at deletion.HasMembersMixin.grant_owner(), so a create site is a single call and cannot forget the rule. Seven sites in this repo route through it.cleanup_user_org_accesspurges rows to prevent.created_byis untouched everywhere. It is an audit field, and "the robot did it" is the truthful answer.owner-display.js) so the table and card views cannot drift apart.Can this PR break any existing features. If yes, please list possible items. If no, please explain why.
Database Migrations
tenant_account_v2/0006_backfill_platform_key_ownershipre-points existing robot-owned rows to the key's creator, so resources made before this change get a real owner too.Env Config
Relevant Docs
Dependencies Versions
Notes on Testing
Two people in the same organization, both org admins:
kirtiman.mishra+sept4_1@zipstack.comkirtiman.mishra+sept4@zipstack.comEach created their own platform API key, then used their own key to create resources:
un-3853-user-wfun-3853-api-key-1What each of them sees in the Owned By column:
un-3853-user-wfun-3853-api-key-1un3853-live-owner-probeThe two views never disagree about who owns a row. They differ only in how "you" is written: each person reads Me on the rows they own, and the other person's email address on the rest.
+1means the resource has a second owner.Before this change every one of these rows read Platform key, and neither person could open, share or delete them — only an org admin could.
The same was checked by creating each resource type through a platform API key: workflow, ETL pipeline, API deployment, Prompt Studio project, connector, adapter, and the agentic project on the cloud side. Every one is owned by the key's creator.
Automated coverage: 16 backend tests and 6 frontend tests. The backend ones drive the real URL and login path with a key created inside the test, so the robot-account swap is genuinely exercised. Each was checked by reverting the fix and confirming the test fails.
Screenshots
What user-1 sees — their own workflow reads "Me", the org admin's reads the admin's address:

What the org admin sees — the mirror image:

Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BAfubrF8kR2rKewMSjXagg