Review feedback

pixl8 · Jul 4, 2023 · 88aba2f · 88aba2f
1 parent 3e1809a
commit 88aba2f
Show file tree

Hide file tree

Showing 6 changed files with 98 additions and 14 deletions.
diff --git a/i18n/storage-providers/s3.properties b/i18n/storage-providers/s3.properties
@@ -13,4 +13,6 @@ field.s3rootUrl.placeholder=e.g. https://s3.amazon.com, or https://s3-eu-west-2.
 field.s3subpath.title=Root path
 field.s3subpath.placeholder=i.e. subfolder within your bucker. e.g. /mysite/live/uploads
 
-validation.connection.error=There was an error connecting to S3 using the credentials you supplied. Either the credentials are invalid/do not have access to the bucket, the bucket does not exist, or the bucket does not exist in the supplied region.
+validation.connection.error=There was an error connecting to S3 using the credentials you supplied. Please check the credentials access and try again.
+validation.bucket.error=There was an error accessing the provided bucket. Either the bucket does not exist, or the configured credentials do not have access to it.
+validation.region.error=The configured region does not match the region of the configured bucket. Please enter the correct region and try again.
diff --git a/java/pom.xml b/java/pom.xml
@@ -4,7 +4,7 @@
 	<artifactId>s3storageprovider</artifactId>
 	<packaging>jar</packaging>
 	<version>1.0.0</version>
-	<name>Lucee Undertow Server</name>
+	<name>S3 Storage Provider AWS service for Preside</name>
 	<url>http://maven.apache.org</url>
 
 	<dependencyManagement>

diff --git a/java/src/main/java/org/pixl8/s3storageprovider/Service.java b/java/src/main/java/org/pixl8/s3storageprovider/Service.java
@@ -44,22 +44,46 @@ public Service( String region, String bucket, String accessKey, String secretKey
 		                    .build();
 	}
 
+
 	/**
 	 * Returns true if there are no errors making an API call to get the configured
 	 * bucket
 	 */
+	public boolean checkS3Access() {
+		try {
+			_s3Client.listBuckets();
+		} catch( Exception e ) {
+			return false;
+		}
+
+		return true;
+	}
+
+	/**
+	 * Returns true if there are no errors making an head API call to the bucket
+	 */
 	public boolean checkBucketAccess() {
 		try {
-			GetBucketLocationResponse resp = _s3Client.getBucketLocation( GetBucketLocationRequest.builder().bucket( _bucket ).build() );
-			return resp.locationConstraintAsString().equals( _region );
-		} catch( Exception e ) {}
+			_s3Client.headBucket( HeadBucketRequest.builder().bucket( _bucket ).build() );
+		} catch( Exception e ) {
+			return false;
+		}
+
+		return true;
+	}
 
-		return false;
+	/**
+	 * Returns true if there are no errors making an API call to get the configured
+	 * bucket and that the bucket
+	 */
+	public boolean checkBucketRegion() {
+		GetBucketLocationResponse resp = _s3Client.getBucketLocation( GetBucketLocationRequest.builder().bucket( _bucket ).build() );
+		return resp.locationConstraintAsString().equals( _region );
 	}
 
 	/**
 	 * Returns a cfquery result of objects matching the provided
-	 * prefix. This ensures no converstion necessary from CFML code
+	 * prefix. This ensures no conversion necessary from CFML code
 	 * after calling this java method.
 	 *
 	 */

diff --git a/services/S3StorageProvider.cfc b/services/S3StorageProvider.cfc
@@ -49,16 +49,38 @@ component implements="preside.system.services.fileStorage.StorageProvider" displ
 		var hasAccess = false;
 
 		try {
-			hasAccess = s3Service.checkBucketAccess();
+			hasAccess = s3Service.checkS3Access();
 		} catch( any e ) {
+			hasAccess = false;
+		}
+		if ( !hasAccess ) {
 			validationResult.addError( "s3accessKey", "storage-providers.s3:validation.connection.error" );
 			return;
 		}
 
