Skip to content

Commit 1922fb8

Browse files
authored
fix(oauth): add Atlassian sites and Notion workspaces on the same login (#9760)
* fix(search): add an Atlassian site to the login that already holds it Atlassian keeps one grant per login and adds each consented site to it, so connecting the same login for another Confluence or Jira site was refused as an already connected account. Treat it as a refresh of that account, which keeps the new grant; every other provider still refuses a duplicate. * fix(oauth): key Notion connections on their workspace A Notion token reaches one workspace, but Sim keyed Notion accounts on the person alone, so a second workspace on the same login was refused. Record the workspace as the tenant. A connection saved before then takes on its workspace when the person reconnects, and must be reconnected before another workspace is added so it is never overwritten. Other providers keep treating a missing tenant as distinct. * fix(oauth): prove the new Atlassian grant is saved and note the site limit
1 parent d644eb9 commit 1922fb8

7 files changed

Lines changed: 145 additions & 10 deletions

File tree

‎apps/docs/content/docs/search/connect-your-account.mdx‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ After your admin adds a source in **Settings → Sources**, connect your own acc
2525
To connect another account of the same app, such as Google Drive for two different work accounts, open the app and choose **Add account**, then pick the other account when the provider asks. You can connect up to five accounts per app, and Search looks across all of them. Open an account to see its tool permissions when the app supports them; each account's menu has **Reconnect** and **Disconnect**.
2626

2727
- Slack accounts must belong to your organization's connected workspace.
28+
- For another Confluence or Jira site on the same Atlassian login, choose **Add account** or **Reconnect** and pick the site. The site joins your existing account. Search reads up to four sites per Atlassian account in one search and notes when it leaves sites out.
29+
- Each Notion workspace is its own account, so add one per workspace. If Sim asks you to reconnect first, reconnect your existing Notion account once, then add the other workspace.
2830
- Zoom, Lucid, and Databricks allow one account, because Sim cannot yet tell their accounts apart.
2931
- If the provider signs you straight back into a connected account, sign in to the other account with the provider first, then choose **Add account** again.
3032

‎apps/sim/lib/auth/connectors/managed-oauth.test.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,8 @@ describe('userinfo-backed managed OAuth connectors', () => {
276276
type: 'user',
277277
user: { id: 'user-1', name: 'Person', person: { email: 'person@example.com' } },
278278
},
279+
workspace_id: 'workspace-1',
280+
workspace_name: 'Engineering',
279281
},
280282
})
281283

@@ -286,6 +288,7 @@ describe('userinfo-backed managed OAuth connectors', () => {
286288

287289
expect(identity).toMatchObject({
288290
providerSubjectId: 'user-1',
291+
providerTenantId: 'workspace-1',
289292
email: 'person@example.com',
290293
emailVerified: true,
291294
})

‎apps/sim/lib/auth/connectors/managed-oauth.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -905,7 +905,12 @@ const USER_INFO_MANAGED_OAUTH_CONNECTORS = new Map<string, () => ManagedOAuthCon
905905
/** Notion only exposes `person.email` for a confirmed workspace member. */
906906
emailVerified: true,
907907
},
908-
{ displayName: user.name, avatarUrl: user.avatar_url }
908+
/** A Notion token reaches one workspace, so one person's workspaces are separate accounts. */
909+
{
910+
displayName: user.name,
911+
avatarUrl: user.avatar_url,
912+
providerTenantId: bot.workspace_id,
913+
}
909914
)
910915
},
911916
}),

‎apps/sim/lib/credential-groups/__integration__/multiple-provider-accounts.integration.ts‎

Lines changed: 81 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,9 @@ vi.mock('@/lib/credential-groups/scoped-availability', () => ({
3939
isScopedCredentialGroupsAvailable: async () => true,
4040
}))
4141

42+
/** Each grant carries its own token, as a real exchange does, so a saved grant is identifiable. */
43+
let grants = 0
44+
4245
vi.mock('@/lib/credential-groups/provider-registry', async (importOriginal) => {
4346
const { getCredentialGroupProviderId } = await import('@/lib/credential-groups/providers')
4447
return {
@@ -61,7 +64,7 @@ vi.mock('@/lib/credential-groups/provider-registry', async (importOriginal) => {
6164
providerTenantId: tenant || null,
6265
displayName: code,
6366
metadata: { email: code },
64-
accessToken: `token-for-${code}`,
67+
accessToken: `token-for-${code}-${(grants += 1)}`,
6568
grantedScopes: POLICY.requiredScopes,
6669
accessTokenExpiresAt: null,
6770
refreshTokenExpiresAt: null,
@@ -93,6 +96,8 @@ const optionFor = (provider: CredentialGroupProvider) => ({
9396
const DRIVE = optionFor('google-drive')
9497
const GITHUB = optionFor('github-repositories')
9598
const SLACK = optionFor('slack')
99+
const CONFLUENCE = optionFor('confluence')
100+
const NOTION = optionFor('notion')
96101

97102
function connect(
98103
code: string,
@@ -110,7 +115,7 @@ function connect(
110115
email,
111116
enrollmentStatus: 'completed',
112117
option,
113-
options: [DRIVE, GITHUB, SLACK],
118+
options: [DRIVE, GITHUB, SLACK, CONFLUENCE, NOTION],
114119
}
115120
const attempt: CredentialGroupOAuthAttempt = {
116121
userId,
@@ -188,7 +193,7 @@ beforeAll(async () => {
188193
organizationId,
189194
publicId: generateId(),
190195
name: 'Connected accounts',
191-
options: [DRIVE, GITHUB, SLACK],
196+
options: [DRIVE, GITHUB, SLACK, CONFLUENCE, NOTION],
192197
})
193198
await db.insert(member).values({
194199
id: generateId(),
@@ -251,7 +256,7 @@ describe('connecting several accounts of one provider', () => {
251256
status: 'active',
252257
})
253258
expect(await decryptManagedOAuthTokenSet(stored?.tokens ?? '')).toMatchObject({
254-
accessToken: 'token-for-work@second-tenant.test',
259+
accessToken: expect.stringMatching(/^token-for-work@second-tenant\.test-/),
255260
})
256261
})
257262

@@ -337,6 +342,78 @@ describe('reconnecting one of several accounts', () => {
337342
})
338343
})
339344

345+
describe('adding another site to an Atlassian login', () => {
346+
it('keeps the new grant on the existing account instead of refusing it', async () => {
347+
const first = await connect('person@atlassian', { kind: 'create' }, CONFLUENCE)
348+
const saved = async () =>
349+
(await decryptManagedOAuthTokenSet((await rows())[0]?.tokens ?? '')).accessToken
350+
const firstToken = await saved()
351+
352+
const added = await connect('person@atlassian', { kind: 'create' }, CONFLUENCE)
353+
354+
expect(added).toMatchObject({ created: false, credentialId: first.credentialId })
355+
expect(await rows()).toHaveLength(1)
356+
expect(await saved()).toMatch(/^token-for-person@atlassian-/)
357+
expect(await saved()).not.toBe(firstToken)
358+
})
359+
360+
it('still adds a different Atlassian login as its own account', async () => {
361+
await connect('person@atlassian', { kind: 'create' }, CONFLUENCE)
362+
await connect('other@atlassian', { kind: 'create' }, CONFLUENCE)
363+
expect(await rows()).toHaveLength(2)
364+
})
365+
})
366+
367+
describe('an account recorded before its tenant was known', () => {
368+
it('stays separate on a provider that always recorded its tenant', async () => {
369+
const legacy = await connect('person', { kind: 'create' })
370+
expect(
371+
await failureOf(
372+
connect('person@workspace-a', { kind: 'reconnect', credentialId: legacy.credentialId })
373+
)
374+
).toBe('account_mismatch')
375+
})
376+
377+
it('takes on its tenant when reconnected or refreshed', async () => {
378+
const legacy = await connect('person', { kind: 'create' }, NOTION)
379+
380+
await expect(
381+
connect(
382+
'person@workspace-a',
383+
{ kind: 'reconnect', credentialId: legacy.credentialId },
384+
NOTION
385+
)
386+
).resolves.toMatchObject({ created: false, credentialId: legacy.credentialId })
387+
expect(await rows()).toMatchObject([{ subject: 'person', tenant: 'workspace-a' }])
388+
})
389+
390+
it('takes on its tenant through an invitation link', async () => {
391+
const legacy = await connect('person', { kind: 'create' }, NOTION)
392+
await expect(connect('person@workspace-a', undefined, NOTION)).resolves.toMatchObject({
393+
credentialId: legacy.credentialId,
394+
})
395+
expect(await rows()).toMatchObject([{ tenant: 'workspace-a' }])
396+
})
397+
398+
it('must be reconnected before another tenant of the same login is added, so it is never overwritten', async () => {
399+
const legacy = await connect('person', { kind: 'create' }, NOTION)
400+
const [before] = await rows()
401+
402+
expect(await failureOf(connect('person@workspace-b', { kind: 'create' }, NOTION))).toBe(
403+
'account_identity_required'
404+
)
405+
expect(await rows()).toEqual([before])
406+
407+
await connect(
408+
'person@workspace-a',
409+
{ kind: 'reconnect', credentialId: legacy.credentialId },
410+
NOTION
411+
)
412+
await connect('person@workspace-b', { kind: 'create' }, NOTION)
413+
expect((await rows()).map((row) => row.tenant).sort()).toEqual(['workspace-a', 'workspace-b'])
414+
})
415+
})
416+
340417
describe('adding an account that needs reconnecting', () => {
341418
it('refreshes it instead of calling it already connected', async () => {
342419
const first = await connect('work@first-tenant.test', { kind: 'create' })

‎apps/sim/lib/credential-groups/account-rules.ts‎

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,15 @@ export function resolveAccountWrite<Account extends ConnectedAccount>({
3434
limit,
3535
accounts,
3636
identity,
37+
grantSpansSites = false,
38+
tenantRecordedLater = false,
3739
}: {
3840
intent?: CredentialGroupConnectionIntent
3941
limit: number
42+
/** Connecting the same login again extends its grant, so it refreshes that account. */
43+
grantSpansSites?: boolean
44+
/** An account saved without a tenant may be this identity's, at a tenant not yet recorded. */
45+
tenantRecordedLater?: boolean
4046
accounts: Account[]
4147
identity: { subject: string; tenant: string | null } | null
4248
}): Account | undefined {
@@ -52,6 +58,14 @@ export function resolveAccountWrite<Account extends ConnectedAccount>({
5258
const disconnectedUnidentified = accounts.find(
5359
(account) => account.revokedAt && account.providerSubjectId === null
5460
)
61+
/** Saved before its provider reported a tenant: the same person, at a tenant still unknown. */
62+
const tenantUnknown =
63+
tenantRecordedLater && identity && identity.tenant !== null && !existing
64+
? accounts.find(
65+
(account) =>
66+
account.providerSubjectId === identity.subject && account.providerTenantId === null
67+
)
68+
: undefined
5569

5670
if (intent?.kind === 'reconnect') {
5771
const target = accounts.find((account) => account.id === intent.credentialId)
@@ -61,7 +75,11 @@ export function resolveAccountWrite<Account extends ConnectedAccount>({
6175
409
6276
)
6377
if (identity) {
64-
if (existing ? existing.id !== target.id : target.providerSubjectId !== null)
78+
if (
79+
existing
80+
? existing.id !== target.id
81+
: target.providerSubjectId !== null && target.id !== tenantUnknown?.id
82+
)
6583
throw accountConflict('account_mismatch')
6684
} else if (
6785
target.providerSubjectId !== null &&
@@ -75,18 +93,20 @@ export function resolveAccountWrite<Account extends ConnectedAccount>({
7593

7694
if (intent?.kind === 'create') {
7795
if (existing && !existing.revokedAt) {
78-
if (existing.managedOauthStatus === 'active')
96+
if (existing.managedOauthStatus === 'active' && !grantSpansSites)
7997
throw accountConflict('account_already_connected')
8098
return existing
8199
}
82100
if (live.length > 0 && !identity) throw accountConflict('account_unverified')
83-
if (hasUnidentifiedLive) throw accountConflict('account_identity_required')
101+
if (hasUnidentifiedLive || (tenantUnknown && !tenantUnknown.revokedAt))
102+
throw accountConflict('account_identity_required')
84103
if (live.length >= limit) throw accountConflict('account_limit_reached')
85-
return existing ?? (live.length === 0 ? disconnectedUnidentified : undefined)
104+
return existing ?? (live.length === 0 ? (tenantUnknown ?? disconnectedUnidentified) : undefined)
86105
}
87106

88107
if (existing && !existing.revokedAt) return existing
89-
if (live.length === 0) return existing ?? disconnectedUnidentified
108+
if (live.length === 0) return existing ?? tenantUnknown ?? disconnectedUnidentified
109+
if (tenantUnknown && !tenantUnknown.revokedAt) return tenantUnknown
90110
if (live.length === 1 && !existing && (!identity || hasUnidentifiedLive)) return live[0]
91111
throw accountConflict(identity ? 'account_mismatch' : 'account_unverified')
92112
}

‎apps/sim/lib/credential-groups/oauth.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@ import {
3535
import { getCredentialGroupProviderAdapter } from '@/lib/credential-groups/provider-registry'
3636
import {
3737
type CredentialGroupProvider,
38+
credentialGroupGrantSpansSites,
39+
credentialGroupTenantRecordedLater,
3840
getCredentialGroupProviderService,
3941
isCredentialGroupProvider,
4042
} from '@/lib/credential-groups/providers'
@@ -247,6 +249,8 @@ async function persistGrant(
247249
limit: CREDENTIAL_GROUP_ACCOUNTS_PER_PROVIDER_LIMIT,
248250
accounts,
249251
identity: { subject: grant.providerSubjectId, tenant: grant.providerTenantId },
252+
grantSpansSites: credentialGroupGrantSpansSites(adapter.provider),
253+
tenantRecordedLater: credentialGroupTenantRecordedLater(adapter.provider),
250254
})
251255

252256
let refreshToken = grant.refreshToken

‎apps/sim/lib/credential-groups/providers.ts‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,17 @@ interface CredentialGroupProviderSupport {
5252
serviceId: string
5353
description: string
5454
configuration: 'oauth' | 'slack_custom_bot'
55+
/**
56+
* One login holds a single grant that gains each site it consents to, so connecting that login
57+
* again adds a site to the existing account rather than a second account.
58+
*/
59+
grantSpansSites?: true
60+
/**
61+
* Sim began recording this provider's tenant after accounts were saved without one. Such an
62+
* account takes on its tenant when the same person reconnects; elsewhere a missing tenant never
63+
* matches another.
64+
*/
65+
tenantRecordedLater?: true
5566
}
5667

5768
const CREDENTIAL_GROUP_PROVIDER_SUPPORT: Record<
@@ -127,11 +138,13 @@ const CREDENTIAL_GROUP_PROVIDER_SUPPORT: Record<
127138
serviceId: 'confluence',
128139
description: 'Let each person connect one Confluence account',
129140
configuration: 'oauth',
141+
grantSpansSites: true,
130142
},
131143
jira: {
132144
serviceId: 'jira',
133145
description: 'Let each person connect one Jira account',
134146
configuration: 'oauth',
147+
grantSpansSites: true,
135148
},
136149
airtable: {
137150
serviceId: 'airtable',
@@ -189,6 +202,7 @@ const CREDENTIAL_GROUP_PROVIDER_SUPPORT: Record<
189202
configuration: 'oauth',
190203
},
191204
notion: {
205+
tenantRecordedLater: true,
192206
serviceId: 'notion',
193207
description: 'Let each person connect one Notion account',
194208
configuration: 'oauth',
@@ -258,6 +272,16 @@ export function getCredentialGroupProviderService(
258272
return service
259273
}
260274

275+
/** Whether connecting the same login again refreshes its existing account, adding sites to it. */
276+
export function credentialGroupGrantSpansSites(provider: CredentialGroupProvider): boolean {
277+
return CREDENTIAL_GROUP_PROVIDER_SUPPORT[provider].grantSpansSites === true
278+
}
279+
280+
/** Whether an account saved without a tenant may take one on when the same person reconnects. */
281+
export function credentialGroupTenantRecordedLater(provider: CredentialGroupProvider): boolean {
282+
return CREDENTIAL_GROUP_PROVIDER_SUPPORT[provider].tenantRecordedLater === true
283+
}
284+
261285
export function getCredentialGroupProviderId(provider: CredentialGroupProvider): string {
262286
return getCredentialGroupProviderService(provider).providerId
263287
}

0 commit comments

Comments
 (0)