TobiasLounsbury / EmpireSkillTest

0 stars 0 forks source link

Nitpick: Presence of an authenticated user is better served in Middleware or Form Request #7

Open djfrailey opened 4 years ago

djfrailey commented 4 years ago

The following check could be eliminated if the auth or auth:api middleware were declared on the Route.

https://github.com/TobiasLounsbury/EmpireSkillTest/blob/9bebb5ebaa5bbde267e03a2ec544b28af9f9c286/app/Http/Controllers/GameController.php#L56

This might require moving routes from routes/api.php to routes/web.php for session token authentication instead of using an api token. This has to do with the way the out of the box http kernel's middleware stack is configured. You could re-configure that stack to allow for session token authentication in either the routes/api.php file or the routes/web.php file. Or change the behavior of RouteServiceProvider::mapApiRoutes() to not automatically apply the api middleware stack to all routes in the routes/api.php file.

Another option would be to create a FormRequest and override FormRequest::authorize to return $this->user()

https://laravel.com/docs/5.8/validation#authorizing-form-requests

TobiasLounsbury commented 4 years ago

I agree completely. This was one of the pieces of code that I felt dissatisfied by in this project. I had some issues with getting the middleware stack to function the way I expected I was getting a mismatched CSRF token when I attempted to use session based authentication and after spending some time on in I made the choice to "punt" for the sake of completing the task, with the expectation that you would see the result, not like it, and that we could have a nice conversation about the best way to implement this and that I would learn something.