Dear Developers!
We are also developing a drupal module which is called shib_auth, Shibboleth Authentication. The Shibboleth is a Single Sign-On method, and the module have to create the new user accounts. We suppose the administrators to use userprotect to disable the possiblity of password change for the users, but this modules are slightly incompatible.
When the shib_auth module create the user and the 'Auto-protect new users' function is turned on, then the userprotect module try to find it out, if the user has enough rights to view a message. This check is causing more error messages/warnings.
Please apply my patch which is just an extension of your if statement to avoid this incompatibility in that case when not the administrator is who create the new user.
Thanks appreciate,
Ivan Marton
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | user_access_no_user.patch | 1.39 KB | hunmonk |
| userprotect-shibauth.patch | 1.08 KB | martoni |
Comments
Comment #1
hunmonk commentedi'm not clear why putting in a check for $user->roles is the right fix here -- can you elaborate on exactly what you're trying to accomplish?
Comment #2
martoni commentedOf course!
Well, in general when your module is called there is a user administrator or an anonymous user who create the new account. When the registration call the user hook and you decide whether to show or not the feedback message there is some role setting up for the user and everything works fine (at least the DRUPAL_ROLE_ANONYMOUS role). This is because your module do not generate any error.
Now, we have really different circumstances. Our module get the username from an apache module (some HTTP_AUTH like thing) and works so that it checks in the init(!) hook if there is such a user account or not and if not then create it. It is absolute necessary because the first page generation also have to behave according of the user rights. BUT in this phase of bootstrapping(?) we have absolutely no rights yet and the user_access() function fails because of the following SQL query:
SELECT p.perm FROM role r INNER JOIN permission p ON p.rid = r.rid WHERE r.rid IN ()Where the role id's should stand in the brackets.
The "$user->roles exist and not empty" checking statement should be there because it can prevent the unnecessary user_access calling. If a user is calling your function then this value is always true and the user_access can decide if the user has enough rights to see the message or not. If it's false then you can know that your functions are called from a module and the message mustn't be shown.
A short comment that the checking is just for decide to show a message or not so this modification absolutely cannot be a security risk.
Thank you appreciate,
Ivan Marton
Comment #3
hunmonk commentedmight it not be better to produce a temporary $user object in your module to avoid such an issue? after all, user_access() is a ubiquitous drupal function, so IMO it makes more sense to make sure that function call can work on your end then to have every module that uses it adjust it's code ;)
Comment #4
bajnokk commentedPlease let me rephrase the problem.
Shib_auth is a module which does automatic user creation based on HTTP environment (think of Basic auth, for example). What it does is calling user_external_login_register() on login and new account creation, which is the correct behaviour with Drupal 6, IMHO.
During the execution, user_external_login_register() sets global $user to FALSE if the user does not exist in the database. Later it calls user_save(), which calls hook_user() functions while global $user is still FALSE.
In this case your "check if the user is a user administrator" test (what the patch is referring) results in a database error message. What we propose is to extend the test with the following logic: if the user has no roles (as she doesn't exist), then she's definitely not an administrator.
It's not only shib_auth's problem: it is the case with every module calling core's user_external_login_register().
Comment #5
hunmonk commentedthen IMO the user_external_login_register() function should handle things in a way that doesn't disrupt other calls to common core functions. this problem should be fixed in core, so that the issue never even comes up for contrib modules.
it seems that the patch you're proposing for userprotect is a band-aid to a problem that should be solved at a deeper level, no?
Comment #6
bajnokk commentedWell, you implicitly assume that userprotect_user with 'insert' operation can only be called with an initialized $user, which is clearly not the case with user_external_login_register.
You might be right, but I think it's quite simple for you to stay on the safe side. You can still open an issue to the user core, though we need to have our users to patch userprotect manually until it gets fixed :/
Comment #7
hunmonk commented@bajnokk: i will not hack my module when IMO this needs to be fixed in core.
changing this to a core bug. attached patch gives consistent results to the user_access call in the case where there is no $user object. untested, but at least it gets the ball rolling :)
Comment #9
damien tournoud commentedThis bug was fixed by #165642: error in SQL syntax in user.module on line 368 (or 378) and has been released in 6.4.