There are situations in which user_access can be called before a valid global $user object has been set. In these cases user_access fails while trying to run the query on line 368 with the following message:

# warning: array_keys() [function.array-keys]: The first argument should be an array in /drupal/modules/user/user.module on line 368.
# warning: implode() [function.implode]: Bad arguments. in /drupal/modules/user/user.module on line 368.
# 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: SELECT DISTINCT(p.perm) FROM role r INNER JOIN permission p ON p.rid = r.rid WHERE r.rid IN () in /drupal/includes/database.mysql.inc on line 172

As an example, I have created an external authentication process using hook_auth. When this function returns true, user_authenticate attempts to user_load the user's information using their name. If the user does not exist, the global user gets set to false and user_authenticate creates an array of user information and calls user_save('', $userinfo), which attempts to add them as a new user. At this point the following function chain happens:

user_save -> user_module_invoke('insert') -> profile_user -> profile_save_profile -> _profile_get_fields -> user_access

user_access then fails while trying to create its sql query, because it makes the assumption that the global $user is actually a valid object, and thus that $user->roles is actually a valid array.

My thinking is that user_access should check if $user->roles is actually an array before trying to execute its query, and the attached patch solves the problem that way (causing user_access to just return FALSE in this situation, which seems right.) However, I am also just getting my feet wet with the whole user/authentication system, and its possible there's a better answer so I'm open to ideas and happy to roll a patch for whatever is best.

Drupal 5.2, PHP 5.2.x, MySQL 5.x

CommentFileSizeAuthor
user.invalid_object.patch786 bytesgdd
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gdd’s picture

Status: Active » Needs review

fixing status since I should have indicated patch included when I submitted

drumm’s picture

Status: Needs review » Needs work

Drupal core never specifically calls contributed modules.

drumm’s picture

Status: Needs work » Needs review

Sorry, wrong browser tab.

drumm’s picture

Version: 5.2 » 6.x-dev
Status: Needs review » Reviewed & tested by the community

Committed to 5.x.

Applies to HEAD with offset.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

What are the cases when user_access() is called, but a user is not set? What could be the side effects?

Dries’s picture

I had the same question as Gabor. Why would we want to support this?

gdd’s picture

Status: Needs review » Needs work

Yes, point well taken.

I was mostly thinking that from a simple defensive standpoint, it seemed to be sensible to have a function check its input and act accordingly if the input is bad. However I also understand that this has some other consequences, like that maybe if the $user object is bad we have a bigger problem and shouldn't be just passing on as if nothing happened. I feel like there needs to be a balance there, and that it should be something slightly more graceful than just dumping a PHP warning and moving on.

Then, of course, the main bug should be addressed IE stop the $user object from getting messed up in the first place.

I am out of town for several days but will come up with a more appropriate way of addressing this when I return.

Gábor Hojtsy’s picture

The problem is that different calls to user_access() in the same request might return contradictory values. This might affect caching and any other stuff which happens very early in the query, which could result in very nasty bugs. Therefore it would be nice to know what calls user_access() too early and look into eliminating that or moving it after the session was brought up.

drumm’s picture

Reverted the 5.x patch.

moshe weitzman’s picture

IMO this is a good change. We recently ran into this problem when a http auth module and devel were interacting badly. the http auth module was doing an unset($GLOBALS['user']) at end of the page, when user was logging out. tokenauth module does this too. then devel_exit() would come along and call user_access('access devel information') and we have a problem. why not build some defensiveness here? i don't think any module is misbehaving.

gdd’s picture

I actually think user_authenticate is misbehaving here wrt this specific instance as described in my bug report. However, I also think that's a separate issue. I'm going to open up another bug for that, I think I can get a patch together for it tomorrow.

As far as this issue, I still think user_access should check for data validity and not have PHP throw warnings when it gets something bad. Maybe a watchdog() entry is appropriate since user_access getting a bad $user object (or a good $user object with no roles, or whatever) is reasonably noteworthy.

Gábor Hojtsy’s picture

Why unset $user? Why not set it to the anonymous user? There are so many other functions which blindly global $user, and then except it to be there. Especially after the bootstrap is done.

moshe weitzman@drupal.org’s picture

Status: Needs work » Closed (works as designed)

that sounds like a good approach.