improvement(permissions): confine workspace role changes to existing members (#6180)

* improvement(permissions): confine workspace role changes to existing members

* improvement(permissions): retry lock timeouts and audit admin upserts truthfully

Bounding the row-lock wait with `lock_timeout` made a competing writer raise
55P03, which the retry set excluded — so the bounded wait failed the request
where the unbounded one would have waited. 55P03 is now retried like the other
contention aborts, the timeout is shortened since each attempt releases its
pooled connection, and contention that outlives every attempt answers with a
busy conflict instead of a generic server error.

The admin member upsert recorded MEMBER_ADDED even when the conflict branch
amended an existing membership, putting a join that never happened in the
workspace audit trail. It now records a role change, matching the action the
response already reported.

Adds the missing test coverage for withTransactionRetry.

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: icecrasher321

apps/sim/app/api/organizations/[id]/roster/route.test.ts

@@ -174,6 +174,8 @@ describe('GET /api/organizations/[id]/roster', () => {
               workspaceId: 'workspace-1',
               workspaceName: 'Workspace One',
               permission: 'admin',
+              roleSource: 'org-admin',
+              isBilledAccount: false,
             },
           ],
         }),
@@ -193,6 +195,8 @@ describe('GET /api/organizations/[id]/roster', () => {
             workspaceId: 'workspace-1',
             workspaceName: 'Workspace One',
             permission: 'read',
+            roleSource: 'explicit',
+            isBilledAccount: false,
           },
         ],
       }),

apps/sim/app/api/organizations/[id]/roster/route.ts

@@ -89,12 +89,17 @@ export const GET = withRouteHandler(
       await expireStalePendingInvitationsForOrganization(organizationId)
 
       const orgWorkspaces = await db
-        .select({ id: workspace.id, name: workspace.name })
+        .select({
+          id: workspace.id,
+          name: workspace.name,
+          ownerId: workspace.ownerId,
+          billedAccountUserId: workspace.billedAccountUserId,
+        })
         .from(workspace)
         .where(and(eq(workspace.organizationId, organizationId), isNull(workspace.archivedAt)))
 
       const orgWorkspaceIds = orgWorkspaces.map((ws) => ws.id)
-      const workspaceNameById = new Map(orgWorkspaces.map((ws) => [ws.id, ws.name]))
+      const workspaceById = new Map(orgWorkspaces.map((ws) => [ws.id, ws]))
       const memberUserIds = memberRows.map((row) => row.userId)
 
       const memberPermissions =
@@ -117,11 +122,14 @@ export const GET = withRouteHandler(
 
       const permissionsByUser = new Map<string, RosterWorkspaceAccess[]>()
       for (const row of memberPermissions) {
+        const ws = workspaceById.get(row.workspaceId)
         const list = permissionsByUser.get(row.userId) ?? []
         list.push({
           workspaceId: row.workspaceId,
-          workspaceName: workspaceNameById.get(row.workspaceId) ?? 'Workspace',
+          workspaceName: ws?.name ?? 'Workspace',
           permission: row.permission,
+          roleSource: ws?.ownerId === row.userId ? 'owner' : 'explicit',
+          isBilledAccount: ws?.billedAccountUserId === row.userId,
         })
         permissionsByUser.set(row.userId, list)
       }
@@ -135,6 +143,14 @@ export const GET = withRouteHandler(
                 workspaceId: ws.id,
                 workspaceName: ws.name,
                 permission: 'admin' as const,
+                /**
+                 * Owner wins over the derived organization grant, matching
+                 * `getUsersWithPermissions` — otherwise the same person reads as
+                 * `owner` in the teammates list and `org-admin` here.
+                 */
+                roleSource:
+                  ws.ownerId === rosterMember.userId ? ('owner' as const) : ('org-admin' as const),
+                isBilledAccount: ws.billedAccountUserId === rosterMember.userId,
               }))
             : (permissionsByUser.get(rosterMember.userId) ?? []),
         }
