fix(knowledge): close two admin-mode gaps found in an architecture audit

Two real bugs, then the duplication and drift a full re-read of the branch
turned up.

Administrator mode was unreachable for Confluence. Entering the mode required
an impersonation subject on every connector, but a Confluence service account
holds an API token that already speaks for the site and impersonates nobody, so
the check refused every attempt. A subject is now required only of a connector
whose auth declares a subject field, and a test pins the token-backed case.

An incremental listing could not carry a revoked grant. A permission change
moves no content — re-sharing a file does not touch its modified time in Drive,
restricting a page does not touch its version in Confluence — so an incremental
run listed only edited documents and the ACL pass refreshed only those. A grant
revoked on an unchanged document stood until the next full sync happened to
run, which is the over-grant direction. Administrator mode now always lists
the whole corpus; content is still hydrated by hash, so unchanged documents are
never re-fetched or re-embedded, and the cost is metadata pages only.

Configuration validation minted without impersonation. The shared token
resolver took the source config as optional, and the validate path did not pass
it, so a Drive service account checked its configuration against an empty
domain. The config is required now, and every caller — sync, validation, mode
switch, creation, and the directory scheduler — passes it.

The Workspace domain was derived in two places with a comment saying they must
agree. It is one function now, beside the Google directory adapter, which moves
from the knowledge library to the Drive connector where Confluence's equivalent
already lives; provider-specific API clients belong with their connector, and
the orchestration that calls them stays provider-agnostic. The Confluence
connector memoised its cloud id in four separate copies, which became one.

A group identifier is canonicalised once, in `canonicalGroupId`, by the crawl
that writes a token and the directory sync that stores the membership it
resolves against. Both already lower-cased by different routes; now they cannot
drift. Confluence identifies groups by id, so the docs claiming "never an opaque
id" were wrong and are corrected in the token vocabulary and the schema.

Removed what nothing read: the impersonation subject the token resolver
returned, a `force` flag no caller passed, two vestigial type aliases, and a
nesting-depth constant that had one consumer and now lives with it. The
directory recency check is one aggregate query rather than two probes; the
Confluence ACL resolver enriches principals once rather than once per page; the
mode picker validates its value instead of casting it; and the admin-mode hint
no longer describes a field only Drive has. The create path had two copies of
the admin-role gate, one per permission-scoped mode, and has one.

Author: waleedlatif1

apps/sim/app/workspace/[workspaceId]/knowledge/[id]/components/connector-access-field/connector-access-field.tsx

@@ -3,6 +3,7 @@
 import type { ReactNode } from 'react'
 import { ButtonGroup, ButtonGroupItem, ChipCombobox, ChipModalField } from '@sim/emcn'
 import type { ConnectorAccessMode } from '@/lib/api/contracts/knowledge/connectors'
+import { isConnectorAccessMode } from '@/lib/knowledge/connectors/access-modes'
 import {
   type ConnectorMemberGroupOptions,
   decodeConnectorMemberGroupOption,
@@ -51,7 +52,7 @@ function accessHint(input: {
     return `Everyone in the workspace is invited by email to connect their ${input.connectorConfig.name} account when the first sync starts. Each member sees only the documents their own account can open; scheduled, API, and chat runs see workspace-visible documents only.`
   }
   if (input.mode === 'admin') {
-    return `Indexed once as the ${input.connectorConfig.name} administrator you name below, keeping each document's own permissions. People see only what ${input.connectorConfig.name} already lets them open; scheduled, API, and chat runs see workspace-visible documents only.`
+    return `Indexed once as an administrator, keeping each document's own permissions. People see only what ${input.connectorConfig.name} already lets them open; scheduled, API, and chat runs see workspace-visible documents only.`
   }
   return input.allowMembers ? undefined : 'Per-member access is turned off for this workspace.'
 }
@@ -117,7 +118,9 @@ export function ConnectorAccessField({
       <div className='flex flex-col gap-2'>
         <ButtonGroup
           value={value.accessMode}
-          onValueChange={(mode) => onChange({ accessMode: mode as ConnectorAccessMode })}
+          onValueChange={(mode) => {
+            if (isConnectorAccessMode(mode)) onChange({ accessMode: mode })
+          }}
         >
           <ButtonGroupItem value='workspace' disabled={disabled}>
             Workspace

apps/sim/connectors/confluence/confluence.ts

@@ -311,6 +311,25 @@ function cqlResultToStub(item: Record<string, unknown>, domain: string): Externa
   })
 }
 
