[Feature][0.9][Merged] Custom auth guard #165

tabacitu · 2017-07-06T07:31:43Z

Merged this PR #141 into a branch, so we can work on it a little bit more.

I've already implemented all of @lloy0076 's comments regarding spacing, docblocks, etc.

…envanhee/Base into jorenvanhee-different-user-model-issue-fix

tabacitu · 2017-07-06T07:36:59Z

src/BaseServiceProvider.php

@@ -111,6 +113,11 @@ public function registerAdminMiddleware(Router $router)
        }
    }

+    public function registerCustomAuthGuard()
+    {
+        View::composer('backpack::*', \Backpack\Base\app\Http\ViewComposers\AuthComposer::class);


@jorenvanhee - I'm wondering if this is broad enough. Wouldn't this make the $backpackAuth guard available only in views that are loaded within the backpack namespace?

If so, I see the following problem:

developer uses $backpackAuth when he overwrites backpack views;

developer create supplementary views, that he considers to be part of the admin panel; he tries to use $backpackAuth there too; he expects $backpackAuth to be there too, but it's not;

I may be mistaken, though. It's been known to happen :-)

Makes sense. What do you think about a helper function backpackAuth()?

Hmm... I think a more Laravely way to go would be a Facade. Something like AdminAuth or BackpackAuth?

A more Laravely way would be to have both a Façade and a helper and then to have arguments on slack/gitter about which is best :(

I'm ok with having both a facade and a helper function. Just like the auth helper in Laravel. The documentation says: "The auth function returns an authenticator instance. You may use it instead of the Auth facade for convenience:". I'll update the code if you're ok with that :)

Having both is probably the better way, given that Laravel does that too. We might be able to avoid the arguments on slack/gitter bit though with a bit of luck (or just agreeing with me 👿).

tabacitu · 2017-07-06T07:38:03Z

src/config/backpack/base.php

@@ -68,11 +68,19 @@

    /*
    |--------------------------------------------------------------------------
-    | User Model
+    | Authentication
    |--------------------------------------------------------------------------
    */

    // Fully qualified namespace of the User model
    'user_model_fqn' => '\App\User',


@jorenvanhee , I think as long as we don't eliminate user_model_fqn at this time, this is a non-breaking change. Right?

It will not break if we keep user_model_fqn (and leave it in the RegisterController). But if you then want to use the option to switch authentication guards, you will have to republish your views. Because the new views will contain $backpackAuth or backpackAuth().

So it's not really breaking I guess? You just can't use the new feature straight away.

Right... that might be very confusing for someone who wants to start using the feature, but doesn't have the views, you're right. Let's call it a breaking change then.

jorenvanhee · 2017-07-12T09:48:01Z

src/app/Http/Controllers/Auth/ForgotPasswordController.php

@@ -29,7 +30,9 @@ class ForgotPasswordController extends Controller
     */
    public function __construct()
    {
-        $this->middleware('guest');
+        $guard = config('backpack.base.guard') ? config('backpack.base.guard') : config('auth.defaults.guard');


I would've kept it like this: config('backpack.base.guard') ?: config('auth.defaults.guard') (also at the other places with a similar situation)

Yup, that's PHP 7+ though, as far as I know. Just changed it for it to work on PHP 5.3 too :-) Still have people using Backpack on that one.

PHP 7 has the null coalescing operator ??. ?: works fine in php 5.3, it is also used in the Laravel source.

That is a textbook case for getting rid of the ternary and/or coalescing operators.

We probably should turn this into somethng readable.

Oh, ok :-) ?: looks clean enough to me then.

OwenMelbz · 2017-09-12T08:06:19Z

Can we make sure we address the issue of

Allowing separate user sessions e.g from different models - without having to specifically add your own custom middleware as this is the underlying issue users are facing - this example here was a deviation that doesn't solve the original issue

tabacitu · 2017-09-12T08:29:57Z

@OwenMelbz that's exactly what I was trying to figure out right now. Wouldn't using a different guard use a separate session?

OwenMelbz · 2017-09-12T08:53:28Z

Theoretically yeah - I think my previously solution meant backpack came shipped with its own guard. Then that allows the user to use the default laravel guard for the frontend.

This means the user has to do absolutely nothing except enable the functionality.

All the other solutions I've seen mean users have to create their own guards for the frontend and the backend?

jorenvanhee · 2017-09-12T10:00:44Z

I think Laravel stores the information about two different guards in one session. So for instance if you are already logged in with the User guard, and then log in with the Admin guard, it will just add that information to the same session. Doesn't really matter how it works behind the scenes, one session or two. It should work :) .

I'll make sure I make a test project with two guards.

Also, if the user of Backpack only wants to use one guard, not much has to be configured. Since it will just use the default guard then.

jorenvanhee · 2017-11-17T10:48:59Z

I would like to finish this PR. Could I get write access to this branch? Or should I make a PR to this PR, seems unnecessary.

lloy0076 · 2017-11-18T14:27:10Z

@jorenvanhee - I can't grant access to the branch but you could start work in a fork of that branch, if that makes sense?

…untController

tabacitu · 2018-03-05T11:36:31Z

Note to self:
I've just merged master into this branch, fixed the conflicts and tested a number of times. Works like a charm. Plus, the admin can now log out without logging out the regular user.

To do:

create BackpackAuth Facade to replace view composer;
create backpack_auth(), backpack_user() and backpack_guard_name() helpers;
remove the $backpackAuth view composer;
use the Facade above in all Backpack views, instead of $backpackAuth or Auth;
use the Facade above when getting the guard in all Backpack controllers;

Maybe:

create a BackpackUser model, that extends App\User with the modifs Base requires at installation;
by default, base.php could be configured to use the new model above as user_model_fqn; this would make it possible to install Backpack\Base without needing to modify stuff on the User model;
~~BaseServiceProvider could load a custom guard, provider and password, that the user can just use; possible names: backpack_guard, backpack_passwords;~~

tabacitu · 2018-03-05T14:01:49Z

Update: I don't think it makes much sense to create custom guards, providers and passwords by default. Hence, the task is now ~~strikethrough~~. The only people who would use them would be those who want separate logins/sessions for user and admins. And those users will likely need to modify them afterwards. So if we create some custom guards and providers we'd also need a way to easily customize them. This gets very unintuitive: you have your auth configuration split between config/auth.php, config/backpack.base.php and Backpack vendor files, which you'd need to overwrite/extend somehow. When you want to customize them, you'll pull your hair out. Laravel Authentication is notoriously complicated, so we shouldn't add to that.

Instead, we can specify clear steps on HOW TO USE SEPARATE LOGINS in the docs:

[STEP 1] In your config/auth.php:

1.1. Add a guard:

    'guards' => [
        // ...

        'admin' => [
            'driver' => 'session',
            'provider' => 'admins',
        ],

    ],

1.2. Add a provider:

    'providers' => [
       // ...

        'admins' => [
            'driver' => 'eloquent',
            'model' => Backpack\Base\app\Models\BackpackUser::class,
        ],

    ],

1.3. Add a password reset configuration:

    'passwords' => [
       // ...

        'admin' => [
            'provider' => 'admins',
            'table' => 'password_resets',
            'expire' => 60,
        ],

    ],

[STEP 2] In your config/backpack/base.php specify your new guard and password reset configuration:

    // The guard that protects the Backpack admin panel.
    // If null, the config.auth.defaults.guard value will be used.
    'guard' => 'admin',

    // The password reset configuration for Backpack.
    // If null, the config.auth.defaults.passwords value will be used.
    'passwords' => 'admin',

@OwenMelbz what do you think? Good solution? Basically instead of having a simple 'use_separate_sessions' => true variable like your PR had, if the developer wants separate sessions for users&admins they'd have to do a little more work, but it's dead simple stuff (the simple steps above). This has the benefit of

not imposing a guard, provider or password on them, by default;
having all auth configuration inside the config/auth.php file; not split between auth.php, base.php and vendor files;
making it dead-simple to change how the guard, provider or password work; they're in the config/auth.php file, like you'd expect;

I think it's a great compromise :-)

OwenMelbz · 2018-03-05T14:11:49Z

@tabacitu as this was such a long time ago lol I can't completely remember.

However I think it's overly simplified in the steps, was much more involved.

e.g If you do things like Auth::check() then it will say "yes your logged in" - however you're not actually logged into the admin, so you end up having to publish views, and extend/overwrite things.

I can't remember off the top of my head - but thats why the PR did more.

It might be that the original PR might need to be looked over - seeing what change was made - and see how that fits into your suggestion

iMokhles · 2018-03-05T15:51:22Z

i'm one of those like to separate sessions, routes and models etc
so i created my own MultiAuthCommand package which create and config everything for me on a fresh or exist projects i used it in my IMPanel which uses Backpack for Laravel
the only changes i did are
changing those files to add guard name
base/inc/menu.blade.php#L21
auth/account/sidemenu.blade.php#L3
base/inc/sidebar.blade.php#L1
( this could happens automatically through MultiAuthCommand if i added Backpack for Laravel integration in it)

also i have disable setup_auth_routes, setup_dashboard_routes, setup_my_account_routes
and replaced user_model_fqn with \App\Admin because routes and models created by MultiAuthCommand already.

if i integrate Backpack for Laravel in MultiAuthCommand will permits us to use it as Admin Panel and Backpack for Laravel features for any User Types

what do you think guys ?

tabacitu · 2018-03-06T06:28:23Z

Thank for the answer @OwenMelbz !

I actually studied PR #87 quite a bit, even resolved all conflicts with master. There were a lot of things I liked (like the helpers, the custom config for guard) so I copied them over to this branch. And some things I didn't quite like (a lot of conditionals, using a different middleware for separate sessions) so I didn't copy those. It's subjective, of course, but I think we ended up with the best of both worlds :-)

Ok, great. If you're saying the steps required with that PR to make separate sessions work were more than just changing separate_admin_session, then the steps above will do just fine. Simple stuff. Just added them to the new docs (unreleased yet).

PR ready! :-)

tabacitu · 2018-03-06T06:35:58Z

Hmm... @iMokhles what do you mean by

if i integrate Backpack for Laravel in MultiAuthCommand will permits us to use it as Admin Panel and Backpack for Laravel features for any User Types

Could you please rephrase?

PS. Great work on IMPanel and MultiAuthCommand. I imagine you created them because in new projects you want your opinionated setup, not the Backpack default? This is something I imagined quite a lot of people would do - create Laravel instances with their opinionated setup. Like the Demo, but for their own personal use. Basically a barebone inhouse CMS, that they reuse for all projects, that is powered by Backpack. Let us know some other stuff you included there, by creating Github issues, if you think we could make those the Backpack default.

iMokhles · 2018-03-06T12:46:12Z

i was meaning

by calling this php artisan make:multi-auth Admin from MultiAuthCommand will generate everything for the Admin guard ( Models, Controllers, Views, Migrations, Routes, Notifications, etc ) and all of those for now based on the original setup by laravel when we run php artisan make:auth

but
when adding Backpack for Laravel support in MultiAuthCommand we can call something like that
php artisan make:backpack-multi-auth Admin it will generate and setup everything like the original command but for the admin guard and with Backpack for laravel ( Models, Controllers, Views, Migrations, Routes, Notifications, etc )

we can call the same command with different guard php artisan make:backpack-multi-auth User
to get the same features for this guard

so we get Admin Panel & User Panel ( or create Panel for each guard we need )

and about IMPanel it has features like Statistics builder, (Personal)Analytics tracker which has it's own database tables ( not through Google Analytics ), and i added those pull requests Laravel-Backpack/Settings#67
#233
in it
with the SideMenuSection/Item Manager which i really would like to see as Backpack Package
also it support DB Notifications which i guess will be awesome if it get added to Backpack also
and improved the https://github.com/Laravel-Backpack/LangFileManager to support localization for Backpack packages files and almost everything under lang/vendor

lloy0076 · 2018-03-06T13:11:43Z

src/app/Models/BackpackUser.php

+    {
+        return (new MailMessage())
+            ->line([
+                'You are receiving this email because we received a password reset request for your account.',


lloy0076 · 2018-03-06T13:11:49Z

src/app/Models/BackpackUser.php

+                'You are receiving this email because we received a password reset request for your account.',
+                'Click the button below to reset your password:',
+            ])
+            ->action('Reset Password', url(config('backpack.base.route_prefix').'/password/reset', $this->token))


lloy0076 · 2018-03-06T13:11:54Z

src/app/Models/BackpackUser.php

+                'Click the button below to reset your password:',
+            ])
+            ->action('Reset Password', url(config('backpack.base.route_prefix').'/password/reset', $this->token))
+            ->line('If you did not request a password reset, no further action is required.');


