Policy Extender and Tests

[askvortsov1]

Foundation for fixing #1832
Part of #1891

Please note that most of this diff is moving policy implementation content from the root folder of each service directory to an Access subfolder.

Changes proposed in this pull request:

• Ask that policies return ->allow, ->deny, ->forceAllow, ->forceDeny instead of true/false to increase flexibility
• Improve evaluation of whether a user has the ability to do something on an instance to avoid order-dependence.
• Rename getPermission (->checkAbility)
• Collect policies via a singleton in user extender
• Move policy stuff into an Access sub-namespace
• Add policy extender and integration tests
• Deprecate old policy stuff

Reviewers should focus on:

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).

Required changes:

• Related documentation PR: (Remove if irrelevant)
• Related core extension PRs: (Remove if irrelevant)

[SychO9]

Just some minor comments

I think we should also mark the old AbstractPolicy class deprecated ? (nvm, we'd have to do that after deprecating ScopeModelVisibility event as well)

[clarkwinkelmann]

Looking good, I just can't figure out where the policy classes are resolved through the container.

Does it happen in User::setGate($this->app->makeWith(Access\Gate::class, ['policies' => $this->app->make('flarum.policies.compiled')])); ?

It would be ideal to only instantiate the class when interacting with that model.

[askvortsov1]

Looking good, I just can't figure out where the policy classes are resolved through the container.

Does it happen in User::setGate($this->app->makeWith(Access\Gate::class, ['policies' => $this->app->make('flarum.policies.compiled')])); ?

It would be ideal to only instantiate the class when interacting with that model.

It happens in $this->app->make('flarum.policies.compiled')

[SychO9]

Looks good to me!

[clarkwinkelmann]

Still haven't been able to test locally, but the code looks good. A few things.

[askvortsov1]

One other thought: do we want to support returning true instead of allow and false instead of deny? It would be a bit complex since we'd need to turn the former into the latter, but might make some cases easier. Alternatively, the allow and deny approach is more consistent with the other 2 options. We can't use true and false as the keys since PHP would convert those into integers, and then strict comparison fails

[clarkwinkelmann]

Thanks. Your changes solve the problem of delaying resolve, but not of selectively resolving.

Most of the time, one request will only interact with the polices for one model, so it would be best to not resolve all policies for all existing models in the application.

What about calling a new method around line 82 which takes the list of class names, and takes/stores instances from an array which is keyed by the class name and serves as a cache?

Could we maybe even use the container itself to store instances? We could check if Container::bound, then Container::make and Container::instance store the policy.

Or alternatively, in the extender, register every policy as a singleton, then just use make in the Gate.

Not sure which of the options is more performant if you end up with a large number of policies.

[clarkwinkelmann]

Regarding the true and false returns, if it makes things more complicated for us, I don't think we need to support it.

I think allowing them might make things ambiguous, because what would they map to?

However it would probably be great to have an error message if you return anything that's not null or one of the allowed values from a policy method, otherwise things would just fail silently and it could be a debugging nightmare.

[askvortsov1]

Hmm, that makes sense. Making the policies singletons feels inefficient, but I see what you mean about resolving model by model. Will try that.

[tankerkiller125]

Testing locally via GUI attempting to break things everything went well. We'll for sure want QA to test the permission stuff more during that process. But overall I'm happy with the results.