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.
In my project, we needed to be able to supply HTTP authentication credentials to a target server when running tests. I added some simple configuration settings to support this, and am attaching patches for both simpletest.module and drupal_web_test_case.php.
Comment | File | Size | Author |
---|---|---|---|
#30 | simpletest_339210.patch | 2.52 KB | klausi |
#28 | simpletest_339210.patch | 2.53 KB | klausi |
#24 | simpletest_339210.patch | 2.28 KB | klausi |
#22 | simpletest_339210.patch | 2.77 KB | klausi |
#20 | simpletest_7_339210.patch | 2.74 KB | klausi |
Comments
Comment #1
fagohm, I'd need this feature too, so it would be nice if this setting would be supported again. Unfortunately I wasn't able to test your patch, as it has a strange format? have a look at http://drupal.org/patch/create
Comment #2
klausiSubscribing
Comment #3
fagothe patch looks good though, could you reroll it please?
Comment #4
klausiI had a look into it and tested the patch. After some troubles with open_basedir in my php.ini (I had to deactivate open_basedir) the patch finally works. Here is a new patch against the DRUPAL-6--2 branch. I omitted the debug stuff and only integrated the HTTP auth variables.
Comment #5
fagothanks.
I tested the patch and it's working fine. Furthermore it looks good -> setting RTBC.
Comment #6
boombatower CreditAttribution: boombatower commentedShould be done in Drupal 7 core then backported.
Comment #7
klausiHere we go again.
This is a reduced patch, not adding an additional admin interface (HTTP user and password can be specified in settings.php in the $conf array) ==> to get this committed faster ==> to get this backported to D6 faster ;-)
We can talk about the right place to show up the setting in the UI later.
Comment #9
klausiSorry, the patch above was not made from the drupal root directory. Shame on me. New patch from root directory attached.
Comment #10
jpetso CreditAttribution: jpetso commentedWorks well, conforms to coding standards and does exactly what's needed to make this work. (An additional admin interface can still be added in a follow-up patch, if desired.) I can see no reason why this patch shouldn't go in straight ahead.
Comment #11
Dries CreditAttribution: Dries commentedDon't we want to specify this on a per-test basis rather than using variable_get/set()? It sounds like these variables need to be parameter-ized instead of stored in the global variables table.
Comment #12
klausiI think that would be an additional feature that can be implemented later. For now it would be nice to have HTTP auth at all, important for people that do not want to have their testing servers publicly available (it was already possible in Simpletest 1.x in D5).
Comment #13
jpetso CreditAttribution: jpetso commentedI'm not sure if there isn't a communication issue here. As far as I can tell, HTTP authentication is commonly required for people who run a password-secured site and still want to run Simpletests on that site. Consequently, the HTTP authentication settings are a site-wide setting - "How can I allow SimpleTest to access my own site?" - and therefore don't need to be changed on a per-test basis.
There might be cases where the tests need to access pages outside the tested site, but *those* (and not the global default setting) should set up curl appropriately. For the common case, variable_get/set() makes perfect sense.
Comment #14
boombatower CreditAttribution: boombatower commentedI'm still curious why this is useful? SimpleTest is not intended to be run, much less installed, on production sites. So if it is on a development site shouldn't it be password free?
Comment #15
jpetso CreditAttribution: jpetso commented@boombatower:
1. No, in fact, all of our dev stuff is HTTPS and password protected.
2. Wouldn't you want to protect *especially* dev sites, which are much more prone to vulnerability and data leakage (with the data cloned from the production site)?
Comment #16
boombatower CreditAttribution: boombatower commentedThat's my question...where are the dev sites located? Internally behind a firewall and everything else? If so I don't see the point, but alas not my choice.
Comment #17
Dries CreditAttribution: Dries commentedjpetso, thanks for explaining the different scenarios. We should capture that knowledge in the patch. Please extend the code comments. Thanks!
Comment #18
jpetso CreditAttribution: jpetso commented@boombatower: A firewall has the disadvantage that it only works if everyone is behind the same network, which doesn't work so well for us because we're a more distributed team. HTTP(S) authentication works from everywhere, and is simple and effective. (Also mind that it wasn't my idea to set it up that way, and even disregarding the assumption that password-protected dev sites do make sense, if there are people out there using it that way then Drupal should enable them to run the tests. Especially if the Simpletest module already contains that logic but in a flawed fashion.)
klausi, motivated to reroll your patch once more with the comments that Dries suggested? :P
Comment #19
boombatower CreditAttribution: boombatower commented@jpetso: I am by no means against this patch. It is logical and adds back previous functionality. I was just curious if those involved had particular use cases since I do not personally. :)
I think the comments suggested by Dries are definitely a good idea, especially for those of us who do not have a setup that utilizes the functionality.
Comment #20
klausiOK, added some more comments.
Comment #21
boombatower CreditAttribution: boombatower commentedComments look good and this has already been reviewed.
Comment #22
klausiNew patch: Moved obtaining the HTTP auth variables to the method run(), because setUp() gets called more than once, and configuration variables from settings.php get lost in the meantime (during refreshVariables()).
EDIT: see also #362373: refreshVariables() loses variables from settings.php
Comment #23
Damien Tournoud CreditAttribution: Damien Tournoud commentedI suggest to change simpletest_(httpauth/httpauth_username/httpauth_pass) with just
simpletest_credentials
(=[user].[pass]
). I don't believe we need to explicitely set CURLOPT_HTTPAUTH for cURL to try to use an authentication.Something like this will probably be enough:
Comment #24
klausiRight, it is actually enough. New patch again, now http auth settings have to be specified differently in settings.php:
Comment #25
fagoI verified the new patch works fine, but I just realized that there was already code in there supporting http auth - so I changed this issue to a bug report.
However the (not ready) patch I submitted at #362373: refreshVariables() loses variables from settings.php made it work too with the existing code and the "old way" having two separate variables.
Comment #26
klausiYo, but this patch has three advantages:
1) efficiency: HTTP auth variables are only read once per test run from the configuration
2) independence of the configuration environment during the test run. Whether the configuration gets refreshed, cleaned, deleted or not, it does not influence the HTTP auth setting.
3) changeability: A simpletest can change the auth credentials easily by manipulating the class variable, maybe for some freaky HTTP auth setting tests
Comment #27
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis patch is nearly ready! Please just:
(1) define the $this->httpauth_credentials member variable in the class definition (defaulting to NULL)
(2) replace $this->httpauth_credentials = variable_get('simpletest_httpauth_credentials', '') by $this->httpauth_credentials = variable_get('simpletest_httpauth_credentials', NULL)
(3) replace if ($this->httpauth_credentials) by if (isset($this->httpauth_credentials))
This will make the code a little bit more robust, and more conform to our coding conventions.
Comment #28
klausiDone.
Comment #29
Damien Tournoud CreditAttribution: Damien Tournoud commentedThanks. Last pass (and I think we are done...):
(Nit-picking) Every comment needs to end with a proper period.
- "necessary" not "neccessary"
- "See the description of $curl_options for other options.
Comment #30
klausiNext try ;-)
Comment #31
fagoI've tested the updated patch, it's (still) working fine, so it fixes http auth for me. As far as I can tell the style looks also good now!
Furthermore I agree, the possibility to alter the httpauth_credentials easily is important to be there.
Comment #32
boombatower CreditAttribution: boombatower commented#31: works.
#29: fixed
Otherwise looks clean.
Comment #33
webchickRead through the patch and looks good; seems that Dries's comments were addressed.
Committed to HEAD. Thanks, folks!
Comment #35
klausiThis functionality should be available in Drupal 6.x, too.
Comment #36
klausiA simple backport is needed.
Comment #37
fagosee #467206: Backport http auth support
Comment #38
fagoclosing this in favour of the other issue.