lloy0076 · 2018-03-06T13:13:40Z

src/helpers.php

+     * Returns the name of the guard defined
+     * by the application config
+     */
+    function backpack_guard_name()


I wonder if these should be camelCase?

Hmm... Good point. I vote no. From what I can see, all Laravel helpers are snake_case. I think we should keep that same rule, for consistency.

sigh Standards only exist so they can be broken 🤦‍♂️

No they don't. The also exist to cover all existing standards :-) https://xkcd.com/927/

…github.com/Laravel-Backpack/Base into jorenvanhee-different-user-model-issue-fix

tabacitu · 2018-03-08T11:08:24Z

Merged into upgrade branch #259 . We'll leave this PR and branch open, and push fixes here if anything comes up in testing.

jorenvanhee and others added 6 commits May 2, 2017 12:08

Make it possible to switch guards

ccbf8fb

Use correct guard in views

a592571

Fix order of use statements

2fd3f86

Make the admin middleware use the backpack guard

08b9390

Merge branch 'different-user-model-issue-fix' of git://github.com/jor…

feac647

…envanhee/Base into jorenvanhee-different-user-model-issue-fix

fixed lloyd's review objections in implementing custom auth guard

87839c9

tabacitu added the working on it label Jul 6, 2017

tabacitu commented Jul 6, 2017

