add token_auth to overlay requests where necessary

[geekdenz]

Description:

With @tsteur we debugged this non-trivial issue.

We did not need to change any core code, just in the client it needed to be ensured that the token_auth parameter gets added to the URL or POST under the correct circumstances.

Notes:

• I did not run phpunit yet. Trying to find out how to do this effectively in PHPStorm rather than just CLI.
• There is a subtle bug that seems to have a redirect loop like symptom when the overlay page is loaded. However, waiting for half a minute it works as expected. This could be a performance problem going forward if not fixed.
• "Open Row Evolution" is broken with "Error: Expected date to be an instance of \Piwik\Date"
• "Open segmented Visits Log" is broken with "Error: Your session has expired due to inactivity. Please log in to continue."
• Working on these problems now.

Review

• Functional review done
• Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
• Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
• Security review done see checklist
• Code review done
• Tests were added if useful/possible
• Reviewed for breaking changes
• Developer changelog updated if needed
• Documentation added if needed
• Existing documentation updated if needed

[tsteur]

👍 nice @geekdenz Left 2 comments. Also quickly searched the code base for &force_api_session=1' and I reckon we also need to adjust widgetloader.directive.js and visitprProfile.js to only add the parameter force_api_session when needed. It's not relevant for this issue but be good to make sure it's also fixed in other places.

[geekdenz]

Need to follow-up with solving force_api_session=1 calls.

Decision: create method to check and add this to an existing URL.

@tsteur, did we agree to add this in broadcast.js or somewhere else?

[tsteur]

sounds good @geekdenz 👍

[geekdenz]

@tsteur Sweet, I think it works now in lieu of the Travis CI tests.

[tsteur]

@geekdenz 👍 nice, left a comment and I think widgetloader.directive.js and reportexport.directive.js also still needs to be adjusted.

[geekdenz]

plugins/Morpheus/icons may need to be removed

[tsteur]

👍 nice, left one last comment @geekdenz Haven't tested it again but looks good. Be great for someone else from the core team to review this one in case I missed something.

[sgiehl]

@geekdenz Would be good not to change/update the submodules in your PRs when it's not needed

[geekdenz]

@geekdenz Would be good not to change/update the submodules in your PRs when it's not needed

Thanks @sgiehl . You are right, the submodules should not be changed unless necessary for the fix.

However, I did a git diff 4.x-dev and saw no updates that are not necessary:

diff --git a/core/View.php b/core/View.php
index 707030b2c3..4c3fca1915 100644
--- a/core/View.php
+++ b/core/View.php
@@ -11,6 +11,7 @@ namespace Piwik;
 use Exception;
 use Piwik\AssetManager\UIAssetCacheBuster;
 use Piwik\Container\StaticContainer;
+use Piwik\Session\SessionAuth;
 use Piwik\View\ViewInterface;
 use Twig\Environment;
 use Twig\Error\Error;
