From e54fecd6e5cf6241c7a944fa77393e16f3bb57a5 Mon Sep 17 00:00:00 2001
From: Pig Fang <g-plane@hotmail.com>
Date: Wed, 3 Jun 2020 09:24:31 +0800
Subject: [PATCH] simplify player owner checking

---
 app/Http/Controllers/PlayerController.php     | 11 ++++++-
 app/Http/Middleware/CheckPlayerOwner.php      | 20 -------------
 .../ControllersTest/PlayerControllerTest.php  | 14 +++++++++
 .../MiddlewareTest/CheckPlayerOwnerTest.php   | 30 -------------------
 4 files changed, 24 insertions(+), 51 deletions(-)
 delete mode 100644 app/Http/Middleware/CheckPlayerOwner.php
 delete mode 100644 tests/HttpTest/MiddlewareTest/CheckPlayerOwnerTest.php

diff --git a/app/Http/Controllers/PlayerController.php b/app/Http/Controllers/PlayerController.php
index 7cbff533..9957fb3c 100644
--- a/app/Http/Controllers/PlayerController.php
+++ b/app/Http/Controllers/PlayerController.php
@@ -22,7 +22,16 @@ class PlayerController extends Controller
 {
     public function __construct()
     {
-        $this->middleware([CheckPlayerOwner::class], [
+        $this->middleware(function (Request $request, $next) {
+            /** @var Player */
+            $player = $request->route('player');
+            if ($player->user->isNot($request->user())) {
+                return json(trans('admin.players.no-permission'), 1)
+                    ->setStatusCode(403);
+            }
+
+            return $next($request);
+        }, [
             'only' => ['delete', 'rename', 'setTexture', 'clearTexture'],
         ]);
     }
diff --git a/app/Http/Middleware/CheckPlayerOwner.php b/app/Http/Middleware/CheckPlayerOwner.php
deleted file mode 100644
index 022c88f1..00000000
--- a/app/Http/Middleware/CheckPlayerOwner.php
+++ /dev/null
@@ -1,20 +0,0 @@
-<?php
-
-namespace App\Http\Middleware;
-
-use App\Models\Player;
-use Closure;
-use Illuminate\Support\Arr;
-
-class CheckPlayerOwner
-{
-    public function handle($request, Closure $next)
-    {
-        $pid = Arr::get($request->route()->parameters, 'pid') ?? $request->input('pid');
-        if ($pid && ($player = Player::find($pid)) && $player->uid != auth()->id()) {
-            return json(trans('admin.players.no-permission'), 1);
-        }
-
-        return $next($request);
-    }
-}
diff --git a/tests/HttpTest/ControllersTest/PlayerControllerTest.php b/tests/HttpTest/ControllersTest/PlayerControllerTest.php
index fd2302bb..95f6d815 100644
--- a/tests/HttpTest/ControllersTest/PlayerControllerTest.php
+++ b/tests/HttpTest/ControllersTest/PlayerControllerTest.php
@@ -37,6 +37,20 @@ class PlayerControllerTest extends TestCase
             ->assertJson([$player->toArray()]);
     }
 
+    public function testAccessControl()
+    {
+        $user = factory(User::class)->make();
+        $player = factory(Player::class)->create();
+
+        $this->actingAs($user)
+            ->deleteJson(route('user.player.delete', ['player' => $player]))
+            ->assertJson([
+                'code' => 1,
+                'message' => trans('admin.players.no-permission'),
+            ])
+            ->assertForbidden();
+    }
+
     public function testAdd()
     {
         Event::fake();
diff --git a/tests/HttpTest/MiddlewareTest/CheckPlayerOwnerTest.php b/tests/HttpTest/MiddlewareTest/CheckPlayerOwnerTest.php
deleted file mode 100644
index ca93cc40..00000000
--- a/tests/HttpTest/MiddlewareTest/CheckPlayerOwnerTest.php
+++ /dev/null
@@ -1,30 +0,0 @@
-<?php
-
-namespace Tests;
-
-use App\Models\Player;
-use App\Models\User;
-use Illuminate\Foundation\Testing\DatabaseTransactions;
-
-class CheckPlayerOwnerTest extends TestCase
-{
-    use DatabaseTransactions;
-
-    public function testHandle()
-    {
-        $other_user = factory(User::class)->create();
-        $player = factory(Player::class)->create();
-        $owner = $player->user;
-
-        $this->actingAs($other_user)
-            ->get('/user/player')
-            ->assertSuccessful();
-
-        $this->actingAs($other_user)
-            ->postJson('/user/player/rename/'.$player->pid)
-            ->assertJson([
-                'code' => 1,
-                'message' => trans('admin.players.no-permission'),
-            ]);
-    }
-}