Skip to content
This repository was archived by the owner on Nov 23, 2020. It is now read-only.

Conversation

OwenMelbz
Copy link
Contributor

This PR does 2 things.

  1. It renames the generic admin middleware to backpack.base.admin

  2. It adds a guard, additional middleware (which can be customised), and config items

This means that if you set 'separate_admin_session' => true in the config, users and admins can have separate sessions, so they can be logged in as a different user on the frontend to the admin.

@OwenMelbz
Copy link
Contributor Author

@tabacitu boop :D 2 people today asking in gitter how they can have this functionality :P

@OwenMelbz
Copy link
Contributor Author

3 people :D

@OwenMelbz
Copy link
Contributor Author

OwenMelbz commented Jan 25, 2017

@tabacitu

(Read my other recent comment first)

But same applies here, I think this is an important update to make it flexible enough for most use cases :) definitely worth making it into 3.2

@tabacitu
Copy link
Member

Hi @OwenMelbz ,

I agree this is urgent and needs to be addressed. But whenever I see a modif that consists of a bunch of if/else, I'm wondering if there isn't a cleaner way to achieve the same thing.

About storing sessions separately for admins and users - I'm not sure this should be a core feature. Out of >15 projects I used Backpack on, I haven't needed separate sessions once. And it sounds like questionable UX too - I'd definitely be confused if I were logged in as 2 users at the same time. Perhaps an "impersonate" feature would make more sense, like Spark has? Or maybe we just make it a lot easier for people who do need this functionality to implement it themselves?

(A) How many times did you need these separate sessions?

What if:

  • we scrap the old "admin" middleware;
  • we publish a new middleware, "CheckIfAdmin"/"VerifyAdmin", and use that everywhere instead of "admin"; wouldn't create a helper for it;
  • we publish empty controllers for everything else (Auth, Login, ResetPassword, etc), that extend the ones in Backpack, so that they would be easier to overwrite if you need separate admin sessions;
  • we create a tutorial for how to achieve separate sessions, what to modify inside those controllers;

The result:

  • for people who don't need separate sessions:
    • no more if/else in >10 methods of the controllers;
    • easy to understand code;
  • for people who DO need the separate sessions:
    • easy to achieve, just follow the tutorial;
    • no more if/else in >10 methods of the controllers (you load directly the middleware and guards you need);

(B) Good workaround? What do you think?

Background: I keep reading and being warned by smart people that "what you don't include in your product is just as important as what you do include". Knowing there's no way back if we do include something, I'm really scared to not introduce too many niche features. I think there's a fine line between doing too little (Basecamp) and doing too much (iTunes). That line seems very very hard to find :-) If big companies struggle to find it... It won't be easy for us either... :-)

@OwenMelbz
Copy link
Contributor Author

Hmm, as it happens EVERY (5) project I've used has needed this separation, and have seen many users asking how to do it in github and in gitter.

I think the current solution is fine for brochure sites, how any application that requires tailored access it becomes problematic.

For example we had a game which had admins who would setup levels etc, however to be able to visit the frontend they would need to be assigned to games/teams/seasons. It would also contribute towards negative statistics as random admin users would be listed towards averages etc.

Another example would be a learning portal, where again users would need to have years/packages etc assigned to them which would completely tailor the frontend, which means again admins couldn't use their session as they don't have any of the correct data.

It may be that it happens to be that all our projects are much more complex than other typical use cases, but it has become laborious pulling out all the backpack controllers each time and setting up the guards, which in tern has made the package harder to keep up to date as we have to manually patch some bits on updates because we've had to pull bits into the App namespace.

Also as you know there are many new comers to laravel that simply don't know what they're doing enough to be able to separate this themselves, which makes it harder.

Also the recent article I linked to you criticising having to hack in functionality is a good example of making it more flexible for more advance projects, after all one of the many reasons to use backpack is to save users time, and having them scaffold out an auth/driver/guard system every time is a chore.

Those are my reasonings for thinking this should be included by default.

