Skip to content
This repository was archived by the owner on Feb 26, 2025. It is now read-only.

Add support for querying and modifying ACLs #71

Open
jvz opened this issue Mar 15, 2017 · 5 comments
Open

Add support for querying and modifying ACLs #71

jvz opened this issue Mar 15, 2017 · 5 comments

Comments

@jvz
Copy link

jvz commented Mar 15, 2017

I started working on a proof of concept of this locally, and the main things I figured out for this are:

  • Can roughly map the AclEntryPermission enum to Permission and back.
  • Should provide an implementation of AclFileAttributeView which can be used to query and modify ACLs (which can be checked against cached ACLs ahead of time to prevent unnecessary network requests).
  • Need to add "acl" as a supportedFileAttributeViews() value in S3FileSystem.
    • On a related note, returning "basic" in this method but not supporting BasicFileAttributeView is most likely a bug (see Attribute Views #67).
  • In order to more properly map UserPrincipal to Grantee, I think using just the identifier as the UserPrincipal::getName value makes more sense (easier to map to the GroupGrantee enum here since these are all known URIs).

Now I'm not an S3 expert by any means, but this was how I started mapping from AWS permissions to AclEntryPermissions:

    public Set<AclEntryPermission> toAclEntryPermissions(Permission permission) {
        switch (permission) {
            case FullControl:
                return EnumSet.allOf(AclEntryPermission.class);
            case Read:
                return EnumSet.of(AclEntryPermission.READ_ATTRIBUTES, AclEntryPermission.READ_DATA, AclEntryPermission.READ_NAMED_ATTRS);
            case Write:
                return EnumSet.of(AclEntryPermission.WRITE_ATTRIBUTES, AclEntryPermission.WRITE_DATA, AclEntryPermission.WRITE_NAMED_ATTRS, AclEntryPermission.APPEND_DATA, AclEntryPermission.DELETE);
            case ReadAcp:
                return EnumSet.of(AclEntryPermission.READ_ACL);
            case WriteAcp:
                return EnumSet.of(AclEntryPermission.WRITE_ACL);
            default:
                throw new IllegalStateException("Unknown Permission: " + permission);
        }
    }
@jvz
Copy link
Author

jvz commented Mar 15, 2017

Also, I had some other ideas for the AclFileAttributeView:

  • The UserPrincipal object from getOwner() should just be an S3UserPrincipal with a name of the bucket or object owner ID.
  • When converting from AccessControlList to List<AclEntry>, you need to combine Grants as each one only has a single permission, but an AclEntry contains all permissions for a single principal.
  • AclEntryType seems to be ALLOW since S3 doesn't return other types of ACLs. However, modifying ACLs via the NIO API using DENY could be implemented by removing that permission if it's already there. I'm not sure if there are equivalents for the other two types.

@amarcionek
Copy link

I'm willing to help with this effort. It seems you and I have recently reached a shared interest in this project. I have an immediate need for the port change (see #68, not as good as your #70), ACL support (and also support when a cloud DOESN'T support ACL, library currently throws AmazonS3Exception,) and a number of other things including a fairly responsive maintainer if I'm going to spend resources on this. It seems to me that @jarnaiz is the maintainer here? I'd be interested in feedback there or offer up myself as a maintainer as well?

This project is MIT licensed, so I may start doing significant work in my fork if you want to collaborate there.

@jvz
Copy link
Author

jvz commented Mar 16, 2017

I actually proposed starting work on Apache Commons VFS 3 just yesterday which would be a rewrite of VFS2 to use the NIO2 API. I'm getting a bit of interest there, and this would be a great feature to bring into that as well.

And that lack of ACL support is exactly another issue with integration testing via s3mock for example.

@jarnaiz
Copy link
Member

jarnaiz commented Mar 16, 2017

Hi @jvz and @amarcionek

Thanks for all your interest on this project.

If you want to contribute with the project you can do a PR and I will be happy to check it, review it and include you as developers in the project info.

If you want to be a mantainers with writing permissions, great, but we should talk about style guideliness and the roadmap... but no problem :)

My company Upplication is using this project in production and we want to continue to mantain it in the long term :)

I want to review this issue and the #68 and #70 tomorrow or maybe this weekend. Until I can investigate further you can look at this method: com.upplication.s3fs.util.S3Utils#toPosixFilePermission

Cheers!

@jvz
Copy link
Author

jvz commented Mar 16, 2017

I've been debating internally at my work about whether we're going to abstract the S3 service itself by using an S3-compatible file store, or if we're going to abstract our API usage via libraries such as this. The way things are going, it sounds like we'll be going the API route, so continued development of this library will be very useful. I'll need to get permission to contribute non-trivial PRs on company time, however. I'd be happy to help in that case. As mentioned above, this library would be useful in the future Apache Commons VFS 3.x project, but we'll discuss integration later on.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants