New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Joomla 4.0: Removing deprecated code from JUser* classes #12267

Merged
merged 10 commits into from Oct 7, 2016

Conversation

Projects
None yet
6 participants
@Hackwar
Member

Hackwar commented Oct 2, 2016

This PR removes deprecated code from the JUser* classes. This also removes the SHA256 password encryption!

This PR has 3 debateable changes:

  1. The SHA256 password encryption gets removed. That encryption was introduced in Joomla 3.2.0 and deprecated in 3.2.1. This means that passwords encrypted under 3.2.0 and whose users have since NOT logged in again, will not be verifyable in Joomla 4.0 and onwards. Considering that 3.2.0 was released in 11/2013 (3 years ago) and 3.2.1 was released in 12/2013 and fixed the issue then, I would argue that users with those old hashing can be omitted. They still can reactivate their accounts by requesting to reset their password. To make this clear: This will only affect users that have created their account in 3.2.0 and have NOT logged in since then or where the site was not updated to a later Joomla version. Considering that the login was broken for those people anyway, I can't see a site that stuck to that version. If a user had an outdated password hash, it automatically gets updated upon next login.
  2. The method JUserHelper::_bin is not deprecated, but I'm removing it anyway. The method is set to private and is only used in the getCryptedPassword method, which gets removed with this PR. No other code can user this method, so this should be save, even though we didn't deprecate it.
  3. The method JUserHelper::_toAPRMD5() is in a similar situation. Again only used in JUserHelper::getCryptedPassword() and set to protected, so it is not really a part of the public API. However, by being protected and not private, a class extending JUserHelper could theoretically use it.
@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 3, 2016

I've also removed JCrypt::hasStrongPasswordSupport(), since our minimum requirement is PHP 5.5.0 and that has this feature built in. Since I don't know how to properly update the composer stuff to remove the class from @ircmaxell, I'm leaving that to someone else.

Merge branch '4.0-dev' of https://github.com/joomla/joomla-cms into j…
…4userhelper

Conflicts:
	administrator/components/com_admin/sql/updates/mysql/4.0.0-2016-10-02.sql

@Hackwar Hackwar force-pushed the Hackwar:j4userhelper branch from 056a533 to 7dfa36c Oct 3, 2016

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Oct 3, 2016

OK I'm willing to hear sides here BUT I'm unconvinced with the sha stuff. We need to do one of two things

  1. Keep the SHA verification only (obviously we never need to generate these PW's)
  2. Automatically go through the database and check for users with sha passwords and enable the password reset required flag.

I know it sucks like hell. But we can't leave the users on upgrade in a state of limbo...

@mbabker

This comment has been minimized.

Member

mbabker commented Oct 3, 2016

Since I don't know how to properly update the composer stuff to remove the class from @ircmaxell, I'm leaving that to someone else.

Leave it for now. Until we start getting other dependencies bumped up (i.e. the Registry package to Framework v2 branch) the polyfill is still going to be there (the Registry v1 package requires Symfony's PHP 5.5 polyfill which requires the password_compat package).

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 3, 2016

@wilsonge 3.2.0 was available for one month and it was unusable for several reasons. Among other things, it couldn't verify the passwords that it generated itself. The number of users that were affected was small. We are practically at the point where the only accounts still having this password are those, where a normal visitor registered on the site and has since NEVER come back. 3.2.0 didn't only break the password system and the authentication system, it broke several other things, too. I'm close to betting money that there is no website out there still running 3.2.0, because it was so broken. I would strongly suggest to simply drop SHA256 and be done with it.

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Oct 3, 2016

I understand. But iirc it was largely bcrypt passwords that had partial backfills in some old linux versions that were broken though. Not the sha/md5 or the 'correctly implemented' bcrypt support.

I don't particularly mind dropping sha support. But if we do we need to force all users who have sha passwords into a password reset (some sort of function in the script.php file). It shouldn't be upto site administrators to have to manually review the database on their own...

Hackwar added some commits Oct 3, 2016

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 3, 2016

Would the SQL query be enough?

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Oct 4, 2016

Looks ok to me. Needs a quick test tho to be sure

@fastslack

This comment has been minimized.

Contributor

fastslack commented Oct 6, 2016

Test ok

  1. Keep the SHA verification only (obviously we never need to generate these PW's)
  2. Automatically go through the database and check for users with sha passwords and enable the
    password reset required flag.

I know it sucks like hell. But we can't leave the users on upgrade in a state of limbo...

I know that we have the .sql file with all database changes but some document with developers anotations could be very helpful for future upgrade extensions.

@zero-24

This comment has been minimized.

Contributor

zero-24 commented Oct 6, 2016

We need the update query also for postgres

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Oct 6, 2016

FWIW I discussed with Hannes last night and this simplistic approach doesn't work as password reset requires validating passwords. The only option is on upgrade to do email reset of passwords :( There is no other way that would work other than allowing SHA's forever

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 6, 2016

So, with just 5 commits, 🙊 I was able to put back the SHA256 password verification. While I really dislike, that you want this back in, I'd rather put in this fix than loose the other 700 LoCs that get removed with this.

@wilsonge wilsonge merged commit b3a710d into joomla:4.0-dev Oct 7, 2016

2 checks passed

continuous-integration/drone the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wilsonge wilsonge added this to the Joomla 4.0 milestone Oct 7, 2016

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Oct 7, 2016

I'm happy now. Well not happy because I think we still need to form a plan for removing SHA support. But all the other changes are good.

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Oct 7, 2016

#12333 Open Issue to track the SHA 256 stuff

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