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

WIP Policy Refactor, Extender #2056

Closed
wants to merge 15 commits into from

Conversation

askvortsov1
Copy link
Sponsor Member

@askvortsov1 askvortsov1 commented Mar 8, 2020

Foundation for fixing #1832
Part of #1891

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), scopeModelVisibility (->scopeQueryListener) methods, as well as the methods scopeModelVisibility calls (->scopeQuery, ->scopeQueryPerAbility), for purposes of clarify.
  • Restructure old scopeModelVisibility calls down to only 2 methods
  • Refactor how policies are registered by moving them into the User service provider, making an Extender more viable
  • Removed the GroupServiceProvider as it no longer does anything
  • Deprecate everything that's no longer used
  • Add Policy extender
  • Added integration tests for policy extender (dependent on Add Authenticated Test Case utility #2052)

Reviewers should focus on:

  • I don't like that the gate->before callback now requires us to app->make ALL the policies every time we want to check a permission. The same applies to the part where we listen to ScopeModelVisibility. Should I add a singleton that contains resolved instances of all policies?
  • Are the new names what we want?
  • Do we want to get rid of ScopeModelVisibility?

TODO
If this is accepted and merged, bundled extensions will need to be updated.

Confirmed

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

@askvortsov1 askvortsov1 changed the title Policy Refactor, Extender WIP: Policy Refactor, Extender Mar 20, 2020
@askvortsov1 askvortsov1 changed the title WIP: Policy Refactor, Extender Policy Refactor, Extender Mar 20, 2020
@franzliedke franzliedke self-requested a review April 13, 2020 12:36
@askvortsov1 askvortsov1 changed the title Policy Refactor, Extender WIP Policy Refactor, Extender Apr 19, 2020
@stale
Copy link

stale bot commented Jul 18, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum.
In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

@stale stale bot added the stale Issues that have had over 90 days of inactivity label Jul 18, 2020
@askvortsov1
Copy link
Sponsor Member Author

Not stale.

@stale stale bot removed the stale Issues that have had over 90 days of inactivity label Jul 19, 2020
@askvortsov1 askvortsov1 added this to the 0.1.0-beta.15 milestone Oct 1, 2020
@askvortsov1 askvortsov1 removed this from the 0.1.0-beta.15 milestone Oct 13, 2020
@askvortsov1 askvortsov1 mentioned this pull request Oct 28, 2020
78 tasks
@askvortsov1 askvortsov1 self-assigned this Oct 28, 2020
@SychO9 SychO9 self-requested a review November 2, 2020 23:06
Comment on lines +23 to +26
public static $ALLOW = 'ALLOW';
public static $DENY = 'DENY';
public static $FORCE_ALLOW = 'FORCE_ALLOW';
public static $FORCE_DENY = 'FORCE_DENY';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these meant to be constants ? if so let's declare them as such:

public const ALLOW = 'ALLOW';
public const DENY = 'DENY';
public const FORCE_ALLOW = 'FORCE_ALLOW';
public const FORCE_DENY = 'FORCE_DENY';

Comment on lines +104 to 107
* @deprecated in favor of checkAbility
*/
public function getPermission(GetPermission $event)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is deprecated, why don't we just call checkAbility from here, instead of having duplicated blocks of code.

Comment on lines -56 to 129
* @param ScopeModelVisibility $event
* @deprecated
*/
public function scopeModelVisibility(ScopeModelVisibility $event)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, why not just call scopeQueryListener


$results = [];
foreach ($this->app->make('flarum.policies') as $policy) {
$results[] = $this->app->make($policy)->checkAbility($actor, $ability, $model);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this refactor, when policies were listeners, they were resolved only once.

How about we resolve them all in an array before making the gate and then we use that array of all resolved policies inside.

src/User/UserServiceProvider.php Show resolved Hide resolved
@askvortsov1 askvortsov1 marked this pull request as draft November 3, 2020 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants