If you assign a role to a user (e. g. in code) and during the same request check if a user has a permission which is granted by only this role and no other role the user has, user_access() returns false.

Example

  • User 2 has no role
  • Role Editor contains permission edit custom content
  • Role Editor is assigned to user 2
  • During the same request a rules event is triggered and the rule action calls user_access('edit custom content');. This returns FALSE.

Problem

The problem is that user_access() caches all permissions checked for all users during a request using the drupal_static() pattern. This means in the following requests the result is correct but during the same request not as the changed permissions no not get reloaded.

Security improvements

I don't see this as a security issue but it is an improvement. Because if you do it the other way and unassign a role from a user and no other role gives the same permission to the user, user_access() still would return true during the same request even if the user doesn't have the requested permission anymore.

Solution

Add an optional paramater $reset_cache = FALSE to user_access() and change the

if (!isset($perm[$account->uid]))

into

if (!isset($perm[$account->uid]) || $reset_cache).

This gives the calling code the possibility to reload the permissions if needed.

CommentFileSizeAuthor
#3 2787041-3.patch867 bytesShashwat Purav
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mvonfrie created an issue. See original summary.

Shashwat Purav’s picture

Shashwat Purav CreditAttribution: Shashwat Purav at Iksula commented
Assigned: Unassigned » Shashwat Purav
Shashwat Purav’s picture

Shashwat Purav CreditAttribution: Shashwat Purav at Iksula commented
Assigned: Shashwat Purav » Unassigned
Status: Active » Needs review
FileSize
2787041-3.patch867 bytes

Added patch.

mvonfrie’s picture

mvonfrie CreditAttribution: mvonfrie commented

Thanks it works.

mvonfrie’s picture

mvonfrie CreditAttribution: mvonfrie commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

The example ist from a real project and could be solved by applying patch #3 and calling user_access('permission', $user, TRUE).

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

Nice improvement, thanks!

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

Nice improvement, thanks!

chx’s picture

chx CreditAttribution: chx at Smartsheet, Migrate Rocks commented
Status: Reviewed & tested by the community » Needs work

> The problem is that user_access() caches all permissions checked for all users during a request using the drupal_static() pattern. This means in the following requests the result is correct but during the same request not as the changed permissions no not get reloaded.

I'd factor out the two drupal_static calls from user_role_grant/revoke_permission and a call to that new function to user_save under the section // Reload user roles if provided.

mvonfrie’s picture

mvonfrie CreditAttribution: mvonfrie commented

@chx: Where exactly would you call drupal_static_reset()?

To be clear, I was not talking about changing the permissions a role has but assigning a role to a user, thus using user_multiple_role_edit(). But it really doesn't matter which action (or function called) changes the concrete permissions a user has, resetting them should happen in user_access() only as it should be treated as a black box even from user_save(), user_multiple_role_edit(), user_role{grant/revoke}_permissions() etc.

What if for some reason in future user_access() changes the way it caches permissions from one variable for all to one variable per user. Then the cache key changes to something like __FUNCTION__ . ':' . $account->uid. If then another function resets the user permission cache after changing some permissions of a user (either by changing the user's roles or by changing any role's permissions) that functions could not react on it as all of them must be changed accordingly.

That's one point. The other one is if we only need to reload the permissions of the user in request or of all users. That can be discussed and thinking about changing permissions of a role that would make sense. But only if it has direct affect to any user during the same request.

chx’s picture

chx CreditAttribution: chx at Smartsheet, Migrate Rocks commented

Sorry, I edited #8it should be more clear where to call that reset: user_save is the only place where a user entity is changed. (Or should be.)