drupal_render_cid_parts($granularity) in common.inc includes the following comment:
'PER_ROLE' and 'PER_USER' are mutually exclusive. 'PER_USER' can be a
resource drag for sites with many users, so when a module is being
equivocal, we favor the less expensive 'PER_ROLE' pattern.
If I have included PER_USER in $granularity (even if I have also specified other flags) then that means I don't want two users seeing each others content, and must accept any performance hit that implies. Instead, Drupal is currently silently assuming there's a bug in my module and instead of throwing an error it silently chooses an option that could reveal confidential data to less privileged users! This is therefore a security vulnerability, even if it isn't intentionally exploitable.
While I agree it is pointless to set both PER_ROLE and PER_USER, they are not actually mutually exclusive. I can't think of any reason you can't include both the role and user in the cache key, and the only reasoning given seems to be to avoid the performance implications of PER_USER.
I was initially going to suggest that if both PER_USER and PER_ROLE were set then we could cache based on the user only, but then I realised there is an edge case where users may not immediately gain/lose access to cached content when they are given a new role. Instead I think we must remove the else and the comment and will attach a patch that does this.
(I came across this bug when testing a patch for another issue, where I assumed that if ($granularity & DRUPAL_CACHE_PER_USER)
then I was safe to include user-specific information in the block. This is currently not the case.)
Comment | File | Size | Author |
---|---|---|---|
#9 | 1966346-9.patch | 1.53 KB | rpayanm |
#5 | 1966346-cache-per-user-5.patch | 1.54 KB | ianthomas_uk |
#1 | 1966346-cache-per-user-1.patch | 1.68 KB | ianthomas_uk |
Comments
Comment #1
ianthomas_ukHere is the patch. I've also expanded the documentation because while the comment for DRUPAL_CACHE_PER_USER mentions resource concerns, there is no mention of performance or resource usage at
http://api.drupal.org/api/drupal/core!includes!common.inc/group/block_ca...
Comment #2
ianthomas_ukI've just been having another look at this and reviewing the original issue where this was committed.
The first time the Per User + Per Role combination was mentioned was in a review by Dries:
https://drupal.org/node/80951#comment-441032
yched, the patch author, responded:
https://drupal.org/node/80951#comment-441038
The issue was not mentioned again that I could see.
To summarise:
- The current behaviour was chosen to protect module authors from accidentally slowing down their site.
- Instead, the same module authors may accidentally show potentially confidential information to the wrong users.
I couldn't find any instances of a module attempting to use both, the reason I want to get this fixed is to avoid introducing a similar issue in a fix for https://drupal.org/node/186636. Without a fix here I'll need to specifically work around this bug there.
Comment #3
ianthomas_uk1: 1966346-cache-per-user-1.patch queued for re-testing.
Comment #5
ianthomas_ukThis is a reroll of #1
Comment #6
jhedstromPatch no longer applies.
Comment #7
rpayanmI not see these functions on common.inc for reroll
Comment #8
ianthomas_ukCache tags means this is no longer relevant on D8
Comment #9
rpayanm