IP whitelist and enable/disable option as config

Wow, totally read my mind. Was just coming here to ask about this for this exact reason. I want to toggle functionality from settings.php for different environments.

Updated patch due to changes in install schema (commit f48a53f0d7a658abbdf4eb68f3877a6ee4d9227b)

Thanks Spurlos, the patch #6 works

The whitelist functionality is fine. I don't like the idea of a configuration item to disable it. Shield is to protect a site. If shield is enabled, I expect the site to be protected. I might even add it to a core module info file to prevent it from being disabled. But if we introduce a configuration kill-switch, then this could easily be missed, if/when someone changes the configuration accidentally or maliciously.

If I write a custom module and add shield as a dependency, it can't be disabled easily. With the enable/disable option though, it will be easy to disable and harder to prevent. I still don't like the feature. I think you can control it for dev/stage/prod simply in your settings.php (use a different one per environment) and set or don't set shield.user and shield.pass.

IP whitelisting for the shield module is a much needed feature for us, and was included in the D7 version of this module. We use config split to enable it on our dev and test environments (or under-development prod enviros) and we often to need to send folks to test features or enter content.

No matter how many times we explain the process, having to "login" to shield then "login" to Drupal is very confusing to virtually every single one of users (PhDs, MDs, and HS grads alike), so having the ability to whitelist our internal IPs makes my life easier. Plus 1 for this patch.

As a maintainer i give a fullhearted placet to bringing back the enable setting.
Reviewed the code, looks totally straightforward.
Tested in #7, so RTBC.
(Will commit in my next console session.)

❤ Thanks everyone!! ❤

I think we need an update hook so that a protected site does not lose protection.

Does this whitelist only work for IP addresses? I'm looking at the use case of removing basic authentication for CDN requests so we can test it on the staging site.

+++ b/shield.install
@@ -28,3 +28,35 @@ function shield_update_8002() {
+function shield_update_8003() {
+  $config = \Drupal::configFactory()->getEditable('shield.settings');
+

Do we really want getEditable (non-overridden config) here? I think it's a nontrivial question. What if an empty username is overridden with a nonempty one? What if a nonempty username is overridden with an empty one? In both cases i tend to take the overridden, effective value into account. Thoughts?

The rest is straightforrard and well-done!

I think if we settle this question, and someone does reasonable manual tests of the new functionalities and the update path, this can well go in!

It was not a good idea to include two separate feature changes into the same issue. Now the IP white list cannot be committed before the "enable" switch is decided and vice versa.

1. Whitelist: In a duplicate issue Whitelisting ip addresses is static - Drupal 8 port the suggested patch implements IP or Subnet Mask listing, instead of just IP. It might be worth considering that implementation.
2. Enable: I believe the "enable" checkbox is slightly unnecessary, as the functionality can be disabled by leaving the credentials empty. Yet, I agree that having a checkbox is more intuitive, and it also allows temporarily disabling the shield without reseting the credentials.
   Considering that nothing breaks if we check "enable" with an empty username, it's safer to enable than leave a site unprotected.

   Please note that adding the "enable" option should also be described in the documentation, and it will conflict with enable/disable in settings.php

Updated the patch as follows:
1. Changed the update hook to get the effective value instead of the stored value.
2. Added support for Network ranges. Now using Symfony\Component\HttpFoundation\IpUtils to check the IP.
3. Added trim() to allow whitespaces in the list. There is no need for validation of the input, because IpUtils::checkIp() handles that.
4. Changed field descriptions and added instructions in Readme. Added red color to the reverse proxy caching issue warning, because it's very important.

Interdiff file attached.

+1 for the trim, was going to mention that as an issue.

I also wonder, how do you feel about a multi-line text field for the IP address, rather than one comma-separated line? I feel like this would be more consistent with how multi-value fields are presented throughout Drupal. It also is much easier to read than one long line of text.

This patch is working off #21, but changes the input to a textarea, and the delimiter to "\n". I've also moved the example values from the description into the placeholder, since it was getting a little messy having a multi-line example in the description.

Thank you, I agree that a text area is better.
I assume that tcfunk has reviewed the patch from #21, and axel.rutz and I reviewed the previous versions, so I'm setting the issue RTBC.

Different line endings should not be a problem when exploding by "\n".
When the input uses "\r\n", the lines will be correctly separated, and trim() will remove the "\r".
Line ending with "\r" alone is not standard, and it hasn't been used in Macs since 2001. There is no need to consider it.

There are plenty of examples in core where new lines are exploded by "\n", and nobody has ever complained :)

I did some digging through core source to see how Drupal handles the multiple-value fields. Looks like it uses the same explode by "\n" method. The method I'm looking at is here:

core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php, line 173

protected static function extractAllowedValues($string, $has_data) {
    $values = [];

    $list = explode("\n", $string);
    $list = array_map('trim', $list);
    $list = array_filter($list, 'strlen');
    ...

If anything it could be worth adding the same $list = array_filter($list, 'strlen'); to our code to filter out empty items.

It's already there, just in a single line:
$whitelist = array_filter(array_map('trim', explode("\n", $whitelist)));

Confirming RTBC. There are several RTBC'd issues for the D8 branch. Is there an active maintainer able to have a look and create a new release?

> It was not a good idea to include two separate feature changes into the same issue. Now the IP white list cannot be committed before the "enable" switch is decided and vice versa.

Yes.

Finally committed this. Please look into the followups. And a big ❤️ to everybody!

Thank you very much Axel! That's really useful, love to see it back in D8.

Tested the patch in #22 https://www.drupal.org/project/shield/issues/2855364#comment-12576428 and observed the following:

     else {
      // Check if user IP is in whitelist.
      $in_whitelist = FALSE;
      if ($whitelist = $config->get('whitelist')) {
        $whitelist = array_filter(array_map('trim', explode("\n", $whitelist)));
        $in_whitelist = IpUtils::checkIp($request->getClientIp(), $whitelist);
      }

This code doesn't check if whitelist is configured. If no Ip's are whitelisted/added to the configuration, $in_whitelist will always be false. Thus, the authentication will always be prompted. If $whitelist is empty, $in_whitelist should be true OR logic shouldn't be processed against it.

$in_whitelist = IpUtils::checkIP($request->getClientIp(), $whitelist)

I think IpUtils::checkIP($request->getClientIp(), $whitelist) introduces unwanted complexity to checking if a ip is included in whitelisted ip configuration. Restrict IP implements similar functionality to whitelist IP's, see here. I've configured ip's successfully using Restrict IP and observed the expected whitelisting. I think there is room to adopt how that module checks ips, here

Next Steps

- Review similar fnctionality used in other contrib modules.
- Create patch

Follow-up: #3052723: Add whitelist default in shield.settings.yml