From d2b59a064853fca8eaa238512f5ba01ddec53401 Mon Sep 17 00:00:00 2001 From: Jordan Nelson Date: Fri, 22 Mar 2024 18:02:55 -0400 Subject: [PATCH] chore: update path validation logic --- .../lib/src/types/storage/storage_path.dart | 4 +- .../content_type_infer_html.dart | 4 +- .../content_type_infer_io.dart | 2 +- .../get_url_special_characters_test.dart | 4 +- .../integration_test/use_case_test.dart | 50 +++++++++---------- .../amplify_storage_s3/example/lib/main.dart | 4 +- .../lib/src/model/s3_item.dart | 3 +- .../src/path_resolver/s3_path_resolver.dart | 13 ++--- .../test/amplify_storage_s3_dart_test.dart | 8 +-- .../path_resolver/path_resolver_test.dart | 13 +++-- .../storage_s3_service_test.dart | 12 ++--- 11 files changed, 56 insertions(+), 61 deletions(-) diff --git a/packages/amplify_core/lib/src/types/storage/storage_path.dart b/packages/amplify_core/lib/src/types/storage/storage_path.dart index ef292bbdd24..e26b084463b 100644 --- a/packages/amplify_core/lib/src/types/storage/storage_path.dart +++ b/packages/amplify_core/lib/src/types/storage/storage_path.dart @@ -22,7 +22,7 @@ class StoragePath { /// ### Example /// {@template amplify_core.storage.storage_path.from_string.example} /// ``` - /// const p = StoragePath.fromString('/path/to/object.png'); + /// const p = StoragePath.fromString('path/to/object.png'); /// ``` /// {@endtemplate} const StoragePath.fromString(String path) : _path = path; @@ -32,7 +32,7 @@ class StoragePath { /// ### Example /// {@template amplify_core.storage.storage_path.with_identity_id.example} /// ``` - /// const p = StoragePath.withIdentityId((String identityId) => '/users/$identityId/object.png'); + /// const p = StoragePath.withIdentityId((String identityId) => 'users/$identityId/object.png'); /// ``` /// {@endtemplate} factory StoragePath.withIdentityId( diff --git a/packages/storage/amplify_storage_s3/example/integration_test/content_type_infer/content_type_infer_html.dart b/packages/storage/amplify_storage_s3/example/integration_test/content_type_infer/content_type_infer_html.dart index ea8f251c2b6..8ea8846d945 100644 --- a/packages/storage/amplify_storage_s3/example/integration_test/content_type_infer/content_type_infer_html.dart +++ b/packages/storage/amplify_storage_s3/example/integration_test/content_type_infer/content_type_infer_html.dart @@ -39,7 +39,7 @@ void testContentTypeInferTest({ tearDownAll(() async { await Amplify.Storage.removeMany( paths: testUploadKeys - .map((key) => StoragePath.fromString('/private/$key')) + .map((key) => StoragePath.fromString('private/$key')) .toList(), ).result; }); @@ -53,7 +53,7 @@ void testContentTypeInferTest({ final result = await s3Plugin .uploadFile( localFile: file, - path: StoragePath.fromString('/private/${testUploadKeys[index]}'), + path: StoragePath.fromString('private/${testUploadKeys[index]}'), options: const StorageUploadFileOptions( pluginOptions: S3UploadFilePluginOptions( getProperties: true, diff --git a/packages/storage/amplify_storage_s3/example/integration_test/content_type_infer/content_type_infer_io.dart b/packages/storage/amplify_storage_s3/example/integration_test/content_type_infer/content_type_infer_io.dart index 338efbd2287..d56b925f56c 100644 --- a/packages/storage/amplify_storage_s3/example/integration_test/content_type_infer/content_type_infer_io.dart +++ b/packages/storage/amplify_storage_s3/example/integration_test/content_type_infer/content_type_infer_io.dart @@ -35,7 +35,7 @@ void testContentTypeInferTest({ tearDownAll(() async { await Amplify.Storage.removeMany( paths: testUploadKeys - .map((key) => StoragePath.fromString('/private/$key')) + .map((key) => StoragePath.fromString('private/$key')) .toList(), ).result; }); diff --git a/packages/storage/amplify_storage_s3/example/integration_test/issues/get_url_special_characters_test.dart b/packages/storage/amplify_storage_s3/example/integration_test/issues/get_url_special_characters_test.dart index 430b0bc3b19..2d4e6f026ba 100644 --- a/packages/storage/amplify_storage_s3/example/integration_test/issues/get_url_special_characters_test.dart +++ b/packages/storage/amplify_storage_s3/example/integration_test/issues/get_url_special_characters_test.dart @@ -41,11 +41,11 @@ void main() { final key = 'Test - ${DateTime.now()}'; await Amplify.Storage.uploadData( data: S3DataPayload.bytes('hello'.codeUnits), - path: StoragePath.fromString('/public/$key'), + path: StoragePath.fromString('public/$key'), ).result; final getUrlResult = await Amplify.Storage.getUrl( - path: StoragePath.fromString('/public/$key'), + path: StoragePath.fromString('public/$key'), ).result; final uri = getUrlResult.url; diff --git a/packages/storage/amplify_storage_s3/example/integration_test/use_case_test.dart b/packages/storage/amplify_storage_s3/example/integration_test/use_case_test.dart index d6ce797a299..431b628fef7 100644 --- a/packages/storage/amplify_storage_s3/example/integration_test/use_case_test.dart +++ b/packages/storage/amplify_storage_s3/example/integration_test/use_case_test.dart @@ -163,7 +163,7 @@ void main() { testBytes, contentType: testContentType, ), - path: StoragePath.fromString('/public/$testObjectKey1'), + path: StoragePath.fromString('public/$testObjectKey1'), options: const StorageUploadDataOptions( metadata: { 'filename': testObjectFileName1, @@ -213,7 +213,7 @@ void main() { .uploadData( data: S3DataPayload.dataUrl(testDataUrl), path: StoragePath.withIdentityId( - (identityId) => '/protected/$identityId/$testObjectKey2', + (identityId) => 'protected/$identityId/$testObjectKey2', ), options: const StorageUploadDataOptions( metadata: { @@ -252,7 +252,7 @@ void main() { .uploadFile( localFile: AWSFile.fromData(testLargeFileBytes), path: StoragePath.withIdentityId( - (identityId) => '/private/$identityId/$testObjectKey3', + (identityId) => 'private/$identityId/$testObjectKey3', ), options: const StorageUploadFileOptions( metadata: { @@ -288,7 +288,7 @@ void main() { ' currently signed in user', (WidgetTester tester) async { final result = await Amplify.Storage.getUrl( path: StoragePath.withIdentityId( - (identityId) => '/private/$identityId/$testObjectKey3', + (identityId) => 'private/$identityId/$testObjectKey3', ), options: const StorageGetUrlOptions( pluginOptions: S3GetUrlPluginOptions( @@ -306,7 +306,7 @@ void main() { (WidgetTester tester) async { final result = Amplify.Storage.getUrl( path: const StoragePath.fromString( - '/public/random/non-existent/object.png', + 'public/random/non-existent/object.png', ), options: const StorageGetUrlOptions( pluginOptions: S3GetUrlPluginOptions( @@ -406,10 +406,10 @@ void main() { (WidgetTester tester) async { final result = await Amplify.Storage.copy( source: StoragePath.withIdentityId( - (identityId) => '/private/$identityId/$testObjectKey3', + (identityId) => 'private/$identityId/$testObjectKey3', ), destination: StoragePath.withIdentityId( - (identityId) => '/private/$identityId/$testObject3CopyKey', + (identityId) => 'private/$identityId/$testObject3CopyKey', ), options: const StorageCopyOptions( pluginOptions: S3CopyPluginOptions( @@ -427,7 +427,7 @@ void main() { (WidgetTester tester) async { final result = await Amplify.Storage.remove( path: StoragePath.withIdentityId( - (id) => '/private/$id/$testObject3CopyMoveKey', + (id) => 'private/$id/$testObject3CopyMoveKey', ), ).result; @@ -487,7 +487,7 @@ void main() { testBytes, contentType: testContentType, ), - path: StoragePath.fromString('/public/$objectKey'), + path: StoragePath.fromString('public/$objectKey'), options: StorageUploadDataOptions( metadata: testMetadata, pluginOptions: const S3UploadDataPluginOptions( @@ -515,7 +515,7 @@ void main() { expect(metadata, equals(testMetadata)); await s3Plugin - .remove(path: StoragePath.fromString('/public/$objectKey')) + .remove(path: StoragePath.fromString('public/$objectKey')) .result; }); }); @@ -537,7 +537,7 @@ void main() { testWidgets('get properties of object with access level guest', (WidgetTester tester) async { final result = await Amplify.Storage.getProperties( - path: StoragePath.fromString('/public/$testObjectKey1'), + path: StoragePath.fromString('public/$testObjectKey1'), ).result; expect(result.storageItem.eTag, object1Etag); @@ -548,7 +548,7 @@ void main() { (WidgetTester tester) async { final result = await Amplify.Storage.getProperties( path: StoragePath.fromString( - '/protected/$user1IdentityId/$testObjectKey2', + 'protected/$user1IdentityId/$testObjectKey2', ), ).result; @@ -561,7 +561,7 @@ void main() { (WidgetTester tester) async { final operation = Amplify.Storage.getProperties( path: StoragePath.withIdentityId( - (identityId) => '/private/$identityId/$testObjectKey3', + (identityId) => 'private/$identityId/$testObjectKey3', ), ); @@ -574,7 +574,7 @@ void main() { testWidgets('get url of object with access level guest', (WidgetTester tester) async { final operation = Amplify.Storage.getUrl( - path: StoragePath.fromString('/public/$testObjectKey1'), + path: StoragePath.fromString('public/$testObjectKey1'), options: const StorageGetUrlOptions( pluginOptions: S3GetUrlPluginOptions( validateObjectExistence: true, @@ -590,7 +590,7 @@ void main() { (WidgetTester tester) async { final operation = Amplify.Storage.getUrl( path: StoragePath.withIdentityId( - (identityId) => '/protected/$identityId/$testObjectKey2', + (identityId) => 'protected/$identityId/$testObjectKey2', ), options: const StorageGetUrlOptions( pluginOptions: S3GetUrlPluginOptions(), @@ -606,7 +606,7 @@ void main() { (WidgetTester tester) async { final operation = Amplify.Storage.getUrl( path: StoragePath.withIdentityId( - (identityId) => '/private/$identityId/$testObjectKey3', + (identityId) => 'private/$identityId/$testObjectKey3', ), options: const StorageGetUrlOptions( pluginOptions: S3GetUrlPluginOptions( @@ -670,7 +670,7 @@ void main() { '/protected/$user1IdentityId/$testObjectKey2', ), destination: StoragePath.withIdentityId( - (identityId) => '/private/$identityId/$testObjectKey2', + (identityId) => 'private/$identityId/$testObjectKey2', ), options: const StorageCopyOptions( pluginOptions: S3CopyPluginOptions( @@ -698,7 +698,7 @@ void main() { testBytes, contentType: 'text/plain', ), - path: StoragePath.fromString('/private/$fileKey'), + path: StoragePath.fromString('private/$fileKey'), options: StorageUploadDataOptions( metadata: { 'filename': fileNameTemp, @@ -719,7 +719,7 @@ void main() { // Clean up files from this test. await Amplify.Storage.removeMany( paths: uploadedKeys - .map((key) => StoragePath.fromString('/private/$key')) + .map((key) => StoragePath.fromString('private/$key')) .toList(), ).result; }); @@ -741,7 +741,7 @@ void main() { testBytes, contentType: 'text/plain', ), - path: StoragePath.fromString('/private/$fileKey'), + path: StoragePath.fromString('private/$fileKey'), options: StorageUploadDataOptions( metadata: { 'filename': fileNameTemp, @@ -769,7 +769,7 @@ void main() { // Clean up files from this test. await Amplify.Storage.removeMany( paths: uploadedKeys - .map((key) => StoragePath.fromString('/private/$key')) + .map((key) => StoragePath.fromString('private/$key')) .toList(), ).result; }); @@ -780,14 +780,14 @@ void main() { final result1 = await Amplify.Storage.uploadData( data: HttpPayload.string('obj1'), path: StoragePath.withIdentityId( - (identityId) => '/private/$identityId/remove-test/obj1', + (identityId) => 'private/$identityId/remove-test/obj1', ), ).result; final result2 = await Amplify.Storage.uploadData( data: HttpPayload.string('obj2'), path: StoragePath.withIdentityId( - (identityId) => '/private/$identityId/remove-test/obj2', + (identityId) => 'private/$identityId/remove-test/obj2', ), ).result; @@ -826,14 +826,14 @@ void main() { final result1 = await Amplify.Storage.uploadData( data: HttpPayload.string('obj1'), path: StoragePath.withIdentityId( - (identityId) => '/private/$identityId/remove-test-user-a/obj1', + (identityId) => 'private/$identityId/remove-test-user-a/obj1', ), ).result; final result2 = await Amplify.Storage.uploadData( data: HttpPayload.string('obj2'), path: StoragePath.withIdentityId( - (identityId) => '/private/$identityId/remove-test-user-a/obj2', + (identityId) => 'private/$identityId/remove-test-user-a/obj2', ), ).result; diff --git a/packages/storage/amplify_storage_s3/example/lib/main.dart b/packages/storage/amplify_storage_s3/example/lib/main.dart index cfc19e02756..844e8a7ba03 100644 --- a/packages/storage/amplify_storage_s3/example/lib/main.dart +++ b/packages/storage/amplify_storage_s3/example/lib/main.dart @@ -147,7 +147,7 @@ class _HomeScreenState extends State { platformFile.readStream!, size: platformFile.size, ), - path: StoragePath.fromString('/public/${platformFile.name}'), + path: StoragePath.fromString('public/${platformFile.name}'), onProgress: (p) => _logger.debug('Uploading: ${p.transferredBytes}/${p.totalBytes}'), ).result; @@ -212,7 +212,7 @@ class _HomeScreenState extends State { }) async { try { await Amplify.Storage.remove( - path: StoragePath.fromString('/public/$key'), + path: StoragePath.fromString('public/$key'), ).result; setState(() { // set the imageUrl to empty if the deleted file is the one being displayed diff --git a/packages/storage/amplify_storage_s3_dart/lib/src/model/s3_item.dart b/packages/storage/amplify_storage_s3_dart/lib/src/model/s3_item.dart index 51a09d3e215..e9977cdcce2 100644 --- a/packages/storage/amplify_storage_s3_dart/lib/src/model/s3_item.dart +++ b/packages/storage/amplify_storage_s3_dart/lib/src/model/s3_item.dart @@ -71,8 +71,7 @@ class S3Item extends StorageItem return S3Item( key: keyDroppedPrefix, - // TODO(Jordan-Nelson): remove when path validation logic is updated - path: '/$key', + path: key, size: object.size?.toInt(), lastModified: object.lastModified, eTag: object.eTag, diff --git a/packages/storage/amplify_storage_s3_dart/lib/src/path_resolver/s3_path_resolver.dart b/packages/storage/amplify_storage_s3_dart/lib/src/path_resolver/s3_path_resolver.dart index dff80af88f9..fdf86c52aab 100644 --- a/packages/storage/amplify_storage_s3_dart/lib/src/path_resolver/s3_path_resolver.dart +++ b/packages/storage/amplify_storage_s3_dart/lib/src/path_resolver/s3_path_resolver.dart @@ -30,10 +30,10 @@ class S3PathResolver { // ignore: invalid_use_of_internal_member _ => path.resolvePath() }; - if (!resolvedPath.startsWith('/')) { + if (resolvedPath.startsWith('/')) { throw const StoragePathValidationException( - 'StoragePath must start with a leading "/"', - recoverySuggestion: 'Update the provided path to include a leading "/"', + 'StoragePath cannot start with a leading "/"', + recoverySuggestion: 'Remove the leading "/" from the StoragePath', ); } return resolvedPath; @@ -47,16 +47,13 @@ class S3PathResolver { paths.whereType().isNotEmpty; final identityId = requiredIdentityId ? await _identityProvider.getIdentityId() : null; - return (await Future.wait( + return Future.wait( paths.map( (path) => resolvePath( path: path, identityId: identityId, ), ), - )) - // TODO(Jordan-Nelson): remove once path validation logic is updated. - .map((path) => path.substring(1)) - .toList(); + ); } } diff --git a/packages/storage/amplify_storage_s3_dart/test/amplify_storage_s3_dart_test.dart b/packages/storage/amplify_storage_s3_dart/test/amplify_storage_s3_dart_test.dart index 64ba99a8239..13e5bf5058d 100644 --- a/packages/storage/amplify_storage_s3_dart/test/amplify_storage_s3_dart_test.dart +++ b/packages/storage/amplify_storage_s3_dart/test/amplify_storage_s3_dart_test.dart @@ -13,7 +13,7 @@ import 'test_utils/test_custom_prefix_resolver.dart'; import 'test_utils/test_path_resolver.dart'; import 'test_utils/test_token_provider.dart'; -const testPath = StoragePath.fromString('/some/path.txt'); +const testPath = StoragePath.fromString('some/path.txt'); void main() { const testDefaultStorageAccessLevel = StorageAccessLevel.guest; @@ -913,9 +913,9 @@ void main() { }); group('copy() API', () { - const testSource = StoragePath.fromString('/public/source-key'); + const testSource = StoragePath.fromString('public/source-key'); final testDestination = StoragePath.withIdentityId( - (identityId) => '/protected/$identityId/destination-key', + (identityId) => 'protected/$identityId/destination-key', ); final testResult = S3CopyResult( @@ -925,7 +925,7 @@ void main() { setUpAll(() { registerFallbackValue(const StorageCopyOptions()); registerFallbackValue( - const StoragePath.fromString('/public/source-key'), + const StoragePath.fromString('public/source-key'), ); }); diff --git a/packages/storage/amplify_storage_s3_dart/test/path_resolver/path_resolver_test.dart b/packages/storage/amplify_storage_s3_dart/test/path_resolver/path_resolver_test.dart index 4874a36dfd1..e37de5bab80 100644 --- a/packages/storage/amplify_storage_s3_dart/test/path_resolver/path_resolver_test.dart +++ b/packages/storage/amplify_storage_s3_dart/test/path_resolver/path_resolver_test.dart @@ -31,24 +31,23 @@ void main() { ); test('should resolve static strings', () async { - const path = StoragePath.fromString('/foo/bar/picture.png'); + const path = StoragePath.fromString('foo/bar/picture.png'); expect( await pathResolver.resolvePath(path: path), - '/foo/bar/picture.png', + 'foo/bar/picture.png', ); }); test('should resolve path with identity Id', () async { - final path = StoragePath.withIdentityId((id) => '/foo/$id/picture.png'); + final path = StoragePath.withIdentityId((id) => 'foo/$id/picture.png'); expect( await pathResolver.resolvePath(path: path), - '/foo/mock-id/picture.png', + 'foo/mock-id/picture.png', ); }); - test('should throw if the path does not start with a leading "/"', - () async { - const path = StoragePath.fromString('foo/bar/picture.png'); + test('should throw if the path starts with a leading "/"', () async { + const path = StoragePath.fromString('/foo/bar/picture.png'); expect( () => pathResolver.resolvePath(path: path), throwsA(isA()), diff --git a/packages/storage/amplify_storage_s3_dart/test/storage_s3_service/storage_s3_service_test.dart b/packages/storage/amplify_storage_s3_dart/test/storage_s3_service/storage_s3_service_test.dart index ad85f2814c2..b32696cffbc 100644 --- a/packages/storage/amplify_storage_s3_dart/test/storage_s3_service/storage_s3_service_test.dart +++ b/packages/storage/amplify_storage_s3_dart/test/storage_s3_service/storage_s3_service_test.dart @@ -19,7 +19,7 @@ import '../test_utils/test_path_resolver.dart'; import '../test_utils/test_token_provider.dart'; const testDelimiter = '#'; -const testPath = StoragePath.fromString('/some/path.txt'); +const testPath = StoragePath.fromString('some/path.txt'); class TestPrefixResolver implements S3PrefixResolver { @override @@ -537,7 +537,7 @@ void main() { expect( storageS3Service.getProperties( - path: const StoragePath.fromString('/a key'), + path: const StoragePath.fromString('a key'), options: testOptions, ), throwsA(isA()), @@ -557,7 +557,7 @@ void main() { expect( storageS3Service.getProperties( - path: const StoragePath.fromString('/a key'), + path: const StoragePath.fromString('a key'), options: testOptions, ), throwsA(isA()), @@ -930,8 +930,8 @@ void main() { group('copy() API', () { late S3CopyResult copyResult; - const testSource = StoragePath.fromString('/public/source'); - const testDestination = StoragePath.fromString('/public/destination'); + const testSource = StoragePath.fromString('public/source'); + const testDestination = StoragePath.fromString('public/destination'); setUpAll(() { registerFallbackValue( @@ -1079,7 +1079,7 @@ void main() { group('remove() API', () { late S3RemoveResult removeResult; - const testPath = StoragePath.fromString('/object-to-remove'); + const testPath = StoragePath.fromString('object-to-remove'); setUpAll(() { registerFallbackValue(