The testing system likes to assume that HTTP Auth is supported by the PHP environment. See this new feature (#669456: Support HTTP auth method in settings) as one example; there is also a test that will not work without HTTP Auth: #668654: Tests assuming apache_mod, failing in cgi..

PHP only supports HTTP Authorization when coupled with Apache as a module - when running PHP via a CGI gateway (which is increasingly popular, offering more flexibility) the PHP_AUTH_* server variables are not available.

There is in fact a way to add support for HTTP authentication in PHP running as a CGI interface. It is a bit roundabout, but not dirty if done right, and it would fit well among the other environment clean-up Drupal does - eg. populating the HTTP_HOST, HTTP_REFERER and SERVER_PROTOCOL variables. On the Apache side, it is based on mod_rewrite, which Drupal already relies on.

I suggest that Drupal should implement this method in its .htaccess and bootstrap phase, adding support for HTTP authentication on environments where Apache and PHP are not coupled together.

Putting together a patch now.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cburschka’s picture

Status: Active » Needs review
FileSize
1.9 KB

Here is my awesome patch of awesomeness. :)

Before:

Drupal HTTP request 31 passes, 2 fails, and 0 exceptions

---- DrupalHTTPRequestTestCase ----
Fail       Other      common.test                    851                       
    $_SERVER["PHP_AUTH_USER"] is passed correctly.
Fail       Other      common.test                    852                       
    $_SERVER["PHP_AUTH_PW"] is passed correctly.

After:

Drupal HTTP request 33 passes, 0 fails, and 0 exceptions
cburschka’s picture

Category: feature » bug

Technically, this is fixing a bug caused by the PHP environment.

cburschka’s picture

To answer a question posed on IRC, this patch also does not depend on the mod_auth_basic module or in fact anything other than mod_rewrite on the Apache end.

In fact, if the plain apache_mod PHP depends on mod_auth_basic for authentication, this patch removes that dependency as well.

MichaelCole’s picture

JacobSingh’s picture

Status: Needs review » Reviewed & tested by the community

This patch looks good to me and works.

chx’s picture

Status: Reviewed & tested by the community » Needs work

we do not want this code piled in the critical path. more and more and more code goes there. this is really really an edge case. .htaccess is fine. Providing a helper or even doing it in full bootstrap is , I guess, fine. But not up high.

cburschka’s picture

I'm sorry but I do not quite understand. Would you prefer the bootstrap code moved to a different phase?

JacobSingh’s picture

@chx: fcgi is not an edge case anymore. Dreamhost for instance? Or Acquia Hosting? There are lots of cgi hosts. This particular functionality is an edge case, but that doesn't mean all those people should hack core. I don't see where else this would go, and since that var is only set for cgi hosts, I don't see how it could affect performance.

Best,
J

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Get other reviewers then?

pwolanin’s picture

@chx - if we want to be able to check authorization it seems like we do need these populated quite early in the request, right?

effulgentsia’s picture

No comment on the merits of this patch. Just a reroll to chase HEAD.

pwolanin’s picture

Version: 7.x-dev » 8.x-dev
FileSize
563 bytes

Looking at this patch, it seems like it should actually be HTTP_AUTHORIZATION, not HTTP_AUTH? In that case it looks as though PHP may correctly populate the other values before index.php is even loaded, which mitigates the concern about adding code to bootstrap.inc

pwolanin’s picture

FileSize
578 bytes

re-roll for fuzz

pwolanin’s picture

FileSize
634 bytes

minor tweak to comments and regex.

This works fine for me under fastCGI to populate HTTP_AUTHORIZATION, PHP_AUTH_USER, and PHP_AUTH_PW, which is the main use case (e.g. for OAuth or basic auth).

humansky’s picture

Status: Needs review » Reviewed & tested by the community

validated it works with FastCGI and the HTTP Authorization environment variables are correctly populated on the server

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Looks fine to me. Committed/pushed to 8.x.

This could probably be backported.

dcam’s picture

Status: Patch (to be ported) » Needs review

#14: 670454-14.patch queued for re-testing.

dcam’s picture

#14 applies to D7 for me. Re-testing.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Green for D7 as well.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.23 release notes
kalman.hosszu’s picture

Version: 7.x-dev » 8.x-dev
Status: Fixed » Needs review
FileSize
641 bytes

On Shield module (https://drupal.org/project/shield) we used the same .htaccess modification but an issue received because if the server uses Plain old CGI, we have to add another line to the .htaccess file. Here is the comment for easer understanding: https://drupal.org/node/1829694#comment-6963666

I attached a patch, please review it

roderik’s picture

FileSize
547 bytes

Just a suggestion for consistency, which I spotted when coming through here from Drupal Planet. (No credit please. I didn't even test.)

- The comment change reads to me like there would be 3 types of CGI. But there are 2. (CGI == Plain old CGI.)

In other words: I think the comment is good as-is. It is just incorrect, before the patch is applied.

- In #14, pwolanin changed .* to ^ , which is applied like that, so I guess we should be consistent with that.

pwolanin’s picture

I'm not sure that's right - seems like REMOTE_USER should be populated after authentication has happened, while the point of the patch here was to allow Drupal to do the authentication, e.g. for OAuth.

Can you point to the original issue or relevant CGI spec?

sun’s picture

Version: 8.x-dev » 6.x-dev
FileSize
1.29 KB

Support for "plain old CGI" can and should be a separate issue. That does not mean that your consideration is not valid; please open a new issue.

Let's proceed with the backporting here. Apparently, D6 is still missing.

pwolanin’s picture

This looks like a straight and simple backport - any reason not to commit it to 6?

sun’s picture

This appears to be an additional dependency for testbots to run on php-fpm:

#2157017: D7: DrupalHTTPRequestTestCase breaks on php-fpm [testbot PHP 5.4 blocker]

And of course, it also fixes inbound OAuth requests on fastcgi/cgi/php-fpm SAPIs in D6.

Any chance to move forward here? Patch hopefully still applies.

sun’s picture

Issue summary: View changes
Issue tags: +php-fpm, +phpfpm-testbot

Sorry, forgot to tag.

sun’s picture

Priority: Normal » Critical

Like the following two issues…

#2157053: Ensure register_shutdown_function() works with php-fpm (blocks testbot php-fpm)
#2157017: D7: DrupalHTTPRequestTestCase breaks on php-fpm [testbot PHP 5.4 blocker]

…this issue also has to be bumped to critical, since $_SERVER['HTTP_AUTHORIZATION'] will no longer be available to tests in contributed modules that are relying on it currently.

For example, all tests of modules that are accepting inbound OAuth Authorization headers will break. (I know of at least one whose tests will completely fail.)

sun’s picture

24: auth.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 24: auth.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
941 bytes

Identical patch as before, re-rolled due to new CHANGELOG.txt.

This change was originally introduced for D8 and backported into the stable D7 line (post 7.0). Attached patch only continues the backport process to D6 and is the exact same change that went into D8 and D7.

In addition, I double-checked that the added .htaccess lines of this patch are still identical in D8 and D7.

Therefore, I do not see any reason for why I shouldn't be able to RTBC this straight backport myself.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 31: auth.patch, failed testing.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.