+/**
+ * The site's cloud id, memoised on the run so it is discovered once per sync
+ * rather than once per call — and taken from the credential where a service
+ * account already carries it, since its API token cannot call
+ * `accessible-resources` to discover one.
+ */
+async function resolveCloudId(
+  accessToken: string,
+  sourceConfig: Record<string, unknown>,
+  syncContext?: Record<string, unknown>
+): Promise<string> {
+  const cached = syncContext?.cloudId
+  if (typeof cached === 'string' && cached) return cached
+  const domain = normalizeConfluenceDomainHost(sourceConfig.domain as string)
+  const cloudId = await getConfluenceCloudId(domain, accessToken)
+  if (syncContext) syncContext.cloudId = cloudId
+  return cloudId
+}
+
 /**
  * The provider segment of every Confluence group token. Fixed, and baked into
  * stored ACLs, so it must never change.
@@ -336,12 +355,7 @@ async function resolveConfluenceAcls(
   externalIds: string[],
   syncContext?: Record<string, unknown>
 ): Promise<Record<string, string[]>> {
-  const domain = normalizeConfluenceDomainHost(sourceConfig.domain as string)
-  let cloudId = syncContext?.cloudId as string | undefined
-  if (!cloudId) {
-    cloudId = await getConfluenceCloudId(domain, accessToken)
-    if (syncContext) syncContext.cloudId = cloudId
-  }
+  const cloudId = await resolveCloudId(accessToken, sourceConfig, syncContext)
 
   const spaceKeys = parseMultiValue(sourceConfig.spaceKey)
   const spacePrincipals: ConfluencePrincipal[] = []
@@ -382,8 +396,10 @@ async function resolveConfluenceAcls(
   })
 
   /**
-   * Addresses are resolved once for every account named anywhere, rather than
-   * per page: a space's own principals appear on every page that inherits them.
+   * Addresses are resolved once for every account named anywhere and written
+   * onto the principals in place: a space's own principals appear on every page
+   * that inherits them, and each restriction object is shared by every chain
+   * that walked through it.
    */
   const accountIds = new Set<string>()
   for (const principal of spacePrincipals) {
@@ -395,10 +411,16 @@ async function resolveConfluenceAcls(
     }
   }
   const emails = await resolveUserEmails(cloudId, accessToken, [...accountIds])
-  const withEmail = (principals: readonly ConfluencePrincipal[]): ConfluencePrincipal[] =>
-    principals.map((principal) =>
-      principal.kind === 'user' ? { ...principal, email: emails.get(principal.id) } : principal
-    )
+  const withEmail = (principals: ConfluencePrincipal[]): void => {
+    for (const principal of principals) {
+      /** A restriction sometimes discloses the address itself; a lookup miss must not erase it. */
+      if (principal.kind === 'user') principal.email = emails.get(principal.id) ?? principal.email
+    }
+  }
+  withEmail(spacePrincipals)
+  for (const restriction of restrictions.values()) {
+    if (restriction !== null) withEmail(restriction)
+  }
 
   const acls: Record<string, string[]> = {}
   let unattributed = 0
