Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The HttpBasic authentication provider is currently enabled for every single page request in the critical performance path. The functionality won't be needed on many Drupal sites, so this should only be added if it gets enabled explicitly by the site administrator.
I see 2 approaches:
* move it to a separate module that can be enabled
* add a configuration file that specifies which authentication providers are enabled (you could even disable session cookie authentication with that)
Comment | File | Size | Author |
---|---|---|---|
#19 | 2041885.patch | 4.66 KB | BTMash |
#16 | 2041885.interdiff.txt | 667 bytes | BTMash |
#16 | 2041885.patch | 4.66 KB | BTMash |
#9 | 2041885.patch | 4.29 KB | BTMash |
#6 | 2041885.patch | 7.83 KB | BTMash |
Comments
Comment #1
Crell CreditAttribution: Crell commentedWe discussed this on the REST call today, and suggested that we should move basic auth to its own module but then move basic_auth, rest, hal, etc. to a new "Web services" package on the modules page. That would help with the UX question of "wtf do all these modules do?" If you're doing web services stuff, though, knowing what those modules are is an entirely reasonable expectation.
Comment #2
cweagansPer #1. Also opened #2042443: Move modules related to web services to a separate package on the module listing page
Comment #3
cweagansComment #4
cweagansSomething like this, perhaps? I'm not really sure how to manually test this, and I didn't see any tests for this authentication provider (granted, I didn't look very hard, so it's very possible that I just overlooked it).
Comment #6
BTMash CreditAttribution: BTMash commentedI'm coming to this issue from #2073041: Cannot log into site after installation. Seems like the initial failure came about from core.services.yml. Updated the patch to include the changes to that portion. The patch did look good to me but could certainly user more eyes (and atleast fixed the immediate issue when I have a site behind a password-protected basic auth but still want to authenticate w/ Drupal).
Comment #7
tim.plunkettIt is also needed by Drupal\rest\test\AuthTest
Comment #8
klausicore/modules/system/lib/Drupal/system/Tests/Authentication/HttpBasicTest.php - that test case should be moved as well?
And could create the patch with git diff -M, so that we see the renamed file?
Comment #9
BTMash CreditAttribution: BTMash commentedI **think** I've accounted for the rest tests and the move over to basic_auth. The patch is with git diff -M so lets see how it goes.
Comment #11
BTMash CreditAttribution: BTMash commented#9: 2041885.patch queued for re-testing.
Comment #12
Crell CreditAttribution: Crell commentedThis looks good to me on visual inspection.
Comment #13
tim.plunkett+1
Comment #14
Crell CreditAttribution: Crell commentedTagging for posterity.
Comment #15
catchNeeds a hook_help().
Comment #16
BTMash CreditAttribution: BTMash commentedImplemented a basic hook_help. Also providing the interdiff.
Comment #17
BTMash CreditAttribution: BTMash commented#16: 2041885.patch queued for re-testing.
Comment #18
catchThat's probably enough, but c&p error on the help path:
Comment #19
BTMash CreditAttribution: BTMash commentedAttaching patch with fixed c/p.
Comment #20
tim.plunkettLooks good to me! Thanks so much @BTMash.
Comment #21
klausi+1 from me as well.
Note that we are introducing a new package "Web services" here for a core module. This is not the first time since we already have "Multilingual" as package for other core modules, so this is OK.
Posted #2092817: Move REST, serialization and HAL to the web services package + CHANGELOG.txt entry as follow-up.
Comment #22
webchickDoesn't necessarily need to hold this up but... isn't a module a bit heavy for something like this? Seems like it could just be plugins provided by REST module?
Comment #23
klausiThe HTTP basic authentication provider could live in REST module, but it does not depend on REST module nor does REST module depend on that particular provider.
If contrib wants to build cool stuff on top of HTTP Basic auth then it would always have to enable the full REST module. If I use REST module and OAuth from contrib then I don't care at all about basic auth, which would still be enabled with REST module.
So as said in the issue summary: either we make authentication providers configurable to be enabled or we move them to separate modules. We decided for separate modules here, because that is less work for us.
Comment #24
webchickOk, that makes sense. At some point I'd love to take a more holistic look at the general pattern of "we need a way to turn on/off individual features without making it a user-facing module" problem but that doesn't sound like it's in this issue. :)
Lin took a look at this and confirmed it's good. So...
Committed and pushed to 8.x. Thanks!
Comment #25
jibranI think we need to add CHANGELOG.txt entry.
Comment #26
klausilet's do that in #2092817: Move REST, serialization and HAL to the web services package + CHANGELOG.txt entry
Comment #28
chx CreditAttribution: chx commentedWhat the heck is this and what it's doing in core? Let's remove it altogether, even the issue summary says
Comment #29
rfayFor moving the testbots to php-fpm/fastcgi, this is a fail, since it requires extra mucking about to get this stuff with fastcgi. I'd think we could remove the test (D8 and D7).
PHP_AUTH_USER and PW aren't available in a fastcgi environment.
Comment #30
catchPlease open a new issue for removing. This issue only moved the stuff out of lib (and the critical path), it did not introduce the code in the first place.
edit: I'd be fine with removing it though. It's dead code and there's other sub-systems we have genuine use cases for shipping alternatives with but don't.
Comment #31
klausi@rfay: the symfony request class (more specifically: ServerBag.php) takes care that PHP_AUTH_USER gets populated. That's why we use a standard framework in D8: to not deal with those tedious implementation details :-)
We just need to follow the documentation:
@chx: this module is useful for web services consumers that don't want to use session cookies and X-CSRF-Token for authentication. I don't think we should remove it from core. See also #2103035: Review/update hook_help() for the basic_auth module.
Comment #32
rfay@klausi it *does* look like I jumped the gun, and I don't see the test failure in D8 (WITH the extra rewrite added).
But D7 would have to process that stuff differently instead of just using the $_SERVER variable. Congrats on using Symfony...
Comment #33
BTMash CreditAttribution: BTMash commentedStarting with my side note, I had been having a rough week and seeing the thread title change and comment by chx actually made me laugh out loud. So thank you for cheering me up.
I separated the functionality out into a module primarily because I had initially wanted a site behind htpasswd (but I still wanted to be able to log into my site with my drupal credentials) and the original functionality being in lib and always enabled made it impossible (see https://drupal.org/node/2073041). It being a module seemed to be the next best option hence I went down that route.
Personally, I don't think this should be in core either. It makes a TON more sense as a contributed module (one which can have a project summary page that actually explains what the module does).
Comment #34
tim.plunkettRemoving this is fine, but that should happen in a new issue. Retitling this for posterity.
Comment #35
catchDid anyone open that issue? #2160021: Basic auth has no login flood control.