Problem/Motivation
In the D7 version of Shield, we had the ability to Include or Exclude paths from Shield protection. I think this is still a valid use case for the D8 version as well. Specifically, I'm using the CORS contrib module to protect a REST API endpoint in Drupal that JS clients need to pull data from - the way Shield implements Basic Authentication is fundamentally incompatible with CORS (see S.O. summary of issue). I need to exclude my API URLs from the Shield.
Proposed resolution
Port the D7 include/exclude functionality to D8. The PathMatcher class makes this pretty straightforward.
Remaining tasks
I've updated the documentation, but this module doesn't provide any tests today.
User interface changes
I added configuration variables to the admin form at /admin/config/system/shield along with relevant descriptions.
API changes
This module does not have an API.
Data model changes
I'm adding two new CMI variables: method and pages, which correspond to their D7 equivalents. I added an update hook to add the CMI vars to site installs that have already installed Shield, as well as sane defaults in shield.settings.yml
Comment | File | Size | Author |
---|---|---|---|
#86 | interdiff_2822720-77-79.txt | 662 bytes | keopx |
#79 | 2822720_include-exclude-pages_79.patch | 27.81 KB | jenue1933 |
Comments
Comment #2
msherron CreditAttribution: msherron at Acquia commented.. and here's the patch.
Comment #3
japerryAwesome 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.
Comment #4
msherron CreditAttribution: msherron at Acquia commentedThanks, Jakob. I forgot to commit the install file, which adds the new CMI vars for existing Shield installs.
Comment #5
japerryNice 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.
Comment #6
msherron CreditAttribution: msherron at Acquia commentedComment #7
valthebaldThis works like a charm! Thank you!
Comment #8
badjava CreditAttribution: badjava at Metasun for Pfizer, Inc. commentedI'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.
Comment #9
valthebald@badjava can you also include an diff from the previous patch?
Comment #10
badjava CreditAttribution: badjava at Metasun for Pfizer, Inc. commented@valthebald Here is the interdiff from #5 to #8.
Comment #11
badjava CreditAttribution: badjava at Metasun for Pfizer, Inc. commentedUpdating the patch to fix an issue with an incorrect scalar specified in the shield.schema.yml file. Replacing int with integer.
Comment #12
badjava CreditAttribution: badjava at Metasun for Pfizer, Inc. commentedRe-rolled the patch based on latest dev and provided diff from #11 which is simply the changes in dev.
Comment #13
oinuma CreditAttribution: oinuma commentedIn 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
Comment #14
Wim Leers@timmillwood's comment at #2842858-18: Basic Auth module conflicts with server-level "Site Lock" implementations pointed here, and said:
I think Shield should automatically whitelist any
rest
routes.Comment #15
mirom CreditAttribution: mirom at Cheppers for Acquia commentedThis patch is whitelisting all paths with _format in query as mentioned in #14.
Comment #16
simeWhen I use the patch in #13 to whitelist Acquia Contenthub webhooks, I get an authentication error
If $bypass is true, it's redundant for the code to try to authenticate.
Comment #17
simePatch to move logic from #13 around a bit.
Comment #18
simeMy 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.
Comment #19
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedReroll of #17
Comment #20
clairedesbois@gmail.comI 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.
Comment #21
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedGood 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.
Comment #22
geek-merlinNot sure if this is related.
Comment #23
tunicI'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.
Comment #24
akalam CreditAttribution: akalam at Metadrop commentedThe 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.
Comment #25
tunicI can confirm what akalam says. Patch needs to manage language prefix, so I move to Needs Work.
Comment #26
BerdirThe 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.
Comment #27
bserem CreditAttribution: bserem at zehnplus commentedPlease 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).
Comment #28
sime#27 well, we're in a bit of a bind as these patches don't currently apply together!
Comment #29
acontia CreditAttribution: acontia commentedWould 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.
Comment #30
BerdirThis issue is only about the path, domain would need to be a separate issue.
Comment #31
acontia CreditAttribution: acontia commentedOk thanks! Will work on it separately and open a new ticket when is ready
Comment #32
nikunjkotechaTo 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.
Comment #33
nikunjkotechaComment #34
nikunjkotechaComment #35
nikunjkotechaLast patch is creating issue for "/" (home page), updated patch.
Comment #36
nikunjkotechaHopefully last try, when it comes to ShieldMiddleware.php, current language is not set properly so looping through all languages.
Comment #37
godotislate CreditAttribution: godotislate commentedBy 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.
Comment #38
godotislate CreditAttribution: godotislate commentedActually, scratch the previous comment. ConfigurableLanguageManager doesn't use the Entity API to load Language objects.
Comment #39
caspervoogt CreditAttribution: caspervoogt at Plethora commented#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.
Comment #40
johnpicozzi#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
Comment #41
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedDo we need a follow up issue for #40?
Comment #42
johnpicozziCouldn't hurt to add a feature request for the help text update.
Comment #43
geek-merlinGreat effort so adding to roadmap. May need a reroll though.
Comment #44
geek-merlinComment #45
geek-merlin#40:
> Could use a note about an item per line.
Yes.
Comment #46
dshields CreditAttribution: dshields commentedI wasn't able to apply #36, so I rerolled the patch against the latest code.
Comment #49
handkerchiefThis would be a great feature! I'm looking forwart to this!
Comment #50
PCate CreditAttribution: PCate as a volunteer commentedUpdated 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.
Comment #51
partdigital CreditAttribution: partdigital commentedPatch #46 broke the "Enable Shield" checkbox. I've rerolled the patch with that fix.
Comment #52
thejimbirch CreditAttribution: thejimbirch at Kanopi Studios commentedThe 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!
Comment #53
Sophie.SKRerolled patch against latest dev version.
Comment #54
Sophie.SKReuploading a version that doesn't redeclare a variable :D
Comment #56
renatogPatch with this solution and #3085510: Bypass authentication/authorization for OPTIONS requests (CORS) at the same time on 1.4 version
Comment #57
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedComment #58
PCate CreditAttribution: PCate as a volunteer commentedPatch #56 applied cleanly for me and appears to work great. Really missed this feature from the D7 version.
Comment #59
PCate CreditAttribution: PCate as a volunteer commentedComment #60
geek-merlinSo 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
Comment #61
PCate CreditAttribution: PCate as a volunteer commentedUpdated patch attached. This builds on #54, NOT #56.
Drupal\Core\Path\AliasManager
withDrupal\path_alias\AliasManager
(https://www.drupal.org/node/3092086)$defaultTheme
property to test. (https://www.drupal.org/node/3083055)Comment #62
PCate CreditAttribution: PCate as a volunteer commentedComment #63
PCate CreditAttribution: PCate as a volunteer commentedNew patch, added tests.
Comment #64
azinck CreditAttribution: azinck at Forum One commentedThe patch in 63 has, by my estimation, a logic problem. To reproduce:
Expected behavior:
Actual behavior:
Patch attached.
Comment #65
PCate CreditAttribution: PCate as a volunteer commentedI agree. When set to include and blank paths, it should be the opposite of the exclude and blank paths as described:
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?
Comment #66
azinck CreditAttribution: azinck at Forum One commentedYes, that's the logic I'd expect. And I believe that's the logic achieved in 64.
Comment #67
PCate CreditAttribution: PCate as a volunteer commentedUpdated 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.
Comment #68
rynnnner CreditAttribution: rynnnner commentedI don't have the tests directory. This file doesn't apply. How do I get I get the tests directory?
Comment #69
andre.bonon CreditAttribution: andre.bonon at CI&T commentedHi @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.
Comment #70
puddyglumTested successfully with #67.
Comment #71
pavlosdan CreditAttribution: pavlosdan at Acquia commentedConfirmed patch at #67 works for us as well.
Comment #72
sfuchsbe CreditAttribution: sfuchsbe at Nestlé commentedActually the patch contains a bug:
$form['general']['shield_allow_cli']
is now present twice in ShieldSettingsFormOfftopic: 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.
Comment #73
PCate CreditAttribution: PCate as a volunteer commentedGood 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.Comment #74
PCate CreditAttribution: PCate as a volunteer commentedComment #75
ankitv18 CreditAttribution: ankitv18 as a volunteer and commentedPatch #73 rejected against module version 1.4.0 and Drupal version 8.9.13
Comment #76
PCate CreditAttribution: PCate as a volunteer commented@ankitv18 are you applying any other Shield module patches as well? Also, can you provide an interdiff for #75?
Comment #77
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedPatch did not apply. Re-rolled patch.
Comment #78
PCate CreditAttribution: PCate as a volunteer commented@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'
Comment #79
jenue1933 CreditAttribution: jenue1933 commentedThis patch can be applying from v1.4
Comment #80
jhedstromThe patch in #77 looks good, and works as expected. Marking RTBC.
Comment #81
introfini CreditAttribution: introfini at Bloomidea commentedIn this patch, there's no wildcard "*" support, I'm correct?
Comment #82
jhedstrom@introfini regarding wildcards,
The
pathMatcher
services handles wildcards, and I have tested them with this patch and they work as expected.Comment #83
introfini CreditAttribution: introfini at Bloomidea commented@jhedstrom, sorry, I forgot the leading slash. I also can confirm it's working.
Perhaps change the field description to:
Comment #84
japerryComment #85
keopxThe path #77 no applied
The patch #79 applied correctly and works:
Here interdiff 2822720-77.patch 2822720_include-exclude-pages_79.patch:
Comment #86
keopxComment #87
Manav CreditAttribution: Manav as a volunteer and for Drupal India Association commentedHi 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".
Comment #88
Manav CreditAttribution: Manav as a volunteer and for Drupal India Association commentedComment #89
puddyglum@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.
Comment #91
vbouchetI 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.
Comment #92
vbouchetThis is the commit with the unit test: https://git.drupalcode.org/project/shield/-/commit/9f9fcb8979315b4705e69...
Comment #94
Kristen PolThanks @puddyglum. I was able to use the patch from #79 on version 1.4 and it's working well.