From d2ad6107d195dc51f294a80b64b22f4e0b9c4452 Mon Sep 17 00:00:00 2001 From: Pig Fang Date: Thu, 25 Apr 2019 13:01:39 +0800 Subject: [PATCH] Refactor middlewares --- app/Http/Controllers/AuthController.php | 9 +++ app/Http/Kernel.php | 16 +++-- app/Http/Middleware/Authenticate.php | 19 ++++++ app/Http/Middleware/CheckAuthenticated.php | 68 ------------------- app/Http/Middleware/DetectLanguagePrefer.php | 20 ++---- app/Http/Middleware/EncryptCookies.php | 17 ----- app/Http/Middleware/EnsureEmailFilled.php | 19 ++++++ app/Http/Middleware/FireUserAuthenticated.php | 16 +++++ app/Http/Middleware/RejectBannedUser.php | 23 +++++++ app/Http/Middleware/VerifyCsrfToken.php | 17 ----- app/Providers/RouteServiceProvider.php | 7 +- app/Providers/RuntimeCheckServiceProvider.php | 3 - resources/views/auth/bind.blade.php | 6 +- routes/api.php | 2 +- routes/web.php | 13 ++-- tests/AuthControllerTest.php | 13 ++++ tests/MiddlewareTest.php | 66 +++++++++--------- 17 files changed, 160 insertions(+), 174 deletions(-) create mode 100644 app/Http/Middleware/Authenticate.php delete mode 100644 app/Http/Middleware/CheckAuthenticated.php delete mode 100644 app/Http/Middleware/EncryptCookies.php create mode 100644 app/Http/Middleware/EnsureEmailFilled.php create mode 100644 app/Http/Middleware/FireUserAuthenticated.php create mode 100644 app/Http/Middleware/RejectBannedUser.php delete mode 100644 app/Http/Middleware/VerifyCsrfToken.php diff --git a/app/Http/Controllers/AuthController.php b/app/Http/Controllers/AuthController.php index 44bcf8de..1164f84c 100644 --- a/app/Http/Controllers/AuthController.php +++ b/app/Http/Controllers/AuthController.php @@ -227,6 +227,15 @@ class AuthController extends Controller return json(trans('auth.reset.success'), 0); } + public function fillEmail(Request $request) + { + $email = $this->validate($request, ['email' => 'required|email|unique:users'])['email']; + $user = $request->user(); + $user->email = $email; + $user->save(); + return redirect('/user'); + } + public function verify($uid) { if (! option('require_verification')) { diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php index 158ee64a..203848d9 100644 --- a/app/Http/Kernel.php +++ b/app/Http/Kernel.php @@ -17,6 +17,7 @@ class Kernel extends HttpKernel \Illuminate\Foundation\Http\Middleware\CheckForMaintenanceMode::class, \Illuminate\Foundation\Http\Middleware\TrimStrings::class, \App\Http\Middleware\ConvertEmptyStringsToNull::class, + \App\Http\Middleware\DetectLanguagePrefer::class, ]; /** @@ -26,12 +27,12 @@ class Kernel extends HttpKernel */ protected $middlewareGroups = [ 'web' => [ - \App\Http\Middleware\EncryptCookies::class, + \Illuminate\Cookie\Middleware\EncryptCookies::class, \Illuminate\Cookie\Middleware\AddQueuedCookiesToResponse::class, \Illuminate\Session\Middleware\StartSession::class, \Illuminate\View\Middleware\ShareErrorsFromSession::class, - \App\Http\Middleware\DetectLanguagePrefer::class, \App\Http\Middleware\ForbiddenIE::class, + \Illuminate\Foundation\Http\Middleware\VerifyCsrfToken::class ], 'api' => [ @@ -39,7 +40,12 @@ class Kernel extends HttpKernel 'bindings', ], - 'static' => [], + 'authorize' => [ + 'auth:web', + \App\Http\Middleware\RejectBannedUser::class, + \App\Http\Middleware\EnsureEmailFilled::class, + \App\Http\Middleware\FireUserAuthenticated::class, + ], ]; /** @@ -50,9 +56,7 @@ class Kernel extends HttpKernel * @var array */ protected $routeMiddleware = [ - 'csrf' => \App\Http\Middleware\VerifyCsrfToken::class, - 'auth' => \App\Http\Middleware\CheckAuthenticated::class, - 'auth.jwt' => \Tymon\JWTAuth\Http\Middleware\Authenticate::class, + 'auth' => \App\Http\Middleware\Authenticate::class, 'bindings' => \Illuminate\Routing\Middleware\SubstituteBindings::class, 'verified' => \App\Http\Middleware\CheckUserVerified::class, 'guest' => \App\Http\Middleware\RedirectIfAuthenticated::class, diff --git a/app/Http/Middleware/Authenticate.php b/app/Http/Middleware/Authenticate.php new file mode 100644 index 00000000..4a2ca9e9 --- /dev/null +++ b/app/Http/Middleware/Authenticate.php @@ -0,0 +1,19 @@ +expectsJson()) { + session([ + 'last_requested_path' => $request->path(), + 'msg' => trans('auth.check.anonymous'), + ]); + return '/auth/login'; + } + } +} diff --git a/app/Http/Middleware/CheckAuthenticated.php b/app/Http/Middleware/CheckAuthenticated.php deleted file mode 100644 index cb596158..00000000 --- a/app/Http/Middleware/CheckAuthenticated.php +++ /dev/null @@ -1,68 +0,0 @@ -permission == User::BANNED) { - Auth::logout(); - - abort(403, trans('auth.check.banned')); - } - - // Ask for filling email - if ($user->email == '') { - return $this->askForFillingEmail($request, $next); - } - - event(new UserAuthenticated($user)); - - return $next($request); - } else { - $this->flashLastRequestedPath(); - - return redirect('auth/login')->with('msg', trans('auth.check.anonymous')); - } - } - - public function askForFillingEmail($request, Closure $next) - { - $user = Auth::user(); - - if (isset($request->email)) { - if (filter_var($request->email, FILTER_VALIDATE_EMAIL)) { - if (User::where('email', $request->email)->get()->isEmpty()) { - $user->email = $request->input('email'); - $user->save(); - - return $next($request); - } else { - return response()->view('auth.bind', ['msg' => trans('auth.bind.registered')]); - } - } else { - return response()->view('auth.bind', ['msg' => trans('auth.validation.email')]); - } - } - - return response()->view('auth.bind'); - } - - protected function flashLastRequestedPath($path = null) - { - $path = $path ?: app('request')->path(); - - return session(['last_requested_path' => $path]); - } -} diff --git a/app/Http/Middleware/DetectLanguagePrefer.php b/app/Http/Middleware/DetectLanguagePrefer.php index 866e14c6..1a325b74 100644 --- a/app/Http/Middleware/DetectLanguagePrefer.php +++ b/app/Http/Middleware/DetectLanguagePrefer.php @@ -10,25 +10,13 @@ class DetectLanguagePrefer { public function handle($request, \Closure $next) { - $response = $next($request); - - if ($response instanceof Response) { - $response->cookie('locale', config('app.locale')); - } - - return $response; - } - - public function detect(Request $request) - { - $locale = $request->input('lang') ?: ($request->cookie('locale') ?: $request->getPreferredLanguage()); - - // If current locale is an alias of other locale + $locale = $request->input('lang') ?? session('locale') ?? $request->getPreferredLanguage(); if (($info = Arr::get(config('locales'), $locale)) && ($alias = Arr::get($info, 'alias'))) { $locale = $alias; } - app()->setLocale($locale); - session(['locale' => config('app.locale')]); + session(['locale' => $locale]); + + return $next($request); } } diff --git a/app/Http/Middleware/EncryptCookies.php b/app/Http/Middleware/EncryptCookies.php deleted file mode 100644 index 2f46ffe1..00000000 --- a/app/Http/Middleware/EncryptCookies.php +++ /dev/null @@ -1,17 +0,0 @@ -user()->email != '' && $request->is('auth/bind')) { + return redirect('/user'); + } elseif ($request->user()->email == '' && ! $request->is('auth/bind')) { + return redirect('/auth/bind'); + } + + return $next($request); + } +} diff --git a/app/Http/Middleware/FireUserAuthenticated.php b/app/Http/Middleware/FireUserAuthenticated.php new file mode 100644 index 00000000..0e8291b2 --- /dev/null +++ b/app/Http/Middleware/FireUserAuthenticated.php @@ -0,0 +1,16 @@ +check()) { + event(new \App\Events\UserAuthenticated($request->user())); + } + return $next($request); + } +} diff --git a/app/Http/Middleware/RejectBannedUser.php b/app/Http/Middleware/RejectBannedUser.php new file mode 100644 index 00000000..91043d01 --- /dev/null +++ b/app/Http/Middleware/RejectBannedUser.php @@ -0,0 +1,23 @@ +user()->permission == User::BANNED) { + if ($request->expectsJson()) { + $response = json(trans('auth.check.banned'), -1); + $response->setStatusCode(403); + return $response; + } else { + abort(403, trans('auth.check.banned')); + } + } + return $next($request); + } +} diff --git a/app/Http/Middleware/VerifyCsrfToken.php b/app/Http/Middleware/VerifyCsrfToken.php deleted file mode 100644 index 0c13b854..00000000 --- a/app/Http/Middleware/VerifyCsrfToken.php +++ /dev/null @@ -1,17 +0,0 @@ -group([ - 'middleware' => ['web', 'csrf'], + 'middleware' => ['web'], 'namespace' => $this->namespace, ], function ($router) { require base_path('routes/web.php'); @@ -93,10 +93,7 @@ class RouteServiceProvider extends ServiceProvider */ protected function mapStaticRoutes(Router $router) { - $router->group([ - 'middleware' => 'static', - 'namespace' => $this->namespace, - ], function ($router) { + $router->group(['namespace' => $this->namespace], function ($router) { require base_path('routes/static.php'); }); } diff --git a/app/Providers/RuntimeCheckServiceProvider.php b/app/Providers/RuntimeCheckServiceProvider.php index 9a3794f5..9fec9efe 100644 --- a/app/Providers/RuntimeCheckServiceProvider.php +++ b/app/Providers/RuntimeCheckServiceProvider.php @@ -16,9 +16,6 @@ class RuntimeCheckServiceProvider extends ServiceProvider */ public function boot(Request $request) { - // Detect current locale - $this->app->call('App\Http\Middleware\DetectLanguagePrefer@detect'); - // Skip the installation check when in setup or under CLI if ($request->is('setup*') || $this->app->runningInConsole()) { return; diff --git a/resources/views/auth/bind.blade.php b/resources/views/auth/bind.blade.php index 65cbb789..b2febf8a 100644 --- a/resources/views/auth/bind.blade.php +++ b/resources/views/auth/bind.blade.php @@ -12,7 +12,7 @@
-
+ @csrf
@@ -21,8 +21,8 @@

