When an external user auth module gives a thumbs-up to a new user, user_external_login_register() finishes the job of creating the user by calling user_save() after initializing some values in $userinfo.
Because of this change, we have a problem...that commit added a call to user_access() inside user_save(). Well, we can't call user_access() because we don't actually have a finished user object to hand it -- we're still in the process of creating it.
But note that user_access() is only called if the 'access' key is not set. So to avoid this problem, we simply add the access key to those that are set while creating the $userinfo array. Now the conditional before the user_access() code successfully prevents the bogus call to user_access() from happening, because the user_access() call is only made if 'access' is empty.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | external_auth-5.x_1.patch | 892 bytes | stevecrozz |
| #6 | external_auth-5.x.patch | 1.11 KB | joshk |
| extauthaccessjv.patch | 1.23 KB | jvandyk |
Comments
Comment #1
jvandyk commentedComment #2
joshk commented+1
This is also an issue in 5.x. Backport patch:
http://drupal.org/node/213417
Comment #3
moshe weitzman commentedfixes the bug and improves readability a bit.
Comment #4
gábor hojtsyWell, setting the access time on a user externally logging in is logical on its own right as well, so committed. Thanks.
Comment #5
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #6
joshk commentedAs per neil's instruction, here's that patch again.
Comment #7
stevecrozz commentedI tested this patch today and it fixed a problem I encountered while writing my own authentication module which I describe in this issue: http://drupal.org/node/237327
I like this patch better than mine, and I will leave a comment to that effect on my own issue. I haven't noticed any problems with the patch yet.
Comment #8
drummThis is not a direct backport of the patch, which only added the 'access' element. With this patch, $userinfo["authname_$module"] is set unconditionally, instead of only when $server evaluates to TRUE. If this was intentional, please explain and set back to reviewed & tested by the community.
Comment #9
stevecrozz commentedI think there is some confusion here, as drumm said the original patch only adds the 'access' element, but that's because the original patch already sets "authname_$module" regardless of the state of $server. Joshk's patch (probably mistakenly) includes an additional change to match the code in the 6.x branch. Since that change is not part of this bug I've taken it out and I'm supplying a new patch that is a direct backport of only the modified lines from the original patch.
I've also tested the patch myself in the first iteration and now this second iteration, so I'm changing the patch status again to reviewed & tested.
Comment #10
damien tournoud commentedThis is a direct backport from 6.x. Should be ready to go in.
Comment #11
drummManually applied and committed to 5.x.
Comment #12
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.