Consider adding some form of user group support

[dankrause]

Currently, our permission system has the following types of permission for a given operation on a given model:

PERMISSION_ANYONE
PERMISSION_LOGGED_IN_USER
PERMISSION_OWNER_USER
PERMISSION_ADMIN

This means that there is currently no built-in way to, for example, grant one non-admin user the ability to create an instance of a given model, but deny another non-admin user the same permission.

It would be possible for a rest_gae user to implement their own group system using a custom user model, and tying into the callbacks we provide. We may decide that this is acceptable, and any direct support for groups is out of scope for this project. In that case, this issue can be closed with no action taken.

It would also be possible to add some set of features that would either fully implement a working group system, or make it easier to implement one.

Here are some features that a user would expect of a full group system:

• The ability to assign one or more groups to a user
• The ability to set a default group for a self-created user
• The ability to assign a permission similar to PERMISSION_GROUP['foo', 'bar']
• The ability to limit which users can assign particular groups to other users

Here are some features that, if implemented, would ease user development of a group system, without actually implementing one:

• A PERMISSION_CUSTOM permission that would result in a callback to decide if permission is granted to access an object. Not sure how this would work with building queries.
• Defining a Permission base class that is used instead of constants. Each existing permission would subclass Permission, and we'd move some logic into it's methods. Users would provide their own subclasses. This would be an alternative to using callbacks. I haven't thought this one through, so I don't know what the implementation would look like.

[dankrause]

I thought about this a bit more, and I really like the Permission subclass route.

Consider this set of Permission subclasses:

class Permission(object):
    def pre_validate(method, model, user=None):
        return True
    def post_validate(method, instance, user=None):
        return True


class PermissionEveryone(Permission):
    pass


class PermissionAdmin(Permission):
    def __init__(self, admin_property):
        self.admin_property = admin_property
    def _user_is_admin(self, user):
        # use self.admin_property to determine if user is admin
    def pre_validate(method, model, user=None):
        return self._user_is_admin(user)
    def post_validate(method, instance, user=None):
        return self._user_is_admin(user)


class PermissionUser(Permission):
    def pre_validate(method, model, user=None):
        return user is not None
    def post_validate(method, instance, user=None):
        return user is not None


class PermissionOwner(Permission):
    def __init__(self, owner_property):
        self.owner_property = owner_property
    def post_validate(method, instance, user=None):
        # use self.owner_property to determine if user is owner

When assigning permissions, instead of saying this:

permissions={
        'GET': PERMISSION_ANYONE,
        'POST': PERMISSION_LOGGED_IN_USER,
        'PUT': PERMISSION_OWNER_USER,
        'DELETE': PERMISSION_ADMIN
      },

We'd say it this way:

permissions={
        'GET': [PermissionAnyone()],
        'POST': [PermissionUser()],
        'PUT': [PermissionOwner('owner'), PermissionAdmin('is_admin')],
        'DELETE': [PermissionAdmin('is_admin')]
      },

In rest_method_wrapper, we'd loop through permissions and call pre_validate and post_validate where appropriate, accepting permission on the first True return value, and throwing a 403 if all of them return False.

This allows us to define permissions like this:

class PermissionGroup(Permission):
    def __init__(self, group_property, *groups):
        self.group_property = group_property
        self.groups = set(groups)
    def post_validate(method, instance, user=None):
        # check the self.group_property of user to see if
        # any of the groups match self.groups

We'd then give our custom user class a groups property, and define permissions like this:

author_permissions = [PermissionGroup('groups', 'Editor', 'Author'), PermissionOwner('owner'), PermissionAdmin('is_admin')]
# ... snip ...
permissions={
        'GET': [PermissionAnyone()],
        'POST': author_permissions,
        'PUT': author_permissions,
        'DELETE': author_permissions
      },

And suddenly we have the beginnings of a group system. This could even be fully backwards compatible with the existing permissions API by defining PERMISSION_ANYONE = [PermissionAnyone()] etc. in rest_gae.py.

There may still be flaws with this approach, but it seems like a pretty good idea to me so far.

[budowski]

Nice!!!!! I really like this approach too. Questions:

1. When do we call post_validate and when do we call pre_validate? At what exact stages?
2. I'm trying to think what is better in terms of convenience for the programmer when specifying the name of a property (e.g. admin_property / user_owner_property): Specifying it in the Permission class constructor vs. in the RESTMeta class.
3. Note that we still need RESTMeta.user_owner_property since that property is being set as part of the the post/put requests. Or will that be now as part of the permission.post_validate method?

[dankrause]

1. Permission.pre_validate would be called before fetching anything from the data store. It's useful for preventing not-logged-in users from even causing a request to make it to the data store. Permission.post_validate would be called once we've retrieved something from the datastore, and we need to ensure that the user is allowed to interact with that particular object. This is needed for PermissionOwner (and the implementation of PermissionGroup that I have in mind). Of course, I'm open to changing the names of those methods to something more obvious, if you have any ideas.
2. It may be more convenient to keep the properties in the RESTMeta class. The way I describe seems more "elegant" to me from a design perspective (it's more loosely coupled), but I'll go along with whatever API we want to present to the user. It would be nice if RESTHandlerClass had no knowledge of "owner" or "admin" at all, and all of that lived in Permissions classes. I'll think about this a bit more.
3. I hadn't considered setting the owner property on POST when I wrote that comment. The way I describe Permission.post_validate in my comment doesn't allow for modifying the created object. I'll have to think about a good way to make this work without being overly complicated. See my desire for loose coupling in 2 above.

[dankrause]

Another concern: This permissions system concept doesn't address how we'd filter requests for all instances of a given model that a user is allowed to access. If a user does a GET /api/foo and foo's GET permission is [PermissionOwner('owner')], we need a way to build the correct query. Same for DELETEs.

This will definitely require some more thought.

[dankrause]

Okay, here's my proposed solution to this:

http://paste.debian.net/82594/

This is a collection of permission classes. Note that Permission.post_validate must return the instances that have been passed to it, potentially modified. The PermissionOwner class implements updating the owner property on POST using this feature

Also note that there is a Permissions.query_all_instances method. This is used to get all instances of a model that the given user has permission to access.

The workflow would look like this:

1. Loop through the permissions for the method that's being performed, calling pre_validate on each one. The first one to return True allows us to continue. We remember which class this is.
2. If the operation we're performing involves fetching all instances, we take the first class that returned True in the previous step, and we call it's query_all_instances method. This returns a ndb.Query object, which we can further filter if needed. We then use it to fetch the instances.
3. We call the post_validate method of the same Permission subclass we used last time (the first one to return True). We pass it all instances that we expect to operate on, and we expect to get that same list as output (possibly modified). If it returns False, we raise a 403. Otherwise, we do normal callbacks, commit the changes, and return the results.

An important take-away from this is that the order the permissions are listed is important. A user would typically want PermissionAdmin to be listed before PermissionOwner. If this were reversed, an admin would query for all instances, and only find the ones that they own.

I've also changed where the admin and owner property names come from. The constructor args for these classes are used to look up the property name in RESTMeta. If all of that were implemented as described, backward compatibility would be as easy as this:

PERMISSION_ANYONE = [PermissionEveryone()]
PERMISSION_LOGGED_IN_USER = [PermissionUser()]
PERMISSION_OWNER_USER = [PermissionAdmin('admin_property'), PermissionOwner('user_owner_property')]
PERMISSION_ADMIN = [PermissionAdmin('admin_property')]

None of that code is tested. I'm just getting ideas down. I'll probably start on that implementation sometime within the next 24 hours.

[dankrause]

I have a branch with this implemented, but it depends on #15 being merged first.