+		try {
+			hasAccess = s3Service.checkBucketRegion();
+		} catch( any e ) {
+			if ( e.message contains "the region '#( arguments.configuration.s3region ?: "" )#' is wrong" ) {
+				hasAccess = false;
+			}
+		}
 		if ( !hasAccess ) {
-			validationResult.addError( "s3accessKey", "storage-providers.s3:validation.connection.error" );
+			validationResult.addError( "s3region", "storage-providers.s3:validation.region.error" );
+			return;
+		}
+
+		try {
+			hasAccess = s3Service.checkBucketAccess();
+		} catch( any e ) {
+			hasAccess = false;
+		}
+		if ( !hasAccess ) {
+			validationResult.addError( "s3bucket", "storage-providers.s3:validation.bucket.error" );
 			return;
 		}
+
+
 	}
 
 	public query function listObjects( required string path, boolean private=false ){

diff --git a/tests/Application.cfc b/tests/Application.cfc
@@ -21,16 +21,15 @@ component {
 		if ( !StructKeyExists( application, "_credentialsVerified" ) || !application._credentialsVerified ) {
 			var env = CreateObject("java", "java.lang.System").getEnv();
 
-
 			application.TEST_S3_ACCESS_KEY = Trim( env.TEST_S3_ACCESS_KEY ?: "" );
 			application.TEST_S3_SECRET_KEY = Trim( env.TEST_S3_SECRET_KEY ?: "" );
 			application.TEST_S3_BUCKET     = Trim( env.TEST_S3_BUCKET     ?: "" );
 			application.TEST_S3_REGION     = Trim( env.TEST_S3_REGION     ?: "" );
 
 			application._credentialsVerified = Len( application.TEST_S3_ACCESS_KEY ) &&
-											   Len( application.TEST_S3_SECRET_KEY ) &&
-											   Len( application.TEST_S3_BUCKET     ) &&
-											   Len( application.TEST_S3_REGION     );
+			                                   Len( application.TEST_S3_SECRET_KEY ) &&
+			                                   Len( application.TEST_S3_BUCKET     ) &&
+			                                   Len( application.TEST_S3_REGION     );
 		}
 
 		header statuscode=500;

diff --git a/tests/unit/S3StorageProviderTest.cfc b/tests/unit/S3StorageProviderTest.cfc
@@ -6,12 +6,49 @@ component extends="testbox.system.BaseSpec" {
 				var svc = _getService();
 				var validationResult = new preside.system.services.validation.ValidationResult();
 
-				svc.validate( { s3accessKey="xxx", s3secretKey="xxx" }, validationResult );
+				svc.validate( {
+					  s3accessKey="xxx"
+					, s3secretKey="xxx"
+				}, validationResult );
 
 				var messages = validationResult.getMessages();
 
 				expect( messages.s3accesskey.message ?: "" ).toBe( "storage-providers.s3:validation.connection.error" );
 			} );
+			it( "should add a validation error when the credentials are correct but the bucket does not exist (or user does not have access)", function(){
+				var svc = _getService();
+				var validationResult = new preside.system.services.validation.ValidationResult();
+
+				svc.validate( {
+					  s3accessKey = application.TEST_S3_ACCESS_KEY
+					, s3secretKey = application.TEST_S3_SECRET_KEY
+					, s3Bucket    = CreateUUId()
+				}, validationResult );
+
+				var messages = validationResult.getMessages();
+
+				expect( messages.s3bucket.message ?: "" ).toBe( "storage-providers.s3:validation.bucket.error" );
+			} );
+			it( "should add a validation error when the credentials are correct but the bucket region is different to the configured region", function(){
+				var svc = _getService();
+				var validationResult = new preside.system.services.validation.ValidationResult();
+				var dummyRegion = "us-east-1";
+
+				if ( dummyRegion == application.TEST_S3_REGION ) {
+					dummyRegion = "eu-west-1";
+				}
+
+				svc.validate( {
+					  s3accessKey = application.TEST_S3_ACCESS_KEY
+					, s3secretKey = application.TEST_S3_SECRET_KEY
+					, s3Bucket    = application.TEST_S3_BUCKET
+					, s3Region    = dummyRegion
+				}, validationResult );
+
+				var messages = validationResult.getMessages();
+
+				expect( messages.s3region.message ?: "" ).toBe( "storage-providers.s3:validation.region.error" );
+			} );
 			it( "should do nothing when credentials are correct", function(){
 				var svc = _getService();
 				var validationResult = new preside.system.services.validation.ValidationResult();