View reviewed changes

tabacitu mentioned this pull request Jul 6, 2017

Add option to switch authentication guards #141

Closed

tabacitu self-assigned this Jul 6, 2017

jorenvanhee reviewed Jul 12, 2017

View reviewed changes

tabacitu changed the base branch from master to upgrade September 11, 2017 14:02

tabacitu changed the base branch from upgrade to master March 5, 2018 10:37

tabacitu added 3 commits March 5, 2018 12:43

merged master and fixed conflicts:

2813799

cleaner config helper usage for guards and passwords

37b49e1

cleaned up admin middleware

107c1f3

lloy0076 approved these changes Mar 5, 2018

View reviewed changes

fixed logout logging out all sessions; applied custom guard to MyAcco…

a5a8015

…untController

tabacitu added 2 commits March 5, 2018 14:59

added backpack_auth helpers and removed view composers

2e1eea2

Apply fixes from StyleCI

83758c6

tabacitu changed the title ~~Custom auth guard~~ [Feature][0.9][WIP] Custom auth guard Mar 5, 2018

cleanup

edb3e52

tabacitu added 3 commits March 5, 2018 15:14

comment method

5cd6190

added BackpackUser model and made it the default user_model_fqn

353d230

Apply fixes from StyleCI

5b50727

tabacitu changed the title ~~[Feature][0.9][WIP] Custom auth guard~~ [Feature][0.9][Ready] Custom auth guard Mar 5, 2018

tabacitu added ready and removed working on it labels Mar 5, 2018

This was referenced Mar 6, 2018

[Enhancement] Allows users and admins to have separate sessions #87

Closed

Avoid calling Auth multiple times. #237

Closed

Rename the Backpack Admin middleware to BackpackAdmin #67

Closed

[Feature][0.9][Merged] Custom admin middleware #258

Merged

lloy0076 reviewed Mar 6, 2018

View reviewed changes

tabacitu added 2 commits March 8, 2018 12:34

translated reset password email

b153b4e

Merge branch 'jorenvanhee-different-user-model-issue-fix' of https://…

bb99618

…github.com/Laravel-Backpack/Base into jorenvanhee-different-user-model-issue-fix

tabacitu mentioned this pull request Mar 8, 2018

[Release][WIP] Base 0.9 Upgrade #259

Merged

7 tasks

tabacitu changed the title ~~[Feature][0.9][Ready] Custom auth guard~~ [Feature][0.9][Merged] Custom auth guard Mar 8, 2018

tabacitu merged commit bb99618 into master Mar 22, 2018

tabacitu removed the ready label Mar 22, 2018

[Feature][0.9][Merged] Custom auth guard #165

[Feature][0.9][Merged] Custom auth guard #165

Conversation

tabacitu commented Jul 6, 2017

tabacitu Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lloy0076 Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorenvanhee Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lloy0076 Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OwenMelbz commented Sep 12, 2017

tabacitu commented Sep 12, 2017

OwenMelbz commented Sep 12, 2017

jorenvanhee commented Sep 12, 2017 • edited Loading

jorenvanhee commented Nov 17, 2017

lloy0076 commented Nov 18, 2017

tabacitu commented Mar 5, 2018 • edited Loading

tabacitu commented Mar 5, 2018

OwenMelbz commented Mar 5, 2018

iMokhles commented Mar 5, 2018

tabacitu commented Mar 6, 2018

tabacitu commented Mar 6, 2018

iMokhles commented Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tabacitu commented Mar 8, 2018

tabacitu Jul 6, 2017 •

edited

Loading

lloy0076 Aug 14, 2017 •

edited

Loading

jorenvanhee Jul 12, 2017 •

edited

Loading

lloy0076 Aug 14, 2017 •

edited

Loading

jorenvanhee commented Sep 12, 2017 •

edited

Loading

tabacitu commented Mar 5, 2018 •

edited

Loading

iMokhles commented Mar 6, 2018 •

edited

Loading