fix(access-control): keep superseded blocks out of the editor's allowlist universe

Author: waleedlatif1

apps/sim/ee/access-control/components/group-detail.tsx

@@ -30,7 +30,7 @@ import { formatDate } from '@sim/utils/formatting'
 import { useQueryState } from 'nuqs'
 import { saveDiscardActions } from '@/components/settings/save-discard-actions'
 import type { ShareAuthType } from '@/lib/api/contracts/public-shares'
-import { isBlockTypeAccessControlExempt } from '@/lib/permission-groups/block-access'
+import { isAccessControlAllowlistRow } from '@/lib/permission-groups/block-access'
 import { PLATFORM_CATEGORY_ORDER, PLATFORM_FEATURES } from '@/lib/permission-groups/features'
 import type { PermissionGroupConfig } from '@/lib/permission-groups/fields'
 import { UnsavedChangesModal } from '@/app/workspace/[workspaceId]/components/credential-detail'
@@ -65,6 +65,11 @@ import {
   useRemovePermissionGroupMember,
   useUpdatePermissionGroup,
 } from '@/ee/access-control/hooks/permission-groups'
+import {
+  allowlistRowsFromStored,
+  toggleAllowlistRow,
+  withAllowlistRows,
+} from '@/ee/access-control/utils/integration-allowlist-rows'
 import { SettingRow } from '@/ee/components/setting-row'
 import { useBlacklistedProviders } from '@/hooks/queries/allowed-providers'
 import { useOrganizationRoster } from '@/hooks/queries/organization'
