We are testing distributed authentication of two drupal sites (different databases) on the same server. It works but we got an error on the very first remote logon:
* warning: array_keys(): The first argument should be an array in /srv/www/mcnix/drupal/modules/user/user.module on line 368.
* warning: implode(): Bad arguments. in /srv/www/mcnix/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 /srv/www/mcnix/drupal/includes/database.mysql.inc on line 172.
I can't sort that one out. It comes on the first login. Vanished on the second one.
Comment | File | Size | Author |
---|---|---|---|
#45 | 165642-user-login-register-d6.patch | 2.09 KB | Damien Tournoud |
#41 | 165642-user-login-register.patch | 716 bytes | Damien Tournoud |
#37 | 20080716-2.user-external-login-register.patch | 741 bytes | jtkeith |
#35 | 20080716.user-external-login-register.patch | 455 bytes | jtkeith |
#29 | user-module-comma.patch | 803 bytes | Bart Jansens |
Comments
Comment #1
firebus CreditAttribution: firebus commentedhow 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.
Comment #2
geste CreditAttribution: geste commentedI 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
Comment #3
NancyDruI'm getting this one when someone uses their DO username on my site. - Straight core stuff, 5.5.
Comment #4
NancyDruLooking 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)
Comment #5
NancyDruOkay, 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.
Comment #6
NancyDruThis 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.
Comment #7
meba CreditAttribution: meba commentedConfirming this error
Comment #8
Swift Arrow CreditAttribution: Swift Arrow commentedNancy'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 :)
Comment #9
NancyDruHere's my patch.
Comment #10
NancyDruJust out of curiosity, are any of the Drupal maintainers looking at this issue?
Comment #11
meba CreditAttribution: meba commentedComment #12
catchDoesn't apply to 6.
Comment #13
Gábor Hojtsy- 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.
Comment #14
NancyDruPosts crossed.
Comment #15
NancyDruHere's a new version.
The latest D6 code is a bit different and may not encounter this problem.
Comment #16
Gábor HojtsyIf this bug is present in D6, you can get reviewers for that much more quickly, just to note.
Comment #17
NancyDruWell the replacement for the drupal module was not available yet the last time I checked, so it's hard to try it.
Comment #18
Gábor Hojtsynancyw: http://drupal.org/project/site_network is drupal.module moved to contrib (CVS checkout might work)
Comment #19
NancyDru"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...
Comment #20
NancyDruComment #21
Bart Jansens CreditAttribution: Bart Jansens commentedThis should use the DRUPAL_ANONYMOUS_RID constant instead
Comment #22
NancyDruChanged
Comment #23
Bart Jansens CreditAttribution: Bart Jansens commentedI 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.
Comment #24
NancyDruGreat. Thanks, Bart. Have you looked at the code in D6 to see if it is fixed there?
Comment #25
Bart Jansens CreditAttribution: Bart Jansens commentedI 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.
Comment #26
Junyor CreditAttribution: Junyor commentedTested #23 and it works in Drupal 5.6 as expected, fixing the described issue. The logic seems sound, as well.
Comment #27
jsenich CreditAttribution: jsenich commentedI'm running into this issue in 5.7 and 5.x-dev as well and this patch works for me.
Comment #28
drummCommitted to 5.x.
Comment #29
Bart Jansens CreditAttribution: Bart Jansens commentedLooks like somehow a comma ended up in the commit and D5-dev is now broken.
Comment #30
drummFixed.
Comment #31
jdj25 CreditAttribution: jdj25 commentedThe 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.Comment #32
JHeffner CreditAttribution: JHeffner commentedThis 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.
Comment #33
drummjdj25- 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.
Comment #34
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #35
jtkeith CreditAttribution: jtkeith commentedI 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.
Comment #36
Damien Tournoud CreditAttribution: Damien Tournoud commentedOk, 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.Comment #37
jtkeith CreditAttribution: jtkeith commentedRight, 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:
2. if the credentials validate, call user_external_login_register with the $name and module name
Comment #38
catchSince there's steps to reproduce now, we can move this back to CNR.
Comment #39
Dries CreditAttribution: Dries commentedThe 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'.
Comment #40
Damien Tournoud CreditAttribution: Damien Tournoud commentedwere duplicates.
Comment #41
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere 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.
Comment #42
Paul Natsuo Kishimoto CreditAttribution: Paul Natsuo Kishimoto commentedSubscribe.
Comment #43
Richard Blackborder CreditAttribution: Richard Blackborder commentedSubscribe.
Comment #44
Dries CreditAttribution: Dries commentedI've committed this to CVS HEAD, should probably go into Drupal 6.
Comment #45
Damien Tournoud CreditAttribution: Damien Tournoud commentedSame patch for D6.
Comment #46
agerson CreditAttribution: agerson commentedsubscribe
Comment #47
Dries CreditAttribution: Dries commentedCommitted to DRUPAL-6.
Comment #48
cridenour CreditAttribution: cridenour commentedGreat 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?
Comment #49
Damien Tournoud CreditAttribution: Damien Tournoud commented#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.
Comment #50
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #51
nlobak CreditAttribution: nlobak commentedI'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.
Comment #52
foripepe CreditAttribution: foripepe commentedThis 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:
The $registered_user never become $user, if the $registered_user->uid is exist.
Possible solution:
Comment #53
smithmb CreditAttribution: smithmb commentedI'm still missing encountering missing roles in Drupal 6 after a call to
user_external_login_register
anduser_save
.Comment #54
tuliosalvaro CreditAttribution: tuliosalvaro commentedThis 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
Comment #55
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis 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.
Comment #56
James Marks CreditAttribution: James Marks commentedThe 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:
so it seems like it would be necessary to either:
1) not run the query unless $account->roles is verified to be an array
or 2) set $account->roles to anonymous in the event that it's not set before running the query:
Not sure which (if either) is preferable though. Thoughts?