ACL checks are not sufficient for checking access. Access is falsely denied when bucket policy is used.

src/main/java/org/carlspring/cloud/storage/s3fs/S3AccessControlList.java

@@ -2,14 +2,9 @@
 
 import java.nio.file.AccessDeniedException;
 import java.nio.file.AccessMode;
-import java.util.EnumSet;
-import java.util.stream.StreamSupport;
 
 import software.amazon.awssdk.services.s3.model.Grant;
 import software.amazon.awssdk.services.s3.model.Owner;
-import software.amazon.awssdk.services.s3.model.Permission;
-import software.amazon.awssdk.utils.StringUtils;
-import static java.lang.String.format;
 
 public class S3AccessControlList
 {
@@ -18,61 +13,55 @@ public class S3AccessControlList
 
  private final String key;
 
- private final Iterable<Grant> grants;
-
- private final Owner owner;
 
+ /**
+ * Creates a new S3AccessControlList
+ *
+ * @param fileStoreName
+ * @param key
+ * @param grants unused
+ * @param owner unused
+ * @deprecated use {@link #S3AccessControlList(String, String)}
+ */
+ @Deprecated
  public S3AccessControlList(final String fileStoreName,
  final String key,
- final Iterable<Grant> grants,
- final Owner owner)
+ final Iterable<Grant> grants, //unused, but keeping to preserve signature
+ final Owner owner //unused, but keeping to preserve signature
+ )
  {
  this.fileStoreName = fileStoreName;
- this.grants = grants;
  this.key = key;
- this.owner = owner;
- }
-
- public String getKey()
- {
- return key;
  }
 
  /**
- * have almost one of the permission set in the parameter permissions
+ * Creates a new S3AccessControlList
  *
- * @param permissions almost one
- * @return
+ * @param fileStoreName
+ * @param key
  */
- private boolean hasPermission(final EnumSet<Permission> permissions)
+ public S3AccessControlList(final String fileStoreName, final String key)
  {
- return StreamSupport.stream(grants.spliterator(), false)
- .anyMatch(grant -> StringUtils.equals(grant.grantee().id(), owner.id()) &&
- permissions.contains(grant.permission()));
+ this.fileStoreName = fileStoreName;
+ this.key = key;
+ }
+
+ public String getKey()
+ {
+ return key;
  }
 
  public void checkAccess(final AccessMode[] modes)
  throws AccessDeniedException
  {
  for (AccessMode accessMode : modes)
  {
- switch (accessMode)
+ // Checking the ACL grants is not sufficient to determine access as bucket policy may override ACL.
+ // Any permission problems will have to be handled at time of access.
+ if (accessMode == AccessMode.EXECUTE)
  {
- case EXECUTE:
- throw new AccessDeniedException(fileName(), null, "file is not executable");
- case READ:
- if (!hasPermission(EnumSet.of(Permission.FULL_CONTROL, Permission.READ)))
- {
- throw new AccessDeniedException(fileName(), null, "file is not readable");
- }
- break;
- case WRITE:
- if (!hasPermission(EnumSet.of(Permission.FULL_CONTROL, Permission.WRITE)))
- {
- throw new AccessDeniedException(fileName(), null,
- format("bucket '%s' is not writable", fileStoreName));
- }
- break;
+ throw new AccessDeniedException(fileName(), null, "file is not executable");
+ }
  }
  }
  }

src/main/java/org/carlspring/cloud/storage/s3fs/S3FileSystemProvider.java

@@ -57,6 +57,7 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
 import software.amazon.awssdk.core.ResponseInputStream;
+import software.amazon.awssdk.core.exception.SdkException;
 import software.amazon.awssdk.core.sync.RequestBody;
 import software.amazon.awssdk.services.s3.S3Client;
 import software.amazon.awssdk.services.s3.model.Bucket;
@@ -718,32 +719,26 @@ public FileStore getFileStore(Path path)
 
  @Override
  public void checkAccess(Path path, AccessMode... modes)
-  throws IOException
+ throws IOException
  {
  S3Path s3Path = toS3Path(path);
 
  Preconditions.checkArgument(s3Path.isAbsolute(), "path must be absolute: %s", s3Path);
 
- if (modes.length == 0)
+ if (!exists(s3Path))
  {
- if (exists(s3Path))
- {
- return;
- }
-
  throw new NoSuchFileException(toString());
  }
 
- final S3Object s3Object = s3Utils.getS3Object(s3Path);
- final String key = s3Object.key();
- final String bucket = s3Path.getFileStore().name();
- final GetObjectAclRequest request = GetObjectAclRequest.builder().bucket(bucket).key(key).build();
- final S3Client client = s3Path.getFileSystem().getClient();
- final List<Grant> grants = client.getObjectAcl(request).grants();
- final Owner owner = s3Path.getFileStore().getOwner();
- final S3AccessControlList accessControlList = new S3AccessControlList(bucket, key, grants, owner);
+ if (modes.length > 0)
+ {
+ final S3Object s3Object = s3Utils.getS3Object(s3Path);
+ final String key = s3Object.key();
+ final String bucket = s3Path.getFileStore().name();
+ final S3AccessControlList accessControlList = new S3AccessControlList(bucket, key);
 
- accessControlList.checkAccess(modes);
+ accessControlList.checkAccess(modes);
+ }
  }
 
 
