1
\$\begingroup\$

I wanted to create a simple and easy to use permissions system. The ones I've found on the Internet would need to attach every permission to a route manually (e.g. Entrust).

My idea is to build a permission name out of the current action, so for example method 'store' in 'TestController' becomes 'test.store' permission.

Is it a good approach? I couldn't find anything similiar on the web.

class SecuredController extends Controller {

    /**
     * @var array
     * unguarded actions (method names)
     */
    protected $unguarded = [];

    /**
     * @var array
     * actions (method names) that need csrf token checked
     */
    protected $csrfguarded = [];

    private $permission;

    public function getCurrentPermission() {
        return $this->permission;
    }

    public function callAction($method, $parameters) {
        if(! $this->canAccessCurrentAction()) {
            return $this->accessDenied();
        }

        return parent::callAction($method, $parameters);
    }

    /**
     * @param string $action e.g. TestController@store
     *
     * @return bool
     * @throws \Illuminate\Session\TokenMismatchException
     */
    public function canAccessAction($action) {
        /**
         * split $action(e.g. TestController@store) into two parts
         * and build a permission name (e.g. test.store) and check for access
         */

        list($controller, $method) = explode('@', $action);

        if(in_array($method, $this->unguarded, true)) {
            return true;
        }

        if(in_array($method, $this->csrfguarded, true)
           && Session::token() !== Input::get('_token'))
        {
            throw new Illuminate\Session\TokenMismatchException;
        }

        if(! Auth::check()) {
            return false;
        }

        $this->permission = $this->buildPermissionName($controller, $method);

        if(Auth::getUser()->capable($this->permission)) {
            return true;
        }

        return false;
    }

    public function canAccessCurrentAction() {
        return $this->canAccessAction( \Route::getCurrentRoute()->getActionName());
    }

    public function buildPermissionName($controller, $method) {
        return strtolower(
            substr(
                $controller,
                strrpos($controller, '\\'),
                - 10 // 'controller' word
            ) . '.' . $method
        );
    }

    /**
     * Override it!
     */
    public function accessDenied() {
        if(Request::ajax()) {
            return Response::make('You cannot access this!', 403);
        }

        return Response::make('You cannot access this!', 403);
    }
}

The capable method of User:

public function capable($neededPermission) {
    foreach($this->getPermissions() as $permission) {
        if(substr($permission, -1) === '*') {
            $permission = substr($permission, 0, -1);

            if(startsWith($neededPermission, $permission)) {
                return true;
            }
        }
        elseif($permission === $neededPermission) {
            return true;
        }
    }

    return false;
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

I see a lot of if-else statements that could be improved upon.

Ones like these for example, are returning a boolean variable depending on an if-else statement with nothing else inside.

In those cases you can return boolean values directly.

  if(in_array($method, $this->unguarded, true)) {
      return true;
  }

into:

return (in_array($method, $this-unguarded, true));

The following code does the same thing for both 'cases', meaning that no matter what 'Request::ajax()' is, the same result will be given:

public function accessDenied() {
    if(Request::ajax()) {
        return Response::make('You cannot access this!', 403);
    }

    return Response::make('You cannot access this!', 403);
}

Which could simply be turned into:

public function accessDenied() {
    return Response::make('You cannot access this!', 403);
}

Your usage of the following code really bothers me, and I believe it could do with refactoring:

return strtolower(
        substr(
            $controller,
            strrpos($controller, '\\'),
            - 10 // 'controller' word
        ) . '.' . $method
    );
\$\endgroup\$
1
  • \$\begingroup\$ Thank you. I can't return boolean values directly because the code below the first return will be unreachable. However I will think about refactoring the buildPermissionName method. \$\endgroup\$
    – kbck
    Jul 4, 2015 at 14:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.