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 minimal first party cookie opt out support to JS tracker #12829

Merged
merged 14 commits into from May 8, 2018

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented May 6, 2018

Added three new methods to the tracker:

  • isUserOptedOut(): checks for piwik_ignore cookie on site being tracked (so not matomo domain).
  • optUserOut(): sets cookie on site.
  • forgetUserOptOut(): deletes the cookie on the site.

If the user is opted out, sendXmlHttpRequest() never sends anything to the server.

These new methods can be used to implement a custom opt out form on a site, which would not otherwise be easily doable since you'd have to set a cookie on the matomo domain.

Note: I tried to work this into the consent feature, since it's pretty similar (opt-out vs. opt-in), but the feature of consents that remembers requests to send is not applicable to opt-out (while a user is opted-out we don't want to remember anything the user did, otherwise we would be tracking them in some way).

I am not sure how well this works w/ other JS tracker features since it's been a while since I've coded there. @tsteur / @mattab can you guys take a look and see if this change will work? Putting in the 3.5.0 milestone, but feel free to move it.

@diosmosis diosmosis added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Needs Review PRs that need a code review labels May 6, 2018
@diosmosis diosmosis added this to the 3.5.0 milestone May 6, 2018
@diosmosis
Copy link
Member Author

If it's good to go, will add to changelog (+ write a faq for creating a custom opt out form?).

js/piwik.js Outdated
* requiring consent may affect how well you comply with relevant privacy laws (ie, GDPR).
*/
this.optUserOut = function () {
setCookie('piwik_ignore', '*', null, configCookiePath, configCookieDomain, configCookieIsSecure);
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering: should it also prevent setting any other Matomo cookies similar to the consent feature? And when the consent feature deletes all cookies, would it also remove a set ignore cookie?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 for preventing cookies getting set.

I was thinking originally that consent would be mutually exclusive w/ opt-out, but now it makes more sense to me to have both. Ie, require consent & provide option to just say no. But making them work together isn't necessarily straightforward...

Here are the scenarios I can think of:

  1. user consents, then later opts out: in this case we delete the consent cookie & set the opt out cookie
  2. user opts out w/o consenting: in this case, we set the piwik opt out cookie, and clear the consent queue
  3. user opts out (before or after consenting), then consents later: is this a sort of "opt-in"? or do they need to also give their consent? i'm thinking in this case, the opt-in cookie should be removed.

Which also makes me think if a user opted out, the consent queue should never be modified.

So that would mean the following changes:

  • when setting opt-out, remove consent cookie & clear consent queue
  • when opt-out cookie exists, do not add to consent queue
  • when consenting, remove opt-out cookie
  • (and make sure when opted-out no other cookies set)

@tsteur
Copy link
Member

tsteur commented May 6, 2018

Actually wondering if we cannot let users instead use somehow the consent feature I wonder?

@tsteur
Copy link
Member

tsteur commented May 6, 2018

Actually, using consent feature might not make sense as we might keep a record of this in the future etc. which wouldn't be needed for opt out etc I reckon

…osted site that prevents tracking requests from going out.
@mattab
Copy link
Member

mattab commented May 7, 2018

Moving to 3.6.0 as it requires more thought. (see also: #12834)

@mattab mattab modified the milestones: 3.5.0, 3.6.0 May 7, 2018
js/piwik.js Outdated
@@ -4127,7 +4129,7 @@ if (typeof window.Piwik !== 'object') {

for (index = 0; index < configCookiesToDelete.length; index++) {
cookieName = getCookieName(configCookiesToDelete[index]);
if (0 !== getCookie(cookieName)) {
if (excluding.indexOf(cookieName) !== -1 && 0 !== getCookie(cookieName)) {
Copy link
Member

Choose a reason for hiding this comment

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

I think here we should always exclude consent_removed and only delete it when it was specifically requested using deleteCookie('consent_removed') maybe?

Copy link
Member

Choose a reason for hiding this comment

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

same for consent cookie probably? Or actually not sure if we should always keep them... hard to say as when calling disableCookies() it would still keep it. On the other site we need to keep track of whether consent was given or removed even when cookies are disabled... I reckon we can leave it for now like you implemented it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it should always keep them, since you can call this.deleteCookies() by itself too, and if a matomo user or plugin did that, it would remove the visitor's opt-out/consent status.

js/piwik.js Outdated
// we want to make sure to remove all previously set cookies again
deleteCookies();
deleteCookies(['consent_removed']);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we also ignore consent? In theory it should probably not make a difference, but may be still good to have?

@@ -4781,6 +4784,56 @@ function preventClickDefault(selector)
}, 2000);
});

test("Test API - optOut (via consent feature)", function () {
Copy link
Member

Choose a reason for hiding this comment

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

FYI We also add for each public method a test to the group API methods

…t() method for building your own opt out form.
@diosmosis
Copy link
Member Author

Tested on a local site & was able to create a custom opt out form:

<div>
Here you can opt out:
<br/>
Current status: <span id="opted-out-status"></span>
<br/>
<a href="" id="opt-out">Opt out!</a>
<br/>
<a href="" id="opt-in">Opt in!</a>
</div>

<script>
jQuery(function ($) {
    _paq.push([function (tracker) {
        $('#opted-out-status').text(this.isUserOptedOut() ? 'opted out' : 'opted in');
    }]);

    $('#opt-out').click(function (e) {
        e.preventDefault();
        _paq.push(['optUserOut']);
        window.location.reload();
    });

    $('#opt-in').click(function (e) {
        e.preventDefault();
        _paq.push(['forgetUserOptOut']);
        window.location.reload();
    });
});
</script>

Verified when opted out it did not send a tracking request.

js/piwik.js Outdated
@@ -3267,6 +3267,8 @@ if (typeof window.Piwik !== 'object') {
return cookieMatch ? decodeWrapper(cookieMatch[2]) : 0;
}

configHasConsent = !getCookie('consent_removed');
Copy link
Member

Choose a reason for hiding this comment

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

I see... I wonder if we maybe need to call getCookie()also after each setCookieDomain() and setCookieNamePrefix() and setCookiePath() and setSiteId() etc as the domain on which the cookie is set might change?

I also wonder if we in general need to do getCookie(getCookieName('consent_removed')) and setCookie(getCookieName('consent_removed') and the same for consent etc

@@ -7167,7 +7174,7 @@ if (typeof window.Piwik !== 'object') {
* @returns number|string
*/
this.getRememberedConsent = function () {
var value = getCookie('consent');
var value = getCookie(CONSENT_COOKIE_NAME);
Copy link
Member

Choose a reason for hiding this comment

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

what if for some reason the user has a CONSENT_REMOVED_COOKIE_NAME set as well? maybe we should check for this first @diosmosis ? and if CONSENT_REMOVED_COOKIE_NAME is set, then we return false?

Copy link
Member

Choose a reason for hiding this comment

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

This is also important for hasRememberedConsent

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 constant is defined above, it's not a global

Copy link
Member Author

Choose a reason for hiding this comment

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

oh sorry, read that incorrectly.

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 makes sense for CONSENT_REMOVED_COOKIE_NAME to override CONSENT_COOKIE_NAME, though in this case, the user will not be tracked even if it appears they gave their consent. Though if they have both, they may never get tracked. I think in this case if CONSENT_REMOVED_COOKIE_NAME is set, we should remove CONSENT_COOKIE_NAME. This defaults to privacy and next time they will get the consent message again.

Copy link
Member

Choose a reason for hiding this comment

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

👍
if both are set, it is fine to remove CONSENT_COOKIE_NAME and assume consent is not given.

This defaults to privacy and next time they will get the consent message again.

This depends on the website owner. Likely the consent message would not be directly shown again.

js/piwik.js Outdated
@@ -7277,10 +7285,31 @@ if (typeof window.Piwik !== 'object') {
* want to re-ask for consent after a specific time period.
*/
this.forgetConsentGiven = function () {
deleteCookie('consent', configCookiePath, configCookieDomain);
deleteCookie(CONSENT_COOKIE_NAME, configCookiePath, configCookieDomain);
Copy link
Member

Choose a reason for hiding this comment

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

In rememberConsentGiven we log if cookies are disabled. I wonder if we should do the same here or ignore disabled cookies and set it anyway? I think though that we cannot really ignore configCookiesDisabled because setCookie checks it.

    if (configCookiesDisabled) {
                    logConsoleError('rememberConsentGiven is called but cookies are disabled, consent will not be remembered');
                    return;
                }

Copy link
Member Author

Choose a reason for hiding this comment

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

Think checking in setCookie is better than everywhere it's used, otherwise we'll just have to keep adding the same check.

Copy link
Member

Choose a reason for hiding this comment

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

👍 Only meant to add a log message. There will be users who implement the opt out themselves but have cookies disabled. Then it will be useful to log that it won't work.

js/piwik.js Outdated
@@ -7229,7 +7236,8 @@ if (typeof window.Piwik !== 'object') {
* want to remember consent across page views, call {@link rememberConsentGiven()} instead.
*/
this.setConsentGiven = function () {
configRequireConsent = false;
configHasConsent = true;
deleteCookie(CONSENT_REMOVED_COOKIE_NAME);
Copy link
Member

Choose a reason for hiding this comment

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

the path and domain is missing here?

js/piwik.js Outdated
@@ -7208,14 +7215,14 @@ if (typeof window.Piwik !== 'object') {
*/
this.requireConsent = function () {
if (!this.hasRememberedConsent()) {
configRequireConsent = true;
configHasConsent = false;
Copy link
Member

Choose a reason for hiding this comment

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

here I wonder if we should set configHasConsent = this.hasRememberedConsent();?

Just in case for some reason it was set to false before, then it would be set to true afterwards.

Copy link
Member Author

Choose a reason for hiding this comment

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

Couldn't hurt. Assuming it's always going to be called before the other consent functions.

@tsteur tsteur modified the milestones: 3.6.0, 3.5.0 May 8, 2018
js/piwik.js Outdated
@@ -7180,7 +7197,7 @@ if (typeof window.Piwik !== 'object') {
* @returns bool
*/
this.hasRememberedConsent = function () {
return !!getCookie('consent');
return !!getCookie(CONSENT_COOKIE_NAME);
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 here we can simply return return !!this.getRememberedConsent()? to make sure we also handle the case when both cookies are set?

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good 👍

@diosmosis
Copy link
Member Author

@tsteur updated the pr

@mattab mattab merged commit 6f5b85b into 3.x-dev May 8, 2018
@mattab mattab deleted the piwik-ignore-tracker branch May 8, 2018 10:20
@Findus23
Copy link
Member

@tsteur, @diosmosis

Do we already have a documentation for those functions?

Because I think it is really important for GDPR that people don't think they are compliant because they have an opt-out iFrame, but it is doing nothing as they are on another domain (which if I am not misunderstanding something is the case most of the time on InnoCraft cloud.)

@mattab
Copy link
Member

mattab commented May 14, 2018

@Findus23 good point: created #12905 Document "How do I create a custom opt-out form?"

InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
…org#12829)

* Add minimal opt out support to JS tracker: methods to set cookie on hosted site that prevents tracking requests from going out.

* Update piwik.js

* Update piwik.js

* Rework tracker opt-out to use consent feature.

* Do not delete consent related cookies ever and add this.isUserOptedOut() method for building your own opt out form.

* Fix tests.

* Add prefix to consent/consent_removed cookie names.

* fix up pr

* one more quick fix for safety

* fix tests.

* Use getRememberedConsent() in hasRememberConsent() to protect against both cookie set case.

* Re-minify JS.

* fix copy paste

* re-minify
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants