swisnl / json-api-server

Set up a JSON API in Laravel in just a few minutes.
MIT License
105 stars 19 forks source link

Authenticate tests not working #11

Closed liepumartins closed 6 years ago

liepumartins commented 6 years ago

Tests use WithoutMiddleware trait https://github.com/swisnl/json-api-server/blob/18f07380ba0e09cd60c5eeaa406ba64628009fde/sample/Tests/SampleAuthenticationTest.php#L13

This effectively cancels all middleware, including auth:api, thus all *_unauthenticated() tests fail, because no authentication is required and operations complete same way they would for authenticated user.

liepumartins commented 6 years ago

I see that middleware is disabled, because tests do not make requests in application/vnd.api+json format. This should not be so.

Nevertheless I experimented with AuthenticationTest.php:

bbrala commented 6 years ago

Hmm, interesting, that is a bit of an oversight i recon. I would need to check how we test in our implementations, perhaps we made some changes there which were not migrated to the package. @Arnaudvz perhaps you could check this?

liepumartins commented 6 years ago

Some of simple tests

$response = $this->get($this->baseUrl);`

can be rewritten like

$response = $this->withHeaders(['Accept'=>'application/vnd.api+json'])->get($this->baseUrl);`

Or maybe set the withHeaders() for all tests, if possible and suits all of them.

As for 401 and 302 responses, I should double check, maybe Laravel by default does not do it and I have made it do so, somewhere. :)

liepumartins commented 6 years ago

Answer to 302 instead of 401 response also lies with #12 Laravel Exception handler responds with 302 by default and 401 only if InteractsWithContentTypes::expectsJson() is true, which is true for ajax calls or calls with header "Accept" which contains either "/json" or "+json". Never worked with "Content-Type"

bbrala commented 6 years ago

The middleware is tested, but i agree it it nicer if the headers are checked.

Interesting find on the Accept header there, it seems the specification doesnt really force sending the Accept header though...

bbrala commented 6 years ago

Merged the change which adds the withHeaders calls. And released 0.3.2

liepumartins commented 6 years ago

Awesome, thank You!