Skip to content

Commit cc92806

Browse files
waleedlatif1waleedlatif1
committed
fix(knowledge): Drive field mask named a permission field that does not exist
`permittedBy` is not a field of the Permission resource — inheritance lives in `permissionDetails` — and Drive answers an invalid field selection with 400. The mask was unconditional, so every `files.list` on this branch failed, in every access mode, and the suite passed only because every Drive call is mocked and the assertion checked a prefix of the mask. Caught by a reviewer checking the mask against the API reference. Also from that review: `groups.list?domain=` enumerates one domain, while a Workspace customer routinely owns several — a grant to a group on a secondary domain named a group the directory never listed, readable by nobody; it is `customer=my_customer` now. A deleted account's grant no longer mints a token for whoever is later provisioned with the recycled address. The Admin SDK calls share the Drive retry wrapper, so one transient error no longer stales a group. And the `permissionIds` count heuristic is gone: the field counts users only, so any file with a domain or public grant tripped a needless fallback; absence of `permissions` is the one signal that matters, and it is kept.
1 parent 8ca4ff1 commit cc92806

7 files changed

Lines changed: 105 additions & 108 deletions

File tree

‎apps/sim/connectors/google-drive/directory.test.ts‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -38,13 +38,13 @@ describe('listDomainGroups', () => {
3838
it('folds group emails so they match the tokens a crawl writes', async () => {
3939
directory({}, [{ email: 'Eng@Corp.com', name: 'Engineering' }])
4040

41-
await expect(listDomainGroups('token', 'corp.com')).resolves.toEqual([{ id: 'eng@corp.com' }])
41+
await expect(listDomainGroups('token')).resolves.toEqual([{ id: 'eng@corp.com' }])
4242
})
4343

4444
it('drops a group with no email, which is the only identifier a grant carries', async () => {
4545
directory({}, [{ name: 'Nameless' }, { email: 'eng@corp.com' }])
4646

47-
await expect(listDomainGroups('token', 'corp.com')).resolves.toEqual([{ id: 'eng@corp.com' }])
47+
await expect(listDomainGroups('token')).resolves.toEqual([{ id: 'eng@corp.com' }])
4848
})
4949

5050
it('follows pagination rather than reporting the first page as the whole directory', async () => {
@@ -54,13 +54,14 @@ describe('listDomainGroups', () => {
5454
)
5555
.mockResolvedValueOnce(jsonResponse({ groups: [{ email: 'b@corp.com' }] }))
5656

57-
await expect(listDomainGroups('token', 'corp.com')).resolves.toHaveLength(2)
57+
await expect(listDomainGroups('token')).resolves.toHaveLength(2)
5858
})
5959

6060
it('throws rather than returning a truncated directory', async () => {
61-
mockFetch.mockResolvedValueOnce(jsonResponse({ error: 'nope' }, 403))
61+
mockFetch.mockReset()
62+
mockFetch.mockResolvedValueOnce(jsonResponse({ error: { message: 'forbidden' } }, 403))
6263

63-
await expect(listDomainGroups('token', 'corp.com')).rejects.toThrow('403')
64+
await expect(listDomainGroups('token')).rejects.toThrow()
6465
})
6566
})
6667

@@ -138,8 +139,25 @@ describe('listGroupMembers', () => {
138139
})
139140

140141
it('throws when a group cannot be read, so its membership is left alone', async () => {
141-
mockFetch.mockResolvedValueOnce(jsonResponse({ error: 'nope' }, 500))
142+
mockFetch.mockReset()
143+
mockFetch.mockResolvedValueOnce(jsonResponse({ error: { message: 'gone' } }, 404))
142144

143-
await expect(listGroupMembers('token', GROUP)).rejects.toThrow('500')
145+
await expect(listGroupMembers('token', GROUP)).rejects.toThrow()
146+
})
147+
148+
/** A directory that hiccups must not cost a group its membership; transient errors are retried. */
149+
it('retries a transient directory error before giving up', async () => {
150+
mockFetch.mockReset()
151+
mockFetch
152+
.mockResolvedValueOnce(
153+
jsonResponse({ error: { errors: [{ reason: 'backendError' }], message: 'try again' } }, 503)
154+
)
155+
.mockResolvedValueOnce(jsonResponse({ members: [USER('alice@corp.com')] }))
156+
157+
await expect(listGroupMembers('token', GROUP)).resolves.toMatchObject({
158+
memberEmails: ['alice@corp.com'],
159+
complete: true,
160+
})
161+
expect(mockFetch).toHaveBeenCalledTimes(2)
144162
})
145163
})

‎apps/sim/connectors/google-drive/directory.ts‎

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { createLogger } from '@sim/logger'
22
import { normalizeEmail } from '@sim/utils/string'
33
import { canonicalGroupId } from '@/lib/knowledge/access/tokens'
4+
import { fetchGoogleDriveWithRetry } from '@/connectors/google-drive/google-drive-errors'
45
import type {
56
ConnectorDirectory,
67
ConnectorDirectoryGroup,
@@ -46,7 +47,8 @@ interface DirectoryListResponse<T> {
4647
}
4748

4849
/**
49-
* Reads a paginated Admin SDK collection, following `nextPageToken`.
50+
* Reads a paginated Admin SDK collection, following `nextPageToken`, with the
51+
* same transient-error retry every Drive call gets.
5052
*
5153
* Throws rather than returning what it managed to read: every caller here is
5254
* building a membership set that is only meaningful in full, and a truncated
@@ -64,15 +66,10 @@ async function listAll<T>(
6466
const query = new URLSearchParams({ ...params, maxResults: String(PAGE_SIZE) })
6567
if (pageToken) query.set('pageToken', pageToken)
6668

67-
const response = await fetch(`${url}?${query.toString()}`, {
69+
const response = await fetchGoogleDriveWithRetry(`${url}?${query.toString()}`, {
6870
method: 'GET',
6971
headers: { Authorization: `Bearer ${accessToken}`, Accept: 'application/json' },
7072
})
71-
if (!response.ok) {
72-
throw new Error(
73-
`Google Directory request failed: ${response.status} ${response.statusText} (${itemsKey})`
74-
)
75-
}
7673

7774
const body = (await response.json()) as DirectoryListResponse<T> & Record<string, unknown>
7875
const pageItems = (body[itemsKey] as T[] | undefined) ?? []
@@ -94,12 +91,17 @@ interface RawMember {
9491
status?: string
9592
}
9693

97-
/** Every group in the Workspace domain the crawl is scoped to. */
98-
export async function listDomainGroups(
99-
accessToken: string,
100-
domain: string
101-
): Promise<ConnectorDirectoryGroup[]> {
102-
const raw = await listAll<RawGroup>(`${DIRECTORY_BASE}/groups`, accessToken, 'groups', { domain })
94+
/**
95+
* Every group in the Workspace customer the administrator belongs to.
96+
*
97+
* `customer=my_customer` rather than `domain=`: a Workspace customer routinely
98+
* owns several domains, and a grant to a group on a secondary domain would
99+
* otherwise name a group the directory never enumerated — readable by nobody.
100+
*/
101+
export async function listDomainGroups(accessToken: string): Promise<ConnectorDirectoryGroup[]> {
102+
const raw = await listAll<RawGroup>(`${DIRECTORY_BASE}/groups`, accessToken, 'groups', {
103+
customer: 'my_customer',
104+
})
103105
const groups: ConnectorDirectoryGroup[] = []
104106
for (const group of raw) {
105107
const id = group.email ? canonicalGroupId(group.email) : ''
@@ -171,7 +173,7 @@ export function openGoogleDirectory(
171173
if (!tenantId) return null
172174
return {
173175
tenantId,
174-
listGroups: () => listDomainGroups(accessToken, tenantId),
176+
listGroups: () => listDomainGroups(accessToken),
175177
listGroupMembers: (group) => listGroupMembers(accessToken, group),
176178
}
177179
}

‎apps/sim/connectors/google-drive/google-drive-errors.ts‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,10 @@
1+
import {
2+
attachRetryHeaders,
3+
isRetryableError,
4+
type RetryOptions,
5+
resolveRetryDelayMs,
6+
retryWithExponentialBackoff,
7+
} from '@/lib/knowledge/documents/utils'
18
import { readBodyWithLimit } from '@/connectors/utils'
29

310
const GOOGLE_ERROR_BODY_MAX_BYTES = 64 * 1024
@@ -141,3 +148,35 @@ export async function readGoogleDriveApiError(response: Response): Promise<Googl
141148
]
142149
return new GoogleDriveApiError(response.status, normalizedReasons)
143150
}
151+
152+
/**
153+
* Fetches a Google API, retrying errors whose structured body identifies a
154+
* transient rejection. Shared by every Google call a connector makes — Drive
155+
* and the Admin SDK use the same error envelope and the same rate-limit
156+
* reasons.
157+
*/
158+
export async function fetchGoogleDriveWithRetry(
159+
url: string,
160+
options: RequestInit,
161+
retryOptions: RetryOptions = {}
162+
): Promise<Response> {
163+
return retryWithExponentialBackoff(
164+
async () => {
165+
const response = await fetch(url, options)
166+
if (response.ok) return response
167+
168+
const error = await readGoogleDriveApiError(response)
169+
attachRetryHeaders(error, response.headers)
170+
const waitMs = resolveRetryDelayMs(response.headers)
171+
if (waitMs !== undefined) error.retryAfterMs = waitMs
172+
throw error
173+
},
174+
{
175+
...retryOptions,
176+
retryCondition: (error) =>
177+
error instanceof GoogleDriveApiError
178+
? error.kind === 'transient' || isRetryableError(error)
179+
: (retryOptions.retryCondition?.(error) ?? isRetryableError(error)),
180+
}
181+
)
182+
}

‎apps/sim/connectors/google-drive/google-drive.test.ts‎

Lines changed: 0 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -648,7 +648,6 @@ describe('mirroring Drive permissions onto listed documents', () => {
648648

649649
const url = String(mockFetch.mock.calls[0][0])
650650
expect(decodeURIComponent(url)).toContain('permissions(id,type,emailAddress,domain,role,')
651-
expect(decodeURIComponent(url)).toContain('permissionIds')
652651
})
653652

654653
it('tags each document with who may read it', async () => {
@@ -687,35 +686,6 @@ describe('mirroring Drive permissions onto listed documents', () => {
687686
expect(doc.acl).toBeUndefined()
688687
})
689688

690-
/**
691-
* Drive sometimes reports more permission ids than it expands. The subset that
692-
* arrived is not the safe answer — the grants that went missing are the ones
693-
* nobody checked — so the file is left readable by nobody.
694-
*/
695-
it('refuses a partial permission set rather than mirroring a subset', async () => {
696-
const doc = await listWith(
697-
driveFile({
698-
permissionIds: ['p1', 'p2'],
699-
permissions: [{ id: 'p1', type: 'user', emailAddress: 'alice@corp.com' }],
700-
}),
701-
ADMIN
702-
)
703-
704-
expect(doc.acl).toBeUndefined()
705-
})
706-
707-
it('accepts a permission set that matches the ids Drive reported', async () => {
708-
const doc = await listWith(
709-
driveFile({
710-
permissionIds: ['p1'],
711-
permissions: [{ id: 'p1', type: 'user', emailAddress: 'alice@corp.com' }],
712-
}),
713-
ADMIN
714-
)
715-
716-
expect(doc.acl).toEqual(['u:alice@corp.com'])
717-
})
718-
719689
it('keeps an openly shared file out of search until the admin opts in', async () => {
720690
const shared = driveFile({ permissions: [{ id: 'p1', type: 'domain', domain: 'corp.com' }] })
721691

‎apps/sim/connectors/google-drive/google-drive.ts‎

Lines changed: 12 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -7,18 +7,11 @@ import {
77
driveFileAcl,
88
type OpenSharingPolicy,
99
} from '@/lib/knowledge/access/drive-permissions'
10-
import {
11-
attachRetryHeaders,
12-
isRetryableError,
13-
type RetryOptions,
14-
resolveRetryDelayMs,
15-
retryWithExponentialBackoff,
16-
VALIDATE_RETRY_OPTIONS,
17-
} from '@/lib/knowledge/documents/utils'
10+
import { VALIDATE_RETRY_OPTIONS } from '@/lib/knowledge/documents/utils'
1811
import { googleWorkspaceDomain, openGoogleDirectory } from '@/connectors/google-drive/directory'
1912
import {
13+
fetchGoogleDriveWithRetry,
2014
GoogleDriveApiError,
21-
readGoogleDriveApiError,
2215
} from '@/connectors/google-drive/google-drive-errors'
2316
import { googleDriveConnectorMeta } from '@/connectors/google-drive/meta'
2417
import type {
@@ -92,33 +85,6 @@ function isSupportedTextFile(mimeType: string): boolean {
9285
return SUPPORTED_TEXT_MIME_TYPES.some((t) => mimeType.startsWith(t))
9386
}
9487

95-
/** Retries Google errors whose structured body identifies a transient rejection. */
96-
async function fetchGoogleDriveWithRetry(
97-
url: string,
98-
options: RequestInit,
99-
retryOptions: RetryOptions = {}
100-
): Promise<Response> {
101-
return retryWithExponentialBackoff(
102-
async () => {
103-
const response = await fetch(url, options)
104-
if (response.ok) return response
105-
106-
const error = await readGoogleDriveApiError(response)
107-
attachRetryHeaders(error, response.headers)
108-
const waitMs = resolveRetryDelayMs(response.headers)
109-
if (waitMs !== undefined) error.retryAfterMs = waitMs
110-
throw error
111-
},
112-
{
113-
...retryOptions,
114-
retryCondition: (error) =>
115-
error instanceof GoogleDriveApiError
116-
? error.kind === 'transient' || isRetryableError(error)
117-
: (retryOptions.retryCondition?.(error) ?? isRetryableError(error)),
118-
}
119-
)
120-
}
121-
12288
async function exportGoogleWorkspaceFile(
12389
accessToken: string,
12490
fileId: string,
@@ -217,15 +183,12 @@ interface DriveFile {
217183
starred?: boolean
218184
trashed?: boolean
219185
parents?: string[]
220-
permissions?: DrivePermission[]
221186
/**
222-
* Requested alongside `permissions` because Drive sometimes omits the
223-
* expanded permission objects while still reporting their ids — Onyx hit the
224-
* same thing. A file whose two counts disagree has an ACL we did not fully
225-
* see, so it is mirrored as readable by nobody rather than under a subset of
226-
* its real grants.
187+
* Absent for a file on a shared drive, and for any file the impersonated
188+
* administrator cannot share: Drive serves those only through
189+
* `permissions.list`, which {@link resolveDriveAcls} calls for them.
227190
*/
228-
permissionIds?: string[]
191+
permissions?: DrivePermission[]
229192
}
230193

231194
interface DriveChange {
@@ -488,16 +451,12 @@ function driveAclContext(sourceConfig: Record<string, unknown>): DriveAclContext
488451
* The file's mirrored ACL from its listing, or undefined when the listing
489452
* cannot speak for it and {@link resolveDriveAcls} must.
490453
*
491-
* Drive does not populate `permissions` for a file on a shared drive at all,
492-
* and sometimes returns fewer expanded entries than it reports ids for — which
493-
* is why both are requested. Mirroring the subset that did arrive would store
494-
* the file under narrower grants than it really has, which sounds like the
495-
* safe direction but is not, because the grants that went missing are exactly
496-
* the ones nobody verified. Undefined sends the file to `permissions.list`.
454+
* Drive leaves `permissions` unpopulated for a file on a shared drive, and for
455+
* any file the requesting user cannot share. Those go to `permissions.list`,
456+
* the one endpoint that answers for every file.
497457
*/
498458
function fileAcl(file: DriveFile, context: DriveAclContext | null): string[] | undefined {
499459
if (!context || !file.permissions) return undefined
500-
if (file.permissionIds && file.permissionIds.length !== file.permissions.length) return undefined
501460
return driveFileAcl({
502461
permissions: file.permissions,
503462
providerId: context.providerId,
@@ -528,7 +487,8 @@ async function listFilePermissions(
528487
let pageToken: string | undefined
529488
for (let page = 0; page < MAX_PERMISSION_PAGES; page += 1) {
530489
const query = new URLSearchParams({
531-
fields: 'nextPageToken,permissions(id,type,emailAddress,domain,role,allowFileDiscovery)',
490+
fields:
491+
'nextPageToken,permissions(id,type,emailAddress,domain,role,allowFileDiscovery,deleted)',
532492
pageSize: '100',
533493
supportsAllDrives: 'true',
534494
})
@@ -648,7 +608,7 @@ export const googleDriveConnector: ConnectorConfig = {
648608
pageSize: String(effectivePageSize),
649609
orderBy: 'modifiedTime desc',
650610
fields:
651-
'kind,nextPageToken,incompleteSearch,files(id,name,mimeType,modifiedTime,createdTime,webViewLink,owners,size,starred,parents,permissionIds,permissions(id,type,emailAddress,domain,role,allowFileDiscovery,permittedBy))',
611+
'kind,nextPageToken,incompleteSearch,files(id,name,mimeType,modifiedTime,createdTime,webViewLink,owners,size,starred,parents,permissions(id,type,emailAddress,domain,role,allowFileDiscovery,deleted))',
652612
supportsAllDrives: 'true',
653613
includeItemsFromAllDrives: 'true',
654614
})

‎apps/sim/lib/knowledge/access/drive-permissions.test.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,13 @@ describe('driveFileAcl', () => {
4040
).toEqual([`g:${PROVIDER}:${TENANT}:sales@corp.com`, 'u:alice@corp.com', 'u:bob@corp.com'])
4141
})
4242

43-
it('maps an inherited grant like any other, leaving membership to directory sync', () => {
43+
it('drops the grant of a deleted account rather than minting a token for a recycled address', () => {
4444
expect(
45-
acl([{ type: 'group', emailAddress: 'eng@corp.com', permittedBy: ['folder-1'] }])
46-
).toEqual([`g:${PROVIDER}:${TENANT}:eng@corp.com`])
45+
acl([
46+
{ type: 'user', emailAddress: 'gone@corp.com', deleted: true },
47+
{ type: 'user', emailAddress: 'alice@corp.com' },
48+
])
49+
).toEqual(['u:alice@corp.com'])
4750
})
4851

4952
describe('open sharing is closed by default', () => {

‎apps/sim/lib/knowledge/access/drive-permissions.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,8 @@ export interface DrivePermission {
1818
* which is what the Drive API documents.
1919
*/
2020
allowFileDiscovery?: boolean | null
21-
/** Set when the grant descends from a folder or shared drive rather than the file. */
22-
permittedBy?: string[] | null
21+
/** Whether the account behind a `user` grant has been deleted. */
22+
deleted?: boolean | null
2323
}
2424

2525
/**
@@ -82,6 +82,11 @@ export function driveFileAcl(input: DriveAclInput): string[] {
8282
const tokens = new Set<string>()
8383

8484
for (const permission of permissions) {
85+
/**
86+
* A deleted account's grant is a grant to nobody — and to whoever is later
87+
* provisioned with the recycled address, if it were minted.
88+
*/
89+
if (permission.deleted) continue
8590
switch (permission.type) {
8691
case 'user': {
8792
const token = userToken(permission.emailAddress)

0 commit comments

Comments
 (0)