Feature: Laravel Passport Authentication

[poppabear8883]

In fulfillment of #154 .

[poppabear8883]

This just needs a test written for it

I know I know .... TDD

[cbj4074]

I'll fix these conflicts shortly and we can get this moving again!

[cbj4074]

Okay, I've fixed the conflicts and merged from master, so this branch is now on Laravel 7.

It seems that the CORS package namespace changed from barryvdh to fruitcake in Laravel 7, just FYI.

@mblarsen Do you have any interest in taking-over this effort, which would require writing tests for what @poppabear8883 has done thus far?

[mblarsen]

@poppabear8883 in case you haven't seen the testing helpers https://laravel.com/docs/7.x/passport#testing

[mblarsen]

@mblarsen Do you have any interest in taking-over this effort, which would require writing tests for what @poppabear8883 has done thus far?

Oh just saw this. Yes I can continue writing the tests.

[mblarsen]

@poppabear8883 @cbj4074 so I've started adding some tests, but ran into a few black holes in my knowledge and some issues with the ApiRoute.

For ApiRoute you can see that I added an option to set the middleware. But really I don't know what's the plan on which route that needs to be protected and which is not. I guess most reads are not but writes are. Maybe we need to make some changes to the ApiRouter defaults so that reads has just 'api' middleweare whereas the writes has 'api' and 'auth:api'. Of course there are some write routes that does not necessarily require auth (like upvoting or similar).

The when the routes are configured correctly all the controller tests will fail (at least the write ones). So I added a setUp to ControllerTestCase that act as a random user. I tried just adding 'auth:api' to all routes and this "fix" greenlights the tests.

I suggest not making any of these changes in this PR!? What do you think?

[mblarsen]

Btw, @poppabear8883 I cannot seem to find a register option. Shouldn't we add that?

[cbj4074]

@mblarsen I can assist by identifying which routes should be protected and to what extent, regarding reads vs. writes.

You're saying that you think we should go ahead and merge this, then worry about the rest of the required changes in a separate PR? That works for me.

[poppabear8883]

I wouldn't merge this until the Implementation has the proper tests written for it.

We can address modifying the ApiRoute and other things in different PR.

[mblarsen]

@poppabear8883 what is a proper test in this case?

Create token and manually include in header or something like that?

Right now there is a test that routes are not accessible without the authorized user. I added a route in the test alone because I didn't want to go ahead with the route changes as mentioned above.

@mblarsen I can assist by identifying which routes should be protected and to what extent, regarding reads vs. writes.

Sounds good. Even though the model I still don't have the strongest intuition about how it should beyond people shouldn't edit other peoples stuff :)

Re: merging, let's wait as @poppabear8883 says

[poppabear8883]

Sorry I misread your comment.

If there are required changes to other implementations (ApiRoute) to get tests working properly for this then that's fine we can do that in a separate PR.

However, this PR should not be merged before tests are done for this implementation. It should just be a general rule that no implementation is allowed in production without some reasonable tests included.

So, I propose that we make the other changes in another PR, merge that PR .. come back to this and make the necessary changes and then we shall merge this. Your guys thoughts ?

[mblarsen]

@poppabear8883 I like it. Let's do that. Should I create a proposal PR for those changes and then you can pitch in? I think it will be a good way for me to learn a bit more about the implementation.

[cbj4074]

Sounds good. Even though the model I still don't have the strongest intuition about how it should beyond people shouldn't edit other peoples stuff :)

Yep! That is the gist of it, but the rules grow more complex in a few different scenarios, which I will work to articulate and document in the near future. Those details seem beyond the scope of this PR, though, correct?

So, I propose that we make the other changes in another PR, merge that PR .. come back to this and make the necessary changes and then we shall merge this. Your guys thoughts ?

That probably does make the most sense. And yes, I agree completely that if a PR decreases code coverage, we don't merge it unless there's a good reason for an exception.

Speaking of which, I need to update the Codecov configuration so we get the inline reports in PRs. I'll do that momentarily.

As far as the specific nature of the tests required for this implementation is concerned, I'll have to defer to @poppabear8883 , as he knows far more about the implementation than I do at this point.