@@ -183,10 +199,13 @@ export const GET = withRouteHandler(
 
       for (const row of externalPermissionRows) {
         const existing = externalMembersByUser.get(row.userId)
+        const externalWorkspace = workspaceById.get(row.workspaceId)
         const workspaceAccess: RosterWorkspaceAccess = {
           workspaceId: row.workspaceId,
-          workspaceName: workspaceNameById.get(row.workspaceId) ?? 'Workspace',
+          workspaceName: externalWorkspace?.name ?? 'Workspace',
           permission: row.permission,
+          roleSource: externalWorkspace?.ownerId === row.userId ? 'owner' : 'explicit',
+          isBilledAccount: externalWorkspace?.billedAccountUserId === row.userId,
         }
 
         if (existing) {
@@ -247,8 +266,11 @@ export const GET = withRouteHandler(
         const list = grantsByInvitation.get(row.invitationId) ?? []
         list.push({
           workspaceId: row.workspaceId,
-          workspaceName: workspaceNameById.get(row.workspaceId) ?? 'Workspace',
+          workspaceName: workspaceById.get(row.workspaceId)?.name ?? 'Workspace',
           permission: row.permission,
+          /** A pending invitee holds no row yet, so nothing is inherited. */
+          roleSource: 'explicit',
+          isBilledAccount: false,
         })
         grantsByInvitation.set(row.invitationId, list)
       }
@@ -269,7 +291,7 @@ export const GET = withRouteHandler(
       const data = {
         members: rosterMembers,
         pendingInvitations,
-        workspaces: orgWorkspaces,
+        workspaces: orgWorkspaces.map((ws) => ({ id: ws.id, name: ws.name })),
       } satisfies OrganizationRoster
       return NextResponse.json({
         success: true,

apps/sim/app/api/v1/admin/workspaces/[id]/members/[memberId]/route.ts

@@ -44,6 +44,7 @@ import {
 import { withAdminAuthParams } from '@/app/api/v1/admin/middleware'
 import {
   badRequestResponse,
+  conflictResponse,
   internalErrorResponse,
   notFoundResponse,
   singleResponse,
@@ -170,10 +171,20 @@ export const PATCH = withRouteHandler(
 
       const now = new Date()
 
-      await db
+      /**
+       * Conditional on the row read above still existing: a concurrent removal
+       * between that read and this write would otherwise match nothing and be
+       * reported to the caller as a successful update.
+       */
+      const updated = await db
         .update(permissions)
         .set({ permissionType: permissionLevel, updatedAt: now })
         .where(eq(permissions.id, memberId))
+        .returning({ id: permissions.id })
+
+      if (updated.length === 0) {
+        return conflictResponse('Workspace member changed during the update. Retry.')
+      }
 
       const [userData] = await db
         .select({ name: user.name, email: user.email, image: user.image })

apps/sim/app/api/v1/admin/workspaces/[id]/members/route.ts

@@ -9,6 +9,12 @@
  *
  * Response: AdminListResponse<AdminWorkspaceMember>
  *
+ * `createdAt` is the member's join time. It previously moved on every role
+ * change, because the in-app role-change endpoint replaced the permission row
+ * rather than amending it; that endpoint now updates in place, so only
+ * `updatedAt` tracks role changes. Consumers that diffed `createdAt` to detect
+ * recently-changed members must read `updatedAt` instead.
+ *
  * POST /api/v1/admin/workspaces/[id]/members
  *
  * Add a user to a workspace with a specific permission level.
@@ -55,6 +61,7 @@ import {
 import { withAdminAuthParams } from '@/app/api/v1/admin/middleware'
 import {
   badRequestResponse,
+  conflictResponse,
   internalErrorResponse,
   listResponse,
   notFoundResponse,
@@ -191,10 +198,20 @@ export const POST = withRouteHandler(
       if (existingPermission) {
         if (existingPermission.permissionType !== permissionLevel) {
           const now = new Date()
-          await db
+          /**
+           * Conditional on the row read above still existing: a concurrent
+           * removal between that read and this write would otherwise match
+           * nothing and be reported to the caller as a successful update.
+           */
+          const updated = await db
             .update(permissions)
             .set({ permissionType: permissionLevel, updatedAt: now })
             .where(eq(permissions.id, existingPermission.id))
+            .returning({ id: permissions.id })
+
+          if (updated.length === 0) {
+            return conflictResponse('Workspace member changed during the update. Retry.')
+          }
 
           logger.info(`Admin API: Updated user ${userId} permissions in workspace ${workspaceId}`, {
             previousPermissions: existingPermission.permissionType,
@@ -247,28 +264,53 @@ export const POST = withRouteHandler(
       const now = new Date()
       const permissionId = generateId()
 
-      await db.insert(permissions).values({
-        id: permissionId,
-        userId,
-        entityType: 'workspace',
-        entityId: workspaceId,
-        permissionType: permissionLevel,
-        createdAt: now,
-        updatedAt: now,
-      })
-
-      logger.info(`Admin API: Added user ${userId} to workspace ${workspaceId}`, {
-        permissions: permissionLevel,
-        permissionId,
-      })
-
+      /**
+       * The existence read above is unlocked, so two concurrent adds for the
+       * same user both reach here. Conflicting on the uniqueness constraint
+       * settles it as the requested role instead of failing the loser with a
+       * 500 for a request that did what it asked.
+       */
+      const [written] = await db
+        .insert(permissions)
+        .values({
+          id: permissionId,
+          userId,
+          entityType: 'workspace',
+          entityId: workspaceId,
+          permissionType: permissionLevel,
+          createdAt: now,
+          updatedAt: now,
+        })
+        .onConflictDoUpdate({
+          target: [permissions.userId, permissions.entityType, permissions.entityId],
+          set: { permissionType: permissionLevel, updatedAt: now },
+        })
+        .returning({ id: permissions.id, createdAt: permissions.createdAt })
+
+      /** A returned id we did not mint means the conflict branch ran. */
+      const wasCreated = written?.id === permissionId
+
+      logger.info(
+        wasCreated
+          ? `Admin API: Added user ${userId} to workspace ${workspaceId}`
+          : `Admin API: Updated user ${userId} permissions in workspace ${workspaceId}`,
+        { permissions: permissionLevel, permissionId }
+      )
+
+      /**
+       * The conflict branch amended a membership that already existed, so it is
+       * a role change rather than an addition — recording it as `MEMBER_ADDED`
+       * would put a join that never happened in the workspace's audit trail.
+       */
       recordAudit({
         workspaceId,
         actorId: 'admin-api',
-        action: AuditAction.MEMBER_ADDED,
+        action: wasCreated ? AuditAction.MEMBER_ADDED : AuditAction.MEMBER_ROLE_CHANGED,
         resourceType: AuditResourceType.WORKSPACE,
         resourceId: workspaceId,
-        description: `Admin API added member to workspace with ${permissionLevel} permissions`,
+        description: wasCreated
+          ? `Admin API added member to workspace with ${permissionLevel} permissions`
+          : `Admin API changed workspace member permissions to ${permissionLevel}`,
         metadata: { targetUserId: userId, permissions: permissionLevel },
         request,
       })
@@ -288,16 +330,16 @@ export const POST = withRouteHandler(
       }
 
       return singleResponse({
-        id: permissionId,
+        id: written?.id ?? permissionId,
         workspaceId,
         userId,
         permissions: permissionLevel,
-        createdAt: now.toISOString(),
+        createdAt: (written?.createdAt ?? now).toISOString(),
         updatedAt: now.toISOString(),
         userName: userData.name,
         userEmail: userData.email,
         userImage: userData.image,
-        action: 'created' as const,
+        action: wasCreated ? ('created' as const) : ('updated' as const),
       })
     } catch (error) {
       logger.error('Admin API: Failed to add workspace member', { error, workspaceId })