Exclude/Include Pages from Shield Protection (D8 Feature Port)

Awesome patch! Works like a charm.

Added the code from #2815945: Make shield work with D8 core basic_auth (1) that allows this module to work with the basic_auth module enabled. Essentially with this patch, you can use shield with basic auth / rest by specifying your rest endpoints to exclude from shield.

Nice install file! Here is an added check for setting $bypass in the helper function. Without it, errors can get thrown like this:

Notice: Undefined variable: bypass in Drupal\shield\ShieldMiddleware->checkPath() (line 155 of /mnt/www/html/docroot/modules/contrib/shield/src/ShieldMiddleware.php) #0

This patch takes in the changes made in #4.

This works like a charm! Thank you!

I've updated the patch fixing some minor issues in the readme, consistent use of path (instead of page) naming and removing the basic auth integration that was added in #3. The last was removed as we shouldn't be combining issues into a single patch.

@badjava can you also include an diff from the previous patch?

@valthebald Here is the interdiff from #5 to #8.

Updating the patch to fix an issue with an incorrect scalar specified in the shield.schema.yml file. Replacing int with integer.

Re-rolled the patch based on latest dev and provided diff from #11 which is simply the changes in dev.

In most cases this worked
However, if you are not running Drupal on the root of the domain, this will not work

We need to change the method to get the current path

@timmillwood's comment at #2842858-18: Basic Auth module conflicts with server-level "Site Lock" implementations pointed here, and said:

then basic auth paths (such as Rest or Relaxed Web Services) need to be white-listed in shield.

I think Shield should automatically whitelist any rest routes.

This patch is whitelisting all paths with _format in query as mentioned in #14.

When I use the patch in #13 to whitelist Acquia Contenthub webhooks, I get an authentication error

Notice: Undefined offset: 1 in Drupal\shield\ShieldMiddleware->handle() (line 87 of /mnt/www/html/ffa01dev/docroot/modules/contrib/shield/src/ShieldMiddleware.php) #0 /mnt/www/html/ffa01dev/docroot/core/includes/bootstrap.inc(566): _drupal_error_handler_real(8, 'Undefined offse...', '/mnt/www/html/f...', 87, Array) #1 /mnt/www/html/ffa01dev/docroot/modules/contrib/shield/src/ShieldMiddleware.php(87): _drupal_error_handler(8, 'Undefined offse...', '/mnt/www/html/f...', 87, Array) #2 /mnt/www/html/ffa01dev/docroot/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\shield\ShieldMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #3 /mnt/www/html/ffa01dev/docroot/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(50): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #4 /mnt/www/html/ffa01dev/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #5 /mnt/www/html/ffa01dev/docroot/core/lib/Drupal/Core/DrupalKernel.php(656): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #6 /mnt/www/html/ffa01dev/docroot/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request)) #7 {main}.

If $bypass is true, it's redundant for the code to try to authenticate.

Patch to move logic from #13 around a bit.

My error happened when I didn't whitelist enough paths for contenthub - so it's possible that exception needs to be handled better, however I think the logic is the patch @17 is a bit clearer.

Reroll of #17

I corrected an error in the patch #19 because excluding paths and the method wasn't correctly save in the configuration form.

After this correction, the exclusion of some paths works well.

Good catch @Calystod - thanks.

I have taken the liberty of renaming the checkPath method to checkPathAllowed, improving its docblock and rewriting its logic.
I have also added a mention that you need a leading slash on the paths, and added an example to only shield the admin interface to the README file.

Not sure if this is related.

I've tested #21 at it works as expected. Not marking as RTBC because I'm going to do more tests (mainly using this in a dev project and check everything works). I'll post any problem encountered.

The standard in drupal is to do not include the language prefix when setting urls in configuration forms, but shield module take it in consideration, taking as diferent urls /en/whatever, /whatever and /es/whatever.

I think it should match the urls without considering the language path prefix.

I can confirm what akalam says. Patch needs to manage language prefix, so I move to Needs Work.

The way to do that would be to use the generic inbound path processing like redirect module is doing.

What I'm wondering is whether this really needs to be a middleware or if it couldn't just be a request listener, in which case the additional path processing wouldn't be necessary as that is additional overhead.

The advantage of a middleware is to be as fast as possible, especially in case you want to return early, I don't think that is a requirement here. Once you are authenticated it makes no difference and it doesn't matter if showing the authentication screen a few ms later.

Please keep in mind the work being done here: https://www.drupal.org/project/shield/issues/2815945#comment-12670696
The patch here needs to be able to apply with the patch there (or the other way round).

#27 well, we're in a bit of a bind as these patches don't currently apply together!

Would this patch work to whitelist requests that contain a specific domain, or does it take into account only paths?

