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

[Proposal] Simplify using local filters within a controller #2432

Closed
adamwathan opened this Issue Oct 8, 2013 · 4 comments

Comments

Projects
None yet
4 participants
@adamwathan
Member

adamwathan commented Oct 8, 2013

Very often I find myself having to write boilerplate permissions checks in a lot of my controller actions that are based on a filter parameter.

Imagine you have an app where users can have projects, but they should only be able to do stuff with their own project pages. You end up writing a lot of stuff like this:

public function show($id)
{
    if ( ! Auth::user()->ownsProject($id) {
        return Response::make('You are not authorized', 401);
    }

    // show the project...
}

Currently you can do something like this, but it can get messy...

public function __construct()
{
    $this->beforeFilter(function($route) {
        $id = $route->getParameter('id');

        if (! Auth::user()->ownsProject($id)) {
            return Response::make('You are not authorized', 401);
        }
    });
}

This is a little better but still sort of sucks:

public function __construct()
{
    $this->beforeFilter(function($route) {
        return $this->hasPermission($route);
    });
}

protected function hasPermission($route)
{   
    $id = $route->getParameter('id');

    if (! Auth::user()->ownsPost($id)) {
        return Response::make('You are not authorized', 401);
    }
}

I'm proposing a syntax like this:

public function __construct()
{
    $this->beforeFilter('@hasPermission');
}

protected function hasPermission($route)
{   
    $id = $route->getParameter('id');

    if (! Auth::user()->ownsPost($id)) {
        return Response::make('You are not authorized', 401);
    }
}

...using the '@' symbol to denote that the filter name refers to a local method within the class, similar to how it's used in other places within the framework. Super preliminary implementation looks something like this (still some bugs I'm sure):

// Illuminate\Routing\Controllers\Controller

    protected function prepareFilter($filter, $options)
    {
        if ($this->isInstanceFilter($filter)) {
            $filter = $this->makeInstanceFilter($filter);
        }

        // When the given filter is a Closure, we will store it off in an array of the
        // callback filters, keyed off the object hash of these Closures and we can
        // easily retrieve it if a filter is determined to apply to this request.
        if ($filter instanceof Closure)
        {
            $options['run'] = $hash = spl_object_hash($filter);

            $this->callbackFilters[$hash] = $filter;
        }
        else
        {
            $options['run'] = $filter;
        }

        return $options;
    }

    protected function isInstanceFilter($filter)
    {
        return is_string($filter) and starts_with($filter, '@');
    }

    protected function makeInstanceFilter($filter)
    {
        $filter = substr($filter, 1);
        $controller = $this;
        return function($route, $request) use ($controller, $filter) {
            return call_user_func_array(array($controller, $filter), array($route, $request));
        };
    }
@adamwathan

This comment has been minimized.

Member

adamwathan commented Oct 8, 2013

This is a little better than the first implementation. Could easily be modified to allow people to pass [$object, $method] callables as parameters to the filter functions as well.

    /**
     * Prepare a filter and return the options.
     *
     * @param  string  $filter
     * @param  array   $options
     * @return array
     */
    protected function prepareFilter($filter, $options)
    {
        // When the given filter is a Closure, we will store it off in an array of the
        // callback filters, keyed off the object hash of these Closures and we can
        // easily retrieve it if a filter is determined to apply to this request.
        if ($filter instanceof Closure)
        {
            $options['run'] = $hash = spl_object_hash($filter);

            $this->callbackFilters[$hash] = $filter;
        }
        elseif ($this->isInstanceFilter($filter))
        {
            $options['run'] = $filter;
            $this->callbackFilters[$filter] = $this->makeInstanceFilter($filter);
        }
        else
        {
            $options['run'] = $filter;
        }

        return $options;
    }

    /**
     * Check if a filter is a local method
     * @param  mixed  $filter
     * @return boolean
     */
    protected function isInstanceFilter($filter)
    {
        return is_string($filter) and starts_with($filter, '@');
    }

    /**
     * Create a callable from a local filter name
     * @param  string $filter
     * @return Closure
     */
    protected function makeInstanceFilter($filter)
    {
        $filter = substr($filter, 1);
        $controller = $this;

        return array($controller, $filter);
    }
@anlutro

This comment has been minimized.

Contributor

anlutro commented Oct 8, 2013

Have you looked into class filters or model binding for this sort of logic?

@adamwathan

This comment has been minimized.

Member

adamwathan commented Oct 8, 2013

I haven't thought about doing it that way too much. Class filters could work but feels like a bit overkill for some simple things where it would be nice to just throw a quick filter in the controller. It also looks like it might get clunky trying to come up with a bunch of non-conflicting filter names for all of these filter classes. Correct me if I'm wrong but you would have to register the filter separately, as well as add that filter with the beforeFilter() method in the controller? I feel like being able to directly use a controller's instance method as a filter without registering it in advance with a unique name could still be a useful feature for quick little things that don't really need an entire class.

I can't really figure out a way where route model binding could help with the situation I outlined, any chance you might be able to give an example? Always interested in learning other ways to do things!

@joshmanders

This comment has been minimized.

Contributor

joshmanders commented Oct 16, 2013

Hmm, interesting... I've always did this by putting a user_id restriction on the query for that user... So if their ID wasn't on the record they couldn't access it anyway...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment