Skip to content

Commit d4f3768

Browse files
TheSyscalljrauh01
TheSyscall
and
jrauh01
authored
Apply suggestions from code review
Co-authored-by: jrauh01 <johannes.rauh@icinga.com> Co-authored-by: Alexander Rieß <alexander.riess@icinga.com>
1 parent cf6621d commit d4f3768

10 files changed

Lines changed: 69 additions & 91 deletions

File tree

‎application/forms/Config/Security/CspConfigForm.php‎

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -104,9 +104,7 @@ class CspConfigForm extends ConfigForm
104104
];
105105

106106
/**
107-
* The number of rows for the custom CSP textarea
108-
*
109-
* @var int
107+
* @var int The number of rows for the custom CSP textarea
110108
*/
111109
protected const TEXTAREA_ROWS = 8;
112110

@@ -159,7 +157,7 @@ protected function assemble(): void
159157
'p',
160158
['class' => $formHintClassList],
161159
$this->translate(
162-
'Enabling CSP will block some requests and may prevent some functionality from working as expected.'
160+
'Enabling CSP will block some requests and may prevent some functionality from working as expected.',
163161
),
164162
)))->addAttributes(Attributes::create(['class' => 'csp-control-group'])));
165163

@@ -184,10 +182,7 @@ protected function assemble(): void
184182
[Csp::getSystemCsp()],
185183
[$this->translate('Directive'), $this->translate('Value')],
186184
function (StaticCspReason $reason, string $directive, string $expression) {
187-
return Table::tr([
188-
Table::td($directive),
189-
$this->buildExpression($directive, $expression),
190-
]);
185+
return Table::tr([Table::td($directive), $this->buildExpression($directive, $expression)]);
191186
},
192187
! $useCustomCsp,
193188
$this->translate('No system policies defined.'),
@@ -277,12 +272,13 @@ function (NavigationCspReason $reason, string $directive, string $expression) {
277272
: Table::td($reason->parent);
278273

279274
$sharedIcon = match ($reason->isShared) {
280-
true => new Icon('share', [
275+
true => new Icon('share', [
281276
'class' => 'shared-item',
282277
'title' => $this->translate('Shared item. Displayed user is owner.'),
283278
]),
284279
false => null,
285280
};
281+
286282
$userCell = $reason->username === null
287283
? Table::td([$sharedIcon, $this->translate('Unknown')])->setAttribute('class', 'empty-state')
288284
: Table::td([$sharedIcon, $reason->username]);
@@ -542,12 +538,14 @@ protected function getSchemeType(string $directive, string $expression): ?string
542538
return 'warning';
543539
}
544540

545-
if ($scheme === 'data' && in_array($directive, static::CRITICAL_DATA_DIRECTIVES)) {
546-
return 'critical';
547-
}
548-
549-
if ($scheme === 'data' && in_array($directive, static::WARNING_DATA_DIRECTIVES)) {
550-
return 'warning';
541+
if ($scheme === 'data') {
542+
if (in_array($directive, static::CRITICAL_DATA_DIRECTIVES)) {
543+
return 'critical';
544+
}
545+
546+
if (in_array($directive, static::WARNING_DATA_DIRECTIVES)) {
547+
return 'warning';
548+
}
551549
}
552550

553551
return 'unknown';
@@ -591,7 +589,7 @@ protected function buildExpression(string $directive, string $expression): BaseH
591589
'class' => 'csp-expression-info',
592590
'title' => $this->translate('This is a potentially unsafe keyword.'),
593591
]),
594-
default => null,
592+
default => null,
595593
};
596594
$result = HtmlElement::create(
597595
'span',
@@ -600,15 +598,15 @@ protected function buildExpression(string $directive, string $expression): BaseH
600598
);
601599
} elseif (($scheme = $this->getSchemeType($directive, $expression)) !== null) {
602600
$icon = match ($scheme) {
603-
'warning' => new Icon('warning', [
601+
'warning' => new Icon('warning', [
604602
'class' => 'csp-expression-info',
605603
'title' => $this->translate('This is a potentially unsafe scheme.'),
606604
]),
607605
'critical' => new Icon('warning', [
608606
'class' => 'csp-expression-info',
609607
'title' => $this->translate('This is a critical scheme and should not be used.'),
610608
]),
611-
default => null,
609+
default => null,
612610
};
613611
$result = HtmlElement::create(
614612
'span',

‎application/forms/Dashboard/DashletForm.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ public function createElements(array $formData)
8989
CalloutType::Info,
9090
$this->translate(
9191
'Any external url is not guaranteed to work as expected. '
92-
. 'Please make sure to check the Content-Security-Policy configuration.'
92+
. 'Please make sure to check the Content-Security-Policy configuration.',
9393
),
9494
$this->translate('Dashboards are not enabled in the CSP configuration'),
9595
)))->render(),

‎application/forms/Navigation/NavigationItemForm.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ public function createElements(array $formData)
6262
CalloutType::Info,
6363
$this->translate(
6464
'Any external url is not guaranteed to work as expected. '
65-
. 'Please make sure to check the Content-Security-Policy configuration.'
65+
. 'Please make sure to check the Content-Security-Policy configuration.',
6666
),
6767
$this->translate('Navigation items are not enabled in the CSP configuration'),
6868
)))->render(),

