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)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

We 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.

cweagans’s picture

Title: Make HTTP Basic Authentication provider optional/enable-able » Move HTTP authentication provider to a separate module
cweagans’s picture

Title: Move HTTP authentication provider to a separate module » Move HTTP basic authentication provider to a separate module
cweagans’s picture

Status: Active » Needs review
FileSize
6.63 KB

Something 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).

Status: Needs review » Needs work

The last submitted patch, 2041885_04-move-http-auth-to-separate-module.patch, failed testing.

BTMash’s picture

Status: Needs work » Needs review
FileSize
7.83 KB

I'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).

tim.plunkett’s picture

It is also needed by Drupal\rest\test\AuthTest

klausi’s picture

Status: Needs review » Needs work

core/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?

BTMash’s picture

Status: Needs work » Needs review
FileSize
4.29 KB

I **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.

Status: Needs review » Needs work

The last submitted patch, 2041885.patch, failed testing.

BTMash’s picture

Status: Needs work » Needs review

#9: 2041885.patch queued for re-testing.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me on visual inspection.

tim.plunkett’s picture

+1

Crell’s picture

Issue tags: +WSCCI

Tagging for posterity.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Needs a hook_help().

BTMash’s picture

Status: Needs work » Needs review
FileSize
4.66 KB
667 bytes

Implemented a basic hook_help. Also providing the interdiff.

BTMash’s picture

#16: 2041885.patch queued for re-testing.

catch’s picture

Status: Needs review » Needs work

That's probably enough, but c&p error on the help path:

> +    case 'admin/help#email':
BTMash’s picture

Status: Needs work » Needs review
FileSize
4.66 KB

Attaching patch with fixed c/p.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me! Thanks so much @BTMash.

klausi’s picture

+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.

webchick’s picture

Doesn'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?

klausi’s picture

The 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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, 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!

jibran’s picture

I think we need to add CHANGELOG.txt entry.

klausi’s picture

Automatically closed -- issue fixed for 2 weeks with no activity.

chx’s picture

Title: Move HTTP basic authentication provider to a separate module » Remove the HTTP basic authentication module
Issue summary: View changes
Status: Closed (fixed) » Active

What the heck is this and what it's doing in core? Let's remove it altogether, even the issue summary says

The functionality won't be needed on many Drupal sites

rfay’s picture

For 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).

  public function applies(Request $request) {
    $username = $request->headers->get('PHP_AUTH_USER');
    $password = $request->headers->get('PHP_AUTH_PW');
    return isset($username) && isset($password);
  }

PHP_AUTH_USER and PW aren't available in a fastcgi environment.

catch’s picture

Status: Active » Closed (fixed)

Please 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.

klausi’s picture

@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:

/*
             * php-cgi under Apache does not pass HTTP Basic user/pass to PHP by default
             * For this workaround to work, add these lines to your .htaccess file:
             * RewriteCond %{HTTP:Authorization} ^(.+)$
             * RewriteRule .* - [E=HTTP_AUTHORIZATION:%{HTTP:Authorization}]
             *
             * A sample .htaccess file:
             * RewriteEngine On
             * RewriteCond %{HTTP:Authorization} ^(.+)$
             * RewriteRule .* - [E=HTTP_AUTHORIZATION:%{HTTP:Authorization}]
             * RewriteCond %{REQUEST_FILENAME} !-f
             * RewriteRule ^(.*)$ app.php [QSA,L]
             */

@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.

rfay’s picture

@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...

BTMash’s picture

Starting 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).

tim.plunkett’s picture

Title: Remove the HTTP basic authentication module » Move HTTP basic authentication provider to a separate module

Removing this is fine, but that should happen in a new issue. Retitling this for posterity.

catch’s picture