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

Conversation

jorenvanhee
Copy link
Contributor

@jorenvanhee jorenvanhee commented May 2, 2017

This fixes #122.

I basically made it possible to switch authentication guards for backpack. This makes it possible to use a different guard besides the default one (from config/auth.php).

For instance, you have a guard for the users on the public facing part of the website. And you have a different guard for the admins of the website. So a user guard and an admin guard. If the user guard is the default one, then it was impossible to use the admin guard with backpack. This pull request makes it possible to set a specific guard for backpack.

There was already a config option user_model_fqn in backpack, but this only changed the behaviour of the user registration. More info about that in issue #122.

This should be a non-breaking change because the user_model_fqn config option is still present and being used. We could remove it and fetch the used model from the guard configuration, but that would be a breaking change.

This is a breaking change because the views are different now, and they have to be republished. Since it is a breaking change, I will also look into removing the user_model_fqn option.

Actually, the views only need to be republished, if you use the new guard option. The old views just use the default guard. So I'm not sure if this is breaking or not...

@jorenvanhee jorenvanhee changed the title Different user model issue fix Make it possible to switch authentication guards, fixes #122 May 2, 2017
@jorenvanhee jorenvanhee changed the title Make it possible to switch authentication guards, fixes #122 Add option to switch authentication guards, fixes #122 May 2, 2017
@jorenvanhee jorenvanhee changed the title Add option to switch authentication guards, fixes #122 Add option to switch authentication guard May 2, 2017
@jorenvanhee jorenvanhee changed the title Add option to switch authentication guard Add option to switch authentication guards May 2, 2017
@soham2008xyz
Copy link

Have you seen #87? Exactly similar to our use case here, hasn't been merged yet.

@lloy0076
Copy link
Contributor

lloy0076 commented Jun 4, 2017

@soham2008xyz @jorenvanhee - it seems that #87 is similar but not quite the same although I do agree that they are probably addressing the same issue.

I think the main point of difference is that 87 includes the guard whereas this one doesn't.

public function broker()
{
$passwords = config('backpack.base.passwords')
?: config('auth.defaults.passwords');
Copy link
Contributor

Choose a reason for hiding this comment

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

NL?

{
$this->middleware('guest');
$guard = config('backpack.base.guard')
?: config('auth.defaults.guard');
Copy link
Contributor

Choose a reason for hiding this comment

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

NL?

return redirect($this->redirectPath());
}

protected function guard()
Copy link
Contributor

Choose a reason for hiding this comment

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

PHP Doc?

protected function guard()
{
$guard = config('backpack.base.guard')
?: config('auth.defaults.guard');
Copy link
Contributor

Choose a reason for hiding this comment

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

NL?

{
$this->middleware('guest');
$guard = config('backpack.base.guard')
?: config('auth.defaults.guard');
Copy link
Contributor

Choose a reason for hiding this comment

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

NL?

protected function guard()
{
$guard = config('backpack.base.guard')
?: config('auth.defaults.guard');
Copy link
Contributor

Choose a reason for hiding this comment

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

NL?

public function handle($request, Closure $next)
{
$guard = config('backpack.base.guard')
?: config('auth.defaults.guard');
Copy link
Contributor

Choose a reason for hiding this comment

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

NL?


class AuthComposer
{
public function compose(View $view)
Copy link
Contributor

Choose a reason for hiding this comment

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

PHP Doc?

public function compose(View $view)
{
$guard = config('backpack.base.guard')
?: config('auth.defaults.guard');
Copy link
Contributor

Choose a reason for hiding this comment

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

NL?

<!-- <li><a href="{{ url('/') }}"><i class="fa fa-home"></i> <span>Home</span></a></li> -->

@if (Auth::guest())
@if ($backpackAuth->guest())
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did $backpackAuth come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the AuthComposer.

@jorenvanhee
Copy link
Contributor Author

Is there something more that I can do to get this PR merged?

If this one gets merged, I will also have to update the authorize method in the CrudRequest class from Backpack/CRUD.

@OwenMelbz
Copy link
Contributor

@jorenvanhee this looks like a duplicate of #87 which has had some discussion over.

@tabacitu
Copy link
Member

tabacitu commented Jul 6, 2017

@jorenvanhee - I'm sorry, I've been hacking away at client projects and preparing something super exciting for the Backpack community that will be revealed soon, so I haven't merged PRs for a while. But I'm back now!

@OwenMelbz - is this a complete replacement for #87? Cause if it is, I think I like it a bit better, and it would be a non-breaking change, so it's ready to test and merge. I especially like that:

  • it avoids using a helper;
  • the guard usage has better readability
  • @if ($backpackAuth->guest()) over @if (!backpack_admin()), because it allows you to call ->check() and other methods too;
  • the configuration file seems easier to understand to me (just replace the Auth middleware if you need to);
  • it falls back to configuration options from Laravel itself (config/auth.php);

Honestly, I think "pretty awesome" is the word for this PR :-)
Really nice job @jorenvanhee :-)

@Laravel-Backpack Laravel-Backpack deleted a comment from lloy0076 Jul 6, 2017
@Laravel-Backpack Laravel-Backpack deleted a comment from lloy0076 Jul 6, 2017
@Laravel-Backpack Laravel-Backpack deleted a comment from lloy0076 Jul 6, 2017
@Laravel-Backpack Laravel-Backpack deleted a comment from lloy0076 Jul 6, 2017
@Laravel-Backpack Laravel-Backpack deleted a comment from lloy0076 Jul 6, 2017
@tabacitu
Copy link
Member

tabacitu commented Jul 6, 2017

@jorenvanhee , @lloy0076 - I've merged this into a separate branch and done @lloy0076 's suggestions: #165

Let's move the conversation there, please.

@tabacitu
Copy link
Member

Closing this - let's move the conversation there.

@tabacitu tabacitu closed this Sep 12, 2017
@OwenMelbz
Copy link
Contributor

@tabacitu just this asking if its a replacement for the other PR - answer is no lol, it does not address the issues we face.

Regards to the comments of

  • it avoids using a helper;
  • the guard usage has better readability
  • @if ($backpackAuth->guest()) over @if (!backpack_admin()), because it allows you to call ->check() and other methods too;
  • the configuration file seems easier to understand to me (just replace the Auth middleware if you need to);
  • it falls back to configuration options from Laravel itself (config/auth.php);

Responses being

  • We've just introduced helpers - so that argument I assume is now void?
  • Not sure this one is entirely true :D even adds another dependency of passing data to the view which means more files will need to be updated for the user
  • The helper was designed to get the user instance - not query the guard - you can still do that
  • Again the issue was not that we want a custom guard - the issue is separation of sessions - you cannot be logged into only 1 without customly writing this yourself which adds annoying overheads.
  • The user can use the default guard if they want

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

user_model_fqn used in Register, but not in Login

5 participants