cleaned up movie list policy

app/Http/Controllers/MovieListController.php

@@ -8,7 +8,6 @@ use App\Http\Resources\MovieListResource;
 use App\Interfaces\MovieDbInterface;
 use App\Models\Movie;
 use App\Models\MovieList;
-use App\Models\Role;
 use App\Models\User;
 use Illuminate\Http\JsonResponse;
 use Illuminate\Http\Request;
@@ -34,18 +33,18 @@ class MovieListController extends Controller
     /**
      * Store a newly created resource in storage.
      */
-    public function store(CreateMovieListRequest $request)
+    public function store(CreateMovieListRequest $request): MovieListResource
     {
         $this->authorize('create', MovieList::class);
 
         $validated = $request->validated();
         $movieList = MovieList::create([
             ...$validated,
-            'owner' => auth()->id(),
+            'owner' => Auth::user()->id,
             'slug' => Str::slug($validated['name']),
         ]);
 
-        return response()->json($movieList, 201);
+        return MovieListResource::make($movieList);
     }
 
     /**
@@ -77,12 +76,13 @@ class MovieListController extends Controller
         $this->authorize('delete', $movieList);
         $movieList->delete();
 
-        return response()->json(['message', 'Movie list deleted successfully'], 204);
+        return response()->json(['message' => 'Movie list deleted successfully'], 204);
     }
 
     public function addMovie(MovieDbInterface $movieDb, Request $request, MovieList $movieList): MovieListResource
     {
-        $this->authorize('update', $movieList);
+        $this->authorize('editMovies', $movieList);
+
         $movieResult = $movieDb->find($request->input('movie')['imdbId'], ['type' => 'imdb']);
         $movie = Movie::where('imdb_id', $movieResult->imdbId)->first();
 
@@ -94,7 +94,7 @@ class MovieListController extends Controller
 
     public function removeMovie(MovieList $movieList, Movie $movie): MovieListResource
     {
-        $this->authorize('update', $movieList);
+        $this->authorize('editMovies', $movieList);
 
         $movieList->movies()->detach($movie);
         $movieList->load('movies');
@@ -104,13 +104,13 @@ class MovieListController extends Controller
 
     public function updateCollaboratorRole(Request $request, MovieList $movieList, User $collaborator): MovieListResource|JsonResponse
     {
+        $this->authorize('update', $movieList);
         $request->validate([
             'role_id' => 'required|exists:roles,id',
         ]);
 
-        $adminRole = Role::query()->where('name', 'ADMIN')->first()?->id;
+        if (Auth::id() === $collaborator->getKey()) {
-        if (Auth::id() !== $movieList->owner && ! Auth::user()->hasRole($movieList, $adminRole)) {
+            return response()->json(['message' => 'Cannot edit own role'], 422);
-            return response()->json(['message' => 'Unauthorized'], 403);
         }
 
         $movieList->collaborators()->updateExistingPivot($collaborator->getKey(), [

app/Models/MovieList.php

@@ -27,7 +27,7 @@ class MovieList extends Model
         return $this->belongsToMany(Movie::class);
     }
 
-    public function getUserRole($userId): string
+    public function getUserRole($userId): ?string
     {
         $roleId = $this->collaborators()
             ->where('user_id', $userId)

app/Models/User.php

@@ -15,6 +15,10 @@ class User extends Authenticatable
     /** @use HasFactory<\Database\Factories\UserFactory> */
     use HasFactory, Notifiable;
 
