fix(cli): scope migrated private/protected S3 access to the caller identity

[ahmedhamouda78]

Description of changes

The gen2-migration generate storage generator rendered Gen1 authenticated
access on the per-user paths private/{entity_id}/* and
protected/{entity_id}/* using allow.authenticated. With that rule the
{entity_id} token is not bound to the requesting user, so the generated
defineStorage access did not match the per-user scoping of the original
Gen1 configuration.

This change maps Gen1 authenticated access on private/ and protected/ to
allow.entity('identity'), so {entity_id} resolves to the caller's own
Cognito identity — matching the Gen1 behavior. protected/ additionally
keeps allow.authenticated read access (Gen1 protected semantics), and
public/* is unchanged. Group and function access mappings are unchanged.

Files:

• packages/amplify-cli/.../generate/amplify/storage/s3.renderer.ts — access-rule mapping + a small entity('identity') render helper.
• packages/amplify-cli/.../__tests__/.../storage/s3.generator.test.ts — new coverage + updated inline snapshots.

Issue #, if available

Description of how you validated changes

• yarn jest s3.generator — 12 tests pass, snapshots updated.
• Added a test asserting the per-user paths render allow.entity('identity')
  and never grant write/delete via allow.authenticated.
• Deployed a sandbox with a representative migrated config and confirmed the
  authenticated-role IAM policy scopes private/protected object access to
  the caller's Cognito identity.

Checklist

• PR description included
• yarn test passes
• Tests are changed or added
• Relevant documentation is changed or added (and PR referenced)
• New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
• Pull request labels are added

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[sarayev]

Review — Approve with comments ✅

Correct security fix: gen2-migration generate was emitting allow.authenticated.to([...]) for private/{entity_id}/* and protected/{entity_id}/*, granting every authenticated user the same access and defeating per-user {entity_id} scoping. Mapping them to allow.entity('identity').to([...]) (keeping allow.authenticated.to(['read']) on protected/ for Gen1 cross-user read) is right. Separate AST nodes per path + private static helpers are clean; all 8 fixture snapshots + 3 inline snapshots updated consistently.

Should-fix (test quality, not correctness)

1. s3.generator.test.ts new test — use a full snapshot for positive assertions. The toMatch() regex + normalized.slice(indexOf(...)) positives conflict with the repo guideline (toMatchInlineSnapshot() on complete output) and the slice-to-EOF is fragile (captures postRefactor/applyEscapeHatches; breaks if path ordering changes). Convert positives to one toMatchInlineSnapshot() on writtenFile('resource.ts'); keep the not.toMatch negative guards as-is.
2. Missing edge case: write-only auth on protected/ (authAccess: ['CREATE_AND_UPDATE']). Code unconditionally appends allow.authenticated.to(['read']) to protected/{entity_id}/* — correct Gen1 semantics but untested; add a test locking owner=entity('identity').to(['write']) + public=authenticated.to(['read']).

Nits

3. s3.renderer.ts createEntityPattern: multi-line JSDoc on a private static helper → use a // comment per the style guide.
4. buildAccessProperty: 3 inline comment blocks could consolidate into one.

Core logic correct and regression-safe for existing scenarios. Please run the full split e2e + all PR checks and get them green before merge.

[ahmedhamouda78]

@sarayev thanks for the thorough review. Pushed 5491fd6 addressing all four points:

1. Full inline snapshot for the per-user test — Replaced the toMatch() regex + normalized.slice(indexOf(...)) positives with a single toMatchInlineSnapshot() on the complete writtenFile('resource.ts'), per the repo guideline. This also removes the slice-to-EOF fragility you flagged (no longer sensitive to path ordering / postRefactor/applyEscapeHatches capture). Kept the scoped not.toMatch write/delete negative guards on the private/protected segments as-is.

2. Write-only protected/ edge case — Added a test scopes write-only authenticated protected access and retains authenticated read with authAccess: ['CREATE_AND_UPDATE']. It locks the owner rule as allow.entity('identity').to(['write']) and confirms protected/{entity_id}/* still gets the unconditional allow.authenticated.to(['read']) (Gen1 cross-user read), while private/{entity_id}/* stays owner-only write.

3. createEntityPattern comment — Switched the multi-line JSDoc on the private static helper to // comments per the style guide.

4. buildAccessProperty comments — Consolidated the three inline comment blocks into one block describing the full per-path auth mapping (public shared / private+protected per-user via entity('identity') / protected authenticated-read overlap).

No production logic changed — all test-quality and comment-style. Local verification: yarn jest s3.generator -> 13 passed (incl. the new edge case), 11 snapshots passed; eslint clean. Full split e2e + PR checks re-running on the new commit.

[sarayev]

Thanks for addressing the feedback — approving, this looks good to me.

One non-blocking thing for a future pass (don't need to fix here): for read-only auth on protected/, the allow.entity('identity').to(['read']) rule is fully subsumed by the allow.authenticated.to(['read']) that also gets emitted, so it's effectively a redundant rule that slightly inflates the generated policy. It's a deliberate uniformity trade-off (and documented in the comment), just worth keeping in mind.