@@ -458,7 +459,25 @@ class View implements ViewInterface
     private function shouldPropagateTokenAuthInAjaxRequests()
     {
         $generalConfig = Config::getInstance()->General;
-        return Common::getRequestVar('module', false) == 'Widgetize' || $generalConfig['enable_framed_pages'] == '1';
+        return Common::getRequestVar('module', false) == 'Widgetize' ||
+            $generalConfig['enable_framed_pages'] == '1' ||
+            $this->validTokenAuthInUrl();
+    }
+
+    /**
+     * @param bool $return the token_auth $_GET variable
+     * @return bool|string
+     * @throws Exception
+     */
+    private function validTokenAuthInUrl(bool $return = false)
+    {
+        $tokenAuth = Common::getRequestVar('token_auth', '', 'string', $_GET);
+        if ($tokenAuth) {
+            if ($tokenAuth == Piwik::getCurrentUserTokenAuth()) {
+                return $return ? $tokenAuth : true;
+            }
+        }
+        return false;
     }
 
     /**
diff --git a/misc/log-analytics b/misc/log-analytics
index 6a0dae6126..b9f5e1e766 160000
--- a/misc/log-analytics
+++ b/misc/log-analytics
@@ -1 +1 @@
-Subproject commit 6a0dae6126b97bd083cd5a48b598526bb25f0509
+Subproject commit b9f5e1e7665a2af5e7d9c59f563070b4bfbca8cc
diff --git a/plugins/Annotations/javascripts/annotations.js b/plugins/Annotations/javascripts/annotations.js
index f833045740..2a19f81ed5 100644
--- a/plugins/Annotations/javascripts/annotations.js
+++ b/plugins/Annotations/javascripts/annotations.js
@@ -112,6 +112,7 @@
 
             var ajaxRequest = new ajaxHelper();
             ajaxRequest.addParams(ajaxParams, 'get');
+            ajaxRequest.withTokenInUrl();
             ajaxRequest.setFormat('html');
             ajaxRequest.setCallback(callback);
             ajaxRequest.send();
diff --git a/plugins/CoreHome/angularjs/common/services/piwik-api.js b/plugins/CoreHome/angularjs/common/services/piwik-api.js
index 53edc3f292..b9a8a9fb2f 100644
--- a/plugins/CoreHome/angularjs/common/services/piwik-api.js
+++ b/plugins/CoreHome/angularjs/common/services/piwik-api.js
@@ -338,7 +338,7 @@ var hasBlockedContent = false;
         }
 
         return {
-            withTokenInUrl: withTokenInUrl,
+            withTokenInUrl: withTokenInUrl, // technically should probably be called withTokenInPost
             bulkFetch: bulkFetch,
             post: post,
             fetch: fetch,
diff --git a/plugins/CoreHome/angularjs/widget-loader/widgetloader.directive.js b/plugins/CoreHome/angularjs/widget-loader/widgetloader.directive.js
index b1c0c3a11d..4614f01bbf 100644
--- a/plugins/CoreHome/angularjs/widget-loader/widgetloader.directive.js
+++ b/plugins/CoreHome/angularjs/widget-loader/widgetloader.directive.js
@@ -114,7 +114,10 @@
                         }
 
                         if (piwik.shouldPropagateTokenAuth && broadcast.getValueFromUrl('token_auth')) {
-                            url += '&force_api_session=1&token_auth=' + broadcast.getValueFromUrl('token_auth');
+                            if (!piwik.broadcast.isWidgetizeRequestWithoutSession()) {
+                                url += '&force_api_session=1';
+                            }
+                            url += '&token_auth=' + encodeURIComponent(broadcast.getValueFromUrl('token_auth'));
                         }
 
                         url += '&random=' + parseInt(Math.random() * 10000);
diff --git a/plugins/CoreHome/javascripts/broadcast.js b/plugins/CoreHome/javascripts/broadcast.js
index badabd0811..7fc0b848d5 100644
--- a/plugins/CoreHome/javascripts/broadcast.js
+++ b/plugins/CoreHome/javascripts/broadcast.js
@@ -176,7 +176,6 @@ var broadcast = {
             }
         }
     },
-
     isWidgetizedDashboard: function() {
         return broadcast.getValueFromUrl('module') == 'Widgetize' && broadcast.getValueFromUrl('moduleToWidgetize') == 'Dashboard';
     },
diff --git a/plugins/CoreHome/javascripts/dataTable_rowactions.js b/plugins/CoreHome/javascripts/dataTable_rowactions.js
index 3481e28b7b..5283944e32 100644
--- a/plugins/CoreHome/javascripts/dataTable_rowactions.js
+++ b/plugins/CoreHome/javascripts/dataTable_rowactions.js
@@ -474,6 +474,7 @@ DataTable_RowActions_RowEvolution.prototype.showRowEvolution = function (apiMeth
 
     var ajaxRequest = new ajaxHelper();
     ajaxRequest.addParams(requestParams, 'get');
+    ajaxRequest.withTokenInUrl();
     ajaxRequest.setCallback(callback);
     ajaxRequest.setFormat('html');
     ajaxRequest.send();
diff --git a/plugins/Live/javascripts/SegmentedVisitorLog.js b/plugins/Live/javascripts/SegmentedVisitorLog.js
index 48bbb289cf..65d0121edc 100644
--- a/plugins/Live/javascripts/SegmentedVisitorLog.js
+++ b/plugins/Live/javascripts/SegmentedVisitorLog.js
@@ -135,6 +135,7 @@ var SegmentedVisitorLog = function() {
 
         var ajaxRequest = new ajaxHelper();
         ajaxRequest.addParams(requestParams, 'get');
+        ajaxRequest.withTokenInUrl();
         ajaxRequest.setCallback(callback);
         ajaxRequest.setFormat('html');
         ajaxRequest.send();
diff --git a/plugins/Live/javascripts/visitorProfile.js b/plugins/Live/javascripts/visitorProfile.js
index 2fdf092dfb..6f743221d4 100644
--- a/plugins/Live/javascripts/visitorProfile.js
+++ b/plugins/Live/javascripts/visitorProfile.js
@@ -156,7 +156,10 @@
             $element.on('mousedown', '.visitor-profile-export', function (e) {
                 var url = $(this).attr('href');
                 if (url.indexOf('&token_auth=') == -1) {
-                    $(this).attr('href', url + '&force_api_session=1&token_auth=' + piwik.token_auth);
+                    if (!piwik.broadcast.isWidgetizeRequestWithoutSession()) {
+                        url += '&force_api_session=1';
+                    }
+                    $(this).attr('href', url + '&token_auth=' + piwik.token_auth);
                 }
             });
 
diff --git a/plugins/Morpheus/icons b/plugins/Morpheus/icons
index 8d89ce17e1..f9a78253a2 160000
--- a/plugins/Morpheus/icons
+++ b/plugins/Morpheus/icons
@@ -1 +1 @@
-Subproject commit 8d89ce17e1006489b91664b24157a762c6d37174
+Subproject commit f9a78253a2851783a706b6e5d550a2261623feef
diff --git a/plugins/Overlay/javascripts/Overlay_Helper.js b/plugins/Overlay/javascripts/Overlay_Helper.js
index 6e843df816..d095768908 100644
--- a/plugins/Overlay/javascripts/Overlay_Helper.js
+++ b/plugins/Overlay/javascripts/Overlay_Helper.js
@@ -29,7 +29,10 @@ var Overlay_Helper = {
 
         var token_auth = piwik.broadcast.getValueFromUrl("token_auth");
         if (token_auth.length && piwik.shouldPropagateTokenAuth) {
-            url += '&force_api_session=1&token_auth='  + encodeURIComponent(token_auth);
+            if (!piwik.broadcast.isWidgetizeRequestWithoutSession()) {
+                url += '&force_api_session=1';
+            }
+            url += '&token_auth='  + encodeURIComponent(token_auth);
         }
 
         if (link) {
diff --git a/plugins/Overlay/javascripts/Piwik_Overlay.js b/plugins/Overlay/javascripts/Piwik_Overlay.js
index 49e5c95401..f33382fceb 100644
--- a/plugins/Overlay/javascripts/Piwik_Overlay.js
+++ b/plugins/Overlay/javascripts/Piwik_Overlay.js
@@ -50,6 +50,7 @@ var Piwik_Overlay = (function () {
         globalAjaxQueue.abort();
         var ajaxRequest = new ajaxHelper();
         ajaxRequest.addParams(params, 'get');
+        ajaxRequest.withTokenInUrl(); // needed because it is calling a controller and not the API
         ajaxRequest.setCallback(
             function (response) {
                 hideLoading();
diff --git a/plugins/Overlay/templates/index.twig b/plugins/Overlay/templates/index.twig
index e4a4c77441..a618224ce5 100644
--- a/plugins/Overlay/templates/index.twig
+++ b/plugins/Overlay/templates/index.twig
@@ -73,7 +73,10 @@
 
             var iframeSrc = 'index.php?module=Overlay&action=startOverlaySession&idSite={{ idSite }}&period={{ period }}&date={{ rawDate }}&segment={{ segment }}';
             if (piwik.shouldPropagateTokenAuth) {
-                iframeSrc += '&force_api_session=1&token_auth=' + piwik.token_auth;
+                if (!piwik.broadcast.isWidgetizeRequestWithoutSession()) {
+                    iframeSrc += '&force_api_session=1';
+                }
+                iframeSrc += '&token_auth=' + piwik.token_auth;
             }
 
             Piwik_Overlay.init(iframeSrc, '{{ idSite }}', '{{ period }}', '{{ rawDate }}', '{{ segment }}');
diff --git a/plugins/Overlay/templates/index_noframe.twig b/plugins/Overlay/templates/index_noframe.twig
index c3f32be6b6..5966a08ab2 100644
--- a/plugins/Overlay/templates/index_noframe.twig
+++ b/plugins/Overlay/templates/index_noframe.twig
@@ -8,7 +8,10 @@
     <script type="text/javascript">
         var newLocation = 'index.php?module=Overlay&action=startOverlaySession&idSite={{ idSite }}&period={{ period }}&date={{ date }}&segment={{ segment }}';
         if (piwik.shouldPropagateTokenAuth) {
-            newLocation += '&force_api_session=1&token_auth=' + piwik.token_auth;
+            if (!piwik.broadcast.isWidgetizeRequestWithoutSession()) {
+                newLocation += '&force_api_session=1';
+            }
+            newLocation += 'token_auth=' + piwik.token_auth;
         }
 
         var locationParts = window.location.href.split('#');

[tsteur]

@geekdenz there is plugins/Morpheus/icons which is maybe meant by it?

[geekdenz]

@geekdenz there is plugins/Morpheus/icons which is maybe meant by it?

Yes, thanks. Sorry I overlooked that and had done a git commit -am '...'. I don't know how to revert that submodule commit. Trying to find out now.

[geekdenz]

I believe the submodule should be in the same state now with the latest commit on 4.x-dev.

This is how I did it:

git checkout 4.x-dev 
git submodule init
git submodule update --recursive
git checkout m-17640 
git checkout 4.x-dev plugins/Morpheus/icons
git add plugins/Morpheus/icons
git commit -m 'revert git submodule to 4.x-dev version #17640'

[sgiehl]

Yes, thanks. Sorry I overlooked that and had done a git commit -am '...'.

@geekdenz Guess you should better try to avoid using git commit -a in this project. There might quite often be changes that you don't want to commit. e.g. some of our plugins are manipulating the javascript tracker resulting in a locally changed piwik.js file. Also when you manually change a file in a submodule and don't commit it (or commit, but don't push it), you might get a dirty submodule state, that breaks the checkout for it when commited, ....
Personally I always use git add -p to manually select the changes I want to add (this doesn't work for binary files), followed by a git commit -m

[geekdenz]

@geekdenz Guess you should better try to avoid using git commit -a in this project.
Personally I always use git add -p to manually select the changes I want to add (this doesn't work for binary files), followed by a git commit -m

Thanks! I wasn't aware of git add -p. I had already realised that I should get out of the habit of doing git commit -a.

Could you please review this PR and get it through the merge process, @sgiehl ?

[geekdenz]

I think we covered everything here now.

[sgiehl]

Left a suggestion. Otherwise looks quite good to merge.
I started preparing some additional UI tests to ensure those things won't fail in the future. But they require an update of Puppeteer (see #17880), so I will finish them once this PR and the Puppeteer update is merged