[FEAT] Platform API gaps for org-to-org clone: service-account group management + name filters#2044
Open
chandrasekharan-zipstack wants to merge 12 commits into
Open
[FEAT] Platform API gaps for org-to-org clone: service-account group management + name filters#2044chandrasekharan-zipstack wants to merge 12 commits into
chandrasekharan-zipstack wants to merge 12 commits into
Conversation
…l instances Enables the unstract-python-client SDK migration subpackage to drive org-to-org data migration purely through admin-issued Platform API keys. Specifically: - adapter_processor_v2/models.py: AdapterInstanceModelManager.for_user returns non-frictionless adapters for service-account callers (was: all()) - permissions/permission.py: IsFrictionLessAdapter grants access to service accounts on non-frictionless adapters, keeping the friction-first check - prompt_studio/permission.py: PromptAcesssToUser short-circuits to True for service accounts so Platform API can GET/POST prompts - tool_instance_v2/views.py: get_queryset scopes via Workflow.for_user so service accounts see all tool instances under workflows they can access Plan: org-to-org data migration v1 (KB: zipstuff/org-data-migration/05).
The SDK migration subpackage relies on list-by-name GET as the cross-run idempotency check (Layer 2). Without this filter, every re-run would re-create adapters that already exist on the target org. Plan: org-to-org data migration v1 (KB: zipstuff/org-data-migration/05).
Mirror the adapter pattern from c05dc05: SDK migrator uses name-based GET against target to detect existing rows before POST. - ConnectorInstanceViewSet.get_queryset: thread CIKey.CONNECTOR_NAME through FilterHelper. - TagViewSet: declare filterset_fields=["name"] so DjangoFilterBackend honors ?name=. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tag URLs previously bound only GET on list/detail, so the migrator (and any other API consumer) got 405 on POST tags/. Wire create/partial_update/ destroy through the same TagViewSet — permission_classes already cover the auth path; no behavior change for callers that only GET. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ConnectorInstanceSerializer.to_representation overrides connector_mode from the catalog every response, so any client-supplied value is silently discarded. Make that explicit via extra_kwargs so DRF OPTIONS reports the field as read-only and round-trippers don't trip the choice validator (catalog mode 'FILESYSTEM' vs model choice 'FILE_SYSTEM'). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Needed by the org-to-org migration SDK's idempotent get-or-create flow: without a name filter the SDK had to list-all-then-linear-search every time. Adds WorkflowKey.WF_NAME to the existing FilterHelper.build_filter_args call — same shape as the recently-added adapter_name / connector_name / tag name filters. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Needed by the org-to-org migration SDK: after CustomTool migration the target's PromptStudioRegistry gets a freshly-minted prompt_registry_id, and downstream ToolInstance migration needs to remap source.tool_id -> target.tool_id (both are registry UUIDs). With this filter the SDK can GET /prompt-studio/registry/?custom_tool=<tool_id> on either side to resolve the registry id without needing to know it up front. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the existing exact-match name filters added for adapter, connector, tag, and workflow lists. Migration SDK's get-or-create flow queries by name on the target before deciding fresh vs adopt; without this the SDK had to list-all-then-linear-scan every time. Pipeline keeps the icontains ?search= alongside; api_deployment keeps the icontains ?search= alongside. New filters are exact-match only. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Greptile flagged on PR #1987 that the prior switch from `created_by=user` to `workflow__in=Workflow.for_user(user)` silently widened tool-instance visibility for regular users with shared-workflow access — they would start seeing every member's tool instances in those workflows, not just their own. Restrict the widened queryset to `is_service_account` callers so the migration SDK still gets org-wide enumeration via Platform API keys, while regular users keep their original per-creator scope. Reported-by: greptile-apps[bot] (PR #1987, P1)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tform-api-gaps Conflict resolutions: - permissions/permission.py, prompt_studio/permission.py: kept main's owner/admin/group-share logic; branch's service-account allowances already merged cleanly above the conflicts. - adapter_processor_v2/models.py: took main's admin short-circuit + group-share visibility in for_user (defines group_shared_ids used below). - connector_v2/serializers.py: kept main's shared_users/shared_to_org read-only kwargs alongside branch's read-only connector_mode. - tool_instance_v2/views.py: dropped branch's service-account-only queryset widening as redundant — main's created_by | accessible-workflows union already grants org-wide access to service accounts via Workflow.for_user's short-circuit. - api_v2/api_deployment_views.py, pipeline_v2/views.py: comment-only conflicts; kept branch's generic phrasing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Group CRUD, member add/remove, and the admin-only resources action were
gated on the org-admin role, which is deliberately False for service
accounts — blocking platform-key automation (org-to-org clone needs to
recreate group shells + memberships). Service accounts bypass
authorization here for the same reason they do in
ShareAuthorizationService: they already bypass other access controls.
- Add _is_admin_or_service_account() write gate in group_views and use it
in IsOrgAdminForWrite plus the inline members/remove_member/resources
checks and the list ?member=<id> filter.
- is_org_admin() in sharing_helpers is intentionally unchanged; it drives
resource-visibility semantics where service accounts are handled
separately.
- Verified GET /users/ already works for platform keys (no admin gate;
returns id/email/role/is_admin) — no change needed.
- Expose is_service_account in the GET /users/ and groups/{pk}/members/
listing rows so API clients can distinguish platform-key identities
without inferring from the email suffix. The users listing itself
still excludes service accounts (unchanged); select_related the user
to avoid N+1 in member serialization.
- Tests: service-account create/add/remove/resources/delete-group,
non-admin member 403 matrix, listing flag/exclusion cases (23 green
via manage.py test).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Summary by CodeRabbit
WalkthroughThis PR extends service account authorization for group management operations. Service accounts can now perform group creation, member management, and resource listing operations alongside organization admins. The API serializers expose ChangesService Account Authorization for Group Operations
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
Contributor
|
| Filename | Overview |
|---|---|
| backend/tenant_account_v2/group_views.py | Adds _is_admin_or_service_account helper and wires it into all write/admin gates; logic is consistent across all action methods and the permission class. |
| backend/tenant_account_v2/organization_member_service.py | Adds select_related("user") to get_members(); the sibling get_members_by_role() is identical in shape but was left without the same optimization. |
| backend/tenant_account_v2/group_serializers.py | Adds is_service_account field to GroupMemberSerializer with correct source and read_only; callers already use select_related("user") so no N+1 risk. |
| backend/tenant_account_v2/serializer.py | Adds is_service_account field to OrganizationMemberSerializer; field will always be False in the users-list endpoint since get_members() already filters service accounts out. |
| backend/tenant_account_v2/tests.py | Good coverage of the new service-account write gates and flag serialization; patch target is correct given the lazy-import pattern in group_views.py. |
| backend/api_v2/api_deployment_views.py | Comment-only change; no logic affected. |
| backend/connector_v2/serializers.py | Comment-only change; no logic affected. |
| backend/pipeline_v2/views.py | Comment-only change; no logic affected. |
Sequence Diagram
sequenceDiagram
participant CLI as unstract clone CLI
participant API as Platform API
participant Perm as IsOrgAdminForWrite
participant Gate as _is_admin_or_service_account
participant SA as is_service_account (User)
participant Admin as is_org_admin (sharing_helpers)
CLI->>API: POST /groups/ (platform-key auth)
API->>Perm: has_permission(request, view)
Perm->>Gate: check write access
Gate->>SA: getattr(user, is_service_account, False)
SA-->>Gate: True
Gate-->>Perm: True
Perm-->>API: Allowed
CLI->>API: "POST /groups/{pk}/members/ (platform-key auth)"
API->>Gate: _is_admin_or_service_account(request)
Gate->>SA: is_service_account?
SA-->>Gate: True
Gate-->>API: Allowed
API-->>CLI: 201 Created
Note over API,Admin: Human admin path (unchanged)
CLI->>API: POST /groups/ (human admin)
API->>Perm: has_permission
Perm->>Gate: check write
Gate->>SA: is_service_account?
SA-->>Gate: False
Gate->>Admin: is_org_admin(user)
Admin-->>Gate: True
Gate-->>Perm: True
Perm-->>API: Allowed
Comments Outside Diff (1)
-
backend/tenant_account_v2/organization_member_service.py, line 78-80 (link)The
select_related("user")N+1 fix was applied toget_members()but the siblingget_members_by_role()has the identical access pattern — serializers readinguser.email/user.idper row will issue one extra query per member.Prompt To Fix With AI
This is a comment left during a code review. Path: backend/tenant_account_v2/organization_member_service.py Line: 78-80 Comment: The `select_related("user")` N+1 fix was applied to `get_members()` but the sibling `get_members_by_role()` has the identical access pattern — serializers reading `user.email` / `user.id` per row will issue one extra query per member. How can I resolve this? If you propose a fix, please make it concise.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
backend/tenant_account_v2/organization_member_service.py:78-80
The `select_related("user")` N+1 fix was applied to `get_members()` but the sibling `get_members_by_role()` has the identical access pattern — serializers reading `user.email` / `user.id` per row will issue one extra query per member.
```suggestion
return OrganizationMember.objects.select_related("user").filter(
role=role, user__is_service_account=False
).order_by("member_id")
```
Reviews (1): Last reviewed commit: "feat(platform-api): allow service accoun..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tenant_account_v2/organization_member_service.py`:
- Around line 62-66: The get_members() query in OrganizationMemberService
currently only filters out service accounts and does not constrain by
organization, causing cross-tenant member leakage; update the call chain so
AuthenticationController.get_organization_members_by_org_id(org_id) accepts and
forwards an organization_id and change OrganizationMemberService.get_members to
accept an organization_id parameter and filter the queryset by
organization=organization_id (e.g.,
OrganizationMember.objects.select_related("user").filter(organization_id=organization_id,
user__is_service_account=False)), and ensure
OrganizationUserViewSet.get_organization_members passes the organization_id
through to AuthenticationController.
🪄 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
Run ID: 9cd2284f-454c-4dc1-abbc-7036aa706438
📒 Files selected for processing (8)
backend/api_v2/api_deployment_views.pybackend/connector_v2/serializers.pybackend/pipeline_v2/views.pybackend/tenant_account_v2/group_serializers.pybackend/tenant_account_v2/group_views.pybackend/tenant_account_v2/organization_member_service.pybackend/tenant_account_v2/serializer.pybackend/tenant_account_v2/tests.py
Comment on lines
62
to
+66
| def get_members() -> list[OrganizationMember]: | ||
| return OrganizationMember.objects.filter(user__is_service_account=False) | ||
| # select_related: serializers read user.email/id per member row. | ||
| return OrganizationMember.objects.select_related("user").filter( | ||
| user__is_service_account=False | ||
| ) |
Contributor
There was a problem hiding this comment.
Filter /users/ by organization before serializing members.
OrganizationUserViewSet.get_organization_members() serializes the result of AuthenticationController.get_organization_members_by_org_id(), and the provided snippet for that controller method forwards straight to OrganizationMemberService.get_members(). This queryset only excludes service accounts; it never constrains organization, so the org-members endpoint will return human members from every tenant once the table contains more than one organization. That is a cross-tenant privacy leak.
Suggested fix
- def get_members() -> list[OrganizationMember]:
- # select_related: serializers read user.email/id per member row.
- return OrganizationMember.objects.select_related("user").filter(
- user__is_service_account=False
- )
+ def get_members(organization_id: str) -> list[OrganizationMember]:
+ # select_related: serializers read user.email/id per member row.
+ return OrganizationMember.objects.select_related("user").filter(
+ organization__organization_id=organization_id,
+ user__is_service_account=False,
+ )Then pass the already-available organization_id through AuthenticationController.get_organization_members_by_org_id(...) instead of dropping it.
🤖 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/tenant_account_v2/organization_member_service.py` around lines 62 -
66, The get_members() query in OrganizationMemberService currently only filters
out service accounts and does not constrain by organization, causing
cross-tenant member leakage; update the call chain so
AuthenticationController.get_organization_members_by_org_id(org_id) accepts and
forwards an organization_id and change OrganizationMemberService.get_members to
accept an organization_id parameter and filter the queryset by
organization=organization_id (e.g.,
OrganizationMember.objects.select_related("user").filter(organization_id=organization_id,
user__is_service_account=False)), and ensure
OrganizationUserViewSet.get_organization_members passes the organization_id
through to AuthenticationController.
muhammad-ali-e
approved these changes
Jun 15, 2026
Comment on lines
+58
to
+59
| if getattr(request.user, "is_service_account", False): | ||
| return True |
Contributor
There was a problem hiding this comment.
NIT: better to add a standalone methode like _is_org_admin
| # select_related: serializers read user.email/id per member row. | ||
| return OrganizationMember.objects.select_related("user").filter( | ||
| user__is_service_account=False | ||
| ) |
Contributor
There was a problem hiding this comment.
NIT: How it differ from the previous query?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Service-account (platform-key) support for org-to-org clone automation:
?name=filters on adapter, connector, tag, workflow, pipeline, api-deployment list endpointsis_service_accountflag exposed on GET /users/ and group members listingWhy
The org-to-org clone CLI (
unstract clonein unstract-python-client) and customer automation authenticate with platform keys → service accounts. UN-2977 group CRUD/membership writes were admin-gated;is_org_admin()deliberately returns False for service accounts, blocking group cloning and post-clone membership automation. Same bypass doctrine as ShareAuthorizationService: service accounts bypass authorization — they already bypass other access controls.How
_is_admin_or_service_accounthelper in tenant_account_v2/group_views.py gates group writesis_service_accountproperty on User model, exposed on list endpointsCan this PR break any existing features. If yes, please list possible items. If no, please explain why.
No. Service accounts were previously blocked from group writes and are now allowed through explicit gates. The gates mirror existing authorization patterns (admin checks). Service account filtering/exposure is read-only and optional (new flags only returned if service account checks pass). Users list endpoint behavior unchanged.
Database Migrations
None required.
Env Config
None required.
Relevant Docs
Related Issues or PRs
Dependencies Versions
None changed.
Notes on Testing
manage.py test tenant_account_v2→ 23 tests OK:is_service_accountflagE2E on chandru-unstract-dev namespace: full
unstract clonerun cloned 3 groups (including empty group), memberships email-matched (missing users skipped), group/user/org share state replicated and verified via API. Idempotent rerun adopted everything.Screenshots
N/A
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code