@geekdenz opened this Pull Request on August 3rd 2021 Contributor

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
@geekdenz commented on August 5th 2021 Contributor

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 commented on August 5th 2021 Member

sounds good @geekdenz 👍

@geekdenz commented on August 5th 2021 Contributor

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

@sgiehl commented on August 9th 2021 Member

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

@geekdenz commented on August 9th 2021 Contributor

@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();
+    }
+
+    /**
+     * <a class='mention' href='https://github.com/param'>@param</a> bool $return the token_auth $_GET variable
+     * <a class='mention' href='https://github.com/return'>@return</a> bool|string
+     * <a class='mention' href='https://github.com/throws'>@throws</a> 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 commented on August 9th 2021 Member

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

@geekdenz commented on August 9th 2021 Contributor

@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.

@sgiehl commented on August 10th 2021 Member

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 commented on August 10th 2021 Contributor

@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 ?

This Pull Request was closed on August 13th 2021
Powered by GitHub Issue Mirror