-
Notifications
You must be signed in to change notification settings - Fork 283
[Feature][0.9][Merged] Allow login with username #257
Conversation
…l-Backpack/Base into allow-login-with-username
|
Let's sum it up. What does this PR do?It allows the developer to quickly switch from using // Username column for authentication
// The Backpack default is the same as the Laravel default (email)
// If you need to switch to username, you also need to create that column in your db
- 'authentication_column' => 'email',
+ 'authentication_column' => 'username',
- 'authentication_column_name' => trans('backpack::base.email_address'),
+ 'authentication_column_name' => 'Username',Result: The login will "just work". However: Problems:(1) the "password reset" screen does NOT ask for My thoughts on the problems aboveProblem (1). On the one hand, of course you plug in your email, that's where it will send the reset link, after all. On the other hand, it highlights that For problems (2) and (3)... I guess we could: I think solution B is better. But it makes a big assumption - that there is no required What to do?It's tough for me to figure this out, since I haven't needed to switch to Questions: |
|
Solution B. Password reset could go along the lines of: "Please enter your username. IF there is an associated e-mail, you will be sent one." And behind the scenes, either because of configuration, the fact there is an e-mail address or the correct coloured bantam was sacrificed, it would just work. |
… instead of email
…l-Backpack/Base into allow-login-with-username
|
@lloy0076 thank you for the feedback. Ok, so. I've fully implemented solution B for the This makes this implementation as good as it can be, from my point of view. It's Ready. However. I'm still not 100% that Backpack should support PROs:
CONs:
Again - this is mostly my code, that I've spent a reasonable amount of time developing. So I want to merge it. I just don't know if it's a good idea. I really need some help in deciding whether this is a feature to include, or not. What do you guys think? Then again, I might be overthinking this... It's been known to happen :-) Tutorial - to be included in the docsHow to login and register with
// Username column for authentication
// The Backpack default is the same as the Laravel default (email)
// If you need to switch to username, you also need to create that column in your db
'authentication_column' => 'username',
'authentication_column_name' => 'Username',That's it. |
|
Questions:
Thank you! |
|
I honestly doubt it's a niche use case; but then again, if I could verify that simply by stating it, I'd be a pollster and not a tech "guru" 😮 |
| $this->middleware('guest'); | ||
|
|
||
| if (!backpack_users_have_email()) { | ||
| abort(403, trans('backpack::base.no_email_column')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
403 feels wrong to me.
Personally I think it's one of the 500s or a view that states "Your super intelligent administrator must be contacted to do this."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. Changing it to 501 (not implemented).
src/helpers.php
Outdated
| */ | ||
| function backpack_avatar_url($user) | ||
| { | ||
| $placehold = 'https://placehold.it/160x160/00a65a/ffffff/&text='.$user->name[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
placeholder => english!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:-)) true. Changing!
| case 'gravatar': | ||
| return Gravatar::fallback('https://placehold.it/160x160/00a65a/ffffff/&text='.$user->name[0])->get($user->email); | ||
| if (backpack_users_have_email()) { | ||
| return Gravatar::fallback('https://placehold.it/160x160/00a65a/ffffff/&text='.$user->name[0])->get($user->email); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to escape the text= or eventually somehow something will go wrong.
https://groups.google.com/forum/#!topic/comp.mail.headers/MDZPvKXTb3w
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wondered the same thing one day, so tried a bunch of email addresses to break it, but wasn't able to. Apparently Gravatar pipes this through a pretty solid method buildUrl() that takes care of everything :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relying on outside services for security is likely to get us in trouble.
EquiFax could purchase the Gravatar guys, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be a treat :-) Gravatar itself doesn't do the escaping. The Laravel package that makes the call to them does the escaping. If we escape it before that package escapes it, we'd just be escaping it twice.
|
Merged into upgrade branch #259 . We'll leave this PR and branch open, and push fixes here if anything comes up in testing. |
|
Glad to see how you improved my PR!! :) Backpack permission package should be edited to allowing |
|
@tabacitu I also agree that this shouldn't be the default authentication. Sticking to login in with email but provide the option to change that to username based feels great. It's an option worth having. |


Merged PR #246 and made a few minor changes (variable naming, necessary view changes).
Warning: I'm not 100% this is a feature we need/want yet. But I think it's worth a shot. That's why I finished it.