Merge pull request #28 from pedroborges/use-laravel-authorization

Use Laravel built-in authorization

app/Http/Controllers/API/Controller.php

@@ -3,7 +3,9 @@
 namespace App\Http\Controllers\API;
 
 use App\Http\Controllers\Controller as BaseController;
+use Illuminate\Foundation\Auth\Access\AuthorizesRequests;
 
 abstract class Controller extends BaseController
 {
+    use AuthorizesRequests;
 }

app/Http/Controllers/API/PlaylistController.php

@@ -29,20 +29,15 @@ class PlaylistController extends Controller
      * Rename a playlist.
      *
      * @param \Illuminate\Http\Request $request
-     * @param int                      $id
+     * @param Playlist                 $playlist
      *
      * @return \Illuminate\Http\JsonResponse
      */
-    public function update(Request $request, $id)
+    public function update(Request $request, Playlist $playlist)
     {
-        $playlist = Playlist::findOrFail($id);
+        $this->authorize('owner', $playlist);
 
-        if ($playlist->user_id !== auth()->user()->id) {
-            abort(403);
-        }
-
-        $playlist->name = $request->input('name');
-        $playlist->save();
+        $playlist->update($request->only('name'));
 
         return response()->json($playlist);
     }
@@ -52,17 +47,13 @@ class PlaylistController extends Controller
      * Any songs that are not populated here will be removed from the playlist.
      *
      * @param \Illuminate\Http\Request $request
-     * @param int                      $id
+     * @param Playlist                 $playlist
      *
      * @return \Illuminate\Http\JsonResponse
      */
-    public function sync(Request $request, $id)
+    public function sync(Request $request, Playlist $playlist)
     {
-        $playlist = Playlist::findOrFail($id);
-
-        if ($playlist->user_id !== auth()->user()->id) {
-            abort(403);
-        }
+        $this->authorize('owner', $playlist);
 
         $playlist->songs()->sync($request->input('songs'));
 
@@ -72,18 +63,15 @@ class PlaylistController extends Controller
     /**
      * Delete a playlist.
      *
-     * @param int $id
+     * @param Playlist $playlist
      *
      * @return \Illuminate\Http\JsonResponse
      */
-    public function destroy($id)
+    public function destroy(Playlist $playlist)
     {
-        // This can't be put into a Request authorize(), due to Laravel(?)'s limitation.
-        if (Playlist::findOrFail($id)->user_id !== auth()->user()->id) {
-            abort(403);
-        }
+        $this->authorize('owner', $playlist);
 
-        Playlist::destroy($id);
+        $playlist->delete();
 
         return response()->json();
     }

app/Http/Controllers/API/UserController.php

@@ -30,11 +30,11 @@ class UserController extends Controller
      * Update a user.
      *
      * @param UserUpdateRequest $request
-     * @param int               $id
+     * @param User              $user
      *
      * @return \Illuminate\Http\JsonResponse
      */
-    public function update(UserUpdateRequest $request, $id)
+    public function update(UserUpdateRequest $request, User $user)
     {
         $data = $request->only('name', 'email');
 
@@ -42,28 +42,26 @@ class UserController extends Controller
             $data['password'] = Hash::make($password);
         }
 
-        return response()->json(User::findOrFail($id)->update($data));
+        return response()->json($user->update($data));
     }
 
     /**
      * Delete a user.
      *
-     * @param int $id
+     * @param User $user
      *
      * @return \Illuminate\Http\JsonResponse
      */
