Column-level permissions (RoleMixin)

[jace]

As outlined in #100 subitem 6, Coaster's current PermissionMixin mechanism is inadequate for query-based API calls (like GraphQL), as PermissionMixin only controls access to an API endpoint. We need a finegrained mechanism that determines read and write access to each column in a model.

We need a new RoleAccessMixin that provides mechanisms for defining and controlling access per column. It works like this:

1. Access is defined in terms of roles rather than permissions.

2. For each column, a list of read- and write-access roles are defined. (To be decided: does write access automatically imply read access? It does not in the case of password fields, but are there any other common examples?)

3. Access may be defined by adding a __roles__ dictionary to the model that uses the column name (or name of any attribute on the model) as the key, with the value as either a dictionary with write and read keys, or a two-tuple in (write_roles_list, read_roles_list) order, or a single-member tuple (write_roles_list) where read access is implicitly (and only) granted to anyone with write access. The roles are always a list. Subclasses must define roles as __roles__ = ParentClass.__roles__ + {…}. (Subclassing will require a bit more consideration, for whenever we actually run into the problem.)

4. This variable syntax for __roles__ makes defining it easy but reading it harder, so either we need a metaclass that rewrites it (similar to LabeledEnum's), or we restrict syntax to just the dictionary method {'column': {'read': ['all'], 'write': ['owner']}.

5. Analogous to PermissionMixin's permissions method, which accepts user and inherited parameters, we now have a roles method that takes the same parameters and returns all roles available to the user.

6. The roles method also accepts a token object instead of a user object. Tokens (as defined in API access architecture #100 subitem 3) using libmacaroons can provide access to a subset of a user's resources. It is unclear at this time how tokens will be defined, and a separate ticket is pending.

7. The role names all and user are reserved. The all role is always present, and the user role is granted to anyone logged in (ie, the user parameter to the roles method is a valid user or token).

8. Beware of having an owner role. As we've learnt in our user concepts exercise, creator and owner are distinct concepts. It helps to define these clearly at the start, perhaps as additional comments on this ticket.

9. We still need a mechanism to control access. A method named access_for (tentative name) takes a user, token or roles parameter (any one). If it gets user or token, it calls the roles method to get a list of roles. It then returns a wrapper dictionary object (with attribute access for easier syntax in Python code) that contains only the attributes that were in the __roles__ dictionary and for which the caller has a matching role. For the sake of efficiency, it lazy-loads contents.

10. The GraphQL API always returns the result of the access_for method, never the raw database object.

[jace]

It's unclear what happens when an attribute is a list, set or lazy-loading relationship. What does write mean now? Permission to delete everything? To edit anything? In this case we'll need distinct role groups such as enumerate, add, delete and replace.

[jace]

It's also unclear how any of these list access controls (add, delete, replace) will be enforced, as the wrapper object can only control read and write access (in the form of value = wrapper.attribute or wrapper.attribute = value).

[jace]

Mike Bayer says SQLAlchemy has some hooks for a security proxy that were introduced for Zope. This may be worth investigating. https://groups.google.com/forum/#!topic/sqlalchemy/wpLaqaoLg7U

[jace]

It's also unclear how any of these list access controls (add, delete, replace) will be enforced, as the wrapper object can only control read and write access (in the form of value = wrapper.attribute or wrapper.attribute = value).

zope.security features a persistent proxy: all attributes returned by the proxy are also wrapped in a proxy.

[shreyas-satish]

Here's a sample implementation for RolesMixin and the roles method in a model beneath. Please note that I've used accessible_attrs instead of access_for. Thoughts?

class RolesMixin(object):
    """
    Provides the :meth:`roles` method and the :meth: `get_accessible_attrs` used by BaseMixin and derived classes
    """
    def roles(self, user=None, inherited=None, token=None):
        """
        Return roles available to the given user on this object
        """
        if inherited is not None:
            return set(inherited)
        else:
            return set()

    def accessible_attrs(self, user=None, token=None, roles=[]):
        attrs = {}
        if not user and not token and not roles:
            return attrs
        if user:
            user_roles = self.roles(user=user)
        elif token:
            user_roles = self.roles(token=token)
        else:
            user_roles = roles
        for attr, role_map in self.__roles__.iteritems():
            for user_role in user_roles:
                if user_role in role_map['read']:
                    attrs[attr] = getattr(self, attr)
        return attrs

class Model(BaseMixin, db.Model):
    __tablename__ = 'model'
    description = MarkdownColumn('description', default=u'', nullable=False)

    __roles__ = {
        'description': {
            'write': ['model_writer'],
            'read': ['model_reader']
        }
    }

    def roles(self, user=None, inherited=None, token=None):
        roles = super(Model, self).roles(user, inherited, token)
        if user or token:
            roles.add('user')
            roles.add('model_reader')
        return roles

[jace]

The accessible_attrs implementation is incorrect. It offers a read-only copy of the attribute, not a proxy that also allows write access.

[jace]

Since it also eager-reads all attributes, it will forcibly load those attributes from the database, negating any intent to lazy-load them.

[shreyas-satish]

The following version allows write access:

class RolesMixin(object):
    """
    Provides the :meth:`roles` and :meth:`accessible_attrs` method used by BaseMixin and derived classes
    """
    def roles(self, user=None, inherited=None, token=None):
        """
        Return roles available to the given user on this object
        """
        if inherited is not None:
            return set(inherited)
        else:
            return set()

    def accessible_attrs(self, user=None, token=None, roles=[], mutations={}):
        attrs = {}
        if not user and not token and not roles:
            return attrs

        user_roles = roles if roles else self.roles(user=user, token=token)

        for attr, role_map in self.__roles__.iteritems():
            for user_role in user_roles:
                user_writable = user_role in role_map['write']
                if mutations.get(attr) and user_writable:
                    setattr(self, attr, mutations[attr])
                if user_writable or user_role in role_map['read']:
                    attrs[attr] = getattr(self, attr)
        return attrs

Looking into adding support for lazy-loading.

Edit: Cleaned up code.

[shreyas-satish]

A method named access_for (tentative name) takes a user, token or roles parameter (any one). If it gets user or token, it calls the roles method to get a list of roles. It then returns a wrapper dictionary object (with attribute access for easier syntax in Python code) that contains only the attributes that were in the roles dictionary and for which the caller has a matching role. For the sake of efficiency, it lazy-loads contents.

Instead of loading the attribute values directly, would it suffice to return a list of the attributes that are accessible on an object? Those values can then be supplied to a load_only SQLAlchemy query.

[jace]

No. We're not reducing an object into two get and set functions. It remains an object.

[jace]

SQLAlchemy provides us hooks by which to enforce access control within the object, thereby potentially removing the need for any sort of proxy. Role context will instead need to be stored in a Flask thread local variable, either under flask.g or a new one specifically for access control.

1. Overriding the __getattribute__(self, key) method will let us intercept read access on all attributes. However, this intercepts everything including access to __dict__, so this method will need an exception list that will always be passed through.

2. Event listeners as documented in this example let us specifically watch for set (write), append and remove events.

[jace]

As __getattribute__ is a low level Python hook, it may have non-trivial performance impact. We should look for something that wraps InstrumentedAttribute instead.

[jace]

Jinja's sandboxed environment feature includes some documentation on how an object may advertise safe and unsafe access. We currently use this in Eventframe for all website templates, but we could potentially use it everywhere, although it is relevant only where Jinja is used (and only as an API reference on this ticket).

[shreyas-satish]

Below is an object proxy that wraps a given object, and perfoms access control on the given model's column attributes.

class AccessibleProxy(object):
    def __init__(self, obj, user=None, token=None, roles=[]):
        self.__dict__['obj'] = obj
        self.__dict__['user_roles'] = roles if roles else obj.roles(user=user, token=token)

    def is_attr_accessible(self, attr, access_level):
        attr_accessible = False
        column_attrs = self.obj.__mapper__.attrs.keys()

        if attr in column_attrs:
            if self.obj.__roles__.get(attr) and self.user_roles.intersection(set(self.obj.__roles__[attr][access_level])):
                attr_accessible = True
        else:
            attr_accessible = True
        return attr_accessible


    def __getattribute__(self, attr):
        if attr in ['__dict__', 'obj', 'user_roles', 'is_attr_accessible']:
            return object.__getattribute__(self, attr)

        if self.is_attr_accessible(attr, access_level='read'):
            return object.__getattribute__(self.obj, attr)


    def __setattr__(self, attr, val):
        if attr in ['__dict__', 'obj', 'user_roles', 'is_attr_accessible']:
            return setattr(self.obj, attr, val)

        if self.is_attr_accessible(attr, access_level='write'):
            return setattr(self.obj, attr, val)

class RolesMixin(object):
    """
    Provides the :meth:`roles` and :meth:`accessible_attrs` method used by BaseMixin and derived classes

     __roles__ = {
        'description': {
            'write': ['item_collection_owner'],
            'read': ['item_collection_owner']
        }
    }
    """
    def roles(self, user=None, inherited=None, token=None):
        """
        Return roles available to the given user on this object
        """
        if inherited is not None:
            return set(inherited)
        else:
            return set()

Usage:

``>>>`` proxy = AccessibleProxy(model_obj, user=user)

Does this approach work?

[jace]

1. Let the proxy just accept roles. It needn't be aware of users and tokens or indeed anything about SQLAlchemy behaviour.
2. The proxy should provide dictionary access as well so that casting into dict for JSON is straightforward. This was the original reason for this proxy.
3. At init time it should figure out what attributes are accessible for read/write for the given roles and save that. Using is_attr_accessible each time is unnecessary.
4. In the roles method, there is no good reason for the inherited parameter to be between user and token.
5. The __roles__ dict could use sets instead of lists so that casting to a set at runtime is unnecessary.
6. A mapper_configured event could produce a reversed __roles__ list so that the attributes accessible by each role don't have to be calculated at runtime.

[jace]

@shreyas-satish One other thing: you don't need __getattribute__ on the proxy. It is only required on the main object itself, if we pursue an implementation that is not based on proxies.

[shreyas-satish]

Cleaned up the proxy and the RolesMixin based on suggestions 1,3,4,5:

class AccessibleProxy(object):
    def __init__(self, obj, roles=[]):
        self.__dict__['obj'] = obj
        self.__dict__['user_roles'] = roles
        self.__dict__['access_map'] = {}
        self.__dict__['column_attrs'] = obj.__mapper__.attrs.keys()

        for column_attr in self.column_attrs:
            self.access_map[column_attr] = {'read': False, 'write': False}
            if self.is_attr_accessible(column_attr, access_level='read'):
                self.access_map[column_attr]['read'] = True
            if self.is_attr_accessible(column_attr, access_level='read'):
                self.access_map[column_attr]['write'] = True


    def is_attr_accessible(self, attr, access_level):
        attr_accessible = False

        if attr in self.column_attrs:
            if self.obj.__roles__.get(attr) and self.user_roles.intersection(self.obj.__roles__[attr][access_level]):
                attr_accessible = True
        else:
            attr_accessible = True
        return attr_accessible


    def __getattribute__(self, attr):
        if attr in ['__dict__', 'obj', 'user_roles', 'access_map', 'column_attrs', 'is_attr_accessible']:
            return object.__getattribute__(self, attr)

        if self.access_map[attr]['read']:
            return object.__getattribute__(self.obj, attr)


    def __setattr__(self, attr, val):
        if attr in ['__dict__', 'obj', 'user_roles', 'access_map', 'column_attrs', 'is_attr_accessible']:
            return setattr(self.obj, attr, val)

        if self.access_map[attr]['write']:
            return setattr(self.obj, attr, val)

class RolesMixin(object):
    """
    Provides the :meth:`roles` and :meth:`accessible_attrs` method used by BaseMixin and derived classes

     __roles__ = {
        'description': {
            'write': {'item_collection_owner'},
            'read': {'item_collection_owner'}
        }
    }
    """
    def roles(self, user=None, token=None, inherited=None):
        """
        Return roles available to the given user on this object
        """
        if inherited is not None:
            return set(inherited)
        else:
            return set()

[shreyas-satish]

AccessibleProxy now includes a to_dict lending it to be jsonifed:

class AccessibleProxy(object):
    def __init__(self, obj, roles=[]):
        self.__dict__['obj'] = obj
        self.__dict__['user_roles'] = roles
        self.__dict__['attr_access_map'] = {}
        self.__dict__['column_attrs'] = obj.__mapper__.attrs.keys()

        for column_attr in self.column_attrs:
            self.attr_access_map[column_attr] = {'read': False, 'write': False}
            if self.is_attr_accessible(column_attr, access_level='read'):
                self.attr_access_map[column_attr]['read'] = True
            if self.is_attr_accessible(column_attr, access_level='read'):
                self.attr_access_map[column_attr]['write'] = True


    def is_attr_accessible(self, attr, access_level):
        attr_accessible = False

        if attr in self.column_attrs:
            if self.obj.__roles__.get(attr) and self.user_roles.intersection(self.obj.__roles__[attr][access_level]):
                attr_accessible = True
        else:
            attr_accessible = True
        return attr_accessible


    def __getattribute__(self, attr):
        if attr in ['__dict__', 'obj', 'user_roles', 'attr_access_map', 'column_attrs', 'is_attr_accessible', '__getattribute__', 'to_dict']:
            return object.__getattribute__(self, attr)

        if self.attr_access_map[attr]['read']:
            return object.__getattribute__(self.obj, attr)


    def __setattr__(self, attr, val):¡
        if attr in ['__dict__', 'obj', 'user_roles', 'attr_access_map', 'column_attrs', 'is_attr_accessible']:
            return setattr(self.obj, attr, val)

        if self.attr_access_map[attr]['write']:
            return setattr(self.obj, attr, val)

    def to_dict(self):
        attr_dict = {}
        for attr, access_map in self.attr_access_map.iteritems():
            if access_map['read'] or access_map['write']:
                attr_dict[attr] = self.__getattribute__(attr)
        return attr_dict

[jace]

Uh no, don't do a to_dict. The API you want is dict(proxy).

[jace]

To mimic a dictionary without being a dict-subclass (which you can't be because of the lazy loading requirement), you need to implement all the methods required for the MutableMapping abstract base class.

[jace]

Actually, you only need collections.Mapping (as base class) plus a __setitem__ method, as deleting attributes is not a supported operation. To use the Mapping baseclass, you need to define all the methods in the "Abstract Methods" column, and optionally any in the "Mixin Methods" column.

[jace]

Here's an explanation on abstract base classes. Note that you're not defining an ABC, merely using the existing collections.Mapping ABC.

[shreyas-satish]

The API you want is dict(proxy)

Since SQLAlchemy objects don't lend themselves to be used this way, I thought the proxy could use a to_dict so it would be easy to jsonify the result.

[jace]

You can do jsonify(proxy) if you implement these methods.

[jace]

Here is something else you can do if you implement the dict API: if an attr is a RoleMixin instance, return the attr's proxy instead of the attr itself. This way you can return an entire lazy-loaded tree without leaking raw SQLAlchemy objects that are linked via relationships.

[jace]

…although you'll need yet another proxy wrapper if a relationship is lazy='dynamic'.

[shreyas-satish]

Changes:

1. The proxy now lends itself to dict i.e dict(proxy)
2. The column attributes are read from the object's __roles__ dictionary instead of SQLAlchemy's mapper.

class AccessibleProxy(object):
    def __init__(self, obj, roles=[]):
        self.__dict__['obj'] = obj
        self.__dict__['user_roles'] = roles
        self.__dict__['attr_access_map'] = {}
        self.__dict__['column_attrs'] = obj.__roles__.keys()

        for column_attr in self.column_attrs:
            self.attr_access_map[column_attr] = {'read': False, 'write': False}
            if self.is_attr_accessible(column_attr, access_level='read'):
                self.attr_access_map[column_attr]['read'] = True
            if self.is_attr_accessible(column_attr, access_level='read'):
                self.attr_access_map[column_attr]['write'] = True

    def __getitem__(self, key):
        return self.__getattribute__(key)

    def __len__(self):
        return len(self.keys())

    def __setitem__(self, key, value):
        self.__setattr__(key, value)

    def __iter__(self):
        for key in self.keys():
            yield key

    def iterkeys(self):
        return self.__iter__()

    def keys(self):
        return self.column_attrs

    def __getattribute__(self, attr):
        if attr in ['__dict__', 'obj', 'user_roles', 'attr_access_map', 'column_attrs', 'is_attr_accessible', '__getattribute__', 'keys']:
            return object.__getattribute__(self, attr)

        if self.attr_access_map.get(attr, {}).get('read'):
            return object.__getattribute__(self.obj, attr)

    def __setattr__(self, attr, val):
        if attr in ['__dict__', 'obj', 'user_roles', 'attr_access_map', 'column_attrs', 'is_attr_accessible']:
            return setattr(self.obj, attr, val)

        if self.attr_access_map.get(attr, {}).get('write'):
            return setattr(self.obj, attr, val)

    def is_attr_accessible(self, attr, access_level):
        attr_accessible = False

        if attr in self.column_attrs:
            if self.obj.__roles__.get(attr) and self.user_roles.intersection(self.obj.__roles__[attr][access_level]):
                attr_accessible = True
        return attr_accessible

[jace]

Fixed in #127.