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.
Comment | File | Size | Author |
---|---|---|---|
#31 | auth.patch | 941 bytes | sun |
#24 | auth.patch | 1.29 KB | sun |
#22 | 670454-22.patch | 547 bytes | roderik |
#21 | 670454-21.patch | 641 bytes | kalman.hosszu |
#14 | 670454-14.patch | 634 bytes | pwolanin |
Comments
Comment #1
cburschkaHere is my awesome patch of awesomeness. :)
Before:
After:
Comment #2
cburschkaTechnically, this is fixing a bug caused by the PHP environment.
Comment #3
cburschkaTo 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.
Comment #4
MichaelCole CreditAttribution: MichaelCole commented#1: bootstrap-http-auth-cgi-670454-1.patch queued for re-testing.
Comment #5
JacobSingh CreditAttribution: JacobSingh commentedThis patch looks good to me and works.
Comment #6
chx CreditAttribution: chx commentedwe 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.
Comment #7
cburschkaI'm sorry but I do not quite understand. Would you prefer the bootstrap code moved to a different phase?
Comment #8
JacobSingh CreditAttribution: JacobSingh commented@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
Comment #9
Gábor HojtsyGet other reviewers then?
Comment #10
pwolanin CreditAttribution: pwolanin commented@chx - if we want to be able to check authorization it seems like we do need these populated quite early in the request, right?
Comment #11
effulgentsia CreditAttribution: effulgentsia commentedNo comment on the merits of this patch. Just a reroll to chase HEAD.
Comment #12
pwolanin CreditAttribution: pwolanin commentedLooking 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
Comment #13
pwolanin CreditAttribution: pwolanin commentedre-roll for fuzz
Comment #14
pwolanin CreditAttribution: pwolanin commentedminor 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).
Comment #15
humansky CreditAttribution: humansky commentedvalidated it works with FastCGI and the HTTP Authorization environment variables are correctly populated on the server
Comment #16
catchLooks fine to me. Committed/pushed to 8.x.
This could probably be backported.
Comment #17
dcam CreditAttribution: dcam commented#14: 670454-14.patch queued for re-testing.
Comment #18
dcam CreditAttribution: dcam commented#14 applies to D7 for me. Re-testing.
Comment #19
moshe weitzman CreditAttribution: moshe weitzman commentedGreen for D7 as well.
Comment #20
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/7a036f1
Comment #21
kalman.hosszu CreditAttribution: kalman.hosszu commentedOn 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
Comment #22
roderikJust 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.
Comment #23
pwolanin CreditAttribution: pwolanin commentedI'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?
Comment #24
sunSupport 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.
Comment #25
pwolanin CreditAttribution: pwolanin commentedThis looks like a straight and simple backport - any reason not to commit it to 6?
Comment #26
sunThis 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.
Comment #27
sunSorry, forgot to tag.
Comment #28
sunLike 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.)
Comment #29
sun24: auth.patch queued for re-testing.
Comment #31
sunIdentical 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.