@@ -789,9 +794,15 @@ export function GroupDetail({
    * otherwise a null→partial transition by a non-revealed admin would silently
    * drop a preview block from the stored allowlist and deny it to revealed
    * users already running it.
+   *
+   * EXCLUDES superseded blocks. They are hidden and so are never rendered, but
+   * they used to be materialized into the allowlist all the same — so an admin
+   * narrowing a previously-unrestricted allowlist by unchecking `slack_v2` wrote
+   * `slack` into it, which the runtime resolves back to `slack_v2` and allows.
+   * A decision about a retired version is made on its successor's row.
    */
   const allBlocks = useMemo(() => {
-    const blocks = getAllBlocks().filter((b) => !isBlockTypeAccessControlExempt(b.type))
+    const blocks = getAllBlocks().filter((b) => isAccessControlAllowlistRow(b.type))
     return blocks.sort((a, b) => {
       const catA = BLOCK_CATEGORY_ORDER[a.category] ?? 3
       const catB = BLOCK_CATEGORY_ORDER[b.category] ?? 3
@@ -881,17 +892,16 @@ export function GroupDetail({
 
   const guard = useSettingsUnsavedGuard({ isDirty: hasChanges })
 
+  const allBlockTypes = useMemo(() => allBlocks.map((b) => b.type), [allBlocks])
+
   /**
    * `null` means "everything allowed". Indexing the allow-lists once keeps the
    * per-row membership checks O(1) — they run for every one of the ~200 block
    * rows on each render, and again in the section-wide `every(...)` scans.
    */
   const allowedIntegrationSet = useMemo(
-    () =>
-      editingConfig.allowedIntegrations === null
-        ? null
-        : new Set(editingConfig.allowedIntegrations),
-    [editingConfig.allowedIntegrations]
+    () => allowlistRowsFromStored(allBlockTypes, editingConfig.allowedIntegrations),
+    [allBlockTypes, editingConfig.allowedIntegrations]
   )
 
   const allowedProviderSet = useMemo(
@@ -994,48 +1004,35 @@ export function GroupDetail({
   const toggleIntegration = useCallback(
     (blockType: string) => {
       setEditingConfig((prev) => {
-        const current = prev.allowedIntegrations
-        let nextAllowed: string[] | null
-        if (current === null) {
-          nextAllowed = allBlocks.map((b) => b.type).filter((t) => t !== blockType)
-        } else if (current.includes(blockType)) {
-          const updated = current.filter((t) => t !== blockType)
-          nextAllowed = updated.length === allBlocks.length ? null : updated
-        } else {
-          const updated = [...current, blockType]
-          nextAllowed = updated.length === allBlocks.length ? null : updated
-        }
+        const nextAllowed = toggleAllowlistRow(allBlockTypes, prev.allowedIntegrations, blockType)
         return {
           ...prev,
           allowedIntegrations: nextAllowed,
           deniedTools: pruneDeniedTools(nextAllowed, prev.deniedTools),
         }
       })
     },
-    [allBlocks, pruneDeniedTools]
+    [allBlockTypes, pruneDeniedTools]
   )
 
   /** Allow or deny a whole section's blocks at once, respecting the active filter. */
   const setBlocksAllowed = useCallback(
     (blocks: BlockConfig[], allowed: boolean) => {
       setEditingConfig((prev) => {
-        const allTypes = allBlocks.map((b) => b.type)
-        const current =
-          prev.allowedIntegrations === null ? new Set(allTypes) : new Set(prev.allowedIntegrations)
-        for (const block of blocks) {
-          if (allowed) current.add(block.type)
-          else current.delete(block.type)
-        }
-        const nextArr = allTypes.filter((t) => current.has(t))
-        const nextAllowed = nextArr.length === allTypes.length ? null : nextArr
+        const nextAllowed = withAllowlistRows(
+          allBlockTypes,
+          prev.allowedIntegrations,
+          blocks.map((block) => block.type),
+          allowed
+        )
         return {
           ...prev,
           allowedIntegrations: nextAllowed,
           deniedTools: pruneDeniedTools(nextAllowed, prev.deniedTools),
         }
       })
     },
-    [allBlocks, pruneDeniedTools]
+    [allBlockTypes, pruneDeniedTools]
   )
 
   const isToolAllowed = useCallback(

apps/sim/ee/access-control/utils/integration-allowlist-rows.test.ts

@@ -0,0 +1,87 @@
+/**
+ * @vitest-environment node
+ *
+ * The universe below is the editor's row set, which excludes superseded blocks
+ * (`isAccessControlAllowlistRow`). Every id is a real one, so the assertions
+ * rest on the repository's own lifecycle facts: `slack` was replaced by
+ * `slack_v2`.
+ */
+import { describe, expect, it } from 'vitest'
+import {
+  allowlistRowsFromStored,
+  toggleAllowlistRow,
+  withAllowlistRows,
+} from '@/ee/access-control/utils/integration-allowlist-rows'
+
+const UNIVERSE = ['agent', 'notion_v2', 'slack_v2'] as const
+
+describe('allowlistRowsFromStored', () => {
+  it('keeps an unrestricted allowlist unrestricted', () => {
+    expect(allowlistRowsFromStored(UNIVERSE, null)).toBeNull()
+  })
+
+  /**
+   * The runtime resolves a stored `slack` to `slack_v2` and allows it, so the
+   * row has to render checked or the editor is lying about what is permitted.
+   */
+  it('reads a stored retired id as the row it actually governs', () => {
+    const rows = allowlistRowsFromStored(UNIVERSE, ['slack'])
+
+    expect(rows?.has('slack_v2')).toBe(true)
+    expect(rows?.has('slack')).toBe(false)
+  })
+
+  it('drops an id no row corresponds to', () => {
+    expect([...(allowlistRowsFromStored(UNIVERSE, ['agent', 'retired_thing']) ?? [])]).toEqual([
+      'agent',
+    ])
+  })
+})
+
+describe('toggleAllowlistRow', () => {
+  /**
+   * The bug this closes. The editor renders only current blocks, so narrowing a
+   * previously-unrestricted allowlist used to materialize the hidden `slack`
+   * alongside the rows — and the runtime resolves `slack` back to `slack_v2`,
+   * re-allowing the integration the admin had just denied.
+   */
+  it('does not leave a superseded id behind when a row is denied', () => {
+    const next = toggleAllowlistRow(UNIVERSE, null, 'slack_v2')
+
+    expect(next).toEqual(['agent', 'notion_v2'])
+    expect(allowlistRowsFromStored(UNIVERSE, next)?.has('slack_v2')).toBe(false)
+  })
+
+  /** A stored retired id must follow its successor's fate, not outlive it. */
+  it('denies a row a stored retired id was granting', () => {
+    const next = toggleAllowlistRow(UNIVERSE, ['agent', 'slack'], 'slack_v2')
+
+    expect(next).toEqual(['agent'])
+  })
+
+  it('grants a row that was not allowed', () => {
+    expect(toggleAllowlistRow(UNIVERSE, ['agent'], 'notion_v2')).toEqual(['agent', 'notion_v2'])
+  })
+
+  /** Permitting everything is stored as "no restriction", not as a frozen list. */
+  it('collapses back to unrestricted when the last row is granted', () => {
+    expect(toggleAllowlistRow(UNIVERSE, ['agent', 'notion_v2'], 'slack_v2')).toBeNull()
+  })
+})
+
+describe('withAllowlistRows', () => {
+  it('denies a whole section at once', () => {
+    expect(withAllowlistRows(UNIVERSE, null, ['notion_v2', 'slack_v2'], false)).toEqual(['agent'])
+  })
+
+  it('emits rows in universe order however the section is ordered', () => {
+    expect(withAllowlistRows(UNIVERSE, [], ['slack_v2', 'agent'], true)).toEqual([
+      'agent',
+      'slack_v2',
+    ])
+  })
+
+  it('collapses to unrestricted when a section grant covers every row', () => {
+    expect(withAllowlistRows(UNIVERSE, ['agent'], [...UNIVERSE], true)).toBeNull()
+  })
+})

apps/sim/ee/access-control/utils/integration-allowlist-rows.ts

@@ -0,0 +1,54 @@
+import { toAccessControlAllowlist } from '@/lib/permission-groups/block-access'
+
+/**
+ * The stored `allowedIntegrations` re-expressed as editor rows: successor-
+ * resolved the way the runtime resolves it, then projected onto the universe of
+ * rows the editor actually offers. `null` stays `null` — everything allowed.
+ *
+ * A stored list can name a retired id (written before the universe excluded
+ * superseded blocks, or through the API directly). The runtime resolves it to
+ * its successor, so a stored `slack` allows `slack_v2`; reading the raw strings
+ * would render that row unchecked, and toggling it would "enable" an
+ * integration that was already permitted. Resolving first makes the checkbox
+ * tell the truth, and the projection drops the stale id on the next write.
+ */
+export function allowlistRowsFromStored(
+  universe: readonly string[],
+  stored: readonly string[] | null
+): Set<string> | null {
+  const resolved = toAccessControlAllowlist(stored)
+  return resolved === null ? null : new Set(universe.filter((type) => resolved.has(type)))
+}
+
+/**
+ * The stored allowlist after a set of rows is allowed or denied.
+ *
+ * Always emitted in universe order and collapsed back to `null` — unrestricted
+ * — when every row survives, so a group that ends up permitting everything is
+ * stored as "no restriction" rather than as a list that silently freezes out
+ * every integration added later.
+ */
+export function withAllowlistRows(
+  universe: readonly string[],
+  stored: readonly string[] | null,
+  blockTypes: readonly string[],
+  allowed: boolean
+): string[] | null {
+  const rows = allowlistRowsFromStored(universe, stored) ?? new Set(universe)
+  for (const blockType of blockTypes) {
+    if (allowed) rows.add(blockType)
+    else rows.delete(blockType)
+  }
+  const next = universe.filter((type) => rows.has(type))
+  return next.length === universe.length ? null : next
+}
+
+/** {@link withAllowlistRows} for one row, flipping whatever it is now. */
+export function toggleAllowlistRow(
+  universe: readonly string[],
+  stored: readonly string[] | null,
+  blockType: string
+): string[] | null {
+  const rows = allowlistRowsFromStored(universe, stored)
+  return withAllowlistRows(universe, stored, [blockType], !(rows === null || rows.has(blockType)))
+}

apps/sim/lib/permission-groups/block-access.test.ts

@@ -2,7 +2,10 @@
  * @vitest-environment node
  */
 import { beforeEach, describe, expect, it, vi } from 'vitest'
-import { isBlockTypeAccessControlExempt } from '@/lib/permission-groups/block-access'
+import {
+  isAccessControlAllowlistRow,
+  isBlockTypeAccessControlExempt,
+} from '@/lib/permission-groups/block-access'
 import { getBlock } from '@/blocks/registry'
 
 const mockGetBlock = getBlock as unknown as ReturnType<typeof vi.fn>
@@ -81,3 +84,40 @@ describe('isBlockTypeAccessControlExempt', () => {
     expect(isBlockTypeAccessControlExempt('manual_trigger')).toBe(true)
   })
 })
+
+describe('isAccessControlAllowlistRow', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+  })
+
+  /**
+   * The bug this closes: the editor renders only visible blocks but used to
+   * materialize an allowlist from every non-exempt one. Unchecking `slack_v2`
+   * on a previously-unrestricted group therefore wrote `slack` into the stored
+   * list, and the runtime resolves `slack` to `slack_v2` — re-allowing exactly
+   * the integration the admin had just denied.
+   */
+  it('is not a row for a superseded block, which has no row of its own', () => {
+    registry({
+      slack: { hideFromToolbar: true, sunset: { status: 'legacy', replacedBy: 'slack_v2' } },
+      slack_v2: {},
+    })
+
+    expect(isAccessControlAllowlistRow('slack')).toBe(false)
+    expect(isBlockTypeAccessControlExempt('slack')).toBe(false)
+  })
+
+  it('is a row for a current block', () => {
+    registry({ slack_v2: {} })
+
+    expect(isAccessControlAllowlistRow('slack_v2')).toBe(true)
+  })
+
+  /** Exempt block types are decided by no row at all. */
+  it('is not a row for an exempt block', () => {
+    registry({ thinking: { hideFromToolbar: true }, start_trigger: {} })
+
+    expect(isAccessControlAllowlistRow('thinking')).toBe(false)
+    expect(isAccessControlAllowlistRow('start_trigger')).toBe(false)
+  })
+})

apps/sim/lib/permission-groups/block-access.ts

@@ -48,3 +48,25 @@ export function isBlockTypeAccessControlExempt(blockType: string): boolean {
   const successor = resolveAccessControlBlockType(blockType)
   return successor === blockType || successor === UNIVERSAL_ENTRY_POINT
 }
+
+/**
+ * Whether `blockType` is a row in the Access Control editor's allowlist
+ * universe — the set the editor materializes an allowlist from and compares
+ * against to collapse one back to `null`.
+ *
+ * Narrower than {@link isBlockTypeAccessControlExempt} on purpose. A superseded
+ * block must stay non-exempt at runtime (legacy `slack` reaches Slack and is
+ * judged as `slack_v2`), but it must not be an editor row: the editor renders
+ * only visible blocks, so an admin narrowing a previously-unrestricted
+ * allowlist by unchecking `slack_v2` would still write the hidden `slack` into
+ * it — and canonical resolution then reads that entry as `slack_v2` and allows
+ * the very integration the admin just denied.
+ *
+ * Viewer-independent, like the exemption: it reads the pure registry and the
+ * generated successor map, never the visibility projection, so a preview block
+ * gated for the acting admin stays in the universe and keeps its stored grant.
+ */
+export function isAccessControlAllowlistRow(blockType: string): boolean {
+  if (isBlockTypeAccessControlExempt(blockType)) return false
+  return resolveAccessControlBlockType(blockType) === blockType
+}