-    public function destroy($id)
+    public function destroy(User $user)
     {
-        if (!auth()->user()->is_admin || auth()->user()->id === $id) {
-            abort(403);
-        }
+        $this->authorize($user);
 
-        return response()->json(User::destroy($id));
+        return response()->json($user->delete());
     }
 
     /**
      * Update the current user's profile.
-     * 
+     *
      * @param ProfileUpdateRequest $request
      *
      * @return \Illuminate\Http\JsonResponse

app/Http/Requests/API/UserUpdateRequest.php

@@ -23,7 +23,7 @@ class UserUpdateRequest extends Request
     {
         return [
             'name' => 'required',
-            'email' => 'required|email|unique:users,email,'.$this->route('user'),
+            'email' => 'required|email|unique:users,email,'.$this->route('user')->id,
         ];
     }
 }

app/Http/routes.php

@@ -31,7 +31,7 @@ Route::group(['prefix' => 'api', 'middleware' => 'auth', 'namespace' => 'API'],
     post('interaction/batch/unlike', 'InteractionController@batchUnlike');
 
     resource('playlist', 'PlaylistController', ['only' => ['store', 'update', 'destroy']]);
-    put('playlist/{id}/sync', 'PlaylistController@sync')->where(['id' => '\d+']);
+    put('playlist/{playlist}/sync', 'PlaylistController@sync')->where(['playlist' => '\d+']);
 
     resource('user', 'UserController', ['only' => ['store', 'update', 'destroy']]);
     put('me', 'UserController@updateProfile');

app/Models/User.php

@@ -32,6 +32,7 @@ AuthenticatableContract,
     protected $guarded = ['id'];
 
     protected $casts = [
+        'id' => 'int',
         'is_admin' => 'bool',
     ];
 

app/Policies/PlaylistPolicy.php

@@ -0,0 +1,17 @@
+<?php
+
+namespace App\Policies;
+
+use App\Models\Playlist;
+use App\Models\User;
+use Illuminate\Auth\Access\HandlesAuthorization;
+
+class PlaylistPolicy
+{
+    use HandlesAuthorization;
+
+    public function owner(User $user, Playlist $playlist)
+    {
+        return $user->id === $playlist->user_id;
+    }
+}

app/Policies/UserPolicy.php

@@ -0,0 +1,16 @@
+<?php
+
+namespace App\Policies;
+
+use App\Models\User;
+use Illuminate\Auth\Access\HandlesAuthorization;
+
+class UserPolicy
+{
+    use HandlesAuthorization;
+
+    public function destroy(User $currentUser, User $userToDestroy)
+    {
+        return $currentUser->is_admin && $currentUser->id !== $userToDestroy->id;
+    }
+}

app/Providers/AuthServiceProvider.php

@@ -2,6 +2,10 @@
 
 namespace App\Providers;
 
+use App\Models\Playlist;
+use App\Models\User;
+use App\Policies\PlaylistPolicy;
+use App\Policies\UserPolicy;
 use Illuminate\Contracts\Auth\Access\Gate as GateContract;
 use Illuminate\Foundation\Support\Providers\AuthServiceProvider as ServiceProvider;
 
@@ -13,7 +17,8 @@ class AuthServiceProvider extends ServiceProvider
      * @var array
      */
     protected $policies = [
-        'App\Model' => 'App\Policies\ModelPolicy',
+        Playlist::class => PlaylistPolicy::class,
+        User::class => UserPolicy::class,
     ];
 
     /**

app/Providers/RouteServiceProvider.php

@@ -25,9 +25,10 @@ class RouteServiceProvider extends ServiceProvider
      */
     public function boot(Router $router)
     {
-        //
-
         parent::boot($router);
+
+        $router->model('playlist', 'App\Models\Playlist');
+        $router->model('user', 'App\Models\User');
     }
 
     /**

tests/PlaylistTest.php

@@ -49,11 +49,16 @@ class PlaylistTest extends TestCase
     public function testUpdatePlaylistName()
     {
         $user = factory(User::class)->create();
+        $user2 = factory(User::class)->create();
 
         $playlist = factory(Playlist::class)->create([
             'user_id' => $user->id,
         ]);
 
+        $this->actingAs($user2)
+            ->put("api/playlist/{$playlist->id}", ['name' => 'Foo Bar'])
+            ->seeStatusCode(403);
+
         $this->actingAs($user)
             ->put("api/playlist/{$playlist->id}", ['name' => 'Foo Bar']);
 
@@ -63,15 +68,15 @@ class PlaylistTest extends TestCase
         ]);
 
         // Other users can't modify it
-        $response = $this->actingAs(factory(User::class)->create())
-            ->call('put', "api/playlist/{$playlist->id}", ['name' => 'Foo Bar']);
-
-        $this->assertEquals(403, $response->status());
+        $this->actingAs(factory(User::class)->create())
+            ->put("api/playlist/{$playlist->id}", ['name' => 'Foo Bar'])
+            ->seeStatusCode(403);
     }
 
     public function testSyncPlaylist()
     {
         $user = factory(User::class)->create();
+        $user2 = factory(User::class)->create();
 
         $playlist = factory(Playlist::class)->create([
             'user_id' => $user->id,
@@ -82,6 +87,12 @@ class PlaylistTest extends TestCase
 
         $removedSong = $songs->pop();
 
+        $this->actingAs($user2)
+            ->put("api/playlist/{$playlist->id}/sync", [
+                'songs' => array_pluck($songs->toArray(), 'id'),
+            ])
+            ->seeStatusCode(403);
+
         $this->actingAs($user)
             ->put("api/playlist/{$playlist->id}/sync", [
                 'songs' => array_pluck($songs->toArray(), 'id'),
@@ -104,14 +115,18 @@ class PlaylistTest extends TestCase
     public function testDeletePlaylist()
     {
         $user = factory(User::class)->create();
+        $user2 = factory(User::class)->create();
 
         $playlist = factory(Playlist::class)->create([
             'user_id' => $user->id,
         ]);
 
-        $this->actingAs($user)
-            ->delete("api/playlist/{$playlist->id}");
+        $this->actingAs($user2)
+            ->delete("api/playlist/{$playlist->id}")
+            ->seeStatusCode(403);
 
-        $this->notSeeInDatabase('playlists', ['id' => $playlist->id]);
+        $this->actingAs($user)
+            ->delete("api/playlist/{$playlist->id}")
+            ->notSeeInDatabase('playlists', ['id' => $playlist->id]);
     }
 }

tests/UserTest.php

@@ -11,14 +11,13 @@ class UserTest extends TestCase
     public function testCreateUser()
     {
         // Non-admins can't do shit
-        $response = $this->actingAs(factory(User::class)->create())
-            ->call('post', 'api/user', [
+        $this->actingAs(factory(User::class)->create())
+            ->post('api/user', [
                 'name' => 'Foo',
                 'email' => 'bar@baz.com',
                 'password' => 'qux',
-            ]);
-
-        $this->assertEquals(403, $response->status());
+            ])
+            ->seeStatusCode(403);
 
         // But admins can
         $this->actingAs(factory(User::class, 'admin')->create())
@@ -58,9 +57,16 @@ class UserTest extends TestCase
     public function testDeleteUser()
     {
         $user = factory(User::class)->create();
-        $this->actingAs(factory(User::class, 'admin')->create())
-            ->delete("api/user/{$user->id}");
+        $admin = factory(User::class, 'admin')->create();
 
-        $this->notSeeInDatabase('users', ['id' => $user->id]);
+        $this->actingAs($admin)
+            ->delete("api/user/{$user->id}")
+            ->notSeeInDatabase('users', ['id' => $user->id]);
+
+        // A user can't delete himself
+        $this->actingAs($admin)
+            ->delete("api/user/{$admin->id}")
+            ->seeStatusCode(403)
+            ->seeInDatabase('users', ['id' => $admin->id]);
     }
 }