Merge branch 'pgx/p2' into feat/permission-groups-coverage

Author: waleedlatif1

apps/sim/app/api/v1/capability-gate.test.ts

@@ -271,9 +271,10 @@ describe('v1 permission-group capability gate', () => {
 
   /**
    * `personal_api_key.use` refuses a *principal kind* rather than a module, so it
-   * is asserted in `resolveWorkspaceScope` — before, and separately from, the
-   * capability the route declares. A workspace key is not a personal key, and its
-   * creator's group must not decide whether it may be used.
+   * is asserted separately from the capability the route declares — but, like
+   * every other group key, only after the workspace role check. A workspace key
+   * is not a personal key, and its creator's group must not decide whether it
+   * may be used.
    */
   describe('personal_api_key.use — the key kind, not the module', () => {
     it('refuses a personal key whose group disables personal API keys', async () => {
@@ -296,6 +297,42 @@ describe('v1 permission-group capability gate', () => {
       expect(response.status).toBe(200)
       expect(mockListTables).toHaveBeenCalledWith(WORKSPACE_ID)
     })
+
+    /**
+     * The group key runs behind the role check, so a stranger to the workspace
+     * is answered with the concealed role failure rather than with a refusal
+     * naming how an organization configured one of its cohorts. Asked the other
+     * way round, a caller with no reach into the workspace at all learns that
+     * the workspace's organization runs a group, and that the group withholds
+     * personal keys.
+     *
+     * The workspace COLUMN still answers first — it names no group, needs no
+     * query, and is the answer whatever the role turns out to be — which is the
+     * split `authorizeWorkspaceOperation` makes and the next case pins.
+     */
+    it('answers a non-member on role, not on the group that withholds personal keys', async () => {
+      mockGetUserEntityPermissions.mockResolvedValue(null)
+      governedBy({ disablePersonalApiKeys: true })
+
+      const response = await getTables(get(`/api/v1/tables?workspaceId=${WORKSPACE_ID}`))
+      const body = await response.json()
+
+      expect(response.status).toBe(403)
+      expect(body.error).toBe('Access denied')
+      expect(mockListTables).not.toHaveBeenCalled()
+    })
+
+    it("answers a non-member on the workspace's own column, which names no group", async () => {
+      mockGetUserEntityPermissions.mockResolvedValue(null)
+      mockGetWorkspaceBillingSettings.mockResolvedValue({ allowPersonalApiKeys: false })
+
+      const response = await getTables(get(`/api/v1/tables?workspaceId=${WORKSPACE_ID}`))
+      const body = await response.json()
+
+      expect(response.status).toBe(403)
+      expect(body.error).toMatch(/personal API key/i)
+      expect(mockListTables).not.toHaveBeenCalled()
+    })
   })
 
   it('refuses on role before capability, so a non-member learns nothing about the group', async () => {

apps/sim/app/api/v1/middleware.test.ts

@@ -7,7 +7,12 @@
  * `used = limit - remaining` gets a negative number.
  */
 
-import { createMockRequest } from '@sim/testing'
+import {
+  createMockRequest,
+  permissionGroupScopeMock,
+  permissionGroupScopeMockFns,
+  resetPermissionGroupScopeMock,
+} from '@sim/testing'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 import { z } from 'zod'
 import { workspaceIdSchema } from '@/lib/api/contracts/primitives'
@@ -17,13 +22,21 @@ import {
   recordRateLimitSnapshot,
 } from '@/lib/api/server/rate-limit-context'
 
-const { mockAuthenticateV1Request, mockGetSubscription, mockCheckRateLimit, mockGetRateLimit } =
-  vi.hoisted(() => ({
-    mockAuthenticateV1Request: vi.fn(),
-    mockGetSubscription: vi.fn(),
-    mockCheckRateLimit: vi.fn(),
-    mockGetRateLimit: vi.fn(),
-  }))
+const {
+  mockAuthenticateV1Request,
+  mockGetSubscription,
+  mockCheckRateLimit,
+  mockGetRateLimit,
+  mockGetUserEntityPermissions,
+  mockGetWorkspaceBillingSettings,
+} = vi.hoisted(() => ({
+  mockAuthenticateV1Request: vi.fn(),
+  mockGetSubscription: vi.fn(),
+  mockCheckRateLimit: vi.fn(),
+  mockGetRateLimit: vi.fn(),
+  mockGetUserEntityPermissions: vi.fn(),
+  mockGetWorkspaceBillingSettings: vi.fn(),
+}))
 
 vi.mock('@/app/api/v1/auth', () => ({
   authenticateV1Request: mockAuthenticateV1Request,
@@ -40,9 +53,22 @@ vi.mock('@/lib/core/rate-limiter', () => ({
   },
 }))
 
+vi.mock('@/lib/permission-groups/config-scope.server', () => permissionGroupScopeMock)
+
+vi.mock('@/lib/workspaces/permissions/utils', () => ({
+  getUserEntityPermissions: mockGetUserEntityPermissions,
+}))
+
+vi.mock('@/lib/workspaces/utils', () => ({
+  getWorkspaceBillingSettings: mockGetWorkspaceBillingSettings,
+  getWorkspaceBilledAccountUserId: vi.fn(async () => 'billed-user'),
+}))
+
+import { DEFAULT_PERMISSION_GROUP_CONFIG } from '@/lib/permission-groups/fields'
 import {
   authenticateRequest,
   checkRateLimit,
+  checkWorkspaceScope,
   createRateLimitResponse,
   v1ValidationErrorResponse,
 } from '@/app/api/v1/middleware'
@@ -274,3 +300,79 @@ describe('rate-limit snapshot context', () => {
     expect(getRateLimitHeaders(req)).toBeNull()
   })
 })
+
+/**
+ * The table routes authorize with `checkWorkspaceScope` and then a domain
+ * helper (`checkAccess`) that runs the workspace ROLE check. `checkAccess`
+ * gates the module (`tables.use`), never the key kind, so `personal_api_key.use`
+ * has to be asked in the wrapper — which means the wrapper has to order itself
+ * behind the role, because nothing downstream will.
+ *
+ * The workspace column keeps answering first: it names no group, so refusing on
+ * it tells a stranger only what the workspace itself is set to.
+ */
+describe('checkWorkspaceScope', () => {
+  const WORKSPACE_ID = '11111111-1111-4111-8111-111111111111'
+  const USER_ID = 'user-1'
+
+  function personalKeyRateLimit() {
+    return {
+      allowed: true,
+      remaining: 1,
+      limit: 1,
+      resetAt: new Date(),
+      userId: USER_ID,
+      keyType: 'personal' as const,
+      principal: { kind: 'personal_api_key' as const, userId: USER_ID, keyId: 'key-1' },
+    }
+  }
+
+  function withholdsPersonalKeys() {
+    permissionGroupScopeMockFns.mockResolvePermissionGroupConfig.mockResolvedValue({
+      ...DEFAULT_PERMISSION_GROUP_CONFIG,
+      disablePersonalApiKeys: true,
+    })
+  }
+
+  beforeEach(() => {
+    vi.clearAllMocks()
+    resetPermissionGroupScopeMock()
+    mockGetWorkspaceBillingSettings.mockResolvedValue({ allowPersonalApiKeys: true })
+    mockGetUserEntityPermissions.mockResolvedValue('admin')
+  })
+
+  it('refuses a member whose group withholds personal API keys', async () => {
+    withholdsPersonalKeys()
+
+    const response = await checkWorkspaceScope(personalKeyRateLimit(), WORKSPACE_ID)
+
+    expect(response).not.toBeNull()
+    expect(response?.status).toBe(403)
+    await expect(response?.json()).resolves.toMatchObject({
+      error: expect.stringMatching(/personal API key/i),
+    })
+  })
+
+  it('leaves a non-member to the downstream role check rather than naming the group', async () => {
+    mockGetUserEntityPermissions.mockResolvedValue(null)
+    withholdsPersonalKeys()
+
+    const response = await checkWorkspaceScope(personalKeyRateLimit(), WORKSPACE_ID)
+
+    expect(response).toBeNull()
+    expect(permissionGroupScopeMockFns.mockResolvePermissionGroupConfig).not.toHaveBeenCalled()
+  })
+
+  it("still refuses a non-member on the workspace's own column, which names no group", async () => {
+    mockGetUserEntityPermissions.mockResolvedValue(null)
+    mockGetWorkspaceBillingSettings.mockResolvedValue({ allowPersonalApiKeys: false })
+
+    const response = await checkWorkspaceScope(personalKeyRateLimit(), WORKSPACE_ID)
+
+    expect(response).not.toBeNull()
+    expect(response?.status).toBe(403)
+    await expect(response?.json()).resolves.toMatchObject({
+      error: expect.stringMatching(/personal API key/i),
+    })
+  })
+})

apps/sim/app/api/v1/middleware.ts

@@ -344,6 +344,15 @@ export async function resolveCapabilityRefusal(
  * - A personal key is rejected when the workspace has disabled personal API
  *   keys (`allowPersonalApiKeys = false`). Other surfaces enforcing the same
  *   policy share `PERSONAL_KEY_DENIED`.
+ *
+ * Both are properties of the workspace rather than of any group, so both run
+ * ahead of the role check, exactly as `authorizeWorkspaceOperation` runs the
+ * `allowPersonalApiKeys` column ahead of `requireCurrentHumanRole`: they need
+ * no group to resolve, and refusing a key the workspace has switched off is the
+ * answer whatever the caller's role turns out to be.
+ *
+ * The group half of the same policy is NOT here — see
+ * {@link resolvePersonalKeyGroupRefusal}.
  */
 export async function resolveWorkspaceScope(
   rateLimit: RateLimitResult,
@@ -370,40 +379,68 @@ export async function resolveWorkspaceScope(
         message: PERSONAL_KEY_DENIED,
       }
     }
-
-    /**
-     * permission-group-enforced: personal_api_key.use — v1 authorizes in this
-     * middleware rather than through the application funnel, so the group check
-     * the funnel applies has to be repeated here or the same key that v2
-     * refuses would still work against v1.
-     */
-    const governedUserId = capabilityGovernedUserId(rateLimit)
-    if (governedUserId) {
-      const withheld = await isWorkspaceCapabilityWithheld(
-        governedUserId,
-        requestedWorkspaceId,
-        'personal_api_key.use'
-      )
-      if (withheld) {
-        return {
-          status: 403,
-          code: 'FORBIDDEN',
-          message: PERSONAL_KEY_DENIED,
-        }
-      }
-    }
   }
 
   return null
 }
 
 /**
- * Core workspace-access check: key scope, then the user's workspace permission
- * level, then the permission-group capability the route declares. Returns a
- * structured failure or null on success.
+ * The group half of the personal-key policy: `personal_api_key.use`, repeated
+ * here because v1 authorizes in this middleware rather than through the
+ * application funnel, and without it the same key that v2 refuses would still
+ * work against v1.
+ *
+ * It answers only AFTER the caller's workspace role has been verified, which is
+ * the ordering `authorizeWorkspaceOperation` uses and the reason
+ * {@link resolveCapabilityRefusal}'s contract says never to run a group key
+ * ahead of the role: the refusal names how an organization configured one
+ * cohort, and handing that to a caller with no reach into the workspace tells a
+ * stranger about the organization's configuration. The column check above may
+ * stay early precisely because it names no group.
  *
- * Capability comes last, matching `authorizeWorkspaceOperation` — see
- * {@link resolveCapabilityRefusal} for why the ordering is load-bearing.
+ * `roleVerifiedFor` is the user id a caller has already checked, not a boolean,
+ * so a caller that verified some OTHER subject's role cannot vouch for this
+ * one. When it does not match, the role is resolved here instead, and a caller
+ * with no read access is handed back `null` so the surface's own role failure —
+ * the concealed one — is what it answers with. That second lookup is free:
+ * `getUserEntityPermissions` for a workspace goes through the request-scoped
+ * memo the role check itself uses.
+ */
+async function resolvePersonalKeyGroupRefusal(
+  rateLimit: RateLimitResult,
+  workspaceId: string,
+  roleVerifiedFor: string | null
+): Promise<WorkspaceAccessError | null> {
+  const governedUserId = capabilityGovernedUserId(rateLimit)
+  if (!governedUserId) return null
+
+  if (roleVerifiedFor !== governedUserId) {
+    const permission = await getUserEntityPermissions(governedUserId, 'workspace', workspaceId)
+    if (!permissionSatisfies(permission, 'read')) return null
+  }
+
+  // permission-group-enforced: personal_api_key.use — v1 authorizes in this middleware, not through the funnel
+  if (!(await isWorkspaceCapabilityWithheld(governedUserId, workspaceId, 'personal_api_key.use'))) {
+    return null
+  }
+
+  return {
+    status: 403,
+    code: 'FORBIDDEN',
+    message: PERSONAL_KEY_DENIED,
+  }
+}
+
+/**
+ * Core workspace-access check: key scope and the workspace's own columns, then
+ * the user's workspace permission level, then the two permission-group
+ * decisions — the personal-key refusal, then the capability the route declares.
+ * Returns a structured failure or null on success.
+ *
+ * Both group keys come after the role, matching `authorizeWorkspaceOperation` —
+ * see {@link resolveCapabilityRefusal} for why the ordering is load-bearing.
+ * The personal-key refusal sits first of the two for the reason the funnel
+ * gives: the remedies differ, and the narrower one is worth naming first.
  */
 export async function resolveWorkspaceAccess(
   rateLimit: RateLimitResult,
@@ -420,22 +457,33 @@ export async function resolveWorkspaceAccess(
     return { status: 403, code: 'FORBIDDEN', message: 'Access denied' }
   }
 
+  const personalKeyRefusal = await resolvePersonalKeyGroupRefusal(rateLimit, workspaceId, userId)
+  if (personalKeyRefusal) return personalKeyRefusal
+
   return resolveCapabilityRefusal(rateLimit, workspaceId, capability)
 }
 
 /**
- * v1 wrapper: renders {@link resolveWorkspaceScope} as the v1 `{ error }` body.
+ * v1 wrapper: renders {@link resolveWorkspaceScope} as the v1 `{ error }` body,
+ * plus the personal-key group refusal that belongs with it.
+ *
+ * It deliberately gates no MODULE capability: it runs before the route's role
+ * check, and a route using it authorizes its resource through a domain helper
+ * afterwards (the table routes call `checkAccess`, which applies `tables.use`
+ * itself), so the capability is declared there.
  *
- * Scope only — it deliberately gates no module capability, because it runs
- * before the route's role check. A route using it authorizes its resource
- * through a domain helper afterwards (the table routes call `checkAccess`,
- * which applies `tables.use` itself), so the capability is declared there.
+ * `personal_api_key.use` cannot wait for that helper — `checkAccess` gates the
+ * module, not the key kind — so it is asked here, and
+ * {@link resolvePersonalKeyGroupRefusal} resolves the caller's role itself
+ * before answering rather than relying on a role check this wrapper never runs.
  */
 export async function checkWorkspaceScope(
   rateLimit: RateLimitResult,
   requestedWorkspaceId: string
 ): Promise<NextResponse | null> {
-  const failure = await resolveWorkspaceScope(rateLimit, requestedWorkspaceId)
+  const failure =
+    (await resolveWorkspaceScope(rateLimit, requestedWorkspaceId)) ??
+    (await resolvePersonalKeyGroupRefusal(rateLimit, requestedWorkspaceId, null))
   return failure ? workspaceAccessErrorResponse(failure) : null
 }
 

apps/sim/app/workspace/[workspaceId]/settings/components/api-keys/api-keys.tsx

@@ -119,9 +119,21 @@ export function ApiKeys({ scope = 'workspace' }: ApiKeysProps) {
    * enterprise organization. The server combines them the same way, so offering
    * a key type here that it would refuse is the only failure worth avoiding —
    * which is why the policy fails closed while its query is pending or errored,
-   * rather than treating an unanswered question as an unrestricted answer. In
-   * personal scope the hook is disabled (no `workspaceId`) and no group
-   * applies, so `isSuccess` is only required when the query actually runs.
+   * rather than treating an unanswered question as an unrestricted answer.
+   *
+   * Failing closed on `isSuccess` needs the query to be able to recover, or one
+   * transient failure disables the create button for the session with nothing
+   * to say why: the client's defaults retry once, never on remount, and do not
+   * refetch on focus outside the desktop app. `useUserPermissionConfig` raises
+   * both, which is what makes this gate self-healing rather than sticky.
+   *
+   * The `!workspaceId` arm is defense, not a live case: this component ships
+   * only from the workspace settings panel, always as `scope='combined'`, and
+   * `workspaceId` is that route's own param, so the query always runs. It
+   * covers the `|| ''` fallback above — a render outside the route would
+   * disable the hook, and `isSuccess` on a query that never runs is false
+   * forever, which would present as a dead button rather than a refusal. The
+   * server is the enforcement either way; this gate is the affordance.
    */
   const permissionPolicyReady = !workspaceId || permissionConfigQuery.isSuccess