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

When consent explicitly given in JS tracker add &consent=1 to URL #13230

Merged
merged 4 commits into from Aug 3, 2018

Conversation

diosmosis
Copy link
Member

When requireConsent() is called and a request is sent (ie, consent is explicitly given for tracking), we add a consent=1 param to the URL.

If requireConsent() is not called, then the site is assuming consent in which case we do not add the parameter.

If consent is not given, of course the request will never be sent so no need to add to the URL.

Note: this allows us to differentiate between assumed & explicit consent.

Fixes #12834

@diosmosis diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Jul 31, 2018
@diosmosis diosmosis added this to the 3.6.0 milestone Jul 31, 2018
@@ -7224,6 +7231,7 @@ if (typeof window.Piwik !== 'object') {
* time the user gives you consent, you do not need to ever call `_paq.push(['setConsentGiven'])`.
*/
this.requireConsent = function () {
configConsentRequired = true;
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be set when setConsentGiven is called rather than requireConsent?

Copy link
Member Author

Choose a reason for hiding this comment

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

setConsentGiven would not be called if requireConsent isn't, unless it's for undo-ing opt-out. And in that case I don't think it's explicit consent, it's still assumed consent, right?

Here are the cases I thought of:

user needs to get explicit consent for GDPR:

  • user calls requireConsent()
  • setConsentGIven called
  • &consent=1 added to mark request as having explicitly given consent

user doesn't need consent:

  • requests are sent w/o any interruption, &consent=1 is not added since it is assumed consent

user has opt out form:

  • user sends initial request w/o &consent=1
  • visitor opts out, forgetConsentGiven is called, no requests tracked
  • visitor opts in, setConsentGiven, requests tracked w/o &consent=1 because user is not required to give consent

I guess the question is, should an opted-in visitor (a visitor who can optionally give their consent) be treated the same as a visitor who is required to give their consent.

Copy link
Member

Choose a reason for hiding this comment

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

I guess the question is, should an opted-in visitor (a visitor who can optionally give their consent) be treated the same as a visitor who is required to give their consent.

a visitor is either required to give their consent, or their data is tracked by default (and our separate opt-out feature relying on 3rd party cookies applies).

Copy link
Member

@mattab mattab left a comment

Choose a reason for hiding this comment

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

Could you check my comment and change/update tests & merge the PR then?

js/piwik.js Outdated
@@ -3735,6 +3738,10 @@ if (typeof window.Piwik !== 'object') {
return;
}
if (!configDoNotTrack && request) {
if (configConsentRequired) { // send a consent=1 when explicit consent is given for the apache logs
Copy link
Member

Choose a reason for hiding this comment

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

I agree with your last comment 👍 So I would expect this if to be instead if (configConsentRequired && configHasConsent ) so we only set the consent=1 when consent was required + given?

Copy link
Member Author

Choose a reason for hiding this comment

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

It already does that, ie, if configHasConsent is false, it never gets to this code. The case I was talking about was when configHasConsent = true because of opt-in, but configConsentRequired = false because the user doesn't care about GDPR.

Copy link
Member Author

Choose a reason for hiding this comment

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

The opt-out feature I'm referring to btw is the new first party cookie one that uses the consent feature: https://github.com/matomo-org/matomo/blob/3.x-dev/js/piwik.js#L7333-L7346

Copy link
Member

Choose a reason for hiding this comment

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

@diosmosis maybe you can add the check in the if statement (is not required but makes it easier to understand) and merge then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, will change it to (configConsentRequired && configHasConsent).

@diosmosis diosmosis merged commit 892d8ff into 3.x-dev Aug 3, 2018
@diosmosis diosmosis deleted the 12834-apache-consent branch August 3, 2018 07:53
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
…tomo-org#13230)

* When consent explicitly given in JS tracker add &consent=1 to URL so apache logs will remember consent was given.

* add parameter when sending the request

* clarify condition for sending consent=1 param
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

2 participants