Posted by Gribnif on November 1, 2011 at 9:10pm
7 followers
| Project: | Lightweight Directory Access Protocol |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | johnbarclay |
| Status: | closed (fixed) |
| Issue tags: | D7 stable release blocker |
Issue Summary
The current implementation of hook_boot() in ldap_authentication() calls drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL). This has unintented consequences:
1. It short-circuits an important optimization in Drupal, by loading code that may not ever be needed.
2. It interferes with other modules by asking Drupal to call their hook_boot()s at an unexpected time.
3. It interferes with other modules by calling their hook_boot() twice. That's because it's called within a loop in bootstrap_invoke_all(), which is then called again as part of the drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL).
Instead, I propose the attached patch. It loads on the needed code only when needed. It also cleans up a few minor coding standards issues.
| Attachment | Size |
|---|---|
| ldap_authentication_boot_2.patch | 2.29 KB |
Comments
#1
I like the direction, but don't like loading user and ldap before a full bootstrap. LDAP isn't designed for that, even though they may work. I think its better to not do any bootstrap and get $auth_conf->seamlessLogin directly from the database, or keep it in the variables table. The other code in common.inc for redirects can just be loaded without the bootstrap or done with a direct php header.
#2
#3
I added a check command line because the hook_boot broke my migration script.
if (!drupal_is_cli() && $GLOBALS['user']->uid == 0) {This still needs attention to get the
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL)out of it.
This is in head.
#4
I'm working on a patch to split the hook_boot part and some of the sso code into a separate ldap_sso module. I believe the craziness of the authentication include file needs to be in ldap authentication to avoid an excessive hook system. But some other code will be more readable and manageable in a separate module.
#5
#6
Here how I approached this:
- created ldap_sso module (enabling and disabling it enables sso). update code enables it if sso was enabled in ldap authentication
- moved easy to move code inlcuding hook_boot into ldap_sso.
- altered ldap_sso_boot() function to:
-- check for command line interface so doesn't break drush, aegir, etc.
-- created static getSaveableProperty function in ldapAuthenticationConfAdmin class so bootstrap and ldap module doesn't need to be loaded.
This is in head.
#7
Is this expected in the new ldap_sso module? Looks like something isn't loaded still in hook_boot?
Call to undefined function drupal_get_path() in /var/www/sites/all/modules/ldap/ldap_sso/ldap_sso.module on line 60
#8
no. this is a bug. the following code fixes it in ldap_sso.module
if (isset($_COOKIE['seamless_login_attempted']))$login_attempted = $_COOKIE['seamless_login_attempted'];
else {
$login_attempted = FALSE;
}
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
require_once('includes/common.inc');
require_once(drupal_get_path('module', 'ldap_authentication') . '/LdapAuthenticationConfAdmin.class.php');
#9
I committed the patch in #8, but it still needs work. A full bootstrap should not be needed to get the value of 1 variable without bootstrapping.
The following resolves this, but I don't believe will work correctly with features/strongarm because of the way it loads the variable.
require_once('includes/common.inc');$ldap_authentication_conf = variable_get('ldap_authentication_conf', array());
if ($ldap_authentication_conf['seamlessLogin'] == 1 && ($login_attempted != 'true')) {
setcookie("seamless_login_attempted", 'true', time() + (int)$auth_conf->cookieExpire, base_path(), "");
$_SESSION['seamless_login_attempted'] = $login_attempted;
drupal_goto('user/login/sso', array('query' => array('destination' => rawurlencode($_GET['q']))));
#10
In the recent dev release this is still pops up:
Call to undefined function _ldap_authentication_user_login_authenticate_validate() in /var/www/sites/all/modules/ldap/ldap_sso/ldap_sso.module on line 122
#11
Thanks for catching this. That call should be:
$user = ldap_authentication_user_login_authenticate_validate(array(), $fake_form_state);
this is in head. Sorry about the turbulence the last couple of commits. Any help on simpletests for ldap_sso would be appreciated.
I think its stable now. I'm going to stay out of the ldap_sso from now on since I don't have a good way to test it. At least its not bootstrapping when the ldap_sso module is disabled.
If anyone can cut down on the amount of bootstrapping needed in ldap_sso_boot() that would be great.
#12
#13
This should probably be:
require_once('includes/common.inc');require_once('includes/path.inc');
drupal_goto('user/login/sso', array('alias' => TRUE, 'query' => array('destination' => rawurlencode($_GET['q']))));Otherwise the second error mentioned in #1379506: SSO: ldap_sso_boot() multiple issues occurs again.
Edit: This error occurs because drupal_goto() calls url() which then calls drupal_get_path_alias(); which can't be found as path.inc isn't loaded.
This last function call can be avoided if we tell drupal_goto() that the URL is already an "alias".
#14
So can drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL); be removed with this change?
#15
Sorry. The edit I made to my above post makes it confusing.
With this change, you only need to include includes/common.inc rather than run a full bootstrap (as you did in #9).
The only change to your code in #9 would be:
drupal_goto('user/login/sso', array('alias' => TRUE, 'query' => array('destination' => rawurlencode($_GET['q']))));#16
John,
In regard to #11 ldap_authentication_user_login_authenticate_validate() does not return the $user object. This results in the wrong condition being met with:
<?phpif ($user && $user->uid > 0) {
?>
because user is being set to VOID.
To correct this ldap_authentication_user_login_authenticate_validate() in ldap_authentication.module needs to return the result of _ldap_authentication_user_login_authenticate_validate($form_state) around line 331.
<?phpfunction ldap_authentication_user_login_authenticate_validate($form, &$form_state) {
require_once('ldap_authentication.inc');
return _ldap_authentication_user_login_authenticate_validate($form_state);
}
?>
There are a couple other tweaks in ldap_sso.module that I will send over to fix a couple minor issues with drupal_goto() redirecting to broken pages. Patch coming soon.
#17
Here is a patch to fix a couple problems with ldap_sso.module. It corrects some indentation for readability and fixes the destination variable that gets messed up before a drupal_goto() causing you to get a page not found after logging in.
#18
Darn forgot status. One day I'll learn to preview.
#19
And just for good measure here's a patch for #16.
If you have other SSO or Active Directory issues you would like me to test let me know as I have an environment that requires both of these on a regular basis.
#20
#17 and #19 are worked into the new ldap_sso. I'm altering it a bit and adding unit tests. We should close this issue as its all over the place. For those of you with ldap_sso working the way you want, avoid the next release unless you want to help with the testing.
#21
Automatically closed -- issue fixed for 2 weeks with no activity.