Skip to content

Commit 6f204a0

Browse files
committed
refactor(knowledge): tighten administrator access after the mechanics review
- directory refresh idempotency keys on the sync interval, so a cron wrapper retry cannot start a second walk of the same directory; the scheduler offers every admin connector and lets the tenant-level freshness check dedupe, since a connector type does not imply one tenant - config validation seeds the run context from the token, so a Confluence service account validates without a discovery call it cannot make - the domain-share vocabulary (group id, wildcard member, domain fold) lives in one module; the Google directory drains pages through the shared Google pagination helper; Confluence's getJson owns the not-found branch - access-mode predicates take the union, and the engine narrows the locked row once; runnable statuses are one constant everywhere - a knowledge base with a mirrored connector reports hasPermissionScopedConnector - dead export, unused import, and stale docblocks removed
1 parent 2c86087 commit 6f204a0

32 files changed

Lines changed: 231 additions & 203 deletions

File tree

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

Lines changed: 14 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,9 @@ vi.mock('@sim/db', () => ({
1818
db: {
1919
select: () => ({
2020
from: () => ({
21-
innerJoin: () => ({ where: () => ({ orderBy: () => mockConnectorRows() }) }),
21+
innerJoin: () => ({
22+
where: () => ({ orderBy: () => ({ limit: () => mockConnectorRows() }) }),
23+
}),
2224
}),
2325
}),
2426
},
@@ -27,12 +29,7 @@ vi.mock('@sim/db', () => ({
2729
import { GET } from '@/app/api/knowledge/connectors/directory-sync/route'
2830

2931
function connector(overrides: Record<string, unknown> = {}) {
30-
return {
31-
id: 'connector-1',
32-
connectorType: 'google_drive',
33-
workspaceId: 'ws-1',
34-
...overrides,
35-
}
32+
return { id: 'connector-1', ...overrides }
3633
}
3734

3835
async function run() {
@@ -47,50 +44,27 @@ describe('connector directory sync scheduler', () => {
4744
mockDispatch.mockResolvedValue(undefined)
4845
})
4946

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)
63-
})
64-
6547
/**
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.
48+
* Every eligible connector is offered under one tick time; the tenant-level
49+
* freshness check in the refresh, not the scheduler, decides which walk.
6850
*/
69-
it('dispatches once for connectors that share a directory', async () => {
51+
it('dispatches a refresh for every admin-mode connector under the same tick', async () => {
7052
mockConnectorRows.mockResolvedValue([connector(), connector({ id: 'connector-2' })])
7153

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())
54+
await expect(run()).resolves.toMatchObject({ considered: 2, dispatched: 2, failed: 0 })
55+
expect(mockDispatch).toHaveBeenCalledTimes(2)
56+
const [, first] = mockDispatch.mock.calls[0]
57+
const [, second] = mockDispatch.mock.calls[1]
58+
expect(first.tickAt).toBe(second.tickAt)
7559
})
7660

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-
])
61+
it('contains a dispatch failure to the connector that caused it', async () => {
62+
mockConnectorRows.mockResolvedValue([connector(), connector({ id: 'connector-2' })])
8263
mockDispatch.mockRejectedValueOnce(new Error('queue unreachable'))
8364

8465
await expect(run()).resolves.toMatchObject({ dispatched: 1, failed: 1 })
8566
})
8667

87-
it('skips a connector whose knowledge base has no workspace', async () => {
88-
mockConnectorRows.mockResolvedValue([connector({ workspaceId: null })])
89-
90-
await expect(run()).resolves.toMatchObject({ directories: 0, dispatched: 0 })
91-
expect(mockDispatch).not.toHaveBeenCalled()
92-
})
93-
9468
it('refuses an unauthenticated tick', async () => {
9569
mockVerifyCronAuth.mockReturnValue(new Response('nope', { status: 401 }))
9670

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

Lines changed: 15 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ 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, asc, eq, inArray, isNull } from 'drizzle-orm'
5+
import { and, asc, eq, inArray, isNotNull, isNull } from 'drizzle-orm'
66
import type { NextRequest } from 'next/server'
77
import { verifyCronAuth } from '@/lib/auth/internal'
88
import { generateRequestId } from '@/lib/core/utils/request'
@@ -14,7 +14,7 @@ export const dynamic = 'force-dynamic'
1414

1515
const logger = createLogger('ConnectorDirectorySyncSchedulerAPI')
1616

17-
/** Directories dispatched per tick. */
17+
/** Connectors offered per tick. */
1818
const MAX_DIRECTORIES_PER_TICK = 200
1919

2020
/**
@@ -26,11 +26,13 @@ const MAX_DIRECTORIES_PER_TICK = 200
2626
* admin crawl refreshes the directory too — so a crawl can never publish grants
2727
* against membership nobody has read — but that is a floor, not the cadence.
2828
*
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.
29+
* Every eligible connector is offered each tick, and
30+
* `syncExternalDirectoryGroups` decides whether its directory is actually due:
31+
* a tenant is the credential's own site or domain, which the row does not
32+
* carry, so connectors sharing one cost a refresh and a skip rather than a
33+
* refresh each. The walk itself runs in the background, like every other
34+
* connector job, because a large domain takes longer than a scheduler request
35+
* lives.
3436
*/
3537
export const GET = withRouteHandler(async (request: NextRequest) => {
3638
const requestId = generateRequestId()
@@ -41,11 +43,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
4143
if (authError) return authError
4244

4345
const connectors = await db
44-
.select({
45-
id: knowledgeConnector.id,
46-
connectorType: knowledgeConnector.connectorType,
47-
workspaceId: knowledgeBase.workspaceId,
48-
})
46+
.select({ id: knowledgeConnector.id })
4947
.from(knowledgeConnector)
5048
.innerJoin(knowledgeBase, eq(knowledgeConnector.knowledgeBaseId, knowledgeBase.id))
5149
.where(
@@ -54,27 +52,16 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
5452
inArray(knowledgeConnector.status, RUNNABLE_CONNECTOR_STATUSES),
5553
isNull(knowledgeConnector.archivedAt),
5654
isNull(knowledgeConnector.deletedAt),
57-
isNull(knowledgeBase.deletedAt)
55+
isNull(knowledgeBase.deletedAt),
56+
isNotNull(knowledgeBase.workspaceId)
5857
)
5958
)
6059
.orderBy(asc(knowledgeConnector.createdAt))
61-
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)
60+
.limit(MAX_DIRECTORIES_PER_TICK)
7461

7562
let dispatched = 0
7663
let failed = 0
77-
for (const connectorId of due) {
64+
for (const { id: connectorId } of connectors) {
7865
try {
7966
await dispatchDirectorySync(connectorId, { requestId, tickAt })
8067
dispatched += 1
@@ -87,7 +74,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
8774
}
8875
}
8976

90-
const summary = { considered: connectors.length, directories: due.length, dispatched, failed }
77+
const summary = { considered: connectors.length, dispatched, failed }
9178
logger.info(`[${requestId}] Connector directory sync scheduler finished`, summary)
9279
return Response.json({ success: true, ...summary })
9380
})

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
MAX_CONSECUTIVE_FAILURES,
2121
MEMBER_SYNC_STALE_LOCK_TTL_MS,
2222
} from '@/lib/knowledge/connectors/sync-limits'
23+
import { RUNNABLE_CONNECTOR_STATUSES } from '@/lib/knowledge/connectors/sync-lock'
2324

2425
export const dynamic = 'force-dynamic'
2526

@@ -168,7 +169,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
168169
.where(
169170
and(
170171
eq(knowledgeConnector.accessMode, 'members'),
171-
inArray(knowledgeConnector.status, ['active', 'error']),
172+
inArray(knowledgeConnector.status, RUNNABLE_CONNECTOR_STATUSES),
172173
inArray(knowledgeConnector.memberSyncStatus, QUEUEABLE_MEMBER_SYNC_STATUSES),
173174
lte(knowledgeConnector.nextMemberSyncAt, now),
174175
isNull(knowledgeConnector.archivedAt),

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
CONNECTOR_SYNC_STALE_LOCK_TTL_MS,
1818
MAX_CONSECUTIVE_FAILURES,
1919
} from '@/lib/knowledge/connectors/sync-limits'
20+
import { RUNNABLE_CONNECTOR_STATUSES } from '@/lib/knowledge/connectors/sync-lock'
2021

2122
export const dynamic = 'force-dynamic'
2223

@@ -304,7 +305,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
304305
.innerJoin(knowledgeBase, eq(knowledgeConnector.knowledgeBaseId, knowledgeBase.id))
305306
.where(
306307
and(
307-
inArray(knowledgeConnector.status, ['active', 'error']),
308+
inArray(knowledgeConnector.status, RUNNABLE_CONNECTOR_STATUSES),
308309
inArray(knowledgeConnector.accessMode, [...CONTENT_ENGINE_ACCESS_MODES]),
309310
lte(knowledgeConnector.nextSyncAt, now),
310311
isNull(knowledgeConnector.archivedAt),

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

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ interface ConnectorAccessFieldProps {
2424
onChange: (value: ConnectorAccessSelection) => void
2525
/** From `useConnectorMemberGroupOptions`; shared with the modal so both agree on what is required. */
2626
groupOptions: ConnectorMemberGroupOptions
27-
/** Only an admin may put a connector into members mode. */
27+
/** Only an admin may move a connector out of workspace mode. */
2828
canAdmin: boolean
2929
disabled?: boolean
3030
/** Whether per-member access may be chosen; false leaves only the way back to workspace access. */
@@ -60,12 +60,15 @@ function accessHint(input: {
6060
}
6161

6262
/**
63-
* The Access section of a connector's settings: sync as the workspace, or
64-
* crawl once per member so each person sees only what the source lets them
65-
* read. Per-member access needs nothing from the admin: a Credential Group is
66-
* found or created for the connector's provider, everyone in the workspace is
63+
* The Access section of a connector's settings: sync as the workspace; crawl
64+
* once per member so each person sees only what the source lets them read; or
65+
* crawl once as an administrator and mirror each document's own permissions.
66+
* Per-member access needs nothing from the admin: a Credential Group is found
67+
* or created for the connector's provider, everyone in the workspace is
6768
* invited, and each person connects their own account. Only a workspace with
68-
* several matching groups is asked which one to use.
69+
* several matching groups is asked which one to use. Administrator access
70+
* needs a connector that can read the source's permissions, and the
71+
* administrator it crawls as is part of the connector's own config.
6972
*/
7073
export function ConnectorAccessField({
7174
connectorConfig,

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,8 @@ import {
2020
import { RefreshCw, SquareArrowUpRight } from '@sim/emcn/icons'
2121
import { createLogger } from '@sim/logger'
2222
import { useParams } from 'next/navigation'
23-
import {
24-
type ConnectorAccessMode,
25-
isCredentialBackedAccessMode,
26-
} from '@/lib/knowledge/connectors/access-modes'
23+
import type { ConnectorAccessMode } from '@/lib/api/contracts/knowledge/connectors'
24+
import { isCredentialBackedAccessMode } from '@/lib/knowledge/connectors/access-modes'
2725
import { getProviderIdFromServiceId, type OAuthProvider } from '@/lib/oauth'
2826
import {
2927
ConnectorAccessField,

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/knowledge-base-selector/knowledge-base-selector.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,9 @@ export function KnowledgeBaseSelector({
184184
const label =
185185
subBlock.placeholder || (isMultiSelect ? 'Select knowledge bases' : 'Select knowledge base')
186186

187-
const hasMemberScopedSelection = selectedKnowledgeBases.some((kb) => kb.hasMemberScopedConnector)
187+
const hasMemberScopedSelection = selectedKnowledgeBases.some(
188+
(kb) => kb.hasPermissionScopedConnector
189+
)
188190

189191
return (
190192
<div className='w-full'>

apps/sim/connectors/confluence/confluence.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -679,7 +679,8 @@ export const confluenceConnector: ConnectorConfig = {
679679

680680
validateConfig: async (
681681
accessToken: string,
682-
sourceConfig: Record<string, unknown>
682+
sourceConfig: Record<string, unknown>,
683+
syncContext?: Record<string, unknown>
683684
): Promise<{ valid: boolean; error?: string }> => {
684685
const domain = sourceConfig.domain as string
685686
const spaceKeys = parseMultiValue(sourceConfig.spaceKey)
@@ -694,7 +695,11 @@ export const confluenceConnector: ConnectorConfig = {
694695
}
695696

696697
try {
697-
const cloudId = await getConfluenceCloudId(domain, accessToken, VALIDATE_RETRY_OPTIONS)
698+
const seededCloudId = syncContext?.cloudId
699+
const cloudId =
700+
typeof seededCloudId === 'string' && seededCloudId
701+
? seededCloudId
702+
: await getConfluenceCloudId(domain, accessToken, VALIDATE_RETRY_OPTIONS)
698703
const params = new URLSearchParams()
699704
for (const key of spaceKeys) params.append('keys', key)
700705
params.append('limit', String(Math.max(spaceKeys.length, 1)))

apps/sim/connectors/confluence/permissions.ts

Lines changed: 23 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,26 @@ function apiBase(cloudId: string): string {
2222
return `https://api.atlassian.com/ex/confluence/${cloudId}/wiki`
2323
}
2424

25-
/** A GET with the same transient-error retry every other Confluence call gets. */
26-
async function getJson<T>(url: string, accessToken: string): Promise<T> {
25+
/**
26+
* A GET with the same transient-error retry every other Confluence call gets.
27+
* With `allowNotFound`, a 404 resolves to null instead of throwing.
28+
*/
29+
async function getJson<T>(url: string, accessToken: string): Promise<T>
30+
async function getJson<T>(
31+
url: string,
32+
accessToken: string,
33+
options: { allowNotFound: true }
34+
): Promise<T | null>
35+
async function getJson<T>(
36+
url: string,
37+
accessToken: string,
38+
options?: { allowNotFound: true }
39+
): Promise<T | null> {
2740
const response = await fetchWithRetry(url, {
2841
method: 'GET',
2942
headers: { Authorization: `Bearer ${accessToken}`, Accept: 'application/json' },
3043
})
44+
if (response.status === 404 && options?.allowNotFound) return null
3145
if (!response.ok) {
3246
throw new Error(`Confluence request failed: ${response.status} ${response.statusText}`)
3347
}
@@ -79,7 +93,7 @@ async function drainV1<T>(url: string, accessToken: string, what: string): Promi
7993
const results = body.results ?? []
8094
items.push(...results)
8195
if (!body._links?.next || results.length === 0) return items
82-
start += body.size ?? results.length
96+
start += body.size || results.length
8397
}
8498
throw new Error(`Confluence ${what} exceeded ${MAX_PAGES} pages`)
8599
}
@@ -249,19 +263,12 @@ export async function describeContent(
249263
): Promise<{ spaceId: string; contentType: 'page' | 'blogpost' } | null> {
250264
for (const contentType of ['page', 'blogpost'] as const) {
251265
const collection = contentType === 'page' ? 'pages' : 'blogposts'
252-
const response = await fetchWithRetry(
266+
const body = await getJson<{ spaceId?: string | number }>(
253267
`${apiBase(cloudId)}/api/v2/${collection}/${encodeURIComponent(contentId)}`,
254-
{
255-
method: 'GET',
256-
headers: { Authorization: `Bearer ${accessToken}`, Accept: 'application/json' },
257-
}
268+
accessToken,
269+
{ allowNotFound: true }
258270
)
259-
if (response.status === 404) continue
260-
if (!response.ok) {
261-
throw new Error(`Confluence request failed: ${response.status} ${response.statusText}`)
262-
}
263-
const body = (await response.json()) as { spaceId?: string | number }
264-
if (body.spaceId !== undefined) return { spaceId: String(body.spaceId), contentType }
271+
if (body?.spaceId !== undefined) return { spaceId: String(body.spaceId), contentType }
265272
}
266273
return null
267274
}
@@ -322,7 +329,7 @@ export async function resolveUserEmails(
322329
}
323330

324331
/** Every group on the site, by the id its permissions and restrictions name. */
325-
export async function listSiteGroups(
332+
async function listSiteGroups(
326333
cloudId: string,
327334
accessToken: string
328335
): Promise<ConnectorDirectoryGroup[]> {
@@ -360,7 +367,7 @@ export async function listGroupMemberEmails(
360367
accessToken,
361368
'group membership'
362369
)
363-
const accountIds = [...new Set(members.map((m) => m.accountId).filter(Boolean) as string[])]
370+
const accountIds = [...new Set(members.flatMap((m) => (m.accountId ? [m.accountId] : [])))]
364371
const emails = await resolveUserEmails(cloudId, accessToken, accountIds)
365372
return {
366373
group,

0 commit comments

Comments
 (0)