| Project: | OG User Roles |
| Version: | 6.x-4.1 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
It seems that the implementation for og_users_roles_grant_roles() is a bit dangerous. The problem seems to be that if the static cache of granted permissions is set, that the function makes those (and only those) the user's roles, instead of merging them in, thus causing any additionally granted roles (by other modules) to vanish.
else {
// Grant all stored roles.
$account->roles = $roles;
}I ran some debugging and outputted the user's roles before this function is called and after. The following is for a single page load. Pay attention to how the role "theme full" vanished at the very end. That role is applied by another module depending on a certain site condition.
before --> authenticated user, official user
after --> authenticated user, official user, group member
before --> authenticated user, official user
after --> authenticated user, official user, group member
before --> authenticated user, official user
after --> authenticated user, official user, group member
before --> authenticated user, official user, group member, theme full
after --> authenticated user, official user, group memberIt also seems like the static cache of roles should be keyed by user and group id, just to be safe, and make sure we're giving the correct user the correct roles.
Patch coming soon, to handle the cache better, and merge in roles, rather than force-set them.
Comments
#1
Also probably a good idea to change the initial sanity check to:
if (!is_object($group_node)) {return array();
}
To avoid a ton of unnecessary calls..
#2
Patch attached
#3
This looks good to me.
Wanted to run tests to confirm existing functionality, but apparently some unknown change must have introduced a critical regression, so that needs to be fixed first; see #1360940: Tests are failing