Skip to content

Commit e339088

Browse files
committed
fix(credentials): close wave 3 review findings on Microsoft app credentials
- SharePoint selectors (workspace and organization paths) and knowledge connectors now pass the service they resolve for (the selector's own serviceId, the connector's declared auth provider), so a Microsoft app credential browses SharePoint with a Graph token instead of being refused. The resolution option is renamed from toolService to service; it is only ever set from server-side policy. - AADSTS700211 and 700212 are no longer mapped: Microsoft documents only 70021 and 700213 for federated credential mismatches. Other keyless refusals still name the mismatch. - DocuSign minter test mocks carry account_name, and the connector access-token test narrows its tokens with a guard instead of non-null assertions.
1 parent 1237119 commit e339088

11 files changed

Lines changed: 125 additions & 28 deletions

File tree

‎apps/sim/lib/credentials/application/organization-credentials.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -392,7 +392,10 @@ export async function authorizeOrganizationCredentialUse(input: {
392392
}
393393

394394
export async function resolveOrganizationCredentialTokenBundle(
395-
input: Parameters<typeof authorizeOrganizationCredentialUse>[0]
395+
input: Parameters<typeof authorizeOrganizationCredentialUse>[0] & {
396+
/** The service the token is for, from the caller's server-side definition. */
397+
service?: string
398+
}
396399
): Promise<ServiceAccountTokenResult | null> {
397400
const { credential: row, userId } = await authorizeOrganizationCredentialUse(input)
398401
if (row.type === 'managed_oauth') {
@@ -409,7 +412,7 @@ export async function resolveOrganizationCredentialTokenBundle(
409412
input.requestId,
410413
input.requiredScopes,
411414
input.impersonateEmail,
412-
{ privacyMode: 'selector' }
415+
{ privacyMode: 'selector', service: input.service }
413416
)
414417
}
415418

‎apps/sim/lib/credentials/client-credential-accounts/minters/docusign.test.ts‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,12 @@ describe('mintDocuSignServiceAccountToken', () => {
112112

113113
it('mints and resolves accounts on the production host when the credential says production', async () => {
114114
mockMint([
115-
{ account_id: DEFAULT_ACCOUNT, is_default: true, base_uri: 'https://na3.docusign.net' },
115+
{
116+
account_id: DEFAULT_ACCOUNT,
117+
is_default: true,
118+
account_name: 'Production',
119+
base_uri: 'https://na3.docusign.net',
120+
},
116121
])
117122

118123
const result = await mintDocuSignServiceAccountToken({ ...FIELDS, dataCenter: 'production' })
@@ -151,7 +156,14 @@ describe('mintDocuSignServiceAccountToken', () => {
151156
['https://demo.docusign.net', 'production'],
152157
['http://na3.docusign.net', 'production'],
153158
])('never binds a %s base_uri to a %s credential', async (baseUri, environment) => {
154-
mockMint([{ account_id: DEFAULT_ACCOUNT, is_default: true, base_uri: baseUri }])
159+
mockMint([
160+
{
161+
account_id: DEFAULT_ACCOUNT,
162+
is_default: true,
163+
account_name: 'Default',
164+
base_uri: baseUri,
165+
},
166+
])
155167

156168
await expect(
157169
mintDocuSignServiceAccountToken({ ...FIELDS, dataCenter: environment })

‎apps/sim/lib/credentials/microsoft-app-service-account.server.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,12 @@ const FEDERATION_MISMATCH_MESSAGE =
185185

186186
/**
187187
* Actionable copy for the Entra errors a person saving the credential can fix.
188+
* AADSTS70021 (no matching federated identity record) is documented in the
189+
* workload identity federation considerations, and AADSTS700213 (no matching
190+
* record for the presented subject, issuer, or audience) in the August 2024
191+
* breaking-changes entry; any other keyless refusal still names the mismatch.
188192
* @see https://learn.microsoft.com/en-us/entra/identity-platform/reference-error-codes
193+
* @see https://learn.microsoft.com/en-us/entra/workload-id/workload-identity-federation-considerations
189194
* @see https://learn.microsoft.com/en-us/entra/identity-platform/reference-breaking-changes
190195
*/
191196
function entraFailureMessage(aadsts: number | undefined, config: MicrosoftAppConfig): string {
@@ -207,8 +212,6 @@ function entraFailureMessage(aadsts: number | undefined, config: MicrosoftAppCon
207212
? 'Microsoft Entra does not recognize this Dataverse environment in the tenant. Check the environment URL.'
208213
: 'Microsoft Entra does not recognize the requested resource in this tenant.'
209214
case 70021:
210-
case 700211:
211-
case 700212:
212215
case 700213:
213216
return FEDERATION_MISMATCH_MESSAGE
214217
default:

‎apps/sim/lib/knowledge/connectors/access-token.test.ts‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -67,11 +67,13 @@ describe('resolveConnectorAccessToken', () => {
6767

6868
mockResolveTokenBundle.mockResolvedValueOnce({ accessToken: 'jwt-token', instanceUrl: bound })
6969
const jwtToken = await resolveDocuSign()
70-
expect(syncContextForToken(jwtToken!)).toEqual({ credentialInstanceUrl: bound })
70+
if (!jwtToken) throw new Error('Expected a JWT Grant connector token')
71+
expect(syncContextForToken(jwtToken)).toEqual({ credentialInstanceUrl: bound })
7172

7273
mockResolveTokenBundle.mockResolvedValueOnce({ accessToken: 'oauth-token' })
7374
const oauthToken = await resolveDocuSign()
74-
expect(syncContextForToken(oauthToken!)).toEqual({})
75+
if (!oauthToken) throw new Error('Expected an OAuth connector token')
76+
expect(syncContextForToken(oauthToken)).toEqual({})
7577
})
7678

7779
it('does not accept an undeclared key alternative on other OAuth connectors', async () => {
@@ -185,7 +187,8 @@ describe('delegated connector access', () => {
185187
'actor',
186188
'request',
187189
delegationAuth.adminServiceAccountScopes,
188-
'admin@example.com'
190+
'admin@example.com',
191+
{ service: 'google-drive' }
189192
)
190193
await expect(token?.getDelegatedAccessToken?.(' Employee@Example.com ')).resolves.toBe(
191194
'user-drive-token'
@@ -294,7 +297,8 @@ describe.each([
294297
'actor',
295298
'request',
296299
[directoryScope],
297-
'directory.admin@example.com'
300+
'directory.admin@example.com',
301+
{ service: auth.mode === 'oauth' ? auth.provider : undefined }
298302
)
299303
expect(token?.getDelegatedAccessToken).toBeTypeOf('function')
300304
await expect(token?.getDelegatedAccessToken?.(' Employee@Example.com ')).resolves.toBe(
@@ -353,7 +357,8 @@ describe('impersonation on the connector path', () => {
353357
'credential-owner',
354358
'req-1',
355359
OAUTH_AUTH.requiredScopes,
356-
'admin@corp.com'
360+
'admin@corp.com',
361+
{ service: 'google-drive' }
357362
)
358363
})
359364
})

‎apps/sim/lib/knowledge/connectors/access-token.ts‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -144,14 +144,11 @@ export async function resolveConnectorAccessToken(params: {
144144
requestId,
145145
connectorServiceAccountScopes(auth, params.accessMode),
146146
subject,
147-
...(githubRepositoryScope || params.signal
148-
? [
149-
{
150-
...(githubRepositoryScope ? { githubRepositoryScope } : {}),
151-
...(params.signal ? { signal: params.signal } : {}),
152-
},
153-
]
154-
: [])
147+
{
148+
service: auth.provider,
149+
...(githubRepositoryScope ? { githubRepositoryScope } : {}),
150+
...(params.signal ? { signal: params.signal } : {}),
151+
}
155152
)
156153
if (!bundle?.accessToken) return null
157154

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -96,10 +96,11 @@ export interface CredentialTokenResolutionOptions {
9696
/** GitHub installation content tokens may only address one connector repository. */
9797
githubRepositoryScope?: GitHubInstallationRepositoryScope
9898
/**
99-
* The OAuth service of the tool the token is for, set only from the server-validated tool
100-
* policy. A Microsoft app credential mints for that service's resource and refuses any other.
99+
* The OAuth service the token is for, set only from server-side policy (a tool's validated
100+
* policy, a selector's or connector's own definition), never from the caller. A Microsoft app
101+
* credential mints for that service's resource and refuses any other.
101102
*/
102-
toolService?: string
103+
service?: string
103104
/**
104105
* Cancels supported service-account token exchanges and retry waits, and stops this caller
105106
* waiting on an OAuth refresh without cancelling the refresh other callers share.
@@ -751,7 +752,7 @@ const SERVICE_ACCOUNT_TOKEN_RESOLVERS: Record<string, ServiceAccountTokenResolve
751752
const { resolveMicrosoftAppCredentialToken } = await import(
752753
'@/lib/credentials/microsoft-app-service-account.server'
753754
)
754-
return resolveMicrosoftAppCredentialToken(credentialId, options.toolService, options.signal)
755+
return resolveMicrosoftAppCredentialToken(credentialId, options.service, options.signal)
755756
},
756757
[DATABRICKS_SERVICE_ACCOUNT_PROVIDER_ID]: async (credentialId, options) => {
757758
const { resolveDatabricksCredentialToken } = await import(

‎apps/sim/lib/oauth/token-resolution.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -687,7 +687,7 @@ describe('Microsoft app credential binding', () => {
687687
'microsoft-app-service-account',
688688
expect.any(Array),
689689
undefined,
690-
{ toolService: provider }
690+
{ service: provider }
691691
)
692692
}
693693
)

‎apps/sim/lib/oauth/token-resolution.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -412,8 +412,7 @@ export async function resolveCredentialToken(
412412
scopes ?? [],
413413
impersonateEmail,
414414
{
415-
toolService: toolCredentialPolicy(input.toolId, input.assistantConnectedAccount)
416-
?.provider,
415+
service: toolCredentialPolicy(input.toolId, input.assistantConnectedAccount)?.provider,
417416
}
418417
)
419418

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
5+
import { credential } from '@sim/db/schema'
6+
import { queueTableRows, resetDbChainMock } from '@sim/testing'
7+
import { jsonResponse } from '@sim/testing/helpers/http'
8+
import { encryptionMock, encryptionMockFns } from '@sim/testing/mocks/encryption.mock'
9+
import { beforeEach, describe, expect, it, vi } from 'vitest'
10+
11+
vi.mock('@/lib/core/security/encryption', () => encryptionMock)
12+
vi.unmock('@/lib/oauth/credential-service')
13+
14+
import { resolveSelectorOAuthAccessToken } from '@/lib/selectors/server/credentials'
15+
import { createSelectorProtectedValues } from '@/lib/selectors/server/protected-values'
16+
17+
const TENANT = '11111111-2222-3333-4444-555555555555'
18+
const CLIENT_ID = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee'
19+
20+
describe('a SharePoint selector with a Microsoft app credential', () => {
21+
const fetch = vi.fn()
22+
23+
beforeEach(() => {
24+
resetDbChainMock()
25+
const row = {
26+
id: 'app-credential',
27+
type: 'service_account',
28+
accountId: null,
29+
workspaceId: 'ws-1',
30+
organizationId: null,
31+
providerId: 'microsoft-app-service-account',
32+
revokedAt: null,
33+
encryptedServiceAccountKey: 'ciphertext-selector',
34+
}
35+
queueTableRows(credential, [row])
36+
queueTableRows(credential, [row])
37+
encryptionMockFns.mockDecryptSecret.mockResolvedValueOnce({
38+
decrypted: JSON.stringify({
39+
type: 'microsoft_app_credential_v1',
40+
config: {
41+
authType: 'client_secret',
42+
tenantId: TENANT,
43+
clientId: CLIENT_ID,
44+
clientSecret: 'app-secret',
45+
},
46+
}),
47+
})
48+
fetch.mockReset().mockImplementation(async (_url: string, init: RequestInit) => {
49+
const scope = new URLSearchParams(String(init.body)).get('scope')
50+
return jsonResponse({ token_type: 'Bearer', expires_in: 3599, access_token: `at:${scope}` })
51+
})
52+
vi.stubGlobal('fetch', fetch)
53+
})
54+
55+
it('browses with a Microsoft Graph app-only token', async () => {
56+
await expect(
57+
resolveSelectorOAuthAccessToken({
58+
credential: {
59+
suppliedId: 'app-credential',
60+
access: {
61+
ok: true,
62+
credentialOwnerUserId: 'user-1',
63+
resolvedCredentialId: 'app-credential',
64+
},
65+
},
66+
serviceId: 'sharepoint',
67+
protectedValues: createSelectorProtectedValues(),
68+
})
69+
).resolves.toBe('at:https://graph.microsoft.com/.default')
70+
expect(fetch).toHaveBeenCalledWith(
71+
`https://login.microsoftonline.com/${TENANT}/oauth2/v2.0/token`,
72+
expect.anything()
73+
)
74+
})
75+
})

‎apps/sim/lib/selectors/server/credentials.test.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ describe('authorizeSelectorCredential', () => {
110110
expectedProviderId: 'google-email',
111111
requiredScopes: ['gmail.modify'],
112112
impersonateEmail: undefined,
113+
service: 'gmail',
113114
})
114115
})
115116

@@ -268,7 +269,7 @@ describe('resolveSelectorOAuthAccessToken', () => {
268269
expect(liveProtectedValues.contains('shared-access-token')).toBe(true)
269270
expect(mocks.resolveCredentialTokenBundle).toHaveBeenCalledTimes(2)
270271
for (const call of mocks.resolveCredentialTokenBundle.mock.calls) {
271-
expect(call[5]).toEqual({ privacyMode: 'selector' })
272+
expect(call[5]).toEqual({ privacyMode: 'selector', service: 'gmail' })
272273
expect(call).not.toContain(canceledController.signal)
273274
expect(call).not.toContain(liveController.signal)
274275
}

0 commit comments

Comments
 (0)