‎library/Icinga/Security/Csp/Loader/ArrayCspLoader.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,8 @@ class ArrayCspLoader implements CspLoader
1818
{
1919
/**
2020
* @param string $name The name to display for CSP reason
21-
* @param array $directives The CSP directives to load
22-
* Each key is a directive name, and each value is an array of values for that directive.
21+
* @param array $directives The CSP directives to load. Each key is a
22+
* directive name, and each value is an array of values for that directive.
2323
*/
2424
public function __construct(
2525
protected string $name,

‎library/Icinga/Security/Csp/Loader/DashboardCspLoader.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ public function loadForUser(User $user): array
4242
$urlString = $url->isExternal()
4343
? $url->getAbsoluteUrl()
4444
: $url->getParam('url');
45+
4546
if ($urlString === null || filter_var($urlString, FILTER_VALIDATE_URL) === false) {
4647
continue;
4748
}

‎library/Icinga/Security/Csp/Loader/ModuleCspLoader.php‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -53,14 +53,11 @@ protected function loadWith(callable $fetch): array
5353
$moduleName = ClassLoader::extractModuleName(get_class($hook));
5454
if ($csp->hasDirective('default-src')) {
5555
throw new RuntimeException(
56-
sprintf("Setting 'default-src' is not allowed. Module: %s", $moduleName)
56+
sprintf("Setting 'default-src' is not allowed. Module: %s", $moduleName),
5757
);
5858
}
5959

60-
$result[] = new AttributedCsp(
61-
$csp,
62-
new ModuleCspReason($moduleName),
63-
);
60+
$result[] = new AttributedCsp($csp, new ModuleCspReason($moduleName));
6461
} catch (Throwable $e) {
6562
Logger::warning(
6663
"Failed to invoke CSP hook %s: %s\n%s",

‎library/Icinga/Security/Csp/Loader/NavigationCspLoader.php‎

Lines changed: 20 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -22,19 +22,16 @@
2222
class NavigationCspLoader implements CspLoader
2323
{
2424
/**
25-
* Loads CSP directives for user navigation items that have an external URL
25+
* Load CSP directives for user navigation items that have an external URL
2626
*
2727
* @param string $type The navigation type
2828
* @param array $typeConfig The navigation type configuration
29-
* @param User $user user to check access for.
29+
* @param User $user User to check access for
3030
*
3131
* @return AttributedCsp[]
3232
*/
33-
protected function loadUserConfig(
34-
string $type,
35-
array $typeConfig,
36-
User $user,
37-
): array {
33+
protected function loadUserConfig(string $type, array $typeConfig, User $user): array
34+
{
3835
$config = Config::navigation($type, $user->getUsername());
3936
if ($config->isEmpty()) {
4037
return [];
@@ -59,11 +56,12 @@ protected function loadUserConfig(
5956
}
6057

6158
/**
62-
* Loads CSP directives for shared navigation items that have an external URL
59+
* Load CSP directives for shared navigation items that have an external URL
6360
*
64-
* @param string $type the navigation type
65-
* @param array $typeConfig the navigation type configuration
66-
* @param ?User $user user to check access for. If null, all shared navigation items are loaded.
61+
* @param string $type The navigation type
62+
* @param array $typeConfig The navigation type configuration
63+
* @param ?User $user User to check access for. If null, all shared
64+
* navigation items are loaded.
6765
*
6866
* @return array
6967
*/
@@ -97,14 +95,14 @@ protected function loadSharedConfig(string $type, array $typeConfig, ?User $user
9795
}
9896

9997
/**
100-
* Parses a navigation section and returns an AttributedCsp if the section is valid and should be loaded
98+
* Parse a navigation section and return an AttributedCsp if the section is valid and should be loaded
10199
*
102-
* @param string $type 1st level navigation type
103-
* @param array $typeConfig 1st level navigation type configuration
104-
* @param string $sectionName the name of the ini section
105-
* @param ConfigObject $section the navigation section
106-
* @param bool $isShared whether the section is shared
107-
* @param ?string $username the username of the user, if the section is bound to a user
100+
* @param string $type First level navigation type
101+
* @param array $typeConfig First level navigation type configuration
102+
* @param string $sectionName The name of the ini section
103+
* @param ConfigObject $section The navigation section
104+
* @param bool $isShared Whether the section is shared
105+
* @param ?string $username The username of the user, if the section is bound to a user
108106
*
109107
* @return ?AttributedCsp
110108
*/
@@ -176,23 +174,15 @@ public function loadForUser(User $user): array
176174
$result = [];
177175
$navigationTypes = Navigation::getItemTypeConfiguration();
178176
foreach ($navigationTypes as $type => $typeConfig) {
179-
$result = array_merge($result, $this->loadSharedConfig(
180-
$type,
181-
$typeConfig,
182-
$user,
183-
));
184-
$result = array_merge($result, $this->loadUserConfig(
185-
$type,
186-
$typeConfig,
187-
$user,
188-
));
177+
$result = array_merge($result, $this->loadSharedConfig($type, $typeConfig, $user));
178+
$result = array_merge($result, $this->loadUserConfig($type, $typeConfig, $user));
189179
}
190180

191181
return $result;
192182
}
193183

194184
/**
195-
* Checks whether the user has access to a shared navigation item
185+
* Check whether the user has access to a shared navigation item
196186
*
197187
* Also handles inheritance of access restrictions. This method mimics the
198188
* behavior of {@see \Icinga\Application\Web::hasAccessToSharedNavigationItem()}.
@@ -212,11 +202,7 @@ private function hasAccessToSharedNavigationItem(ConfigObject $config, Config $n
212202
if (isset($config['parent']) && $navConfig->hasSection($config['parent'])) {
213203
$parentConfig = $navConfig->getSection($config['parent']);
214204

215-
return $this->hasAccessToSharedNavigationItem(
216-
$parentConfig,
217-
$navConfig,
218-
$user,
219-
);
205+
return $this->hasAccessToSharedNavigationItem($parentConfig, $navConfig, $user);
220206
}
221207

222208
if (isset($config['users'])) {

‎library/Icinga/Security/Csp/Reason/NavigationCspReason.php‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,12 @@
1212
readonly class NavigationCspReason implements CspReason
1313
{
1414
/**
15-
* @param string $type the type of the navigation item
16-
* @param array $typeConfiguration the configuration of the navigation item type
17-
* @param string|null $parent
15+
* @param string $type The type of the navigation item
16+
* @param array $typeConfiguration The configuration of the navigation item type
17+
* @param ?string $parent
1818
* @param string $name
1919
* @param bool $isShared
20-
* @param string|null $username
20+
* @param ?string $username
2121
*/
2222
public function __construct(
2323
public string $type,

‎library/Icinga/Security/Csp/Reason/StaticCspReason.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
readonly class StaticCspReason implements CspReason
1313
{
1414
/**
15-
* @param string $name the name to display for CSP reason
15+
* @param string $name The name to display for CSP reason
1616
*/
1717
public function __construct(
1818
public string $name,

0 commit comments

Comments
 (0)