restfulserver, object-level and field-level permissions
Ingo Schommer
While reviewing RestfulServer I've noticed that the implementation of
our
Permission::check() and DataObject::can*() is a bit sketchy.
With releasing the Restful API soon the topic of control becomes more
important to people, and we should set clear guidelines from the start.
I've added some documentation to DataObject.php
about permission codes and checking for group memberships.
We're deviating a bit from our practice of defining statics
for this model metadata by requiring the overloading
of the can*() methods - we're using a different concept
on the controller level with Controller::$allowed_actions.
I think it would be pretty straightforward
to implement something similiar for DataObject::$allowed_access,
and with a bit more effort progressively define and evaluate this
setting
throughout the core datamodel
Basically we're setting a standard here with requiring
canView() { return Permission::check('ADMIN');} -
its just not noticed very much at the moment because
we don't enforce it outside of RestfulServer. If we fully integrate
access control,
this implies that DataObjects would have to be expicitly
marked as "viewable by anonymous users".
Secure by default, but requires devs to learn the permission
syntax as an additional step. Currently they'd have to write
a custom method that checks a single permission 99% of the time,
how about simplifying this metadata definition?
We could extend DataObject as follows:
class DataObject extends ViewableData {
static $allowed_access = array(
'view' => 'ADMIN',
'create' => 'ADMIN',
'edit' => 'ADMIN',
'delete' => 'ADMIN',
);
// alternative syntax
'view' => '->methodName',
'view' => '*', // all
}
Then SiteTree could have the following override,
with the 'SITETREE_*' permissions implied
for ADMIN users.
class SiteTree extends DataObject {
static $allowed_access = array(
'view' => '*',
'create' => 'SITETREE_CREATE',
'edit' => 'SITETREE_EDIT',
'delete' => 'SITETREE_DELETE',
);
}
Field-level access control is another sensitive area -
at the moments its "all-allowed" or "all-denied" as described
above. "All-alowed" by default includes any dynamic getters if the
&add_fields=
URL-parameter is used for RestfulServer.
At the moment you have to overload dynamic getters
to actively prevent users from getting certain fields.
Common example is the exposure of generic Member data
through the Restful-Server - you might want to enable
Firstname/Lastname/Email for address book sync or something,
but at the moment you'd also grant access to password-hashes etc.
class Member extends DataObject {
static $api_access = true;
// default would be to allow all if $allowed_fields is not overwritten
// if $allowed_fields is used, all fields not mentioned would be denied
static $allowed_fields= array(
'FirstName' => '*',
'LastName' => '*',
'Password' => 'ADMIN'
);
}
Also, should we provide common Permission codes for all DataObject
subclasses
by default through PermissionProvider::providePermission()?
Would love to hear your thoughts - I've started a development plan
at http://open.silverstripe.com/wiki/development/permissions
-------
Ingo Schommer | Senior Developer
SilverStripe
http://silverstripe.com
Phone: +64 4 978 7330 ext 42
Skype: chillu23
Sam Minnee
> class DataObject extends ViewableData {
> static $allowed_access = array(
> 'view' => 'ADMIN',
> 'create' => 'ADMIN',
> 'edit' => 'ADMIN',
> 'delete' => 'ADMIN',
> );
> // alternative syntax
> 'view' => '->methodName',
> 'view' => '*', // all
> }
that, IMO, record-level security is going to be the exception rather
than the norm here. If we assume that under your system, you would
implement a method such as checkPermission() for actually checking
permissions.
That means, in the case of a record-level security system implemented
as a canView, permission checking is going to go through three pieces
of the system
* checkPermission
* allowed_access
* canView
This is a bit too convoluted for my liking.
I recommend this approach:
static $allowed_access = array(
"view" => true / false / permission code
);
DataObject::can($permission) {
if($methodName = $this->hasmethod("can$permission")) return $this->
$methodName();
else {
$can = $this->stat('allowed_access');
return $can[$permission] === true ||
(is_string($can[$permission]) &&
Permission::check($can[$permission]));
}
}
I guess, by the time that we get to that third implementation, the
only point of difference i'm recommending is the automatic
identification of canXXX() methods, rather than having to use the "->"
syntax in allowed_access. This has the following advantages:
* It's easier to create canView() - you just need to define the
method rather than define the method and stitch it in. Convention
over configuration
* allowed_actions is simpler - a string value can always be
interpreted as a permission code. A boolean "true" is more clear to
me than a string "*". You also don't need to allow for the "->"
syntax. At all.
Regarding providePermissions(), it should iterate over allowed_access
and include any permission codes specified in the array. The same
could be said about Controller::$allowed_actions.
I think that with field level permissions it would be acceptable to
force developers to create a method to provide field level security.
Something like this would be a pretty good way of letting users
specify the field permissions.
canViewFields($fields = array()) {
// $fields can optionally be used to give a hint to this method as
to what fields the caller is interested in
// returns array of fields that
}
canEditFields($fields = array()) {
// $fields can optionally be used to give a hint to this method as
to what fields the caller is interested in
// returns array of fields
}
Ingo Schommer
>
>>
> I'm not sure if this is the best way forward here. The problem is
> that, IMO, record-level security is going to be the exception rather
> than the norm here. If we assume that under your system, you would
> implement a method such as checkPermission() for actually checking
> permissions.
I agree with "convention over configuration" in terms
of reducing the effort for a programmer trying to secure
his application. If we encourage restful access, which
makes sense for the majority of public data really,
I don't think permission control should be seen as "the exception".
>
>
> That means, in the case of a record-level security system implemented
> as a canView, permission checking is going to go through three pieces
> of the system
>
> * checkPermission
> * allowed_access
> * canView
>
> This is a bit too convoluted for my liking.
I would argue its extendable and concise. With your approach, you're
basically
allowing for the same chain, just removing the option to specify
the methodname explicitly. One line of code less, but means
you'll see classes which just work on $allowed_access definition,
and others having can*() methods scattered about. I'd prefer
a common starting point to see, at a glance, which permission checks
are in place. As I think that definition of permission codes
will be more likely than custom can*() methods, this "starting point"
should be $allowed_access.
Also, we should keep capabilities of DataObject::$allowed_actions
and Controller::$allowed_access in sync, right?
So with your approach, we'd have to consequently remove the
already documented and released "->" syntax from $allowed_actions.
>
> I guess, by the time that we get to that third implementation, the
> only point of difference i'm recommending is the automatic
> identification of canXXX() methods, rather than having to use the "->"
> syntax in allowed_access. This has the following advantages:
>
> * It's easier to create canView() - you just need to define the
> method rather than define the method and stitch it in. Convention
> over configuration
> * allowed_actions is simpler - a string value can always be
> interpreted as a permission code. A boolean "true" is more clear to
> me than a string "*". You also don't need to allow for the "->"
> syntax. At all.
Ah c'mon, its not *that* hard to distinguish the syntax,
its already implemented for Controller anyway ;)
>
>
> Regarding providePermissions(), it should iterate over allowed_access
> and include any permission codes specified in the array. The same
> could be said about Controller::$allowed_actions.
Yeah, would be consistent to provide for both.
>
>
> I think that with field level permissions it would be acceptable to
> force developers to create a method to provide field level security.
> Something like this would be a pretty good way of letting users
> specify the field permissions.
Good point, it allows for both blacklisting and whitelisting
depending on implementation, and keeps everything in once place.
Sam Minnee
we've got to so far:
http://open.silverstripe.com/wiki/development/permissions#For2.3
Note that we think that record level security should be pushed into
2.3 to coincide with the RestfulServer release, field level security
raises too many questions and will be pushed to 2.4. The new data
mapper planned for 2.4 should be able to help with some of the
trickier points about access control, too.