machuga/authority

Discussions for moving toward 3.0

machuga opened this issue · 4 comments

I've pushed up the first iteration of working toward 3.0 here https://github.com/machuga/authority/tree/3.0-concepts/src/Authority. I want to start generating some discussion around the code changes, simplifications, feature removals, and potential feature additions. The minimal PHP version will be 5.4.x.

The API will also be somewhat broken when I finally get to the Authority class. I am going to do a bind() on the closures within each Rule to the scope of the current Authority object they were spawned from which will allow the allow() callback API to be simpler as it was in the original CodeIgniter and Laravel 3 versions.

Example:

$authority->allow('read', 'Post', function($post) {
    return $this->user()->id === $post->user_id;
});

I'd love to hear feedback on any and all things moving forward.

Once we start heading a stable direction here I will open up the same discussion on Authority-L4 for new feature additions and simplifications.

@JesseObrien, @janhartigan, @rtablada, @conarwelsh

I'm also highly considering removing the dependency on Illuminate\Event in favor of a simple no-op NullDispatcher or some very simplistic pub/sub to replace it.

Having just learned about Closure::bind(), I'm all for it. I will also consider moving my packages to this when I move them to 5.4.

Regarding removing the dependency on illuminate/event, I think it makes good sense if you can somehow make it trivial to pass events through to whichever event system a dev has. So something like:

Authority::eventDispatcher(function($event, $params)
{
    //using some arbitrary events library, ignore syntax
    Event::fire($event, $params);
});

...the benefit of this being that the dev wouldn't have to spend efforts on two distinct event systems.

Am I wrong or is this the only place that's dispatching an event right now:

https://github.com/machuga/authority/blob/master/src/Authority/Authority.php#L48-L50

If it's only in one place, it might make sense to simplify the construct a bit. On the other hand, it might also be a nice feature to have events fire whenever a rule passes/fails.

I like the direction the new version is going. Any idea on when it'll be released?

I've basically forked Authority for my own use, because I needed some features that aren't currently possible, and they totally break backwards compatibility.

  • Authority bound to $condition scope

This, as well as the clean up and abstraction of the Rule class you've already done are similar to things I've implemented.

  • Ability to pass arbitrary parameters to condition

For example, here's an simple check (all these examples use Laravel and assume $this is Authority):

Authority::can('edit', $meeting, function($meeting) {
    return $meeting->event->isOwner($this->getCurrentUser());
});

Unfortunately, that requires loading the event. But much of the time I'll already have this object loaded, so I just want to be able to pass it in. Even with a caching layer preventing this from hitting the database, it's possibly adding complex and duplicated logic just to get what is needed.

Authority::can('edit', $event, function($meeting, $event = null) {
    $event = $event ?: $meeting->event;
    return $event->isOwner($this->getCurrentUser());
});

That's a somewhat simplistic example. But in my app I have complex relationships that require quite a bit of logic to load and parse within the enclosed scope of the condition, whereas all the information is readily available in the controller or view scope where the check is occurring. This makes Authority much more work than just inline checking the permission like if ($event->isOwner(Auth::user())) {}.

  • Ability to define rules with an array of actions

This just makes for fewer duplicated rules and a shorter definition config. Action aliases get me part of the way there, but aliases are slightly different conceptually from defining multiple actions (and IMO more geared towards use in Authority::can et al). And even those must be explicitly defined, when I'd often just rather use them inline.

Authority::allow(['view', 'edit'], 'Event', function ($event) {
    return $event->isOwner();
});
  • Ability to define rules with an array of resources

This makes for somewhat more complex conditions, but is still very useful when multiple resources share the same complex condition logic.

This, and the task above both change the return value of allow/deny to not return the rule, since there are multiple (though it could return a (sub-)collection of them I suppose). But I never access rules directly like that, so it doesn't really matter all that much to me.

Authority::allow('edit', ['Event', 'Meeting'], function () {
    $args = func_get_args();
    if ($args[0] instanceof Meeting) {
        $event = $args[0]->event;
        // Or, with the multiple condition parameters described above:
        // $event = $args[1] instanceof Event ? $args[1] : $args[0]->event;
    } elseif ($args[0] instanceof Event) {
        $event = $args[0];
    } else {
        return false;
    }
    return $event->isOwner();
});
  • Ability to check multiple actions at once

Pretty straightforward, and just allow less verbose checks. This is easy to add by extending Authority:

Authority::canAny(['view', 'edit'], $resource);
Authority::canAll(['view', 'edit'], $resource);

I'm also planning on doing some other things, like adding traits to inject can/cannot into models, as well as adding can/cannot into the Auth class. But these are more Laravel-specific.

Anyway, I know that's a lot. I love Authority, because it doesn't depend on roles/permissions, which just about every other authorization package does (I need, and rather use, the more flexible run-time checks). The stuff above makes Authority much more flexible for me. My code has moved pretty far afield from Authority, so isn't currently compatible, but if you're interested in all these features, I'd definitely consider bringing them back together again...