Skip to content

Commit 2c86087

Browse files
committed
fix(knowledge): close the audit findings on administrator access
Admin mode was unreachable end to end: the queue still refused every mode but workspace, and neither modal ever sent the mode, so editing an admin connector saved it as workspace and published the corpus. A switch into admin mode also flipped before hiding, showing workspace documents under a mode whose reader expects source ACLs. - queue: dispatch every content-engine mode; the modals send the chosen mode; the access field offers administrator mode on availability, not on credential-group support - switch: hide before flipping into a mode that hides on entry; the engine finishes a pending hide before it lists, strips listing caps, and hides owned documents the listing did not name - Drive: domain shares resolve through a synthetic per-domain group whose one member is a wildcard the reader matches by their own domain; CUSTOMER members and every customer domain are covered - Confluence: each page falls back to its own space's readers, not the union across spaces; ancestors from the v2 collection the API still serves; space-role assignments expanded; pagination follows next links; per-page containment; retry on every call - directory sync: one background job per directory instead of a full walk inside the scheduler request; runnable statuses only; freshness keyed off the latest confirmed group so one unreadable group cannot keep a directory due forever - config edits on an admin connector re-assert the administrator subject; a connector whose ACL is derived cannot leave its documents behind - the access-mode enum lives in the leaf module and the contract derives from it; the mirror assertion moves to a leaf; the remaining ad-hoc email folds use foldedEmail; the impersonated subject leaves the logs
1 parent cc92806 commit 2c86087

51 files changed

Lines changed: 1675 additions & 545 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

apps/sim/app/api/knowledge/connectors/directory-sync/route.test.ts

Lines changed: 36 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -4,41 +4,21 @@
44
import { createMockRequest } from '@sim/testing'
55
import { beforeEach, describe, expect, it, vi } from 'vitest'
66

