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

Conversation

@tabacitu
Copy link
Member

@tabacitu tabacitu commented Mar 6, 2018

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.

@tabacitu
Copy link
Member Author

tabacitu commented Mar 6, 2018

Let's sum it up.

What does this PR do?

It allows the developer to quickly switch from using email for login to using username or any other column, just by changing some variables in config/backpack/base.php:

    // 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:

screen shot 2018-03-06 at 09 32 39

The login will "just work". However:

Problems:

(1) the "password reset" screen does NOT ask for username; it will still ask for email;
(2) the "my account" screen does NOT allow the admin to change his username; only name and email;
(3) the "register" screen does NOT show that custom column; only name, email, password and confirm_password;

My thoughts on the problems above

Problem (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 email would still be a required column in the DB. Which is not the case for most systems that need username, I think. If you have email and it's unique, why would you place the burden of remembering a username on the user? BIG QUESTION: Do developers who need username actually have an email column?!

For problems (2) and (3)... I guess we could:
(A) "magically" add the specified column to the "my account" and "register" forms. With the required, string and unique validation.
or
(B) change the "my account" and "register" forms to use username instead of email, just like the login form does; we'd also disable the "forgoten password" functionality;

I think solution B is better. But it makes a big assumption - that there is no required email column in the db.

What to do?

It's tough for me to figure this out, since I haven't needed to switch to username. Ever. I can only rely on you guys (and intuition) on what to do. @kichetof , @iMokhles , @lloy0076 , @niladam what do you think?

Questions:
[Q1] How many people need username in their admin panels? Please speak up. Is this really a feature we should include in Backpack\Base, or should we just create a tutorial for how to achieve it?
[Q2] Developers that need username - do you have an email column in your db? This is very important, because we can figure out whether to use solution A or B above.

@lloy0076
Copy link
Contributor

lloy0076 commented Mar 6, 2018

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.

@tabacitu tabacitu changed the title [Feature][0.9][WIP] Allow login with username [Feature][0.9][Ready] Allow login with username Mar 7, 2018
@tabacitu
Copy link
Member Author

tabacitu commented Mar 7, 2018

@lloy0076 thank you for the feedback.

Ok, so. I've fully implemented solution B for the username situation, but I haven't changed the password_reset procedure to use username instead of email. Because (1) people who forget their password also forget their username (rarely their email though) and (2) It makes no sense to change the working procedure & error texts in all languages, for something that still uses email, after all. So I've just disabled the password_reset functionality if the email column is not present on the users table.

This makes this implementation as good as it can be, from my point of view. It's Ready.

screen recording 2018-03-07 at 09 24 am

However. I'm still not 100% that Backpack should support username out-of-the-box. Keep in mind that I've worked quite a bit on this - the work has already been done. I'm just wondering if it's the right move to merge it, from Backpack's perspective. The way I see it:

PROs:

  • developers who need admin panels with username have a quick way of building that;

CONs:

  • in order to make username an option, we've had to modify quite a few stuff in Backpack; so we now have a bunch of conditionals everywhere, for something that might be a niche use case; I wouldn't quite say that it's bloat; but that's how good code turns into bloat - little by little;
  • if we provide out-of-the-box username support, we'll need to maintain username support; for ever and ever;
  • all Backpack packages that deal with Users will be forced to support this new feature; current packages and future packages;
  • as-is, PermissionManager will break for those who use username; by default it allows admins to edit email, not username; we can provide support, but should we automagically allow them to edit the email column too, if present? nasty;
  • username in authentication is something that Laravel itself only provides partial support for; the docs instruct you to do one thing and it will "just work", but it doesn't; you still have to modify at least 3 more blade files and 2 more controllers to make Register and Forgotten Password to work; so it's a custom feature for Laravel; not a supported out-of-the-box feature;

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 docs

How to login and register with username instead of email, when/if this PR gets merged:

  1. Create a username column in your users table and add it in $fillable on your User model. Best to do this with a migration.
  2. Delete your email column and remove it from $fillable on your User model. Alternatively, just remove UNIQUE and NOT NULL from it. Best to do this with a migration.
  3. Change your config/backpack/base.php config options:
    // 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.

@tabacitu
Copy link
Member Author

tabacitu commented Mar 7, 2018

Questions:

  • Do you think this is bloat?
  • Do you think this is a feature we should provide out-of-the-box?

Thank you!

@lloy0076
Copy link
Contributor

lloy0076 commented Mar 7, 2018

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'));
Copy link
Contributor

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."

Copy link
Member Author

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];
Copy link
Contributor

Choose a reason for hiding this comment

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

placeholder => english!

Copy link
Member Author

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);
Copy link
Contributor

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

Copy link
Member Author

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 :-)

Copy link
Contributor

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.

Copy link
Member Author

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.

@tabacitu tabacitu mentioned this pull request Mar 8, 2018
7 tasks
@tabacitu
Copy link
Member Author

tabacitu commented Mar 8, 2018

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

@tabacitu tabacitu changed the title [Feature][0.9][Ready] Allow login with username [Feature][0.9][Merged] Allow login with username Mar 8, 2018
@kichetof
Copy link
Contributor

Glad to see how you improved my PR!! :)
Everything works fine for me! And as you said, Backpack offer a great solution out of the box and if you want to customize it with any other login field, feel free :)

Backpack permission package should be edited to allowing authentication_column instead of default email value.
That the only last thing that I need to do to have Backpack setting up with Active Directory support with samAccountName field instead of email.

@niladam
Copy link

niladam commented Mar 16, 2018

@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.

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.

4 participants