Provide hook_seckit_options_alter() D8

Implemented in http://cgit.drupalcode.org/seckit/commit/?id=734e0d2 in the 7.x version, and that commit pre-dates 7.x-1.9 which was the starting point for the 8.x version IIRC.

Maybe the hook was removed in favour of some alternative D8 approach?

Not being able to edit views settings I think makes this a major priority.

Raven module needs to be able to alter the CSP and CT report-uri at runtime. This works fine in the Drupal 7 version of this module, but apparently isn't yet possible in the Drupal 8 version?

I guess the D8 change record at https://www.drupal.org/node/1894902 is the place to start.

The change record's child issue https://www.drupal.org/project/drupal/issues/2301577#comment-10046822 says:

New D8 way is \Drupal::moduleHandler()->alter($type, &$data, &$context1 = NULL, &$context2 = NULL)

I'm not seeing any instances of ->alter in the D8 branch though, so I guess this really is unimplemented?

Perhaps the above is sufficient for a patch to be provided.

This appears to be trickier than I'd hoped.

Invoking an alter hook is trivial enough -- I believe we can literally substitute the code \Drupal::moduleHandler()->alter(...) in place of the original drupal_alter(...) to do that. The alter method itself is described in ModuleHandlerInterface::alter()

The problem is that we're now (at least presently) dealing directly with the Drupal 8 configuration system, rather than indirectly obtaining the config via a _seckit_get_options() function (which, in D7, read the options, merged with defaults where necessary, invoked the alter hook, and then cached and returned the results).

So how does one provide run-time overrides to the Drupal 8 configuration system?

The place to start seems to be Configuration override system and to me that seems to suggest a couple of possibilities:

1. Providing overrides from modules shows how modules can override config without any kind of hook. That sounds great, but it's a bunch more code than a simple alter hook implementation (albeit also not an absolutely crazy amount of code). I have absolutely no idea how this works in detail, though -- are the alterations executed at run-time on every request? Or are they expected to be cached and not re-generated regularly? If the latter, this would provide a much lesser level of control over the SecKit options than hook_seckit_options_alter() does, in which case I would suggest that it's insufficient. If the former... maybe this was how the authors of the D8 port of SecKit expected people to deal with this issue?

2. Global overrides points out something which is definitely on the low-level side, but is also a lot simpler, and will (I strongly suspect) provide run-time overrides.

On that basis, a potential approach is to change SecKitEventSubscriber::__construct() in src/EventSubscriber/SecKitEventSubscriber.php like so:

   /**
    * Constructs an SecKitEventSubscriber object.
    *
    * @param \Psr\Log\LoggerInterface $logger
    *   The Seckit logger channel.
    * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
    *   The config factory.
    */
   public function __construct(LoggerInterface $logger, ConfigFactoryInterface $config_factory) {
     $this->logger = $logger;
     $this->config = $config_factory->get('seckit.settings');
 +
 +   // Invoke hook_seckit_settings_alter() to allow other modules to alter them.
 +   // @see https://www.drupal.org/docs/8/api/configuration-api/configuration-override-system
 +   global $config;
 +   \Drupal::moduleHandler()->alter('seckit_settings', $config['seckit.settings']);
   }