@@ -949,6 +944,12 @@ private <T> void verifySupportedOptions(Set<? extends T> allowedOptions, Set<? e
  Preconditions.checkArgument(unsupported.isEmpty(), "the following options are not supported: %s", unsupported);
  }
 
+ private static boolean isBucketRoot(S3Path s3Path)
+ {
+ String key = s3Path.getKey();
+ return key.equals("") || key.equals("/");
+ }
+
  /**
  * check that the paths exists or not
  *
@@ -958,6 +959,21 @@ private <T> void verifySupportedOptions(Set<? extends T> allowedOptions, Set<? e
  boolean exists(S3Path path)
  {
  S3Path s3Path = toS3Path(path);
+
+ if(isBucketRoot(s3Path))
+ {
+ // check to see if bucket exists
+ try
+ {
+ s3Utils.listS3Objects(s3Path);
+ return true;
+ }
+ catch (SdkException e)
+ {
+ return false;
+ }
+ }
+
  try
  {
  s3Utils.getS3Object(s3Path);

src/main/java/org/carlspring/cloud/storage/s3fs/util/S3Utils.java

@@ -16,14 +16,15 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import software.amazon.awssdk.core.exception.SdkClientException;
+import software.amazon.awssdk.core.exception.SdkException;
 import software.amazon.awssdk.services.s3.S3Client;
 import software.amazon.awssdk.services.s3.model.GetObjectAclRequest;
 import software.amazon.awssdk.services.s3.model.GetObjectAclResponse;
 import software.amazon.awssdk.services.s3.model.Grant;
 import software.amazon.awssdk.services.s3.model.HeadObjectRequest;
 import software.amazon.awssdk.services.s3.model.HeadObjectResponse;
 import software.amazon.awssdk.services.s3.model.ListObjectsV2Request;
-import software.amazon.awssdk.services.s3.model.ListObjectsV2Response;
+import software.amazon.awssdk.services.s3.model.NoSuchBucketException;
 import software.amazon.awssdk.services.s3.model.Owner;
 import software.amazon.awssdk.services.s3.model.Permission;
 import software.amazon.awssdk.services.s3.model.S3Exception;
@@ -38,6 +39,44 @@ public class S3Utils
 
  private static final Logger LOGGER = LoggerFactory.getLogger(S3Utils.class);
 
+ /**
+ * Returns a list of {@link S3Object}s for the given path.
+ *
+ * @param s3Path {@link S3Path}
+ * @return {@link List} of {@link S3Object}
+ * @throws NoSuchBucketException
+ * The specified bucket does not exist.
+ * @throws SdkException
+ * Base class for all exceptions that can be thrown by the SDK (both service and client). Can be used for
+ * catch all scenarios.
+ * @throws SdkClientException
+ * If any client side error occurs such as an IO related failure, failure to get credentials, etc.
+ * @throws S3Exception
+ * Base class for all service exceptions.
+ */
+ public List<S3Object> listS3Objects(S3Path s3Path)
+ {
+
+ final String key = s3Path.getKey();
+ final String bucketName = s3Path.getFileStore().name();
+ final S3Client client = s3Path.getFileSystem().getClient();
+
+ // is a virtual directory
+ String keyFolder = key;
+ if (!keyFolder.endsWith("/"))
+ {
+ keyFolder += "/";
+ }
+
+ final ListObjectsV2Request request = ListObjectsV2Request.builder()
+ .bucket(bucketName)
+ .prefix(keyFolder)
+ .maxKeys(1)
+ .build();
+
+ return client.listObjectsV2(request).contents();
+ }
+
  /**
  * Get the {@link S3Object} that represent this Path or her first child if this path not exists
  *
@@ -95,23 +134,10 @@ public S3Object getS3Object(S3Path s3Path)
  // try to find the element as a directory.
  try
  {
- // is a virtual directory
- String keyFolder = key;
- if (!keyFolder.endsWith("/"))
- {
- keyFolder += "/";
- }
-
- final ListObjectsV2Request request = ListObjectsV2Request.builder()
- .bucket(bucketName)
- .prefix(keyFolder)
- .maxKeys(1)
- .build();
-
- final ListObjectsV2Response current = client.listObjectsV2(request);
- if (!current.contents().isEmpty())
+ List<S3Object> s3Objects = listS3Objects(s3Path);
+ if (!s3Objects.isEmpty())
  {
- return current.contents().get(0);
+ return s3Objects.get(0);
  }
  }
  catch (Exception e)

src/test/java/org/carlspring/cloud/storage/s3fs/fileSystemProvider/CheckAccessTest.java

@@ -1,5 +1,6 @@
 package org.carlspring.cloud.storage.s3fs.fileSystemProvider;
 
+import java.nio.file.NoSuchFileException;
 import org.carlspring.cloud.storage.s3fs.S3FileSystem;
 import org.carlspring.cloud.storage.s3fs.S3Path;
 import org.carlspring.cloud.storage.s3fs.S3UnitTestBase;
@@ -18,12 +19,8 @@
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
-import software.amazon.awssdk.services.s3.model.GetObjectAclRequest;
-import software.amazon.awssdk.services.s3.model.GetObjectAclResponse;
-import software.amazon.awssdk.services.s3.model.Owner;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.mockito.Mockito.doReturn;
 
 class CheckAccessTest
  extends S3UnitTestBase
@@ -64,27 +61,6 @@ void checkAccessRead()
  s3fsProvider.checkAccess(file1, AccessMode.READ);
  }
 
- @Test
- void checkAccessReadWithoutPermission()
- throws IOException
- {
- // fixtures
- final S3ClientMock client = S3MockFactory.getS3ClientMock();
- final String bucketName = "bucketA";
- final String directory = "dir";
- client.bucket(bucketName).dir(directory);
-
- final FileSystem fs = createNewS3FileSystem();
- final String path = String.format("/%s/%s", bucketName, directory);
- final Path file1 = fs.getPath(path);
-
- // We're expecting an exception here to be thrown
- final Exception exception = assertThrows(AccessDeniedException.class,
- () -> s3fsProvider.checkAccess(file1, AccessMode.READ));
-
- // TODO: Assert that the exception message is as expected
- assertNotNull(exception);
- }
 
  @Test
  void checkAccessWrite()
@@ -105,59 +81,26 @@ void checkAccessWrite()
  }
 
  @Test
- void checkAccessWriteDifferentUser()
- throws IOException
- {
- // fixtures
- final S3ClientMock client = S3MockFactory.getS3ClientMock();
- final String bucketName = "bucketA";
- final String directory = "dir";
- final String filePath = directory + "/readOnly";
- client.bucket(bucketName).dir(directory).file(filePath);
-
- // return empty list
- final Owner owner = Owner.builder().id("2").displayName("Read Only").build();
- final GetObjectAclRequest request = GetObjectAclRequest.builder().bucket(bucketName).key(filePath).build();
- final GetObjectAclResponse response = GetObjectAclResponse.builder().owner(owner).build();
- doReturn(response).when(client).getObjectAcl(request);
-
- final S3FileSystem fs = createNewS3FileSystem();
- final String path = String.format("/%s/%s", bucketName, filePath);
- final S3Path file1 = fs.getPath(path);
-
- // We're expecting an exception here to be thrown
- final Exception exception = assertThrows(AccessDeniedException.class,
- () -> s3fsProvider.checkAccess(file1, AccessMode.WRITE));
-
- assertNotNull(exception);
- }
-
- @Test
- void checkAccessWriteWithoutPermission()
- throws IOException
+ void checkAccessMissingFile()
+ throws IOException
  {
  // fixtures
  final S3ClientMock client = S3MockFactory.getS3ClientMock();
  final String bucketName = "bucketA";
  final String directory = "dir";
- client.bucket("bucketA").dir(directory);
-
- // return empty list
- final GetObjectAclResponse response = GetObjectAclResponse.builder().build();
- final GetObjectAclRequest request = GetObjectAclRequest.builder().bucket(bucketName).key(
- directory + "/").build();
- doReturn(response).when(client).getObjectAcl(request);
+ client.bucket("bucketA");
 
  final String path = String.format("/%s/%s", bucketName, directory);
  final Path file1 = createNewS3FileSystem().getPath(path);
 
  // We're expecting an exception here to be thrown
- final Exception exception = assertThrows(AccessDeniedException.class,
+ final Exception exception = assertThrows(NoSuchFileException.class,
  () -> s3fsProvider.checkAccess(file1, AccessMode.WRITE));
 
  assertNotNull(exception);
  }
 
+
  @Test
  void checkAccessExecute()
  throws IOException