From 3d1fba650b07d20e0c8d594c63178cd868c9ddea Mon Sep 17 00:00:00 2001 From: Kevin Papst Date: Fri, 25 Jun 2021 16:35:16 +0200 Subject: [PATCH] improve color chooser and name validation (#2622) --- config/services.yaml | 2 +- .../StringAccessibleConfigTrait.php | 17 ++++++++- src/Configuration/SystemConfiguration.php | 27 +++----------- src/DependencyInjection/Configuration.php | 2 +- src/Form/Type/ColorChoiceType.php | 36 ++++++++++++++++++- src/Validator/Constraints/ColorChoices.php | 3 +- .../Constraints/ColorChoicesValidator.php | 9 +++-- .../Configuration/SystemConfigurationTest.php | 5 +-- .../DependencyInjection/AppExtensionTest.php | 2 +- .../DependencyInjection/ConfigurationTest.php | 2 +- .../Constraints/ColorChoicesValidatorTest.php | 12 ++++--- 11 files changed, 77 insertions(+), 40 deletions(-) diff --git a/config/services.yaml b/config/services.yaml index d52af5be..15b91c8f 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -242,6 +242,6 @@ services: arguments: ['App\Entity\Invoice'] App\Repository\BookmarkRepository: - class: \Repository\BookmarkRepository + class: App\Repository\BookmarkRepository factory: ['@doctrine.orm.entity_manager', getRepository] arguments: ['App\Entity\Bookmark'] diff --git a/src/Configuration/StringAccessibleConfigTrait.php b/src/Configuration/StringAccessibleConfigTrait.php index c1b8f325..217298b2 100644 --- a/src/Configuration/StringAccessibleConfigTrait.php +++ b/src/Configuration/StringAccessibleConfigTrait.php @@ -17,6 +17,10 @@ trait StringAccessibleConfigTrait * @var array */ protected $settings; + /** + * @var array + */ + protected $original; /** * @var ConfigLoaderInterface */ @@ -29,7 +33,7 @@ trait StringAccessibleConfigTrait public function __construct(ConfigLoaderInterface $repository, array $settings) { $this->repository = $repository; - $this->settings = $settings; + $this->original = $this->settings = $settings; } /** @@ -80,6 +84,17 @@ trait StringAccessibleConfigTrait */ abstract protected function getPrefix(): string; + /** + * @param string $key + * @return mixed + */ + public function default(string $key) + { + $key = $this->prepareSearchKey($key); + + return $this->get($key, $this->original); + } + /** * @param string $key * @return mixed diff --git a/src/Configuration/SystemConfiguration.php b/src/Configuration/SystemConfiguration.php index cfa36239..a8f26686 100644 --- a/src/Configuration/SystemConfiguration.php +++ b/src/Configuration/SystemConfiguration.php @@ -367,32 +367,13 @@ class SystemConfiguration implements SystemBundleConfiguration return (int) $this->find('theme.autocomplete_chars'); } - public function getThemeColorChoices(): ?array + public function getThemeColorChoices(): ?string { $config = $this->find('theme.color_choices'); - if (empty($config)) { - return null; - } - $config = explode(',', $config); - - $colors = []; - foreach ($config as $item) { - if (empty($item)) { - continue; - } - $item = explode('|', $item); - $key = $item[0]; - $value = $key; - if (\count($item) > 1) { - $value = $item[1]; - } - - if (empty($key)) { - $key = $value; - } - $colors[$key] = $value; + if (!empty($config)) { + return $config; } - return array_unique($colors); + return $this->default('theme.color_choices'); } } diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index 9cb98574..d10f9835 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -429,7 +429,7 @@ class Configuration implements ConfigurationInterface ->end() ->scalarNode('color_choices') ->defaultValue(implode(',', [ - Constants::SOFTWARE . '|' . Constants::DEFAULT_COLOR, 'Silver|#c0c0c0', 'Gray|#808080', 'Black|#000000', + 'Silver|#c0c0c0', 'Gray|#808080', 'Black|#000000', 'Maroon|#800000', 'Brown|#a52a2a', 'Red|#ff0000', 'Orange|#ffa500', 'Gold|#ffd700', 'Yellow|#ffff00', 'Peach|#ffdab9', 'Khaki|#f0e68c', 'Olive|#808000', 'Lime|#00ff00', 'Jelly|#9acd32', 'Green|#008000', 'Teal|#008080', diff --git a/src/Form/Type/ColorChoiceType.php b/src/Form/Type/ColorChoiceType.php index b9ee69f1..26b18b1b 100644 --- a/src/Form/Type/ColorChoiceType.php +++ b/src/Form/Type/ColorChoiceType.php @@ -66,9 +66,12 @@ class ColorChoiceType extends AbstractType implements DataTransformerInterface if ($this->isLimitedColors()) { $choices = []; - foreach ($this->systemConfiguration->getThemeColorChoices() as $name => $color) { + $colors = $this->convertStringToColorArray($this->systemConfiguration->getThemeColorChoices()); + + foreach ($colors as $name => $color) { $choices[$name] = $color; } + $options['choices'] = $choices; $options['search'] = false; $options['attr']['data-renderer'] = 'color'; @@ -77,6 +80,37 @@ class ColorChoiceType extends AbstractType implements DataTransformerInterface $resolver->setDefaults($options); } + private function convertStringToColorArray(string $config): array + { + $config = explode(',', $config); + + $colors = []; + foreach ($config as $item) { + if (empty($item)) { + continue; + } + $item = explode('|', $item); + $key = $item[0]; + $value = $key; + + if (\count($item) > 1) { + $value = $item[1]; + } + + if (empty($key)) { + $key = $value; + } + + if ($value === Constants::DEFAULT_COLOR) { + continue; + } + + $colors[$key] = $value; + } + + return array_unique($colors); + } + /** * {@inheritdoc} */ diff --git a/src/Validator/Constraints/ColorChoices.php b/src/Validator/Constraints/ColorChoices.php index 89d3e4a9..a95b364e 100644 --- a/src/Validator/Constraints/ColorChoices.php +++ b/src/Validator/Constraints/ColorChoices.php @@ -22,5 +22,6 @@ class ColorChoices extends Constraint ]; public $message = 'The given value {{ value }} is not a valid hexadecimal color.'; - public $invalidNameMessage = 'The given value {{ name }} is not a valid color name for {{ color }}. Allowed are {{ max }} characters, given {{ count }}.'; + public $invalidNameMessage = 'The given value {{ name }} is not a valid color name for {{ color }}. Allowed are {{ max }} alpha-numerical characters, including minus and space.'; + public $maxLength = 20; } diff --git a/src/Validator/Constraints/ColorChoicesValidator.php b/src/Validator/Constraints/ColorChoicesValidator.php index 6e0002ea..d071af5e 100644 --- a/src/Validator/Constraints/ColorChoicesValidator.php +++ b/src/Validator/Constraints/ColorChoicesValidator.php @@ -57,12 +57,15 @@ class ColorChoicesValidator extends ConstraintValidator return; } - if (!\is_string($name) || 1 !== preg_match('/^[0-9a-zA-Z]{1,10}$/i', $name)) { + $name = str_replace(['-', ' '], '', $name); + $length = mb_strlen($name); + + if (!\is_string($name) || $length > $constraint->maxLength || !ctype_alnum($name)) { $this->context->buildViolation($constraint->invalidNameMessage) ->setParameter('{{ name }}', $this->formatValue($name)) ->setParameter('{{ color }}', $this->formatValue($code)) - ->setParameter('{{ max }}', $this->formatValue(10)) - ->setParameter('{{ count }}', $this->formatValue(\strlen($name))) + ->setParameter('{{ max }}', $this->formatValue($constraint->maxLength)) + ->setParameter('{{ count }}', $this->formatValue($length)) ->setCode(ColorChoices::COLOR_CHOICES_NAME_ERROR) ->addViolation(); } diff --git a/tests/Configuration/SystemConfigurationTest.php b/tests/Configuration/SystemConfigurationTest.php index 13d4435e..cb033fad 100644 --- a/tests/Configuration/SystemConfigurationTest.php +++ b/tests/Configuration/SystemConfigurationTest.php @@ -136,7 +136,7 @@ class SystemConfigurationTest extends TestCase $this->assertEquals(99, $sut->find('timesheet.active_entries.hard_limit')); $this->assertTrue($sut->find('theme.colors_limited')); $this->assertTrue($sut->isThemeColorsLimited()); - $this->assertEquals(['Maroon' => '#800000', 'Brown' => '#a52a2a', 'Red' => '#ff0000', 'Orange' => '#ffa500', '#ffffff' => '#ffffff', '#000000' => '#000000'], $sut->getThemeColorChoices()); + $this->assertEquals('Maroon|#800000,Brown|#a52a2a,Red|#ff0000,Orange|#ffa500,#ffffff,,|#000000', $sut->getThemeColorChoices()); } public function testDefaultWithLoader() @@ -148,6 +148,7 @@ class SystemConfigurationTest extends TestCase $this->assertEquals(7, $sut->find('timesheet.active_entries.hard_limit')); $this->assertFalse($sut->isSamlActive()); $this->assertFalse($sut->find('theme.colors_limited')); + $this->assertEquals('Europe/London', $sut->default('defaults.customer.timezone')); } public function testDefaultWithMixedConfigs() @@ -160,7 +161,7 @@ class SystemConfigurationTest extends TestCase ]); $this->assertFalse($sut->find('timesheet.rules.allow_future_times')); $this->assertTrue($sut->isSamlActive()); - $this->assertNull($sut->getThemeColorChoices()); + $this->assertEquals('Maroon|#800000,Brown|#a52a2a,Red|#ff0000,Orange|#ffa500,#ffffff,,|#000000', $sut->getThemeColorChoices()); $this->assertEquals('2020-03-27', $sut->getFinancialYearStart()); } diff --git a/tests/DependencyInjection/AppExtensionTest.php b/tests/DependencyInjection/AppExtensionTest.php index 2dc22b99..12c9c0ff 100644 --- a/tests/DependencyInjection/AppExtensionTest.php +++ b/tests/DependencyInjection/AppExtensionTest.php @@ -175,7 +175,7 @@ class AppExtensionTest extends TestCase 'background_color' => '#d2d6de', ], 'colors_limited' => true, - 'color_choices' => 'Kimai|#d2d6de,Silver|#c0c0c0,Gray|#808080,Black|#000000,Maroon|#800000,Brown|#a52a2a,Red|#ff0000,Orange|#ffa500,Gold|#ffd700,Yellow|#ffff00,Peach|#ffdab9,Khaki|#f0e68c,Olive|#808000,Lime|#00ff00,Jelly|#9acd32,Green|#008000,Teal|#008080,Aqua|#00ffff,LightBlue|#add8e6,DeepSky|#00bfff,Dodger|#1e90ff,Blue|#0000ff,Navy|#000080,Purple|#800080,Fuchsia|#ff00ff,Violet|#ee82ee,Rose|#ffe4e1,Lavender|#E6E6FA' + 'color_choices' => 'Silver|#c0c0c0,Gray|#808080,Black|#000000,Maroon|#800000,Brown|#a52a2a,Red|#ff0000,Orange|#ffa500,Gold|#ffd700,Yellow|#ffff00,Peach|#ffdab9,Khaki|#f0e68c,Olive|#808000,Lime|#00ff00,Jelly|#9acd32,Green|#008000,Teal|#008080,Aqua|#00ffff,LightBlue|#add8e6,DeepSky|#00bfff,Dodger|#1e90ff,Blue|#0000ff,Navy|#000080,Purple|#800080,Fuchsia|#ff00ff,Violet|#ee82ee,Rose|#ffe4e1,Lavender|#E6E6FA' ], 'kimai.theme.select_type' => 'selectpicker', 'kimai.theme.show_about' => true, diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index 4c40d832..91e7c06b 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -371,7 +371,7 @@ class ConfigurationTest extends TestCase 'background_color' => '#d2d6de' ], 'colors_limited' => true, - 'color_choices' => 'Kimai|#d2d6de,Silver|#c0c0c0,Gray|#808080,Black|#000000,Maroon|#800000,Brown|#a52a2a,Red|#ff0000,Orange|#ffa500,Gold|#ffd700,Yellow|#ffff00,Peach|#ffdab9,Khaki|#f0e68c,Olive|#808000,Lime|#00ff00,Jelly|#9acd32,Green|#008000,Teal|#008080,Aqua|#00ffff,LightBlue|#add8e6,DeepSky|#00bfff,Dodger|#1e90ff,Blue|#0000ff,Navy|#000080,Purple|#800080,Fuchsia|#ff00ff,Violet|#ee82ee,Rose|#ffe4e1,Lavender|#E6E6FA' + 'color_choices' => 'Silver|#c0c0c0,Gray|#808080,Black|#000000,Maroon|#800000,Brown|#a52a2a,Red|#ff0000,Orange|#ffa500,Gold|#ffd700,Yellow|#ffff00,Peach|#ffdab9,Khaki|#f0e68c,Olive|#808000,Lime|#00ff00,Jelly|#9acd32,Green|#008000,Teal|#008080,Aqua|#00ffff,LightBlue|#add8e6,DeepSky|#00bfff,Dodger|#1e90ff,Blue|#0000ff,Navy|#000080,Purple|#800080,Fuchsia|#ff00ff,Violet|#ee82ee,Rose|#ffe4e1,Lavender|#E6E6FA' ], 'industry' => [ 'translation' => null, diff --git a/tests/Validator/Constraints/ColorChoicesValidatorTest.php b/tests/Validator/Constraints/ColorChoicesValidatorTest.php index f913e317..9ebbe1a9 100644 --- a/tests/Validator/Constraints/ColorChoicesValidatorTest.php +++ b/tests/Validator/Constraints/ColorChoicesValidatorTest.php @@ -32,6 +32,8 @@ class ColorChoicesValidatorTest extends ConstraintValidatorTestCase yield ['#000aaa']; yield ['#fffaaa']; yield ['Foo|#fffaaa,|#fffaaa,#fffaaa,Bar|#fffaaa,']; + yield ['Fo o - sdsd|#fffaaa']; + yield ['abcdefghijklmnopqrst|#fffaaa']; yield ['']; yield [null]; } @@ -56,9 +58,9 @@ class ColorChoicesValidatorTest extends ConstraintValidatorTestCase public function getInvalidColors() { - yield ['sdf sdf|#000000', null, 'sdf sdf', '#000000']; + yield ['sdf_sdf|#000000', null, 'sdf_sdf', '#000000']; yield ['sdfghjklöß.|#aaabbb', null, 'sdfghjklöß.', '#aaabbb']; - yield ['abcdefghijklmn|#aaabbb', null, 'abcdefghijklmn', '#aaabbb']; + yield ['abcdefghijklmnopqrstu|#aaabbb', null, 'abcdefghijklmnopqrstu', '#aaabbb']; yield ['string', 'string', null]; yield ['000', '000', null]; yield ['aaa', 'aaa', null]; @@ -94,10 +96,10 @@ class ColorChoicesValidatorTest extends ConstraintValidatorTestCase } if (null !== $invalidName) { - $this->buildViolation('The given value {{ name }} is not a valid color name for {{ color }}. Allowed are {{ max }} characters, given {{ count }}.') + $this->buildViolation('The given value {{ name }} is not a valid color name for {{ color }}. Allowed are {{ max }} alpha-numerical characters, including minus and space.') ->setParameter('{{ color }}', '"' . ($invalidNameCode ?? $color) . '"') - ->setParameter('{{ max }}', '10') - ->setParameter('{{ count }}', (string) \strlen($invalidName)) + ->setParameter('{{ max }}', (string) $constraint->maxLength) + ->setParameter('{{ count }}', (string) mb_strlen($invalidName)) ->setParameter('{{ name }}', '"' . $invalidName . '"') ->setCode(ColorChoices::COLOR_CHOICES_NAME_ERROR) ->assertRaised();