Can we look into making ACL operations really nice for users?

@ryanseys

** revised proposal: #351 (comment) **

Hi guys,

In gcloud-node, the ACL methods are very closely tied to the API specification, for example:

myBucket.acl.add({
  scope: '[email protected]',
  permission: Storage.acl.OWNER_ROLE
}, function(err, aclObject) {});

There are definitely times where I want to do low level operations, but other times I want to first-class citizen methods for the different options... Additionally, I now have to know a few things about this API:

the name of the scope parameter
the name of the permission parameter
the fact that user's need to be prefixed with user-
the constant for the name of the role (OWNER_ROLE... is it READER_ROLE or READ_ROLE ?) and where to look for those constants when I inevitably forget them...

Can take a stab at making this more friendly? For example, it might be cool to have:

myBucket.acl.owners.addUser('[email protected]', function(err, aclObject) {});
myBucket.acl.readers.removeDomain('example.com', function(err, aclObject) {});
myBucket.acl.writers.addAllUsers(function(err, aclObject) {});
myBucket.acl.readers.addAllAuthenticatedUsers(function(err, aclObject) {});

Or we could go the same route that gcloud-python went with the grant_* and revoke_* directives acting as the "commit" operation, so the code would look like:

myBucket.acl.user('[email protected]').grantOwner(function(err, aclObject) {})
myBucket.acl.domain('example.com').revokeRead(function(err, aclObject) {})
myBucket.acl.allUsers().grantWrite(function(err, aclObject) {})
myBucket.acl.allAuthenticatedUsers().grantRead(function(err, aclObject) {})

Ideally you could chain this stuff together:

myBucket.acl.user('[email protected]').user('[email protected]').grantOwner(function(err, aclObject) {})

Thoughts?

/cc @ryanseys @stephenplusplus

I like the idea of having predefined bucket objects like .owners, .readers, .writers that we can add/remove users/domains, all users and such like the first example. The last chain example is weird because an operation on a user should not be another user, and might be better expressed as:

myBucket.acl.user(['[email protected]', '[email protected]']).grantOwner(function(){});

Additionally you should not be able to run a grant or revoke or whatever after a grant/revoke operation such as .grantOwner(function(){}).revokeReader(function(){}). That would be hard to understand again wouldn't make sense.
The abstraction might be a little deep here though, e.g. many layers and objects to understand to do an operation with a simple email address.

I prefer the first example to the second, and here's another suggestion:

var gcloud = require("gcloud")({ /*credentials*/ })
var ACL = gcloud.storage.acl
var myBucket = gcloud.storage.bucket("my-bucket")

var user = ACL.user("[email protected]")
var domain = ACL.domain("domain.com")

myBucket.acl.owners.add([user, domain], function(err) {})
myBucket.acl.writers.remove(user, function(err) {})

myBucket.acl.writers.add(ACL.ALL_USERS, function(err) {})
myBucket.acl.owners.add(ACL.AUTHENTICATED_USERS, function(err) {})

This way a dev only needs to know two methods (add/remove), and the rest should be logical.

I'm down with the owners/writers/readers and add/remove.

I'm not a fan of var user = ACL.user('...') -- it's more typing when we can figure out what people mean here, right?

I also am not a huge fan of the ACL.ALL_USERS thing because it's a second import that we could handle with a method...

I also think we should keep the shortcut for making public/private. So:

var bucket = gcloud.storage.bucket('...');
bucket.acl.owners.add('[email protected]', function(err) { });
bucket.acl.readers.remove('domain.com', function(err) { });

// Then the magic methods: addAll(), addAllAuthenticated(), makePublic(), makePrivate()
bucket.acl.writers.addAll(); 
bucket.acl.writers.addAllAuthenticated();
bucket.acl.makePublic();
bucket.acl.makePrivate();

Thoughts?

I'm not a fan of var user = ACL.user('...') -- it's more typing when we can figure out what people mean here, right?

https://cloud.google.com/storage/docs/json_api/v1/objectAccessControls There are many types of "entities" that the API expects in string format matching the following conventions:

user-userId
user-email
group-groupId
group-email
domain-domain
project-team-projectId
allUsers
allAuthenticatedUsers

I'm not sure parsing a string is going to be able to handle detecting what's a projectId vs a domain vs a groupId, etc. It would be easier to have methods for these, user, group, domain, team.

I still think the way the library is now, adding permissions to a scope through methods like add and remove, is the most straight-forward this can be. Better documentation to explain scope is expected in the format of the JSON API's entity property would definitely help, however.

Just to sum up my thoughts:

Keep the API the same as it is now
Document scope being the format of the entity property: https://cloud.google.com/storage/docs/json_api/v1/objectAccessControls
Add helper methods:
- (file || bucket).acl.makePublic(function(err) {})
- (file || bucket).acl.makePrivate(function(err) {})

I'm hugely -1 on scope and permission being the primary ways we do this, however I can see the concern regarding parsing strings.

To be clear: I still think the current way we do it (with .add({scope: ...})) should still exist -- I just want these in addition to that way. I cannot see myself ever wanting to type scope: 'user-' + someUsername, and very much see myself wanting to type .addUser(someUsername). I also recognize that Google might add "newCategory-something" to the list, so it makes sense to keep this syntax around.

Revised proposal

Keep acl.add() (and acl.remove()?)
Add owners/readers/writers that can be added/removed from.
Add methods for the "all's": .addAll(), .addAllAuthenticated(), .removeAll(), .removeAllAuthenticated()
Add methods for add/remove User/Domain/Project: .addUser(), .removeDomain(), etc
Add shortcut methods for private/public: .makePrivate(), .makePublic()

Example

var bucket = gcloud.storage.bucket('...');
bucket.acl.owners.addUser('[email protected]', function(err) { });
bucket.acl.readers.removeDomain('domain.com', function(err) { });

// Then the magic methods: addAll(), addAllAuthenticated(), makePublic(), makePrivate()
bucket.acl.writers.addAll(function(err) { ... }); 
bucket.acl.writers.addAllAuthenticated(function(err) { ... });
bucket.acl.makePublic(function(err) { ... });
bucket.acl.makePrivate(function(err) { ... });

If we make the helper methods for user, group, domain, project, allusers, allAuthenticated we will fit more closely with the gcloud-python library as well. Personally it's a little more verbose but it's easy to understand and I don't need to know prefixes at all! I'm really fond of stephen's suggestion earlier:

var gcloud = require("gcloud")({ /*credentials*/ })
var ACL = gcloud.storage.acl
var myBucket = gcloud.storage.bucket("my-bucket")

var user = ACL.user("[email protected]")
var domain = ACL.domain("domain.com")

myBucket.acl.owners.add([user, domain], function(err) {})
myBucket.acl.writers.remove(user, function(err) {})

myBucket.acl.writers.add(ACL.ALL_USERS, function(err) {})
myBucket.acl.owners.add(ACL.AUTHENTICATED_USERS, function(err) {})

Are we all -1 to the .add<type>() methods? (acl.writers.addUser('[email protected]'))

If so -- why is that? gcloud-python's syntax is slightly different, but along these lines (acl.user('[email protected]').grant_write())

@jgeewax

Sorry @jgeewax, missed your most recent comment. I'm happy with that suggestion as well with the added benefit that we don't have magic constants for all users and all authenticated users! 👍

I'm not -1 for addUser and removeDomain etc... in fact, I like those. We can keep generic add and remove for backward compatibility and more "to the book" users.