Override "id" method in JWTGuard

[mabdullahsari]

Description

Hiya! 👋

PR #257 added a nice getUserId method to prevent database round-trips. However, I'm not really sure why he decided to go this route because all authentication guards implementing the GuardHelpers trait already contain such a method (which is also the one being called when you use Auth::id()).

This PR simply adds this functionality to the id method, so you can also benefit from the eliminated database round-trips when using auth()->id() or Auth::id().

Checklist:

• I've added tests for my changes or tests are not applicable
• I've changed documentations or changes are not required
• I've added my changes to CHANGELOG.md

[Messhias]

Looks good to me.

[mabdullahsari]

There is a missing piece.

[mabdullahsari]

I'm updating it as we speak.

[mabdullahsari]

So I added a fallback clause to the default behavior because people might be using test fakes like $this->be and $this->actingAs, in which case the getUserId() method won't work.

[Messhias]

So I added a fallback clause to the default behavior because people might be using test fakes like $this->be and $this->actingAs, in which case the getUserId() method won't work.

Considering the user "always" is logged in that could work, but I approved the workflow again.

Also can you elaborate more tests case / scenarios?

Just be in case of something can go wrong.

[mabdullahsari]

Also can you elaborate more tests case / scenarios?

Just be in case of something can go wrong.

Sure thing! So normally, when using the real system, you're going to provide a JWT and the JWTGuard is going to authenticate you based on this token. When the system calls Auth::id(), everything is going to be fine.

However, things change slightly when you would to like test your HTTP endpoints. Calling the login endpoint each time before running your test cases would be extremely cumbersome, so Laravel provides the be and actingAs (alias for be) tester methods that you can use in order to authenticate before calling your endpoints.

An example:

#[Test]
public function user_can_retrieve_their_details(): void
{
    $response = $this->be($this->aUser(), 'jwt')->get('https://api.myapp.test/v1/me');

    $response->assertOk()->assertJsonStructure([
        'data' => [
            'email',
            'first_name',
            'id',
            'last_name',
            'phone' => ['country', 'number'],
        ],
    ]);
}

The "problem" here is that, since the user is not authenticated through JWT, a direct call to getUserId is going to return null but a call to user() is going to return the value defined in the aUser test method. Using this approach, we can still provide the performance benefits that we get from JWT while still making sure that HTTP tests work as intended.

Let me know if there's something else you'd like to know!

[mfn]

On paper, the change sounds indeed good, but IMHO this requires a major version bump due to change in behaviour.

Why? Users may rely that the "user" is always resolved, which isn't true anymore.
They could have implemented certain expectations around their user provider resolver, which then aren't matched anymore. I'm talking about the default id() behaviour inherited from \Illuminate\Auth\GuardHelpers::id().

Also: there should be a test showing this change.

[mabdullahsari]

Why? Users may rely that the "user" is always resolved, which isn't true anymore.

Hello! 👋

Thank you for your more profound considerations. What you are saying is indeed true and a possibility which shouldn’t be blindly overlooked. However, you can also probably understand that this is a moot point because the method name itself literally suggests that you’d like to retrieve the user’s identifier. So, the primary behavior of this function is to return the identifier. This insinuates “whatever means necessary”. As the API’s consumer, as long as I receive an identifier, I am golden.

Again, your observation is most definitely correct and I’d like to present my own train of thoughts here. It’s a good thing to consider possible problems. 😄

I shall add tests if all objections are cleared up and the PR receives greenlight again.

Thank you!