Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move permissions-type conditions to a Capabilities API (Trac #4888) #4888

Open
elgg-gitbot opened this issue Feb 16, 2013 · 22 comments
Open

Comments

@elgg-gitbot
Copy link

Original ticket http://trac.elgg.org/ticket/4888 on 42797668-09-16 by trac user mrclay, assigned to unknown.

Elgg version: 1.8

Elgg is currently full of code like:

if (container->canEdit()) { do something...

I propose we refactor this into an API that looks something like:

if (elgg_has_capability('do_something', container)) { do something...

In the short term we'll have just moved the condition code into a permissions hook handler, but 1) this will open the door for more granular permissions control, and 2) anyone who wants to create a roles implementation can simply tie roles to the named capabilities.

@elgg-gitbot
Copy link
Author

trac user mrclay wrote on 42800481-09-08

Examples: Drupal user_access; Moodle has_capability. WordPress current_user_can. I rather like sound of "user_can".

The hard part is determining what/how much context info to pass on to whatever underlying systems may make the decision. I'd suggest the context where this is occurring (group|user|site), and optionally the object being worked (usually an entity).

Most elgg_is_admin_logged_in() calls would become like elgg_user_can('do something'). Most $entity->canEdit() would become like elgg_user_can('modify title', array('object' => $entity)), or a new entity method could be $entity->can('modify title').

The core would have a built-in capabilities hook handler that had a look-up table for the core capabilities. This handler would, at a low level, call functions like elgg_is_admin_logged_in or trigger low-level check_permissions hooks to make the decision.

@elgg-gitbot
Copy link
Author

trac user mrclay wrote on 42813883-01-01

Evan suggested $user->can($verb, $object, $target) like [http://activitystrea.ms/specs/json/1.0/#activity Activity Streams]. I definitely like the sentence-y look (and using more from the AS spec), but there's currently no user entity for anonymous. If we made one we'd be requiring boilerplate with each use:

$user = elgg_get_logged_in_user_entity() || elgg_get_anon_user_entity();
$user->can(...);

Also I don't like at this point in the code that $user may or may not be real user.

@elgg-gitbot
Copy link
Author

trac user mrclay wrote on 42849864-01-15

Added more info to [https://docs.google.com/a/elgg.org/document/d/1aIQqRgu3bICTqSXn9GURwNjZdigVGGwXy18_Q_LJKvo/edit this doc].

@ewinslow
Copy link
Contributor

Thought about this more. We could have $session->userCan('verb', $object, $target); for the current user situation. Also probably makes sense to have $session->assertUserCan('verb', $object, $target) for actions and pages as a gatekeeper style function.

@ewinslow
Copy link
Contributor

@mrclay, I've written a gist with my proposal, including proposed documentation. We may have to add end-to-end permissions tests before we can reasonably hope to refactor the permissions system, though...

https://gist.github.com/ewinslow/446080109b5a69ad69c2

@mrclay
Copy link
Member

mrclay commented May 22, 2013

Here's the core of a plugin implementation I'm putting in production soon.

https://gist.github.com/mrclay/d277b311402f1f37f2b5

ufcoe_grant_capabilities() is a shortcut to setting site-wide global capabilities, which would be mostly useful for plugin-specific capabilities. I used closures to bind the handlers to a particular location in an array, so you can set many capabilities at priority 400 but they will all be resolved in a single hook handler for performance. Of course we don't necessarily need to provide this feature.

@mrclay
Copy link
Member

mrclay commented May 24, 2013

A hurdle I see ahead is race conditions between plugins that check permissions and those that "set" them. Is there a logical boot up event before init/system that plugins should use for setting perms?

@ewinslow
Copy link
Contributor

Looks good. I think this has a bug as it relates to creating new content,
though.

can('post', new Blog ())
can('post', new Plugin())

If the first returns true, so will the second because of the caching layer,
even if it shouldn't. Point being that I would cache by type-subtype when
guid is not available.
On May 23, 2013 6:43 PM, "Steve Clay" notifications@github.com wrote:

A hurdle I see ahead is race conditions between plugins that check
permissions and those that "set" them. Is there a logical boot up event
before init/system that plugins should use for setting perms?


Reply to this email directly or view it on GitHubhttps://github.com//issues/4888#issuecomment-18382298
.

@ewinslow
Copy link
Contributor

ewinslow commented Dec 2, 2013

A different direction that I like more and more the longer I think about it: consolidate further around "actions".

$actions->can('action/name', $params, $user = null); // defaults to logged in user

After all, what you need to be able to do at the end of the day is POST to an action endpoint, right?

@ewinslow
Copy link
Contributor

ewinslow commented Dec 2, 2013

What I like about this approach is that it doesn't require forcing all permissions into a subject/object/target model; the activitystreams-inspired model is nice and clean but might not end up working very well in real life for unforseen reasons/actions/capabilities.

It also feels very much like consolidation around a concept and not some new thing we're adding to the platform that further adds to what devs have to learn. There is a one-to-one correspondence b/w permission and action.

@mrclay
Copy link
Member

mrclay commented Dec 2, 2013

In practice I have permissions for viewing pages as well so not all will be POST operations. Agree with freeing the model, and I do like the tie in with actions. Registering an action could be an implicit grant in the capabilities system and have all actions check for hook-provided capabilities.

Here's an init function from one of my plugins:

function init_user_permissions(\ElggUser $user = null) {
    if (!$user) {
        return;
    }

    if ($user->isAdmin()) {
        ufcoe_grant_capabilities(array(
            'delete group',
        ));
    }
    if ($user->isAdmin() || $user->is_ufcoe_mentor) {
        ufcoe_grant_capabilities(array(
            'download user stats',
            'create group',
            'edit profile tags',
            'view others activity',
        ));
    }

    // users can edit own tags
    elgg_register_plugin_hook_handler('ufcoe_can', 'edit profile tags', function ($h, $t, $v, $p) {
        /* @var \ElggUser[] $p */
        if ($p['object'] && $p['subject'] && ($p['object']->guid == $p['subject']->guid)) {
            return true;
        }
    });
}

This model is working well for me, though I would prefer to pass in a useful object instead of an array. E.g. this would be nicer:

elgg_register_plugin_hook_handler('ufcoe_can', 'edit profile tags', function ($h, $t, $v, CanParams $p) {
    if ($p->subjectIsObject()) {
        return true;
    }
});

@ewinslow
Copy link
Contributor

ewinslow commented Dec 2, 2013

Hadn't thought about the view permissions. Seems like that complicates
things and we already have acls to handle that. Is it something we really
need to support in core?
On Dec 2, 2013 6:24 AM, "Steve Clay" notifications@github.com wrote:

In practice I have permissions for viewing pages as well so not all will
be POST operations. Agree with freeing the model, and I do like the tie in
with actions. Registering an action could be an implicit grant in the
capabilities system and have all actions check for hook-provided
capabilities.

Here's an init function from one of my plugins:

function init_user_permissions(\ElggUser $user = null) {
if (!$user) {
return;
}

if ($user->isAdmin()) {
    ufcoe_grant_capabilities(array(
        'delete group',
    ));
}
if ($user->isAdmin() || $user->is_ufcoe_mentor) {
    ufcoe_grant_capabilities(array(
        'download user stats',
        'create group',
        'edit profile tags',
        'view others activity',
    ));
}

// users can edit own tags
elgg_register_plugin_hook_handler('ufcoe_can', 'edit profile tags', function ($h, $t, $v, $p) {
    /* @var \ElggUser[] $p */
    if ($p['object'] && $p['subject'] && ($p['object']->guid == $p['subject']->guid)) {
        return true;
    }
});}

This model is working well for me, though I would prefer to pass in a
useful object instead of an array. E.g. this would be nicer:

elgg_register_plugin_hook_handler('ufcoe_can', 'edit profile tags', function ($h, $t, $v, CanParams $p) {
if ($p->subjectIsObject()) {
return true;
}});


Reply to this email directly or view it on GitHubhttps://github.com//issues/4888#issuecomment-29620889
.

@mrclay
Copy link
Member

mrclay commented Dec 2, 2013

Say we have a dashboard page made for special users, with various interfaces. If ACLs is all we have, we either have to make a corresponding object for that page/each bit of UI that might vary or at the very least associate the page with an ACL and test if the current user is in it.

@ewinslow
Copy link
Contributor

ewinslow commented Dec 2, 2013

OK agreed this seems like a good feature but do we need a unified model?

I guess the current way we get what you're asking for is just admin/logged
in/public which isn't granular enough, you're saying.

Something to consider is what this would look like for web services. How
does a JSON API honor these permissions?
On Dec 2, 2013 9:56 AM, "Steve Clay" notifications@github.com wrote:

Say we have a dashboard page made for special users, with various
interfaces. If ACLs is all we have, we either have to make a corresponding
object for that page/each bit of UI that might vary or at the very least
associate the page with an ACL and test if the current user is in it.


Reply to this email directly or view it on GitHubhttps://github.com//issues/4888#issuecomment-29640973
.

@mrclay
Copy link
Member

mrclay commented Dec 2, 2013

Sorry, I meant to suggest that having to bring in ACLs in that case was messy. Expecting developers to know about ACLs and capabilities (when they need them) is not onerous IMO.

I don't understand why the JSON API would add difficulty.

@ewinslow
Copy link
Contributor

Tl;dr: Can we return a collection of users instead of a boolean for a given user? That would make for a much more flexible system in a way that I think people can still understand.

Thinking about this more today, it's pretty common to want to ask "who can do X?" in addition to "can a given user do X? I.e. you want to be able to audit permissions without looping through all users in the system and running some code for each one (prohibitively expensive in general). This, I think, would provide our currently complex permissions system with some much needed transparency which I have not really seen anywhere else.

I think this is worth pursuing because it provides some really nifty advantages beyond just a unified API signature.

If you can answer "who can do X", you can just check if the current user is in that set of users to check if the current user can do X, so no functionality is lost.

A nice property that comes out of such a system is that you can also have users who do not have permission to perform certain actions submit a request for that action to be done. The system can then search for everyone who can do that and notify them about the request, or notify some subset, or let the requester choose who to notify, whatever.

Think "request to join" and "request to transfer ownership" and even "invite to join" or "suggest an edit". In general these are all proposals to take an action. They generally come with some justification or explanation and can be rejected, approved, or countered with an alternate proposal.

I also mentioned audits. For any action, it is possible to get a master list of people who can do that action. That is frankly incredible! I think this would really make the system feel more approachable and understandable to everyone, and certainly less mysterious and frustrating.

Proposed API:

Permissions {
  function whoCan(Action $action): Set<User>;
}

Session {
  public function can(Action $action) {
    return $permissions->whoCan($action)->contains($this->user);
  }
}

By default this returns the empty set (no one can do anything, secure by default).

We would need a sentinel value of some kind for "current user" in order to be able to handle logged out users. Or else another API method specifically for the logged out case.

Thoughts? One more very nice use-case for collections, IMO.

@hypeJunction
Copy link
Contributor

I like it a lot.
Is action an object that can be granulated, e.g. have a target entity and
context?
Who can approve join requests for group X?
Who can edit blog Y?
Who can create a blog in group Z?
Who can upload a resource with a simpletype image to a folder A in a group
B?

This definitely changes how we think of permissions in Elgg, and I think
this would make for a much better interface than pure ACL. Seems like we
need to start thinking of permissions as a set of conditions to be met,
where ACL is just one of them.
On Mar 14, 2015 9:14 PM, "Evan Winslow" notifications@github.com wrote:

Tl;dr: Can we return a collection of users instead of a boolean for a
given user? That would make for a much more flexible system in a way that I
think people can still understand.

Thinking about this more today, it's pretty common to want to ask "who can
do X?" in addition to "can a given user do X? I.e. you want to be able to
audit permissions without looping through all users in the system and
running some code for each one (prohibitively expensive in general). This,
I think, would provide our currently complex permissions system with some
much needed transparency which I have not really seen anywhere else.

I think this is worth pursuing because it provides some really nifty
advantages beyond just a unified API signature.

If you can answer "who can do X", you can just check if the current user
is in that set of users to check if the current user can do X, so no
functionality is lost.

A nice property that comes out of such a system is that you can also have
users who do not have permission to perform certain actions submit a
request for that action to be done. The system can then search for everyone
who can do that and notify them about the request, or notify some subset,
or let the requester choose who to notify, whatever.

Think "request to join" and "request to transfer ownership" and even
"invite to join" or "suggest an edit". In general these are all proposals
to take an action. They generally come with some justification or
explanation and can be rejected, approved, or countered with an alternate
proposal.

I also mentioned audits. For any action, it is possible to get a master
list of people who can do that action. That is frankly incredible! I think
this would really make the system feel more approachable and understandable
to everyone, and certainly less mysterious and frustrating.

Proposed API:

Permissions { function whoCan(Action $action): Set;}Session { public function can(Action $action) { return $permissions->whoCan($action)->contains($this->user); }}

By default this returns the empty set (no one can do anything, secure by
default).

We would need a sentinel value of some kind for "current user" in order to
be able to handle logged out users. Or else another API method specifically
for the logged out case.

Thoughts? One more very nice use-case for collections, IMO.


Reply to this email directly or view it on GitHub
#4888 (comment).

@ewinslow
Copy link
Contributor

Yes, I was thinking the action could be what we discussed above: 1:1 relationship with current meaning of action in elgg. Accept arbitrary params. Or we could restrict the meaning. Haven't thought about that as much yet.

@ewinslow
Copy link
Contributor

Alternatively, we could move the whoCan onto the action...

Action {
whoCan(): Set
}

Or if we go to restful controllers:

whoCanPost, whoCanGet, whoCanDelete, whoCanPut, etc.

I'm not sure what the right level is to put this...

@mrclay
Copy link
Member

mrclay commented Mar 15, 2015

FWIW in Drupal users either have or don't have a permission, and they happen to be granted by roles. You can query ppl who have a permission or a role. Having the permission purely depend on the DB makes the system easy to reason about and design caching around. We can bind these permissions within "contexts" (eg a group) to get more flexibility, but key is to not guarantee arbitrary query-time logic (no hooks).

@mrclay
Copy link
Member

mrclay commented Mar 15, 2015

I guess I see how to persist the permissions data in a reasonable way as being the bigger question.

Also how often does the system really have to query all users who can do something? The system is constantly trying determine what the logged in user can do. Maybe I'm missing something; I'll reread this ticket again later.

@ewinslow
Copy link
Contributor

Also how often does the system really have to query all users who can do something?

I don't know how often this would need to happen because it isn't even remotely possible right now. But I suspect it would be a nice feature once it is possible. I listed two examples already:

  1. notify approvers upon a request for an action
  2. Auditing permissions. Seeing who can do certain things and making sure it is the people who should be allowed to do such things.

I'm sure we could come up with more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants