search

benedmunds / CodeIgniter-Ion-Auth

Simple and Lightweight Auth System for CodeIgniter
http://benedmunds.com/ion_auth/
MIT License
2.35k stars 1.14k forks source link

Remove admin specific hash params, use the same params for all users #1498

Closed benedmunds closed 3 years ago

jamieburchell commented 3 years ago

The PASSWORD_ARGON... constants will not be available to everyone, since it requires PHP to be compiled with support, or the installation of php-sodium and so some users may receive an undefined constant warning.

In my case I used those constants in the config because I know I have support. I guess the constants could be checked for existence in the config.

The _get_hash_parameters() method still has $identity = null in its signature. Not sure if that is for BC, but it's no longer used and so any calls that pass it could also be updated.

benedmunds commented 3 years ago

@jamieburchell thanks for the review. Committed those changes, please review again when you can.

benedmunds commented 3 years ago

Ah nice, I didn't know PASSWORD_BCRYPT_DEFAULT_COST was available. Thanks! Review again when you can please.

jamieburchell commented 3 years ago

Looks good to me. As you said, in hindsight passing options direct to the password hash probably would have been the way to go and easier to maintain, but I can't think of a neat way of adding this without introducing BC.

The defined check is purely to stop PHP warnings/notices if bcrypt or argon support is missing. If they aren't available, the params at the end of the ternary are actually useless (as neither can be used). If they are available, the constants will be used.

The "default" in the parameter names now don't need to be there since "admin" has been removed, but people will still have that in their configs. Unless you start checking for old config params etc in the library, I don't see how to neatly change that.

There will be issues/unexpected consequences if people have not configured the "default". Or if people have used stronger params for "admin" than default.

benedmunds commented 3 years ago

There will be issues if people have not configured the "default". Or if people have used stronger params for "admin" than default.

What issues are you thinking of here? These should rehash but I don't see where it'd cause any broken functionality for users.

jamieburchell commented 3 years ago

If a user has configured their "admin" user hash params specifically to be stronger than their "default" user hash params, this change means all hashes will now use those configured in the "default". It doesn't break anything, but people might not realise the "admin" params are now redundant and aren't doing anything and that potentially passwords are rehashed with lesser params. It's probably a non-issue if the defaults are strong too.

Following on from that, if people haven't configured their default user hash params (maybe they ignored them because they only use admin users) the change might be unexpected. Again, not breaking, but a change in previous functionality.

benedmunds commented 3 years ago

Good points, I'll add a comment to the config warning about the changes as well.