Pre-auth role permissions are wrong with another module calls user_access before logintoboggan_init runs

scottgifford - June 19, 2009 - 21:52
Project:LoginToboggan
Version:5.x-1.5
Component:Code
Category:bug report
Priority:normal
Assigned:Unassigned
Status:won't fix
Description

I ran into a weird problem today where, after installing a seemingly unrelated module, pre-auth users were able to send invitations with the Invite module.

It turns out the problem was the other module implemented a hook_init which called user_access. This module's hook_init ran before LT's, so when it called user_access the authorized role hadn't yet been removed from the user's roles. user_access built the permissions based on the user being authorized, then cached them. Later, when the Invite module called user_access, the cached permissions indicated the user could send invitations.

The solution may be to assign a low weight to LT when it is installed. I was able to fix this by manually adjusting the module weights, so the problem module was heavier than LT.

#1

jrglasgow - September 16, 2009 - 13:12

I ran into a similar problem with the 6.x branch, this one was with comment permissions. We didn't want pre-auth userss to post comments, but they were allowed to. I moved the LoginToboggan weight to -2 and it was fixed.

Here are some patches... the 6.x patch should work on both 6.x-1.x and 6.x-2.x

AttachmentSize
logintoboggan-5.x-496814.patch 1.16 KB
logintoboggan-6.x-496814.patch 1.18 KB

#2

jrglasgow - September 16, 2009 - 13:13
Status:active» needs review

#3

hunmonk - September 17, 2009 - 12:47

the problem is, the current hook ordering system is poorly equipped to handle dependencies like this. unless there are two modules that are *always* installed and run together, it seems like a cat chasing it's tail to try and fix this via 'official' module weighting.

in this case, i think it's better to just make site admins aware that they may need to adjust module weights for their install. not ideal, i know, but really the best approach given the current limitations.

i'd be more in favor of a patch that adds some documentation to the README or INSTALL files letting site admins know of this possible situation.

#4

scottgifford - September 19, 2009 - 04:43

I think adjusting the weight in the module is a good idea. I found this effect of LT very surprising, and in it can certainly create a security problem; imagine if on my site, a spammer had realized he could send spam from an unconfirmed account, and had blasted out thousands of mails before I realized what was going on.

A note in the README is very easy to miss, especially if at the time LT is installed this problem isn't an issue, but installing a module later triggers this problem. If the solution is a documentation change, I would recommend it be placed in the UI near the option to enable an unconfirmed user, since the consequences of missing it are so dire.

A module weight of -2 seems likely to solve the problem for basically all users. Yes, module weights are a bit of a hack in Drupal, but that's not a good reason to ignore them altogether when an easy fix presents itself.

#5

hunmonk - September 19, 2009 - 13:32

@jrglasgow: i don't see how you can be running into the exact same issue as scottgifford if you're running the latest 6.x release. if you examine _logintoboggan_user_roles_alter(), which is called in logintoboggan_init(), you'll see that the permissions cache is reset by LT.

@scottgifford: which means that your problem could be solved by upgrading to 6.x :)

let's cover the issue with 5.x, since user_access() doesn't allow the hack we use in 6.x...

A module weight of -2 seems likely to solve the problem for basically all users. Yes, module weights are a bit of a hack in Drupal, but that's not a good reason to ignore them altogether when an easy fix presents itself.

yes, it's an easy fix for the problem you describe when considered with the other contrib module you're using. however, that's only one piece of the weighting mess. there's also:

  1. other contribs using a hook init that would set their module weight less than (or possibly equal to) -2. there are already > 4000 contribs around -- a quick grep shows ~635 modules that implement hook_init(). i have no idea which of those might fall into that category. anybody interested in reviewing the hook_init() implementations of 635 modules, please speak up now... ;) otherwise, we're simply guessing that a weight of -2 is the best fix. and, while we're on the subject of another modules hook_init() messing up LT's functionality, there's also the reverse problem: if a module runs hook_init() *after* LT, and it reloads the global user object, we're right back in the same mess. this means that moving LT's weight lower would make that problem worse.
  2. the fact that *every* hook now runs with the new weighting, not just hook_init(). LT of course uses other hooks, and the global reweight could very well bust other things. i don't really think it's a good idea to introduce that kind of possible instability into stable branches of the module. there would need to be extensive testing in order to ensure this problem didn't bite us. any volunteers here?

considering the above problems, and the workload involved, and the fact the 5.x won't live all that much longer, and the fact that there's a workaround for those few people that actually experience this issue, i'm not very convinced we should do reweighting.

#6

hunmonk - October 9, 2009 - 00:31
Status:needs review» won't fix

given my reasoning in #5, and the fact that i haven't heard anything in awhile, i've decided that this issue isn't worth addressing in 5.x.

#7

scottgifford - October 12, 2009 - 16:08

@hunmonk, yes, I planned to post straightforward solutions to the problems you mentioned above, but as I dug around in the code no obvious solutions presented themselves. I will live with my workaround for now. Thanks!

 
 

Drupal is a registered trademark of Dries Buytaert.