Having multiple Authenticatable models is incorrectly tracked

[martindj]

Pulse Version

v1.0.0-beta2

Laravel Version

10.26.2

PHP Version

8.2.13

Database Driver & Version

No response

Description

User requests are logged with only the ID value in the key-field, however it is possible to have multiple Authenticatable models in Laravel.

The default behavior for Pulse seems to be to log all Authenticated requests, also for other Authenticatable models, but not differentiate the model type.

This leads to a bug where request count are aggregated towards the User model with the ID even if another model is making the authenticated request.

Steps To Reproduce

Install Laravel Pulse on a project with a custom Authenticatable model that can login for instance using Laravel\Sanctum\HasApiTokens

[toitzi]

It is possible - i think - to work arround this by using a custom UserRecorder and usersResolver. What i could also think of, is adapting pulse default behaviour by saving the model of the user (either in a new column or in the "values" field) and then serializing or something like it from that one. I am not sure about that or what would be the preferred way here, but if one of the team members has an idea of how to implement it or how - if at all - they would like to have it implemented, i'm happy to work on it and do a PR, just let me know.

[jessarcher]

The recorder could probably capture a JSON serialized tuple of [Model, id], similar to what we do in other places. The resolving logic would need some tweaks though.

[martindj]

It would be preferable that the solution requires no configuration if possible, to keep Laravel Pulse working out of the box with different Laravel setups.

Storing a [Model, id] tuple sounds like a great idea, however that requires changes to the resolver function as well as changes to how custom resolvers using Pulse::users work.

[juhasev]

We also ran into this issue with a multitenant application. Ideally, you would have one admin panel for all your tenants if your primary goal is to look into server performance in general.

[timacdonald]

We’ll likely need to capture the “user provider”, rather than the model.

I’ve started working on a fix and we’ll have this out shortly.

[timacdonald]

Would love some thoughts here.

I could capture the user id + user provider. This would likely solve the problem for standard setups, but it does require that every user provider is registered in config/auth.php and is a static value across requests.

In the case of multi tenancy, where you may change your config based on the incoming request, this may be more difficult to handle in a generic manner.

My thoughts are:

1. We capture the id and provider by default, e.g., [$id, $provider] = [2345, "users"] where "users" references auth.providers.users.
2. We provide a hook for more complex setups to provide a list of details that are reqired to uniquely identify a user.

Pulse::setUser(fn () => [Request::tennant()->id, Auth::id()]);

and then when resolving the users you could access those details...

Pulse::users(function ($users) {
   return $users->mapSpread(function ($tennant, $user) {     
       return Tennant::find($tennant)->user($user);
   });
});

Would this solve the problem we have here? Are there any other suggestions on how this could be solved?

[AlexDanault]

but it does require that every user provider is registered in config/auth.php and is a static value across requests

Would this work for guards registered in packages, like this:

        $this->app->auth->provider('admin', function ($app, array $config) {
            return new AdminUserProvider($app['hash'], $config['model']);
        });

        Config::set('auth.guards.admin', [
            'driver' => 'session',
            'provider' => 'admin',
        ]);

        Config::set('auth.providers.admin', [
            'driver' => 'admin',
            'model' => Admin::class,
        ]);

I'm guessing that yes, it would work. If that is the case, for my use-case your solution would be perfect.

[timacdonald]

@AlexDanault I believe it would work by default with your setup. No extra config required.

We would store [432,"admin"], where "admin" points to your 'auth.providers.admin'.

The under the hood we would...

$user = Auth::createUserProvider("admin")->retrieveById(432);

[AlexDanault]

We would store [432,"admin"], where "admin" points to your 'auth.providers.admin'.

Then I'm sure this would solve 95% of issues, and the hook you provide would fixe the remaining 5% easily.

[juhasev]

@timacdonald I think the suggested solution would work for multitenant applications. Our tenant ID is basically UUID4.

Pulse::setUser(fn () => [Request::tennant()->id, Auth::id()]);

[jny986]

Would love some thoughts here.

I could capture the user id + user provider. This would likely solve the problem for standard setups, but it does require that every user provider is registered in config/auth.php and is a static value across requests.

In the case of multi tenancy, where you may change your config based on the incoming request, this may be more difficult to handle in a generic manner.

My thoughts are:

1. We capture the id and provider by default, e.g., [$id, $provider] = [2345, "users"] where "users" references auth.providers.users.
2. We provide a hook for more complex setups to provide a list of details that are reqired to uniquely identify a user.

Pulse::setUser(fn () => [Request::tennant()->id, Auth::id()]);

and then when resolving the users you could access those details...

