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

Split RouteAccessControl from AccessBehaviorTrait #7

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

handcode
Copy link
Member

@handcode handcode commented Nov 14, 2023

This PR create a new RouteAccessControl filter which (re)implements the logic from the prev. AccessBehaviorTrait rule.

The new filter can be used as "normal" behavior without the trait drawbacks.

For BC the AccessBehaviorTrait still exists and use the new RouteAccessControl filter now.

Besides the separation itself, a few small changes were made to the AccessCheck logic during refactoring:

  • the $owner of the behavior is used as controller to get the route (parts) if the $owner is an instance of yii\web\Controller. In all other cases, \Yii::$app->controller is used.
  • The separator that is used for the checked permissions is a configurable property now (defaults to '_' as before)
  • The user->can() checks for the various "levels" of the route-permission are made inside the access rule now. This means that the check can be used with any yii\web\User instance now, previously only with dmstr\web\User instances, as the actual route checks were executed there.
  • The used user instance for the checks is taken from the yii\filters\AccessControl::user property, so it is configurable now.

If this PR, or rather the idea behind it, is accepted, we should be able to remove the dmstr\web\User::checkAccessRoute() method, as it is no longer necessary for the RouteAccess check and as it is private it cannot be used outside of class.

…orTrait rule; use the new filter in the trait
@handcode handcode requested review from schmunk42 and eluhr November 14, 2023 16:49
@handcode handcode assigned handcode and unassigned schmunk42 and eluhr Nov 14, 2023
@handcode
Copy link
Member Author

And while we're at it, I would recommend moving the new RouteAccessControl filter into its own package, which will then be required by dmstr/yii2-web.

The advantage would be: since the filter itself has no dependencies, it would be an easier to use component in other yii2 projects.


public function beforeAction($action)
{
$this->rules[] = $this->getRouteAcessControlRule();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let‘s add a index here e.g. routeAccess. Even if we don‘t need it (yet), it is better to have it.

Copy link
Member Author

Choose a reason for hiding this comment

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

done in 9e8213d

…ndex in the rules array, fix routeCheckParams usage
@handcode
Copy link
Member Author

If this PR, or rather the idea behind it, is accepted, we should be able to remove the dmstr\web\User::checkAccessRoute() method, as it is no longer necessary for the RouteAccess check and as it is private it cannot be used outside of class.

I have to correct myself here:
Although \User::checkAccessRoute() is private, it can be triggered directly (without filter or trait) with User::can($perm, ['route' => true]).
And since we can't know if $anyone_anywhere does this, we can't just remove \User::checkAccessRoute().

Since the separator for the route checks is now configurable in the new RouteAccessControl filter, I have also patched \User::checkAccessRoute() so that the separator can also be configured there via the optional routePartSeparator parameter. see: 7fc3775

The default is the underscore in both places as before, but if the separator can be changed, it must be possible in both places.

@schmunk42
Copy link
Member

@handcode merge & release at will

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.

3 participants