error in SQL syntax in user.module on line 368 (or 378)

how are you doing the remote login? what module or system?

the error is because $account->roles is empty. it should not be empty here - it should be an array.

your remote login system should set roles for the account on first login before user_access is called.

the fact that it gets better on second login suggests that roles are set up after user_access is called.

i hope that helps you track it down. i think that's the best i can do without knowing more about your auth system.

I get this error. My set up is Drupal 5.2 and I am using LDAP Integration along with the Webserver_auth module. Webserver_auth gets REMOTE_USER from our authentication system (PubCookie) and is set to automatically create user if not previously found. So, the error occurs only once (but it would be nice to avoid it).

When you say "your remote login system should set roles for the account on first login before user_access is called." can you elaborate a bit? Sounds like I would want to tweak webserver_auth to set something like "authenticated user"???

Jim

I'm getting this one when someone uses their DO username on my site. - Straight core stuff, 5.5.

Looking at the code, I see that "user_login_validate" calls "user_authenticate" to check and load the $user array. While "user_login" and "user_login_submit" have 'global $user', "user_login_validate" does not. So whatever was loaded by "user_authenticate" will be lost for this session. "User_authenticate" does a 'user_save' so the data is available at the next login, which is what we are seeing.

Well, adding the global statement helped a little, but I still had to make a change in 'user_access' (and maybe that's all I needed)

  if (!isset($perm[$account->uid])) {
		// If no roles yet, use anonymous?
		if (empty($account->roles)) { $rids = array(1); }
    else { $rids = array_keys($account->roles); }

Okay, that took care of all the problem. I'm not sure it's a valid choice to go with 'anonymous', but it worked. If someone will tell me if that's right or I should go with 'authenticated' I will be happy to roll a proper patch.

This code is unchanged in D6 although the Drupal authentication module has been moved out of core. The problem may very well still be in D6.

Confirming this error

Nancy's fix works like a charm... Just tested it myself. I'd say roll a patch for 5 asap, and fix it in 6 before a patch is required...
But I have no say in that :)

Here's my patch.

Just out of curiosity, are any of the Drupal maintainers looking at this issue?

Doesn't apply to 6.

- There are also coding style errors in the latest patch. Conditional bodies should be on their own line.
- We should examine whether the user gets the proper roles later and/or why don't they get it by this time.

Posts crossed.

Here's a new version.

The latest D6 code is a bit different and may not encounter this problem.

If this bug is present in D6, you can get reviewers for that much more quickly, just to note.

Well the replacement for the drupal module was not available yet the last time I checked, so it's hard to try it.

nancyw: http://drupal.org/project/site_network is drupal.module moved to contrib (CVS checkout might work)

"There are no published releases for this project."

I did get a check out to work. Now I need to get a working D6 site to test it with...

+      $rids = array(1);

This should use the DRUPAL_ANONYMOUS_RID constant instead

Changed

I had a look at what was going on here, it seems that when an external user is loaded, the global $user is overwritten by the value "FALSE". user_access() assumes that the global $user is properly initialized and causes these errors.

Patching user_access() would just be trying to fix the symptoms rather than the underlying cause. $user should always be set, there are a lot more functions that rely on $user.

I traced the error back to user_authenticate(), that function uses a temporary variable with the name $user, overwriting the global user object with.

Patch attached.

Great. Thanks, Bart. Have you looked at the code in D6 to see if it is fixed there?

I haven't tested D6 because the drupal module is no longer in core and the distributed authentication hooks have been removed. The affected code from user.module has been removed as well, so its unlikely that this error still exists in D6.

Tested #23 and it works in Drupal 5.6 as expected, fixing the described issue. The logic seems sound, as well.

I'm running into this issue in 5.7 and 5.x-dev as well and this patch works for me.

Committed to 5.x.

Looks like somehow a comma ended up in the commit and D5-dev is now broken.

Fixed.

The patch in #23 / #29 still doesn't fix the problem completely.

By switching to $registered_user for the comparison, the $user variable isn't being populated if it actually finds an account that matches. So, yes the account is being created, but then it can't login because it's not being loaded.

You need to add this at the end of the if ($!registered_user->uid) {} block. These lines should start at line 1020.

        else {
          $user = $registered_user;
        }

This just showed up for the first time today in Drupal 5.7, PHP 4. I updated a few modules including FCKeditor today. Nancy's patch in #22 fixed it right up.

jdj25- I tested this with new and existing distributed login users provided by drupal.org. Everything seems fine and the user is logged-in. $user = user_save(...) on line 1016 does seem to take care of user loading.

Please provide more details for reproducing any problems.

I ran into this problem with D6 with the user_external_login_register function (which now seems to have the code that was in user_authenticate for D5). I mirrored the patch in #23 and it now seems to work for me. Patch attached.

Ok, reassigning to D7, which has exactly the same code.

user_external_login_register() is not called anywhere in Drupal Core, how to you use it and what are the steps to reproduce this bug?

Note: patch in #35 is wrong: user_external_login_register() is meant to login or register. The patch makes it only login new users, not login already registered users.

Right, the patch needs the "else" clause added. My mistake, which I also just rediscovered.

The steps to reproduce:
1. use form_alter for user_login and user_login_block to replace user_login_authenticate_validate with my own validate that presents username/password to an external service:

    $key = array_search ('user_login_authenticate_validate', $form['#validate']);
    $form['#validate'][$key] = 'chockstone_authenticate_validate';

2. if the credentials validate, call user_external_login_register with the $name and module name

    user_external_login_register ($name, 'chockstone');

Since there's steps to reproduce now, we can move this back to CNR.

The last patch still has coding style issues.

Since this is not something we test regularly, I'd definitely like to see a test case for this written as well.

Switching back to 'code needs work'.

• #226547: SQL and other errors on external authentication if profile module is enabled
• #281465: Profie module prints out php errors on the creation of external user

were duplicates.

Here is a slightly modified patch.

The issue is exactly the same as the one from D5: user_external_login_register() messes with the global $user object if an existing user is not found.

About tests: we have no consumer of that function in Drupal Core since the Drupal module is gone for good. I see no point in making a regression tests for this: I consider this legacy material, and we should drop it as soon as possible...

Because the fix is trivial and is known to solve the problem, I'm marking this as RTBC. Dries, please consider.

Subscribe.

Subscribe.

I've committed this to CVS HEAD, should probably go into Drupal 6.

Same patch for D6.

subscribe

Committed to DRUPAL-6.

Great to see this fixed in 6 but I am seeing the same problem in 5.9

http://drupal.org/node/181448 seems to be a duplicate of this issue (but still active) - can anyone else confirm this is still an issue in 5.x?

#165642: error in SQL syntax in user.module on line 368 (or 378) was a duplicate. This has been fixed since 5.8, please open a new ticket if you are still having some trouble, but I fell you are facing a different issue.

I'm sorry for the ignorence, but I have no idea how do you use a patch file?

I would really appriciate explanation.

Btw, my problem is: I cannot log in with my password, and "request new password" doesn't work, I hope I wrote in the correct forum.

This bug is still active in the latest version of the Drupal 5 (5.15).

If somebody use user_authenticate() function, and they have a hook_auth() that is returns TRUE, the user_authenticate() function bounce back an anonymous user.

The problem is in the user.module user_authenticate() function:

  // Try each external authentication source in series. Register user if
  // successful.
  else {
    foreach (module_implements('auth') as $module) {
      if (module_invoke($module, 'auth', $name, $pass, $server)) {
        $registered_user = user_load(array('name' => $fullname));
        if (!$registered_user->uid) { // Register this new user.
          $userinfo = array(
            'name' => $fullname,
            'pass' => user_password(), 
            'init' => $fullname,
            'status' => 1, 
            'access' => time(),
          );
          $userinfo["authname_$module"] = $fullname;
          $user = user_save('', $userinfo);
          watchdog('user', t('New external user: %user using module %module.', array('%user' => $fullname, '%module' => $module)), WATCHDOG_NOTICE, l(t('edit'), 'user/'. $user->uid .'/edit'));
          break;
        }
      }
    }
  }
  return $user;

The $registered_user never become $user, if the $registered_user->uid is exist.

Possible solution:

        else {
          $user = $registered_user;
        }

I'm still missing encountering missing roles in Drupal 6 after a call to user_external_login_register and user_save.

This bug is still alive in the current stable version of drupal 5 - 5.19.

As suggested by foripepe - January 27, 2009 - 16:38, the only thing missing is an else clause for if (!$registered_user->uid) { // Register this new user.

Cheers,
Tulio Salvaro

This looks correct. If the user is not authenticated by the time we reach the "register the new user" part, we are not supposed to authenticate it using an existing user account.

The patch won't work in instances where $account->roles isn't an array (which is one of the causes of the error messages). In the 2nd, 3rd and 4th call to user_access on my site, var_dump of $user and $account produced the following:

user_access() $execution_count: 2

$user:
bool(false)

$account:
NULL

user_access() $execution_count: 3

$user:
bool(false)

$account:
NULL

user_access() $execution_count: 4

$user:
bool(false)

$account:
NULL

so it seems like it would be necessary to either:
1) not run the query unless $account->roles is verified to be an array

  if (!isset($perm[$account->uid]) && isset($account->roles)) {
    $result = db_query("SELECT p.perm FROM {role} r INNER JOIN {permission} p ON p.rid = r.rid WHERE r.rid IN (". db_placeholders($account->roles) .")", array_keys($account->roles));

or 2) set $account->roles to anonymous in the event that it's not set before running the query:

  if (!isset($perm[$account->uid])) {
    if (!isset($account->roles)) {
      $account->roles[1] = 'anonymous user';
    }
    $result = db_query("SELECT p.perm FROM {role} r INNER JOIN {permission} p ON p.rid = r.rid WHERE r.rid IN (". db_placeholders($account->roles) .")", array_keys($account->roles));

Not sure which (if either) is preferable though. Thoughts?