Skip to content

Commit 5491fd6

Browse files
ahmedhamouda78ahmedhamouda78
committed
test(cli): address review feedback on storage per-user access tests
- Convert the per-user scoping test's positive assertions to a full toMatchInlineSnapshot() on the generated resource.ts (per repo guideline); keep the scoped not.toMatch write/delete guards. - Add a write-only auth edge case (authAccess CREATE_AND_UPDATE) locking protected/ = entity('identity').to(['write']) + authenticated.to(['read']). - s3.renderer: use // on the createEntityPattern helper and consolidate the buildAccessProperty auth-mapping comments into one block.
1 parent 662bea5 commit 5491fd6

2 files changed

Lines changed: 123 additions & 16 deletions

File tree

‎packages/amplify-cli/src/__tests__/commands/gen2-migration/generate/amplify/storage/s3.generator.test.ts‎

Lines changed: 114 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -248,10 +248,51 @@ describe('S3Generator', () => {
248248
const content = writtenFile('resource.ts');
249249
const normalized = content.replace(/\s+/g, ' ');
250250

251-
expect(normalized).toMatch(
252-
/'private\/\{entity_id\}\/\*':\s*\[\s*allow\.entity\('identity'\)\.to\(\['read', 'write', 'delete'\]\)\s*,?\s*\]/,
253-
);
254-
expect(normalized).toMatch(/'protected\/\{entity_id\}\/\*':\s*\[\s*allow\.entity\('identity'\)\.to\(\['read', 'write', 'delete'\]\)/);
251+
// Positive assertion: full generated file locks the per-user mapping (owner scoped to
252+
// entity('identity'); protected/ retains authenticated read).
253+
expect(content).toMatchInlineSnapshot(`
254+
"import { defineStorage } from '@aws-amplify/backend';
255+
import { CfnResource } from 'aws-cdk-lib';
256+
import type { Backend } from '../backend';
257+
258+
const branchName = process.env.AWS_BRANCH ?? 'sandbox';
259+
260+
export const storage = defineStorage({
261+
name: \`myBucket-main-\${branchName}\`,
262+
access: (allow) => ({
263+
'public/*': [allow.authenticated.to(['read', 'write', 'delete'])],
264+
'protected/{entity_id}/*': [
265+
allow.entity('identity').to(['read', 'write', 'delete']),
266+
allow.authenticated.to(['read']),
267+
],
268+
'private/{entity_id}/*': [
269+
allow.entity('identity').to(['read', 'write', 'delete']),
270+
],
271+
}),
272+
});
273+
274+
export function postRefactor(backend: Backend) {
275+
const s3Bucket = backend.storage.resources.cfnResources.cfnBucket;
276+
s3Bucket.bucketName = 'myBucket-main-abc123';
277+
}
278+
279+
export function applyEscapeHatches(backend: Backend) {
280+
const s3Bucket = backend.storage.resources.cfnResources.cfnBucket;
281+
for (const cfnResource of backend.storage.stack.node
282+
.findAll()
283+
.filter(
284+
(c) =>
285+
CfnResource.isCfnResource(c) &&
286+
['AWS::S3::Bucket', 'Custom::S3AutoDeleteObjects'].includes(
287+
c.cfnResourceType
288+
)
289+
)) {
290+
(cfnResource as CfnResource).addOverride('UpdateReplacePolicy', 'Retain');
291+
(cfnResource as CfnResource).addOverride('DeletionPolicy', 'Retain');
292+
}
293+
}
294+
"
295+
`);
255296

256297
// private/ and protected/ must not grant write/delete via allow.authenticated (public/*
257298
// legitimately can, so the check is scoped to those paths). Rules render in the order
@@ -266,6 +307,75 @@ describe('S3Generator', () => {
266307
}
267308
});
268309

310+
it('scopes write-only authenticated protected access and retains authenticated read', async () => {
311+
const gen1App = await createGen1App({
312+
providers: { awscloudformation: { StackName: 'amplify-test-main-123456', Region: 'us-east-1' } },
313+
storage: { myBucket: { service: 'S3', output: { BucketName: 'myBucket-main-abc123' } } },
314+
});
315+
// Write-only Gen1 auth (no READ in the auth set): the owner rule must still render as
316+
// entity('identity').to(['write']) and protected/ must still get the unconditional
317+
// authenticated read (Gen1 cross-user read semantics).
318+
jest.spyOn(gen1App, 'cliInputs').mockReturnValue({
319+
authAccess: ['CREATE_AND_UPDATE'],
320+
guestAccess: [],
321+
});
322+
jest.spyOn(gen1App.aws, 'fetchBucketAccelerate').mockResolvedValue(undefined);
323+
jest.spyOn(gen1App.aws, 'fetchBucketVersioning').mockResolvedValue(undefined);
324+
jest.spyOn(gen1App.aws, 'fetchBucketEncryption').mockResolvedValue(undefined);
325+
326+
const generator = new S3Generator(
327+
gen1App,
328+
backendGenerator,
329+
outputDir,
330+
{ category: 'storage', resourceName: 'myBucket', service: 'S3', key: 'storage:S3' },
331+
logger,
332+
);
333+
const ops = await generator.plan();
334+
await ops[0].execute();
335+
336+
expect(writtenFile('resource.ts')).toMatchInlineSnapshot(`
337+
"import { defineStorage } from '@aws-amplify/backend';
338+
import { CfnResource } from 'aws-cdk-lib';
339+
import type { Backend } from '../backend';
340+
341+
const branchName = process.env.AWS_BRANCH ?? 'sandbox';
342+
343+
export const storage = defineStorage({
344+
name: \`myBucket-main-\${branchName}\`,
345+
access: (allow) => ({
346+
'public/*': [allow.authenticated.to(['write'])],
347+
'protected/{entity_id}/*': [
348+
allow.entity('identity').to(['write']),
349+
allow.authenticated.to(['read']),
350+
],
351+
'private/{entity_id}/*': [allow.entity('identity').to(['write'])],
352+
}),
353+
});
354+
355+
export function postRefactor(backend: Backend) {
356+
const s3Bucket = backend.storage.resources.cfnResources.cfnBucket;
357+
s3Bucket.bucketName = 'myBucket-main-abc123';
358+
}
359+
360+
export function applyEscapeHatches(backend: Backend) {
361+
const s3Bucket = backend.storage.resources.cfnResources.cfnBucket;
362+
for (const cfnResource of backend.storage.stack.node
363+
.findAll()
364+
.filter(
365+
(c) =>
366+
CfnResource.isCfnResource(c) &&
367+
['AWS::S3::Bucket', 'Custom::S3AutoDeleteObjects'].includes(
368+
c.cfnResourceType
369+
)
370+
)) {
371+
(cfnResource as CfnResource).addOverride('UpdateReplacePolicy', 'Retain');
372+
(cfnResource as CfnResource).addOverride('DeletionPolicy', 'Retain');
373+
}
374+
}
375+
"
376+
`);
377+
});
378+
269379
it('renders guest access patterns', async () => {
270380
const gen1App = await createGen1App({
271381
providers: { awscloudformation: { StackName: 'amplify-test-main-123456', Region: 'us-east-1' } },

‎packages/amplify-cli/src/commands/gen2-migration/generate/amplify/storage/s3.renderer.ts‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -280,17 +280,16 @@ export class S3Renderer {
280280
publicPathAccess.push(S3Renderer.createAllowPattern(allowIdentifier, 'guest', accessPatterns.guest));
281281
}
282282
if (accessPatterns.auth && accessPatterns.auth.length > 0) {
283-
// public/* is a shared path: authenticated users get the Gen1 auth permission set.
283+
// Gen1 authenticated access maps per path:
284+
// - public/* is shared: authenticated users get the Gen1 auth permission set.
285+
// - private/ and protected/ are per-user: use allow.entity('identity') so {entity_id}
286+
// resolves to the caller's own Cognito identity, matching the Gen1 per-user scoping.
287+
// - protected/ additionally grants authenticated read (Gen1 cross-user read). When the
288+
// auth set is read-only this subsumes the owner's entity read; the overlap is harmless
289+
// and keeps the per-path mapping uniform across permission sets.
284290
publicPathAccess.push(S3Renderer.createAllowPattern(allowIdentifier, 'authenticated', accessPatterns.auth));
285-
// private/ and protected/ are per-user paths. Use allow.entity('identity') so the
286-
// {entity_id} token resolves to the caller's own Cognito identity, matching the
287-
// per-user scoping of the Gen1 configuration.
288291
privatePathAccess.push(S3Renderer.createEntityPattern(allowIdentifier, accessPatterns.auth));
289292
protectedPathAccess.push(S3Renderer.createEntityPattern(allowIdentifier, accessPatterns.auth));
290-
// Gen1 protected/ also grants read to other authenticated users. When the Gen1 auth
291-
// permission set is read-only, this authenticated read subsumes the owner's
292-
// entity('identity') read rule above; the overlap is harmless and keeps the per-path
293-
// mapping uniform across permission sets.
294293
protectedPathAccess.push(S3Renderer.createAllowPattern(allowIdentifier, 'authenticated', ['read']));
295294
}
296295
if (accessPatterns.groups) {
@@ -360,10 +359,8 @@ export class S3Renderer {
360359
);
361360
}
362361

363-
/**
364-
* Renders `allow.entity('identity').to([...])`, which scopes an {entity_id} path to the
365-
* caller's own Cognito identity in the generated IAM policy.
366-
*/
362+
// Renders `allow.entity('identity').to([...])`, which scopes an {entity_id} path to the
363+
// caller's own Cognito identity in the generated IAM policy.
367364
private static createEntityPattern(allowIdentifier: ts.Identifier, permissions: readonly Permission[]): CallExpression {
368365
return factory.createCallExpression(
369366
factory.createPropertyAccessExpression(

0 commit comments

Comments
 (0)