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.)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ianthomas_uk’s picture

Status: Active » Needs review
FileSize
1.68 KB

Here 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...

ianthomas_uk’s picture

I'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

I'm not sure I agree with [per role overridding per user]. If the block can change on a per-user basis, we should prefer that. If a block is personalized on a per-user basis, we should never cache it on a per-role basis. If you think I'm mistaken, I'd like to see the explanation in the code to be expanded.

yched, the patch author, responded:

https://drupal.org/node/80951#comment-441038

well, setting both flags for a given block is basically broken code, and reveals the module author does not fully get the notion of caching patterns. In that case, I thought it best to 'babysit' by picking the harmless 'PER_ROLE'. 'PER_USER' can by a resource drag, so "If you do need PER_USER, say so, but don't be equivocal...". I did not change that part for now, and improved code comments. Let me know if you do want PER_USER to win :-)

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.

ianthomas_uk’s picture

1: 1966346-cache-per-user-1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: 1966346-cache-per-user-1.patch, failed testing.

ianthomas_uk’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.54 KB

This is a reroll of #1

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

rpayanm’s picture

I not see these functions on common.inc for reroll

ianthomas_uk’s picture

Version: 8.0.x-dev » 7.x-dev

Cache tags means this is no longer relevant on D8

rpayanm’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.53 KB

Status: Needs review » Needs work

The last submitted patch, 9: 1966346-9.patch, failed testing.