multi_server_environment setting now disallows config edits from Web UI #16760

nina-py · 2020-11-20T06:32:43Z

Description:

Turning the multi_server_environment setting on now hides the following
settings from the Administration -> Settings -> General settings page:

Archiving settings
Email server
Trusted Matomo hostname
Update settings

...so that users cannot set these separately on different instances of Matomo. Assuming I got these right, are there any other settings that should be disabled when multi_server_environment is turned on?

Fixes #14390.

Review

Functional review done
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 · 2020-11-22T20:07:05Z

Makes sense @nina-py

Might need to add a note in config/global.ini.php for multi_server_environment that this flag doesn't need to be enabled when the config file is on a shared filesystem like NFS or EFS.

nina-py · 2020-11-22T23:16:58Z

Thanks @tsteur, added a note to the global config file.

nina-py · 2020-11-30T02:28:01Z

Hi @tsteur , is there anything else that needs to be done in this PR? Or are you guys just way too busy at the moment following the 4.0.x release?

tsteur · 2020-11-30T02:32:10Z

@nina-py at the moment we're indeed quite busy with the Matomo 4 release. @sgiehl can you do another review maybe?

sgiehl · 2020-11-30T09:11:47Z

plugins/CoreAdminHome/templates/generalSettings.twig

@@ -10,7 +10,7 @@
    {{ ajax.errorDiv() }}
    {{ ajax.loadingDiv() }}

-{% if isGeneralSettingsAdminEnabled %}
+{% if isGeneralSettingsAdminEnabled and not isMultiServerEnvironment %}


I guess that is actually not needed to hide the archiving settings. They are stored in the database when configured through UI. See

matomo/core/ArchiveProcessor/Rules.php

Lines 265 to 272 in f5e9420

public static function setBrowserTriggerArchiving($enabled)

{

if (!is_bool($enabled)) {

throw new Exception('Browser trigger archiving must be set to true or false.');

}

Option::set(self::OPTION_BROWSER_TRIGGER_ARCHIVING, (int)$enabled, $autoLoad = true);

Cache::clearCacheGeneral();

}

Only if that values is not available the one in config should be used.

Hiding the email settings only makes more sense, as they are stored in config only.

Thanks @sgiehl, I'll tackle this over the weekend.

@sgiehl, I updated the template to hide only the Email Server settings if in multi-server environment.

sgiehl · 2020-11-30T09:16:00Z

plugins/CoreUpdater/SystemSettings.php

-        $isWritable = Piwik::hasUserSuperUserAccess() && CoreAdminController::isGeneralSettingsAdminEnabled();
+        $isWritable = Piwik::hasUserSuperUserAccess()
+                        && CoreAdminController::isGeneralSettingsAdminEnabled()
+                        && SettingsPiwik::isMultiServerEnvironment() === false;
        $this->releaseChannel = $this->createReleaseChannel();
        $this->releaseChannel->setIsWritableByCurrentUser($isWritable);

-        $isWritable = $isWritable && PluginUpdateCommunication::canBeEnabled();
-        $this->sendPluginUpdateEmail = $this->createSendPluginUpdateEmail();
-        $this->sendPluginUpdateEmail->setIsWritableByCurrentUser($isWritable);
-
-        $isWritable = Piwik::hasUserSuperUserAccess() && CoreAdminController::isGeneralSettingsAdminEnabled();
        $dbSettings = new Settings();
        if ($isWritable && $dbSettings->getUsedCharset() !== 'utf8mb4' && DbHelper::getDefaultCharset() === 'utf8mb4') {
            $this->updateToUtf8mb4 = $this->createUpdateToUtf8mb4();
        }
+
+        $isWritable = $isWritable && PluginUpdateCommunication::canBeEnabled();
+        $this->sendPluginUpdateEmail = $this->createSendPluginUpdateEmail();
+        $this->sendPluginUpdateEmail->setIsWritableByCurrentUser($isWritable);


Actually only the release channel is a configuration that is managed in config. The other values are "normal" settings, stored in database. So for those it wouldn't hurt to have them writable.

Thank you for that, I have updated the code to reflect that (and did a very minor refactor to remove duplicate code).

Turning the multi_server_environment setting on now hides the following settings from the Administration -> Settings -> General settings page: - Archiving settings - Email server - Trusted Matomo hostname - Update settings ...so that users cannot set these separately on different instances of Matomo. Fixes #14390.

…stems

…the general settings page. - Only making the release channel not writable in CoreUpdater->SystemSettings, and leaving the other settings as before.

nina-py · 2020-12-14T10:31:07Z

