diff --git a/application/controllers/ConfigController.php b/application/controllers/ConfigController.php index 1570be4019..9996ec0506 100644 --- a/application/controllers/ConfigController.php +++ b/application/controllers/ConfigController.php @@ -8,6 +8,7 @@ use Exception; use GuzzleHttp\Psr7\ServerRequest; use Icinga\Application\Version; +use Icinga\Web\Session; use InvalidArgumentException; use Icinga\Application\Config; use Icinga\Application\Icinga; @@ -132,24 +133,14 @@ public function securityAction(): void $this->view->title = $this->translate('Security'); - $config = Config::app(); - $cspForm = new CspConfigForm($config); - // Keep the auto-generation options off by default for installations that already - // had CSP enabled, so their existing behavior isn't changed. For installations - // enabling CSP for the first time, default them on as the recommended setting. - $defaultValue = $cspForm->isCspEnabled() ? '0' : '1'; - $cspForm->populate([ - 'security__csp_enable_modules' => $config->get('security', 'csp_enable_modules', $defaultValue), - 'security__csp_enable_dashboards' => $config->get('security', 'csp_enable_dashboards', $defaultValue), - 'security__csp_enable_navigation' => $config->get('security', 'csp_enable_navigation', $defaultValue), - ]); - - $cspForm->on(ContractForm::ON_SUBMIT, function (CspConfigForm $form) { - if ($form->hasConfigChanged()) { - $this->getResponse()->setReloadWindow(true); - } - }); - $cspForm->handleRequest(ServerRequest::fromGlobals()); + $cspForm = (new CspConfigForm(Config::app())) + ->setCsrfCounterMeasureId(Session::getSession()->getId()) + ->on(ContractForm::ON_SUBMIT, function (CspConfigForm $form) { + if ($form->hasConfigChanged()) { + $this->getResponse()->setReloadWindow(true); + } + }) + ->handleRequest(ServerRequest::fromGlobals()); $this->view->cspForm = $cspForm; $this->createApplicationTabs()->activate('security'); } diff --git a/application/forms/Config/Security/CspConfigForm.php b/application/forms/Config/Security/CspConfigForm.php index 30c4dd9605..ea148bfbc7 100644 --- a/application/forms/Config/Security/CspConfigForm.php +++ b/application/forms/Config/Security/CspConfigForm.php @@ -17,12 +17,12 @@ use Icinga\Security\Csp\Reason\StaticCspReason; use Icinga\Util\Csp; use Icinga\Web\Form\ConfigForm; -use Icinga\Web\Session; use ipl\Html\Attributes; use ipl\Html\BaseHtmlElement; use ipl\Html\HtmlElement; use ipl\Html\Table; use ipl\Html\Text; +use ipl\Stdlib\Str; use ipl\Validator\CallbackValidator; use ipl\Web\Common\CalloutType; use ipl\Web\Common\Csp as CspInstance; @@ -130,7 +130,7 @@ protected function assemble(): void $this->addElement($this->createUidElement()); - $this->addCsrfCounterMeasure(Session::getSession()->getId()); + $this->addCsrfCounterMeasure(); $this->addElement('checkbox', 'security__use_strict_csp', [ 'label' => $this->translate('Send CSP header'), @@ -194,7 +194,7 @@ function (StaticCspReason $reason, string $directive, string $expression) { 'Should module defined CSP directives be enabled?' . ' Modules can define or change csp directives at any point.', ), - 'security__csp_enable_modules', + 'csp_enable_modules', ! $useCustomCsp, ); @@ -219,7 +219,7 @@ function (ModuleCspReason $reason, string $directive, string $expression) { . ' header that is sent to the user will only contain the subset of directives that actually' . ' matters to them.', ), - 'security__csp_enable_dashboards', + 'csp_enable_dashboards', ! $useCustomCsp, ); @@ -252,7 +252,7 @@ function (DashboardCspReason $reason, string $directive, string $expression) { . ' all users. The actual header that is sent to the user will only contain the subset of' . ' directives that actually matters to them.', ), - 'security__csp_enable_navigation', + 'csp_enable_navigation', ! $useCustomCsp, ); @@ -307,6 +307,7 @@ function (NavigationCspReason $reason, string $directive, string $expression) { 'class' => 'autosubmit csp-form-content-aligned csp-label-header-h3 csp-form-header', 'checkedValue' => '1', 'uncheckedValue' => '0', + 'value' => '0', ], ); @@ -399,7 +400,7 @@ public function isCustomCspEnabled(): bool * * @param string $label The label of the checkbox * @param string $description The description of the checkbox - * @param string $field The name of the checkbox field + * @param string $key The key of the checkbox field in the config * @param bool $enabled Whether the checkbox should be checked and enabled * * @return void @@ -407,7 +408,7 @@ public function isCustomCspEnabled(): bool protected function addDirectiveCheckboxElement( string $label, string $description, - string $field, + string $key, bool $enabled, ): void { $classList = [ @@ -420,6 +421,17 @@ protected function addDirectiveCheckboxElement( $classList[] = 'csp-disabled'; } + $field = 'security' . $this->sectionKeyDelimiter . $key; + + // Keep the auto-generation options off by default for installations that already + // had CSP enabled, so their existing behavior isn't changed. For installations + // enabling CSP for the first time, default them on as the recommended setting. + $cspWasEnabled = $this->config->get('security', 'use_strict_csp') === '1'; + $defaultValue = $cspWasEnabled ? '0' : '1'; + if (Str::isEmpty($this->getPopulatedValue($field)) && $this->config->get('security', $key) === null) { + $this->populate([$field => $defaultValue]); + } + $this->addElement('checkbox', $field, [ 'label' => $label, 'description' => $description, @@ -427,7 +439,6 @@ protected function addDirectiveCheckboxElement( 'checkedValue' => '1', 'uncheckedValue' => '0', 'disabled' => ! $enabled, - 'value' => $this->getPopulatedValue($field), ]); } diff --git a/library/Icinga/Web/Form/ConfigForm.php b/library/Icinga/Web/Form/ConfigForm.php index 510190f73e..8fee677528 100644 --- a/library/Icinga/Web/Form/ConfigForm.php +++ b/library/Icinga/Web/Form/ConfigForm.php @@ -8,7 +8,6 @@ use Exception; use Icinga\Application\Config; use ipl\Html\Contract\FormElement; -use ipl\Html\Contract\ValueCandidates; use ipl\Html\HtmlElement; use ipl\Html\ValidHtml; use ipl\Stdlib\Str; @@ -16,6 +15,7 @@ use ipl\Web\Compat\DisplayFormElement; use ipl\Web\Widget\CopyToClipboard; use LogicException; +use stdClass; /** * Form base-class providing standard functionality for configuration forms @@ -42,6 +42,16 @@ class ConfigForm extends CompatForm */ protected string $sectionKeyDelimiter = '__'; + /** + * The original values of the form elements + * + * This is used to determine whether an element's submitted value differs from + * the original value. + * + * @var array + */ + protected array $originalValues = []; + /** * Create a new configuration form * @@ -50,34 +60,30 @@ class ConfigForm extends CompatForm public function __construct( protected Config $config, ) { - $this->on(static::ON_ELEMENT_REGISTERED, function (FormElement $element) { - [$section, $key] = Str::symmetricSplit($element->getName(), $this->sectionKeyDelimiter, 2); - if ($key === null || $element->hasValue()) { - return; - } - - $configValue = $this->config->get($section, $key); - if ($configValue === null) { - return; - } - - if (! ($element instanceof ValueCandidates)) { - $element->setValue($configValue); + } - return; + /** + * Register the given element, seeding it with its configuration value first + * + * @param FormElement $element + * + * @return $this + */ + public function registerElement(FormElement $element): static + { + $name = $element->getName(); + [$section, $key] = Str::symmetricSplit($name, $this->sectionKeyDelimiter, 2); + if ($key !== null) { + $this->originalValues[$name] = $element->getValue(); + + $sentinel = new stdClass(); + $configValue = $this->config->get($section, $key, $this->originalValues[$name] ?? $sentinel); + if ($configValue !== $sentinel) { + $this->populatedValues[$name] = array_merge([$configValue], $this->populatedValues[$name] ?? []); } + } - $candidates = $element->getValueCandidates(); - if (empty($candidates)) { - // Initial render: no prior submission, set value and seed candidates - $element->setValue($configValue); - $element->setValueCandidates([$configValue]); - } else { - // POST: candidates were set by registerElement; append config value so - // PasswordElement's (count > 1) condition resolves DUMMYPASSWORD on re-render - $element->setValueCandidates(array_merge($candidates, [$configValue])); - } - }); + return parent::registerElement($element); } /** @@ -160,8 +166,11 @@ protected function save(): void throw new LogicException(sprintf('Cannot save element "%s": array values are not supported', $element)); } + $originalValue = $this->originalValues[$element] ?? ''; $configSection = $this->config->getSection($section); - if (Str::isEmpty($value)) { + if ($originalValue !== '' && Str::isEmpty($value)) { + $configSection[$key] = ''; + } elseif (Str::isEmpty($value) || $originalValue === $value) { unset($configSection[$key]); } else { $configSection[$key] = $value; diff --git a/test/php/application/forms/Config/Security/CspConfigFormTest.php b/test/php/application/forms/Config/Security/CspConfigFormTest.php new file mode 100644 index 0000000000..bd0b831d33 --- /dev/null +++ b/test/php/application/forms/Config/Security/CspConfigFormTest.php @@ -0,0 +1,73 @@ + +// SPDX-License-Identifier: GPL-3.0-or-later + +namespace Tests\Icinga\Forms\Config\Security; + +use Icinga\Application\Config; +use Icinga\Forms\Config\Security\CspConfigForm; +use Icinga\Test\BaseTestCase; + +class CspConfigFormTest extends BaseTestCase +{ + public function testDirectiveCheckboxesDefaultOnWhenStrictCspIsUnconfigured(): void + { + $this->assertDirectiveDefaults([], '1'); + } + + public function testDirectiveCheckboxesDefaultOnWhenStrictCspWasDisabled(): void + { + $this->assertDirectiveDefaults(['security' => ['use_strict_csp' => '0']], '1'); + } + + public function testDirectiveCheckboxesDefaultOffWhenStrictCspWasEnabled(): void + { + $this->assertDirectiveDefaults(['security' => ['use_strict_csp' => '1']], '0'); + } + + public function testConfiguredDirectiveCheckboxValuesOverrideDefaults(): void + { + $form = $this->createDirectiveForm([ + 'security' => [ + 'use_strict_csp' => '1', + 'csp_enable_modules' => '1', + 'csp_enable_dashboards' => '0', + 'csp_enable_navigation' => '1', + ], + ]); + + $this->assertSame('1', $form->getValue('security__csp_enable_modules')); + $this->assertSame('0', $form->getValue('security__csp_enable_dashboards')); + $this->assertSame('1', $form->getValue('security__csp_enable_navigation')); + } + + private function assertDirectiveDefaults(array $configData, string $expected): void + { + $form = $this->createDirectiveForm($configData); + + foreach (['csp_enable_modules', 'csp_enable_dashboards', 'csp_enable_navigation'] as $key) { + $this->assertSame($expected, $form->getValue('security__' . $key), $key); + } + } + + private function createDirectiveForm(array $configData): CspConfigForm + { + $form = new class (Config::fromArray($configData)) extends CspConfigForm { + protected function assemble(): void + { + } + + public function addDirectiveCheckboxForTest(string $key): void + { + $this->addDirectiveCheckboxElement($key, $key, $key, true); + } + }; + + foreach (['csp_enable_modules', 'csp_enable_dashboards', 'csp_enable_navigation'] as $key) { + $form->addDirectiveCheckboxForTest($key); + } + + return $form; + } +} diff --git a/test/php/library/Icinga/Web/Form/ConfigFormTest.php b/test/php/library/Icinga/Web/Form/ConfigFormTest.php index 65203c3a82..41bedb3cf3 100644 --- a/test/php/library/Icinga/Web/Form/ConfigFormTest.php +++ b/test/php/library/Icinga/Web/Form/ConfigFormTest.php @@ -11,6 +11,7 @@ use Icinga\Web\Form\ConfigForm; use ipl\Html\FormElement\PasswordElement; use LogicException; +use PHPUnit\Framework\Attributes\DataProvider; class ConfigFormTest extends BaseTestCase { @@ -31,7 +32,8 @@ public function testSubmitButtonIsAddedAfterAssembly(): void $this->assertTrue($form->hasElement('store')); } - public function testSaveThrowsForArrayElementValue(): void + #[DataProvider('populationTimings')] + public function testSaveThrowsForArrayElementValue(bool $populateBeforeAssembly): void { $this->expectException(LogicException::class); @@ -53,12 +55,12 @@ public function exposeSave(): void $this->save(); } }; - $form->ensureAssembled(); - $form->populate(['mysection__key' => ['a', 'b']]); + $this->populateAroundAssembly($form, [['mysection__key' => ['a', 'b']]], $populateBeforeAssembly); $form->exposeSave(); } - public function testUnchangedPasswordElementRetainsConfigValueOnSave(): void + #[DataProvider('populationTimings')] + public function testUnchangedPasswordElementRetainsConfigValueOnSave(bool $populateBeforeAssembly): void { $config = new class(new ConfigObject(['mysection' => ['password' => 'secret']])) extends Config { public function saveIni($filePath = null, $fileMode = 0660): void {} @@ -75,14 +77,106 @@ public function exposeSave(): void $this->save(); } }; - $form->populate(['mysection__password' => PasswordElement::DUMMYPASSWORD]); - $form->ensureAssembled(); + $this->populateAroundAssembly( + $form, + [['mysection__password' => PasswordElement::DUMMYPASSWORD]], + $populateBeforeAssembly, + ); $form->exposeSave(); $this->assertSame('secret', $config->get('mysection', 'password')); } - public function testEmptySectionIsRemovedOnSave(): void + public function testElementDefaultIsPreservedWhenConfigKeyIsNotSet(): void + { + $form = new class(Config::fromArray([])) extends ConfigForm { + protected function assemble(): void + { + $this->addElement('text', 'mysection__key', ['value' => 'defaultvalue']); + } + }; + $form->ensureAssembled(); + + $this->assertSame('defaultvalue', $form->getValue('mysection__key')); + $this->assertSame('defaultvalue', $form->getPopulatedValue('mysection__key')); + } + + #[DataProvider('populationTimings')] + public function testElementIsClearedWhenValueIsEmpty(bool $populateBeforeAssembly): void + { + $config = new class(new ConfigObject(['mysection' => ['key' => 'value']])) extends Config { + public function saveIni($filePath = null, $fileMode = 0660): void {} + }; + + $form = new class($config) extends ConfigForm { + protected function assemble(): void + { + $this->addElement('text', 'mysection__key', []); + } + + public function exposeSave(): void + { + $this->save(); + } + }; + $this->populateAroundAssembly($form, [['mysection__key' => '']], $populateBeforeAssembly); + $form->exposeSave(); + + $this->assertNull($config->get('mysection', 'key')); + } + + #[DataProvider('populationTimings')] + public function testElementIsClearedWhenValueIsOriginalValue(bool $populateBeforeAssembly): void + { + $config = new class(new ConfigObject(['mysection' => ['key' => 'value']])) extends Config { + public function saveIni($filePath = null, $fileMode = 0660): void {} + }; + + $form = new class($config) extends ConfigForm { + protected function assemble(): void + { + $this->addElement('text', 'mysection__key', ['value' => 'value']); + } + + public function exposeSave(): void + { + $this->save(); + } + }; + $this->populateAroundAssembly($form, [['mysection__key' => 'value']], $populateBeforeAssembly); + $form->exposeSave(); + + $this->assertNull($config->get('mysection', 'key')); + } + + #[DataProvider('populationTimings')] + public function testClearingFieldWithNonEmptyDefaultWritesEmptyStringToConfig( + bool $populateBeforeAssembly, + ): void { + $config = new class(new ConfigObject(['mysection' => ['key' => 'value']])) extends Config { + public function saveIni($filePath = null, $fileMode = 0660): void {} + }; + + $form = new class($config) extends ConfigForm { + protected function assemble(): void + { + $this->addElement('text', 'mysection__key', ['value' => 'default']); + } + + public function exposeSave(): void + { + $this->save(); + } + }; + $this->populateAroundAssembly($form, [['mysection__key' => '']], $populateBeforeAssembly); + $form->exposeSave(); + + $this->assertTrue($config->hasSection('mysection')); + $this->assertSame('', $config->get('mysection', 'key', 'not-set')); + } + + #[DataProvider('populationTimings')] + public function testEmptySectionIsRemovedOnSave(bool $populateBeforeAssembly): void { $config = new class(new ConfigObject(['mysection' => ['key' => 'value']])) extends Config { public function saveIni($filePath = null, $fileMode = 0660): void {} @@ -99,10 +193,111 @@ public function exposeSave(): void $this->save(); } }; - $form->ensureAssembled(); - $form->populate(['mysection__key' => '']); + $this->populateAroundAssembly($form, [['mysection__key' => '']], $populateBeforeAssembly); $form->exposeSave(); $this->assertFalse($config->hasSection('mysection')); } + + #[DataProvider('populationTimings')] + public function testZeroValueIsSaved(bool $populateBeforeAssembly): void + { + $config = new class (new ConfigObject(['mysection' => ['key' => 'value']])) extends Config { + public function saveIni($filePath = null, $fileMode = 0660): void + { + } + }; + + $form = new class ($config) extends ConfigForm { + protected function assemble(): void + { + $this->addElement('text', 'mysection__key'); + } + + public function exposeSave(): void + { + $this->save(); + } + }; + $this->populateAroundAssembly($form, [['mysection__key' => '0']], $populateBeforeAssembly); + $form->exposeSave(); + + $this->assertSame('0', $config->get('mysection', 'key')); + } + + #[DataProvider('populationTimings')] + public function testEveryPopulatedValueIsAppliedInOrder(bool $populateBeforeAssembly): void + { + $config = Config::fromArray(['mysection' => ['key' => 'configured']]); + $form = new class ($config) extends ConfigForm { + protected function assemble(): void + { + $this->addElement('text', 'mysection__key'); + } + }; + $this->populateAroundAssembly( + $form, + [['mysection__key' => ''], ['mysection__key' => '0']], + $populateBeforeAssembly, + ); + + $this->assertSame('0', $form->getValue('mysection__key')); + $this->assertSame('0', $form->getPopulatedValue('mysection__key')); + } + + #[DataProvider('populationTimings')] + public function testNullValueClearsElement(bool $populateBeforeAssembly): void + { + $config = new class (new ConfigObject(['mysection' => ['key' => 'value']])) extends Config { + public function saveIni($filePath = null, $fileMode = 0660): void + { + } + }; + + $form = new class ($config) extends ConfigForm { + protected function assemble(): void + { + $this->addElement('text', 'mysection__key'); + } + + public function exposeSave(): void + { + $this->save(); + } + }; + $this->populateAroundAssembly($form, [['mysection__key' => null]], $populateBeforeAssembly); + $form->exposeSave(); + + $this->assertNull($config->get('mysection', 'key')); + } + + /** @return array */ + public static function populationTimings(): array + { + return [ + 'before assembly' => [true], + 'after assembly' => [false], + ]; + } + + /** + * @param list> $populations + */ + private function populateAroundAssembly( + ConfigForm $form, + array $populations, + bool $populateBeforeAssembly, + ): void { + if (! $populateBeforeAssembly) { + $form->ensureAssembled(); + } + + foreach ($populations as $values) { + $form->populate($values); + } + + if ($populateBeforeAssembly) { + $form->ensureAssembled(); + } + } }