Problem/Motivation

Masquerade module uses role IDs to identify and store administrator roles. These admin roles are stored in variable masquerade_admin_roles. (The masquerade user roles are configured under Administer > Configuration > People > Masquerade.) The problem occurs when masquerade admin roles are exported to a feature and then enabled on another site. Role IDs may differ between different sites. Therefore, adding masquerade admin roles to a feature and deploying this feature on another site does not result in the same administrator roles settings.

Example:

SITE 1 has administer role settings:
[x] Manager (Role id =1)
[x] webmaster (Role id = 2)
[] editor (Role id = 3)
In this case 'masquerade_admin_roles' will be 

Array( 1 => 1 , 2 => 2, 3 => 0 )

On SITE 2, which does not have the same role IDs. Applying the feature will give the following administer role settings:
[x] Director (Role id =1)
[x] Manager (Role id =2)
[] editor (Role id = 3)
[] webmaster (Role id = 4)

Proposed solution

This can be fixed by storing the role names instead of role IDs. So masquerade_admin_roles will be: 

Array(0 => 'Manager', 1 => 'webmaster')

To reproduce:

  1. Create a new role with name 'test role'

    2. - go to admin/people/permissions/roles to create the new role

  2. Set masquerade administrators

    3. - go to admin/config/people/masquerade
    - select role 'test role' and click 'Save configuration'

  3. Simulate a site where roles are different

    4. - go back to admin/people/permissions/roles to change roles
    - edit role 'test role', rename it to 'test role (renamed)'
    - create a new role called 'test role'

  4. go back to masquerade admin interface admin/config/people/masquerade'

    5. - you should be able to see the issue: 'test role (renamed)' is selected instead of 'test role', the role which was initially configured as masquerade administrator

CommentFileSizeAuthor
#9 masquerade-admin-role-names-2784103-9-D7.patch8.38 KBkarolinam
#5 masquerade-admin-role-names-2784103-5-D7.patch7.1 KBkarolinam
#4 masquerade-admin-role-names-2784103-2-D7.patch3.93 KBlambic
#2 masquerade-admin-role-names-2784103-2-D7.patch3.72 KBkarolinam

Comments

karolinam created an issue. See original summary.

karolinam’s picture

karolinam
she/her
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new masquerade-admin-role-names-2784103-2-D7.patch3.72 KB

Here is a patch which implements the proposed solution.

fengtan’s picture

fengtan
Location Montreal, Canada
commented

Successfully tested the patch.
Featurizing role names instead of role ID's would be really useful.

lambic’s picture

lambic commented
StatusFileSize
new masquerade-admin-role-names-2784103-2-D7.patch3.93 KB

This patch wasn't working quite right, here is a fixed version.

karolinam’s picture

karolinam
she/her
commented
StatusFileSize
new masquerade-admin-role-names-2784103-5-D7.patch7.1 KB

Thanks for the fix. Reviewed and tested the patch. Works well.
I am adding a test to cover the case where it was failing.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Looks mostly ready, the only worry here is a role names could be changed (renamed, translated) but there's no hooks to react on rolename change in the patch

  1. +++ b/masquerade.install
@@ -181,3 +181,32 @@ function masquerade_update_7001() {
+  if (!empty($missing_ids)) {

    if ($missing_ids) should be enough 

  2. +++ b/masquerade.module
@@ -334,6 +334,14 @@ function masquerade_admin_settings_submit($form, &$form_state) {
+  foreach ($form_state['values']['masquerade_admin_roles'] as $id => $val) {
+    if ($val == (int) $id) {
+      $role_names[] = $form['masquerade_admin_roles']['#options'][$id];

    are sure that values here are not translated? 

  3. +++ b/masquerade.module
@@ -405,9 +413,10 @@ function masquerade_field_extra_fields() {
+  $admin_roles = masquerade_get_masquerade_admin_roles();
+  $user_roles = array_keys((array)$account->roles);
+  $perm = $account->uid == 1 || array_intersect($user_roles, $admin_roles) ?

    ++ here 

  4. +++ b/masquerade.module
@@ -434,6 +443,31 @@ function masquerade_user_view($account, $view_mode, $langcode) {
+  if (empty($roles)) {
+    return $role_ids;
+  }

    this is unneded here, please remove

karolinam’s picture

karolinam
she/her
commented

Thank you for your feedback. I will make these changes.
Side note: Your comment #1 and 4 are debatable but I'll make these changes if you prefer.

andypost’s picture

andypost
he/him
Primary language Russian
commented

They just need hooks to react on role changes, and I just remember core issue where role name could be translatable

karolinam’s picture

karolinam
she/her
commented
StatusFileSize
new masquerade-admin-role-names-2784103-9-D7.patch8.38 KB

Here is the patch with a hook for role changes and modification for translations.

izmeez’s picture

izmeez commented
Related issues: +#3010095: Masquerade 7.x-1.0 stable release plan

Adding patch in #9 to #3010095: Masquerade 7.x-1.0 stable release plan

ciss’s picture

ciss commented

I'd strongly advise against using role names as identifiers. Role names are supposed to be descriptive and can be changed at any given point. Additionally other modules (e.g. views) also rely on role IDs.

If you want to securely export anything role related, I highly recommend integrating the Role Export module into your setup, as it will use numerical hashes as role IDs.

It also seems that this patch would break existing features. At a minimum the change should be hidden behind a flag.

ciss’s picture

ciss commented
Status: Needs review » Needs work
ressa’s picture

ressa
he/him
commented
Status: Needs work » Closed (outdated)

Thanks for the suggestion. In an attempt to help the maintainers, I am going through some of the Drupal 7 issues, and closing them, since Drupal 7 is EOL.

Maintainers should grant credit for the great work to the relevant users, even if the patch did not get committed: https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett....

Since Drupal 7 is EOL, we can probably close this issue, but feel free to re-open if this is still relevant, and we should consider looking at this for the Drupal 11 version?

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.