Here is my use case:
I have a bunch of sites hosted in Acquia Site Factory. My sites should be publicly accessible only via the business domains, say example1.org, example2.org, etc. However, Acquia makes your sites available also under Acquia specific domains, for example site1.example.acsitefactory.com, site2.example.acsitefactory.com, etc.
To avoid this I want to enable Shield only when the request contains "site1.example.acsitefactory.com".
Whitelisting based on domains would allow me to also enable shield on certain environments (dev/test/pprod).

I see that we have patches for "path whitelisting" on this thread, and also "IP whitelisting" on https://www.drupal.org/project/shield/issues/2855364, but not "domain whitelisting".

Could this be covered as part of this ticket? If not, I'm happy to work on this feature request on a new ticket and provide a patch, if this is likely to be merged. Otherwise I will work on a new contrib module.

This issue is only about the path, domain would need to be a separate issue.

Ok thanks! Will work on it separately and open a new ticket when is ready

To me it makes sense to keep using current approach of middleware, I updated the code bit more to check for path include/exclude only if other simple conditions fail.

Last patch is creating issue for "/" (home page), updated patch.

Hopefully last try, when it comes to ShieldMiddleware.php, current language is not set properly so looping through all languages.

By using the language manager to load all language entities, there's an issue that certain entity hooks (hook_entity_load, for starters) won't be invoked correctly later in the request.

With the Shield middleware running so early in the request (before the page cache middleware), module hooks aren't loaded yet. If the language entities are loaded this early, there will be an empty array returned for entity_load implementations, which gets cached for the request.

See https://www.drupal.org/project/domain/issues/2896434, particularly starting around comment #34.

Actually, scratch the previous comment. ConfigurableLanguageManager doesn't use the Entity API to load Language objects.

#36 works for me and looks basically RTBC to me.

I tested if this works with wildcards, and it does. The help text for the paths field says "According to the Shield path method selected above, these paths will be either excluded from, or included in Shield protection. Leave this blank and select "exclude" to protect all paths. Include a leading slash." Maybe update this to say:

"According to the Shield path method selected above, these paths will be either excluded from, or included in Shield protection. Leave this blank and select "exclude" to protect all paths. Include a leading slash. The '*' character is a wildcard. An example path is /user/* for every user page. "

Just for kicks I tested ; that does not work for me, so I left that out of my suggested help text above.

#36 works for me as well. Was able to export/import config and test the module. I agree the help text could be updated. Could use a note about an item per line. However, I don't think it's a blocker. Marking RTBC

Do we need a follow up issue for #40?

Couldn't hurt to add a feature request for the help text update.

Great effort so adding to roadmap. May need a reroll though.

#40:
> Could use a note about an item per line.

Yes.

I wasn't able to apply #36, so I rerolled the patch against the latest code.

This would be a great feature! I'm looking forwart to this!

Updated patch to remove some of the PHPCS formatting errors.

Also, in the hook_help I removed some hard line breaks that would cause the paragraph to not reflow with its container width.

Patch #46 broke the "Enable Shield" checkbox. I've rerolled the patch with that fix.

The patch in #51 applies cleanly to ^1.4.

On the configuration page, I am presented a new option for Paths, with Include or Exclude variables:

In my case I included a single path, which allows me to have a single page on the site password protected.

I am going to change the status to Needs Review so the tests run.

Thanks for the module, this issue, and the work on the patch!

Rerolled patch against latest dev version.

Reuploading a version that doesn't redeclare a variable :D

Patch with this solution and #3085510: Bypass authentication/authorization for OPTIONS requests (CORS) at the same time on 1.4 version

Patch #56 applied cleanly for me and appears to work great. Really missed this feature from the D7 version.

So we have a bit of confusion here:
* #56 is not on dev and contains unrelated code so please IGNORE
(@RenatoG if you upload such convenience patches please add a big WARNING to not spread confusion)
* #54 has test failures, so needs to be fixed

Updated patch attached. This builds on #54, NOT #56.

