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.

Sign up for GitHub

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

Jump to bottom

Introduced usage of Sha1 hash generation with UTF-8 encoding, without hyphens #49

Merged
merged 6 commits into from
Apr 18, 2017
Merged

Introduced usage of Sha1 hash generation with UTF-8 encoding, without hyphens #49

merged 6 commits into from
Apr 18, 2017

Conversation

ptr1120
Copy link
Collaborator

@ptr1120 ptr1120 commented Apr 13, 2017 •
edited
Loading

Added tests to prevent changes in hash representation before changing the hash provider concerning #35.

  • added regression tests in order to trigger changes in hash representation
  • extracted generation of hashes to CryptoExtensions

As we discussed, Encoding.Default will not work in different environments (tests did not fail on my side, but on Travis). Therefore we should discuss another API change (VisitorId, CookieName, UserId would be affected) towards using explicit UTF-8 encoding instead of environment dependent Encoding.Default.

ptr1120 added 2 commits April 13, 2017 16:34
@ptr1120
- extracted generation of hashes to cryptoextensions
bb6dc46 
- added regression tests in order to trigger changes in hash generation
@ptr1120
fixed 2 false positive test expectations
a494020
@ptr1120 ptr1120 closed this Apr 13, 2017
@ptr1120 ptr1120 deleted the RegressionTestsForHashGeneration branch April 13, 2017 15:47
@ptr1120 ptr1120 restored the RegressionTestsForHashGeneration branch April 13, 2017 15:52
@ptr1120 ptr1120 reopened this Apr 13, 2017
@ptr1120
Copy link
Collaborator Author

ptr1120 commented Apr 13, 2017

sorry for the closing/reopening, clicked the wrong button.

@ptr1120 ptr1120 changed the title Regression tests for hash generation Introduces usage of UTF-8 encoding for Hash generation Apr 13, 2017
@ptr1120 ptr1120 changed the title Introduces usage of UTF-8 encoding for Hash generation Introduced usage of UTF-8 encoding for Hash generation Apr 13, 2017
@ptr1120
Copy link
Collaborator Author

ptr1120 commented Apr 13, 2017

As discussed implemented usage of UTF8 encoding for hash generation instead of "default" encoding which depends on the environment.

@ptr1120
Copy link
Collaborator Author

ptr1120 commented Apr 13, 2017

Should I also remove the usage of md5 here? Then we are rid of it as we change the API anyway. You already mentioned that

Also, in JavaScript, MD5 is not used any more.
In https://github.com/piwik/piwik-dotnet-tracker/blob/master/Piwik.Tracker/PiwikTracker.cs#L434, we can use SHA1 as in Javascript : https://github.com/piwik/piwik/blob/3.x-dev/js/piwik.js#L3747

@ptr1120
- Sha1 removes hyphens now by default
3b6c75a 
- removed md5 hash method
- implemented usage of Sha1 hash generation method => SetNewVisitorId changed from md5 to sha1; GetUserIdHashed changed from Sha1 with hyphens to sha1 without hyphens
@ptr1120 ptr1120 changed the title Introduced usage of UTF-8 encoding for Hash generation Introduced usage of UTF-8 encoding for Hash generation; removed usage of Md5 hashes; removed hyphens from Sha1 hashes Apr 13, 2017
@ptr1120
Copy link
Collaborator Author

ptr1120 commented Apr 13, 2017

As discussed I removed the MD5 hash method in favour for sha1 (which breaks SetNewVisitorId()), removed hyphens from Sha1 hash method (which breaks GetUserIdHashed()).

@ptr1120 ptr1120 changed the title Introduced usage of UTF-8 encoding for Hash generation; removed usage of Md5 hashes; removed hyphens from Sha1 hashes Introduced usage of Sha1 Hash generation with UTF-8 encoding, without hyphens Apr 13, 2017
@ptr1120 ptr1120 changed the title Introduced usage of Sha1 Hash generation with UTF-8 encoding, without hyphens Introduced usage of Sha1 hash generation with UTF-8 encoding, without hyphens Apr 13, 2017
@ptr1120
sha1 : added overload for byte array
e98d5bc 
renaming CreateSha1 -> ToSha1
julienmoumne
julienmoumne requested changes Apr 14, 2017
View reviewed changes
Copy link
Member

@julienmoumne julienmoumne left a comment

Choose a reason for hiding this comment

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

Thanks Peter!
Your PR will allow us to fix #50 #51

Piwik.Tracker/PiwikTracker.cs Outdated
@@ -431,8 +429,7 @@ public void ClearCustomTrackingParameters()
/// </summary>
public void SetNewVisitorId()
{
var encodedGuidBytes = new MD5CryptoServiceProvider().ComputeHash(Encoding.Default.GetBytes(Guid.NewGuid().ToString()));
_randomVisitorId = BitConverter.ToString(encodedGuidBytes).Replace("-", "").Substring(0, LengthVisitorId).ToLower();
_randomVisitorId = Guid.NewGuid().ToByteArray().ToSha1().Substring(0, LengthVisitorId).ToLower();
Copy link
Member

Choose a reason for hiding this comment

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

Not a big deal but I think ToLower is not required here since ToSha1 produces lower case chars right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, thats right "x2" formats as lowercase hex. Done.

@ptr1120
Copy link
Collaborator Author

ptr1120 commented Apr 14, 2017

Yes, thats right, I will remove it on Monday.

@julienmoumne julienmoumne merged commit 663e3c2 into matomo-org:master Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julienmoumne julienmoumne julienmoumne approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ptr1120 @julienmoumne