Skip to content
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

Standardize password hash function usage #9480

Merged
merged 7 commits into from Jan 11, 2016

Conversation

Joey3000
Copy link
Contributor

@Joey3000 Joey3000 commented Jan 7, 2016

As preparation for introduction of a stronger password hash function in #5728, this standardizes the usage of the current password hash function across the Login and UsersManager plugins. Specifically:

  • Use UsersManager::getPasswordHash() throughout, instead of previous direct use of md5() in some places.
  • Concentrate the hash length sanity check in the newly created UsersManager::checkPasswordHash(). Previously, UsersManager\API and Login\PasswordResetter classes did it separately. Both checks were moved into the newly created function in the UsersManager class, as that is also where other public static password check functions reside.
  • Replace the "md5" string with "hash" in affected variable names and comments.

No functional change is intended.

One little caveat is that I can't test at the moment. It would be great if someone could take this on a test ride.


To summarize the hash function uses by the Login and UsersManager plugins:

Following hash function uses were not modified:

As preparation for introduction of a stronger hash function in matomo-org#5728, this standardizes the usage of the current **password** hash function across the Login and UsersManager plugins. Specifically:

* Use UsersManager::getPasswordHash() throughout, instead of previous direct use of md5() in some places

* Concentrate the hash length sanity check in the newly created UsersManager::checkPasswordHash(). Previously, UsersManager\Auth and Login\PasswordResetter classes did it separately. Both checks were moved into the newly created UsersManager class function, as that is also where other public static password check functions reside.

* Replaced the "md5" string with "hash" in affected variable names and comments

No functional change is intended.
As preparation for Common::hash() changing its hash function some time in future
For performance reasons
@Joey3000
Copy link
Contributor Author

Joey3000 commented Jan 8, 2016

Does anyone know why the test fails?

I've tested the patch with piwik-2.16.0-b1.zip (on a remote test server, updated from piwik-2.15.0-b14 via a complete delete and re-upload via FTP, keeping the DB tables) and don't get any errors with PHP 5.5. Tested:

  • As super user: Login plugin: login, logout; UsersManager plugin: create a "view" user, change own and "view" user's password
  • As the new "view" user: Login plugin: login, logout
  • Not logged in: password change (was accepted - failed correctly: server has e-mailing disabled)

I don't get any passwordHash related errors.


The only error I see does not seem to be related: UsersManager: When trying to change own password as the "view" user (not tested as super user), I get following error:

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'use_12_hour_clock' in 'field list'

(Without trying to change the clock format. And similarly for 'use_24_hour_clock' if I try to change it to 24h-format.)

But that error could be due to the update from the 2.15.0-b14 beta. I'd need to try updating from a released version some time.

@Joey3000
Copy link
Contributor Author

Joey3000 commented Jan 8, 2016

I found it, thanks to https://builds-artifacts.piwik.org/piwik/piwik/master/17449/ (missing semicolon in Login\PasswordResetter::checkPasswordHash(), which I didn't test due to the disabled e-mailing on the server).

The remaining passwordHash-related issues are due to the function argument renaming in https://github.com/piwik/piwik/pull/9480/files#diff-243038712dace7532e3b1856dd63274eR789. (Including the API list screenshot - scroll down to the "Module UsersManager". So, it's expected. But I can revert the renaming, if desired.

Please let me know if it'd be better to do this in the 3.0 branch, which #5728 is for (or not at all).

* @return string
*/
public function getTokenAuth($userLogin, $md5Password)
public function getTokenAuth($userLogin, $passwordHash)
Copy link
Member

Choose a reason for hiding this comment

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

That's why tests fail as it breaks the API. Unfortunately we cannot rename this one although it would be good :(

Users call it eg with index.php?module=API&method=API.getTokenAuth&userLogin=foo&md5Password=bar.... This would break all applications, libraries etc that use this method (eg the Piwik Mobile App but also many other libraries and apps).

Unfortunately we do not have a versioning in our API to handle such cases. The only thing that I could imagine would be making $md5Password optional, add $passwordHash optional and then checking in the method that at least one of them is set and if not throw an exception.

We'd need to document this in the developer changelog and deprecate $md5Password parameter. We'd probably never actually remove that parameter instead deprecate the whole API and replace it with a REST API in a few years

@tsteur
Copy link
Member

tsteur commented Jan 10, 2016

PR looks good! Thx for that. Only one issue with the failing test. Explained it in a code comment

@Joey3000
Copy link
Contributor Author

Thanks for the feedback! I reverted the renaming (adding a comment in https://github.com/piwik/piwik/pull/9480/files#diff-243038712dace7532e3b1856dd63274eR786 instead) and fixed another issue left over.

I don't know what the remaining test failures are about, though. E.g.:

Piwik\Tests\System\PeriodIsRangeDateIsLastNMetadataAndNormalAPITest: Differences with expected in '/home/travis/build/piwik/piwik/tests/PHPUnit/Framework/TestCase/../../System/processed/test_periodIsRange_dateIsLastN_MetadataAndNormalAPI__Live.getVisitorProfile.xml'

Failed asserting that two DOM documents are equal.

--- Expected

+++ Actual

@@ @@

 <?xml version="1.0"?>

 <result>

    <totalVisits>2</totalVisits>

    <totalVisitDuration>361</totalVisitDuration>

    <totalActions>2</totalActions>

+   <totalEvents>0</totalEvents>

+   <totalOutlinks>1</totalOutlinks>

+   <totalDownloads>0</totalDownloads>

    <totalSearches>0</totalSearches>

+   <totalPageViews>1</totalPageViews>

[...]

As well as the moved text in the screenshots in https://builds-artifacts.piwik.org/piwik/piwik/master/17472/.

I would appreciate any hints.

@tsteur
Copy link
Member

tsteur commented Jan 11, 2016

Thx for the quick change! The failing test is not due to your change. I merged a couple of PRs yesterday but haven't fixed the build yet.

tsteur added a commit that referenced this pull request Jan 11, 2016
Standardize password hash function usage
@tsteur tsteur merged commit fa5dcae into matomo-org:master Jan 11, 2016
@tsteur tsteur added the Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. label Jan 11, 2016
@tsteur tsteur added this to the 2.16.0 milestone Jan 11, 2016
@Joey3000 Joey3000 deleted the standard_hash branch January 12, 2016 02:04
@Joey3000
Copy link
Contributor Author

The only error I see does not seem to be related: UsersManager: When trying to change own password as the "view" user (not tested as super user), I get following error:

  SQLSTATE[42S22]: Column not found: 1054 Unknown column 'use_12_hour_clock' in 'field list'

Reported in #9666.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants