UN-2977 [FEAT] Introduce users groups with project sharing #1986
Open
kirtimanmishrazipstack wants to merge 18 commits into
Open
UN-2977 [FEAT] Introduce users groups with project sharing #1986kirtimanmishrazipstack wants to merge 18 commits into
kirtimanmishrazipstack wants to merge 18 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
<review_stack_artifact> </review_stack_artifact> ✨ Finishing Touches🧪 Generate unit tests (beta)
|
…group, and the resource has been shared to that group
…y-s-Introduce-Group-Based-Project-Sharing
Contributor
|
| Filename | Overview |
|---|---|
| backend/tenant_account_v2/group_views.py | New GroupViewSet with CRUD and member management; AgenticProject omitted from blast-radius resources endpoint; members GET is open to all org members (inconsistent with the admin-gated resources endpoint). |
| backend/tenant_account_v2/sharing_helpers.py | New central module for group-based resource sharing logic; well-structured with proper UUID casting workaround, correct transaction atomicity, and a clean ShareAuthorizationService. |
| backend/tenant_account_v2/group_serializers.py | Serializers for group CRUD and member management; quota validation has a TOCTOU race that can let concurrent creates slightly exceed MAX_GROUPS_PER_ORG/MAX_MEMBERS_PER_GROUP. |
| backend/tenant_account_v2/signals.py | New signal wiring for org-member removal and resource-delete cleanup; correct cascade semantics and atomic transaction, but cleanup_user_org_access uses N+1 individual remove() calls. |
| backend/permissions/resource_share_views.py | New ResourceShareManagementMixin providing share/ and effective-members/ actions; clean axis-agnostic design with proper input coercion and correct delegation to ShareAuthorizationService. |
| backend/permissions/permission.py | Adds has_group_access() helper and wires it into IsOwnerOrSharedUser and IsOwnerOrSharedUserOrSharedToOrg; correct lazy import to avoid circular deps at startup. |
| backend/tenant_account_v2/migrations/0003_resource_group_share.py | Migration for the polymorphic ResourceGroupShare table; correct indexes on (content_type, object_id) and (organization, group), unique constraint on (group, content_type, object_id). |
| backend/tenant_account_v2/migrations/0002_organization_group_group_membership.py | Migration for OrganizationGroup and GroupMembership; correct unique constraints and composite index on (user, group) for the for_user() subquery path. |
Sequence Diagram
sequenceDiagram
participant Admin
participant GroupViewSet
participant ShareModal
participant ResourceViewSet
participant ShareAuthorizationService
participant ResourceGroupShare
participant ForUser as Resource.for_user()
Admin->>GroupViewSet: POST /groups/ (create group)
Admin->>GroupViewSet: "POST /groups/{id}/members/ (add users)"
GroupViewSet->>GroupMembership: bulk_create memberships
Admin->>ShareModal: Open share modal for a Workflow
ShareModal->>ResourceViewSet: "GET /workflow/{id}/ (shared_groups property)"
ShareModal->>GroupViewSet: GET /groups/ (list groups)
Admin->>ShareModal: Apply (add group)
ShareModal->>ResourceViewSet: "POST /workflow/{id}/share/ {shared_groups: [id]}"
ResourceViewSet->>ShareAuthorizationService: authorize_and_commit(actor, resource, desired)
ShareAuthorizationService->>ResourceGroupShare: upsert rows
User->>ForUser: list workflows
ForUser->>GroupMembership: get user_group_ids
ForUser->>ResourceGroupShare: get group-shared workflow UUIDs
ForUser-->>User: "union(owned | shared_users | shared_to_org | group_shared)"
Admin->>GroupViewSet: "DELETE /groups/{id}/ (delete group)"
GroupViewSet->>ResourceGroupShare: CASCADE delete all group shares
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
backend/tenant_account_v2/group_views.py:196-222
**`AgenticProject` omitted from blast-radius resource listing**
`_collect_resources_shared_with_group` enumerates six resource types but not `AgenticProject`, which is listed in `signals._SHAREABLE_MODELS` and therefore does receive `ResourceGroupShare` rows in cloud deployments. When an admin opens the delete-confirm dialog for a group that has agentic projects shared with it, the endpoint returns an incomplete list — the admin sees no agentic projects in the "affected resources" count and proceeds without realising those projects will silently lose group access on deletion. The data is correctly cascade-deleted (via `ResourceGroupShare → OrganizationGroup` FK), but the UI gives a misleading blast-radius estimate.
The `AgenticProject` import should be guarded with a `try/except LookupError` (mirroring the signal handler's pattern) and appended to `sources` when the app is installed.
### Issue 2 of 4
backend/tenant_account_v2/group_serializers.py:68-70
**Group quota check is a TOCTOU race — concurrent creates can exceed `MAX_GROUPS_PER_ORG`**
Two simultaneous POST requests can both read the same `current` count, both pass the `< MAX_GROUPS_PER_ORG` guard, and both commit, leaving the org with `current + 2` groups. The unique constraint on `(organization, name)` prevents duplicates with the same name but does not cap the total count. Wrapping the count-and-insert in a `select_for_update` on the organization row would make this atomic.
```suggestion
if self.instance is None:
from account_v2.models import Organization
# Lock the organization row to make the count-and-create atomic.
Organization.objects.filter(pk=organization.pk).select_for_update().get()
current = OrganizationGroup.objects.filter(organization=organization).count()
if current >= settings.MAX_GROUPS_PER_ORG:
```
### Issue 3 of 4
backend/tenant_account_v2/group_views.py:119-136
**`members` GET exposes group membership to any org member**
`IsOrgAdminForWrite.has_permission` returns `True` for all GET requests, so any authenticated org member can call `GET /groups/{pk}/members/` and enumerate the full membership list of any group in their org — including groups they are not in. The `resources` endpoint on the same viewset was explicitly gated to admins with the comment "Leaving it open to any org member would leak… resources shared with groups they are not in". The same reasoning applies here: a non-admin can discover that `alice@company.com` belongs to "Executive Board" or "Finance Team" without being a member of those groups.
If org-wide member visibility is intentional, a brief code comment stating that would prevent this from being revisited. If it should be admin-only, adding the same `if not _is_org_admin(request): raise PermissionDenied(...)` guard as in `resources` and `remove_member` would close the gap.
### Issue 4 of 4
backend/tenant_account_v2/signals.py:64-81
**N+1 `shared_users.remove()` calls in `cleanup_user_org_access`**
For each resource type the signal fetches every resource the departing user was shared with, then calls `resource.shared_users.remove(instance.user)` once per row — one `DELETE` statement per resource. If a user was shared with 50 workflows the signal issues 50 individual deletes. The same result can be achieved with a single bulk delete against the M2M through table: `model.shared_users.through.objects.filter(user=instance.user, <fk_to_model>__organization=instance.organization).delete()`. This runs entirely inside the existing `transaction.atomic()` block so rollback semantics are unchanged.
Reviews (9): Last reviewed commit: "Merge branch 'main' of github.com:Zipsta..." | 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
backend/tenant_account_v2/views.py (1)
72-77: ⚡ Quick winLog the traceback for 500-path exceptions.
Use
logger.exception(...)(orexc_info=True) here so incident debugging has stack context.Proposed fix
- except Exception as error: - logger.error("Error while get User : %s", error) + except Exception: + logger.exception("Error while getting user organization") return Response( status=status.HTTP_500_INTERNAL_SERVER_ERROR, data={"message": "Internal Error"}, )🤖 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/views.py` around lines 72 - 77, Replace the plain logger.error call in the exception handler in tenant_account_v2/views.py (the except Exception as error block that currently logs "Error while get User : %s" and returns a 500 Response) with a logging call that records the full traceback (use logger.exception(...) or logger.error(..., exc_info=True)) so the stack context is captured for the 500-path; keep the same Response payload and status but ensure the logger call references the same message/context used currently.frontend/src/components/groups/groups-service.js (1)
13-16: ⚡ Quick winRename this to a custom hook (
useGroupsService) to match hook usage.This function calls React hooks (
useAxiosPrivate,useSessionStore) but is not named like a hook. Renaming touseGroupsServicereduces invalid-call risk and aligns with hook rules across tooling.🤖 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 `@frontend/src/components/groups/groups-service.js` around lines 13 - 16, The function groupsService calls React hooks (useAxiosPrivate and useSessionStore) but isn't named as a hook; rename groupsService to useGroupsService and update all imports/usages accordingly to follow hook naming conventions and avoid invalid-hook warnings; ensure the internal logic (including the path construction using sessionDetails?.orgId.replaceAll) and any exported symbol names are updated to the new name so consumers import useGroupsService instead of groupsService.backend/tenant_account_v2/group_views.py (1)
127-128: 💤 Low valueFragile access to permission class message.
Accessing
self.permission_classes[1].messagerelies on the exact ordering of thepermission_classeslist. If the order changes or a class is added/removed, this will silently use the wrong message or raise an IndexError.Consider using the class directly:
♻️ Proposed fix
- if not _is_org_admin(request): - raise PermissionDenied(self.permission_classes[1].message) + if not _is_org_admin(request): + raise PermissionDenied(IsOrgAdminForWrite.message)Apply the same fix at line 150.
Also applies to: 149-150
🤖 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/group_views.py` around lines 127 - 128, The code currently reads a permission message via an index (self.permission_classes[1].message) which is fragile; replace that indexed access with the explicit permission class's message (e.g., OrgAdminPermission.message or whatever concrete class is present in your permission_classes) so the code reads raise PermissionDenied(OrgAdminPermission.message) or the equivalent direct class reference; update both occurrences (the check that calls _is_org_admin and the later occurrence at the other spot) and import or reference the actual permission class name instead of relying on list ordering.frontend/src/components/groups/GroupMemberManager.jsx (1)
155-159: ⚡ Quick win
onCloseprop is optional but required for modal dismissal.The
onCloseprop is marked as optional in PropTypes (no.isRequired), but it's passed directly toModal.onCancel. While this won't crash (Ant Design handles undefined), the modal can't be closed without it.♻️ Mark onClose as required
GroupMemberManager.propTypes = { open: PropTypes.bool.isRequired, group: PropTypes.object, - onClose: PropTypes.func, + onClose: PropTypes.func.isRequired, };🤖 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 `@frontend/src/components/groups/GroupMemberManager.jsx` around lines 155 - 159, The PropTypes declaration for GroupMemberManager marks onClose optional but the component passes it to Modal.onCancel; update GroupMemberManager.propTypes so onClose is required (change the onClose entry to PropTypes.func.isRequired) to ensure consumers provide a dismissal handler for the Modal.onCancel; keep open and group prop definitions unchanged.frontend/src/hooks/useShareModal.js (1)
40-48: 💤 Low valueHandle potential undefined
groupsResponsemore explicitly.When
groupsApiis not provided,groupsResponsewill beundefined. The code at lines 78-86 handles this with the?.datacheck, but destructuring fromPromise.allwith a variable array length could be clearer.♻️ Consider explicit handling
- const [usersResponse, sharedUsersResponse, groupsResponse] = - await Promise.all(calls); + const results = await Promise.all(calls); + const [usersResponse, sharedUsersResponse] = results; + const groupsResponse = groupsApi?.listGroups ? results[2] : undefined;Also applies to: 78-86
🤖 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 `@frontend/src/hooks/useShareModal.js` around lines 40 - 48, The destructuring from Promise.all in useShareModal assumes three results but calls may only contain two when groupsApi is absent; fix by ensuring calls always has the third promise (e.g., if groupsApi?.listGroups is falsy push Promise.resolve({ data: [] }) so the array length is constant) and then safely destructure into usersResponse, sharedUsersResponse, groupsResponse; reference the calls array, groupsApi.listGroups and the Promise.all destructuring in useShareModal to implement this change.frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx (1)
345-352: ⚡ Quick winModal closes even on share failure.
The
setOpenSharePermissionModal(false)is in.finally(), so the modal closes even when sharing fails. This matches theuseShareModalhook behavior (lines 118-124 in that file), but users won't see their failed changes. Consider keeping the modal open on error so users can retry.♻️ Keep modal open on failure
axiosPrivate(requestOptions) + .then(() => { + setOpenSharePermissionModal(false); + }) .catch((err) => { setAlertDetails(handleException(err, "Failed to load")); - }) - .finally(() => { - setOpenSharePermissionModal(false); });🤖 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 `@frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx` around lines 345 - 352, The modal is always closed because setOpenSharePermissionModal(false) is in the .finally() of the axiosPrivate call; change control so the modal is only closed on success: remove setOpenSharePermissionModal(false) from .finally(), and instead call it inside the success path (the .then or after a successful axiosPrivate response) while leaving it out of the .catch so the modal remains open on errors; keep the existing error handling (setAlertDetails(handleException(err, "Failed to load"))) in the catch so users see the failure and can retry (refer to axiosPrivate, setAlertDetails, handleException, and setOpenSharePermissionModal in ListOfTools.jsx and the useShareModal behavior).
🤖 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/adapter_processor_v2/serializers.py`:
- Around line 29-41: The view currently reads request.data["shared_users"] in
AdapterInstanceViewSet.partial_update even though BaseAdapterSerializer marks
shared_users as read_only, causing side-effects from ignored payloads; update
partial_update to only run the "clear UserDefaultAdapter links" logic when the
incoming payload actually intends to change shared_users — i.e., check that
"shared_users" is present in request.data AND that the serializer will accept it
(for example ensure serializer.fields["shared_users"].read_only is False or that
"shared_users" appears in serializer.initial_data/validated_data) before
performing the clear/unshare operations, leaving existing behavior unchanged
otherwise.
In `@backend/adapter_processor_v2/views.py`:
- Around line 364-368: The code in partial_update reads request_data and
constructs requested_user_ids with int(...) before serializer validation, which
can raise ValueError for non-integer inputs; update the logic in the
partial_update handler (where requested_user_ids, current_shared_users,
removed_user_ids are computed) to defensively validate shared_users from
request_data: parse/iterate the list and catch non-integer values, and if any
invalid entries exist raise a serializers.ValidationError (or return a 400
response) with a clear message instead of allowing a ValueError to bubble up;
ensure you reference request_data.get("shared_users", []) and build
requested_user_ids only after validating/converting each entry.
In `@backend/tenant_account_v2/share_serializer_mixin.py`:
- Around line 32-44: The create and update flows call
super().create/super().update and then set_resource_share_groups separately,
risking partial writes; wrap the call to super().create (in create) and
super().update (in update) together with the subsequent
set_resource_share_groups call inside a single DB transaction (e.g., using
Django's transaction.atomic()) so both the model save and the group-sharing
update commit or roll back together; modify the methods (create and update in
ShareSerializerMixin) to perform the create/update and the
set_resource_share_groups(instance, [g.id for g in groups]) call within the same
atomic block.
In `@backend/workflow_manager/workflow_v2/urls/workflow.py`:
- Around line 29-30: The new POST share endpoint is currently allowed by the
broader IsOwnerOrSharedUserOrSharedToOrg path; update
WorkflowViewSet.get_permissions so that "share" is included in the same action
set as "destroy", "update", and "partial_update" (i.e., make "share" use the
owner-only permission class returned for those actions) to ensure only owners
can re-share workflows.
In `@frontend/src/components/groups/GroupCreateEditModal.jsx`:
- Around line 33-36: The submit logic currently dereferences group.id unguarded
when building call (mode === "edit" ? service.updateGroup(group.id, values) :
service.createGroup(values)), which can throw if props.group is undefined;
update the handler (the code that sets call, and similar logic around lines
handling update in the submit flow) to first assert or guard that group exists
in edit mode (e.g., if mode === "edit" and !group, abort/throw/return early or
use group?.id and surface an explicit error), and use
service.updateGroup(group.id, values) only after confirming group is defined;
apply the same guard to the similar edit-path at the other location referenced
(around the 82-86 edit logic).
In `@frontend/src/components/groups/groups-service.js`:
- Around line 16-17: The code builds path using
sessionDetails?.orgId.replaceAll(...) which will throw if orgId is
null/undefined; update the expression that computes path so you guard orgId
before calling replaceAll—use optional chaining on orgId
(sessionDetails?.orgId?.replaceAll(...)) or coerce to a safe default (e.g.,
(sessionDetails?.orgId || "").replaceAll(...)) and keep csrfToken usage
unchanged; modify the expression that defines path to reference sessionDetails
and orgId safely.
In `@frontend/src/components/groups/Groups.jsx`:
- Around line 195-200: TopBar's built-in search assumes user rows with fields
like username/email, causing a crash when Groups.jsx passes group rows; update
the wiring so TopBar searches group fields instead: add a new prop to TopBar
(e.g., searchKey or searchFields) and change TopBar.jsx’s search logic
(currently using username.includes(...)/email) to iterate over the provided
key(s) and test those string fields, then in Groups.jsx pass searchKey="name" or
searchFields={['name','description','member_count']} along with the existing
searchData and setFilteredUserList so the filter operates on valid group fields
instead of username/email.
In `@frontend/src/components/navigations/side-nav-bar/SideNavBar.jsx`:
- Around line 126-134: The Platform main nav item in the SideNavBar component
doesn't remain active on the new Groups page because the Platform "active"
condition doesn't include the groups path; update the Platform active-state
check (the expression that computes whether the Platform item is active) to also
consider the Groups route by adding `/${orgName}/groups` (or checking pathname
includes `/groups`) to the boolean expression that determines active, and apply
the same change to the duplicate active-check occurrence mentioned (the earlier
block around lines 162-164) so both places treat the Groups route as part of
Platform.
In `@frontend/src/components/tool-settings/tool-settings/ToolSettings.jsx`:
- Around line 213-215: The code currently closes the share modal in a .finally
block which hides it on both success and failure; remove
setOpenSharePermissionModal(false) from the .finally and instead call
setOpenSharePermissionModal(false) only in the successful resolution handler
(the .then callback of the share request promise), leaving the modal open on
errors so users can retry; ensure the .catch still surfaces the error (e.g.,
error toast/log) but does not close the modal.
---
Nitpick comments:
In `@backend/tenant_account_v2/group_views.py`:
- Around line 127-128: The code currently reads a permission message via an
index (self.permission_classes[1].message) which is fragile; replace that
indexed access with the explicit permission class's message (e.g.,
OrgAdminPermission.message or whatever concrete class is present in your
permission_classes) so the code reads raise
PermissionDenied(OrgAdminPermission.message) or the equivalent direct class
reference; update both occurrences (the check that calls _is_org_admin and the
later occurrence at the other spot) and import or reference the actual
permission class name instead of relying on list ordering.
In `@backend/tenant_account_v2/views.py`:
- Around line 72-77: Replace the plain logger.error call in the exception
handler in tenant_account_v2/views.py (the except Exception as error block that
currently logs "Error while get User : %s" and returns a 500 Response) with a
logging call that records the full traceback (use logger.exception(...) or
logger.error(..., exc_info=True)) so the stack context is captured for the
500-path; keep the same Response payload and status but ensure the logger call
references the same message/context used currently.
In `@frontend/src/components/custom-tools/list-of-tools/ListOfTools.jsx`:
- Around line 345-352: The modal is always closed because
setOpenSharePermissionModal(false) is in the .finally() of the axiosPrivate
call; change control so the modal is only closed on success: remove
setOpenSharePermissionModal(false) from .finally(), and instead call it inside
the success path (the .then or after a successful axiosPrivate response) while
leaving it out of the .catch so the modal remains open on errors; keep the
existing error handling (setAlertDetails(handleException(err, "Failed to
load"))) in the catch so users see the failure and can retry (refer to
axiosPrivate, setAlertDetails, handleException, and setOpenSharePermissionModal
in ListOfTools.jsx and the useShareModal behavior).
In `@frontend/src/components/groups/GroupMemberManager.jsx`:
- Around line 155-159: The PropTypes declaration for GroupMemberManager marks
onClose optional but the component passes it to Modal.onCancel; update
GroupMemberManager.propTypes so onClose is required (change the onClose entry to
PropTypes.func.isRequired) to ensure consumers provide a dismissal handler for
the Modal.onCancel; keep open and group prop definitions unchanged.
In `@frontend/src/components/groups/groups-service.js`:
- Around line 13-16: The function groupsService calls React hooks
(useAxiosPrivate and useSessionStore) but isn't named as a hook; rename
groupsService to useGroupsService and update all imports/usages accordingly to
follow hook naming conventions and avoid invalid-hook warnings; ensure the
internal logic (including the path construction using
sessionDetails?.orgId.replaceAll) and any exported symbol names are updated to
the new name so consumers import useGroupsService instead of groupsService.
In `@frontend/src/hooks/useShareModal.js`:
- Around line 40-48: The destructuring from Promise.all in useShareModal assumes
three results but calls may only contain two when groupsApi is absent; fix by
ensuring calls always has the third promise (e.g., if groupsApi?.listGroups is
falsy push Promise.resolve({ data: [] }) so the array length is constant) and
then safely destructure into usersResponse, sharedUsersResponse, groupsResponse;
reference the calls array, groupsApi.listGroups and the Promise.all
destructuring in useShareModal to implement this change.
🪄 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: ab651623-939b-44d0-b5eb-5da241ab7926
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (64)
backend/adapter_processor_v2/models.pybackend/adapter_processor_v2/serializers.pybackend/adapter_processor_v2/urls.pybackend/adapter_processor_v2/views.pybackend/api_v2/api_deployment_views.pybackend/api_v2/models.pybackend/api_v2/serializers.pybackend/api_v2/urls.pybackend/backend/settings/base.pybackend/connector_v2/models.pybackend/connector_v2/serializers.pybackend/connector_v2/urls.pybackend/connector_v2/views.pybackend/permissions/permission.pybackend/permissions/resource_share_views.pybackend/pipeline_v2/models.pybackend/pipeline_v2/serializers/crud.pybackend/pipeline_v2/serializers/sharing.pybackend/pipeline_v2/urls.pybackend/pipeline_v2/views.pybackend/prompt_studio/permission.pybackend/prompt_studio/prompt_studio_core_v2/models.pybackend/prompt_studio/prompt_studio_core_v2/serializers.pybackend/prompt_studio/prompt_studio_core_v2/urls.pybackend/prompt_studio/prompt_studio_core_v2/views.pybackend/sample.envbackend/tenant_account_v2/admin.pybackend/tenant_account_v2/apps.pybackend/tenant_account_v2/group_serializers.pybackend/tenant_account_v2/group_views.pybackend/tenant_account_v2/groups_urls.pybackend/tenant_account_v2/migrations/0002_organization_group_group_membership.pybackend/tenant_account_v2/migrations/0003_resource_group_share.pybackend/tenant_account_v2/models.pybackend/tenant_account_v2/share_serializer_mixin.pybackend/tenant_account_v2/sharing_helpers.pybackend/tenant_account_v2/signals.pybackend/tenant_account_v2/urls.pybackend/tenant_account_v2/views.pybackend/workflow_manager/workflow_v2/models/workflow.pybackend/workflow_manager/workflow_v2/serializers.pybackend/workflow_manager/workflow_v2/urls/workflow.pybackend/workflow_manager/workflow_v2/views.pyfrontend/src/components/custom-tools/list-of-tools/ListOfTools.jsxfrontend/src/components/deployments/api-deployment/ApiDeployment.jsxfrontend/src/components/deployments/api-deployment/api-deployments-service.jsfrontend/src/components/groups/GroupCreateEditModal.jsxfrontend/src/components/groups/GroupMemberManager.jsxfrontend/src/components/groups/Groups.cssfrontend/src/components/groups/Groups.jsxfrontend/src/components/groups/groups-service.jsfrontend/src/components/navigations/side-nav-bar/SideNavBar.jsxfrontend/src/components/pipelines-or-deployments/pipeline-service.jsfrontend/src/components/pipelines-or-deployments/pipelines/Pipelines.jsxfrontend/src/components/tool-settings/tool-settings/ToolSettings.jsxfrontend/src/components/widgets/share-permission/SharePermission.cssfrontend/src/components/widgets/share-permission/SharePermission.jsxfrontend/src/components/workflows/workflow/Workflows.jsxfrontend/src/components/workflows/workflow/workflow-service.jsfrontend/src/hooks/useShareModal.jsfrontend/src/index.jsxfrontend/src/pages/ConnectorsPage.jsxfrontend/src/pages/GroupsPage.jsxfrontend/src/routes/useMainAppRoutes.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/permissions/permission.py (1)
27-44: ⚡ Quick winDefensive guard for unauthenticated callers in
has_group_access(...)
CustomAuthMiddlewarereturns401for unauthenticated requests on non-whitelisted/non-internal routes, so these permission checks should not normally run withrequest.user=AnonymousUser. Still,has_group_access(...)directly readsuser.group_membershipswithout an auth check, so adding an early return prevents brittle 500s if call sites/middleware behavior change later.💡 Proposed fix
def has_group_access(user: Any, obj: Any) -> bool: """Check if a user has access to a resource via group membership. Reads from the polymorphic ``ResourceGroupShare`` table rather than a per-resource ``shared_groups`` M2M (see UN-2977). Callers can OR this in safely for any resource — non-shareable objects yield no rows. """ + if not getattr(user, "is_authenticated", False): + return False + # Lazy import — ``permissions`` is imported by `account_v2`/`api_v2` # before `tenant_account_v2` finishes loading. from django.contrib.contenttypes.models import ContentType from tenant_account_v2.models import ResourceGroupShare🤖 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/permissions/permission.py` around lines 27 - 44, has_group_access currently assumes user.group_memberships exists and will 500 for unauthenticated/None users; add a defensive early return (False) at the top of has_group_access to handle unauthenticated or missing-user cases by checking getattr(user, "is_authenticated", False) (or user is None) and/or absence of group_memberships before accessing user.group_memberships; keep the rest of the logic using ContentType.objects.get_for_model(type(obj)) and ResourceGroupShare as-is.
🤖 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/permissions/permission.py`:
- Around line 27-44: has_group_access currently assumes user.group_memberships
exists and will 500 for unauthenticated/None users; add a defensive early return
(False) at the top of has_group_access to handle unauthenticated or missing-user
cases by checking getattr(user, "is_authenticated", False) (or user is None)
and/or absence of group_memberships before accessing user.group_memberships;
keep the rest of the logic using ContentType.objects.get_for_model(type(obj))
and ResourceGroupShare as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 492cc0b3-3de5-458d-8a27-0939db2156b0
📒 Files selected for processing (7)
backend/adapter_processor_v2/models.pybackend/api_v2/models.pybackend/connector_v2/models.pybackend/permissions/permission.pybackend/pipeline_v2/models.pybackend/prompt_studio/prompt_studio_core_v2/models.pybackend/workflow_manager/workflow_v2/models/workflow.py
- group list: validate ?member= as int (400 not 500)
- group views: reference IsOrgAdminForWrite.message, not list index
- sharing_helpers: cast UUID ids in list_resources_shared_with_group
(fixes GET /groups/{pk}/resources/ crash); treat org-less user as
non-admin without error-logging noise
- share serializer mixin: make model save + group-share write atomic
- adapter: move default-adapter cleanup to the share action where
unsharing actually happens (shared_users is read-only on PATCH)
- frontend: guard group id on edit, orgId before replaceAll, TopBar
search key for non-user rows, /groups nav active state, keep share
modal open on failure
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/adapter_processor_v2/views.py (1)
151-157:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRestrict
shareaction to owners/admin-authorized roles only.Line 154 puts
sharebehindIsOwnerOrSharedUserOrSharedToOrg, which allows already-shared users to mutate sharing state. That is an ACL escalation path for non-owners. Keepshareunder owner-only permissions.Suggested patch
if self.action in [ "list_of_shared_users", "adapter_info", - "share", "effective_members", ]: return [IsOwnerOrSharedUserOrSharedToOrg()] + if self.action == "share": + return [IsOwner()]🤖 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/adapter_processor_v2/views.py` around lines 151 - 157, The current permission branch bundles "share" with IsOwnerOrSharedUserOrSharedToOrg allowing shared users to mutate sharing; remove "share" from that list and add a separate conditional branch for when self.action == "share" that returns an owner/admin-only permission (e.g., IsOwnerOrAdmin()) instead; update the permission logic in the same method handling self.action so existing actions ("list_of_shared_users", "adapter_info", "effective_members") keep IsOwnerOrSharedUserOrSharedToOrg() while "share" explicitly requires IsOwnerOrAdmin().backend/tenant_account_v2/sharing_helpers.py (2)
330-337:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep org-scope validation on the service-account path.
This early return skips
_check_users_axis()and_check_groups_axis(), so a service account can writeshared_usersorshared_groupsfrom another organization straight into the resource. Bypassing authorization here may be intentional; bypassing the org-boundary validation is not.Suggested fix
if getattr(actor, "is_service_account", False): + cls._authorize( + actor, + resource, + desired, + is_owner=True, + is_admin=True, + ) cls._commit(resource, desired) return🤖 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/sharing_helpers.py` around lines 330 - 337, The early-return for service accounts skips org-boundary validations; ensure the users/groups axis checks still run for service accounts by invoking cls._check_users_axis(resource, desired, actor) and cls._check_groups_axis(resource, desired, actor) before returning on getattr(actor, "is_service_account", False). In practice, move or add those calls so service accounts still undergo org-scope validation, then proceed to cls._commit(resource, desired) (and only skip cls._authorize for service accounts if intended).
267-286:⚠️ Potential issue | 🟠 MajorFix missing org scoping in
is_org_admin()
is_org_admin()callsAuthenticationController.get_organization_members_by_user(user=...), which returnsOrganizationMemberService.get_user_by_id(id=user.id).OrganizationMemberService.get_user_by_id()doesOrganizationMember.objects.get(user=id)without filtering by the current organization (noUserContext/organization_idis used), even thoughOrganizationMemberis unique per("organization", "user").- For users with memberships in multiple orgs, this makes the admin-role check ambiguous and can incorrectly evaluate as non-admin. Scope the lookup by current org (e.g., via
UserContext) or pass an explicitorganization_id.🤖 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/sharing_helpers.py` around lines 267 - 286, is_org_admin currently calls AuthenticationController.get_organization_members_by_user(user=...) which ultimately uses OrganizationMemberService.get_user_by_id(user.id) without scoping to the current organization; for users in multiple orgs this yields ambiguous/non-admin results. Update is_org_admin to resolve the current organization (e.g., read organization_id from UserContext or accept/pass an explicit organization_id) and call the auth controller with that scope (e.g., get_organization_members_by_user(user=..., organization_id=...)) or use a method that filters OrganizationMember by both organization and user; keep the lazy import of AuthenticationController and preserve the existing try/except behavior and return False on lookup failures.backend/tenant_account_v2/group_views.py (1)
150-158:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
user_idbefore deleting the membership.
list()already normalizesmemberto an integer, but this path accepts any token from the URL and forwards it straight to Line 156.DELETE /members/foo/should fail with a deterministic 400 instead of reaching the delete query unvalidated.Suggested fix
def remove_member( self, request: Request, pk: str | None = None, user_id: str | None = None ) -> Response: if not _is_org_admin(request): raise PermissionDenied(IsOrgAdminForWrite.message) + try: + member_user_id = int(user_id) if user_id is not None else None + except (TypeError, ValueError) as exc: + raise ValidationError({"user_id": "Must be a numeric user ID."}) from exc group = self._get_group_or_404(pk) - deleted, _ = group.memberships.filter(user_id=user_id).delete() + deleted, _ = group.memberships.filter(user_id=member_user_id).delete() if not deleted: raise NotFound("User is not a member of this group.") return Response(status=status.HTTP_204_NO_CONTENT)🤖 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/group_views.py` around lines 150 - 158, In remove_member, validate the user_id URL token before calling group.memberships.filter(...). Convert/normalize user_id to the expected integer type (e.g., via int(...) or using the same normalization as list()) and catch conversion errors; if conversion fails, raise a 400 Bad Request (or DRF ParseError/ValidationError) with a clear message like "Invalid user_id"; only proceed to the delete call when the user_id is valid to avoid passing arbitrary tokens to the query.
🤖 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 `@frontend/src/components/widgets/top-bar/TopBar.jsx`:
- Around line 13-14: TopBar currently always renders the hard-coded placeholder
"Search Users" (even when searchKey ≠ "email"); add a new prop searchPlaceholder
to the TopBar component signature with a sensible default (e.g., "Search Users")
and use that prop where the input placeholder is rendered so callers can pass a
custom placeholder for groups or other flows; update the prop destructuring
(include searchPlaceholder) and the JSX input placeholder reference to use
searchPlaceholder instead of the static string.
---
Outside diff comments:
In `@backend/adapter_processor_v2/views.py`:
- Around line 151-157: The current permission branch bundles "share" with
IsOwnerOrSharedUserOrSharedToOrg allowing shared users to mutate sharing; remove
"share" from that list and add a separate conditional branch for when
self.action == "share" that returns an owner/admin-only permission (e.g.,
IsOwnerOrAdmin()) instead; update the permission logic in the same method
handling self.action so existing actions ("list_of_shared_users",
"adapter_info", "effective_members") keep IsOwnerOrSharedUserOrSharedToOrg()
while "share" explicitly requires IsOwnerOrAdmin().
In `@backend/tenant_account_v2/group_views.py`:
- Around line 150-158: In remove_member, validate the user_id URL token before
calling group.memberships.filter(...). Convert/normalize user_id to the expected
integer type (e.g., via int(...) or using the same normalization as list()) and
catch conversion errors; if conversion fails, raise a 400 Bad Request (or DRF
ParseError/ValidationError) with a clear message like "Invalid user_id"; only
proceed to the delete call when the user_id is valid to avoid passing arbitrary
tokens to the query.
In `@backend/tenant_account_v2/sharing_helpers.py`:
- Around line 330-337: The early-return for service accounts skips org-boundary
validations; ensure the users/groups axis checks still run for service accounts
by invoking cls._check_users_axis(resource, desired, actor) and
cls._check_groups_axis(resource, desired, actor) before returning on
getattr(actor, "is_service_account", False). In practice, move or add those
calls so service accounts still undergo org-scope validation, then proceed to
cls._commit(resource, desired) (and only skip cls._authorize for service
accounts if intended).
- Around line 267-286: is_org_admin currently calls
AuthenticationController.get_organization_members_by_user(user=...) which
ultimately uses OrganizationMemberService.get_user_by_id(user.id) without
scoping to the current organization; for users in multiple orgs this yields
ambiguous/non-admin results. Update is_org_admin to resolve the current
organization (e.g., read organization_id from UserContext or accept/pass an
explicit organization_id) and call the auth controller with that scope (e.g.,
get_organization_members_by_user(user=..., organization_id=...)) or use a method
that filters OrganizationMember by both organization and user; keep the lazy
import of AuthenticationController and preserve the existing try/except behavior
and return False on lookup failures.
🪄 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: 76c5a7de-7df4-4781-ad2b-8216a73c37ae
📒 Files selected for processing (10)
backend/adapter_processor_v2/views.pybackend/tenant_account_v2/group_views.pybackend/tenant_account_v2/share_serializer_mixin.pybackend/tenant_account_v2/sharing_helpers.pyfrontend/src/components/groups/GroupCreateEditModal.jsxfrontend/src/components/groups/Groups.jsxfrontend/src/components/groups/groups-service.jsfrontend/src/components/navigations/side-nav-bar/SideNavBar.jsxfrontend/src/components/tool-settings/tool-settings/ToolSettings.jsxfrontend/src/components/widgets/top-bar/TopBar.jsx
…aceholder - group_views.remove_member: guard non-numeric user_id (400 not 500), mirroring the list action - TopBar: searchPlaceholder prop (default "Search Users"); Groups passes "Search Groups" Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The /groups/{pk}/resources/ action is the delete blast-radius view used
only by the admin delete-confirm flow, but IsOrgAdminForWrite permits GET
for any authenticated org member. That let non-members enumerate the
names/UUIDs of resources shared with groups they are not in, contradicting
the model where org admin has no implicit resource access. Gate the action
to org admins, mirroring the existing members POST / remove_member guards.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Group sharing is manual-only after the 2026-05-25 reversal; there is no IdP group sync. Remove the dead IDP_GROUP_SYNC_INTERVAL_MIN var and the "LOCAL + IDP combined" wording so sample.env stops documenting a feature that no longer exists. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
kirtimanmishrazipstack
left a comment
kirtimanmishrazipstack
left a comment
There was a problem hiding this comment.
Comprehensive review — UN-2977 group-based sharing
Reviewed alongside the cloud counterpart (#1508) since they share the sharing abstractions. Overall this is a careful, well-structured change — multi-tenant isolation on the suspect paths (the varchar→UUID cast, has_group_access, the org-scoped managers) holds up, and the share-authorization matrix is coherent. Findings below are real and currently present in the code.
🔴 Needs a decision before merge
1. Org-admin implicit access: the code does the opposite of what this PR's description says.
The description states (three times): "being an org admin grants no implicit access… Org admin implicit access is explicitly denied — Enforced in for_user() and the permission classes — an admin who is not owner/shared cannot view or run a resource."
The actual code grants admins full access on both axes:
for_user()in all 6 managers short-circuits:if OrganizationMemberService.is_user_organization_admin(user): return self.all()—pipeline_v2/models.py:35,api_v2/models.py:42,workflow_manager/workflow_v2/models/workflow.py:33,connector_v2/models.py:34,adapter_processor_v2/models.py:41,prompt_studio/prompt_studio_core_v2/models.py:30.- Every permission class ORs
_is_organization_admin(request)—permissions/permission.py:79and:113— andIsOwner's docstring affirms "Org admins can manage every resource in their organization."
One of the two is wrong. Either the code is correct and the description (incl. the "Can this PR break" section) is materially stale, or the description reflects the intended invariant and there's a least-privilege/multi-tenancy gap across all 6 resources + the permission layer. Please reconcile against the design source of truth (mfbt UNS-612) and align the description and/or the code. This also drives the cloud parity fix in #1508.
🟠 Important
2. member_count is always 1 when the group list is filtered by member.
tenant_account_v2/group_serializers.py:175-180 applies .filter(memberships__user=user) before .annotate(Count("memberships")). Filtering and aggregating on the same multi-valued relation makes Count run over the constrained join → member_count = 1 for every group in both ?member=me (non-admin "my groups") and ?member=<id> (admin filter). The unfiltered list path is correct. Fix: compute the count via a decoupled Subquery so the member filter can't taint it.
3. SharedGroupsSerializerMixin is dead code, and its docstring advertises a writable API.
shared_groups is read_only=True on all 7 resource serializers, so DRF strips it from validated_data and the mixin's create/update bodies (tenant_account_v2/share_serializer_mixin.py:34-50) always no-op — every group write actually goes through ShareAuthorizationService._commit. The module docstring (:12-19) even shows a writable PrimaryKeyRelatedField(queryset=…) example that contradicts every call site. Delete the mixin (and drop it from the serializers), or make it actually fire. The dead path also skips the org-scope check, so it's a latent footgun if reactivated.
4. No orphan cleanup of ResourceGroupShare when a resource is deleted.
object_id is a plain varchar (no FK/GFK), and tenant_account_v2/signals.py only purges on OrganizationMember removal. Deleting a Workflow/Pipeline/Connector/etc. leaves dangling share rows indefinitely. Add a post_delete receiver for the shareable models mirroring the existing lazy-model loop in signals.py (it already no-ops cleanly for apps that aren't installed, so it can include the cloud AgenticProject).
5. The post_delete purge signal isn't atomic and has no per-step error handling.
signals.py:cleanup_user_org_access bulk-deletes memberships then loops resource.shared_users.remove(...) across the shareable models with no transaction.atomic() and no try/except. A mid-loop failure leaves the user purged from some resources but not others — re-opening the "rejoin backdoor" the docstring claims to close — with no log of the partial purge. Wrap the cleanup in atomic() and log-and-reraise per model.
6. Share modal closes on failure inconsistently across resources.
frontend/src/hooks/useShareModal.js:119-124, ListOfTools.jsx, and Workflows.jsx close the modal in .finally() (success or failure), discarding the user's selection on a 400/403 with only a transient toast; ToolSettings.jsx correctly closes only on success. For partial-authorization rejections the user needs to see what was rejected. Close on .then only, keep open on .catch, and make all resources consistent.
🟡 Suggestions
- Defensive UUID cast (latent):
tenant_account_v2/sharing_helpers.py:137and:160douuid.UUID(s)with only an empty-string guard. The write path stores valid PKs today, but since the column is deliberately varchar, one malformedobject_idwould 500 the entire resource list for every member of that group. Skip-and-log malformed IDs instead. - Stringly-typed share axes (project anti-pattern): the same axis vocabulary is declared three times as bare strings (
sharing_helpers.py:306-308,permissions/resource_share_views.py:19, mixinshare_axes);group_views.py:204-209and theaccess_vialiterals incompute_effective_members(sharing_helpers.py:192,211,242) duplicate constants that exist elsewhere. AShareAxisenum +DesiredShareState/AccessViawould collapse these and double as thecontent_typeallowlist the model currently lacks. (AxisDiffalready exists, butShareAuthorizationServicere-derives tuples instead of consuming it.) - Unbounded queryset:
compute_effective_members._add_org_membersiterates every org member with no pagination on a synchronous GET (effective-members/). Cap/paginate or.iterator(). - Adapter default-cleanup runs outside the share transaction (
adapter_processor_v2/views.py:264-282) — best-effort cleanup with no try/except after an already-committed share. Fold into the atomic block, or log on failure. - Comment accuracy: the
resource_share_views.py"axis-agnostic… any number of M2M axes" claim is inaccurate (shared_groupsis not M2M); the duplicated- Service accounts see all org resourcesbullet added above the existing combined line in all 6for_userdocstrings is redundant; thecleanup_user_org_access"closes the rejoin backdoor" wording overstates the (currently non-atomic) guarantee.
✅ Well-handled
Polymorphic single-table design avoids per-resource migration sprawl; migration dependencies + reversibility are correct; quota checks return structured 400s; lazy % logging is consistent; org-scope is re-validated defense-in-depth inside ShareAuthorizationService; the merge-order coupling with #1508 is correctly documented.
🤖 Claude Code–assisted multi-agent review (code quality, silent-failure, type-design, comment accuracy); high-impact findings verified against the code.
…uards Addresses self-review findings on PR #1986: - member_count: count via a decoupled Subquery so the optional ?member filter on the same relation no longer collapses it to 1. - Add a post_delete receiver purging ResourceGroupShare rows when a shareable resource is deleted (object_id is varchar, no FK/cascade). - Wrap cleanup_user_org_access in transaction.atomic() with per-model error handling that logs and re-raises, so a partial purge can't re-open the rejoin backdoor. - Skip-and-log malformed object_id UUIDs instead of 500ing the list. - Error-handle the post-commit adapter default-cleanup save. - Comment accuracy: shared_groups is polymorphic (not M2M); drop the duplicate service-account docstring bullet; clarify the mixin's writable vs read-only usage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This PR had moved the share-modal close into .finally() (close on success or failure), discarding the user's selection on a 400/403. Revert useShareModal, ListOfTools and Workflows to close only on success — restoring pre-PR behavior and matching ToolSettings — so a rejected share keeps the modal open for the user to review and retry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Thanks for the thorough review — worked through every item. Each was validated against the code and Fixed & pushed (
Addressed by reply (no code change):
|
All 7 resource serializers (6 OSS + cloud AgenticProject) declare shared_groups as read_only, so DRF strips it from validated_data and the mixin's create/update never wrote group shares — every group write flows through ShareAuthorizationService._commit via the POST /share/ action. The mixin was therefore dead code with a misleading docstring and a latent org-scope footgun if a field were ever made writable without re-adding validation. Delete it and unwire it from the 6 serializers; the read_only shared_groups declarations stay. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Correction to my earlier reply on #3: I said the cloud So I deleted it rather than keeping it — commit |
…y-s-Introduce-Group-Based-Project-Sharing # Conflicts: # backend/prompt_studio/permission.py
Contributor
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
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
1 participant
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
Adds org-scoped groups as a first-class sharing target alongside the existing per-user share and org-wide share, across all 7 shareable resources (Workflow, Pipeline, API Deployment, Connector, Adapter, Prompt Studio project, and Agentic project). An org admin creates a group, manages its members, and shares resources with the group in one action; access tracks membership automatically.
Why
Admins needed to grant a set of users access to a resource in a single action and have that access follow membership changes, instead of re-sharing to each user individually. Groups are manual-only — login method (SSO or email/password) is authentication only and never drives membership.
How
tenant_account_v2):OrganizationGroup,GroupMembership(explicit through-model), andResourceGroupShare— a single polymorphicgroup → resourcetable that covers every resource type, so no per-resourceshared_groupscolumn/migration is needed.shared_groupsis exposed as a read property on each model backed by this table.for_user(user)unionscreated_by | shared_users | shared_to_org | group-shared; group ids are read live fromGroupMembership, so membership changes take effect immediately.resources_visible_via_groupscasts the varcharobject_idto the model's UUID PK to avoid a Postgres type mismatch.POST /<resource>/{id}/share/action (theshared_users/shared_groups/shared_to_orgfields areread_onlyon the resource serializers);ShareAuthorizationServicediffs requested vs. current state per axis and authorizes each change (owners and admins on every axis). The groupresources/endpoint is admin-only.ResourceShareManagementMixinprovidesshare/+effective-members/on every shareable viewset; an admin-gatedOrganizationGroupViewSetprovides group CRUD + member management.post_deletesignal onOrganizationMemberpurges the removed user's group memberships and directshared_usersrows (closes the rejoin backdoor);created_byis untouched.POST /share/).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)
read_onlyon resource serializers; any caller that previously PATCHedshared_users/shared_to_orgdirectly must use the newPOST /<resource>/{id}/share/action. Internal callers and the UI are migrated; external API clients doing direct PATCH-share would silently no-op.post_deletesignal removes the user from every resource'sshared_users(in addition to group memberships). Intentional, but a behavior change vs. before.for_user()and the permission classes admit admins; group sharing never restricts them — it only adds access for the group's non-admin members.Database Migrations
tenant_account_v2/0002_organization_group_group_membership.py—OrganizationGroup,GroupMembership.tenant_account_v2/0003_resource_group_share.py—ResourceGroupShare.shared_groupsis the polymorphic table;shared_users/shared_to_orgpre-existed).Env Config
MAX_GROUPS_PER_ORG(default200) — max groups per org.MAX_MEMBERS_PER_GROUP(default500) — max members per group.code.Relevant Docs
prompting/group-based-sharing/phase-1-resource-sharing/SPEC.mdUNS-612.Related Issues or PRs
Dependencies Versions
Notes on Testing
Manual verification per spec §7: create a group, add/remove members, share each resource type with the group, confirm group-only members gain view/run access and lose it on removal, and confirm the quota limits return HTTP 400.
Screenshots
Checklist
I have read and understood the Contribution Guidelines.