diff --git a/.changeset/thick-flies-fail.md b/.changeset/thick-flies-fail.md new file mode 100644 index 00000000000..c3b68caff8e --- /dev/null +++ b/.changeset/thick-flies-fail.md @@ -0,0 +1,5 @@ +--- +'@aws-amplify/ui-react-storage': minor +--- + +deduplicate storage browser locations which have broader permissions diff --git a/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/listLocations.spec.ts b/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/listLocations.spec.ts index 60fd72da488..a35a6dbb960 100644 --- a/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/listLocations.spec.ts +++ b/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/listLocations.spec.ts @@ -17,7 +17,10 @@ jest.mock('../../../storage-internal'); const mockListCallerAccessGrants = jest.mocked(listCallerAccessGrants); const generateMockLocations = (size: number, mockLocations: LocationAccess) => - Array(size).fill(mockLocations); + Array.from({ length: size }, (_, i) => ({ + ...mockLocations, + scope: `s3://bucket/prefix${i + 1}/*`, + })); const accountId = 'account-id'; const credentials: LocationCredentialsProvider = jest.fn(); @@ -80,18 +83,20 @@ describe('listLocationsHandler', () => { permission: 'READWRITE', type: 'PREFIX', }; + const allLocations = generateMockLocations(5, mockLocation); + const mockOutputPage1: ListLocationsOutput = { - locations: [mockLocation], + locations: [allLocations[0]], nextToken: 'token1', }; const mockOutputPage2: ListLocationsOutput = { - locations: [...generateMockLocations(2, mockLocation)], + locations: [allLocations[1], allLocations[2]], nextToken: 'token2', }; const mockOutputPage3: ListLocationsOutput = { - locations: [...generateMockLocations(2, mockLocation)], + locations: [allLocations[3], allLocations[4]], nextToken: undefined, }; diff --git a/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/utils.spec.ts b/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/utils.spec.ts index c7dcb57e312..038e260b8d7 100644 --- a/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/utils.spec.ts +++ b/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/utils.spec.ts @@ -12,6 +12,7 @@ jest.mock('aws-amplify', () => ({ import { createFileDataItem, + deduplicateLocations, getBucketRegion, getFileKey, getFilteredLocations, @@ -246,6 +247,207 @@ describe('utils', () => { }); }); + describe('deduplicateLocations', () => { + it('returns all locations when there are no duplicates', () => { + const locations: LocationData[] = [ + { + bucket: 'bucket1', + id: 'id1', + permissions: ['get', 'list'], + prefix: 'prefix1/', + type: 'PREFIX', + }, + { + bucket: 'bucket2', + id: 'id2', + permissions: ['get', 'list'], + prefix: 'prefix2/', + type: 'PREFIX', + }, + ]; + + const result = deduplicateLocations(locations); + expect(result).toHaveLength(2); + expect(result).toEqual(locations); + }); + + it('deduplicates READ + READWRITE, keeping READWRITE (superset)', () => { + const locations: LocationData[] = [ + { + bucket: 'idfc-sboms', + id: 'id1', + permissions: ['get', 'list'], + prefix: '', + type: 'BUCKET', + }, + { + bucket: 'idfc-sboms', + id: 'id2', + permissions: ['delete', 'get', 'list', 'write'], + prefix: '', + type: 'BUCKET', + }, + ]; + + const result = deduplicateLocations(locations); + expect(result).toHaveLength(1); + expect(result[0].permissions).toEqual(['delete', 'get', 'list', 'write']); + expect(result[0].id).toBe('id2'); + }); + + it('does NOT deduplicate READ + WRITE (not superset, keeps both)', () => { + const locations: LocationData[] = [ + { + bucket: 'bucket1', + id: 'id1', + permissions: ['get', 'list'], + prefix: 'prefix/', + type: 'PREFIX', + }, + { + bucket: 'bucket1', + id: 'id2', + permissions: ['delete', 'write'], + prefix: 'prefix/', + type: 'PREFIX', + }, + ]; + + const result = deduplicateLocations(locations); + expect(result).toHaveLength(2); + expect(result.find((l) => l.id === 'id1')).toBeDefined(); + expect(result.find((l) => l.id === 'id2')).toBeDefined(); + }); + + it('keeps first location when permissions are identical', () => { + const locations: LocationData[] = [ + { + bucket: 'bucket1', + id: 'id1', + permissions: ['get', 'list'], + prefix: 'prefix/', + type: 'PREFIX', + }, + { + bucket: 'bucket1', + id: 'id2', + permissions: ['get', 'list'], + prefix: 'prefix/', + type: 'PREFIX', + }, + ]; + + const result = deduplicateLocations(locations); + expect(result).toHaveLength(1); + expect(result[0].id).toBe('id1'); + }); + + it('deduplicates when READWRITE is superset of READ and WRITE', () => { + const locations: LocationData[] = [ + { + bucket: 'bucket1', + id: 'id1', + permissions: ['get', 'list'], + prefix: 'prefix/', + type: 'PREFIX', + }, + { + bucket: 'bucket1', + id: 'id2', + permissions: ['delete', 'write'], + prefix: 'prefix/', + type: 'PREFIX', + }, + { + bucket: 'bucket1', + id: 'id3', + permissions: ['delete', 'get', 'list', 'write'], + prefix: 'prefix/', + type: 'PREFIX', + }, + ]; + + const result = deduplicateLocations(locations); + // READWRITE is superset of both READ and WRITE, so only READWRITE should remain + expect(result).toHaveLength(1); + expect(result[0].permissions).toEqual(['delete', 'get', 'list', 'write']); + expect(result[0].id).toBe('id3'); + }); + + it('treats different prefixes as separate locations', () => { + const locations: LocationData[] = [ + { + bucket: 'bucket1', + id: 'id1', + permissions: ['get', 'list'], + prefix: 'prefix1/', + type: 'PREFIX', + }, + { + bucket: 'bucket1', + id: 'id2', + permissions: ['delete', 'get', 'list', 'write'], + prefix: 'prefix2/', + type: 'PREFIX', + }, + ]; + + const result = deduplicateLocations(locations); + expect(result).toHaveLength(2); + }); + + it('treats different buckets as separate locations', () => { + const locations: LocationData[] = [ + { + bucket: 'bucket1', + id: 'id1', + permissions: ['get', 'list'], + prefix: 'prefix/', + type: 'PREFIX', + }, + { + bucket: 'bucket2', + id: 'id2', + permissions: ['get', 'list'], + prefix: 'prefix/', + type: 'PREFIX', + }, + ]; + + const result = deduplicateLocations(locations); + expect(result).toHaveLength(2); + }); + + it('handles empty location array', () => { + const result = deduplicateLocations([]); + expect(result).toEqual([]); + }); + + it('deduplicates WRITE + READWRITE, keeping READWRITE (superset)', () => { + const locations: LocationData[] = [ + { + bucket: 'bucket1', + id: 'id1', + permissions: ['delete', 'write'], + prefix: 'prefix/', + type: 'PREFIX', + }, + { + bucket: 'bucket1', + id: 'id2', + permissions: ['delete', 'get', 'list', 'write'], + prefix: 'prefix/', + type: 'PREFIX', + }, + ]; + + const result = deduplicateLocations(locations); + expect(result).toHaveLength(1); + expect(result[0].permissions).toEqual(['delete', 'get', 'list', 'write']); + expect(result[0].id).toBe('id2'); + }); + }); + describe('getBucketRegion', () => { beforeEach(() => { jest.clearAllMocks(); diff --git a/packages/react-storage/src/components/StorageBrowser/actions/handlers/listLocations.ts b/packages/react-storage/src/components/StorageBrowser/actions/handlers/listLocations.ts index d368a7af390..2e7be8bc2cc 100644 --- a/packages/react-storage/src/components/StorageBrowser/actions/handlers/listLocations.ts +++ b/packages/react-storage/src/components/StorageBrowser/actions/handlers/listLocations.ts @@ -8,7 +8,7 @@ import type { LocationData, ActionInputConfig, } from './types'; -import { getFilteredLocations } from './utils'; +import { deduplicateLocations, getFilteredLocations } from './utils'; const DEFAULT_PAGE_SIZE = 1000; @@ -76,5 +76,11 @@ export const listLocationsHandler: ListLocationsHandler = async (input) => { return { items, nextToken: output.nextToken }; }; - return fetchLocations([], nextToken); + const result = await fetchLocations([], nextToken); + + // Deduplicate locations with the same bucket and prefix, keeping broader permissions + return { + items: deduplicateLocations(result.items), + nextToken: result.nextToken, + }; }; diff --git a/packages/react-storage/src/components/StorageBrowser/actions/handlers/utils.ts b/packages/react-storage/src/components/StorageBrowser/actions/handlers/utils.ts index c084e351b55..6bae8219e4f 100644 --- a/packages/react-storage/src/components/StorageBrowser/actions/handlers/utils.ts +++ b/packages/react-storage/src/components/StorageBrowser/actions/handlers/utils.ts @@ -142,6 +142,86 @@ export const shouldExcludeLocation = ( return excludedByPermssions || excludedByType; }; +/** + * Determines if permissions1 is a strict superset (broader) of permissions2. + * Returns true only if permissions1 contains all permissions from permissions2 + * AND has more permissions. + */ +const hasBroaderPermissions = ( + permissions1: LocationPermissions, + permissions2: LocationPermissions +): boolean => { + // permissions1 must have more permissions (strict superset, not equal) + if (permissions1.length <= permissions2.length) { + return false; + } + + // permissions1 must contain all permissions from permissions2 + return permissions2.every((perm) => permissions1.includes(perm)); +}; + +/** + * Deduplicates locations with the same bucket and prefix. + * Only deduplicates when one location's permissions are a superset of another's. + * This prevents deduplication of incompatible grants like READ + WRITE. + * + * Examples: + * - READ + READWRITE → Keep READWRITE (superset) + * - READ + READ → Keep first (identical) + * - READ + WRITE → Keep both (not superset, need separate locations) + */ +export const deduplicateLocations = ( + locations: LocationData[] +): LocationData[] => { + // Group locations by bucket:prefix + const locationGroups = new Map(); + + for (const location of locations) { + const key = `${location.bucket}:${location.prefix}`; + const group = locationGroups.get(key) ?? []; + group.push(location); + locationGroups.set(key, group); + } + + // For each group, keep only non-redundant locations + const result: LocationData[] = []; + + for (const group of locationGroups.values()) { + if (group.length === 1) { + result.push(group[0]); + continue; + } + + // Find locations that are not subsets of any other location in the group + const nonRedundant: LocationData[] = []; + + for (const location of group) { + // Check if this location is a subset of any other location + const isSubsetOfAnother = group.some((other) => { + if (other === location) return false; + return hasBroaderPermissions(other.permissions, location.permissions); + }); + + if (!isSubsetOfAnother) { + // Check if we already have an identical permission set + const isDuplicate = nonRedundant.some((existing) => { + const sortedNew = [...location.permissions].sort().join(','); + const sortedExisting = [...existing.permissions].sort().join(','); + return sortedNew === sortedExisting; + }); + + if (!isDuplicate) { + nonRedundant.push(location); + } + } + } + + result.push(...nonRedundant); + } + + return result; +}; + export const getFilteredLocations = ( locations: AccessGrantLocation[], exclude?: ListLocationsExcludeOptions diff --git a/packages/react-storage/src/components/StorageBrowser/adapters/createAmplifyAuthAdapter/createAmplifyListLocationsHandler.ts b/packages/react-storage/src/components/StorageBrowser/adapters/createAmplifyAuthAdapter/createAmplifyListLocationsHandler.ts index 3a27170486d..b28b9d62494 100644 --- a/packages/react-storage/src/components/StorageBrowser/adapters/createAmplifyAuthAdapter/createAmplifyListLocationsHandler.ts +++ b/packages/react-storage/src/components/StorageBrowser/adapters/createAmplifyAuthAdapter/createAmplifyListLocationsHandler.ts @@ -2,6 +2,7 @@ import type { LocationData } from '../../actions'; import type { ListPathsOutput } from '../../storage-internal'; import { listPaths } from '../../storage-internal'; import type { ListLocations, ListLocationsInput } from '../../actions'; +import { deduplicateLocations } from '../../actions/handlers/utils'; import { parseAmplifyAuthPermission } from '../permissionParsers'; import { getPaginatedLocations } from './getPaginatedLocations'; @@ -36,7 +37,8 @@ export const createAmplifyListLocationsHandler = (): ListLocations => { } ); - cachedItems = sanitizedItems; + // Deduplicate locations with the same bucket and prefix, keeping broader permissions + cachedItems = deduplicateLocations(sanitizedItems); return getPaginatedLocations({ items: cachedItems,