Skip to content

improvement(governance): org-ws-credential roles clarity#5134

Open
icecrasher321 wants to merge 2 commits into
stagingfrom
improvement/gov-model-guarantees
Open

improvement(governance): org-ws-credential roles clarity#5134
icecrasher321 wants to merge 2 commits into
stagingfrom
improvement/gov-model-guarantees

Conversation

@icecrasher321

Copy link
Copy Markdown
Collaborator

Summary

Org Admins are auto Workspace Admins. And workspace admins are auto credential admins.

Type of Change

  • Other: UX improvement

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel

vercel Bot commented Jun 19, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
docs Ready Ready Preview, Comment Jun 19, 2026 1:20am

Request Review

@cursor

cursor Bot commented Jun 19, 2026

Copy link
Copy Markdown

PR Summary

High Risk
Changes effective permission resolution across workspaces, credentials, env secrets, and workflow authorization—security-critical paths where incorrect inheritance could grant or deny access broadly.

Overview
This PR implements a governance inheritance chain: organization Owners/Admins are treated as workspace Admins on every workspace in that org (without a per-workspace invite or permission row), and workspace Admins are treated as credential Admins on shared credentials (OAuth, service accounts, workspace env secrets)—personal env secrets stay private.

Backend enforcement adds getEffectiveWorkspacePermission / canAdmin on workspace access, merges org-admin workspaces into workspace listing (listAccessibleWorkspaceRowsForUser), and mirrors the same rules in workflow-authz. Credential admin checks and workflow credential access now grant admin/use when the caller is a workspace admin even without an explicit credentialMember row. Credential member APIs merge in workspace admins with roleSource: 'workspace-admin', block demoting/removing them, and stop bulk-seeding every workspace admin as DB credential admins on create/sync.

Product guardrails: PATCH workspace permissions rejects changing org owner/admin roles; org roster shows org admins as admin on all org workspaces. Member and credential UIs disable role changes for inherited roles and show RoleLockTooltip explanations. Permissions docs and FAQs describe credential access and the fixed inherited roles.

Tests cover credential actor context, org-admin workspace access, and accessible workspace listing elevation.

Reviewed by Cursor Bugbot for commit fc753a8. Configure here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

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.

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 fc753a8. Configure here.

Comment thread apps/sim/app/api/credentials/route.ts
@greptile-apps

greptile-apps Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR implements a two-level role-inheritance model: org owners/admins automatically become workspace admins, and workspace admins automatically become credential admins for all shared credentials. It wires that through the API layer, the UI (greyed-out role chips with tooltips), and the workflow-authz package, and replaces the old approach of seeding admin credential-member rows at creation time with a runtime derivation.

  • getEffectiveWorkspacePermission (new) merges explicit grants, workspace-owner status, and org-admin membership into a single resolved permission; all permission checks now funnel through this.
  • getOrgAdminWorkspaceRows (new) fetches workspaces an org admin controls without an explicit invite, feeding both the workspace listing endpoint and getManageableWorkspaces; this function has a role-filtering bug described below.
  • Credential-member seeding is simplified to member role only; the adminUserIds set and the orphan-prevention logic in revokeWorkspaceCredentialMemberships are removed as no longer needed.

Confidence Score: 4/5

The inheritance model is correctly wired in most paths, but getOrgAdminWorkspaceRows has a role-filter gap that can silently deny org admins their derived workspace access in the sidebar and the getManageableWorkspaces API.

getOrgAdminWorkspaceRows queries all memberships for the user with .limit(1) and checks the role after — meaning if the DB happens to return a non-admin row first, an org admin loses their derived workspace access without any error. All other new paths (checkWorkspaceAccess, credential admin derivation, PATCH guard, UI lock tooltips) look correct and well-tested.

apps/sim/lib/workspaces/utils.ts — getOrgAdminWorkspaceRows needs the role predicate moved into the WHERE clause before .limit(1).

Important Files Changed

Filename Overview
apps/sim/lib/workspaces/utils.ts Adds getOrgAdminWorkspaceRows and listAccessibleWorkspaceRowsForUser; the former has a bug where .limit(1) without a role filter can return a non-admin membership and silently deny derived workspace access.
apps/sim/lib/workspaces/permissions/utils.ts Introduces getEffectiveWorkspacePermission that correctly merges owner, explicit grant, and org-admin inheritance; checkWorkspaceAccess now exposes canAdmin; getUsersWithPermissions merges org admins as derived members.
packages/workflow-authz/src/index.ts Duplicates the org-admin inheritance logic from getEffectiveWorkspacePermission into resolveEffectiveWorkspacePermission; logic appears correct but the duplication creates a maintenance risk.
apps/sim/lib/credentials/access.ts Simplifies credential admin derivation — workspace admin status now implicitly grants credential admin; removes orphan-prevention code that is no longer needed.
apps/sim/lib/credentials/environment.ts Removes adminUserIds from getWorkspaceMembership; credential members are now seeded as member role only; admins are derived at runtime via workspace role.
apps/sim/app/api/credentials/[id]/members/route.ts GET now merges workspace admins as derived credential admins; POST and DELETE guard against demoting/removing workspace admins from shared credentials.
apps/sim/app/api/workspaces/[id]/permissions/route.ts PATCH now blocks attempts to change org admins' workspace role, with a clear 400 error message.
apps/sim/components/permissions/role-lock.tsx New RoleLockTooltip component wraps disabled role controls and explains why a role is fixed; clean implementation with no issues.
apps/sim/lib/auth/credential-access.ts Reorders workspace-access and membership checks so workspace admins bypass the per-credential membership requirement for shared credentials.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[User] -->|is owner of| WS[Workspace]
    A -->|has explicit permission row| WS
    A -->|is org owner/admin of| ORG[Organization]
    ORG -->|owns| WS

    WS -->|resolves to| WPERM{Effective Workspace Permission}
    WPERM -->|owner or org-admin| WADMIN[workspace: admin]
    WPERM -->|explicit write| WWRITE[workspace: write]
    WPERM -->|explicit read| WREAD[workspace: read]
    WPERM -->|none| WNONE[no access]

    WADMIN -->|shared credential| CADMIN[credential: admin auto-derived]
    WWRITE -->|explicit member row| CMEMBER[credential: member]
    WREAD -->|explicit member row| CMEMBER
    WWRITE -->|explicit admin row| CADMIN2[credential: admin explicit]
    WREAD -->|explicit admin row| CADMIN2

    CADMIN --> CAN_MANAGE[can use + edit + delete + share]
    CADMIN2 --> CAN_MANAGE
    CMEMBER --> CAN_USE[can use only]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[User] -->|is owner of| WS[Workspace]
    A -->|has explicit permission row| WS
    A -->|is org owner/admin of| ORG[Organization]
    ORG -->|owns| WS

    WS -->|resolves to| WPERM{Effective Workspace Permission}
    WPERM -->|owner or org-admin| WADMIN[workspace: admin]
    WPERM -->|explicit write| WWRITE[workspace: write]
    WPERM -->|explicit read| WREAD[workspace: read]
    WPERM -->|none| WNONE[no access]

    WADMIN -->|shared credential| CADMIN[credential: admin auto-derived]
    WWRITE -->|explicit member row| CMEMBER[credential: member]
    WREAD -->|explicit member row| CMEMBER
    WWRITE -->|explicit admin row| CADMIN2[credential: admin explicit]
    WREAD -->|explicit admin row| CADMIN2

    CADMIN --> CAN_MANAGE[can use + edit + delete + share]
    CADMIN2 --> CAN_MANAGE
    CMEMBER --> CAN_USE[can use only]
Loading

Comments Outside Diff (2)

  1. packages/workflow-authz/src/index.ts, line 436-472 (link)

    P2 Duplicated governance logic will diverge silently

    resolveEffectiveWorkspacePermission here is a hand-rolled copy of getEffectiveWorkspacePermission in apps/sim. The comment acknowledges the duplication, but any future change to the inheritance rules (e.g., adding a new elevated role) must be applied to both places. A missed update would mean workflow execution enforces different access rules than the rest of the app — a subtle auth gap. Consider exporting a shared utility from a @sim/* package, or at minimum add a CI lint rule or a comment cross-referencing the exact function name and file so reviewers know to keep them in sync.

  2. apps/sim/lib/api/contracts/credentials.ts, line 782-783 (link)

    P2 roleSource is optional in the schema but always set by the server

    The GET endpoint (/credentials/[id]/members) now always populates roleSource on every member, yet the schema marks it .optional(). Consumers in credential-members-section.tsx already guard for undefined correctly, but because Zod will accept responses that omit roleSource without error, a regression that accidentally drops the field would be silent. If roleSource is always expected in new responses, making it required (with a default: 'explicit' or a migration plan for any clients that may send old shapes) would close that gap.

    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!

Reviews (1): Last reviewed commit: "revert isHosted" | Re-trigger Greptile

Comment on lines +58 to +66
const [membership] = await db
.select({ organizationId: member.organizationId, role: member.role })
.from(member)
.where(eq(member.userId, userId))
.limit(1)

if (!membership || (membership.role !== 'owner' && membership.role !== 'admin')) {
return []
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Role filter missing before .limit(1)

The query fetches any membership for the user and checks the role after, but .limit(1) without a role filter means the DB can return a non-admin row first (ordering is non-deterministic without ORDER BY). A user who is an admin in org-B but whose first returned row happens to be a member row for org-A will silently get [] back — their derived workspace admin access will be invisible in the sidebar, permission checks, and getManageableWorkspaces.

The inArray(member.role, ['owner', 'admin']) predicate needs to be inside the WHERE clause, not just checked on the result after the limit is applied.

Suggested change
const [membership] = await db
.select({ organizationId: member.organizationId, role: member.role })
.from(member)
.where(eq(member.userId, userId))
.limit(1)
if (!membership || (membership.role !== 'owner' && membership.role !== 'admin')) {
return []
}
const [membership] = await db
.select({ organizationId: member.organizationId, role: member.role })
.from(member)
.where(and(eq(member.userId, userId), inArray(member.role, ['owner', 'admin'])))
.limit(1)
if (!membership) {
return []
}

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