Skip to content

fix(credentials): reflect workspace permission in credential member role#4699

Open
minijeong-log wants to merge 5 commits into
simstudioai:stagingfrom
minijeong-log:fix/credential-member-role
Open

fix(credentials): reflect workspace permission in credential member role#4699
minijeong-log wants to merge 5 commits into
simstudioai:stagingfrom
minijeong-log:fix/credential-member-role

Conversation

@minijeong-log
Copy link
Copy Markdown
Contributor

Closes #4698

Summary

Workspace admin users were incorrectly assigned member role on credential_member when workspace-scoped secrets were created or synced. Only the workspace owner got admin. Now the workspace permissions table is consulted to determine the correct credential role.

Mapping

Workspace Permission Credential Role
owner (workspace.ownerId) admin
admin (permissions table) admin
write member
read member

Changes

  • environment.ts: Query workspace permissions in ensureWorkspaceCredentialMemberships and map admin permission → credential admin role
  • route.ts POST: Apply same mapping during credential creation

Test Plan

  • Create workspace with owner + admin + write + read members
  • Create workspace-scoped secret
  • Verify credential_member roles match the mapping above
  • Verify workspace admin can edit/delete secrets
  • Verify workspace write/read users cannot edit/delete secrets

Workspace admin users were incorrectly assigned 'member' role on
credential_member when workspace-scoped secrets were created or synced.
Only the workspace owner got 'admin'. Now workspace permissions table
is consulted: owner/admin → credential admin, write/read → member.

- environment.ts: query workspace permissions in ensureWorkspaceCredentialMemberships
- route.ts POST: apply same mapping during credential creation
@vercel
Copy link
Copy Markdown

vercel Bot commented May 21, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped May 21, 2026 11:14am

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented May 21, 2026

PR Summary

Medium Risk
Changes how credential_member.role is assigned for workspace-scoped credentials, which affects authorization for editing/deleting secrets; scope is limited to role mapping but can impact access control if incorrect.

Overview
Fixes workspace-scoped credential membership role assignment so workspace admins (from the permissions table) are created/synced as credential_member.role = admin, not just the workspace owner.

Updates both credential creation (POST /api/credentials for env_workspace and service_account) and env key sync/creation in environment.ts to query workspace permissions, build a user→permission map, and derive the credential role from owner/admin vs write/read.

Reviewed by Cursor Bugbot for commit 78b6c88. Bugbot is set up for automated code reviews on this repo. Configure here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 21, 2026

Greptile Summary

This PR fixes workspace admin users being incorrectly assigned member role on credential_member when workspace-scoped secrets were created or synced. It adds a permissions table lookup in ensureWorkspaceCredentialMemberships and in the POST route handler so that workspace admins receive admin credential role.

  • environment.ts: ensureWorkspaceCredentialMemberships now accepts workspaceId, queries workspace permissions, and promotes users with admin workspace permission to admin credential role; the call site in syncWorkspaceEnvCredentials is updated accordingly.
  • route.ts POST: A parallel permissions query is added alongside the existing member-ID fetch, and the same owner-or-admin check drives the role assigned to each credential_member row.
  • createWorkspaceEnvCredentials (also in environment.ts) is a third membership-insertion path that was not updated and still uses the old owner-only admin check, leaving workspace admins with member role when credentials are created through that function.

Confidence Score: 3/5

The fix is correct on the two paths it touches but leaves a third membership-insertion path (createWorkspaceEnvCredentials) using the old logic, so workspace admins will still get the wrong role on credentials created through that function.

Two of the three places that write credential_member rows for workspace-scoped credentials are corrected. createWorkspaceEnvCredentials in environment.ts — which bulk-inserts memberships for newly added env keys — was not updated and still only grants admin to the workspace owner, leaving workspace admins incorrectly assigned member through that code path.

apps/sim/lib/credentials/environment.ts — the createWorkspaceEnvCredentials function (lines 272–285) needs the same permissions-table lookup applied to the other two paths.

Important Files Changed

Filename Overview
apps/sim/lib/credentials/environment.ts Adds workspace permission lookup to ensureWorkspaceCredentialMemberships and its call site, but createWorkspaceEnvCredentials (a third membership-insertion path) retains the old owner-only admin check and will still assign member to workspace admins.
apps/sim/app/api/credentials/route.ts Correctly queries the permissions table in parallel and maps workspace admin → credential admin during credential creation; preserves the pre-existing creator-gets-admin behaviour.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Workspace credential created or synced] --> B{Type: env_workspace or service_account?}
    B -- No --> C[Insert single credentialMember as admin for creator]
    B -- Yes --> D[Fetch workspace member IDs + workspace permissions]
    D --> E[For each member user]
    E --> F{Is owner OR creator OR wsPermission === 'admin'?}
    F -- Yes --> G[role = 'admin']
    F -- No --> H[role = 'member']
    G --> I[Insert/Update credentialMember row]
    H --> I

    subgraph "createWorkspaceEnvCredentials (NOT updated)"
        J[Fetch workspace member IDs ONLY] --> K[For each member user]
        K --> L{Is owner?}
        L -- Yes --> M[role = 'admin']
        L -- No --> N[role = 'member — workspace admins get wrong role']
    end
Loading

Comments Outside Diff (1)

  1. apps/sim/lib/credentials/environment.ts, line 272-285 (link)

    P1 createWorkspaceEnvCredentials still uses the old role mapping

    createWorkspaceEnvCredentials is a third code path that bulk-inserts credential_member rows for newly added workspace env keys (line 278: memberUserId === ownerUserId ? 'admin' : 'member'). It never consults the permissions table, so workspace admins who are enrolled via the permissions table will still receive member role when credentials are created through this path. The fix applied to ensureWorkspaceCredentialMemberships and the POST route needs to be applied here as well.

Reviews (1): Last reviewed commit: "fix(credentials): reflect workspace perm..." | Re-trigger Greptile

Comment thread apps/sim/lib/credentials/environment.ts
…ntials

Address Bugbot review: the parallel credential creation path
(createWorkspaceEnvCredentials) still used owner-only admin logic.
Now queries workspace permissions table for consistent role mapping.
Comment thread apps/sim/lib/credentials/environment.ts Outdated
Address Bugbot review: permissions query was executed N times (once per
credential) inside ensureWorkspaceCredentialMemberships loop. Now queried
once in the caller and passed as a Map parameter.
Comment thread apps/sim/lib/credentials/environment.ts
Derive memberUserIds from wsPermissionRows + workspace owner instead
of calling getWorkspaceMemberUserIds separately. This removes a
duplicate query on the permissions table at every call site.
Comment thread apps/sim/app/api/credentials/route.ts Outdated
memberUserIds: string[],
ownerUserId: string
ownerUserId: string,
wsPermissionByUser: Map<string, string>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exported function now unused after callers removed

Low Severity

getWorkspaceMemberUserIds is still exported from environment.ts but is no longer called anywhere in the codebase. All callers were replaced in this PR with inline permission queries. This leaves dead code that could confuse future developers.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d1891c9. Configure here.

…ency

The credential creator (session.user.id) was always granted admin role
regardless of their workspace permission. This created inconsistency
with environment.ts sync logic which correctly derives role solely from
workspace permission. Now both paths use the same mapping.
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 78b6c88. Configure here.

.from(permissions)
.where(
and(eq(permissions.entityType, 'workspace'), eq(permissions.entityId, workspaceId))
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Permissions query bypasses transaction using db instead of tx

Medium Severity

Inside db.transaction(async (tx) => { ... }), the workspace permissions query on line 537 uses db instead of the transaction handle tx. Every other query in this transaction block correctly uses tx. This causes the permissions read to execute on a separate connection outside the transaction's isolation boundary, which means it won't see uncommitted changes from concurrent transactions consistently and consumes an extra connection from the pool. Since tx is available and used for all other operations in the same block, this looks like an oversight.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 78b6c88. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant