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

Jump to bottom

Improved opt out #19528

Merged
merged 65 commits into from Sep 9, 2022
Merged

Improved opt out #19528

merged 65 commits into from Sep 9, 2022

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Jul 14, 2022 •
edited

Description:

Fixes #17452

This is PR contains the UI changes to allow a choice between the tracker opt-out and the self-contained opt-out.

The self-contained code is ~85 lines and this contains a small 'MatomoConsent' class to handle basic checking, reading and writing of the consent cookie. All settings, including single language translations, are passed in as an object so they are easily editable at the top of the code block.

The 'MatomoConsent' class is reused the fallback method for the tracker opt out JavaScript to set cookies directly should the Matomo tracker be unavailable.

The tracker opt-out supports the following URL parameters and is compatible with the existing iFrame opt-out URL (just change optOut to optOutJS and add the div) :

backgroundColor
fontColor
fontSize
fontFamily
language              default "auto"            Language code for translations ("auto" for browser language)
showIntro             default 1                 Should the opt-out intro text be shown?
divId                 default "matomo-opt-out"  The id of the div which will contain the opt-out form
useCookiesIfNoTracker default 1                 Directly read/written consent cookies if no tracker found?
useCookiesTimeout     default 10                How long to wait for the tracker to be detected?
useSecureCookies      default 1                 Set secure cookies?

Review

@bx80 bx80 added Critical Indicates the severity of an issue is very critical and the issue has a very high priority. c: Privacy For issues that impact or improve the privacy. Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Jul 14, 2022
@bx80 bx80 added this to the 4.12.0 milestone Jul 14, 2022
@bx80 bx80 self-assigned this Jul 14, 2022
@bx80 bx80 removed the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Jul 22, 2022
@bx80
Copy link
Contributor Author

bx80 commented Jul 22, 2022

@sgiehl I've haven't implemented the requirement that if the tracker cannot be found then detect if the user is logged into the Matomo dashboard and show an error on the webpage and log an application error. I'm not seeing a straightforward way to do this without trying to read Matomo session cookies and add new API calls to log errors, which seems like an overkill. Any suggestions?

@bx80
Copy link
Contributor Author

bx80 commented Jul 25, 2022

In case it's helpful for testing, this is the tracking page I used during development, it can load the tracker after a random delay (or not at all) and includes a URL string with all the parameters.

<html>
  <head>
    <meta charset="utf-8">
    <title>OptOutJS</title>

    <!-- Matomo -->
    <script>

    // Test variables
    let loadTracker = true;
    let maxRandomDelay = 10;
    let minRandomDelay = 5;
    // ---

    function loadMatomoTracker() {
      console.log('.. now loading Matomo tracker');
      var _paq = window._paq = window._paq || [];
      _paq.push(['trackPageView']);
      _paq.push(['enableLinkTracking']);
      (function() {
        var u="//matomo/";
        _paq.push(['setTrackerUrl', u+'matomo.php']);
        _paq.push(['setSiteId', '3']);
        var d=document, g=d.createElement('script'), s=d.getElementsByTagName('script')[0];
        g.async=true; g.src=u+'matomo.js'; s.parentNode.insertBefore(g,s);
      })();
    }

    if (loadTracker === true)  {
      let randomInterval = (Math.floor((Math.floor(Math.random() * ((maxRandomDelay * 1000) - (minRandomDelay * 1000)) ) + (minRandomDelay * 1000)) / 1000) * 1000);
      setTimeout(function() { loadMatomoTracker() }, randomInterval);
      console.log('delaying Matomo tracker load for '+randomInterval / 1000+' seconds...');
    } else {
      console.log('tracker loading disabled');
    }
    </script>
    <!-- End Matomo Code -->

  </head>
  <body>
  OptOutJS
  <br><br>

  <div id="matomo-opt-out" style="text-align:center;width:350px;padding:10px">...opt-out loading...</div>
  <script src="https://matomo//index.php?module=CoreAdminHome&action=optOutJS&useCookiesTimeout=10&div=m-opt-out&language=auto&backgroundColor=BBBBFF&fontColor=444444&fontSize=16px&fontFamily=TimesNewRoman&showIntro=1&useCookiesIfNoTracker=1"></script>

  </body>
</html>

@bx80 bx80 added the Needs Review PRs that need a code review label Jul 25, 2022
@sgiehl sgiehl requested a review from Findus23 July 25, 2022 15:44
@sgiehl
Copy link
Member

sgiehl commented Jul 25, 2022

I've haven't implemented the requirement that if the tracker cannot be found then detect if the user is logged into the Matomo dashboard and show an error on the webpage and log an application error. I'm not seeing a straightforward way to do this without trying to read Matomo session cookies and add new API calls to log errors, which seems like an overkill. Any suggestions?

I guess I would have tried to solve that using the opt out code. The page, that includes the optout, fetches some javascript from Matomo, so in case the user is logged in you could simply add some more javascript code, that if matomo.js can't be loaded displays a warning and/or sends back a request to matomo, to log an error. But that for sure would only work if the javascript returned for the optout wasn't cached.
Maybe @tsteur has a better idea.

@bx80
Copy link
Contributor Author

bx80 commented Jul 26, 2022

The page, that includes the optout, fetches some javascript from Matomo, so in case the user is logged in you could simply add some more javascript code, that if matomo.js can't be loaded displays a warning and/or sends back a request to matomo, to log an error.

That was my plan, but the optOutJS request in many cases is likely to be cross-site and therefore doesn't pass any Matomo session cookies (which are Same-site:Lax and httpOnly by default), so when processing the optOutJS request Matomo doesn't know about the current session and is unaware that the user is logged in. For the similar reasons there's not going to be a secure way for the optOutJS code running on domain a to access the Matomo session for domain b at runtime either.

Without access to the Matomo API I suppose we could still use the tracking API to force some sort of tracking failure log (eg. GET https://matomo/matomo.php?idsite=9999998&rec=1), but I'm not sure that we'd want to create failure logs for every visitor who can't load the tracker JS.

@bx80
Copy link
Contributor Author

bx80 commented Aug 31, 2022

Thanks for the feedback @tsteur and @sgiehl

I've made the following changes:

  • Added a checkbox to toggle custom styling
  • Made the 'Remember to test...' box more visible (bootstrap blue background)
  • Added a separate header for the developer guide for custom opt-out section: 'Build your own'
  • Updated the change log to explain the new options and that the iframe call will still work
  • Added a 'What's new?' entry

The code should now be ready for further review.

To document the various ways the opt-out can work, would it be better to add another section to this guide: https://developer.matomo.org/guides/tracking-javascript-guide#optional-creating-a-custom-opt-out-form or maybe the custom opt-out form guide and the new information about how the opt-out can work should be moved to a separate developer docs section?

I can create draft updates of both these FAQs too:
https://matomo.org/faq/general/faq_20000/
https://matomo.org/faq/general/configure-privacy-settings-in-matomo/
Not sure if there is any other documentation to update?

sgiehl
sgiehl reviewed Aug 31, 2022
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Had a look through the code changes. Generally looks fine, but we should try to avoid adding style attributes to elements where possible. Otherwise it won't be easily possible for themes to changes those elements. Would be better to either reuse existing css classes if possible or to add new classes if needed.

@bx80 bx80 requested a review from sgiehl September 1, 2022 23:45
sgiehl
sgiehl approved these changes Sep 2, 2022
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Left a minor comment, we can also apply after hat one has been merged.
Otherwise this now looks good. So if @tsteur doesn't have any further objections, this should be good to merge.

plugins/PrivacyManager/changes.json Outdated
{
"title": "Privacy Manager - User Opt-Out Improvements",
"description": "The privacy manager user opt-out has been updated to provide new code generation options that improve compatibility and allow additional customisation.",
"version": "4.12.0"
Copy link
Member

Choose a reason for hiding this comment

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

If we plan to write some kind of documentation around the new opt out, we could include the link here.

@tsteur
Copy link
Member

tsteur commented Sep 5, 2022

Great to see our first what's new entry 🎉 That's awesome.

Had another quick look. I believe the information box uses a custom color that we maybe usually don't use in that combination?
image

Using class system notification notification-info + white color should do the trick.

image

I haven't tested the opt out otherwise. It be great to only merge though once we've created all the documentation (especially all the different flows and what we expect to happen in which cases), and possibly a small blog post? Not sure what's there already. Just wanting to make sure we won't forget it.

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Sep 5, 2022
@bx80 bx80 mentioned this pull request Sep 6, 2022
Merged
11 tasks
@bx80 bx80 mentioned this pull request Sep 8, 2022
Merged
11 tasks
@bx80 bx80 merged commit 178c87b into 4.x-dev Sep 9, 2022
@bx80 bx80 deleted the m-17452-opt-out-first-party branch September 9, 2022 04:08
@justinvelluppillai justinvelluppillai mentioned this pull request Nov 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinvelluppillai justinvelluppillai justinvelluppillai left review comments

@sgiehl sgiehl sgiehl approved these changes

@Findus23 Findus23 Awaiting requested review from Findus23

Assignees

@bx80 bx80

Labels
c: Privacy For issues that impact or improve the privacy. Critical Indicates the severity of an issue is very critical and the issue has a very high priority. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.12.0
Development

Successfully merging this pull request may close these issues.

Offer opt out without iframe / 3rd party cookies
4 participants
@bx80 @sgiehl @tsteur @justinvelluppillai