Skip to content

Commit 54683d7

Browse files
author
Theodore Li
committed
Address comments
1 parent 6241ca9 commit 54683d7

File tree

11 files changed

+76
-24
lines changed

11 files changed

+76
-24
lines changed

apps/sim/app/api/auth/oauth/credentials/route.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -164,11 +164,14 @@ export async function GET(request: NextRequest) {
164164

165165
if (platformCredential) {
166166
if (platformCredential.type === 'service_account') {
167-
if (workflowId) {
168-
if (!effectiveWorkspaceId || platformCredential.workspaceId !== effectiveWorkspaceId) {
169-
return NextResponse.json({ error: 'Forbidden' }, { status: 403 })
170-
}
171-
} else {
167+
if (
168+
workflowId &&
169+
(!effectiveWorkspaceId || platformCredential.workspaceId !== effectiveWorkspaceId)
170+
) {
171+
return NextResponse.json({ error: 'Forbidden' }, { status: 403 })
172+
}
173+
174+
if (!workflowId) {
172175
const [membership] = await db
173176
.select({ id: credentialMember.id })
174177
.from(credentialMember)

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ const {
1111
mockGetCredential,
1212
mockRefreshTokenIfNeeded,
1313
mockGetOAuthToken,
14+
mockResolveOAuthAccountId,
15+
mockGetServiceAccountToken,
1416
mockAuthorizeCredentialUse,
1517
mockCheckSessionOrInternalAuth,
1618
mockLogger,
@@ -29,6 +31,8 @@ const {
2931
mockGetCredential: vi.fn(),
3032
mockRefreshTokenIfNeeded: vi.fn(),
3133
mockGetOAuthToken: vi.fn(),
34+
mockResolveOAuthAccountId: vi.fn(),
35+
mockGetServiceAccountToken: vi.fn(),
3236
mockAuthorizeCredentialUse: vi.fn(),
3337
mockCheckSessionOrInternalAuth: vi.fn(),
3438
mockLogger: logger,
@@ -40,6 +44,8 @@ vi.mock('@/app/api/auth/oauth/utils', () => ({
4044
getCredential: mockGetCredential,
4145
refreshTokenIfNeeded: mockRefreshTokenIfNeeded,
4246
getOAuthToken: mockGetOAuthToken,
47+
resolveOAuthAccountId: mockResolveOAuthAccountId,
48+
getServiceAccountToken: mockGetServiceAccountToken,
4349
}))
4450

4551
vi.mock('@sim/logger', () => ({
@@ -50,6 +56,10 @@ vi.mock('@/lib/auth/credential-access', () => ({
5056
authorizeCredentialUse: mockAuthorizeCredentialUse,
5157
}))
5258

59+
vi.mock('@/lib/core/utils/request', () => ({
60+
generateRequestId: vi.fn().mockReturnValue('test-request-id'),
61+
}))
62+
5363
vi.mock('@/lib/auth/hybrid', () => ({
5464
AuthType: { SESSION: 'session', API_KEY: 'api_key', INTERNAL_JWT: 'internal_jwt' },
5565
checkHybridAuth: vi.fn(),
@@ -62,6 +72,7 @@ import { GET, POST } from '@/app/api/auth/oauth/token/route'
6272
describe('OAuth Token API Routes', () => {
6373
beforeEach(() => {
6474
vi.clearAllMocks()
75+
mockResolveOAuthAccountId.mockResolvedValue(null)
6576
})
6677

6778
/**

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,10 +140,9 @@ export async function POST(request: NextRequest) {
140140
}
141141

142142
try {
143-
const defaultScopes = ['https://www.googleapis.com/auth/cloud-platform']
144143
const accessToken = await getServiceAccountToken(
145144
resolved.credentialId,
146-
scopes && scopes.length > 0 ? scopes : defaultScopes,
145+
scopes ?? [],
147146
impersonateEmail
148147
)
149148
return NextResponse.json({ accessToken }, { status: 200 })

apps/sim/app/api/auth/oauth/utils.test.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,12 @@ describe('OAuth Utils', () => {
160160

161161
describe('refreshAccessTokenIfNeeded', () => {
162162
it('should return valid access token without refresh if not expired', async () => {
163+
const mockResolvedCredential = {
164+
id: 'credential-id',
165+
type: 'oauth',
166+
accountId: 'account-id',
167+
workspaceId: 'workspace-id',
168+
}
163169
const mockCredentialRow = { type: 'oauth', accountId: 'account-id' }
164170
const mockAccountRow = {
165171
id: 'account-id',
@@ -169,6 +175,7 @@ describe('OAuth Utils', () => {
169175
providerId: 'google',
170176
userId: 'test-user-id',
171177
}
178+
mockSelectChain([mockResolvedCredential])
172179
mockSelectChain([mockCredentialRow])
173180
mockSelectChain([mockAccountRow])
174181

@@ -179,6 +186,12 @@ describe('OAuth Utils', () => {
179186
})
180187

181188
it('should refresh token when expired', async () => {
189+
const mockResolvedCredential = {
190+
id: 'credential-id',
191+
type: 'oauth',
192+
accountId: 'account-id',
193+
workspaceId: 'workspace-id',
194+
}
182195
const mockCredentialRow = { type: 'oauth', accountId: 'account-id' }
183196
const mockAccountRow = {
184197
id: 'account-id',
@@ -188,6 +201,7 @@ describe('OAuth Utils', () => {
188201
providerId: 'google',
189202
userId: 'test-user-id',
190203
}
204+
mockSelectChain([mockResolvedCredential])
191205
mockSelectChain([mockCredentialRow])
192206
mockSelectChain([mockAccountRow])
193207
mockUpdateChain()
@@ -215,6 +229,12 @@ describe('OAuth Utils', () => {
215229
})
216230

217231
it('should return null if refresh fails', async () => {
232+
const mockResolvedCredential = {
233+
id: 'credential-id',
234+
type: 'oauth',
235+
accountId: 'account-id',
236+
workspaceId: 'workspace-id',
237+
}
218238
const mockCredentialRow = { type: 'oauth', accountId: 'account-id' }
219239
const mockAccountRow = {
220240
id: 'account-id',
@@ -224,6 +244,7 @@ describe('OAuth Utils', () => {
224244
providerId: 'google',
225245
userId: 'test-user-id',
226246
}
247+
mockSelectChain([mockResolvedCredential])
227248
mockSelectChain([mockCredentialRow])
228249
mockSelectChain([mockAccountRow])
229250

apps/sim/app/api/auth/oauth/utils.ts

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -90,14 +90,24 @@ export async function resolveOAuthAccountId(
9090
}
9191

9292
/**
93-
* Generates a short-lived access token for a Google service account credential
94-
* using the two-legged OAuth JWT flow (RFC 7523).
93+
* Userinfo scopes are excluded because service accounts don't represent a user
94+
* and cannot request user identity information. Google rejects token requests
95+
* that include these scopes for service account credentials.
9596
*/
9697
const SA_EXCLUDED_SCOPES = new Set([
9798
'https://www.googleapis.com/auth/userinfo.email',
9899
'https://www.googleapis.com/auth/userinfo.profile',
99100
])
100101

102+
/**
103+
* Generates a short-lived access token for a Google service account credential
104+
* using the two-legged OAuth JWT flow (RFC 7523).
105+
*
106+
* @param impersonateEmail - Optional. Required for Google Workspace APIs (Gmail, Drive, Calendar, etc.)
107+
* where the service account must impersonate a domain user via domain-wide delegation.
108+
* Not needed for project-scoped APIs like BigQuery or Vertex AI where the service account
109+
* authenticates directly with its own IAM permissions.
110+
*/
101111
export async function getServiceAccountToken(
102112
credentialId: string,
103113
scopes: string[],
@@ -348,11 +358,11 @@ export async function refreshAccessTokenIfNeeded(
348358
}
349359

350360
if (resolved.credentialType === 'service_account' && resolved.credentialId) {
351-
const effectiveScopes = scopes?.length
352-
? scopes
353-
: ['https://www.googleapis.com/auth/cloud-platform']
361+
if (!scopes?.length) {
362+
throw new Error('Scopes are required for service account credentials')
363+
}
354364
logger.info(`[${requestId}] Using service account token for credential`)
355-
return getServiceAccountToken(resolved.credentialId, effectiveScopes, impersonateEmail)
365+
return getServiceAccountToken(resolved.credentialId, scopes, impersonateEmail)
356366
}
357367

358368
// Get the credential directly using the getCredential helper

apps/sim/app/api/credentials/[id]/route.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -129,9 +129,9 @@ export async function PUT(request: NextRequest, { params }: { params: Promise<{
129129
const parsed = JSON.parse(parseResult.data.serviceAccountJson)
130130
if (
131131
parsed.type !== 'service_account' ||
132-
!parsed.client_email ||
133-
!parsed.private_key ||
134-
!parsed.project_id
132+
typeof parsed.client_email !== 'string' ||
133+
typeof parsed.private_key !== 'string' ||
134+
typeof parsed.project_id !== 'string'
135135
) {
136136
return NextResponse.json({ error: 'Invalid service account JSON key' }, { status: 400 })
137137
}
@@ -166,6 +166,12 @@ export async function PUT(request: NextRequest, { params }: { params: Promise<{
166166
const row = await getCredentialResponse(id, session.user.id)
167167
return NextResponse.json({ credential: row }, { status: 200 })
168168
} catch (error) {
169+
if (error instanceof Error && error.message.includes('unique')) {
170+
return NextResponse.json(
171+
{ error: 'A service account credential with this name already exists in the workspace' },
172+
{ status: 409 }
173+
)
174+
}
169175
logger.error('Failed to update credential', error)
170176
return NextResponse.json({ error: 'Internal server error' }, { status: 500 })
171177
}

apps/sim/app/api/tools/gmail/label/route.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { type NextRequest, NextResponse } from 'next/server'
66
import { getSession } from '@/lib/auth'
77
import { validateAlphanumericId } from '@/lib/core/security/input-validation'
88
import { generateRequestId } from '@/lib/core/utils/request'
9+
import { getScopesForService } from '@/lib/oauth/utils'
910
import {
1011
getServiceAccountToken,
1112
refreshAccessTokenIfNeeded,
@@ -69,7 +70,7 @@ export async function GET(request: NextRequest) {
6970
if (resolved.credentialType === 'service_account' && resolved.credentialId) {
7071
accessToken = await getServiceAccountToken(
7172
resolved.credentialId,
72-
['https://www.googleapis.com/auth/gmail.labels'],
73+
getScopesForService('gmail'),
7374
impersonateEmail
7475
)
7576
} else {

apps/sim/app/api/tools/gmail/labels/route.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { type NextRequest, NextResponse } from 'next/server'
66
import { getSession } from '@/lib/auth'
77
import { validateAlphanumericId } from '@/lib/core/security/input-validation'
88
import { generateRequestId } from '@/lib/core/utils/request'
9+
import { getScopesForService } from '@/lib/oauth/utils'
910
import {
1011
getServiceAccountToken,
1112
refreshAccessTokenIfNeeded,
@@ -73,7 +74,7 @@ export async function GET(request: NextRequest) {
7374
if (resolved.credentialType === 'service_account' && resolved.credentialId) {
7475
accessToken = await getServiceAccountToken(
7576
resolved.credentialId,
76-
['https://www.googleapis.com/auth/gmail.labels'],
77+
getScopesForService('gmail'),
7778
impersonateEmail
7879
)
7980
} else {

apps/sim/app/workspace/[workspaceId]/settings/components/integrations/integrations-manager.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1149,7 +1149,9 @@ export function IntegrationsManager() {
11491149
onClick={handleConfirmDelete}
11501150
disabled={disconnectOAuthService.isPending || deleteCredential.isPending}
11511151
>
1152-
{disconnectOAuthService.isPending ? 'Disconnecting...' : 'Disconnect'}
1152+
{disconnectOAuthService.isPending || deleteCredential.isPending
1153+
? 'Disconnecting...'
1154+
: 'Disconnect'}
11531155
</Button>
11541156
</ModalFooter>
11551157
</ModalContent>

apps/sim/lib/auth/credential-access.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,8 @@ export async function authorizeCredentialUse(
9999
if (requesterPerm === null) {
100100
return { ok: false, error: 'You do not have access to this workspace.' }
101101
}
102+
} else if (!workflowContext) {
103+
return { ok: false, error: 'workflowId is required' }
102104
}
103105

104106
return {

0 commit comments

Comments
 (0)