laravel / ideas

Issues board used for Laravel internals discussions.
938 stars 28 forks source link

authorizeResource() and nested resources support #1612

Open royduin opened 5 years ago

royduin commented 5 years ago

When we've a post with comments we can use:

// App/Http/Controllers/PostController.php
public function __construct()
{
    $this->authorizeResource(Post::class);
}

In our PostController, with a PostPolicy and the reference in the AuthServiceProvider. But; when we can't use authorizeResource() in the CommentController when we've nested them:

// routes/web.php
Route::resource('post.comment', CommentController::class)

And we need access to the post and the comment in the CommentPolicy. For example:

// App/Policies/CommentPolicy.php
public function delete(User $user, Comment $comment, Post $post)
{
    // Some rules based on the post and the comment.
}

This is currently only possible by specifying the authorization in every controller method, for example:

// App/Http/Controllers/CommentController.php
public function destroy(Post $post, Comment $comment)
{
    $this->authorize('delete', [$comment, $post]);

    //
}

With this example you can say; sure but you can access the post from the comment right? Just use $comment->post in the policy and you're done. But what about the create method in the policy? For example; a user may only create a comment when the post is published:

// App/Policies/CommentPolicy.php
public function create(User $user, Post $post)
{
    return $post->published;
}

To accomplish this we've to use:

// App/Http/Controllers/CommentController.php
public function create(Post $post)
{
    $this->authorize('create', [Comment::class, $post]);

    //
}

It would be cleaner to have the possibility to use authorizeResource() with nested resources so we don't have to specify authorization in every restful controller method.

axeloz commented 5 years ago

+1

I've had the exact same problem today when writing a simple API. To stick to your example, I still had to use the $this->authorizeResource(Post::class) into the CommentController to prevent anyone playing with the post param of the URL /posts/{post}/comments/{comment}.

Also in order to fix the authorization part, I've had to add an explicit model binding into RouteServiceProvider:

Route::bind('comment', function($value, $router) {
     return Comment::where('post_id', $router->parameter('post'))->findOrFail($value);
});

I find this quite dirty and I'm sure there is a better way to fix. But at least, comments MUST BELONG to a valid Post. And then the Post policy applies. Hope it helps...

Axel

EDIT 1: thinking about it afterwards, the best way to solve this is in my opinion to globally change the implicit model binding method when using nested resources. The where condition I wrote could maybe be automatic in the future. In other words, when using nested resources, you cannot access a child object that does not belong to the parent object. That would solve a lot of things I guess...

pauladams8 commented 5 years ago

@royduin I've been having exactly the same problem and decided the best solution for me was to have two layers of policies.

Using your example, I'd create a route group for your post related sub-resources and apply the can:view,post middleware. Your PostPolicy would ensure the post belongs to the authenticated user.

You could then just use $this->authorizeResource(Comment::class); in the comment controller and use the CommentPolicy to ensure the comment belongs to the post, without needing to worry about authorising access to the post at that stage.

KeithBush commented 4 years ago

I came up with a workaround that allows you to use authorizeResourcein a nested resource pattern as long as the nested resource is included in the route path.

IMHO, the docs should contain an example of how to handle this scenario (similar to my example below) or the implementation of authorizeResource should be updated to allow passing additional resources needed for nesting.

My nested context is a little different than the Posts -> Comments relationship but I'll rewrite everything to use the previously mentioned example as it may be more relatable.

First, for this to work, the nested resource that you need must exist within the URL. Your route file may look similar to this:

    Route::resource('/post/{post}/comment', 'CommentController');

In your CommentController.php, you need a __construct method that calls authorizeResource.

class CommentController extends Controller
{
    public function __construct()
    {
        $this->authorizeResource(Comment::class);
    }

    // .... your other resource methods ...
}

In your CommentPolicy.php, you need a __construct method that does this:

class UserPolicy
{
    use HandlesAuthorization;

    public function __construct(Request $request)
    {
        $this->post = $request->route('post');
    }

    //  ... your other policy methods ...
}

Then in your individual policy checks, you should be able to access $this->post to get the nested resource from within the method.

    public function create(User $user)
    {
        dd($this->post); // dumps the post model resolved from the route!
    }

    // A more functional example ...
    public function delete(User $user, Comment $comment)
    {
        // Maybe comment creators can delete their comments...
        // Or the post owner can delete any comment associated with their post...
        return $user->id == $comment->user_id || 
            (
                $user->id == $this->post->user_id && 
                $comment->post_id == $this->post->id
            );
    }

If this is documented somewhere, I couldn't find it... At a minimum, I think it would be very helpful to have a similar example listed in the docs under Authorization.

lpeterke commented 4 years ago

Just like with $this->middleware() It is possible to do it like this:

$this->authorizeResource(Comment::class, 'comment,post');

However this doesn't work for resource methods without a model (index, create, store) as Laravel will use the model name instead of the parameter. See line 90 in AuthorizeRequests.php:

$modelName = in_array($method, $this->resourceMethodsWithoutModels()) ? $model : $parameter;

I was able to fix this by switching out this line with this bit of code:

if(in_array($method, $this->resourceMethodsWithoutModels())){
  if(Str::contains($parameter, Str::snake(class_basename($model)))){
    $modelName = Str::replaceFirst(Str::snake(class_basename($model)), $model, $parameter);
  } else {
    $modelName = $model;
  }
} else {
  $modelName = $parameter;
}

But this is just my quick fix, maybe someone can make a nice PR out of it?

cwilby commented 4 years ago

While I agree modifications could be made to Laravel - it is possible to do this using the middleware helper in __construct:

<?php

namespace App\Http\Controllers\Post;

use App\Http\Controllers\Controller;
use App\Http\Requests\StoreComment;
use App\Http\Requests\UpdateComment;
use App\Http\Resources\Polymorphic\CommentResource;
use App\Models\Polymorphic\Comment;
use App\Models\Post\Post;

class PostCommentsController extends Controller
{
    public function __construct()
    {
        $this->middleware('can:view,post')->only(['index', 'store']);
        $this->middleware('can:create,' . Comment::class)->only('store');
        $this->middleware('can:update,comment')->only('update');
        $this->middleware('can:destroy,comment')->only('destroy');
    }

    public function index(Post $post)
    {
        return CommentResource::collection($post->comments);
    }

    public function store(Post $post, StoreComment $request)
    {
        return $post->comments()->create(array_merge($request->validated(), [
            'user_id' => auth()->id()
        ]));
    }

    public function update(Comment $comment, UpdateComment $request)
    {
        $comment->update($request->validated());
    }

    public function destroy(Comment $comment)
    {
        $comment->delete();
    }
}
gdinko commented 3 years ago

Let's say we have nested resource "clients" of a business :

Route::resource('business.client', App\Http\Controllers\ClientController::class)->only([ 'index', 'create', 'edit' ]);

If we want to use build in authorizeResource() we can do this :

public function __construct() { $this->authorizeResource('App\Models\Client,business', 'client,business'); }

Don't forget to type hint business in client controller :

public function index(Business $business)

and in ClientPolicy

public function viewAny(User $user, Business $business)

Best Regards to all