Skip to content

Commit c4bc8a2

Browse files
committed
refactor(oauth): apply simplify-pass cleanups to the inlined token resolution
1 parent 8e7e197 commit c4bc8a2

15 files changed

Lines changed: 116 additions & 200 deletions

File tree

‎apps/sim/app/api/auth/oauth/token/route.ts‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -132,12 +132,10 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
132132
callerUserId,
133133
auditRequest: request,
134134
authenticate: () => checkSessionOrInternalAuth(request, { requireWorkflowId: false }),
135-
...(managedOAuthDelegation
136-
? {
137-
resolveManagedPrincipal: (managedCredentialId: string) =>
138-
authenticateManagedOAuthDelegation(managedOAuthDelegation, managedCredentialId),
139-
}
140-
: {}),
135+
resolveManagedPrincipal: managedOAuthDelegation
136+
? (managedCredentialId: string) =>
137+
authenticateManagedOAuthDelegation(managedOAuthDelegation, managedCredentialId)
138+
: undefined,
141139
})
142140

143141
if (!result.ok) {

‎apps/sim/executor/utils/credential-token.ts‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -53,22 +53,20 @@ export async function resolveExecutorCredentialToken(
5353
callerUserId: userId && params.enforceCredentialAccess ? userId : undefined,
5454
authenticate: () => ({
5555
success: true,
56-
...(userId ? { userId } : {}),
56+
userId,
5757
authType: AuthType.INTERNAL_JWT,
5858
}),
59-
...(executorDelegationOrigin
60-
? {
61-
resolveManagedPrincipal: (managedCredentialId: string) =>
62-
bindExecutorManagedOAuthDelegation(executorDelegationOrigin, managedCredentialId),
63-
}
64-
: {}),
59+
resolveManagedPrincipal: executorDelegationOrigin
60+
? (managedCredentialId: string) =>
61+
bindExecutorManagedOAuthDelegation(executorDelegationOrigin, managedCredentialId)
62+
: undefined,
6563
})
6664

6765
if (!result.ok) {
6866
logger.error(`[${requestId}] Credential token resolution failed`, {
6967
status: result.status,
7068
credentialId,
71-
...(result.code ? { code: result.code } : {}),
69+
code: result.code,
7270
})
7371
throw new Error(
7472
`Failed to obtain credential for ${params.toolLabel ?? credentialId}: ${result.error}`

‎apps/sim/executor/utils/http.test.ts‎

Lines changed: 2 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -4,66 +4,30 @@
44
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
55

66
const mocks = vi.hoisted(() => ({
7-
generateInternalDelegationToken: vi.fn(),
87
generateInternalToken: vi.fn(),
98
}))
109

1110
vi.mock('@/lib/auth/internal', () => ({
12-
generateInternalDelegationToken: mocks.generateInternalDelegationToken,
1311
generateInternalToken: mocks.generateInternalToken,
1412
}))
1513

16-
import { buildAuthHeaders, buildExecutorDelegationHeaders } from '@/executor/utils/http'
14+
import { buildAuthHeaders } from '@/executor/utils/http'
1715

1816
describe('executor HTTP authentication headers', () => {
1917
beforeEach(() => {
2018
vi.clearAllMocks()
21-
mocks.generateInternalDelegationToken.mockResolvedValue('delegation-token')
2219
mocks.generateInternalToken.mockResolvedValue('legacy-token')
2320
})
2421

2522
afterEach(() => {
2623
vi.unstubAllGlobals()
2724
})
2825

29-
it('issues a workflow-scoped executor delegation', async () => {
30-
await expect(
31-
buildExecutorDelegationHeaders({
32-
subjectUserId: 'user-1',
33-
workflowId: 'workflow-1',
34-
executionId: 'execution-1',
35-
})
36-
).resolves.toEqual({
37-
'Content-Type': 'application/json',
38-
Authorization: 'Bearer delegation-token',
39-
})
40-
41-
expect(mocks.generateInternalDelegationToken).toHaveBeenCalledWith({
42-
subjectUserId: 'user-1',
43-
workflowId: 'workflow-1',
44-
executionId: 'execution-1',
45-
})
46-
expect(mocks.generateInternalToken).not.toHaveBeenCalled()
47-
})
48-
49-
it('fails instead of issuing trusted delegation headers in a browser', async () => {
50-
vi.stubGlobal('window', {})
51-
52-
await expect(
53-
buildExecutorDelegationHeaders({
54-
subjectUserId: 'user-1',
55-
workflowId: 'workflow-1',
56-
})
57-
).rejects.toThrow('Executor delegation headers can only be created on the server')
58-
expect(mocks.generateInternalDelegationToken).not.toHaveBeenCalled()
59-
})
60-
61-
it('keeps the legacy helper separate during endpoint migration', async () => {
26+
it('mints an internal token for server-side calls', async () => {
6227
await expect(buildAuthHeaders('user-1')).resolves.toEqual({
6328
'Content-Type': 'application/json',
6429
Authorization: 'Bearer legacy-token',
6530
})
6631
expect(mocks.generateInternalToken).toHaveBeenCalledWith('user-1')
67-
expect(mocks.generateInternalDelegationToken).not.toHaveBeenCalled()
6832
})
6933
})

‎apps/sim/executor/utils/http.ts‎

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,7 @@
1-
import {
2-
type GenerateInternalDelegationTokenInput,
3-
generateInternalDelegationToken,
4-
generateInternalToken,
5-
} from '@/lib/auth/internal'
1+
import { generateInternalToken } from '@/lib/auth/internal'
62
import { getBaseUrl, getInternalApiBaseUrl } from '@/lib/core/utils/urls'
73
import { HTTP } from '@/executor/constants'
84

9-
/** @deprecated Use `buildExecutorDelegationHeaders` for protected application routes. */
105
export async function buildAuthHeaders(userId?: string): Promise<Record<string, string>> {
116
const headers: Record<string, string> = {
127
'Content-Type': HTTP.CONTENT_TYPE.JSON,
@@ -20,21 +15,6 @@ export async function buildAuthHeaders(userId?: string): Promise<Record<string,
2015
return headers
2116
}
2217

23-
/** Builds server-only headers for an executor call bound to its workflow execution origin. */
24-
export async function buildExecutorDelegationHeaders(
25-
input: GenerateInternalDelegationTokenInput
26-
): Promise<Record<string, string>> {
27-
if (typeof window !== 'undefined') {
28-
throw new Error('Executor delegation headers can only be created on the server')
29-
}
30-
31-
const token = await generateInternalDelegationToken(input)
32-
return {
33-
'Content-Type': HTTP.CONTENT_TYPE.JSON,
34-
Authorization: `Bearer ${token}`,
35-
}
36-
}
37-
3818
export function buildAPIUrl(path: string, params?: Record<string, string>): URL {
3919
const baseUrl = path.startsWith('/api/') ? getInternalApiBaseUrl() : getBaseUrl()
4020
const url = new URL(path, baseUrl)

‎apps/sim/lib/credentials/application/managed-oauth-delegation.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,8 @@ export async function authenticateManagedOAuthDelegation(
4545
/**
4646
* In-process sibling of {@link authenticateManagedOAuthDelegation}: binds the
4747
* executor's own delegation origin to one managed credential without minting and
48-
* re-verifying a delegation JWT. The binding still re-validates the workflow and
49-
* deployment context, so trust matches the wire path minus the signature check,
50-
* which proves nothing when the executor is the caller.
48+
* re-verifying a delegation JWT — see {@link createExecutorPrincipalFromDelegationOrigin}
49+
* for why that loses nothing.
5150
*/
5251
export async function bindExecutorManagedOAuthDelegation(
5352
origin: ExecutorDelegationOrigin,

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ vi.mock('@/lib/oauth/terminal-errors', () => ({
6464
markCredentialDead: vi.fn(),
6565
}))
6666

67-
import { resolveCredentialAccessToken } from '@/lib/oauth/credential-service'
67+
import { resolveCredentialTokenBundle } from '@/lib/oauth/credential-service'
6868

6969
const RAW_CREDENTIAL_ID = 'credential-raw-secret-id'
7070
const RAW_ACCOUNT_ID = 'account-raw-secret-id'
@@ -127,7 +127,7 @@ async function observeRefresh(
127127
])
128128

129129
await expect(
130-
resolveCredentialAccessToken(
130+
resolveCredentialTokenBundle(
131131
RAW_CREDENTIAL_ID,
132132
RAW_USER_ID,
133133
'selector-execution',
@@ -149,7 +149,7 @@ async function observeRefresh(
149149
}
150150
}
151151

152-
describe('resolveCredentialAccessToken selector privacy', () => {
152+
describe('resolveCredentialTokenBundle selector privacy', () => {
153153
beforeEach(() => {
154154
vi.clearAllMocks()
155155
resetDbChainMock()

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1028,7 +1028,7 @@ export async function getOAuthToken(userId: string, providerId: string): Promise
10281028
* Pipedrive's `x-api-token`. OAuth credentials resolve with `accessToken`
10291029
* only.
10301030
*/
1031-
export async function resolveCredentialAccessToken(
1031+
export async function resolveCredentialTokenBundle(
10321032
credentialId: string,
10331033
userId: string,
10341034
requestId: string,
@@ -1131,7 +1131,7 @@ export async function resolveCredentialAccessToken(
11311131
/**
11321132
* Refreshes an OAuth token if needed based on credential information.
11331133
* Also handles service account credentials by generating a JWT-based token.
1134-
* Thin string wrapper over {@link resolveCredentialAccessToken}.
1134+
* Thin string wrapper over {@link resolveCredentialTokenBundle}.
11351135
* @param credentialId The ID of the credential to check and potentially refresh
11361136
* @param userId The user ID who owns the credential (for security verification)
11371137
* @param requestId Request ID for log correlation
@@ -1146,7 +1146,7 @@ export async function refreshAccessTokenIfNeeded(
11461146
impersonateEmail?: string,
11471147
options?: CredentialTokenResolutionOptions
11481148
): Promise<string | null> {
1149-
const result = await resolveCredentialAccessToken(
1149+
const result = await resolveCredentialTokenBundle(
11501150
credentialId,
11511151
userId,
11521152
requestId,

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

Lines changed: 45 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,6 @@ const INTERNAL_AUTH = { success: true, userId: 'user-1', authType: 'internal_jwt
9292
describe('resolveCredentialToken', () => {
9393
beforeEach(() => {
9494
vi.clearAllMocks()
95-
mockResolveOAuthAccountId.mockResolvedValue(null)
9695
})
9796

9897
it('fails closed when the credential is not authorized', async () => {
@@ -103,6 +102,7 @@ describe('resolveCredentialToken', () => {
103102

104103
const result = await resolveCredentialToken(INTERNAL_AUTH, {
105104
requestId: 'req-1',
105+
resolvedCredential: null,
106106
credentialId: 'cred-1',
107107
})
108108

@@ -124,7 +124,7 @@ describe('resolveCredentialToken', () => {
124124

125125
const result = await resolveCredentialToken(
126126
{ success: true, authType: 'internal_jwt' },
127-
{ requestId: 'req-1', credentialId: 'cred-1' }
127+
{ requestId: 'req-1', credentialId: 'cred-1', resolvedCredential: null }
128128
)
129129

130130
expect(result).toEqual({ ok: false, status: 403, error: 'Authentication required' })
@@ -147,6 +147,7 @@ describe('resolveCredentialToken', () => {
147147

148148
const result = await resolveCredentialToken(INTERNAL_AUTH, {
149149
requestId: 'req-1',
150+
resolvedCredential: null,
150151
credentialId: 'cred-1',
151152
workflowId: 'wf-1',
152153
})
@@ -177,6 +178,7 @@ describe('resolveCredentialToken', () => {
177178

178179
const result = await resolveCredentialToken(INTERNAL_AUTH, {
179180
requestId: 'req-1',
181+
resolvedCredential: null,
180182
credentialId: 'cred-1',
181183
})
182184

@@ -201,6 +203,7 @@ describe('resolveCredentialToken', () => {
201203
await expect(
202204
resolveCredentialToken(INTERNAL_AUTH, {
203205
requestId: 'req-1',
206+
resolvedCredential: null,
204207
credentialId: 'cred-1',
205208
})
206209
).resolves.toEqual({
@@ -231,6 +234,7 @@ describe('resolveCredentialToken', () => {
231234
await expect(
232235
resolveCredentialToken(INTERNAL_AUTH, {
233236
requestId: 'req-1',
237+
resolvedCredential: null,
234238
credentialId: 'cred-1',
235239
})
236240
).resolves.toEqual({
@@ -251,6 +255,7 @@ describe('resolveCredentialToken', () => {
251255

252256
const result = await resolveCredentialToken(INTERNAL_AUTH, {
253257
requestId: 'req-1',
258+
resolvedCredential: null,
254259
credentialId: 'cred-1',
255260
})
256261

@@ -259,34 +264,26 @@ describe('resolveCredentialToken', () => {
259264
})
260265

261266
it('authorizes service-account credentials before minting a token', async () => {
262-
mockResolveOAuthAccountId.mockResolvedValue({
263-
credentialType: 'service_account',
264-
credentialId: 'sa-1',
265-
providerId: 'google',
266-
workspaceId: 'ws-1',
267-
accountId: '',
268-
usedCredentialTable: true,
269-
})
270267
mockAuthorizeCredentialUseForAuth.mockResolvedValue({ ok: false, error: 'Unauthorized' })
271268

272269
const result = await resolveCredentialToken(INTERNAL_AUTH, {
273270
requestId: 'req-1',
274271
credentialId: 'cred-1',
272+
resolvedCredential: {
273+
credentialType: 'service_account',
274+
credentialId: 'sa-1',
275+
providerId: 'google',
276+
workspaceId: 'ws-1',
277+
accountId: '',
278+
usedCredentialTable: true,
279+
},
275280
})
276281

277282
expect(result).toEqual({ ok: false, status: 403, error: 'Unauthorized' })
278283
expect(mockResolveServiceAccountToken).not.toHaveBeenCalled()
279284
})
280285

281286
it('surfaces the classified service-account failure code', async () => {
282-
mockResolveOAuthAccountId.mockResolvedValue({
283-
credentialType: 'service_account',
284-
credentialId: 'sa-1',
285-
providerId: 'atlassian',
286-
workspaceId: 'ws-1',
287-
accountId: '',
288-
usedCredentialTable: true,
289-
})
290287
mockAuthorizeCredentialUseForAuth.mockResolvedValue({ ok: true, requesterUserId: 'user-1' })
291288
mockResolveServiceAccountToken.mockRejectedValue(
292289
new TokenServiceAccountValidationError('invalid_credentials', 401)
@@ -295,6 +292,14 @@ describe('resolveCredentialToken', () => {
295292
const result = await resolveCredentialToken(INTERNAL_AUTH, {
296293
requestId: 'req-1',
297294
credentialId: 'cred-1',
295+
resolvedCredential: {
296+
credentialType: 'service_account',
297+
credentialId: 'sa-1',
298+
providerId: 'atlassian',
299+
workspaceId: 'ws-1',
300+
accountId: '',
301+
usedCredentialTable: true,
302+
},
298303
})
299304

300305
expect(result).toEqual({
@@ -308,6 +313,7 @@ describe('resolveCredentialToken', () => {
308313
it('rejects a malformed impersonation subject before touching the credential', async () => {
309314
const result = await resolveCredentialToken(INTERNAL_AUTH, {
310315
requestId: 'req-1',
316+
resolvedCredential: null,
311317
credentialId: 'cred-1',
312318
impersonateEmail: 'not-an-email',
313319
})
@@ -378,6 +384,27 @@ describe('resolveCredentialAccessToken', () => {
378384
})
379385
})
380386

387+
it('treats an empty impersonation subject as absent', async () => {
388+
mockAuthorizeCredentialUseForAuth.mockResolvedValue({
389+
ok: true,
390+
requesterUserId: 'user-1',
391+
credentialOwnerUserId: 'owner-1',
392+
workspaceId: 'ws-1',
393+
resolvedCredentialId: 'account-1',
394+
})
395+
mockGetCredential.mockResolvedValue({ providerId: 'google' })
396+
mockRefreshTokenIfNeeded.mockResolvedValue({ accessToken: 'fresh', refreshed: false })
397+
398+
const result = await resolveCredentialAccessToken({
399+
requestId: 'req-1',
400+
credentialId: 'cred-1',
401+
impersonateEmail: '',
402+
authenticate,
403+
})
404+
405+
expect(result).toEqual({ ok: true, token: { accessToken: 'fresh', idToken: undefined } })
406+
})
407+
381408
it('rejects a managed credential when no delegation resolver is wired', async () => {
382409
mockResolveOAuthAccountId.mockResolvedValue(MANAGED_RESOLVED)
383410

0 commit comments

Comments
 (0)