spatie / laravel-activitylog

Log activity inside your Laravel app
https://docs.spatie.be/laravel-activitylog
MIT License
5.29k stars 713 forks source link

ActivityLog for Multi Auth guard #222

Closed slouma2000 closed 6 years ago

slouma2000 commented 7 years ago

In case you have Multi Auth (admin & web) with the 'web' guard as the default one. ActivityLog cannot detect the logged user (causer) because the guard is different and it cannot detect changes. Any idea to change the 'default_auth_driver' by the current guard of logged user

freekmurze commented 7 years ago

Could you PR a fix and a tests to demonstrate and solve the issue?

brunodevel commented 6 years ago

Hey @freekmurze , Is this feature already supported?

pavoltanuska commented 6 years ago

For me, the real problem here is, how to decide which guard to choose for which request. I can be authenticated for frontend and backend simultaneously, so both guards (Auth::guard('web') as well as Auth::guard('admin')) are not null.

Therefore I've created a simple ReplaceLogGuard Middleware, which is applied to the admin routes only.

public function handle($request, Closure $next, $guard = null)
{
    if (Auth::guard('admin')->check()) {        
        Config::set('activitylog.default_auth_driver', 'admin');
    }

    return $next($request);
}

I don't know if this workaround is "clean enough" and applicable to your use case, but I didn't find any shortcomings of this approach in my projects (yet).

slouma2000 commented 6 years ago

@pavoltanuska Thanks for reformulating the problem.

Gummibeer commented 6 years ago

@pavoltanuska Could be me but isn't it a general problem with your app that two guards (session based) could be active at the same time with different users!? How do you get the current user in other places? Do you everytime hardcode the guard at the place where you need the user? For me this sounds more like a problem with your auth process - yes it would be nice to change the guard on the fly but I think that it just should use the current active guard and you change your default/active one in the laravel auth/guard.

pavoltanuska commented 6 years ago

@Gummibeer you may be right, but is it really a problem to use two (or more) session based guards at the same time? Honest question, I really don't know and didn't come accross any problems with this approach yet (and I don't think of this specific issue as a problem, since I've found a solution).

Generally, I have two sets of "user" entities (separate DB tables, models, guards etc.) - User and Admin.

Do you everytime hardcode the guard at the place where you need the user?

Depends on what would you call "hardcoding". For frontend I use the default Laravel's guard (so no hardcoding here), for "backend" (admin panel, e.g. everything behind https://www.example.com/admin/* routes) I use a separate admins guard.

I've taken multiple approaches to this in the past:

If you would call these approaches as "hardcoding", then yes, I'm hardcoding the guard.

Would you prefer to implement an admin role (even if you don't use roles for anything else)? Or would you just add some kind of "is_admin" column to the users' DB table?

I'm using the same admin package in multiple projects, whether I have some kind of "normal" (session based) frontend or just an API for a SPA. And this approach allows me to change as little things as possible in each project's configuration etc.

What are your thoughts? (sorry for such a long comment, I'm honestly interested in a second opinion, whether my approach is correct or not)

Gummibeer commented 6 years ago

My first problem would be that you just can have one session as a user at the same time. So I have no idea how you've managed to differ the two sessions and user-pools. The next one: isn't an admin just a user with higher privileges!? https://laravel.com/docs/5.5/authorization

My problems/thoughts:

two user bases/tables put them in one table with a column is_admin or an extra roles or privilegs or whatever table. https://laravel.com/docs/5.5/authorization

two session based guards related to the two tables

Guards define how users are authenticated for each request. For example, Laravel ships with a session guard which maintains state using session storage and cookies. https://laravel.com/docs/5.5/authentication

define the guard everytime you use it it would be better to define the default guard in a middleware, route file or so to be able to use Auth::user() without a specific guard. http://mattallan.org/2016/setting-the-guard-per-route-in-laravel/

For me it feels wrong/dirty in all. How to solve it depends a lot on your situation, used, packages, time, needs and so on. Some ideas in the list. If you want to keep this database structure it would be an idea to use a subdomain or route group to define for all these another guard. If you keep two session guards in one app it should be save that they will never conflict each other - I never had this situation so these are just ideas:

The cleanest solution, in my opinion, would be to have one user table with all users/admins and one session guard. After this you can add a middleware admin and add a method on the user model isAdmin() or whatever you want. A second thing: in most cases it doesn't cost you much work but saves you a lot of time later to use Policies from the start and as modular as possible (create, update, list, show, delete for every model e.g.) and if you don't have a permission system atm just return hard true or $user-isAdmin() but if you want to implement a permission system later you just have to adjust the policies and don't touch all the controllers, form-requests and so on.

AlexVanderbist commented 6 years ago

We're using the same multi-auth approach @pavoltanuska in our CMS at spatie: github.com/spatie/blender. 2 tables, 2 guards, 2 sessions and that works out alright. (You can use the auth middleware with a guard parameter: https://laravel.com/docs/5.5/authentication#protecting-routes)

To get back to the main issue; @pavoltanuska ReplaceLogGuard middleware is a nice solution for now. A better solution might be to add a causedBy to the event logging code here: https://github.com/spatie/laravel-activitylog/blob/bded09c8f6c15c611b53a3e4be2944b6194f1a36/src/Traits/LogsActivity.php#L34-L38

Feel free to open a tested PR for this :)

ctf0 commented 3 years ago

a continue to @pavoltanuska solution, here is a one solution fits all

 public function handle(Request $request, Closure $next)
{
    $list = array_keys(config('auth.guards'));

    foreach ($list as $guard) {
        if (auth($guard)->check()) {
            config(['activitylog.default_auth_driver' => $guard]);
        }
    }

    return $next($request);
}

and append it to web group