@lang('auth.bind.introduction')

- @if (isset($msg)) -
{{ $msg }}
+ @if ($errors->count() > 0) +
{{ $errors->first() }}
@endif
diff --git a/routes/api.php b/routes/api.php index e827b78f..293829d7 100644 --- a/routes/api.php +++ b/routes/api.php @@ -6,7 +6,7 @@ Route::prefix('auth')->group(function ($route) { $route->post('refresh', 'AuthController@apiRefresh')->middleware('auth.jwt'); }); -Route::prefix('user')->middleware('auth.jwt')->group(function ($route) { +Route::prefix('user')->middleware('auth:api')->group(function ($route) { $route->put('sign', 'UserController@sign'); $route->get('players', 'PlayerController@listAll'); diff --git a/routes/web.php b/routes/web.php index 7d530e6a..af915933 100644 --- a/routes/web.php +++ b/routes/web.php @@ -1,5 +1,7 @@ 'auth'], function () { Route::post('/register', 'AuthController@handleRegister'); Route::post('/forgot', 'AuthController@handleForgot'); Route::post('/reset/{uid}', 'AuthController@handleReset')->middleware('signed'); - + Route::view('/bind', 'auth.bind')->middleware(['authorize', Middleware\EnsureEmailFilled::class]); + Route::post('/bind', 'AuthController@fillEmail')->middleware(['authorize', Middleware\EnsureEmailFilled::class]); Route::get('/verify/{uid}', 'AuthController@verify')->name('auth.verify')->middleware('signed'); }); @@ -42,7 +45,7 @@ Route::group(['prefix' => 'auth'], function () { * User Center */ Route::group([ - 'middleware' => ['web', 'auth', \App\Http\Middleware\RequireBindPlayer::class], + 'middleware' => ['authorize', Middleware\RequireBindPlayer::class], 'prefix' => 'user', ], function () { Route::any('', 'UserController@index'); @@ -90,7 +93,9 @@ Route::group(['prefix' => 'skinlib'], function () { Route::any('/show/{tid}', 'SkinlibController@show'); Route::any('/data', 'SkinlibController@getSkinlibFiltered'); - Route::group(['middleware' => ['auth', 'verified']], function () { + Route::group([ + 'middleware' => ['authorize', 'verified'] + ], function () { Route::get('/upload', 'SkinlibController@upload'); Route::post('/upload', 'SkinlibController@handleUpload'); Route::post('/model', 'SkinlibController@model'); @@ -104,7 +109,7 @@ Route::group(['prefix' => 'skinlib'], function () { /* * Admin Panel */ -Route::group(['middleware' => ['auth', 'admin'], 'prefix' => 'admin'], function () { +Route::group(['middleware' => ['authorize', 'admin'], 'prefix' => 'admin'], function () { Route::view('/', 'admin.index'); Route::get('/chart', 'AdminController@chartData'); diff --git a/tests/AuthControllerTest.php b/tests/AuthControllerTest.php index 79bfb52c..3c491d6e 100644 --- a/tests/AuthControllerTest.php +++ b/tests/AuthControllerTest.php @@ -505,6 +505,19 @@ class AuthControllerTest extends TestCase $this->assertTrue($user->verifyPassword('12345678')); } + public function testFillEmail() + { + $user = factory(User::class)->create(['email' => '']); + $other = factory(User::class)->create(); + $this->actingAs($user)->post('/auth/bind')->assertRedirect('/'); + $this->actingAs($user)->post('/auth/bind', ['email' => 'a'])->assertRedirect('/'); + $this->actingAs($user)->post('/auth/bind', ['email' => $other->email])->assertRedirect('/'); + + $this->actingAs($user)->post('/auth/bind', ['email' => 'a@b.c'])->assertRedirect('/user'); + $user->refresh(); + $this->assertEquals('a@b.c', $user->email); + } + public function testVerify() { $url = URL::signedRoute('auth.verify', ['uid' => 1]); diff --git a/tests/MiddlewareTest.php b/tests/MiddlewareTest.php index 5f0c6c18..6aaa2c4f 100644 --- a/tests/MiddlewareTest.php +++ b/tests/MiddlewareTest.php @@ -2,6 +2,7 @@ namespace Tests; +use Event; use App\Models\User; use App\Models\Player; use App\Services\Facades\Option; @@ -12,42 +13,10 @@ class MiddlewareTest extends TestCase { use DatabaseTransactions; - public function testCheckAuthenticated() + public function testAuthenticate() { - // Not logged in $this->get('/user')->assertRedirect('auth/login'); - $this->assertGuest(); - - // Normal user - $this->actAs('normal') - ->assertAuthenticated(); - - // Banned User - $this->actAs('banned') - ->get('/user') - ->assertSee('banned') - ->assertStatus(403); - - // Binding email - $noEmailUser = factory(\App\Models\User::class)->create(['email' => '']); - $this->actingAs($noEmailUser) - ->get('/user') - ->assertSee('Bind') - ->assertDontSee('User Center'); - - $this->actingAs($noEmailUser) - ->get('/user?email=email') - ->assertSee('Bind'); - - $other = factory(User::class)->create(); - $this->actingAs($noEmailUser) - ->get('/user?email='.$other->email) - ->assertSee(trans('auth.bind.registered')); - - $this->actingAs($noEmailUser) - ->get('/user?email=a@b.c') - ->assertSee('User Center'); - $this->assertEquals('a@b.c', User::find($noEmailUser->uid)->email); + $this->actAs('normal')->assertAuthenticated(); } public function testCheckUserVerified() @@ -168,6 +137,26 @@ class MiddlewareTest extends TestCase ]); } + public function testEnsureEmailFilled() + { + $noEmailUser = factory(User::class)->create(['email' => '']); + $this->actingAs($noEmailUser)->get('/user')->assertRedirect('/auth/bind'); + + $normalUser = factory(User::class)->create(); + $this->actingAs($normalUser)->get('/auth/bind')->assertRedirect('/user'); + } + + public function testFireUserAuthenticated() + { + Event::fake(); + $user = factory(User::class)->create(); + $this->actingAs($user)->get('/user'); + Event::assertDispatched(\App\Events\UserAuthenticated::class, function ($event) use ($user) { + $this->assertEquals($user->uid, $event->user->uid); + return true; + }); + } + public function testRedirectIfAuthenticated() { $this->get('/auth/login') @@ -179,6 +168,15 @@ class MiddlewareTest extends TestCase ->assertRedirect('/user'); } + public function testRejectBannedUser() + { + $user = factory(User::class, 'banned')->create(); + $this->actingAs($user)->get('/user')->assertForbidden(); + $this->get('/user', ['accept' => 'application/json']) + ->assertForbidden() + ->assertJson(['code' => -1, 'message' => trans('auth.check.banned')]); + } + public function testRequireBindPlayer() { $user = factory(User::class)->create();