Closed jace closed 7 years ago
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
.
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
).
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
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
orwrapper.attribute = value
).
zope.security
features a persistent proxy: all attributes returned by the proxy are also wrapped in a proxy.
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
The accessible_attrs
implementation is incorrect. It offers a read-only copy of the attribute, not a proxy that also allows write access.
Since it also eager-reads all attributes, it will forcibly load those attributes from the database, negating any intent to lazy-load them.
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.
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.
No. We're not reducing an object into two get and set functions. It remains an object.
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.
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.
Event listeners as documented in this example let us specifically watch for set
(write), append
and remove
events.
As __getattribute__
is a low level Python hook, it may have non-trivial performance impact. We should look for something that wraps InstrumentedAttribute
instead.
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).
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?
dict
for JSON is straightforward. This was the original reason for this proxy.is_attr_accessible
each time is unnecessary.roles
method, there is no good reason for the inherited
parameter to be between user
and token
.__roles__
dict could use sets instead of lists so that casting to a set at runtime is unnecessary.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. @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.
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()
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
Uh no, don't do a to_dict
. The API you want is dict(proxy)
.
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.
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.
Here's an explanation on abstract base classes. Note that you're not defining an ABC, merely using the existing collections.Mapping
ABC.
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.
You can do jsonify(proxy)
if you implement these methods.
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.
…although you'll need yet another proxy wrapper if a relationship is lazy='dynamic'
.
Changes:
dict
i.e dict(proxy)
__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
Fixed in #127.
As outlined in #100 subitem 6, Coaster's current
PermissionMixin
mechanism is inadequate for query-based API calls (like GraphQL), asPermissionMixin
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:Access is defined in terms of roles rather than permissions.
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?)
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 withwrite
andread
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.)This variable syntax for
__roles__
makes defining it easy but reading it harder, so either we need a metaclass that rewrites it (similar toLabeledEnum
's), or we restrict syntax to just the dictionary method{'column': {'read': ['all'], 'write': ['owner']}
.Analogous to
PermissionMixin
'spermissions
method, which acceptsuser
andinherited
parameters, we now have aroles
method that takes the same parameters and returns all roles available to the user.The
roles
method also accepts atoken
object instead of auser
object. Tokens (as defined in #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.The role names
all
anduser
are reserved. Theall
role is always present, and theuser
role is granted to anyone logged in (ie, theuser
parameter to theroles
method is a valid user or token).Beware of having an
owner
role. As we've learnt in our user concepts exercise,creator
andowner
are distinct concepts. It helps to define these clearly at the start, perhaps as additional comments on this ticket.We still need a mechanism to control access. A method named
access_for
(tentative name) takes auser
,token
orroles
parameter (any one). If it getsuser
ortoken
, it calls theroles
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.The GraphQL API always returns the result of the
access_for
method, never the raw database object.