Skip to content

Commit c816f6c

Browse files
fix(s3vectors): return Grant covering all index ARNs in _internalGrant (#1358)
* fix(s3vectors): return Grant covering all index ARNs in _internalGrant When granting on an explicit indexIds array, _internalGrant reassigned indexResult on each loop iteration (keeping only the last index ARN) and then returned `bucketResult ?? indexResult`, so callers received a Grant referencing only the bucket - never the index permissions actually granted. The IAM policy document was always correct (statements are added as a side effect); only the returned Grant handle was wrong, which breaks dependency wiring via grant.applyBefore()/assertSuccess(). - Combine per-index grants with Grant.combine instead of overwriting - Combine the bucket and index grants for the return value - Add a regression test asserting the returned Grant references every specified index ARN, not just the last * test(s3vectors): assert bucket ARN survives combine + parametrize grantRead/Write/Delete Address @krokoko's two review suggestions on the returned-Grant regression test: - Assert the bucket statement survives bucketResult.combine(indexResult), not just the index ARNs (a standalone VectorBucketArn resource without /index/). - Parametrize across grantRead/grantWrite/grantDelete via test.each so all three exercise the multi-index combine path; grantDelete is the exact last-index-only scenario from the bug title. Test-only; the fix in vector-bucket.ts is unchanged. All 3 cases fail on the pre-fix return (bucketResult ?? indexResult) and pass with the combine. --------- Co-authored-by: Alain Krok <alkrok@amazon.com>
1 parent 26ff647 commit c816f6c

2 files changed

Lines changed: 49 additions & 1 deletion

File tree

src/cdk-lib/s3vectors/vector-bucket.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -301,15 +301,18 @@ export abstract class VectorBucketBase extends Resource implements IVectorBucket
301301
resource: this,
302302
});
303303
} else if (Array.isArray(indexIds)) {
304+
// Grant per index and combine the results so the returned Grant
305+
// references every index ARN, not just the last one in the array.
304306
for (const indexId of indexIds) {
305307
const indexArn = `${this.vectorBucketArn}/index/${indexId}`;
306308

307-
indexResult = iam.Grant.addToPrincipalOrResource({
309+
const indexGrant = iam.Grant.addToPrincipalOrResource({
308310
actions: indexActions,
309311
grantee: grantee,
310312
resourceArns: [indexArn],
311313
resource: this,
312314
});
315+
indexResult = indexResult ? indexResult.combine(indexGrant) : indexGrant;
313316
}
314317
} else {
315318
throw new ValidationError(lit`@aws-cdk/s3vectors:VectorBucket`, 'indexIds must be a string (\'*\') or an array of strings', this);
@@ -320,6 +323,11 @@ export abstract class VectorBucketBase extends Resource implements IVectorBucket
320323
this.encryptionKey?.grant(grantee, ...keyActions);
321324
}
322325

326+
// Combine the bucket and index grants so the returned Grant represents all
327+
// permissions granted (bucket + indexes), not only the bucket statement.
328+
if (bucketResult && indexResult) {
329+
return bucketResult.combine(indexResult);
330+
}
323331
return bucketResult ?? indexResult ?? iam.Grant.drop(grantee, 'No actions to grant');
324332
}
325333
}

test/cdk-lib/s3vectors/vector-bucket.test.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -623,6 +623,46 @@ describe('VectorBucket', () => {
623623
});
624624
});
625625

626+
// The returned Grant object is used by callers for dependency wiring
627+
// (grant.applyBefore(...), grant.assertSuccess(), inspecting statements).
628+
// It must represent ALL index ARNs that were granted - not only the last
629+
// index in the array - AND the bucket statement, which is the second half
630+
// of the fix (bucketResult.combine(indexResult)). grantDelete is the exact
631+
// "last index only" scenario from the bug title, so all three grant methods
632+
// exercise the multi-index combine path here.
633+
test.each([
634+
['grantRead', (g: iam.IGrantable, ids: string[]) => bucket.grantRead(g, ids)],
635+
['grantWrite', (g: iam.IGrantable, ids: string[]) => bucket.grantWrite(g, ids)],
636+
['grantDelete', (g: iam.IGrantable, ids: string[]) => bucket.grantDelete(g, ids)],
637+
] as const)(
638+
'%s returns a Grant referencing every index ARN and the bucket ARN, not just the last index',
639+
(_name, grantFn) => {
640+
const grant = grantFn(role, ['index1', 'index2']);
641+
642+
const resolved = [
643+
...grant.principalStatements,
644+
...grant.resourceStatements,
645+
].map((statement) => stack.resolve(statement.toStatementJson()).Resource);
646+
const resources = JSON.stringify(resolved);
647+
648+
// Every specified index ARN is present - not just the last one.
649+
expect(resources).toContain('/index/index1');
650+
expect(resources).toContain('/index/index2');
651+
652+
// The bucket statement survives the combine. Index ARNs are built from
653+
// the bucket ARN (Fn::Join of the VectorBucketArn GetAtt + "/index/..."),
654+
// so a plain "VectorBucketArn" match alone would also pass from an index
655+
// statement. Assert a standalone bucket resource exists - one that
656+
// references VectorBucketArn WITHOUT the "/index/" suffix.
657+
expect(resources).toContain('VectorBucketArn');
658+
const bucketOnly = resolved.some((resource) => {
659+
const asString = JSON.stringify(resource);
660+
return asString.includes('VectorBucketArn') && !asString.includes('/index/');
661+
});
662+
expect(bucketOnly).toBe(true);
663+
},
664+
);
665+
626666
test('grantWrite grants write permissions', () => {
627667
bucket.grantWrite(role);
628668

0 commit comments

Comments
 (0)