Refactoring Policies

[askvortsov1]

Functionality Discussion

Is your feature request related to a problem? Please describe.
Policies are currently designed in a way that is confusing, and does not work well with our proposed extenders system, as ScopeModelVisibility depends on a non-domain event.

I've started on some of these in #2056.

Describe the solution you'd like
There's a lot of components to this, but I would propose the following:

1. Split off visibility scoping into a second abstract class from AbstractPolicy. The way I see it, AbstractPolicy applies to whether or not a user has a permission on a given instance/class of a model. ScopeModelVisibility restricts a query to records that a user can see, optionally based off of some action. It seems like these 2 were grouped together for sake of convenience, but similar methods (->view() vs ->find()), support for custom methods (based on the permission in question), and significant differences in functionality make this a suboptimal design IMO.

2. in AbstractPolicy, rename getPermission to canPerform, or similar. Permissions are something concrete that are attached to groups and stored in the database. Policy checks allow logic to determine whether a user can perform a somewhat more abstract action, such as view, edit, or something custom like overrideAdminSettings (made that one up but it's possible to implement). canPerform would be more descriptive, or something like canPerformActionOnInstance (although that one is clunky).

3. Rename pretty much everything on the new ScopeModelVisibility-related abstract class. I don't like find, because it's not a searching sysgtem, and it's not looking for individual instances. I'd prefer that devs, when extending the abstract model, define:

• $this->scopeQuery(): now called find, this scopes a query for viewing
• $this->scopeQueryForAction(): now called findACTION_NAME, where a different method is defined for each action. I think these should be consolidated into one method, where the action is provided with a string, then devs could conditionally apply filters depending on the action in question. Alternatively, $this->scopeQueryForACTION_NAME could also work.

4. Move AbstractPolicy away from events in implementation. One serious issue with the current implementation can be found here: Replacement options for dispatcher->until()  #2091. A potential fix could be found here: WIP Policy Refactor, Extender #2056

5. Move AbstractQueryVisibilityScoper away from events in implementation. This removed yet another non-domain event, which is part of our path to stable. I propose the following:

• We create a QueryVisibilityScoperManager class. This is passed to the AbstractModel class as a static instance variable, similar to how we pass the event dispatcher
• We create an extender which adds QueryVisiblityScopers defined in extensions to this manager
• We refactor the ScopeVisibilityTrait to use this new manager class instead of dispatching a ScopeModelVisibility event
• ???
• Profit!

[askvortsov1]

Franz and I had a very productive conversation about this today, here are my takeaways:

What I'm Proposing

• ScopeModelVisibility stuff is split from policy stuff. They both deal with permissions/access, but are very different and shouldn't be confused.
• In both cases, events will NOT be used.

Policies

• The laravel gate is currently not being used at all, and can be removed completely. $user->can() can directly reference the permission logic in UserServiceProvider.
• Policy stuff uses the system here: Replacement options for dispatcher->until()  #2091 to avoid extension order issues.
• Policies declare, and are applied based on, a namespace, typically corresponding to the relevant model. This must now explicitly be used when checking permissions. For instance, instead of $actor->can('edit', $user), we'd have `$actor->can('user.edit', $user). This is more explicit and cleaner.
• Policies are applied based on the namespace of the ability in question. This massively opens up flexibility (policies can now be used on abilities not tied to models), and is a more effective implementation
• The internal logic of policies is slightly reworked to be cleaner (see example below). In essense, permisions will have a namespace and content (e.g. user.editCredentials). Policies will be applied via the namespace, at which point, it will be removed, and the a method with the name equal to the content (editCredentials) will be called if found. If not, or if the first method doesn't return anything, a generic can method will be called.
  • This is similar to what we have not, but the namespace processing is cleaner and more standard.
• This logic:

if ($actor->isAdmin() || (! $model && $actor->hasPermission($ability))) {
                return true;
            }

• is changed so that if the actor is in a group that has the ability, it doesn't matter if a model instance is/isn't provided.

Policy Implementation

abstract class AbstractPolicy {
    protected $namespace;

    public function checkAccess(User $actor, string $namespacedAbility, $model = null) {
          $ability = substr($namespacedAbility, strlen($this->namespace) + 1);
          if ($method_exists($this, $ability) {
               $result = call_user_func_array([$this, $ability], [$actor, $model]);

            if (! is_null($result)) {
                return $result;
             }
          }
          if (method_exists($this, 'can')) {
            return call_user_func_array([$this, 'can'], [$actor, $ability, $model]);
        }
    }
}

Scope Model Visibility

• This is only applied to queries/models, so namespaces are unnecessary
• They will be added on the model in question during boot.
• Relevant logic will remain in the scopeVisibility trait, but that trait will be applied to AbstractModel so that it's available to all models.
• Methods will also be slightly renamed to be more self-explanatory

Concerns

• The policies system doesn't allow for a second tier of namespacing (e.g. user.edit.credentials), and doesn't allow definition of ability methods for abilities with non-variable-name characters.

[luceos]

Will this refactoring also remove the semi hardcoded admin users? Eg "admin" currently isn't even a specified permission, whether a user is an admin is based on whether they are part of the group with ID 1. This also applies to the Moderator group, which (I'm not sure) should not even have any value across our codebase, see User:

    /**
     * Check whether or not the user is an administrator.
     *
     * @return bool
     */
    public function isAdmin()
    {
        return $this->groups->contains(Group::ADMINISTRATOR_ID);
    }

Group

    /**
     * The ID of the administrator group.
     */
    const ADMINISTRATOR_ID = 1;

    /**
     * The ID of the guest group.
     */
    const GUEST_ID = 2;

    /**
     * The ID of the member group.
     */
    const MEMBER_ID = 3;

    /**
     * The ID of the mod group.
     */
    const MODERATOR_ID = 4;

[askvortsov1]

So right now, the permission system has, essentially 4 layers, checked in the following order.

1. Run through all relevant policies. If non-null returned, return that. Else, continue.
2. If the user is in a group that has the given permission and the permsission is not being evaluated on an instance of a model, return true. Else, continue.
3. If the user is in the admin group, return true. Else, continue.
4. Return false.

My proposed changes to the actual process are:

1. Have an order of precedence for step (1), that being forceDeny, forceAllow, deny, allow. This solves the issue of dependence on extension boot order,
2. Don't disqualify step 2 if the permission is being evaluated on an instance of a model. I'm not really sure why that was there in the first place.

So no, this wouldn't cause any issues in the area you addressed.

[askvortsov1]

I've also been thinking about how we could do proper namespacing, and I think I have an idea that could massively increase flexibility. @tankerkiller125 and I discussed policies and how they could be improved a few days ago, and I think this goes a bit closer to fulfilling some of his ideas. What if we stored the policies, after they are registered, in a prefix tree? This means that:

• We could have a policy that applies to all permissions (with a namespace of '')
• We could have a policy that applies to a model (with a namespace of 'user')
• We could have a policy that applies to a subset of actions on a model (with a namespace of user.edit)
• And so on

This would even further increase flexibility and allow for elegant organization (and lookup!) of really really complex permission systems.

If that's fine with us, the only other major issue to consider is whether we'd want to go all out, and require a callable (class/function) for every individual permission (essentially splitting up the policy classes into many separate classes/functions). This could have merit, but also could be messy. Personally, I think that grouping them together but namespacing properly is the better option here, but I'm not set in that opinion.

[askvortsov1]

Another interesting question is how this should play into any possible future expansions we want to make to extension permissioning, and/or a group rewrite. Tags, for instance, introduces a new 'permission key' per-tag iirc. Is that something we'd like to fit into the standard namespacing system, or perhaps add another delimiter to the permission keys?

[askvortsov1]

Issue that was raised in developer call: Do we want to have policy/permissions inheritance? Perhaps higher namespaced policies could only be applied if no lower-namespaced policies exist? Should this be mirrored in permissions (if group has user.edit, group would have user.edit.username, user.edit.groups, etc)?

[askvortsov1]

On second thought, instead of automatic inheritance, I think we might want to allow wildcard suffixes (and probably suffixes only: no wildcards on empty strings, and no wildcards with stuff after them) instead.

[askvortsov1]

Fixed in #2461