HOWEVER, going back to your actual post.

  1. I think if users are confused that they can be logged into the front/backend - they need more training from their company about the product they've picked, and why its a requirement. Baring in mind, the admin functionality is for internal use - not the public.

  2. Impersonate would be a good feature as a whole, but it does not address the actual problem thats trying to be solved.

  3. Publishing middleware I'm not sure will do much, apart form let users adjust the existing ones?

  4. Publishing a intermediate controller would help, as you can then overwrite the middleware, however you then introduce the issue of people overwriting stuff thats needed in future updates, maybe something like a pseudo method like the setup which proxies down to construct you could to something like setMiddleware which users only set the middleware in, which means you can still modify the core package one.

However, you then still have the issue of users having to do things like Auth::guard('custom_guard')->user() etc - which will be trickier again for new users.

Maybe we just need a poll :D do users want the feature or not :D

@OwenMelbz
Copy link
Contributor Author

Just had another person asking how to do this :D as they were migrating an existing project with frontend users, who cannot have admin sessions :P

@OwenMelbz
Copy link
Contributor Author

I've just refactored a little of this, so it seems a bit cleaner, which might help convince you to merge it :D

@mariavilaro
Copy link

I need this so much! Do you think it will be merged any time soon?

@xbelmondo
Copy link

Almost all my projects needed separate admin/user sessions...

@AbbyJanke
Copy link
Contributor

Any update on when this might get merged in?

@tabacitu
Copy link
Member

tabacitu commented Apr 6, 2017

@OwenMelbz - I think I have an alternative solution for this, which would be a non-breaking change and I'm happy to include it this/next week. Please tell me what you think, and if you see any possible problems. The idea is to include this in the backpack/base.php config file:

    /*
    |--------------------------------------------------------------------------
    | Backpack Middleware
    |--------------------------------------------------------------------------
    |
    | This middleware will be used by all Backpack package that require login.
    |
    | By default the \Backpack\Base\app\Http\Middleware\Admin middleware will be used
    | which is just a soft wrapper around the Auth middleware, that makes sure the redirects
    | use the admin prefix.
    |
    */

    'register_middleware' => true,
    'middleware_name' => 'admin',
    'middleware_class' => \Backpack\Base\app\Http\Middleware\Admin::class,

This would allow you to easily change the middleware that all Backpack packages use, or just its name, or not register it altogethere, without causing a breaking change for current users. Correct?

What do you think?

Cheers!

@OwenMelbz
Copy link
Contributor Author

Erm, tbh until I see that in action, I'm not really sure how it would work.

Would it mean that users would then need to create 2 of their own middleware and guards to be able to do it get to the desired outcome?

@AbbyJanke
Copy link
Contributor

What about instead of two separate sessions, in the BaseServiceProvider we add an IF statement for a new config setting "dashboard middleware." I personally don't specifically need two sessions, but just a way to limit access to normal users.

So if the config is false (by default) it skips the middleware, then if it has a middleware defined (enter the namespace path) it runs the middleware?

Could also make the IF statement have an isset statement first to avoid any breaks?

@OwenMelbz
Copy link
Contributor Author

@AbbyJanke that doesn't solve the issue this is trying to fix 😄 You can already use permission systems to reject users from accessing this.

This issue is about allowing the admin users to be completely different from frontend users

@mpixelz
Copy link

mpixelz commented May 1, 2017

Hi,
i just read this thread.. ive been testing few other systems like spark and laravel-boilerplate.. both are using impersonate function and i think thats how it should be.. for instance.. we have learning management system where students cannot access the backend and they have their own frontend portal.. but in order to develop and test student side of things.. we use 2 different browsers to see both in action.. so if user does one thing.. admin side get the appropriate action and we can see them without having to logout and login with that user permissions.
what do you think?

@OwenMelbz
Copy link
Contributor Author

@mPixeL - that's not what this PR is trying to solve. Sure having impersonate would useful in the future.

This is more aimed at when admin users are completely disconnected from frontend users.

