Laravel-Backpack / community-forum

A workspace to discuss improvement and feature ideas, before they're actually implemented.
28 stars 0 forks source link

[Refactoring] Using the ```Hash::make()``` instead of ```bcrypt()``` helper #182

Open simaremare opened 2 years ago

simaremare commented 2 years ago

Feature Request

By default, Backpack uses the bcrypt() helper to generate the hashed password, I think this is an issue to flexibility since we can change the default bcrypt algorithm into something else in the config/hashing.php. When changed to argon2id, for example, Backpack breaks.

What's the feature you think Backpack should have?

Backpack should use the Illuminate\Support\Facades\Hash::make().

Have you already implemented a prototype solution, for your own project?

I replace all the bcrypt() to Hash::make().

Do you see this as a core feature or an add-on?

The use of Hash::make() should be by default to promote higher flexibility.

welcome[bot] commented 2 years ago

Hello there! Thanks for opening your first issue on this repo!

Just a heads-up: Here at Backpack we use Github Issues only for tracking bugs. Talk about new features is also acceptable. This helps a lot in keeping our focus on improving Backpack. If you issue is not a bug/feature, please help us out by closing the issue yourself and posting in the appropriate medium (see below). If you're not sure where it fits, it's ok, a community member will probably reply to help you with that.

Backpack communication channels:

Please keep in mind Backpack offers no official / paid support. Whatever help you receive here, on Gitter, Slack or Stackoverflow is thanks to our awesome awesome community members, who give up some of their time to help their peers. If you want to join our community, just start pitching in. We take pride in being a welcoming bunch.

Thank you!

-- Justin Case The Backpack Robot

pxpm commented 2 years ago

Hello @simaremare

Totally agree with you. I've tagged it with should, so it's almost guaranteed that we are going to do this change in 4.2!

Thanks for the report and submitting your issue.

In the meantime, if you want to submit the PR we will review it and merge in 4.2 and you would show up as contributor, since you already did the change in your project.

Thanks, Pedro

tabacitu commented 2 years ago

Totally agree. Thanks @simaremare . We should consider this a potentially breaking change though, we don't know if people have changed stuff in their config/hashing.php but expect Backpack to ignore it...

Also (for future us)... we should maybe consider adding a config option of our own, in config/backpack/base.php, that defaults to the one in config/hashing.php. That way, the people who are in that situation (if any) would still be able to achieve their goal.

Thanks again @simaremare !

ziming commented 1 year ago

Thought I share something I just found out today.

So, I have a task of trying to import users from internal App A to internal App B. App A is using argon2id, App B is using bcrypt

Initially I thought I am going to need to get everyone from App A to reset their passwords in App B, but to my surprise, I tried to login to App B with my account credentials in App A and it worked!

In the users table, that user record password is the argon2id hash (not the bcrypt hash), turns out laravel is smart enough to try both bcrypt and argon when verifying if a password is correct on authentication. So users are able to login regardless of if the password hash on their record is bcrypt or argon2id

Then I go edit profile to update my password in App B, check the database, and it is now a bcrypt hash since App B config/hashing.php set bcrypt as the password hasher to use.

So this is likely not a breaking change after all. You just have to live with the fact that users who do not change their password would remain on the previous password hash type. But it will not affect their ability to login as laravel will run it through all password hashing drivers till it find a match

On a side note if you are thinking of switching from bcrypt to argon2id in new apps, reconsider that decision, argon2id is not as straightforward better as one would think

https://twitter.com/terahashcorp/status/1155129705034653698