7-
const {
8-
mockVerifyCronAuth,
9-
mockConnectorRows,
10-
mockResolveIdentity,
11-
mockResolveToken,
12-
mockRefreshDirectory,
13-
} = vi.hoisted(() => ({
7+
const { mockVerifyCronAuth, mockConnectorRows, mockDispatch } = vi.hoisted(() => ({
148
mockVerifyCronAuth: vi.fn(() => null),
159
mockConnectorRows: vi.fn(),
16-
mockResolveIdentity: vi.fn(),
17-
mockResolveToken: vi.fn(),
18-
mockRefreshDirectory: vi.fn(),
10+
mockDispatch: vi.fn(),
1911
}))
2012

2113
vi.mock('@/lib/auth/internal', () => ({ verifyCronAuth: mockVerifyCronAuth }))
22-
vi.mock('@/lib/credentials/access', () => ({
23-
resolveCredentialTokenIdentity: mockResolveIdentity,
24-
}))
25-
vi.mock('@/lib/knowledge/connectors/access-token', () => ({
26-
resolveConnectorAccessToken: mockResolveToken,
27-
}))
28-
vi.mock('@/lib/knowledge/connectors/external-group-sync', () => ({
29-
refreshMirroredDirectory: mockRefreshDirectory,
30-
}))
31-
vi.mock('@/connectors/registry.server', () => ({
32-
CONNECTOR_REGISTRY: {
33-
google_drive: { auth: { mode: 'oauth', provider: 'google-drive' }, openDirectory: vi.fn() },
34-
notion: { auth: { mode: 'oauth', provider: 'notion' } },
35-
},
14+
vi.mock('@/lib/knowledge/connectors/directory-queue', () => ({
15+
dispatchDirectorySync: mockDispatch,
3616
}))
3717
vi.mock('@sim/db', () => ({
3818
db: {
3919
select: () => ({
4020
from: () => ({
41-
innerJoin: () => ({ where: () => ({ limit: () => mockConnectorRows() }) }),
21+
innerJoin: () => ({ where: () => ({ orderBy: () => mockConnectorRows() }) }),
4222
}),
4323
}),
4424
},
@@ -50,11 +30,7 @@ function connector(overrides: Record<string, unknown> = {}) {
5030
return {
5131
id: 'connector-1',
5232
connectorType: 'google_drive',
53-
credentialId: 'credential-1',
54-
encryptedApiKey: null,
55-
sourceConfig: { adminEmail: 'admin@corp.com' },
5633
workspaceId: 'ws-1',
57-
knowledgeBaseOwnerId: 'owner-1',
5834
...overrides,
5935
}
6036
}
@@ -68,57 +44,51 @@ describe('connector directory sync scheduler', () => {
6844
beforeEach(() => {
6945
vi.clearAllMocks()
7046
mockVerifyCronAuth.mockReturnValue(null)
71-
mockResolveIdentity.mockResolvedValue({ kind: 'service_account' })
72-
mockResolveToken.mockResolvedValue({ accessToken: 'token' })
73-
mockRefreshDirectory.mockResolvedValue(undefined)
47+
mockDispatch.mockResolvedValue(undefined)
7448
})
7549

76-
it('refreshes the directory of every admin-mode connector it finds', async () => {
77-
mockConnectorRows.mockResolvedValue([connector(), connector({ id: 'connector-2' })])
78-
79-
await expect(run()).resolves.toMatchObject({ considered: 2, refreshed: 2, failed: 0 })
80-
expect(mockRefreshDirectory).toHaveBeenCalledTimes(2)
50+
it('dispatches one refresh per directory an admin-mode connector mirrors', async () => {
51+
mockConnectorRows.mockResolvedValue([
52+
connector(),
53+
connector({ id: 'connector-2', workspaceId: 'ws-2' }),
54+
])
55+
56+
await expect(run()).resolves.toMatchObject({
57+
considered: 2,
58+
directories: 2,
59+
dispatched: 2,
60+
failed: 0,
61+
})
62+
expect(mockDispatch).toHaveBeenCalledTimes(2)
8163
})
8264

8365
/**
84-
* The tick refreshes every workspace's directory, so one workspace's lapsed
85-
* credential or unreachable source must not stop the others.
66+
* Two connectors of one type in one workspace mirror one directory; walking
67+
* it twice per tick would double the Admin SDK cost for nothing.
8668
*/
87-
it('contains a failure to the connector that caused it', async () => {
69+
it('dispatches once for connectors that share a directory', async () => {
8870
mockConnectorRows.mockResolvedValue([connector(), connector({ id: 'connector-2' })])
89-
mockRefreshDirectory.mockRejectedValueOnce(new Error('directory unreachable'))
9071

91-
await expect(run()).resolves.toMatchObject({ considered: 2, refreshed: 1, failed: 1 })
72+
await expect(run()).resolves.toMatchObject({ considered: 2, directories: 1, dispatched: 1 })
73+
expect(mockDispatch).toHaveBeenCalledTimes(1)
74+
expect(mockDispatch).toHaveBeenCalledWith('connector-1', expect.anything())
9275
})
9376

94-
it('reports a connector whose credential no longer resolves rather than failing', async () => {
95-
mockConnectorRows.mockResolvedValue([connector()])
96-
mockResolveToken.mockResolvedValue(null)
77+
it('contains a dispatch failure to the directory that caused it', async () => {
78+
mockConnectorRows.mockResolvedValue([
79+
connector(),
80+
connector({ id: 'connector-2', workspaceId: 'ws-2' }),
81+
])
82+
mockDispatch.mockRejectedValueOnce(new Error('queue unreachable'))
9783

98-
await expect(run()).resolves.toMatchObject({ unusable: 1, refreshed: 0 })
99-
expect(mockRefreshDirectory).not.toHaveBeenCalled()
84+
await expect(run()).resolves.toMatchObject({ dispatched: 1, failed: 1 })
10085
})
10186

102-
it('skips a connector whose source has no directory to read', async () => {
103-
mockConnectorRows.mockResolvedValue([connector({ connectorType: 'notion' })])
104-
105-
await expect(run()).resolves.toMatchObject({ skipped: 1, refreshed: 0 })
106-
expect(mockRefreshDirectory).not.toHaveBeenCalled()
107-
})
108-
109-
/**
110-
* Token reads are scoped to the credential's own account owner, not the
111-
* knowledge base owner, who is routinely a different member.
112-
*/
113-
it('resolves the token as the credential owner for an OAuth credential', async () => {
114-
mockConnectorRows.mockResolvedValue([connector()])
115-
mockResolveIdentity.mockResolvedValue({ kind: 'oauth', userId: 'credential-owner' })
116-
117-
await run()
87+
it('skips a connector whose knowledge base has no workspace', async () => {
88+
mockConnectorRows.mockResolvedValue([connector({ workspaceId: null })])
11889

119-
expect(mockResolveToken).toHaveBeenCalledWith(
120-
expect.objectContaining({ userId: 'credential-owner' })
121-
)
90+
await expect(run()).resolves.toMatchObject({ directories: 0, dispatched: 0 })
91+
expect(mockDispatch).not.toHaveBeenCalled()
12292
})
12393

12494
it('refuses an unauthenticated tick', async () => {

apps/sim/app/api/knowledge/connectors/directory-sync/route.ts

Lines changed: 35 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -2,24 +2,20 @@ import { db } from '@sim/db'
22
import { knowledgeBase, knowledgeConnector } from '@sim/db/schema'
33
import { createLogger } from '@sim/logger'
44
import { getErrorMessage } from '@sim/utils/errors'
5-
import { and, eq, isNull } from 'drizzle-orm'
5+
import { and, asc, eq, inArray, isNull } from 'drizzle-orm'
66
import type { NextRequest } from 'next/server'
77
import { verifyCronAuth } from '@/lib/auth/internal'
8-
import { mapWithConcurrency } from '@/lib/core/utils/concurrency'
98
import { generateRequestId } from '@/lib/core/utils/request'
109
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
11-
import { resolveCredentialTokenIdentity } from '@/lib/credentials/access'
12-
import { resolveConnectorAccessToken } from '@/lib/knowledge/connectors/access-token'
13-
import { refreshMirroredDirectory } from '@/lib/knowledge/connectors/external-group-sync'
14-
import { CONNECTOR_REGISTRY } from '@/connectors/registry.server'
10+
import { dispatchDirectorySync } from '@/lib/knowledge/connectors/directory-queue'
11+
import { RUNNABLE_CONNECTOR_STATUSES } from '@/lib/knowledge/connectors/sync-lock'
1512

1613
export const dynamic = 'force-dynamic'
1714

1815
const logger = createLogger('ConnectorDirectorySyncSchedulerAPI')
1916

20-
/** Directories refreshed per tick, and how many at once. */
17+
/** Directories dispatched per tick. */
2118
const MAX_DIRECTORIES_PER_TICK = 200
22-
const REFRESH_CONCURRENCY = 4
2319

2420
/**
2521
* Refreshes the external directories that admin-mode connectors mirror.
@@ -30,15 +26,15 @@ const REFRESH_CONCURRENCY = 4
3026
* admin crawl refreshes the directory too — so a crawl can never publish grants
3127
* against membership nobody has read — but that is a floor, not the cadence.
3228
*
33-
* Every eligible connector is offered each tick; `syncExternalDirectoryGroups`
34-
* decides whether its directory is actually due. Connectors sharing a directory
35-
* therefore cost one refresh between them: the first brings it up to date and
36-
* the rest skip. Two ticks overlapping on one directory would both enumerate
37-
* and write the same rows — wasteful, never wrong, and not worth a lease to
38-
* prevent, since every write here is idempotent.
29+
* Connectors sharing a directory cost one refresh between them: the tick
30+
* dispatches one connector per workspace and provider, and
31+
* `syncExternalDirectoryGroups` decides whether that directory is actually
32+
* due. The walk itself runs in the background, like every other connector
33+
* job, because a large domain takes longer than a scheduler request lives.
3934
*/
4035
export const GET = withRouteHandler(async (request: NextRequest) => {
4136
const requestId = generateRequestId()
37+
const tickAt = new Date()
4238
logger.info(`[${requestId}] Connector directory sync scheduler triggered`)
4339

4440
const authError = verifyCronAuth(request, 'Connector directory sync scheduler')
@@ -48,83 +44,50 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
4844
.select({
4945
id: knowledgeConnector.id,
5046
connectorType: knowledgeConnector.connectorType,
51-
credentialId: knowledgeConnector.credentialId,
52-
encryptedApiKey: knowledgeConnector.encryptedApiKey,
53-
sourceConfig: knowledgeConnector.sourceConfig,
5447
workspaceId: knowledgeBase.workspaceId,
55-
knowledgeBaseOwnerId: knowledgeBase.userId,
5648
})
5749
.from(knowledgeConnector)
5850
.innerJoin(knowledgeBase, eq(knowledgeConnector.knowledgeBaseId, knowledgeBase.id))
5951
.where(
6052
and(
6153
eq(knowledgeConnector.accessMode, 'admin'),
54+
inArray(knowledgeConnector.status, RUNNABLE_CONNECTOR_STATUSES),
6255
isNull(knowledgeConnector.archivedAt),
6356
isNull(knowledgeConnector.deletedAt),
6457
isNull(knowledgeBase.deletedAt)
6558
)
6659
)
67-
.limit(MAX_DIRECTORIES_PER_TICK)
60+
.orderBy(asc(knowledgeConnector.createdAt))
6861

69-
const outcomes = await mapWithConcurrency(connectors, REFRESH_CONCURRENCY, async (connector) => {
70-
/**
71-
* Every failure is contained here. One workspace whose credential lapsed
72-
* must not stop the tick refreshing every other workspace's directory.
73-
*/
74-
try {
75-
if (!connector.workspaceId) return 'skipped'
76-
const connectorConfig = CONNECTOR_REGISTRY[connector.connectorType]
77-
if (!connectorConfig?.openDirectory) return 'skipped'
78-
79-
/**
80-
* The credential's own account owner, not the knowledge base owner —
81-
* token reads are scoped to `account.userId`, and a service account
82-
* ignores the argument entirely.
83-
*/
84-
let credentialUserId = connector.knowledgeBaseOwnerId
85-
if (connector.credentialId) {
86-
const identity = await resolveCredentialTokenIdentity(
87-
connector.credentialId,
88-
connector.workspaceId
89-
)
90-
if (!identity) return 'unusable'
91-
if (identity.kind === 'oauth') credentialUserId = identity.userId
92-
}
93-
94-
const sourceConfig = connector.sourceConfig as Record<string, unknown>
95-
const token = await resolveConnectorAccessToken({
96-
auth: connectorConfig.auth,
97-
connector,
98-
userId: credentialUserId,
99-
requestId,
100-
sourceConfig,
101-
})
102-
if (!token) return 'unusable'
62+
/**
63+
* One connector per directory. A connector type implies its provider, and
64+
* two connectors of one type in one workspace share a directory by
65+
* construction — the first to be created stands for it.
66+
*/
67+
const representatives = new Map<string, string>()
68+
for (const connector of connectors) {
69+
if (!connector.workspaceId) continue
70+
const key = `${connector.workspaceId}:${connector.connectorType}`
71+
if (!representatives.has(key)) representatives.set(key, connector.id)
72+
}
73+
const due = [...representatives.values()].slice(0, MAX_DIRECTORIES_PER_TICK)
10374

104-
await refreshMirroredDirectory({
105-
workspaceId: connector.workspaceId,
106-
connectorConfig,
107-
sourceConfig,
108-
syncContext: {},
109-
accessToken: token.accessToken,
110-
})
111-
return 'refreshed'
75+
let dispatched = 0
76+
let failed = 0
77+
for (const connectorId of due) {
78+
try {
79+
await dispatchDirectorySync(connectorId, { requestId, tickAt })
80+
dispatched += 1
11281
} catch (error) {
113-
logger.error(`[${requestId}] Directory refresh failed for a connector`, {
114-
connectorId: connector.id,
82+
failed += 1
83+
logger.error(`[${requestId}] Failed to dispatch a directory refresh`, {
84+
connectorId,
11585
error: getErrorMessage(error),
11686
})
117-
return 'failed'
11887
}
119-
})
120-
121-
const summary = {
122-
considered: connectors.length,
123-
refreshed: outcomes.filter((outcome) => outcome === 'refreshed').length,
124-
skipped: outcomes.filter((outcome) => outcome === 'skipped').length,
125-
unusable: outcomes.filter((outcome) => outcome === 'unusable').length,
126-
failed: outcomes.filter((outcome) => outcome === 'failed').length,
12788
}
89+
90+
const summary = { considered: connectors.length, directories: due.length, dispatched, failed }
12891
logger.info(`[${requestId}] Connector directory sync scheduler finished`, summary)
12992
return Response.json({ success: true, ...summary })
13093
})

apps/sim/app/workspace/[workspaceId]/knowledge/[id]/components/add-connector-modal/add-connector-modal.tsx

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ export function AddConnectorModal({
9595
const { ownerBilling, features } = useWorkspaceHostContext()
9696
const { canAdmin } = useUserPermissionsContext()
9797
const memberAccessAvailable = features?.knowledgeMemberAccess === true
98+
const mirroredAccessAvailable = features?.knowledgeSourceMirroredAccess === true
9899
const { mutate: createConnector, isPending: isCreating } = useCreateConnector()
99100

100101
const hasMaxAccess = hasWorkspaceMaxConnectorAccess(ownerBilling)
@@ -247,7 +248,7 @@ export function AddConnectorModal({
247248
credentialGroupId: access.credentialGroupId,
248249
credentialGroupOptionId: access.credentialGroupOptionId,
249250
}
250-
: { credentialId: effectiveCredentialId! }),
251+
: { accessMode: access.accessMode, credentialId: effectiveCredentialId! }),
251252
sourceConfig: finalSourceConfig,
252253
syncIntervalMinutes: syncInterval,
253254
},
@@ -333,13 +334,15 @@ export function AddConnectorModal({
333334
</div>
334335
) : connectorConfig ? (
335336
<>
336-
{!isApiKeyMode && memberAccessAvailable && (
337+
{!isApiKeyMode && (memberAccessAvailable || mirroredAccessAvailable) && (
337338
<ConnectorAccessField
338339
connectorConfig={connectorConfig}
339340
value={access}
340341
onChange={setAccess}
341342
groupOptions={groupOptions}
342343
canAdmin={canAdmin}
344+
allowMembers={memberAccessAvailable}
345+
allowAdmin={mirroredAccessAvailable}
343346
disabled={isCreating}
344347
/>
345348
)}

0 commit comments

Comments
 (0)