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

add disable finger print options #18599

Merged
merged 20 commits into from Jan 17, 2022
Merged

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Jan 10, 2022

Description:

Fixes: #18448
add disable finger print

FAQ Preview: https://matomo.org/faq/how-do-i-disable-browser-feature-detection-completely/

Review

add disable finger print
@peterhashair peterhashair added the c: Documentation For issues related to in-app product help messages, or to the Matomo knowledge base. label Jan 10, 2022
@peterhashair peterhashair added this to the 4.7.0 milestone Jan 10, 2022
@peterhashair
Copy link
Contributor Author

@tsteur sorry to bother, just checking if I am on the right track.

Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

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

@peterhashair left a few comments, overall goes in the right direction

js/piwik.js Outdated Show resolved Hide resolved
js/piwik.js Show resolved Hide resolved
js/piwik.js Outdated Show resolved Hide resolved
js/piwik.js Outdated Show resolved Hide resolved
@peterhashair peterhashair added Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Jan 11, 2022
js/piwik.js Outdated Show resolved Hide resolved
Peter and others added 2 commits January 12, 2022 10:12
add disableBrowserFeatureDetection to apply first
@justinvelluppillai
Copy link
Contributor

@peterhashair why are you adding those eslint rules?

@peterhashair
Copy link
Contributor Author

peterhashair commented Jan 12, 2022

@justinvelluppillai
Ah maybe we need those ignore it throw lots of errors for pikwi.js, see screen shots
image

Peter added 2 commits January 12, 2022 14:26
remove eslint rules
js/piwik.js Outdated Show resolved Hide resolved
Peter added 2 commits January 12, 2022 15:23
swap apply event place
apply ignores
@peterhashair
Copy link
Contributor Author

build js

@justinvelluppillai
Copy link
Contributor

@peterhashair be good to get this one ready for next review and request reviewer if it is being ignored. Also looking at the FAQ I think maybe it is better to add the information to this page which describes how the config_id works more generally also https://matomo.org/faq/general/how-is-the-visitor-config_id-processed/ (or at least link to this page and the page on fingerprinting).

@peterhashair
Copy link
Contributor Author

@justinvelluppillai sure, will do.

move disableBrowserFeatureDetection up
@peterhashair
Copy link
Contributor Author

build js

Peter added 2 commits January 17, 2022 14:02
move function below
update format
js/piwik.js Outdated Show resolved Hide resolved
Peter and others added 2 commits January 17, 2022 14:06
update some naming
Co-authored-by: Justin Velluppillai <justin@innocraft.com>
Copy link
Contributor

@justinvelluppillai justinvelluppillai left a comment

Choose a reason for hiding this comment

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

Working for me, looking good also. @tsteur did you want to have a look?

@tsteur
Copy link
Member

tsteur commented Jan 17, 2022

@justinvelluppillai I haven't fully tested it but I think we could merge for now so it's in the release. Then add some follow up work like add also an enable... method (which they can call if they get consent) and maybe there could be also one or more automated tests for this? Like making sure when the feature is disabled, that we're not sending any browser resolution in the tracking request?

Copy link
Contributor

@bx80 bx80 left a comment

Choose a reason for hiding this comment

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

Works for me. It would probably be a good idea to have a tracker test to make sure screen resolution and other fingerprint data are not recorded when this option is enabled to prevent any future regression.

@justinvelluppillai
Copy link
Contributor

This is looking good thanks @peterhashair, still good to publish your FAQ and update the other mentioned doc, after that once these tests pass this is good to merge and will probably be the last merge before the next RC.

@peterhashair
Copy link
Contributor Author

peterhashair commented Jan 17, 2022

@justinvelluppillai updated the FAQ, just reference a link at the bottom. publish it now.

@peterhashair
Copy link
Contributor Author

@peterhashair peterhashair merged commit c6442f3 into 4.x-dev Jan 17, 2022
@peterhashair peterhashair deleted the m-18448-disable-fingerprint branch January 17, 2022 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Documentation For issues related to in-app product help messages, or to the Matomo knowledge base. Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an option to disable fingerprinting / config_id entirely
5 participants