Skip to content

Commit 70f57cb

Browse files
committed
fix(atlassian): reject mixed All scopes and run permission tests in CI
1 parent 6b56f14 commit 70f57cb

9 files changed

Lines changed: 189 additions & 8 deletions

File tree

‎.github/workflows/test-build.yml‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,19 @@ jobs:
240240
lib/knowledge/__integration__/connector-upload.integration.ts
241241
lib/uploads/contexts/organization-logo/application.integration.ts
242242
243+
- name: Verify Confluence audiences and permission queries in PostgreSQL
244+
working-directory: apps/sim
245+
env:
246+
KNOWLEDGE_ACL_TEST_DATABASE_URL: postgresql://postgres:postgres@127.0.0.1:5432/sim_auth_scim
247+
run: |
248+
bunx vitest run --mode integration \
249+
lib/knowledge/access/group-membership.integration.ts \
250+
lib/knowledge/__integration__/confluence-identity.integration.ts \
251+
lib/knowledge/__integration__/directory-sync.integration.ts
252+
bunx vitest run \
253+
lib/knowledge/access/predicate.postgres.test.ts \
254+
lib/knowledge/connectors/external-directory.postgres.test.ts
255+
243256
test-build:
244257
name: Lint and Test
245258
runs-on: ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && 'blacksmith-8vcpu-ubuntu-2404' || 'ubuntu-latest' }}

‎apps/sim/connectors/all-selection.test.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,42 @@ beforeEach(() => {
1717
afterEach(() => vi.unstubAllGlobals())
1818

1919
describe('dynamic All source validation', () => {
20+
describe.each([
21+
{ name: 'Jira', connector: jiraConnector, field: 'projectKey' },
22+
{ name: 'Confluence', connector: confluenceConnector, field: 'spaceKey' },
23+
])('$name mixed All selection', ({ connector, field }) => {
24+
const error = 'Use "*" by itself for All, or remove it to select individual items.'
25+
26+
it.each([{ value: '*, ENG' }, { value: ['*', 'ENG'] }, { value: ['ENG', ' * '] }])(
27+
'rejects mixed keys before validation requests (%j)',
28+
async ({ value }) => {
29+
await expect(
30+
connector.validateConfig('token', { domain: DOMAIN, [field]: value })
31+
).resolves.toEqual({ valid: false, error })
32+
expect(fetchMock).not.toHaveBeenCalled()
33+
}
34+
)
35+
36+
it.each([false, true])(
37+
'rejects mixed keys before listing or splitting the scope (per member: %s)',
38+
async (perMemberListing) => {
39+
await expect(
40+
connector.listDocuments('token', { domain: DOMAIN, [field]: ['*', 'ENG'] }, undefined, {
41+
perMemberListing,
42+
})
43+
).rejects.toThrow(error)
44+
expect(fetchMock).not.toHaveBeenCalled()
45+
}
46+
)
47+
48+
it('rejects mixed keys during resumed hydration before reading the provider', async () => {
49+
await expect(
50+
connector.getDocument('token', { domain: DOMAIN, [field]: 'ENG, *' }, 'document-1')
51+
).rejects.toThrow(error)
52+
expect(fetchMock).not.toHaveBeenCalled()
53+
})
54+
})
55+
2056
it.each([{ value: '*' }, { value: ['*'] }])(
2157
'validates Confluence All without enumerating or submitting literal space keys (%j)',
2258
async ({ value: spaceKey }) => {

‎apps/sim/connectors/confluence/confluence.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ import {
3434
openConfluenceDirectory,
3535
validateConfluencePermissionAccess,
3636
} from '@/connectors/confluence/permissions'
37-
import { isAllSourceItems } from '@/connectors/selection'
37+
import { getSourceSelectionError, isAllSourceItems } from '@/connectors/selection'
3838
import type {
3939
ConnectorAclContext,
4040
ConnectorConfig,
@@ -637,6 +637,8 @@ async function resolveConfluenceAcls(
637637
syncContext?: Record<string, unknown>,
638638
aclContext?: ConnectorAclContext
639639
): Promise<Record<string, MirroredDocumentAcl>> {
640+
const selectionError = getSourceSelectionError(sourceConfig.spaceKey)
641+
if (selectionError) throw new Error(selectionError)
640642
const cloudId = await resolveCloudId(accessToken, sourceConfig, syncContext)
641643

642644
const spaceIdForKey = memoizeAsync((spaceKey: string) =>
@@ -836,6 +838,8 @@ export const confluenceConnector: ConnectorConfig = {
836838
...confluenceConnectorMeta,
837839

838840
listDocuments: async (accessToken, sourceConfig, cursor, syncContext) => {
841+
const selectionError = getSourceSelectionError(sourceConfig.spaceKey)
842+
if (selectionError) throw new Error(selectionError)
839843
const cloudId = await resolveCloudId(accessToken, sourceConfig, syncContext)
840844
return listConfluenceAttachments({
841845
accessToken,
@@ -864,6 +868,8 @@ export const confluenceConnector: ConnectorConfig = {
864868
externalId: string,
865869
syncContext?: Record<string, unknown>
866870
): Promise<ExternalDocument | null> => {
871+
const selectionError = getSourceSelectionError(sourceConfig.spaceKey)
872+
if (selectionError) throw new Error(selectionError)
867873
const domain = normalizeConfluenceDomainHost(sourceConfig.domain as string)
868874
const cloudId = await resolveCloudId(accessToken, sourceConfig, syncContext)
869875

@@ -946,6 +952,8 @@ export const confluenceConnector: ConnectorConfig = {
946952
sourceConfig: Record<string, unknown>,
947953
syncContext?: Record<string, unknown>
948954
): Promise<{ valid: boolean; error?: string }> => {
955+
const selectionError = getSourceSelectionError(sourceConfig.spaceKey)
956+
if (selectionError) return { valid: false, error: selectionError }
949957
const domain = sourceConfig.domain as string
950958
const allSpaces = isAllSourceItems(sourceConfig.spaceKey)
951959
const spaceKeys = parseMultiValue(sourceConfig.spaceKey)

‎apps/sim/connectors/jira/jira.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {
1010
import { fetchWithRetry } from '@/lib/knowledge/documents/secure-fetch.server'
1111
import { type RetryOptions, VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils'
1212
import { jiraConnectorMeta } from '@/connectors/jira/meta'
13-
import { isAllSourceItems } from '@/connectors/selection'
13+
import { getSourceSelectionError, isAllSourceItems } from '@/connectors/selection'
1414
import type { ConnectorConfig, ExternalDocument, ExternalDocumentList } from '@/connectors/types'
1515
import {
1616
computeContentHash,
@@ -303,6 +303,8 @@ export const jiraConnector: ConnectorConfig = {
303303
cursor?: string,
304304
syncContext?: Record<string, unknown>
305305
): Promise<ExternalDocumentList> => {
306+
const selectionError = getSourceSelectionError(sourceConfig.projectKey)
307+
if (selectionError) throw new Error(selectionError)
306308
const domain = sourceConfig.domain as string
307309
const siteUrl = normalizeAtlassianSiteUrl(domain)
308310
const projectKeys = parseMultiValue(sourceConfig.projectKey)
@@ -527,6 +529,8 @@ export const jiraConnector: ConnectorConfig = {
527529
externalId: string,
528530
syncContext?: Record<string, unknown>
529531
): Promise<ExternalDocument | null> => {
532+
const selectionError = getSourceSelectionError(sourceConfig.projectKey)
533+
if (selectionError) throw new Error(selectionError)
530534
const domain = sourceConfig.domain as string
531535
const siteUrl = normalizeAtlassianSiteUrl(domain)
532536
const cloudId = await resolveCloudId(accessToken, domain, syncContext)
@@ -573,6 +577,8 @@ export const jiraConnector: ConnectorConfig = {
573577
sourceConfig: Record<string, unknown>,
574578
syncContext?: Record<string, unknown>
575579
): Promise<{ valid: boolean; error?: string }> => {
580+
const selectionError = getSourceSelectionError(sourceConfig.projectKey)
581+
if (selectionError) return { valid: false, error: selectionError }
576582
const domain = sourceConfig.domain as string
577583
const projectKeys = parseMultiValue(sourceConfig.projectKey)
578584

‎apps/sim/connectors/selection.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import type { ConnectorMeta } from '@/connectors/types'
12
import { parseMultiValue } from '@/connectors/utils'
23

34
/** Persisted scope marker; providers resolve the accessible set during each listing. */
@@ -7,3 +8,27 @@ export function isAllSourceItems(value: unknown): boolean {
78
const items = parseMultiValue(value)
89
return items.length === 1 && items[0] === ALL_SOURCE_ITEMS
910
}
11+
12+
export function getSourceSelectionError(
13+
value: unknown,
14+
allValue = ALL_SOURCE_ITEMS
15+
): string | undefined {
16+
const items = parseMultiValue(value)
17+
if (items.length > 1 && items.includes(allValue)) {
18+
return `Use "${allValue}" by itself for All, or remove it to select individual items.`
19+
}
20+
}
21+
22+
export function findSourceSelectionError(
23+
connector: Pick<ConnectorMeta, 'configFields'>,
24+
sourceConfig: Record<string, unknown>
25+
): string | undefined {
26+
for (const field of connector.configFields) {
27+
if (!field.selectAllValue) continue
28+
const error = getSourceSelectionError(
29+
sourceConfig[field.canonicalParamId ?? field.id],
30+
field.selectAllValue
31+
)
32+
if (error) return error
33+
}
34+
}

‎apps/sim/lib/knowledge/application/personal-source-setup.test.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,17 @@ const runConnect = (changes = {}) =>
9090
personalSourceSetup.execute({ principal, input: { ...connect, keys: ['PROJECT'], ...changes } })
9191

9292
describe('personal source setup', () => {
93+
it.each(['confluence', 'jira'] as const)(
94+
'rejects mixed All and explicit %s keys before discovery or saving',
95+
async (connectorType) => {
96+
await expect(runConnect({ connectorType, keys: ['*', 'ENG'] })).rejects.toThrow(
97+
'Use "*" by itself for All, or remove it to select individual items.'
98+
)
99+
expect(mocks.selector).not.toHaveBeenCalled()
100+
expect(mocks.configure).not.toHaveBeenCalled()
101+
}
102+
)
103+
93104
it.each(['confluence', 'jira'] as const)(
94105
'authorizes %s All without freezing the available keys',
95106
async (connectorType) => {

‎apps/sim/lib/knowledge/application/personal-source-setup.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ import { MAX_SELECTOR_PAGES } from '@/lib/selectors/limits'
2828
import type { SelectorExecutionResult, SelectorRequest } from '@/lib/selectors/types'
2929
import { MAX_PERSONAL_SOURCE_SETUP_KEYS } from '@/lib/sim-search/personal-source-setup'
3030
import { CONNECTOR_META_REGISTRY } from '@/connectors/registry'
31-
import { isAllSourceItems } from '@/connectors/selection'
31+
import { getSourceSelectionError, isAllSourceItems } from '@/connectors/selection'
3232

3333
const logger = createLogger('PersonalSourceSetup')
3434
const VALIDATION_PHASE_TIMEOUT_MS = 30_000
@@ -178,6 +178,8 @@ export const personalSourceSetup = defineAuthorizedKnowledgeUseCase({
178178
throw new OrchestrationError('validation', 'Select between 1 and 1,000 projects or spaces')
179179
}
180180
const keys = [...new Set(input.keys.map((key) => key.trim()))]
181+
const selectionError = getSourceSelectionError(keys)
182+
if (selectionError) throw new OrchestrationError('validation', selectionError)
181183
const remaining = new Set(keys)
182184
const cursors = new Set<string>()
183185
const timeout = AbortSignal.timeout(VALIDATION_PHASE_TIMEOUT_MS)

‎apps/sim/lib/knowledge/orchestration/connectors.test.ts‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,8 +101,23 @@ vi.mock('@/connectors/registry.server', () => ({
101101
},
102102
notion: {
103103
auth: { mode: 'apiKey', optional: true },
104+
configFields: [],
104105
validateConfig: vi.fn().mockResolvedValue({ valid: true }),
105106
},
107+
jira: {
108+
name: 'Jira',
109+
auth: { mode: 'oauth', provider: 'jira' },
110+
permissionScopedListing: { capFieldIds: [] },
111+
configFields: [
112+
{ id: 'projectSelector', canonicalParamId: 'projectKey', selectAllValue: '*' },
113+
],
114+
},
115+
confluence: {
116+
name: 'Confluence',
117+
auth: { mode: 'oauth', provider: 'confluence' },
118+
permissionScopedListing: { capFieldIds: [] },
119+
configFields: [{ id: 'spaceSelector', canonicalParamId: 'spaceKey', selectAllValue: '*' }],
120+
},
106121
google_drive: {
107122
name: 'Google Drive',
108123
auth: { mode: 'oauth', provider: 'google-drive' },
@@ -155,6 +170,30 @@ describe('performCreateKnowledgeConnector', () => {
155170
resolveAccessToken: vi.fn(),
156171
}
157172

173+
it.each([
174+
{ connectorType: 'jira', field: 'projectKey' },
175+
{ connectorType: 'confluence', field: 'spaceKey' },
176+
])(
177+
'rejects mixed All keys for credentialless $connectorType members',
178+
async ({ connectorType, field }) => {
179+
const outcome = await performCreateKnowledgeConnector({
180+
...createParams,
181+
connectorType,
182+
sourceConfig: { domain: 'example.atlassian.net', [field]: ['*', 'ENG'] },
183+
accessMode: 'members',
184+
membersBinding: { credentialGroupId: 'group-1', credentialGroupOptionId: 'option-1' },
185+
})
186+
expect(outcome).toMatchObject({
187+
success: false,
188+
errorCode: 'validation',
189+
error: 'Use "*" by itself for All, or remove it to select individual items.',
190+
})
191+
expect(dbChainMockFns.insert).not.toHaveBeenCalled()
192+
expect(mockGrant).not.toHaveBeenCalled()
193+
expect(createParams.resolveAccessToken).not.toHaveBeenCalled()
194+
}
195+
)
196+
158197
it('validates and encrypts a GitHub PAT without resolving an OAuth account or returning the secret', async () => {
159198
dbChainMockFns.limit.mockResolvedValueOnce([{ id: 'kb-1' }])
160199
dbChainMockFns.returning.mockResolvedValueOnce([
@@ -481,6 +520,41 @@ describe('performUpdateKnowledgeConnector', () => {
481520

482521
afterAll(resetDbChainMock)
483522

523+
it.each([
524+
{ connectorType: 'jira', field: 'projectKey' },
525+
{ connectorType: 'confluence', field: 'spaceKey' },
526+
])(
527+
'rejects mixed All keys when editing credentialless $connectorType members',
528+
async ({ connectorType, field }) => {
529+
queueTableRows(schemaMock.knowledgeConnector, [
530+
{
531+
id: 'conn-1',
532+
connectorType,
533+
accessMode: 'members',
534+
status: 'active',
535+
memberSyncStatus: 'idle',
536+
credentialId: null,
537+
},
538+
])
539+
const validateSourceConfig = vi.fn()
540+
const outcome = await performUpdateKnowledgeConnector({
541+
...ACTOR,
542+
knowledgeBase: KB,
543+
connectorId: 'conn-1',
544+
updates: { sourceConfig: { domain: 'example.atlassian.net', [field]: '*, ENG' } },
545+
resolveBillingAttribution,
546+
validateSourceConfig,
547+
})
548+
expect(outcome).toMatchObject({
549+
success: false,
550+
errorCode: 'validation',
551+
error: 'Use "*" by itself for All, or remove it to select individual items.',
552+
})
553+
expect(dbChainMockFns.update).not.toHaveBeenCalled()
554+
expect(validateSourceConfig).not.toHaveBeenCalled()
555+
}
556+
)
557+
484558
it('rejects an update that names nothing before reading the connector', async () => {
485559
const outcome = await performUpdateKnowledgeConnector({
486560
...ACTOR,

‎apps/sim/lib/knowledge/orchestration/connectors.ts‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ import { createTagDefinition } from '@/lib/knowledge/tags/service'
6464
import { captureServerEvent } from '@/lib/posthog/server'
6565
import { searchSourceIdentity } from '@/lib/sim-search/source-identity'
6666
import { getConnectorApiKeyConfig } from '@/connectors/auth'
67+
import { findSourceSelectionError } from '@/connectors/selection'
6768
import { PER_MEMBER_LISTING_CONTEXT } from '@/connectors/utils'
6869

6970
const logger = createLogger('KnowledgeConnectorOrchestration')
@@ -259,6 +260,9 @@ export async function performCreateKnowledgeConnector(
259260
return fail(`Unknown connector type: ${connectorType}`, 'validation')
260261
}
261262

263+
const selectionError = findSourceSelectionError(connectorConfig, sourceConfig)
264+
if (selectionError) return fail(selectionError, 'validation')
265+
262266
try {
263267
await assertLiveSyncAllowed(resourceScopeFromOwner(kb), syncIntervalMinutes)
264268
} catch (error) {
@@ -825,11 +829,13 @@ export async function performUpdateKnowledgeConnector(
825829
let nextSourceConfig = params.prepareSourceConfig
826830
? await params.prepareSourceConfig(existing, updates.sourceConfig)
827831
: updates.sourceConfig
828-
if (aclIsDerived(accessMode)) {
829-
/** A derived-ACL mode has no listing cap; a save may refuse one, never store one. */
830-
const { CONNECTOR_REGISTRY } = await import('@/connectors/registry.server')
831-
const connectorConfig = CONNECTOR_REGISTRY[existing.connectorType]
832-
if (connectorConfig) {
832+
const { CONNECTOR_REGISTRY } = await import('@/connectors/registry.server')
833+
const connectorConfig = CONNECTOR_REGISTRY[existing.connectorType]
834+
if (connectorConfig) {
835+
const selectionError = findSourceSelectionError(connectorConfig, nextSourceConfig)
836+
if (selectionError) return fail(selectionError, 'validation')
837+
if (aclIsDerived(accessMode)) {
838+
/** A derived-ACL mode has no listing cap; a save may refuse one, never store one. */
833839
const capViolation = findListingCapViolation(connectorConfig, nextSourceConfig)
834840
if (capViolation) return fail(capViolation, 'validation')
835841
nextSourceConfig = stripListingCapFields(connectorConfig, nextSourceConfig)

0 commit comments

Comments
 (0)