Conversation
Co-authored-by: Davide Zeni <davidezeni02@gmail.com>
This commit also adds headers to the general config section
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
| use ipl\Web\Compat\CompatForm; | ||
| use ipl\Web\Widget\Callout; | ||
|
|
||
| class CspConfigForm extends CompatForm |
There was a problem hiding this comment.
This currently doesn't output the config.ini to the screen like the old ConfigForm did.
I think changing the form type to a CompatForm is still the right call, because we don't want to rely on Zend_Forms forever.
If this behavior essential, we should reimplement the behavior of ConfigFrom as a CompatForm instead.
There was a problem hiding this comment.
Even in new modules we reimplemented this behavior: Icinga/icingadb-web#1269
So I'd not downgrade this functionality.
Al2Klimov
left a comment
There was a problem hiding this comment.
16 review comments should be enough for a Monday 16th. :)
| use ipl\Web\Compat\CompatForm; | ||
| use ipl\Web\Widget\Callout; | ||
|
|
||
| class CspConfigForm extends CompatForm |
There was a problem hiding this comment.
What about always showing the generated CSP? Even if they're not in effect, they're a good cheat sheet, especially for a custom CSP.
There was a problem hiding this comment.
I'm personally not a fan of that, because it might confuse people about what is currently active and that isn't.
I think waiting for @flourish86 to design something and talking about that with @lippserd is the right call here.
| 'frame-src' => [$absoluteUrl], | ||
| ], | ||
| 'reason' => [ | ||
| 'type' => 'dashlet', |
There was a problem hiding this comment.
Yes. I agree, that was my first idea as well, but then I didn't want to introduce classes that are exclusively used inside the Csp-System.
PS: I think you wanted to link Icinga/ipl-web#358
This makes it compatible with previous versions.
This commit also adds parsing to the policies. Turning urls into clickable links, and coloring potentially dangerous policies orange.
| $section['use_strict_csp'] = $this->getValue('use_strict_csp'); | ||
| if ($this->isCspEnabled()) { | ||
| $section['use_custom_csp'] = $this->getValue('use_custom_csp'); | ||
| if ($this->isCustomCspEnabled()) { |
There was a problem hiding this comment.
We have three states here:
- No CSP
- Generated CSP
- Custom CSP
Ideal use case for a dropdown.
@flourish86 What do you think?
(I.e use_strict_csp would keep values "0" and "1" internally for compatibility and add "custom".)
There was a problem hiding this comment.
A dropdown would be the easiest choice here. That is a common pattern for forms in icingaweb. I personally like the two "nested" checkboxes, because it makes it clearer what you are doing in a techncal level.
| html.no-js .progress-label { | ||
| display: none; | ||
| } | ||
|
|
| use ipl\Web\Compat\CompatForm; | ||
| use ipl\Web\Widget\Callout; | ||
|
|
||
| class CspConfigForm extends CompatForm |
There was a problem hiding this comment.
Consider a toggle which appears along with generated CSP which controls whether to include user-entered stuff, such as dashlets, in the CSP.
| ); | ||
|
|
||
| $this->addPolicyTable( | ||
| t('Dashboard'), |
There was a problem hiding this comment.
If you're concerned about #5477 (comment), you should be about this one too:
As not a IW2 dev, I'd have absolutely no clue that these are only my dashboards (if any!) and others may have their own.
Consider listing the dashboard-CSPs of all users, after all we have them (by name) in the preference store or something idk.
Module-provided dashboards should already be included as you should see all modules as admin. I guess they may contain external URLs as well.
| 'class' => 'collapsible', | ||
| 'data-visible-height' => 250, | ||
| ], | ||
| new CspConfigurationTable(), |
There was a problem hiding this comment.
With the table, this form is already large enough to have its own IW2 configuration tab.
- While on it, Implement password policy with hook #5419 could also go there.
CC @JolienTrog
| ), | ||
| 'class' => 'autosubmit', | ||
| 'checkedValue' => '1', | ||
| 'uncheckedValue' => '0', |
There was a problem hiding this comment.
New form elements don't need these, do they?
| ]); | ||
|
|
||
| $cspForm->on(ContractForm::ON_SUBMIT, function (CspConfigForm $form) use ($config) { | ||
| if ($form->isCspEnabled() && $form->hasConfigChanged()) { |
There was a problem hiding this comment.
What if I disable the CSP? Don't I deserve a reload?
|
|
||
| public function __construct() | ||
| { | ||
| $this->getAttributes()->add('class', 'csp-config-table'); |
There was a problem hiding this comment.
I wonder if BaseHtmlElement#defaultAttributes can be used instead.
| return Table::tr([ | ||
| Table::td($reason['navType']), | ||
| Table::td($reason['name']), | ||
| Table::td($reason['parent'] ?? 'NA'), |
| if ($policy === '*') { | ||
| $result = HtmlElement::create('span', ['class' => 'wildcard'], $policy); | ||
| } elseif ($policy === "'self'") { | ||
| $result = HtmlElement::create('span', ['class' => 'self'], $policy); |
There was a problem hiding this comment.
@flourish86 I wonder if such generic names for CSS classes could clash with new ones in the future.
| } | ||
| } | ||
|
|
||
| unset($policyDirectives); |
Taking over #5337 (#5337 (comment)) and implementing an override for a completely custom CSP-Header.
As well as adding a table below the form which displays the source of the automatically generated CSP-Header.
Styling for this table is still WIP.
requires Icinga/ipl-web#358
closes #5337
closes #5333