Imagine backpack is powering an iOS/Android game. You can't have admins trying to login to the frontend as they don't have any of the required fields to play it.

Or if your website users NEEDED to have subscriptions or "groups" but your admins don't.

This allows you to have completely separate logic for users and admins making it more scalable. Imagine having a leaderboard. You'd need to have "if not admin" checks everywhere to not include admins etc

So impersonate isn't solving the actual issue here

{
if (config('backpack.base.separate_admin_session')) {
if (!\Auth::guard(config('backpack.base.admin_guard.name'))->check()) {
if ($request->ajax() || $request->wantsJson()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not: $request->expectsJson() unless we don't like its pjson clause.

That said, why couldn't someone access admin functionality via JSON?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... this might be an issue. @OwenMelbz wouldn't this prevent ajax calls to pages that are behind the backpack_middleware(), if the new guard is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only applies is they are using separate sessions - if they're using the default laravel guard it will never affect it as that will have already handled the permission - baring in mind this whole implementation was based around the idea of using the default laravel guard for everything - then if turned on it simply adds another condition into the chain for the backend

src/helpers.php Outdated
$middleware = $chainedWith;
}

return $middlware;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no e in the middle of $middleware. Errk.

$guard = null;
}

return $guard;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return config('backpack.base.separate_admin_session', null); seems more succinct.

@OliverZiegler
Copy link
Contributor

Hi there. I'm currently in the progress of developing a small artisan package to provide this functionality. It currently provides functionality for basic laravel multiauth, for role-auth with spate/laravel-permission and multiauth in backpack.
For backpack the package sets everything up with one command specifying the new "Admin-Model" to use.
It also provides overridable controllers for register, login, forgot password that inherit from the base laravel-backpack controllers.

You can find it here.
I think for many use-cases this will solve the issue of multi-auth. And as the extension of the Backpack-Controllers is quite minimal, the issues with updates should be quite small.

Love to here if this helps someone.

@fabiodiniz
Copy link

Any idea about when this PR will be merged?

@Laravel-Backpack Laravel-Backpack deleted a comment from lloy0076 Sep 12, 2017
@tabacitu tabacitu changed the base branch from master to upgrade September 12, 2017 08:39
@tabacitu
Copy link
Member

@OwenMelbz I've been on this issue for more than 10 hours now and haven't figured it out. Maybe you can provide some insight.

For the life of me, I can't get users & admins on separate sessions, using this PR. I considered I might have broken it so I tried an old version - still nothing. When an admin is logged in, so is the user. Any idea why? Did you do anything else other than this PR to get that result?

Thanks!

@OwenMelbz
Copy link
Contributor Author

@tabacitu ha definitely worked when I left it, were using it on couple of projects.

Might need more info on what's been done, possibly need a screen sharing session?

@OwenMelbz
Copy link
Contributor Author

Guess also need to consider that other PRs have conflicted if they've changed/renamed some bits

@voidstate
Copy link

voidstate commented Dec 13, 2017

@tabacitu @OwenMelbz Any move towards resolving this? here seems to be an impasse between @tabacitu not managing to get it working, and @OwenMelbz needing more info.

@tabacitu tabacitu changed the base branch from upgrade to master March 5, 2018 09:34
@tabacitu
Copy link
Member

tabacitu commented Mar 6, 2018

Closing this in favour of #165 - which is a complete replacement now. Thanks a lot for this @OwenMelbz - it looks like a lot of work, and I reused A LOT from this.

@tabacitu
Copy link
Member

@OwenMelbz , it took me a while... but I now agree with you on this one :-)

I think it’s better if we completely separate Backpack login from the scaffolded Laravel login. Separate sessions, separate auth guard, separate password broker, separate auth provider. I think it makes more sense that way - easier to understand there are two separate logins (user and admin), which share nothing but the users table in the database.

I’ve done so in this PR - #293 - let me know what you think, if you have the time.

Thanks again for the improvements - sorry it took so long to agree with you :-)

@tabacitu tabacitu deleted the separate-sessions branch October 18, 2018 10:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants