improvement(perms): member removal reassignment policies#4906
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Org removal still moves departing-owned workspaces to the organization owner (with admin permissions via upsert). It additionally runs Workspace removal (user and admin) uses shared Admin workspace deletes are transactional: ownership transfer, workflow reassignment, permission/credential/permission-group cleanup, with Reviewed by Cursor Bugbot for commit 24640fb. Configure here. |
|
@greptile |
|
@greptile |
Greptile SummaryThis PR introduces reassignment policies for member removal at both the workspace and organization level: departing-owned workspaces now transfer
Confidence Score: 3/5Safe to merge for workspaces where departing users are not the billing account; the blocking scenario should be resolved before production rollout. The core transaction logic is well-structured and the onConflictDoUpdate fix is a genuine improvement. The concern is that workspaces where billedAccountUserId equals departingUserId are added to unresolved before any workflow count is checked, permanently blocking removal of a user who is billing account for a workspace with zero of their own workflows. apps/sim/lib/workspaces/utils.ts (unresolved-check logic) and apps/sim/lib/workspaces/utils.test.ts (missing coverage for transferWorkspaceOwnershipToBilledAccountForMemberRemovalTx) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Member Removal Request] --> B{Is billing account?}
B -- Yes --> C[Return 400 early guard]
B -- No --> D[Open DB Transaction]
D --> E[transferWorkspaceOwnershipToBilledAccountForMemberRemovalTx]
E --> F{Is departing user workspace owner?}
F -- No --> G[Return false no-op]
F -- Yes --> H{billedAccountUserId === departingUserId?}
H -- Yes --> I[Throw WorkspaceBillingAccountRemovalError]
H -- No --> J[Update workspace.ownerId]
J --> K[Upsert admin permission for new owner]
G --> L[reassignWorkflowOwnershipForWorkspaceMemberRemovalTx]
K --> L
L --> M{billedAccountUserId === departingUserId?}
M -- Yes --> N[Add to unresolved - no workflow count check]
M -- No --> O[Add to reassignmentWorkspaceIds]
O --> P[Bulk UPDATE workflow.userId]
N --> Q{unresolved.length > 0?}
P --> Q
Q -- Yes --> I
Q -- No --> R[Delete permissions and memberships]
R --> S[Commit Transaction]
I --> T[Rollback - 400 response]
Reviews (1): Last reviewed commit: "improve UI notif" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR adds reassignment policies for both workspace ownership and workflow ownership when org or workspace members are removed. Two new transaction-scoped utilities centralize the logic, and all member-removal routes are updated to use them atomically, with billing-account guards preventing removal of the workspace's billing identity.
Confidence Score: 3/5Safe to review further before merging — one function in the new utils layer will write a null workspace owner to the DB when billedAccountUserId is unset, relying on a later sibling call in the same transaction to roll things back rather than being self-contained. transferWorkspaceOwnershipToBilledAccountForMemberRemovalTx skips a null check for billedAccountUserId. In that case it issues UPDATE workspace SET ownerId = NULL and subsequently tries to upsert a permission row with userId = NULL, both inside the same transaction. The transaction does roll back because the following reassignWorkflowOwnershipForWorkspaceMemberRemovalTx call flags the workspace as unresolved and throws, but that safety net is entirely implicit and depends on call order staying the same. apps/sim/lib/workspaces/utils.ts (the null-guard gap in transferWorkspaceOwnershipToBilledAccountForMemberRemovalTx) and apps/sim/lib/billing/organizations/membership.ts (two silent catch blocks for billing-account errors). Important Files Changed
Sequence DiagramsequenceDiagram
participant Route as API Route
participant Guard as Pre-check
participant TX as DB Transaction
participant OwnerTx as transferWorkspaceOwnershipTx
participant WorkflowTx as reassignWorkflowOwnershipTx
participant DB as Database
Route->>Guard: Is departing user the billing account?
alt "billedAccountUserId === userId"
Guard-->>Route: 400 Bad Request
else
Guard-->>Route: OK proceed
Route->>TX: db.transaction()
TX->>OwnerTx: transfer workspace ownerId
OwnerTx->>DB: SELECT workspace
alt departingUser is owner
OwnerTx->>DB: UPDATE workspace ownerId
OwnerTx->>DB: UPSERT permissions admin
OwnerTx-->>TX: true
else
OwnerTx-->>TX: false
end
TX->>WorkflowTx: reassign workflow.userId
WorkflowTx->>DB: SELECT billedAccountUserId
alt null or equals departingUser
WorkflowTx-->>TX: unresolved
TX-->>Route: rollback 400
else
WorkflowTx->>DB: UPDATE workflow userId
WorkflowTx-->>TX: reassigned
end
TX->>DB: DELETE permissions
TX->>DB: revoke credentials
TX->>DB: DELETE permissionGroupMember
TX-->>Route: success
Route-->>Route: 200 OK
end
Reviews (2): Last reviewed commit: "improve UI notif" | Re-trigger Greptile |
Greptile SummaryThis PR implements member-removal reassignment policies across org-level and workspace-level removal paths. When a member departs, their owned workspaces transfer to the org owner (org removal) or the workspace billing account (direct removal), and their workflows are re-attributed to the workspace billed account so provider config and env-var resolution remain valid.
Confidence Score: 3/5The core logic is sound, but a missing null guard in the ownership-transfer utility can cause an unexpected DB error whenever a workspace has no billing account and its owner is being removed. The transferWorkspaceOwnershipToBilledAccountForMemberRemovalTx function skips the null case for billedAccountUserId, falling through to set ownerId = null. This produces either a DB constraint error (500) or temporary data corruption before rollback, rather than the intended clean 400. All other changes — the workflow subquery reassignment, transaction wrapping across routes, the admin PATCH permission guard, and the UI toast — are correct. apps/sim/lib/workspaces/utils.ts — the null guard on billedAccountUserId inside transferWorkspaceOwnershipToBilledAccountForMemberRemovalTx should be added before merging. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Route as API Route
participant TX as DB Transaction
participant OwnerUtil as transferWorkspaceOwnership...
participant WorkflowUtil as reassignWorkflowOwnership...
Client->>Route: DELETE /workspaces/members/[id]
Route->>Route: "Pre-check billedAccount === userId"
Route->>TX: db.transaction()
TX->>OwnerUtil: transferWorkspaceOwnershipToBilledAccountForMemberRemovalTx
alt departing user is workspace owner
OwnerUtil->>TX: "UPDATE workspace SET ownerId = billedAccountUserId"
OwnerUtil->>TX: UPSERT permissions admin for new owner
end
TX->>WorkflowUtil: reassignWorkflowOwnershipForWorkspaceMemberRemovalTx
WorkflowUtil->>TX: SELECT workspace.billedAccountUserId
alt billedAccountUserId null or equals departingUserId
WorkflowUtil-->>TX: unresolved workspace
TX-->>Route: throw WorkspaceBillingAccountRemovalError
Route-->>Client: 400 Bad Request
else valid billed account
WorkflowUtil->>TX: UPDATE workflow SET userId via correlated subquery
end
TX->>TX: DELETE permissions for departing user
TX->>TX: revokeWorkspaceCredentialMembershipsTx
TX->>TX: DELETE permissionGroupMember
TX-->>Route: ownershipTransferred and workflowOwnershipReassignment
Route-->>Client: 200 success
Reviews (3): Last reviewed commit: "improve UI notif" | Re-trigger Greptile |
1 participant
Summary
Org-level member removal: departing-owned workspaces transfer to the organization owner, and the org owner is granted/admin-promoted for those workspaces.
Workflow ownership: departing-owned workflows still transfer to the workspace billing account, so provider config/env resolution has a valid workspace identity.
Direct workspace member removal: workspace owner transfer remains to the workspace billing account, matching the existing workspace-level rule we were applying there.
Type of Change
Testing
N/A
Checklist