+    private static $adminRoleId = null;
+
+    private static $editorRoleId = null;
+
     /**
      * The attributes that are mass assignable.
      *
@@ -45,10 +49,33 @@ class User extends Authenticatable
         return $this->hasMany(MovieList::class, 'owner');
     }
 
-    public function hasRole(MovieList $movieList, int $role): bool
+    public function isListEditor(MovieList $movieList): bool
+    {
+        self::$editorRoleId = Role::query()
+            ->where('name', 'EDITOR')
+            ->value('id');
+
+        return $this->isListAdmin($movieList) || $this->hasRole($movieList->getKey(), self::$editorRoleId);
+    }
+
+    public function isListAdmin(MovieList $movieList): bool
+    {
+        self::$adminRoleId = Role::query()
+            ->where('name', 'ADMIN')
+            ->value('id');
+
+        return $this->isListOwner($movieList) || $this->hasRole($movieList->getKey(), self::$adminRoleId);
+    }
+
+    public function isListOwner(MovieList $movieList): bool
+    {
+        return $this->getKey() === $movieList->owner;
+    }
+
+    public function hasRole(int $movieListId, int $role): bool
     {
         return $this->sharedLists()
-            ->wherePivot('movie_list_id', $movieList->id)
+            ->wherePivot('movie_list_id', $movieListId)
             ->wherePivot('role_id', $role)
             ->exists();
     }
@@ -60,6 +87,13 @@ class User extends Authenticatable
             ->withTimestamps();
     }
 
+    public function roles(): BelongsToMany
+    {
+        return $this->belongsToMany(Role::class, 'movie_list_user')
+            ->withPivot('role_id')
+            ->withTimestamps();
+    }
+
     /**
      * Get the attributes that should be cast.
      *

app/Policies/MovieListPolicy.php

@@ -22,29 +22,23 @@ class MovieListPolicy
 
     public function view(User $user, MovieList $movieList): bool
     {
-        if ($movieList->owner === $user->getKey() || $movieList->isPublic || $user->sharedLists->contains($movieList)) {
+        return $movieList->is_public
-            return true;
+            || $user->isListOwner($movieList)
-        }
+            || $user->sharedLists->contains($movieList);
-
-        return false;
-    }
-
-    public function update(User $user, MovieList $movieList): bool
-    {
-
-        if ($movieList->owner === $user->getKey()) {
-            return true;
-        }
-
-        return false;
     }
 
     public function delete(User $user, MovieList $movieList): bool
     {
-        if ($movieList->owner === $user->getKey()) {
+        return $user->isListOwner($movieList);
-            return true;
     }
 
-        return false;
+    public function editMovies(User $user, MovieList $movieList): bool
+    {
+        return $user->isListEditor($movieList);
+    }
+
+    public function update(User $user, MovieList $movieList): bool
+    {
+        return $user->isListAdmin($movieList);
     }
 }

routes/api.php

@@ -10,7 +10,6 @@ use Illuminate\Support\Facades\Route;
 // Public auth routes
 Route::post('/register', [AuthController::class, 'register'])->name('auth.register');
 Route::post('/login', [AuthController::class, 'login'])->name('auth.login');
-Route::post('/reset-password', [AuthController::class, 'resetPassword'])->name('auth.reset-password');
 Route::post('/forgot-password', [AuthController::class, 'forgotPassword'])->name('auth.forgot-password');
 Route::get('/invitations/{token}/accept', [InvitationController::class, 'accept'])->name('invitations.accept');
 Route::get('/invitations/{token}/decline', [InvitationController::class, 'decline'])->name('invitations.decline');
@@ -18,6 +17,7 @@ Route::get('/invitations/{token}/decline', [InvitationController::class, 'declin
 // Authenticated routes
 Route::middleware('auth:sanctum')->group(function () {
     Route::post('/logout', [AuthController::class, 'logout'])->name('auth.logout');
+    Route::post('/reset-password', [AuthController::class, 'resetPassword'])->name('auth.reset-password');
 
     // Invitations
     Route::post('/invitations', [InvitationController::class, 'store'])->name('invitations.store');

tests/Feature/AuthTest.php

@@ -16,8 +16,6 @@ class AuthTest extends TestCase
             ->postJson('/api/register', [
                 'username' => 'johndoe',
                 'email' => 'john@example.com',
-                'password' => 'password123',
-                'password_confirmation' => 'password123',
             ]);
 
         $response->assertStatus(201)
@@ -31,12 +29,10 @@ class AuthTest extends TestCase
         $response = $this->postJson('/api/register', [
             'username' => '',
             'email' => 'not-an-email',
-            'password' => 'short',
-            'password_confirmation' => 'mismatch',
         ]);
 
         $response->assertStatus(422)
-            ->assertJsonValidationErrors(['username', 'email', 'password']);
+            ->assertJsonValidationErrors(['username', 'email']);
     }
 
     public function test_registration_fails_with_duplicate_email(): void
@@ -46,8 +42,6 @@ class AuthTest extends TestCase
         $response = $this->postJson('/api/register', [
             'username' => 'johndoe',
             'email' => 'john@example.com',
-            'password' => 'password123',
-            'password_confirmation' => 'password123',
         ]);
 
         $response->assertStatus(422)
@@ -61,8 +55,6 @@ class AuthTest extends TestCase
         $response = $this->postJson('/api/register', [
             'username' => 'johndoe',
             'email' => 'john@example.com',
-            'password' => 'password123',
-            'password_confirmation' => 'password123',
         ]);
 
         $response->assertStatus(422)
@@ -111,18 +103,8 @@ class AuthTest extends TestCase
 
     public function test_unauthenticated_user_cannot_access_protected_routes(): void
     {
-        $response = $this->getJson('/api/user');
+        $response = $this->getJson('/api/roles');
 
         $response->assertStatus(401);
     }
-
-    public function test_authenticated_user_can_access_user_endpoint(): void
-    {
-        $user = User::factory()->create();
-
-        $response = $this->actingAs($user)->getJson('/api/user');
-
-        $response->assertOk()
-            ->assertJsonFragment(['email' => $user->email]);
-    }
 }

tests/Feature/UpdateCollaboratorRoleTest.php

@@ -14,28 +14,11 @@ class UpdateCollaboratorRoleTest extends TestCase
     use RefreshDatabase;
 
     private Role $adminRole;
+
     private Role $editorRole;
+
     private Role $viewerRole;
 
-    protected function setUp(): void
-    {
-        parent::setUp();
-        $this->seed(DatabaseSeeder::class);
-
-        $this->adminRole = Role::where('name', 'ADMIN')->first();
-        $this->editorRole = Role::where('name', 'EDITOR')->first();
-        $this->viewerRole = Role::where('name', 'VIEWER')->first();
-    }
-
-    private function makeList(User $owner): MovieList
-    {
-        return MovieList::create([
-            'name' => 'Test List',
-            'owner' => $owner->getKey(),
-            'slug' => 'test-list',
-        ]);
-    }
-
     public function test_role_id_is_required(): void
     {
         $owner = User::factory()->create();
@@ -50,6 +33,15 @@ class UpdateCollaboratorRoleTest extends TestCase
             ->assertJsonValidationErrors(['role_id']);
     }
 
+    private function makeList(User $owner): MovieList
+    {
+        return MovieList::create([
+            'name' => 'Test List',
+            'owner' => $owner->getKey(),
+            'slug' => 'test-list',
+        ]);
+    }
+
     public function test_role_id_must_exist_in_roles_table(): void
     {
         $owner = User::factory()->create();
@@ -125,6 +117,26 @@ class UpdateCollaboratorRoleTest extends TestCase
         $response->assertForbidden();
     }
 
+    public function test_admin_collaborator_cannot_update_own_role(): void
+    {
+        $owner = User::factory()->create();
+        $admin = User::factory()->create();
+        $movieList = $this->makeList($owner);
+        $movieList->collaborators()->attach($admin, ['role_id' => $this->adminRole->getKey()]);
+
+        $response = $this->actingAs($admin)
+            ->patchJson("/api/movielists/{$movieList->getKey()}/collaborators/{$admin->getKey()}", [
+                'role_id' => $this->editorRole->getKey(),
+            ]);
+
+        $response->assertUnprocessable();
+        $this->assertDatabaseHas('movie_list_user', [
+            'movie_list_id' => $movieList->getKey(),
+            'user_id' => $admin->getKey(),
+            'role_id' => $this->adminRole->getKey(),
+        ]);
+    }
+
     public function test_unrelated_user_cannot_update_collaborator_role(): void
     {
         $owner = User::factory()->create();
@@ -140,4 +152,14 @@ class UpdateCollaboratorRoleTest extends TestCase
 
         $response->assertForbidden();
     }
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        $this->seed(DatabaseSeeder::class);
+
+        $this->adminRole = Role::where('name', 'ADMIN')->first();
+        $this->editorRole = Role::where('name', 'EDITOR')->first();
+        $this->viewerRole = Role::where('name', 'VIEWER')->first();
+    }
 }