fix(credentials): grant admin credential role to workspace admins#4736
fix(credentials): grant admin credential role to workspace admins#4736serhiizghama wants to merge 2 commits into
Conversation
Add getWorkspaceAdminUserIds helper that queries the permissions table for users with permissionType='admin' on the workspace (plus the owner). Update ensureWorkspaceCredentialMemberships, syncWorkspaceEnvCredentials, and createWorkspaceEnvCredentials to use adminUserIds when determining the credential_member role, so workspace admins receive 'admin' instead of 'member' and can edit or delete shared workspace secrets.
|
@serhiizghama is attempting to deploy a commit to the Sim Team on Vercel. A member of the Team first needs to authorize it. |
PR SummaryMedium Risk Overview Adds Existing rows are not backfilled by this change; only new creates and env sync paths that run membership ensure get the corrected roles. Reviewed by Cursor Bugbot for commit 7819ed3. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryThis PR fixes workspace admin users receiving a
Confidence Score: 4/5Safe to merge; the fix correctly identifies all affected insertion paths and the admin-set construction is sound. The only concerns are extra DB round-trips and a now-misleading parameter name in the internal helper. The core logic is correct across all four code paths. Both syncWorkspaceEnvCredentials and createWorkspaceEnvCredentials now issue three workspace-table queries and two permissions queries per call where one of each would suffice — that's a present inefficiency introduced by this PR, not a future risk. No data-correctness bugs or auth regressions were found. apps/sim/lib/credentials/environment.ts — redundant DB queries in the two bulk-credential functions and the misleading ownerUserId parameter name in the private helper. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant POST as POST /api/credentials
participant ENV as environment.ts helpers
participant DB as Database
Client->>POST: "POST {type: env_workspace/service_account}"
POST->>DB: BEGIN TRANSACTION
POST->>DB: INSERT credential
POST->>ENV: getWorkspaceMemberUserIds(workspaceId)
ENV->>DB: SELECT workspace (ownerId)
ENV->>DB: SELECT permissions (all members)
ENV-->>POST: memberUserIds[]
POST->>ENV: getWorkspaceAdminUserIds(workspaceId)
ENV->>DB: SELECT workspace (ownerId) ← duplicate query
ENV->>DB: "SELECT permissions WHERE permissionType='admin'"
ENV-->>POST: adminIds (Set)
loop each memberUserId
POST->>DB: "INSERT credentialMember (role = adminIds.has(uid) OR uid==creator ? admin : member)"
end
POST->>DB: COMMIT
POST-->>Client: 200 OK
Reviews (1): Last reviewed commit: "fix(credentials): apply admin role check..." | Re-trigger Greptile |
| const [[workspaceRow], memberUserIds, adminUserIds] = await Promise.all([ | ||
| db | ||
| .select({ ownerId: workspace.ownerId }) | ||
| .from(workspace) | ||
| .where(eq(workspace.id, workspaceId)) | ||
| .limit(1), | ||
| getWorkspaceMemberUserIds(workspaceId), | ||
| getWorkspaceAdminUserIds(workspaceId), | ||
| ]) |
There was a problem hiding this comment.
Redundant workspace queries from helper functions
syncWorkspaceEnvCredentials (and createWorkspaceEnvCredentials at line 246) now issues three separate workspace-table SELECTs per call: one directly in the Promise.all, one inside getWorkspaceMemberUserIds, and one inside getWorkspaceAdminUserIds. The permissions table is also queried twice (once for all members, once filtered to admin). A single SELECT permissions WHERE entityId = workspaceId joined with the workspace owner fetch would cover both sets, eliminating the extra round-trips.
| async function ensureWorkspaceCredentialMemberships( | ||
| credentialId: string, | ||
| memberUserIds: string[], | ||
| ownerUserId: string | ||
| ownerUserId: string, | ||
| adminUserIds: Set<string> | ||
| ) { |
There was a problem hiding this comment.
ownerUserId parameter now serves only as invitedBy
After this change, ownerUserId is no longer used for role determination inside ensureWorkspaceCredentialMemberships — adminUserIds handles that entirely. The parameter is still used for invitedBy, but its name no longer reflects that narrowed purpose. Callers reading the signature may incorrectly assume it still drives the admin-role check. Consider renaming it to invitedByUserId (or a similar name) to avoid confusion.
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!
Problem
Workspace admin users (users with
permissionType = 'admin'in the permissions table for a workspace) receive amemberrole oncredential_memberwhen a workspace-scoped secret is created. This prevents them from editing or deleting the secret, even though they have admin access to the workspace.Root cause: three code paths that create
credential_memberrows determined the role solely by checkingmemberUserId === ownerUserId, ignoring thepermissionstable:ensureWorkspaceCredentialMembershipsinlib/credentials/environment.tssyncWorkspaceEnvCredentials(calls the above)createWorkspaceEnvCredentials(inline bulk insert)app/api/credentials/route.tsSolution
Add a
getWorkspaceAdminUserIds(workspaceId)helper that returns aSet<string>containing:workspace.ownerId)permissionType = 'admin'on the workspace (permissionstable)Use this set instead of the bare owner ID check in all four affected paths so workspace admins receive
adminoncredential_member.Testing
Before this fix: admin user got
memberrole and saw a permission error.After this fix: admin user gets
adminrole and can manage the secret.