Hi @sgiehl, in addition to the changes you requested earlier, I have updated a UI screenshot (an extra comment for the setting about half-way through the screenshot): https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/44409/UIIntegrationTest_admin_diagnostics_configfile.png. CI build in progress.

As far as I can see, the other failures in those two jobs are not related to the changes introduced in this PR. I've also rebased the branch. Please review.

nina-py · 2020-12-14T11:04:22Z

@sgiehl, just to follow up on the most recent CI build for this PR - the updated screenshot matches but there is one failure elsewhere with the following:

There was 1 failure:

1) Piwik\Tests\Integration\ReleaseCheckListTest::test_screenshotsStoredInLfs

Some Screenshots are not stored in LFS: tests/UI/expected-screenshots/UIIntegrationTest_admin_diagnostics_configfile.png

Failed asserting that an array is empty.

/home/travis/build/matomo-org/matomo/tests/PHPUnit/Integration/ReleaseCheckListTest.php:180

Do I need to do anything about this one?

sgiehl · 2020-12-14T11:06:42Z

@nina-py We are storing our screenshots in Git LFS to not waste space in this repo. You may need to install Git LFS to update the the srceenshots properly

Updated UIIntegrationTest_admin_diagnostics_configfile.png - with Git LFS this time around.

nina-py · 2020-12-14T12:03:54Z

Thank you @sgiehl, I think I got this right this time around - the commit diff shows

Git LFS file not shown

but the build is yet to run.

sgiehl

Looks good now 👍 @nina-py

nina-py · 2020-12-14T20:05:16Z

Thank you @sgiehl for all your help with this one!

tsteur added the Needs Review PRs that need a code review label Nov 22, 2020

tsteur added this to the 4.1.0 milestone Nov 22, 2020

sgiehl requested changes Nov 30, 2020

View reviewed changes

nina-py added 3 commits December 14, 2020 20:44

Added a note for multi_server_environment setting about shared filesy…

8927606

…stems

- multi_server_environment now only hides email server settings from …

1a518ac

…the general settings page. - Only making the release channel not writable in CoreUpdater->SystemSettings, and leaving the other settings as before.

Update UI test

e4d06fb

Updated UIIntegrationTest_admin_diagnostics_configfile.png - with Git LFS this time around.

sgiehl approved these changes Dec 14, 2020

View reviewed changes

sgiehl merged commit 89c0dc2 into matomo-org:4.x-dev Dec 15, 2020

nina-py deleted the 14390-multi-server-environment-bug-fix branch December 15, 2020 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi_server_environment setting now disallows config edits from Web UI #16760

multi_server_environment setting now disallows config edits from Web UI #16760

nina-py commented Nov 20, 2020

tsteur commented Nov 22, 2020

nina-py commented Nov 22, 2020

nina-py commented Nov 30, 2020

tsteur commented Nov 30, 2020

sgiehl Nov 30, 2020

nina-py Dec 1, 2020

nina-py Dec 7, 2020

sgiehl Nov 30, 2020

nina-py Dec 7, 2020

nina-py commented Dec 14, 2020

nina-py commented Dec 14, 2020

sgiehl commented Dec 14, 2020

nina-py commented Dec 14, 2020

sgiehl left a comment

nina-py commented Dec 14, 2020

	public static function setBrowserTriggerArchiving($enabled)
	{
	if (!is_bool($enabled)) {
	throw new Exception('Browser trigger archiving must be set to true or false.');
	}
	Option::set(self::OPTION_BROWSER_TRIGGER_ARCHIVING, (int)$enabled, $autoLoad = true);
	Cache::clearCacheGeneral();
	}

multi_server_environment setting now disallows config edits from Web UI #16760

multi_server_environment setting now disallows config edits from Web UI #16760

Conversation

nina-py commented Nov 20, 2020

Description:

Review

tsteur commented Nov 22, 2020

nina-py commented Nov 22, 2020

nina-py commented Nov 30, 2020

tsteur commented Nov 30, 2020

sgiehl Nov 30, 2020

Choose a reason for hiding this comment

nina-py Dec 1, 2020

Choose a reason for hiding this comment

nina-py Dec 7, 2020

Choose a reason for hiding this comment

sgiehl Nov 30, 2020

Choose a reason for hiding this comment

nina-py Dec 7, 2020

Choose a reason for hiding this comment

nina-py commented Dec 14, 2020

nina-py commented Dec 14, 2020

sgiehl commented Dec 14, 2020

nina-py commented Dec 14, 2020

sgiehl left a comment

Choose a reason for hiding this comment

nina-py commented Dec 14, 2020