(I've used the name "hook_seckit_settings_alter" instead of "hook_seckit_options_alter" just to follow the naming scheme used in the D8 code.)

Assuming that this gets executed on a per-request basis, that would seem somewhat similar to the Drupal 7 code.

Someone else will need to tell me whether it actually works, though.

Offhand (and notwithstanding my lack of exposure to D8 code) it didn't seem particularly Drupal-8-ish to be messing with the global variable like this; but on the other hand this is what they've documented, so perhaps it's fine. I don't know enough to say either way. I've only discussed it with one other developer, and they didn't like the idea, but it also wasn't clear to me that there was any practical alternative which facilitates arbitrary run-time alterations to the settings.

Naturally, with overrides in place, the SecKit config form will need to ensure that it is Avoiding overrides for the admin config forms.

In Drupal 7 _seckit_get_options() provides a boolean argument $alter to say whether or not the alter hook would be invoked, and so in the admin form we pass FALSE for that in order to obtain the un-altered configuration:

function seckit_admin_form() {
  // Get default/configured (and unaltered) options.
  if ($hooks = module_implements('seckit_options_alter')) {
    foreach ($hooks as $key => $module) {
      $hooks[$key] = $module . '_seckit_options_alter()';
    }
    drupal_set_message(t("Some settings may be overridden at runtime. See @hooks.", array('@hooks' => implode(', ', $hooks))), 'status', FALSE);
    $options = _seckit_get_options(TRUE, FALSE);
  }
  else {
    $options = _seckit_get_options();
  }
  $defaults = _seckit_get_options_defaults();
  ...

The problem is that we're now (at least presently) dealing directly with the Drupal 8 configuration system, rather than indirectly obtaining the config via a _seckit_get_options() function (which, in D7, read the options, merged with defaults where necessary, invoked the alter hook, and then cached and returned the results).

A third option is, of course, to revert to precisely that behaviour, and (re-)implement a function for returning all of the settings.

If the other options to alteration are considered undesirable, this might actually be the way to go.

I needed this for my current project as well, mainly to be able to define a per-request nonce and some additional CSP attributes that the module config does not support.
Without getting into the details to cover an override/alter mechanism for all settings, I'll provide a patch that only adds support for an alter hook, so as to modify the CSP directives that are build in SecKitEventSubscriber::seckitCsp.

It can be used like this:

/**
 * Implements hook_seckit_csp_directives_alter().
 */
function MY_MODULE_seckit_csp_directives_alter(&$directives) {
  // Add CSP for inline scripts in element attributes, e.g. iframes.
  $directives['script-src-attr'] = "script-src-attr 'unsafe-inline'";
  // Add the nounce to the script-src rule.
  $directives['script-src'] .= " 'nonce-" . CREATE_NONCE_FOR_REQUEST() . "'";
}

For Raven module, I ended up going the ConfigFactoryOverride route to automatically override the Security Kit configuration - but still would be nice to see an easier way for modules to alter the CSP header...

Moving this to the 2.x branch, and adding Needs tests tag.

I noticed a couple minor issues, the new $module_handler parameter should be described in comment, and I guess a post_update is needed to force a rebuild for the new parameter.

Looking briefly at this again, I am strongly in favour of #9: Do more or less exactly what we do in Drupal 7, and then invoke the alter hook from that function. AFAICS Drupal simply isn't providing any sane alternative to that (very simple) solution, which will add a fairly important feature that's been missing from the D8 version of this module ever since it was ported.

Note that as of https://www.drupal.org/project/seckit/issues/3052779#comment-14399405 the D7 hook_seckit_options_alter() enables users to add CSP directives as well as modify existing ones, so this ability should be maintained in the D8+ port.

@jweowu: Sounds good to me. I'm setting this issue to needs work as the supplied patch works fine for altering CSP, but needs to support other seckit options.

FYI, CSP module has pretty nice developer experience, as a CSP event subscriber can call Csp::fallbackAwareAppendIfEnabled() - https://git.drupalcode.org/project/csp/-/blob/8.x-1.x/src/Csp.php#L318 - and it will take care of all the logic of adding a CSP directive. But just an old-fashioned hook_seckit_options_alter() would be good enough, as the issue summary proposed - I don't have any strong opinion on exactly how seckit architects this :)

Hi @jweowu & @mfb, I've attached a patch which attempts to replicate (D9 style) how the D7 version of seckit alters options via hook as mentioned in #12. Some of the patterns in the D7 still seem to make sense currently so some logic is replicated. An initial test is included, but could be expanded on.

Since we're dealing with the configuration system and we don't want overrides appearing in the config form, getSeckitConfig() interacts with ImmutableConfig, and caches the overrides as advised since it runs more than once during the request cycle.

Including the "Needs manual testing" tag :^)

Ah, wasn't using the same pattern for the report path seen in the other tests. Updated patch.

@jnlar I tested out converting some code that used config.factory.override to use this hook instead, and seems to work fine. The one gotcha I ran into was that the style-src key is missing from the config install yml file, so my code has to check if it exists. We could open a separate issue for that.

Looks like you have a test so I also removed the "Needs tests" tag.

To flush caches, I would suggest adding an empty post_update function, rather than a hook_update_N() with drupal_flush_all_caches(), reason being that the way you wrote it will AFAIK result in flushing the caches twice.

You can remove the core_version_requirement key from the test module.

There are some coding standards messages for your patch, but looks like the 2.x branch already has plenty of those so not sure if it's an issue for the maintainers.

@mfb Thanks for reviewing.

Opening a separate issue for adding the style-src key in config install sounds good.

The way I wrote it will indeed flush the caches twice. I've removed the hook_update_N() in favour of an empty hook_post_update_NAME().

Wouldn't want to creep in more coding standards issues so I've looked at the ones in the modified files.

Created an issue for adding the CSP setting style-src to seckit.settings.yml

We've been using #18 to adjust the CSP to allow for some rich legacy experiences that take over the page. We're taking configuration from Composer files in separate repositories to adjust the CSP rules for particular page requests -- highly custom but very effective. This patch has been of great benefit to us; and given that the patch provides new tests for this case, and that they're all passing, I'm willing to RTBC this.

As of 2.0.2 this patch no longer applies to 2.x

I've tried rerolling #18 against the latest 2.x. It seems to work but I reckon it needs a bit of a look. If anyone else wants to check the branch out and make changes that's all good.

I've tried #22 and it seems to work well for my use case at least (Adding to CSP domains list)

Hi all,

Pulled in #22 and the changes look OK on my end :^) CI is passing + I've done some manual testing of the hook with things such as per request nonces in the CSP and blocking based on the CSRF Origin header.

I've created a quick patch based on the merge request #22 as an interim solution.