• Fixes logic error that caused tests to fail.
• Fixed some PHPCS formatting errors.
• Replaced use of deprecated Drupal\Core\Path\AliasManager with Drupal\path_alias\AliasManager (https://www.drupal.org/node/3092086)
• Set min core version to 8.8 because of above.
• Add D9 needed $defaultTheme property to test. (https://www.drupal.org/node/3083055)

New patch, added tests.

The patch in 63 has, by my estimation, a logic problem. To reproduce:

• Set the shield path method to "include"
• Do not set any paths

Expected behavior:

• No paths are shielded

Actual behavior:

• All paths are shielded.

Patch attached.

The patch in 63 has, by my estimation, a logic problem. To reproduce:

Set the shield path method to "include"
Do not set any paths
Expected behavior:

No paths are shielded
Actual behavior:

All paths are shielded.
Patch attached.

I agree. When set to include and blank paths, it should be the opposite of the exclude and blank paths as described:

According to the Shield path method selected above, these paths will be either excluded from, or included in Shield protection. Leave this blank and select "exclude" to protect all paths.

So:

Include and blank: No paths are protected.
Include and paths specified: Specified paths are protected, rest of site is not-protected.

Exclude and blank: All paths are protected (i.e. none excluded from protection.)
Exclude and paths specified: Specified paths are not protected, rest of site is protected.

Does that sound right?

Yes, that's the logic I'd expect. And I believe that's the logic achieved in 64.

Yes, that's the logic I'd expect. And I believe that's the logic achieved in 64.

Updated patch with tests that confirm this.

I also had the include/exclude switched in a couple of tests in #64 so I fixed them and used the include/exclude constants.

I don't have the tests directory. This file doesn't apply. How do I get I get the tests directory?

Hi @rynnnner,
If I'm not mistaken, I believe you need to checkout the 8.x-1.x branch in order to run the tests.
Those tests are not available at the tagged version 1.4.

Tested successfully with #67.

Confirmed patch at #67 works for us as well.

Actually the patch contains a bug:

$form['general']['shield_allow_cli'] is now present twice in ShieldSettingsForm

Offtopic: Additionally in ShieldSettingsForm there is a dependency injection for ModuleHandlerInterface which is not actively used. Probably we want to remove it directly. Alternatively we need to create a new issue for that.

Actually the patch contains a bug:
$form['general']['shield_allow_cli'] is now present twice in ShieldSettingsForm

Good catch. Updated patch attached. I didn't remove the ModuleHandlerInterface as its beyond the scope of this issue. I agree though that a new issue should be created for it.

Patch #73 rejected against module version 1.4.0 and Drupal version 8.9.13

@ankitv18 are you applying any other Shield module patches as well? Also, can you provide an interdiff for #75?

Patch did not apply. Re-rolled patch.

@ankitv18 @Suresh Prabhu Parkala

You'll need to use the dev version of the module for the patch to apply: composer require 'drupal/shield:1.x-dev'

This patch can be applying from v1.4

The patch in #77 looks good, and works as expected. Marking RTBC.

In this patch, there's no wildcard "*" support, I'm correct?

@introfini regarding wildcards,

+++ b/src/ShieldMiddleware.php
@@ -156,4 +192,55 @@ class ShieldMiddleware implements HttpKernelInterface {
+    // Match the path using path matcher service against paths in config.
+    $path_match = $this->pathMatcher->matchPath($path, $paths_to_check);

The pathMatcher services handles wildcards, and I have tested them with this patch and they work as expected.

@jhedstrom, sorry, I forgot the leading slash. I also can confirm it's working.

Perhaps change the field description to:

According to the Shield path method selected above, these paths will be either excluded from, or included in Shield protection. Leave this blank and select "exclude" to protect all paths. Include a leading slash. One path per line, you can use wildcard "*".

The path #77 no applied

The patch #79 applied correctly and works:

Here interdiff 2822720-77.patch 2822720_include-exclude-pages_79.patch:

diff -u b/composer.json b/composer.json
--- b/composer.json
+++ b/composer.json
@@ -7,7 +7,7 @@
         "drupal/key": "^1.0"
     },
     "require": {
-        "drupal/core": "^8.6 || ^9"
+        "drupal/core": "^8.7.7 || ^9"
     },
     "extra": {
         "branch-alias": {
diff -u b/config/schema/shield.schema.yml b/config/schema/shield.schema.yml
--- b/config/schema/shield.schema.yml
+++ b/config/schema/shield.schema.yml
@@ -30,6 +30,9 @@
     whitelist:
       type: string
       label: 'Bypass shield based on user IP'
+    domains:
+      type: text
+      label: 'Bypass shield based on domain name'
 
 shield.credentials.shield:
   type: mapping

Hi guys I am not able to find any 8.x-1.x-dev version to test this #79 patch.
I have tried this on 8.x-1,x but its failed.

There are so much of code in this single patch so we need more review on this.
Now I am changing the status to "Need review".

@Manav, #77 is the correct patch, and continues to work against the 8.x-1.x dev branch. Patch #79 is provided for those who want to patch from 8.x-1.4.

I applied the patch #77 to 8.x-1.x branch. It will be available in the next stable release. I will create a basic unit test for it. Thanks for your work on this very useful feature.

This is the commit with the unit test: https://git.drupalcode.org/project/shield/-/commit/9f9fcb8979315b4705e69...

Thanks @puddyglum. I was able to use the patch from #79 on version 1.4 and it's working well.