@@ -407,8 +429,8 @@ async function resolveConfluenceAcls(
     /** A page whose restrictions could not be read is readable by nobody, not by everyone. */
     if (!chain) continue
     const result = confluencePageAcl({
-      spacePrincipals: withEmail(spacePrincipals),
-      restrictionChain: chain.map((entry) => (entry === null ? null : withEmail(entry))),
+      spacePrincipals,
+      restrictionChain: chain,
       providerId: CONFLUENCE_ACL_PROVIDER_ID,
       tenantId: cloudId,
     })
@@ -445,11 +467,7 @@ export const confluenceConnector: ConnectorConfig = {
       throw new Error('At least one space key is required')
     }
 
-    let cloudId = syncContext?.cloudId as string | undefined
-    if (!cloudId) {
-      cloudId = await getConfluenceCloudId(domain, accessToken)
-      if (syncContext) syncContext.cloudId = cloudId
-    }
+    const cloudId = await resolveCloudId(accessToken, sourceConfig, syncContext)
 
     /**
      * Route through CQL when a label filter is set, when multiple spaces are
@@ -508,15 +526,11 @@ export const confluenceConnector: ConnectorConfig = {
 
   getDocumentAcls: resolveConfluenceAcls,
 
-  openDirectory: async (accessToken, sourceConfig, syncContext) => {
-    const domain = normalizeConfluenceDomainHost(sourceConfig.domain as string)
-    let cloudId = syncContext?.cloudId as string | undefined
-    if (!cloudId) {
-      cloudId = await getConfluenceCloudId(domain, accessToken)
-      if (syncContext) syncContext.cloudId = cloudId
-    }
-    return openConfluenceDirectory(cloudId, accessToken)
-  },
+  openDirectory: async (accessToken, sourceConfig, syncContext) =>
+    openConfluenceDirectory(
+      await resolveCloudId(accessToken, sourceConfig, syncContext),
+      accessToken
+    ),
 
   getDocument: async (
     accessToken: string,
@@ -525,11 +539,7 @@ export const confluenceConnector: ConnectorConfig = {
     syncContext?: Record<string, unknown>
   ): Promise<ExternalDocument | null> => {
     const domain = normalizeConfluenceDomainHost(sourceConfig.domain as string)
-    let cloudId = syncContext?.cloudId as string | undefined
-    if (!cloudId) {
-      cloudId = await getConfluenceCloudId(domain, accessToken)
-      if (syncContext) syncContext.cloudId = cloudId
-    }
+    const cloudId = await resolveCloudId(accessToken, sourceConfig, syncContext)
 
     /**
      * Fetch the `view` representation rather than `storage`. Storage format only

…edge/connectors/google-directory.test.ts …onnectors/google-drive/directory.test.tsapps/sim/lib/knowledge/connectors/google-directory.test.ts renamed to apps/sim/connectors/google-drive/directory.test.ts apps/sim/lib/knowledge/connectors/google-directory.test.ts renamed to apps/sim/connectors/google-drive/directory.test.ts

@@ -2,8 +2,7 @@
  * @vitest-environment node
  */
 import { beforeEach, describe, expect, it, vi } from 'vitest'
-import { MAX_GROUP_NESTING_DEPTH } from '@/lib/knowledge/access/external-groups'
-import { listDomainGroups, listGroupMembers } from '@/lib/knowledge/connectors/google-directory'
+import { listDomainGroups, listGroupMembers } from '@/connectors/google-drive/directory'
 
 const mockFetch = vi.fn()
 
@@ -115,7 +114,7 @@ describe('listGroupMembers', () => {
 
   it('reports an incomplete walk rather than a truncated membership', async () => {
     const members: Record<string, unknown[]> = {}
-    for (let depth = 0; depth <= MAX_GROUP_NESTING_DEPTH + 1; depth += 1) {
+    for (let depth = 0; depth <= 32; depth += 1) {
       members[`g${depth}@corp.com`] = [USER(`u${depth}@corp.com`), NESTED(`g${depth + 1}@corp.com`)]
     }
     directory(members)

…knowledge/connectors/google-directory.ts …sim/connectors/google-drive/directory.tsapps/sim/lib/knowledge/connectors/google-directory.ts renamed to apps/sim/connectors/google-drive/directory.ts apps/sim/lib/knowledge/connectors/google-directory.ts renamed to apps/sim/connectors/google-drive/directory.ts

@@ -1,5 +1,5 @@
 import { createLogger } from '@sim/logger'
-import { MAX_GROUP_NESTING_DEPTH } from '@/lib/knowledge/access/external-groups'
+import { canonicalGroupId } from '@/lib/knowledge/access/tokens'
 import type {
   ConnectorDirectory,
   ConnectorDirectoryGroup,
@@ -14,8 +14,30 @@ const PAGE_SIZE = 200
 /** Guards against a directory that keeps paginating; far above any real domain. */
 const MAX_PAGES = 200
 
-export type DirectoryGroup = ConnectorDirectoryGroup
-export type DirectoryGroupMembership = ConnectorDirectoryMembership
+/**
+ * How deep nested groups are followed when flattening membership.
+ *
+ * A directory can nest groups arbitrarily and can contain cycles, so the walk
+ * needs both a visited set and a depth bound. Onyx does not recurse at all,
+ * which silently drops everyone who is a member only through a subgroup; a
+ * bounded walk covers every real directory while still terminating.
+ */
+const MAX_GROUP_NESTING_DEPTH = 10
+
+/**
+ * The Workspace domain an administrator's address belongs to, or undefined
+ * when the address is blank.
+ *
+ * This is the tenant of every group token a Drive crawl writes and of every
+ * group the directory sync stores, so it is derived in exactly one place: a
+ * crawl and a directory that spelled it differently would produce grants
+ * nothing ever resolves.
+ */
+export function googleWorkspaceDomain(adminEmail: unknown): string | undefined {
+  if (typeof adminEmail !== 'string') return undefined
+  const domain = adminEmail.trim().toLowerCase().split('@')[1]
+  return domain || undefined
+}
 
 interface DirectoryListResponse<T> {
   nextPageToken?: string
@@ -75,11 +97,11 @@ interface RawMember {
 export async function listDomainGroups(
   accessToken: string,
   domain: string
-): Promise<DirectoryGroup[]> {
+): Promise<ConnectorDirectoryGroup[]> {
   const raw = await listAll<RawGroup>(`${DIRECTORY_BASE}/groups`, accessToken, 'groups', { domain })
-  const groups: DirectoryGroup[] = []
+  const groups: ConnectorDirectoryGroup[] = []
   for (const group of raw) {
-    const id = group.email?.trim().toLowerCase()
+    const id = group.email ? canonicalGroupId(group.email) : ''
     if (!id) continue
     groups.push({ id })
   }
@@ -100,8 +122,8 @@ export async function listDomainGroups(
  */
 export async function listGroupMembers(
   accessToken: string,
-  group: DirectoryGroup
-): Promise<DirectoryGroupMembership> {
+  group: ConnectorDirectoryGroup
+): Promise<ConnectorDirectoryMembership> {
   const memberEmails = new Set<string>()
   const visited = new Set<string>([group.id])
   let complete = true
@@ -139,18 +161,12 @@ export async function listGroupMembers(
   return { group, memberEmails: [...memberEmails], complete }
 }
 
-/**
- * The Workspace domain the crawl is looking at, as a directory.
- *
- * The tenant is the impersonated administrator's email domain, which is what
- * `driveAclContext` writes into every group token, so the two must derive it the
- * same way or the tokens a crawl writes name a directory nothing resolves.
- */
+/** The Workspace domain the crawl is looking at, as a directory. */
 export function openGoogleDirectory(
   accessToken: string,
-  adminEmail: string | undefined
+  adminEmail: unknown
 ): ConnectorDirectory | null {
-  const tenantId = adminEmail?.trim().toLowerCase().split('@')[1]
+  const tenantId = googleWorkspaceDomain(adminEmail)
   if (!tenantId) return null
   return {
     tenantId,

apps/sim/connectors/google-drive/google-drive.ts

@@ -6,7 +6,6 @@ import {
   driveFileAcl,
   type OpenSharingPolicy,
 } from '@/lib/knowledge/access/drive-permissions'
-import { openGoogleDirectory } from '@/lib/knowledge/connectors/google-directory'
 import {
   attachRetryHeaders,
   isRetryableError,
@@ -15,6 +14,7 @@ import {
   retryWithExponentialBackoff,
   VALIDATE_RETRY_OPTIONS,
 } from '@/lib/knowledge/documents/utils'
+import { googleWorkspaceDomain, openGoogleDirectory } from '@/connectors/google-drive/directory'
 import {
   GoogleDriveApiError,
   readGoogleDriveApiError,
@@ -465,15 +465,13 @@ interface DriveAclContext {
  * directory a group belongs to, and how far the admin has opted into open
  * sharing being searchable.
  *
- * The tenant is the impersonated administrator's email domain, which is the
- * Workspace domain the crawl is looking at. It has to be decided once and kept:
- * it is baked into every stored `g:` token, so deriving it differently later
- * would orphan every ACL already written. Null when no administrator is
- * configured, which is every crawl that is not mirroring permissions.
+ * The tenant is the impersonated administrator's Workspace domain, derived by
+ * the same function the directory sync uses so the two can never disagree.
+ * Null when no administrator is configured, which is every crawl that is not
+ * mirroring permissions.
  */
 function driveAclContext(sourceConfig: Record<string, unknown>): DriveAclContext | null {
-  const admin = typeof sourceConfig.adminEmail === 'string' ? sourceConfig.adminEmail : ''
-  const domain = admin.trim().toLowerCase().split('@')[1]
+  const domain = googleWorkspaceDomain(sourceConfig.adminEmail)
   if (!domain) return null
   const openSharing = sourceConfig.openSharing
   return {
@@ -646,7 +644,7 @@ export const googleDriveConnector: ConnectorConfig = {
   },
 
   openDirectory: async (accessToken, sourceConfig) =>
-    openGoogleDirectory(accessToken, sourceConfig.adminEmail as string | undefined),
+    openGoogleDirectory(accessToken, sourceConfig.adminEmail),
 
   getDocument: async (
     accessToken: string,

apps/sim/lib/knowledge/access/external-groups.ts

@@ -23,13 +23,3 @@ export const EXTERNAL_GROUP_STALE_AFTER_MS = 24 * 60 * 60 * 1000
  * thirty for the heavier APIs.
  */
 export const EXTERNAL_GROUP_SYNC_INTERVAL_MS = 5 * 60 * 1000
-
-/**
- * How deep nested groups are followed when flattening membership.
- *
- * A directory can nest groups arbitrarily and can contain cycles, so the walk
- * needs both a visited set and a depth bound. Onyx does not recurse at all,
- * which silently drops everyone who is a member only through a subgroup; a
- * bounded walk covers every real directory while still terminating.
- */
-export const MAX_GROUP_NESTING_DEPTH = 10

apps/sim/lib/knowledge/access/scope.ts

@@ -76,6 +76,12 @@ const emailHeldByAnotherAccount = sql<boolean>`EXISTS (
  * is survivable, an abandoned sync is not.
  */
 async function loadExternalGroupTokens(email: string, workspaceId: string): Promise<string[]> {
+  /**
+   * A query of its own rather than a fourth join on the credential query above:
+   * that one already fans out per managed credential, and joining groups onto
+   * it would multiply the two — every credential row repeated for every group.
+   * Two indexed reads cost less than one cross product.
+   */
   const freshEnough = new Date(Date.now() - EXTERNAL_GROUP_STALE_AFTER_MS)
   const rows = await db
     .select({
@@ -114,12 +120,13 @@ export interface KnowledgeAccessScopeContext {
 }
 
 /**
- * The tokens a person holds in a workspace: the workspace pair plus one `s:`
- * token per active managed credential bound to them through a credential-group
- * enrollment. The person must be email-verified — the enrollment binding is by
+ * The tokens a person holds in a workspace: the workspace pair, one `s:` token
+ * per active managed credential bound to them through a credential-group
+ * enrollment, their own `u:` address, and a `g:` token per directory group it
+ * belongs to. The person must be email-verified — every binding here is by
  * email, and an unverified address must not inherit grants made to whoever
- * really owns it. Nothing here is cached: revoking or suspending a credential
- * is visible on the next read.
+ * really owns it. Nothing here is cached: revoking a credential or leaving a
+ * group is visible on the next read.
  */
 async function loadUserAccessTokens(
   userId: string,

apps/sim/lib/knowledge/access/tokens.ts

@@ -112,22 +112,31 @@ export interface GroupIdentity {
   /** The provider's tenant, or {@link NO_TENANT_SEGMENT} where it reports none. */
   tenantId: string | null
   /**
-   * The group as the provider names it, in whichever identifier both the
-   * crawl and the directory sync can see: Drive and Confluence both hand back
-   * an email or a name, never an opaque id.
+   * The group in whichever identifier both the crawl and the directory sync
+   * can see — a group email on Drive, a group id on Confluence. Whatever the
+   * source's permissions API returns is what the directory is keyed by, so no
+   * lookup ever stands between a grant and the membership that resolves it.
    */
   groupId: string
 }
 
 /**
- * The token of a group grant. Case-folded like {@link userToken}, since group
- * identifiers are emails on Drive and names on Confluence, and neither source
- * is consistent about case.
+ * A group identifier in the one form every writer and reader agrees on.
+ *
+ * Both the crawl that writes a `g:` token and the directory sync that stores
+ * the group's membership pass through this, so the two can never disagree about
+ * case or whitespace — Drive spells a group email however it was typed, and a
+ * grant that folds differently from its membership row grants nobody.
  */
+export function canonicalGroupId(groupId: string): string {
+  return groupId.trim().toLowerCase()
+}
+
+/** The token of a group grant. */
 export function groupToken(group: GroupIdentity): string | null {
   const { providerId } = group
   const tenant = group.tenantId || NO_TENANT_SEGMENT
-  const groupId = group.groupId?.trim().toLowerCase()
+  const groupId = canonicalGroupId(group.groupId ?? '')
   if (!providerId || !groupId) return null
   if (providerId.includes(':') || tenant.includes(':')) return null
   const token = `g:${providerId}:${tenant}:${groupId}`