From 16f18f29c3c77e185af6f9df53e258cb6cb2ce00 Mon Sep 17 00:00:00 2001 From: Altamash Shaikh Date: Fri, 11 Oct 2024 21:33:36 +0530 Subject: [PATCH 1/3] Adds test for PHPCS --- .github/workflows/phpcs.yml | 43 +++++++++++++++++++++++++++++++++++++ phpcs.xml | 36 +++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 .github/workflows/phpcs.yml create mode 100644 phpcs.xml diff --git a/.github/workflows/phpcs.yml b/.github/workflows/phpcs.yml new file mode 100644 index 0000000..c37a899 --- /dev/null +++ b/.github/workflows/phpcs.yml @@ -0,0 +1,43 @@ +name: PHPCS check + +on: pull_request + +permissions: + actions: read + checks: read + contents: read + deployments: none + issues: read + packages: none + pull-requests: read + repository-projects: none + security-events: none + statuses: read + +jobs: + phpcs: + name: PHPCS + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + lfs: false + persist-credentials: false + - name: Setup PHP + uses: shivammathur/setup-php@v2 + with: + php-version: '7.4' + tools: cs2pr + - name: Install dependencies + run: + composer init --name=matomo/trackingspamprevention --quiet; + composer --no-plugins config allow-plugins.dealerdirect/phpcodesniffer-composer-installer true -n; + composer config repositories.matomo-coding-standards vcs https://github.com/matomo-org/matomo-coding-standards -n; + composer require matomo-org/matomo-coding-standards:dev-master; + composer install --dev --prefer-dist --no-progress --no-suggest + - name: Check PHP code styles + id: phpcs + run: ./vendor/bin/phpcs --report-full --standard=phpcs.xml --report-checkstyle=./phpcs-report.xml + - name: Show PHPCS results in PR + if: ${{ always() && steps.phpcs.outcome == 'failure' }} + run: cs2pr ./phpcs-report.xml --prepend-filename diff --git a/phpcs.xml b/phpcs.xml new file mode 100644 index 0000000..d00a071 --- /dev/null +++ b/phpcs.xml @@ -0,0 +1,36 @@ + + + + Matomo Coding Standard for TrackingSpamPrevention plugin + + + + . + + tests/javascript/* + */vendor/* + + + + + + + + tests/* + + + + + Updates/* + + + + + tests/* + + + + + tests/* + + \ No newline at end of file From 06d66c1687dd9bfdc7f99284134117c43a2595ae Mon Sep 17 00:00:00 2001 From: Jacob Ransom Date: Mon, 21 Oct 2024 10:24:32 +1300 Subject: [PATCH 2/3] Making PHPCS changes --- AllowListIpRange.php | 2 +- BanIpNotificationEmail.php | 19 ++++++++------- BlockedGeoIp.php | 2 +- BlockedIpRanges.php | 7 +++--- BlockedIpRanges/Aws.php | 18 +++++++++++--- BlockedIpRanges/Azure.php | 12 ++++++---- BlockedIpRanges/DigitalOcean.php | 2 +- BlockedIpRanges/ExceptionRange.php | 3 +-- BlockedIpRanges/Gcloud.php | 2 +- BlockedIpRanges/IpRangeProviderInterface.php | 2 +- BlockedIpRanges/Oracle.php | 2 +- BlockedIpRanges/VariableRange.php | 2 +- BrowserDetection.php | 2 +- Commands/BlockGeoIpOrganisation.php | 3 ++- Configuration.php | 14 ++++++----- Settings/BlockCloudsSetting.php | 2 ++ SystemSettings.php | 17 ++++++++----- Tasks.php | 2 +- Tracker/RequestProcessor.php | 2 +- TrackingSpamPrevention.php | 24 +++++++++++++------ config/config.php | 9 +++---- config/test.php | 3 ++- config/tracker.php | 3 ++- tests/Fixtures/TrackingFixture.php | 3 ++- tests/Integration/AllowListIpRangeTest.php | 3 +-- tests/Integration/BlockedGeoIpTest.php | 8 +++---- .../BlockedIpRanges/ProvidersTest.php | 2 +- tests/Integration/BlockedIpRangesTest.php | 2 +- tests/Integration/BrowserDetectionTest.php | 4 +++- tests/Integration/ConfigurationTest.php | 3 +-- tests/Integration/NotificationEmailTest.php | 3 +-- tests/Integration/SystemSettingsTest.php | 2 +- tests/Integration/TasksTest.php | 2 +- .../Tracker/RequestProcessorTest.php | 2 +- .../TrackingSpamPreventionTest.php | 13 ++++++---- .../CheckDirectDependencyUseCommandTest.php | 3 ++- 36 files changed, 125 insertions(+), 79 deletions(-) diff --git a/AllowListIpRange.php b/AllowListIpRange.php index 2b4f307..0ffedd3 100644 --- a/AllowListIpRange.php +++ b/AllowListIpRange.php @@ -1,4 +1,5 @@ addTo($email); $mail->setSubject('An IP was banned as too many actions were tracked.'); $mail->setDefaultFromPiwik(); - if (empty($mail->getFromName()) || in_array($mail->getFromName(), [ + if ( + empty($mail->getFromName()) || in_array($mail->getFromName(), [ 'CoreHome_WebAnalyticsReports', 'TagManager_MatomoTagName' - ])) { + ]) + ) { $mail->setFrom($mail->getFrom(), 'Web Analytics Reports'); } $mailBody = 'This is for your information. The following IP was banned because visit tried to track more than ' . Common::sanitizeInputValue($maxActionsAllowed) . ' actions:'; - $mailBody .= PHP_EOL.PHP_EOL.'"' . Common::sanitizeInputValue($ipRange) . '"'.PHP_EOL; + $mailBody .= PHP_EOL . PHP_EOL . '"' . Common::sanitizeInputValue($ipRange) . '"' . PHP_EOL; $instanceId = SettingsPiwik::getPiwikInstanceId(); @@ -58,12 +60,12 @@ public function send($ipRange, $ip, $email, $maxActionsAllowed, $locationData, $ } if (!empty($instanceId)) { - $mailBody .= PHP_EOL.'Instance ID: ' . Common::sanitizeInputValue($instanceId); + $mailBody .= PHP_EOL . 'Instance ID: ' . Common::sanitizeInputValue($instanceId); } - $mailBody .= PHP_EOL.'Current date (UTC): ' . Common::sanitizeInputValue($nowDateTime) . ' + $mailBody .= PHP_EOL . 'Current date (UTC): ' . Common::sanitizeInputValue($nowDateTime) . ' IP as detected in header: ' . Common::sanitizeInputValue($ip) . ' GET request info: ' . json_encode($get) . ' -POST request info: ' . json_encode($post). PHP_EOL; +POST request info: ' . json_encode($post) . PHP_EOL; if (!empty($locationData)) { $mailBody .= 'Geo IP info: ' . json_encode($locationData) . PHP_EOL; @@ -82,9 +84,8 @@ public function send($ipRange, $ip, $email, $maxActionsAllowed, $locationData, $ $mail->send(); } - $a=$mail->getBodyText(); + $a = $mail->getBodyText(); return $mail->getBodyText(); } - } diff --git a/BlockedGeoIp.php b/BlockedGeoIp.php index 91006a8..0cfd59e 100644 --- a/BlockedGeoIp.php +++ b/BlockedGeoIp.php @@ -1,4 +1,5 @@ setBlockedRanges([]); } @@ -190,5 +192,4 @@ public function updateBlockedIpRanges() } $this->setBlockedRanges($indexedRange); } - } diff --git a/BlockedIpRanges/Aws.php b/BlockedIpRanges/Aws.php index 81387db..1298d22 100644 --- a/BlockedIpRanges/Aws.php +++ b/BlockedIpRanges/Aws.php @@ -1,4 +1,5 @@ ranges; } - } diff --git a/BrowserDetection.php b/BrowserDetection.php index e76a752..481b871 100644 --- a/BrowserDetection.php +++ b/BrowserDetection.php @@ -1,4 +1,5 @@ isLibrary(); } - } diff --git a/Commands/BlockGeoIpOrganisation.php b/Commands/BlockGeoIpOrganisation.php index 131befe..302fa1c 100644 --- a/Commands/BlockGeoIpOrganisation.php +++ b/Commands/BlockGeoIpOrganisation.php @@ -1,4 +1,5 @@ getOption('organisation-name'); $pluginConfig[Configuration::KEY_GEOIP_MATCH_PROVIDERS][] = mb_strtolower(trim($name)); - + $pluginConfig[Configuration::KEY_GEOIP_MATCH_PROVIDERS] = array_values(array_unique($pluginConfig[Configuration::KEY_GEOIP_MATCH_PROVIDERS])); $config->TrackingSpamPrevention = $pluginConfig; diff --git a/Configuration.php b/Configuration.php index 91f858c..362be3e 100644 --- a/Configuration.php +++ b/Configuration.php @@ -1,23 +1,25 @@ makeSetting('excluded_countries', [], FieldConfig::TYPE_ARRAY, function (FieldConfig $field) { $field->title = Piwik::translate('TrackingSpamPrevention_SettingExcludedCountriesTitle'); $field->description = Piwik::translate('TrackingSpamPrevention_SettingExcludedCountriesDescription'); @@ -161,7 +163,8 @@ public function transformCountryList($value) return $value; } - private function createIncludedCountriesSetting() { + private function createIncludedCountriesSetting() + { return $this->makeSetting('included_countries', [], FieldConfig::TYPE_ARRAY, function (FieldConfig $field) { $field->title = Piwik::translate('TrackingSpamPrevention_SettingIncludedCountriesTitle'); $field->description = Piwik::translate('TrackingSpamPrevention_SettingIncludedCountriesDescription'); @@ -190,7 +193,8 @@ private function createIncludedCountriesSetting() { }); } - private function createBlockServerSideLibrariesSetting() { + private function createBlockServerSideLibrariesSetting() + { return $this->makeSetting('blockServerSideLibraries', false, FieldConfig::TYPE_BOOL, function (FieldConfig $field) { $field->title = Piwik::translate('TrackingSpamPrevention_SettingBlockServerSideLibrariesTitle'); $field->inlineHelp = Piwik::translate('TrackingSpamPrevention_SettingBlockServerSideLibrariesDescription', array('','','
')); @@ -199,7 +203,8 @@ private function createBlockServerSideLibrariesSetting() { } - private function listCountries() { + private function listCountries() + { $regionDataProvider = StaticContainer::get(RegionDataProvider::class); $countryList = $regionDataProvider->getCountryList(); array_walk($countryList, function (&$item, $key) { @@ -219,7 +224,8 @@ public function getIncludedCountryCodes() return $this->settingToCountryCodes($this->includedCountries); } - private function settingToCountryCodes(Setting $setting) { + private function settingToCountryCodes(Setting $setting) + { $val = $setting->getValue(); if (empty($val) || !is_array($val)) { @@ -234,5 +240,4 @@ private function settingToCountryCodes(Setting $setting) { } return $codes; } - } diff --git a/Tasks.php b/Tasks.php index afb85ac..e52f4d8 100644 --- a/Tasks.php +++ b/Tasks.php @@ -1,4 +1,5 @@ blockedIpRanges->unsetAllIpRanges(); } } - } diff --git a/Tracker/RequestProcessor.php b/Tracker/RequestProcessor.php index d48e897..66ba90f 100644 --- a/Tracker/RequestProcessor.php +++ b/Tracker/RequestProcessor.php @@ -1,4 +1,5 @@ getBrowserLanguage(); $browserDetection = new BrowserDetection(); - if ($settings->blockHeadless->getValue() - && $browserDetection->isHeadlessBrowser($request->getUserAgent())) { + if ( + $settings->blockHeadless->getValue() + && $browserDetection->isHeadlessBrowser($request->getUserAgent()) + ) { // note above user agent could have been overwritten with UA parameter but that's fine since it's easy to change useragent anyway Common::printDebug("Excluding visit as headless browser detected"); $excluded = 'excluded: headless browser'; return; } - if ($settings->block_clouds->getValue() - && $blockGeoIp->isExcludedProvider($ipString, $browserLang)) { + if ( + $settings->block_clouds->getValue() + && $blockGeoIp->isExcludedProvider($ipString, $browserLang) + ) { // only needs to be done when cloud providers are blocked specifically Common::printDebug("Excluding visit as geoip detects a cloud provider"); $excluded = 'excluded: geoip cloud provider'; @@ -118,8 +123,14 @@ public function isExcludedVisit(&$excluded, Request $request) return; } - if ($blockGeoIp->isExcludedCountry($ipString, $browserLang, - $settings->getExcludedCountryCodes(), $settings->getIncludedCountryCodes())) { + if ( + $blockGeoIp->isExcludedCountry( + $ipString, + $browserLang, + $settings->getExcludedCountryCodes(), + $settings->getIncludedCountryCodes() + ) + ) { Common::printDebug("Excluding visit as geoip detects an excluded (or not included) country"); $excluded = 'excluded: country'; return; @@ -154,5 +165,4 @@ public function isTrackerPlugin() { return true; } - } diff --git a/config/config.php b/config/config.php index 18ab8ca..1795534 100644 --- a/config/config.php +++ b/config/config.php @@ -1,4 +1,5 @@ Piwik\DI::add(array( @@ -15,10 +16,10 @@ 'Piwik\Plugins\TrackingSpamPrevention\BlockedGeoIp' => Piwik\DI::autowire() ->constructor(Piwik\DI::get('trackingspam.block_geoip_organisations')), - 'trackingspam.block_geoip_organisations' => function (\Piwik\Container\Container $c) { - if ($c->has('ini.TrackingSpamPrevention.block_geoip_organisations')) { - return $c->get('ini.TrackingSpamPrevention.block_geoip_organisations'); - } + 'trackingspam.block_geoip_organisations' => function (\Piwik\Container\Container $c) { + if ($c->has('ini.TrackingSpamPrevention.block_geoip_organisations')) { + return $c->get('ini.TrackingSpamPrevention.block_geoip_organisations'); + } return []; }, diff --git a/config/test.php b/config/test.php index 723eda6..25a78f9 100644 --- a/config/test.php +++ b/config/test.php @@ -1,4 +1,5 @@ - array( diff --git a/config/tracker.php b/config/tracker.php index 67107a3..9a25907 100644 --- a/config/tracker.php +++ b/config/tracker.php @@ -1,3 +1,4 @@ -assertFalse($this->allowList->isAllowed('2001:db8:0000:0001:ffff:ffff:ffff:fffe')); $this->assertFalse($this->allowList->isAllowed('2002:db8:0000:0000:ffff:ffff:ffff:fffe')); } - - } diff --git a/tests/Integration/BlockedGeoIpTest.php b/tests/Integration/BlockedGeoIpTest.php index 38064a4..4533e10 100644 --- a/tests/Integration/BlockedGeoIpTest.php +++ b/tests/Integration/BlockedGeoIpTest.php @@ -1,4 +1,5 @@ assertEquals([ 'country_code' => 'xx', - 'continent_code' => 'unk', - 'continent_name' => 'General_Unknown', - 'country_name' => 'General_Unknown'], $this->blockedGeoIp->detectLocation('127.0.0.1', 'en')); + 'continent_code' => 'unk', + 'continent_name' => 'General_Unknown', + 'country_name' => 'General_Unknown'], $this->blockedGeoIp->detectLocation('127.0.0.1', 'en')); } public function test_isExcludedCountry_noCountriesGiven() @@ -68,5 +69,4 @@ public function test_isExcluded_When_UserCountryPluginIsDisabled() \Piwik\Plugin\Manager::getInstance()->deactivatePlugin('UserCountry'); $this->assertFalse($this->blockedGeoIp->isExcludedProvider('127.0.0.1', 'en')); } - } diff --git a/tests/Integration/BlockedIpRanges/ProvidersTest.php b/tests/Integration/BlockedIpRanges/ProvidersTest.php index 8ab8da3..e0dcf82 100644 --- a/tests/Integration/BlockedIpRanges/ProvidersTest.php +++ b/tests/Integration/BlockedIpRanges/ProvidersTest.php @@ -1,4 +1,5 @@ assertFalse($this->ranges->isExcluded('2001:db8:0000:0000:ffff:ffff:ffff:fffe')); $this->assertFalse($this->ranges->isExcluded('2002:db8:0000:0000:ffff:ffff:ffff:fffe')); } - } diff --git a/tests/Integration/BrowserDetectionTest.php b/tests/Integration/BrowserDetectionTest.php index 5a2784c..079c788 100644 --- a/tests/Integration/BrowserDetectionTest.php +++ b/tests/Integration/BrowserDetectionTest.php @@ -1,4 +1,5 @@ assertSame($expected, $this->browser->isLibrary($userAgent)); } diff --git a/tests/Integration/ConfigurationTest.php b/tests/Integration/ConfigurationTest.php index edb25ca..c21d06f 100644 --- a/tests/Integration/ConfigurationTest.php +++ b/tests/Integration/ConfigurationTest.php @@ -1,4 +1,5 @@ assertSame([], $this->configuration->getIpRangesAlwaysAllowed()); } - - } diff --git a/tests/Integration/NotificationEmailTest.php b/tests/Integration/NotificationEmailTest.php index b0df4fc..274d7ac 100644 --- a/tests/Integration/NotificationEmailTest.php +++ b/tests/Integration/NotificationEmailTest.php @@ -1,4 +1,5 @@ task->updateBlockedIpRanges(); $this->assertSame(['10.' => ['10.10.0.0/21']], $this->ranges->getBlockedRanges()); } - } diff --git a/tests/Integration/Tracker/RequestProcessorTest.php b/tests/Integration/Tracker/RequestProcessorTest.php index eb34804..1efabf3 100644 --- a/tests/Integration/Tracker/RequestProcessorTest.php +++ b/tests/Integration/Tracker/RequestProcessorTest.php @@ -1,4 +1,5 @@ isAuthenticated(); return $req; } - } diff --git a/tests/Integration/TrackingSpamPreventionTest.php b/tests/Integration/TrackingSpamPreventionTest.php index 605ec27..66b25bd 100644 --- a/tests/Integration/TrackingSpamPreventionTest.php +++ b/tests/Integration/TrackingSpamPreventionTest.php @@ -1,4 +1,5 @@ assertFalse($excluded->isExcluded()); } - public function test_isExcludedVisit_whenBlockServerSideLibraryDisabledAndNotServerSideUserAgent() { + public function test_isExcludedVisit_whenBlockServerSideLibraryDisabledAndNotServerSideUserAgent() + { StaticContainer::get(SystemSettings::class)->blockServerSideLibraries->setValue(0); Cache::clearCacheGeneral(); @@ -106,7 +108,8 @@ public function test_isExcludedVisit_whenBlockServerSideLibraryDisabledAndNotSer $this->assertFalse($isExcluded); } - public function test_isExcludedVisit_whenBlockServerSideLibraryDisabledAndServerSideUserAgent() { + public function test_isExcludedVisit_whenBlockServerSideLibraryDisabledAndServerSideUserAgent() + { StaticContainer::get(SystemSettings::class)->blockServerSideLibraries->setValue(0); Cache::clearCacheGeneral(); @@ -117,7 +120,8 @@ public function test_isExcludedVisit_whenBlockServerSideLibraryDisabledAndServer $this->assertFalse($isExcluded); } - public function test_isExcludedVisit_whenBlockServerSideLibraryEnabledAndNotServerSideUserAgent() { + public function test_isExcludedVisit_whenBlockServerSideLibraryEnabledAndNotServerSideUserAgent() + { StaticContainer::get(SystemSettings::class)->blockServerSideLibraries->setValue(1); Cache::clearCacheGeneral(); @@ -128,7 +132,8 @@ public function test_isExcludedVisit_whenBlockServerSideLibraryEnabledAndNotServ $this->assertFalse($isExcluded); } - public function test_isExcludedVisit_whenBlockServerSideLibraryEnabledAndServerSideUserAgent() { + public function test_isExcludedVisit_whenBlockServerSideLibraryEnabledAndServerSideUserAgent() + { StaticContainer::get(SystemSettings::class)->blockServerSideLibraries->setValue(1); Cache::clearCacheGeneral(); diff --git a/tests/System/CheckDirectDependencyUseCommandTest.php b/tests/System/CheckDirectDependencyUseCommandTest.php index ab97c30..d38c792 100644 --- a/tests/System/CheckDirectDependencyUseCommandTest.php +++ b/tests/System/CheckDirectDependencyUseCommandTest.php @@ -1,4 +1,5 @@ usesFoundList[$pluginName]); } -} \ No newline at end of file +} From a799903a3ab3399b880a0d64ad8a361bbe5ff6fb Mon Sep 17 00:00:00 2001 From: Jacob Ransom Date: Mon, 21 Oct 2024 10:42:16 +1300 Subject: [PATCH 3/3] Updating test after removal of unused import --- tests/System/CheckDirectDependencyUseCommandTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/System/CheckDirectDependencyUseCommandTest.php b/tests/System/CheckDirectDependencyUseCommandTest.php index d38c792..9f84348 100644 --- a/tests/System/CheckDirectDependencyUseCommandTest.php +++ b/tests/System/CheckDirectDependencyUseCommandTest.php @@ -46,7 +46,6 @@ public function testCommand() 'Matomo\Network' => [ 'TrackingSpamPrevention/AllowListIpRange.php', 'TrackingSpamPrevention/BlockedIpRanges.php', - 'TrackingSpamPrevention/Tracker/RequestProcessor.php', 'TrackingSpamPrevention/TrackingSpamPrevention.php', ], 'DI' => [