When ldap_integration is turned on and authentication fails the $user object is left set to null. This causes warnings and invalid SQL queries the next time user_access() is called.

This patch changes the hack added in ldap_integration_login_validate() to check for a NULL return value from the auth function before changing the $user global.

The original drupal code gets avoids doing this by having the auth function get the user global, potentially modify it, and then return it so the calling function can re-set the global to the same value. It's ugly... :)

CommentFileSizeAuthor
ldap_integration_null_user_fix.patch667 bytesjfitzell

Comments

rblomme@drupal.org’s picture

rblomme@drupal.org commented
Version: master » 4.7.x-1.x-dev

When authentication fails, I get the following messages:

    * Sorry. Unrecognized username or password. Have you forgotten your password?
    * warning: array_keys() [function.array-keys]: The first argument should be an array in /var/www/html/modules/user.module on line 349.
    * warning: implode() [function.implode]: Bad arguments. in /var/www/html/modules/user.module on line 349.
    * user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')' at line 1 query: user_access SELECT DISTINCT(p.perm) FROM role r INNER JOIN permission p ON p.rid = r.rid WHERE r.rid IN () in /var/www/html/includes/database.mysql.inc on line 120.

I didn't find a solution for this problem on the drupal.org site, so I started debugging...
I solved the problem (very similar to the posted solution / patch file).
I decided to browse the drupal.org bug list again, and I found this bug report!

By adding the error messages, I hope it will be easier for others to find their way to this bug report and patch.

pablobm’s picture

pablobm commented
Assigned: Unassigned » pablobm

Thanks for the tip. Actually, I think the real mistake for my part is to return null at _ldapauth_code_changed_login_validate, this way:

function _ldapauth_code_changed_login_validate($name, $pass) {
  $user = null;

  // ...etc...

Instead, this function should have a behaviour similar to that of user_authenticate, so the followine:

function _ldapauth_code_changed_login_validate($name, $pass) {
  global $user;

  // ...etc...

FYI, I'm refactoring the code these days, as it was growing too big. A new version with better looking code and many bugs solved will follow shortly.

pablobm’s picture

pablobm commented

Oh, crap, where it reads "so the followine" above, it should read "so the following code should replace that above".

jfitzell’s picture

jfitzell commented

Actually, I thought it was a poor design decision in the drupal code for user_authenticate() to modify that global directly (particularly since it also returns it). I thought your choice to return null was much better. But... either change will obviously solve the problem.

pablobm’s picture

pablobm commented

You are probably right there, but that's what Drupal expects, so that's the best decission from my part (I think).

Don't blame me, I'm just following an interface! ;)

Thanks again for pointing out. Now, I declare this issue: closed. A closed bug is a good bug! :D

johnbarclay’s picture

johnbarclay commented
Status: Needs review » Closed (won't fix)

Closing 4.7 issues to clean out issue queue.