Pulse::users(function ($users) {
   return $users->mapSpread(function ($tennant, $user) {     
       return Tennant::find($tennant)->user($user);
   });
});

Would this solve the problem we have here? Are there any other suggestions on how this could be solved?

I like the ideas above however it does not allow for the possibility of multi tenant and multiple providers which is the situation that I find myself in.

In order for pulse to be able to get the user information it needs to initiate the tenant and then get the user from the correct provider.

I think that you would need an optional 3rd data saved which is the tenant id and then have a way of informing pulse how to initialize and end the tenant session.

Love your work. Hope this helps

[timacdonald]

Josh! 👋

I think what I'm proposing should suit your needs.

In the following snippet...

Pulse::setUser(fn () => [Request::tennant()->id, Auth::id()]);

what is returned from the closure could be an array containing anything. So you could include something like [$tennantId, $userId, $userProvider], then you also have control over how this is information is rehydrated back into a user.

You could do something like...

Pulse::users(function ($users) {
   return $users->mapSpread(function (string $tennantId, string $userId, string $userProvider) {     
       Tennant::initialize($tennantId);

       $user = Auth::createUserProvider($userProvider)->retrieveById($userId);

       Tennant::endSession();

       return $user;
   });
});

Totally just making up the user land APIs of course.

How does that fit with you, @jny986

[jny986]

Josh! 👋

I think what I'm proposing should suit your needs.

In the following snippet...

Pulse::setUser(fn () => [Request::tennant()->id, Auth::id()]);

what is returned from the closure could be an array containing anything. So you could include something like [$tennantId, $userId, $userProvider], then you also have control over how this is information is rehydrated back into a user.

You could do something like...

Pulse::users(function ($users) {
   return $users->mapSpread(function (string $tennantId, string $userId, string $userProvider) {     
       Tennant::initialize($tennantId);

       $user = Auth::createUserProvider($userProvider)->retrieveById($userId);

       Tennant::endSession();

       return $user;
   });
});

Totally just making up the user land APIs of course.

How does that fit with you, @jny986

Yeah I think that would work perfectly.

Is the Pulse::setUser(...) a new method that you would add or is it something similar to resolveAuthenticatedUserIdUsing

Any idea when this will come out and is there anything else that I can do to help?

[adrianspacely]

In reply to @timacdonald's question in PHP Australia: (I'm not familiar with multiple auths and I'm not sure how pulse "segments" stats by user, so this is just multitenancy thoughts)

Can you just allow the ability to "categorise/tag" instead of worrying about how multi-tenancy is done? There are three contexts to consider. I recommend a simple Pulse::tag(...) which can be placed in:

• Jobs: Queue::before() and Queue::after() for jobs.
• Console: app()->runningInConsole() && app()->runningUnitTests() === false for commands (I understand the edge cases with both sides of those conditionals - perhaps you need to add Command::before() and Command::after() hooks to make that more solid if they don't exist already)
• Http: Middleware@handle and Middleware@terminate.
• User land: the "set tenant" logic.

Perhaps the docs can outline those 3 or 4 contexts as a home for Pulse::tag("tenant-{$tenant->id}). I suggest this over Pulse::tagUsing() because you need to consider if the Queue/Command/Middleware "after hooks" have already removed the multi-tenancy context you'd need to infer a tag with.

Caveat: Both queue workers and Octane are daemons. Pulse can probably handle a Pulse::clearTags() or Pulse::reset() call after "committing".

What if: the user only unsets the tenant when it changes and three jobs in a row on the same worker can run without the "switch tenant" code running? IMO that's a userland problem that can be easily solved with Queue::before().

Consideration:

1. I suggested Pulse::tag(...) over Pulse::categorise(...) because what if a single command loops over all tenants? If you set a tenant, that process should add a tag. If there is an "unset tenant" hook in their implementation, you should advise against Pulse::reset/clearTags(). In fact, perhaps those should be private.
2. I've worked on apps where "multi-tenancy" is considered an "instance", and then there is an optional second level of multi-tenancy. This is why I'd recommend tagging and either: segmenting/comparing stats by selected tags; or, filtering by tag/s. If pulse is more easily maintained by pre-aggregating by each tag (meaning filter by one tag only), then as a user I'd add two tags: Pulse::tag("instance:{$instance->id}") and Pulse::tag("instance:{$instance->id},{$tenant?->id ?? 'null'}").
3. Aren't we talking about "setting users"? That's applicable to only one of the three contexts. What about handling multi-tenancy for jobs and commands? Should Laravel support "hydrate user/s" and "hydrate request" for jobs too?

Hope these considerations/experiences/contexts help ✌️