matomo-org · diosmosis · Feb 17, 2021 · Dec 29, 2020 · Dec 30, 2020 · Jan 16, 2021
diff --git a/core/CronArchive.php b/core/CronArchive.php
@@ -885,11 +885,11 @@ private function invalidateWithSegments($idSites, $date, $period, $_forceInvalid
             }
 
             foreach ($this->segmentArchiving->getAllSegmentsToArchive($idSite) as $segmentDefinition) {
-                $params = new Parameters(new Site($idSite), $periodObj, new Segment($segmentDefinition, [$idSite], $periodObj->getDateStart(), $periodObj->getDateEnd()));
+                $params = new Parameters(new Site($idSite), $periodObj, new Segment(urlencode($segmentDefinition), [$idSite], $periodObj->getDateStart(), $periodObj->getDateEnd()));
                 if ($this->isThereExistingValidPeriod($params)) {
                     $this->logger->debug('  Found usable archive for {archive}, skipping invalidation.', ['archive' => $params]);
                 } else {
-                    $this->getApiToInvalidateArchivedReport()->invalidateArchivedReports($idSite, $date, $period, $segmentDefinition,
+                    $this->getApiToInvalidateArchivedReport()->invalidateArchivedReports($idSite, $date, $period, urlencode($segmentDefinition),
                         $cascadeDown = false, $_forceInvalidateNonexistant);
                 }
             }

diff --git a/core/CronArchive/QueueConsumer.php b/core/CronArchive/QueueConsumer.php
@@ -510,6 +510,7 @@ private function findSegmentForArchive(&$archive)
         $hash = substr($flag, 4);
         $storedSegment = $this->segmentArchiving->findSegmentForHash($hash, $archive['idsite']);
         if (!isset($storedSegment['definition'])) {
+            $this->logger->debug("Could not find stored segment for done flag hash: $flag");
             $archive['segment'] = null;
             return false;
         }

diff --git a/core/CronArchive/SegmentArchiving.php b/core/CronArchive/SegmentArchiving.php
@@ -77,20 +77,20 @@ public function __construct($beginningOfTimeLastNInYears = self::DEFAULT_BEGINNI
         $this->segmentListCache = $segmentListCache ?: new Transient();
         $this->now = $now ?: Date::factory('now');
         $this->logger = $logger ?: StaticContainer::get('Psr\Log\LoggerInterface');
-        $this->forceArchiveAllSegments = $this->getShouldForceArchiveAllSegments();
+        $this->forceArchiveAllSegments = self::getShouldForceArchiveAllSegments();
     }
 
     public function findSegmentForHash($hash, $idSite)
     {
         foreach ($this->getAllSegments() as $segment) {
             if (!$this->isAutoArchivingEnabledFor($segment)
-                || !$this->isSegmentForSite($segment, $idSite)
+                || !self::isSegmentForSite($segment, $idSite)
             ) {
                 continue;
             }
 
             try {
-                $segmentObj = new Segment($segment['definition'], [$idSite]);
+                $segmentObj = new Segment(urlencode($segment['definition']), [$idSite]);
             } catch (\Exception $ex) {
                 $this->logger->debug("Could not process segment {$segment['definition']} for site {$idSite}. Segment should not exist for the site, but does.");
                 continue;
@@ -205,7 +205,7 @@ public function getAllSegmentsToArchive($idSite)
         return Rules::getSegmentsToProcess([$idSite]);
     }
 
-    private function isSegmentForSite($segment, $idSite)
+    public static function isSegmentForSite($segment, $idSite)
     {
         return $segment['enable_only_idsite'] == 0
             || $segment['enable_only_idsite'] == $idSite;
@@ -216,7 +216,7 @@ public function isAutoArchivingEnabledFor($storedSegment)
         return $this->forceArchiveAllSegments || !empty($storedSegment['auto_archive']);
     }
 
-    private function getShouldForceArchiveAllSegments()
+    public static function getShouldForceArchiveAllSegments()
     {
         return !Rules::isBrowserTriggerEnabled() && !Rules::isBrowserArchivingAvailableForSegments();
     }
@@ -227,7 +227,7 @@ public function reArchiveSegment($segmentInfo)
             return;
         }
 
-        $definition = $segmentInfo['definition'];
+        $definition = urlencode($segmentInfo['definition']);
         $idSite = $segmentInfo['enable_only_idsite'] ?? 'all';
 
         $idSites = Access::doAsSuperUser(function () use ($idSite) {

diff --git a/core/DataAccess/Model.php b/core/DataAccess/Model.php
@@ -621,7 +621,7 @@ public function getArchiveIdsForSegments($archiveTableName, array $segments, $ol
     private function getDeletedSegmentWhereClause(array $segment)
     {
         $idSite = (int)$segment['enable_only_idsite'];
-        $segmentHash = Segment::getSegmentHash($segment['definition']);
+        $segmentHash = Segment::getSegmentHash(urlencode($segment['definition']));
         // Valid segment hashes are md5 strings - just confirm that it is so it's safe for SQL injection
         if (!ctype_xdigit($segmentHash)) {
             throw new Exception($segment . ' expected to be an md5 hash');

diff --git a/plugins/CoreConsole/tests/System/ArchiveCronTest.php b/plugins/CoreConsole/tests/System/ArchiveCronTest.php
@@ -8,6 +8,7 @@
 namespace Piwik\Plugins\CoreConsole\tests\System;
 
 use Piwik\CronArchive;
+use Piwik\Http;
 use Piwik\Plugins\SegmentEditor\API;
 use Piwik\Site;
 use Piwik\Tests\Framework\TestingEnvironmentVariables;
@@ -60,11 +61,35 @@ public static function setUpBeforeClass(): void
     private static function addNewSegmentToPast()
     {
         Config::getInstance()->General['enable_browser_archiving_triggering'] = 0;
+        self::$fixture->getTestEnvironment()->overrideConfig('General', 'enable_browser_archiving_triggering', 0);
+        self::$fixture->getTestEnvironment()->save();
+
         // add one segment and set it's created/updated time to some time in the past so we don't re-archive for it
         $idSegment = API::getInstance()->add(self::NEW_SEGMENT_NAME, self::NEW_SEGMENT, self::$fixture->idSite, $autoArchive = 1, $enabledAllUsers = 1);
-        // add another segment w/ special encoded value
-        $idSegment2 = API::getInstance()->add(self::ENCODED_SEGMENT_NAME, self::ENCODED_SEGMENT, self::$fixture->idSite, $autoArchive = 1, $enabledAllUsers = 1);
+
+        // add another segment w/ special encoded value (but do it through the API which will use a definition query param which is not
+        // encoded the same as Request::getRawSegmentFromRequest()
+        $url = Fixture::getTestRooturl("") . '?' . http_build_query([
+            'module' => 'API',
+            'method' => 'SegmentEditor.add',
+            'name' => self::ENCODED_SEGMENT_NAME,
+            'definition' => self::ENCODED_SEGMENT,
+            'idSite' => self::$fixture->idSite,
+            'autoArchive' => 1,
+            'enabledAllUsers' => 1,
+            'format' => 'json',
+            'token_auth' => Fixture::getTokenAuth(),
+        ]);
+        self::assertStringContainsString(urlencode(self::ENCODED_SEGMENT), $url);
+
+        $idSegment2 = Http::sendHttpRequest($url, 10);
+        $idSegment2 = json_decode($idSegment2, $isAssoc = true);
+        $idSegment2 = $idSegment2['value'];
+
         Config::getInstance()->General['enable_browser_archiving_triggering'] = 1;
+        self::$fixture->getTestEnvironment()->overrideConfig('General', 'enable_browser_archiving_triggering', 1);
+        self::$fixture->getTestEnvironment()->save();
+
         Db::exec("UPDATE " . Common::prefixTable('segment') . ' SET ts_created = \'2015-01-02 00:00:00\', ts_last_edit = \'2015-01-02 00:00:00\' WHERE idsegment IN (' . $idSegment . ", " . $idSegment2 . ")");
     }
 
@@ -164,6 +189,7 @@ public function testArchivePhpCron()
     {
         self::$fixture->getTestEnvironment()->overrideConfig('General', 'enable_browser_archiving_triggering', 0);
         self::$fixture->getTestEnvironment()->overrideConfig('General', 'browser_archiving_disabled_enforce', 1);
+        self::$fixture->getTestEnvironment()->save();
 
         Config::getInstance()->General['enable_browser_archiving_triggering'] = 0;
         Config::getInstance()->General['browser_archiving_disabled_enforce'] = 1;
@@ -197,12 +223,6 @@ public function testArchivePhpCron()
             self::undoForceCurlCliMulti();
         }
 
-        $expectedInvalidations = [];
-        $invalidationEntries = $this->getInvalidatedArchiveTableEntries();
-        $this->assertEquals($expectedInvalidations, $invalidationEntries);
-
-        $this->compareArchivePhpOutputAgainstExpected($output);
-
         foreach ($this->getApiForTesting() as $testInfo) {
 
             list($api, $params) = $testInfo;
@@ -228,6 +248,7 @@ public function testArchivePhpCronWithSingleReportRearchive()
     {
         self::$fixture->getTestEnvironment()->overrideConfig('General', 'enable_browser_archiving_triggering', 0);
         self::$fixture->getTestEnvironment()->overrideConfig('General', 'browser_archiving_disabled_enforce', 1);
+        self::$fixture->getTestEnvironment()->save();
 
         Config::getInstance()->General['enable_browser_archiving_triggering'] = 0;
         Config::getInstance()->General['browser_archiving_disabled_enforce'] = 1;
@@ -323,24 +344,6 @@ private function runArchivePhpCron($options = array(), $archivePhpScript = false
         return $output;
     }
 
-    private function compareArchivePhpOutputAgainstExpected($output)
-    {
-        $fileName = 'test_ArchiveCronTest_archive_php_cron_output.txt';
-        list($pathProcessed, $pathExpected) = static::getProcessedAndExpectedDirs();
-
-        $expectedOutputFile = $pathExpected . $fileName;
-        $processedFile = $pathProcessed . $fileName;
-
-        file_put_contents($processedFile, $output);
-
-        try {
-            $this->assertTrue(is_readable($expectedOutputFile));
-            $this->assertEquals(file_get_contents($expectedOutputFile), $output);
-        } catch (Exception $ex) {
-            $this->comparisonFailures[] = $ex;
-        }
-    }
-
     public static function provideContainerConfigBeforeClass()
     {
         return array(

diff --git a/...m/expected/test_ArchiveCronTest_segmentNoAutoArchive_noOptions__VisitsSummary.get_day.xml b/...m/expected/test_ArchiveCronTest_segmentNoAutoArchive_noOptions__VisitsSummary.get_day.xml
@@ -1,6 +1,18 @@
 <?xml version="1.0" encoding="utf-8" ?>
 <results>
-	<result idSite="1" />
+	<result idSite="1">
+		<nb_uniq_visitors>4</nb_uniq_visitors>
+		<nb_users>0</nb_users>
+		<nb_visits>4</nb_visits>
+		<nb_actions>4</nb_actions>
+		<nb_visits_converted>4</nb_visits_converted>
+		<bounce_count>4</bounce_count>
+		<sum_visit_length>0</sum_visit_length>
+		<max_actions>1</max_actions>
+		<bounce_rate>100%</bounce_rate>
+		<nb_actions_per_visit>1</nb_actions_per_visit>
+		<avg_time_on_site>0</avg_time_on_site>
+	</result>
 	<result idSite="2" />
 	<result idSite="3" />
 </results>
diff --git a/...expected/test_ArchiveCronTest_segmentNoAutoArchive_noOptions__VisitsSummary.get_month.xml b/...expected/test_ArchiveCronTest_segmentNoAutoArchive_noOptions__VisitsSummary.get_month.xml
@@ -1,6 +1,42 @@
 <?xml version="1.0" encoding="utf-8" ?>
 <results>
-	<result idSite="1" />
-	<result idSite="2" />
-	<result idSite="3" />
+	<result idSite="1">
+		<nb_uniq_visitors>12</nb_uniq_visitors>
+		<nb_users>0</nb_users>
+		<nb_visits>12</nb_visits>
+		<nb_actions>15</nb_actions>
+		<nb_visits_converted>11</nb_visits_converted>
+		<bounce_count>10</bounce_count>
+		<sum_visit_length>305</sum_visit_length>
+		<max_actions>3</max_actions>
+		<bounce_rate>83%</bounce_rate>
+		<nb_actions_per_visit>1.3</nb_actions_per_visit>
+		<avg_time_on_site>25</avg_time_on_site>
+	</result>
+	<result idSite="2">
+		<nb_uniq_visitors>1</nb_uniq_visitors>
+		<nb_users>0</nb_users>
+		<nb_visits>1</nb_visits>
+		<nb_actions>1</nb_actions>
+		<nb_visits_converted>0</nb_visits_converted>
+		<bounce_count>1</bounce_count>
+		<sum_visit_length>0</sum_visit_length>
+		<max_actions>1</max_actions>
+		<bounce_rate>100%</bounce_rate>
+		<nb_actions_per_visit>1</nb_actions_per_visit>
+		<avg_time_on_site>0</avg_time_on_site>
+	</result>
+	<result idSite="3">
+		<nb_uniq_visitors>1</nb_uniq_visitors>
+		<nb_users>0</nb_users>
+		<nb_visits>1</nb_visits>
+		<nb_actions>3</nb_actions>
+		<nb_visits_converted>0</nb_visits_converted>
+		<bounce_count>0</bounce_count>
+		<sum_visit_length>1</sum_visit_length>
+		<max_actions>3</max_actions>
+		<bounce_rate>0%</bounce_rate>
+		<nb_actions_per_visit>3</nb_actions_per_visit>
+		<avg_time_on_site>1</avg_time_on_site>
+	</result>
 </results>
diff --git a/.../expected/test_ArchiveCronTest_segmentNoAutoArchive_noOptions__VisitsSummary.get_week.xml b/.../expected/test_ArchiveCronTest_segmentNoAutoArchive_noOptions__VisitsSummary.get_week.xml
@@ -1,6 +1,18 @@
 <?xml version="1.0" encoding="utf-8" ?>
 <results>
-	<result idSite="1" />
+	<result idSite="1">
+		<nb_uniq_visitors>11</nb_uniq_visitors>
+		<nb_users>0</nb_users>
+		<nb_visits>11</nb_visits>
+		<nb_actions>14</nb_actions>
+		<nb_visits_converted>10</nb_visits_converted>
+		<bounce_count>9</bounce_count>
+		<sum_visit_length>305</sum_visit_length>
+		<max_actions>3</max_actions>
+		<bounce_rate>82%</bounce_rate>
+		<nb_actions_per_visit>1.3</nb_actions_per_visit>
+		<avg_time_on_site>28</avg_time_on_site>
+	</result>
 	<result idSite="2" />
 	<result idSite="3" />
 </results>
diff --git a/.../expected/test_ArchiveCronTest_segmentNoAutoArchive_noOptions__VisitsSummary.get_year.xml b/.../expected/test_ArchiveCronTest_segmentNoAutoArchive_noOptions__VisitsSummary.get_year.xml
@@ -1,6 +1,36 @@
 <?xml version="1.0" encoding="utf-8" ?>
 <results>
-	<result idSite="1" />
-	<result idSite="2" />
-	<result idSite="3" />
+	<result idSite="1">
+		<nb_visits>12</nb_visits>
+		<nb_actions>15</nb_actions>
+		<nb_visits_converted>11</nb_visits_converted>
+		<bounce_count>10</bounce_count>
+		<sum_visit_length>305</sum_visit_length>
+		<max_actions>3</max_actions>
+		<bounce_rate>83%</bounce_rate>
+		<nb_actions_per_visit>1.3</nb_actions_per_visit>
+		<avg_time_on_site>25</avg_time_on_site>
+	</result>
+	<result idSite="2">
+		<nb_visits>1</nb_visits>
+		<nb_actions>1</nb_actions>
+		<nb_visits_converted>0</nb_visits_converted>
+		<bounce_count>1</bounce_count>
+		<sum_visit_length>0</sum_visit_length>
+		<max_actions>1</max_actions>
+		<bounce_rate>100%</bounce_rate>
+		<nb_actions_per_visit>1</nb_actions_per_visit>
+		<avg_time_on_site>0</avg_time_on_site>
+	</result>
+	<result idSite="3">
+		<nb_visits>1</nb_visits>
+		<nb_actions>3</nb_actions>
+		<nb_visits_converted>0</nb_visits_converted>
+		<bounce_count>0</bounce_count>
+		<sum_visit_length>1</sum_visit_length>
+		<max_actions>3</max_actions>
+		<bounce_rate>0%</bounce_rate>
+		<nb_actions_per_visit>3</nb_actions_per_visit>
+		<avg_time_on_site>1</avg_time_on_site>
+	</result>
 </results>
diff --git a/plugins/ExamplePlugin/API.php b/plugins/ExamplePlugin/API.php
@@ -11,6 +11,8 @@
 use Piwik\Archive;
 use Piwik\DataTable;
 use Piwik\DataTable\Row;
+use Piwik\Piwik;
+use Piwik\Segment;
 
 /**
  * API for plugin ExamplePlugin
@@ -48,6 +50,8 @@ public function getAnswerToLife($truth = true)
      */
     public function getExampleReport($idSite, $period, $date, $segment = false)
     {
+        Piwik::checkUserHasViewAccess($idSite);
+
         $table = DataTable::makeFromSimpleArray(array(
             array('label' => 'My Label 1', 'nb_visits' => '1'),
             array('label' => 'My Label 2', 'nb_visits' => '5'),
@@ -66,8 +70,18 @@ public function getExampleReport($idSite, $period, $date, $segment = false)
      */
     public function getExampleArchivedMetric($idSite, $period, $date, $segment = false)
     {
+        Piwik::checkUserHasViewAccess($idSite);
+
         $archive = Archive::build($idSite, $period, $date, $segment);
         $dataTable = $archive->getDataTableFromNumeric([Archiver::EXAMPLEPLUGIN_METRIC_NAME, Archiver::EXAMPLEPLUGIN_CONST_METRIC_NAME]);
         return $dataTable;
     }
+
+    public function getSegmentHash($idSite, $segment)
+    {
+        Piwik::checkUserHasViewAccess($idSite);
+
+        $segment = new Segment($segment, [(int) $idSite]);
+        return $segment->getHash();
+    }
 }
diff --git a/plugins/SegmentEditor/API.php b/plugins/SegmentEditor/API.php
@@ -390,6 +390,14 @@ public function getAll($idSite = false)
 
         $segments = $this->sortSegmentsCreatedByUserFirst($segments);
 
+        $model = new \Piwik\Plugins\SitesManager\Model();
+        $allIdSites = $model->getSitesId();
+        foreach ($segments as &$segmentInfo) {
+            $idSites = !empty($segmentInfo['enable_only_idsite']) ? [(int) $segmentInfo['enable_only_idsite']] : $allIdSites;
+            $segmentObj = new Segment(urlencode($segmentInfo['definition']), $idSites);
+            $segmentInfo['hash'] = $segmentObj->getHash();
+        }
+
         return $segments;
     }
 

diff --git a/plugins/SegmentEditor/SegmentEditor.php b/plugins/SegmentEditor/SegmentEditor.php
@@ -16,6 +16,7 @@
 use Piwik\Common;
 use Piwik\Config;
 use Piwik\Container\StaticContainer;
+use Piwik\CronArchive\SegmentArchiving;
 use Piwik\DataAccess\ArchiveSelector;
 use Piwik\Notification;
 use Piwik\Piwik;
@@ -122,7 +123,20 @@ public function getKnownSegmentsToArchiveAllSites(&$segments)
     public function getKnownSegmentsToArchiveForSite(&$segments, $idSite)
     {
         $model = new Model();
-        $segmentToAutoArchive = $model->getSegmentsToAutoArchive($idSite);
+        $segmentToAutoArchive = $model->getAllSegmentsAndIgnoreVisibility();
+
+        $forceAutoArchive = SegmentArchiving::getShouldForceArchiveAllSegments();
+        foreach ($segmentToAutoArchive as $index => $segmentInfo) {
+            if (!SegmentArchiving::isSegmentForSite($segmentInfo, $idSite)) {
+                unset($segmentToAutoArchive[$index]);
+            }
+
+            if (!$forceAutoArchive
+                && empty($segmentInfo['auto_archive'])
+            ) {
+                unset($segmentToAutoArchive[$index]);
+            }
+        }
 
         foreach ($segmentToAutoArchive as $segmentInfo) {
             $segments[] = $segmentInfo['definition'];