Skip to content

Commit 77840cc

Browse files
fix(cli): scope migrated private/protected S3 access to the caller identity
gen2-migration emitted Gen1 authenticated access on private/{entity_id}/* and protected/{entity_id}/* using allow.authenticated, so the {entity_id} token was not bound to the requesting user. Map Gen1 authenticated access on private/ and protected/ to allow.entity('identity') so {entity_id} resolves to the caller's Cognito identity, matching the per-user scoping of the Gen1 configuration. protected/ retains allow.authenticated read access; public/* is unchanged. Adds a test covering per-user scoping and updates the affected inline snapshots.
1 parent f06e870 commit 77840cc

2 files changed

Lines changed: 75 additions & 9 deletions

File tree

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

Lines changed: 47 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -189,8 +189,11 @@ describe('S3Generator', () => {
189189
name: \`myBucket-main-\${branchName}\`,
190190
access: (allow) => ({
191191
'public/*': [allow.authenticated.to(['read', 'write'])],
192-
'protected/{entity_id}/*': [allow.authenticated.to(['read', 'write'])],
193-
'private/{entity_id}/*': [allow.authenticated.to(['read', 'write'])],
192+
'protected/{entity_id}/*': [
193+
allow.entity('identity').to(['read', 'write']),
194+
allow.authenticated.to(['read']),
195+
],
196+
'private/{entity_id}/*': [allow.entity('identity').to(['read', 'write'])],
194197
}),
195198
});
196199
@@ -218,6 +221,43 @@ describe('S3Generator', () => {
218221
`);
219222
});
220223

224+
// private/ and protected/ are per-user paths: authenticated access must be scoped to the
225+
// caller's own identity via allow.entity('identity'), matching the Gen1 configuration.
226+
it('scopes authenticated private/protected access to the caller identity', async () => {
227+
const gen1App = await createGen1App({
228+
providers: { awscloudformation: { StackName: 'amplify-test-main-123456', Region: 'us-east-1' } },
229+
storage: { myBucket: { service: 'S3', output: { BucketName: 'myBucket-main-abc123' } } },
230+
});
231+
jest.spyOn(gen1App, 'cliInputs').mockReturnValue({
232+
authAccess: ['READ', 'CREATE_AND_UPDATE', 'DELETE'],
233+
guestAccess: [],
234+
});
235+
jest.spyOn(gen1App.aws, 'fetchBucketAccelerate').mockResolvedValue(undefined);
236+
jest.spyOn(gen1App.aws, 'fetchBucketVersioning').mockResolvedValue(undefined);
237+
jest.spyOn(gen1App.aws, 'fetchBucketEncryption').mockResolvedValue(undefined);
238+
239+
const generator = new S3Generator(
240+
gen1App,
241+
backendGenerator,
242+
outputDir,
243+
{ category: 'storage', resourceName: 'myBucket', service: 'S3', key: 'storage:S3' },
244+
logger,
245+
);
246+
const ops = await generator.plan();
247+
await ops[0].execute();
248+
const content = writtenFile('resource.ts');
249+
const normalized = content.replace(/\s+/g, ' ');
250+
251+
// Per-user paths use entity('identity') so {entity_id} maps to the caller's identity.
252+
expect(normalized).toMatch(
253+
/'private\/\{entity_id\}\/\*':\s*\[\s*allow\.entity\('identity'\)\.to\(\['read', 'write', 'delete'\]\)\s*,?\s*\]/,
254+
);
255+
expect(normalized).toMatch(/'protected\/\{entity_id\}\/\*':\s*\[\s*allow\.entity\('identity'\)\.to\(\['read', 'write', 'delete'\]\)/);
256+
257+
// Enforce per-user scoping: private/protected must not grant write/delete via allow.authenticated.
258+
expect(content).not.toMatch(/'(private|protected)\/\{entity_id\}\/\*': \[[^\]]*allow\.authenticated\.to\(\[[^\]]*'(write|delete)'/s);
259+
});
260+
221261
it('renders guest access patterns', async () => {
222262
const gen1App = await createGen1App({
223263
providers: { awscloudformation: { StackName: 'amplify-test-main-123456', Region: 'us-east-1' } },
@@ -324,10 +364,11 @@ describe('S3Generator', () => {
324364
allow.authenticated.to(['read', 'write', 'delete']),
325365
],
326366
'protected/{entity_id}/*': [
327-
allow.authenticated.to(['read', 'write', 'delete']),
367+
allow.entity('identity').to(['read', 'write', 'delete']),
368+
allow.authenticated.to(['read']),
328369
],
329370
'private/{entity_id}/*': [
330-
allow.authenticated.to(['read', 'write', 'delete']),
371+
allow.entity('identity').to(['read', 'write', 'delete']),
331372
],
332373
}),
333374
});
@@ -402,11 +443,12 @@ describe('S3Generator', () => {
402443
allow.groups(['admin']).to(['read', 'write', 'delete']),
403444
],
404445
'protected/{entity_id}/*': [
446+
allow.entity('identity').to(['read']),
405447
allow.authenticated.to(['read']),
406448
allow.groups(['admin']).to(['read', 'write', 'delete']),
407449
],
408450
'private/{entity_id}/*': [
409-
allow.authenticated.to(['read']),
451+
allow.entity('identity').to(['read']),
410452
allow.groups(['admin']).to(['read', 'write', 'delete']),
411453
],
412454
}),

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

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -280,10 +280,15 @@ export class S3Renderer {
280280
publicPathAccess.push(this.createAllowPattern(allowIdentifier, 'guest', accessPatterns.guest));
281281
}
282282
if (accessPatterns.auth && accessPatterns.auth.length > 0) {
283-
const pattern = this.createAllowPattern(allowIdentifier, 'authenticated', accessPatterns.auth);
284-
publicPathAccess.push(pattern);
285-
protectedPathAccess.push(pattern);
286-
privatePathAccess.push(pattern);
283+
// public/* is a shared path: authenticated users get the Gen1 auth permission set.
284+
publicPathAccess.push(this.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.
288+
privatePathAccess.push(this.createEntityPattern(allowIdentifier, accessPatterns.auth));
289+
protectedPathAccess.push(this.createEntityPattern(allowIdentifier, accessPatterns.auth));
290+
// Gen1 protected/ also grants read to other authenticated users.
291+
protectedPathAccess.push(this.createAllowPattern(allowIdentifier, 'authenticated', ['read']));
287292
}
288293
if (accessPatterns.groups) {
289294
for (const [groupName, permissions] of Object.entries(accessPatterns.groups)) {
@@ -352,6 +357,25 @@ export class S3Renderer {
352357
);
353358
}
354359

360+
/**
361+
* Renders `allow.entity('identity').to([...])`, which scopes an {entity_id} path to the
362+
* caller's own Cognito identity in the generated IAM policy.
363+
*/
364+
private createEntityPattern(allowIdentifier: ts.Identifier, permissions: readonly Permission[]): CallExpression {
365+
return factory.createCallExpression(
366+
factory.createPropertyAccessExpression(
367+
factory.createCallExpression(
368+
factory.createPropertyAccessExpression(allowIdentifier, factory.createIdentifier('entity')),
369+
undefined,
370+
[factory.createStringLiteral('identity')],
371+
),
372+
factory.createIdentifier('to'),
373+
),
374+
undefined,
375+
[factory.createArrayLiteralExpression(permissions.map((p) => factory.createStringLiteral(p)))],
376+
);
377+
}
378+
355379
private createResourcePattern(allowIdentifier: ts.Identifier, functionName: string, permissions: readonly Permission[]): CallExpression {
356380
return factory.createCallExpression(
357381
factory.createPropertyAccessExpression(

0 commit comments

Comments
 (0)