fix: cognito users in a group cannot access their own files in s3

Author: iankhou

.changeset/wicked-experts-shake.md

@@ -0,0 +1,5 @@
+---
+'@aws-amplify/backend-storage': patch
+---
+
+fix: users in Cognito groups could not access their own files

packages/backend-storage/src/access_builder.test.ts

@@ -4,9 +4,13 @@ import { roleAccessBuilder } from './access_builder.js';
 import {
   ConstructContainer,
   ConstructFactoryGetInstanceProps,
+  ResourceAccessAcceptor,
   ResourceAccessAcceptorFactory,
   ResourceProvider,
+  SsmEnvironmentEntry,
 } from '@aws-amplify/plugin-types';
+import { App, Stack } from 'aws-cdk-lib';
+import { Policy, PolicyStatement } from 'aws-cdk-lib/aws-iam';
 
 void describe('storageAccessBuilder', () => {
   const resourceAccessAcceptorMock = mock.fn();
@@ -115,13 +119,21 @@ void describe('storageAccessBuilder', () => {
       accessDefinition.idSubstitution,
       '${cognito-identity.amazonaws.com:sub}',
     );
-    assert.deepStrictEqual(
-      accessDefinition.getResourceAccessAcceptors.map(
-        (getResourceAccessAcceptor) =>
-          getResourceAccessAcceptor(stubGetInstanceProps),
-      ),
-      [resourceAccessAcceptorMock],
-    );
+
+    // Should have 2 resource access acceptors now: authenticated role + compound group acceptor
+    assert.equal(accessDefinition.getResourceAccessAcceptors.length, 2);
+
+    // First acceptor should be the authenticated role
+    const authAcceptor =
+      accessDefinition.getResourceAccessAcceptors[0](stubGetInstanceProps);
+    assert.equal(authAcceptor, resourceAccessAcceptorMock);
+
+    // Second acceptor should be the compound group acceptor
+    const groupAcceptor =
+      accessDefinition.getResourceAccessAcceptors[1](stubGetInstanceProps);
+    assert.equal(typeof groupAcceptor.identifier, 'string');
+    assert.equal(typeof groupAcceptor.acceptResourceAccess, 'function');
+
     assert.equal(
       getConstructFactoryMock.mock.calls[0].arguments[0],
       'AuthResources',
@@ -172,4 +184,130 @@ void describe('storageAccessBuilder', () => {
       [group1AccessAcceptorMock, group2AccessAcceptorMock],
     );
   });
+
+  void it('entity permissions apply to all group roles when groups are defined', () => {
+    // Mock acceptResourceAccess calls to verify they're made
+    const adminAcceptResourceAccessMock = mock.fn();
+    const usersAcceptResourceAccessMock = mock.fn();
+    const adminAcceptorMock: ResourceAccessAcceptor = {
+      identifier: 'adminAcceptor',
+      acceptResourceAccess: adminAcceptResourceAccessMock,
+    };
+    const usersAcceptorMock: ResourceAccessAcceptor = {
+      identifier: 'usersAcceptor',
+      acceptResourceAccess: usersAcceptResourceAccessMock,
+    };
+
+    // Create a group-specific mock that returns the correct types
+    const groupSpecificMock = mock.fn((roleName: string) => {
+      if (roleName === 'Admins') return adminAcceptorMock;
+      if (roleName === 'Users') return usersAcceptorMock;
+      return resourceAccessAcceptorMock;
+    });
+
+    // Mock auth resources with groups
+    const mockAuthResources = {
+      resources: {
+        groups: {
+          Admins: { role: {} },
+          Users: { role: {} },
+        },
+      },
+      getResourceAccessAcceptor: groupSpecificMock,
+    };
+
+    const mockConstructFactory = mock.fn(
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      <T extends ResourceProvider>(_: string) => ({
+        getInstance: () => mockAuthResources as unknown as T,
+      }),
+    );
+
+    const mockGetInstanceProps: ConstructFactoryGetInstanceProps = {
+      constructContainer: {
+        getConstructFactory: mockConstructFactory,
+      } as unknown as ConstructContainer,
+    } as unknown as ConstructFactoryGetInstanceProps;
+
+    const accessDefinition = roleAccessBuilder
+      .entity('identity')
+      .to(['read', 'write', 'delete']);
+
+    // Should have 2 resource access acceptors: authenticated role + compound group acceptor
+    assert.equal(accessDefinition.getResourceAccessAcceptors.length, 2);
+
+    // Get the compound group acceptor
+    const groupAcceptor =
+      accessDefinition.getResourceAccessAcceptors[1](mockGetInstanceProps);
+
+    // Verify it has the expected structure
+    assert.ok(groupAcceptor.identifier.includes('entityAccessForAllGroups'));
+    assert.ok(groupAcceptor.identifier.includes('Admins'));
+    assert.ok(groupAcceptor.identifier.includes('Users'));
+    assert.equal(typeof groupAcceptor.acceptResourceAccess, 'function');
+
+    // Call acceptResourceAccess on the compound acceptor
+    const stack = new Stack(new App(), 'TestStack');
+    const mockPolicy = new Policy(stack, 'TestPolicy', {
+      statements: [new PolicyStatement()],
+    });
+    const mockSsmEntries: SsmEnvironmentEntry[] = [];
+    groupAcceptor.acceptResourceAccess(mockPolicy, mockSsmEntries);
+
+    // Verify both group roles had the policy applied
+    assert.equal(adminAcceptResourceAccessMock.mock.callCount(), 1);
+    assert.equal(usersAcceptResourceAccessMock.mock.callCount(), 1);
+    assert.deepStrictEqual(
+      adminAcceptResourceAccessMock.mock.calls[0].arguments,
+      [mockPolicy, mockSsmEntries],
+    );
+    assert.deepStrictEqual(
+      usersAcceptResourceAccessMock.mock.calls[0].arguments,
+      [mockPolicy, mockSsmEntries],
+    );
+  });
+
+  void it('entity permissions handle case when no groups are defined', () => {
+    // Mock auth resources without groups
+    const mockAuthResources = {
+      resources: {
+        groups: undefined,
+      },
+      getResourceAccessAcceptor: getResourceAccessAcceptorMock,
+    };
+
+    const mockConstructFactory = mock.fn(
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      <T extends ResourceProvider>(_: string) => ({
+        getInstance: () => mockAuthResources as unknown as T,
+      }),
+    );
+
+    const mockGetInstanceProps: ConstructFactoryGetInstanceProps = {
+      constructContainer: {
+        getConstructFactory: mockConstructFactory,
+      } as unknown as ConstructContainer,
+    } as unknown as ConstructFactoryGetInstanceProps;
+
+    const accessDefinition = roleAccessBuilder
+      .entity('identity')
+      .to(['read', 'write', 'delete']);
+
+    // Should still have 2 resource access acceptors
+    assert.equal(accessDefinition.getResourceAccessAcceptors.length, 2);
+
+    // Get the compound group acceptor (should be empty/no-op)
+    const groupAcceptor =
+      accessDefinition.getResourceAccessAcceptors[1](mockGetInstanceProps);
+    assert.equal(groupAcceptor.identifier, 'entityAccessForAllGroups-empty');
+
+    // Should not throw when called (no-op behavior)
+    const stack = new Stack(new App(), 'TestStack');
+    const emptyPolicy = new Policy(stack, 'EmptyPolicy', {
+      statements: [new PolicyStatement()],
+    });
+    assert.doesNotThrow(() => {
+      groupAcceptor.acceptResourceAccess(emptyPolicy, []);
+    });
+  });
 });

packages/backend-storage/src/access_builder.ts

@@ -59,7 +59,10 @@ export const roleAccessBuilder: StorageAccessBuilder = {
   }),
   entity: (entityId) => ({
     to: (actions) => ({
-      getResourceAccessAcceptors: [getAuthRoleResourceAccessAcceptor],
+      getResourceAccessAcceptors: [
+        getAuthRoleResourceAccessAcceptor,
+        getEntityAccessForAllGroups,
+      ],
       uniqueDefinitionIdValidations: [
         {
           uniqueDefinitionId: `entity${entityId}`,
@@ -102,6 +105,71 @@ const getUnauthRoleResourceAccessAcceptor = (
     'unauthenticatedUserIamRole',
   );
 
+const getEntityAccessForAllGroups = (
+  getInstanceProps: ConstructFactoryGetInstanceProps,
+) => {
+  const authResources = getInstanceProps.constructContainer
+    .getConstructFactory<
+      ResourceProvider & ResourceAccessAcceptorFactory<AuthRoleName | string>
+    >('AuthResources')
+    ?.getInstance(getInstanceProps);
+
+  if (!authResources) {
+    // If no auth resources, return a no-op acceptor
+    return {
+      identifier: 'entityAccessForAllGroups-empty',
+      acceptResourceAccess: () => {
+        // No groups to apply permissions to
+      },
+    };
+  }
+
+  const resources =
+    'resources' in authResources ? authResources.resources : undefined;
+  const groups =
+    resources && typeof resources === 'object' && 'groups' in resources
+      ? (resources as { groups: Record<string, unknown> }).groups
+      : undefined;
+
+  if (!groups) {
+    // If no groups defined, return a no-op acceptor
+    return {
+      identifier: 'entityAccessForAllGroups-empty',
+      acceptResourceAccess: () => {
+        // No groups to apply permissions to
+      },
+    };
+  }
+
+  // Create a compound acceptor that applies to all group roles
+  const groupNames = Object.keys(groups);
+
+  return {
+    identifier: `entityAccessForAllGroups-${groupNames.join('-')}`,
+    acceptResourceAccess: (policy: unknown, ssmEnvironmentEntries: unknown) => {
+      // Apply the policy to each group role
+      groupNames.forEach((groupName) => {
+        try {
+          const groupAcceptor = getUserRoleResourceAccessAcceptor(
+            getInstanceProps,
+            groupName,
+          );
+          groupAcceptor.acceptResourceAccess(
+            policy as Parameters<typeof groupAcceptor.acceptResourceAccess>[0],
+            ssmEnvironmentEntries as Parameters<
+              typeof groupAcceptor.acceptResourceAccess
+            >[1],
+          );
+        } catch (error) {
+          // Group role might not exist, ignore silently
+          // This is expected behavior when a group role hasn't been created yet
+          void error; // Acknowledge the error parameter to satisfy ESLint
+        }
+      });
+    },
+  };
+};
+
 const getUserRoleResourceAccessAcceptor = (
   getInstanceProps: ConstructFactoryGetInstanceProps,
   roleName: AuthRoleName | string,